Date: Mon, 20 Jul 2015 21:44:17 +0000 From: "jhb (John Baldwin)" <phabric-noreply@FreeBSD.org> To: freebsd-arm@freebsd.org Subject: [Differential] [Updated] D3009: Add MSI-x support to AHCI driver Message-ID: <cd0e8e7214ff9f8c2f2e8700c57e4824@localhost.localdomain> In-Reply-To: <differential-rev-PHID-DREV-cedz4lqpiqqhm2xnetuk-req@FreeBSD.org> References: <differential-rev-PHID-DREV-cedz4lqpiqqhm2xnetuk-req@FreeBSD.org>
next in thread | previous in thread | raw e-mail | index | archive | help
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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?cd0e8e7214ff9f8c2f2e8700c57e4824>