[Differential] [Updated] D3009: Add MSI-x support to AHCI driver

[jhb (John Baldwin)]

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