| Message ID | 1433073319-13796-4-git-send-email-rric@kernel.org |
|---|---|
| State | Not Applicable |
| Delegated to: | David Miller |
| Headers | show |
Hello, Robert. Maybe qualifying the subject that it's only for single IRQ would be a good idea? > @@ -52,6 +53,7 @@ > > enum { > AHCI_PCI_BAR_STA2X11 = 0, > + AHCI_PCI_BAR_CAVIUM = 0, > AHCI_PCI_BAR_ENMOTUS = 2, > AHCI_PCI_BAR_STANDARD = 5, I thought I already asked but please separate out CAVIUM specific changes from msix implementation and follow up with why cavium depends on msix support. > +static int ahci_init_msix(struct pci_dev *pdev, unsigned int n_ports, > + struct ahci_host_priv *hpriv) > +{ Please add a comment describing that it's for single msix only and why that'd be necessary for certain controllers. ... > + /* > + * Per-port msix interrupts are not supported. Assume single > + * port interrupts for: > + * > + * n_ports == 1, or > + * nvec < n_ports. > + * > + * We also need to check for n_ports != 0 which is implicitly > + * covered here since nvec > 0. > + */ > + if (n_ports != 1 && nvec >= n_ports) { > + rc = -ENOSYS; > + goto fail; > + } Didn't I ask this one too the last time? Can you explain why we can't initialize single IRQ mode if nvec >= n_ports? > @@ -1260,6 +1339,10 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports, > if (nvec >= 0) > return nvec; > > + nvec = ahci_init_msix(pdev, n_ports, hpriv); > + if (nvec >= 0) > + return nvec; Please add a comment explaining why we're doing msix after msi. Thanks.
Tejun, thanks for applying first 2 patches of this series already. I will address the comments you made on this patch in my next submission. But see my question below. On 03.06.15 14:44:22, Tejun Heo wrote: > > + /* > > + * Per-port msix interrupts are not supported. Assume single > > + * port interrupts for: > > + * > > + * n_ports == 1, or > > + * nvec < n_ports. > > + * > > + * We also need to check for n_ports != 0 which is implicitly > > + * covered here since nvec > 0. > > + */ > > + if (n_ports != 1 && nvec >= n_ports) { > > + rc = -ENOSYS; > > + goto fail; > > + } > > Didn't I ask this one too the last time? Can you explain why we can't > initialize single IRQ mode if nvec >= n_ports? I was hoping the comment above the code explains it. Since the code is generic I implemented it a bit conservative wrt enabling msix. So the above does not enable msix for devices with more than one port if the number of interrupts is greater than the number of ports. In this case the device could be multiple IRQ capable. Now, after reading the section in the ahci spec about multiple IRQs more detailed, I tend to be less stricter here. Single IRQ mode is default for each device and multi mode must be explicitly enabled. Thus, we could just enable single msix support for all kind of devices. There are 2 options now. One is to keep the above which means we need to enable multi IRQ capable devices later. The other would be to drop the check above completly and enable single IRQ support for all msix devices. I can't estimate the impact to other devices of this change (option 2). If you think this will be ok I will remove the check. But we could leave it in for the first time and remove it later once tested on such a device. Please let me know your opinion on this. Thanks, -Robert -- To unsubscribe from this list: send the line "unsubscribe linux-ide" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello, Robert. On Thu, Jun 04, 2015 at 11:03:55AM +0200, Robert Richter wrote: > I can't estimate the impact to other devices of this change (option > 2). If you think this will be ok I will remove the check. But we could > leave it in for the first time and remove it later once tested on such > a device. Please let me know your opinion on this. Yeah, let's just enable msix if supported. If some devices are broken, they need to be blacklisted anyway. Thanks.
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c index a3c66c3bb76e..30200e757cdb 100644 --- a/drivers/ata/ahci.c +++ b/drivers/ata/ahci.c @@ -42,6 +42,7 @@ #include <linux/device.h> #include <linux/dmi.h> #include <linux/gfp.h> +#include <linux/msi.h> #include <scsi/scsi_host.h> #include <scsi/scsi_cmnd.h> #include <linux/libata.h> @@ -52,6 +53,7 @@ enum { AHCI_PCI_BAR_STA2X11 = 0, + AHCI_PCI_BAR_CAVIUM = 0, AHCI_PCI_BAR_ENMOTUS = 2, AHCI_PCI_BAR_STANDARD = 5, }; @@ -499,6 +501,9 @@ static const struct pci_device_id ahci_pci_tbl[] = { /* Enmotus */ { PCI_DEVICE(0x1c44, 0x8000), board_ahci }, + /* Cavium */ + { PCI_DEVICE(0x177d, 0xa01c), .driver_data = board_ahci }, + /* Generic, PCI class code for AHCI */ { PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_ANY_ID, PCI_CLASS_STORAGE_SATA_AHCI, 0xffffff, board_ahci }, @@ -1201,6 +1206,80 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host) {} #endif +static struct msi_desc *msix_get_desc(struct pci_dev *dev, u16 entry) +{ + struct msi_desc *desc; + + list_for_each_entry(desc, &dev->msi_list, list) { + if (desc->msi_attrib.entry_nr == entry) + return desc; + } + + return NULL; +} + +static int ahci_init_msix(struct pci_dev *pdev, unsigned int n_ports, + struct ahci_host_priv *hpriv) +{ + struct msi_desc *desc; + int rc, nvec; + struct msix_entry entry = {}; + + /* Do not init MSI-X if MSI is disabled for the device */ + if (hpriv->flags & AHCI_HFLAG_NO_MSI) + return -ENODEV; + + nvec = pci_msix_vec_count(pdev); + if (nvec < 0) + return nvec; + + if (!nvec) { + rc = -ENODEV; + goto fail; + } + + /* + * Per-port msix interrupts are not supported. Assume single + * port interrupts for: + * + * n_ports == 1, or + * nvec < n_ports. + * + * We also need to check for n_ports != 0 which is implicitly + * covered here since nvec > 0. + */ + if (n_ports != 1 && nvec >= n_ports) { + rc = -ENOSYS; + goto fail; + } + + /* + * There can exist more than one vector (e.g. for error + * detection or hdd hotplug). Then the first vector is used, + * all others are ignored. Only enable the first entry here + * (entry.entry = 0). + */ + rc = pci_enable_msix_exact(pdev, &entry, 1); + if (rc < 0) + goto fail; + + desc = msix_get_desc(pdev, 0); /* first entry */ + if (!desc) { + rc = -EINVAL; + goto fail; + } + + hpriv->irq = desc->irq; + + return 1; +fail: + dev_err(&pdev->dev, + "failed to enable MSI-X with error %d, # of vectors: %d\n", + rc, nvec); + + return rc; +} + static int ahci_init_msi(struct pci_dev *pdev, unsigned int n_ports, struct ahci_host_priv *hpriv) { @@ -1260,6 +1339,10 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports, if (nvec >= 0) return nvec; + nvec = ahci_init_msix(pdev, n_ports, hpriv); + if (nvec >= 0) + return nvec; + /* lagacy intx interrupts */ pci_intx(pdev, 1); hpriv->irq = pdev->irq; @@ -1302,11 +1385,13 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent) dev_info(&pdev->dev, "PDC42819 can only drive SATA devices with this driver\n"); - /* Both Connext and Enmotus devices use non-standard BARs */ + /* Some devices use non-standard BARs */ if (pdev->vendor == PCI_VENDOR_ID_STMICRO && pdev->device == 0xCC06) ahci_pci_bar = AHCI_PCI_BAR_STA2X11; else if (pdev->vendor == 0x1c44 && pdev->device == 0x8000) ahci_pci_bar = AHCI_PCI_BAR_ENMOTUS; + else if (pdev->vendor == 0x177d && pdev->device == 0xa01c) + ahci_pci_bar = AHCI_PCI_BAR_CAVIUM; /* * The JMicron chip 361/363 contains one SATA controller and one