From owner-freebsd-arm@freebsd.org Mon Jul 20 21:44:17 2015 Return-Path: Delivered-To: freebsd-arm@mailman.ysv.freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:1900:2254:206a::19:1]) by mailman.ysv.freebsd.org (Postfix) with ESMTP id 3A1AA9A6DA2 for ; Mon, 20 Jul 2015 21:44:17 +0000 (UTC) (envelope-from daemon-user@freebsd.org) Received: from phabric-backend.isc.freebsd.org (phabric-backend.isc.freebsd.org [IPv6:2001:4f8:3:ffe0:406a:0:50:2]) by mx1.freebsd.org (Postfix) with ESMTP id 23AD219AE for ; Mon, 20 Jul 2015 21:44:17 +0000 (UTC) (envelope-from daemon-user@freebsd.org) Received: by phabric-backend.isc.freebsd.org (Postfix, from userid 1346) id 1AF6AFE27; Mon, 20 Jul 2015 21:44:17 +0000 (UTC) Date: Mon, 20 Jul 2015 21:44:17 +0000 To: freebsd-arm@freebsd.org From: "jhb (John Baldwin)" Reply-to: D3009+327+e1fd0ca814329cfb@FreeBSD.org Subject: [Differential] [Updated] D3009: Add MSI-x support to AHCI driver Message-ID: X-Priority: 3 Thread-Topic: D3009: Add MSI-x support to AHCI driver X-Herald-Rules: <28> X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-To: X-Phabricator-Cc: X-Phabricator-Cc: In-Reply-To: References: Thread-Index: ODRjZmU3ZjM0YmRkYWRjYjdmNDlhOTk2NzE1IFWta7E= Precedence: bulk X-Phabricator-Sent-This-Message: Yes X-Mail-Transport-Agent: MetaMTA X-Auto-Response-Suppress: All X-Phabricator-Mail-Tags: , MIME-Version: 1.0 Content-Transfer-Encoding: 8bit Content-Type: text/plain; charset="utf-8" X-BeenThere: freebsd-arm@freebsd.org X-Mailman-Version: 2.1.20 List-Id: "Porting FreeBSD to ARM processors." List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 20 Jul 2015 21:44:17 -0000 jhb added a comment. In general I think this looks fine. This is the first driver that doesn't "know" where it's PBA and table are. We could certainly add little accessor functions to return the BAR values cached in the dinfo. Also, I think you need to explicitly handle the case that the BAR for the PBA and/or table might match the 'r_mem' BAR? Right now if that is true I think you fail to attach? (Someday I will fix pci_alloc_msix() to reserve the PBA and table BARs, but that wasn't very feasible back when MSI was first added. It is somewhat less painful now that "reserved" resources exist as a real thing and not a hack of PCI bus attach.) INLINE COMMENTS sys/dev/ahci/ahci_pci.c:498 I would not set ctlr->numirqs here. You don't know which variant you are going to use. As it is you will now pass the MSI-X count to pci_alloc_msi() if pci_alloc_msix() fails which might cause it to fail (if it has more than 1 MSI-X message, but only 1 MSI message for example). Instead, I would do something like this: if (msi_count == 0 && msix_count == 0) ctlr->msi = 0; if (ctlr->msi < 0) ctlr->msi = 0; else if (ctlr->msi == 1) { /* Only use a single message if present. */ msi_count = min(1, msi_count); msix_count = min(1, msix_count); } else if (ctlr->msi > 1) { ctlr->msi = 2; /* Allocate MSI/MSI-X messages. */ if (ctlr->msi > 0) { error = ENXIO; if (msix_count > 0) { error = pci_alloc_msix(dev, &msix_count); if (error == 0) ctlr->numirqs = msix_count; } if ((error != 0) && (msi_count > 0)) { error = pci_alloc_msi(dev, &msi_count); if (error == 0) ctlr->numirqs = msi_count; } if (error != 0) ctlr->msi = 0; } sys/dev/ahci/ahci_pci.c:502 Maybe use ENXIO instead of -1 for readability since the values here are errno values? sys/dev/ahci/ahci_pci.c:522 I think just ctlr->msi > 0 is sufficient. If there are no messages present or they fail to allocate the code above always clears ctlr->msi to 0. REPOSITORY rS FreeBSD src repository REVISION DETAIL https://reviews.freebsd.org/D3009 EMAIL PREFERENCES https://reviews.freebsd.org/settings/panel/emailpreferences/ To: wma_semihalf.com, zbb, mav, jhb Cc: freebsd-arm-list, imp