AHCI: Add generic MSI-X interrupt support to SATA PCI driver
Subsystems (2)
v4 cover AHCI: Add generic MSI-X interrupt support to SATA PCI driver
From: Robert Richter <[email protected]>
This patch set adds generic support for MSI-X interrupts to the SATA PCI driver.
The first 2 patches rework the code, one splits msi and intx code into separate functions, the other changes interrupt initialization to store the irq number in the ahci data structure (struct ahci_host_priv). Both changes are needed to implement MSI-X support in the last 3rd patch.
v4:
* removed implementation of ahci_init_intx()
* improved patch descriptions
* rebased onto libata/for-4.2
v3:
* store irq number in struct ahci_host_priv
* change initialization order from msix-msi-intx to msi-msix-intx
* improve comments in ahci_init_msix()
* improve error message in ahci_init_msix()
* do not enable MSI-X if MSI is actively disabled for the device
v2:
* determine irq vector from pci_dev->msi_list
Robert Richter (3):
ahci: Move interrupt enablement code to a separate function
ahci: Store irq number in struct ahci_host_priv
AHCI: Add generic MSI-X interrupt support to SATA PCI driver
drivers/ata/acard-ahci.c | 4 +- drivers/ata/ahci.c | 133 ++++++++++++++++++++++++++++++++++++----- drivers/ata/ahci.h | 4 +- drivers/ata/libahci.c | 16 +++-- drivers/ata/libahci_platform.c | 4 +- drivers/ata/sata_highbank.c | 3 +- 6 files changed, 135 insertions(+), 29 deletions(-)
Signature · 1 line
2.1.1
PATCH v4 1/3 ahci: Move interrupt enablement code to a separate function
From: Robert Richter <[email protected]>
This patch refactors ahci_init_interrupts() and moves msi code to a separate function. Need the split since we add msix initialization in a later patch. The initialization for msix will be done after msi but before intx.
--- drivers/ata/ahci.c | 31 ++++++++++++++++++++++--------- 1 file changed, 22 insertions(+), 9 deletions(-)
drivers/ata/ahci.c +22 -9
@@ -1201,17 +1201,17 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host) {}#endif -static int ahci_init_interrupts (struct pci_dev *pdev,unsigned int n_ports, -struct ahci_host_priv *hpriv) +static int ahci_init_msi (struct pci_dev *pdev,unsigned int n_ports, +struct ahci_host_priv *hpriv) {int rc, nvec;if (hpriv->flags &AHCI_HFLAG_NO_MSI ) - goto intx; +return -ENODEV ; nvec =pci_msi_vec_count (pdev);if (nvec <0 ) - goto intx; +return nvec;/* * If number of MSIs is less than number of ports then Sharing Last
@@ -1224,8 +1224,8 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports, rc =pci_enable_msi_exact (pdev, nvec);if (rc == -ENOSPC ) goto single_msi; -else if (rc <0 ) - goto intx; +if (rc <0 ) +return rc;/* fallback to single MSI mode if the controller enforced MRSM mode */ if (readl (hpriv->mmio +HOST_CTL ) &HOST_MRSM ) {
@@ -1240,12 +1240,25 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,return nvec; single_msi: -if (pci_enable_msi (pdev)) - goto intx; + rc =pci_enable_msi (pdev); +if (rc <0 ) +return rc; +return 1 ; +} -intx: +static int ahci_init_interrupts (struct pci_dev *pdev,unsigned int n_ports, +struct ahci_host_priv *hpriv) +{ +int nvec; + + nvec =ahci_init_msi (pdev, n_ports, hpriv); +if (nvec >=0 ) +return nvec; + +/* lagacy intx interrupts */ pci_intx (pdev,1 ); +return 0 ; }
Signature · 1 line
2.1.1
PATCH v4 2/3 ahci: Store irq number in struct ahci_host_priv
In reply to Robert RichterFrom: Robert Richter <[email protected]>
Currently, ahci supports only msi and intx. To also support msix the handling of the irq number need to be changed. The irq number for msix devices is taken from msi_list instead of pci_dev. Thus, the irq number of a device needs to be stored in struct ahci_host_priv now. This allows the host controller to be activated in a generic way.
This change is only intended for ahci drivers. For that reason the irq number is stored in struct ahci_host_priv used only by ahci drivers. Thus, the ABI changes only for ahci_host_activate(), but existing ata drivers (about 50) are unaffected and keep unchanged. All users of ahci_host_activate() have been updated.
While touching drivers/ata/libahci.c, doing a small code cleanup in ahci_port_start().
--- drivers/ata/acard-ahci.c | 4 +++- drivers/ata/ahci.c | 15 ++++++++++----- drivers/ata/ahci.h | 4 ++-- drivers/ata/libahci.c | 16 +++++++--------- drivers/ata/libahci_platform.c | 4 +++- drivers/ata/sata_highbank.c | 3 ++- 6 files changed, 27 insertions(+), 19 deletions(-)
drivers/ata/acard-ahci.c +3 -1
@@ -433,6 +433,8 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_id hpriv =devm_kzalloc (dev,sizeof (*hpriv),GFP_KERNEL );if (!hpriv)return -ENOMEM ; + + hpriv->irq = pdev->irq; hpriv->flags |= (unsigned long )pi.private_data;if (!(hpriv->flags &AHCI_HFLAG_NO_MSI ))
@@ -498,7 +500,7 @@ static int acard_ahci_init_one(struct pci_dev *pdev, const struct pci_device_idacard_ahci_pci_print_info (host);pci_set_master (pdev); -return ahci_host_activate (host, pdev->irq, &acard_ahci_sht); +return ahci_host_activate (host, &acard_ahci_sht); }module_pci_driver (acard_ahci_pci_driver);
drivers/ata/ahci.c +10 -5
@@ -1237,14 +1237,18 @@ static int ahci_init_msi(struct pci_dev *pdev, unsigned int n_ports,if (nvec >1 ) hpriv->flags |=AHCI_HFLAG_MULTI_MSI ; -return nvec; + goto out; single_msi: + nvec =1 ; + rc =pci_enable_msi (pdev);if (rc <0 )return rc; +out: + hpriv->irq = pdev->irq; -return 1 ; +return nvec; } static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
@@ -1258,6 +1262,7 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,/* lagacy intx interrupts */ pci_intx (pdev,1 ); + hpriv->irq = pdev->irq;return 0 ; }
@@ -1423,13 +1428,13 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)*/ n_ports =max (ahci_nr_ports (hpriv->cap),fls (hpriv->port_map)); -ahci_init_interrupts (pdev, n_ports, hpriv); - host =ata_host_alloc_pinfo (&pdev->dev, ppi, n_ports);if (!host)return -ENOMEM ; host->private_data = hpriv; +ahci_init_interrupts (pdev, n_ports, hpriv); +if (!(hpriv->cap &HOST_CAP_SSS ) || ahci_ignore_sss) host->flags |=ATA_HOST_PARALLEL_SCAN ;else
@@ -1475,7 +1480,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)pci_set_master (pdev); -return ahci_host_activate (host, pdev->irq, &ahci_sht); +return ahci_host_activate (host, &ahci_sht); }module_pci_driver (ahci_pci_driver);
drivers/ata/ahci.h +2 -2
@@ -343,6 +343,7 @@ struct ahci_host_priv {struct phy **phys;unsigned nports;/* Number of ports */ void *plat_data;/* Other platform data */ +unsigned int irq;/* interrupt line */ /* * Optional ahci_start_engine override, if not set this gets set to the * default ahci_start_engine during ahci_save_initial_config, this can
@@ -395,8 +396,7 @@ void ahci_set_em_messages(struct ahci_host_priv *hpriv, struct ata_port_info *pi);int ahci_reset_em (struct ata_host *host);void ahci_print_info (struct ata_host *host,const char *scc_s); -int ahci_host_activate (struct ata_host *host,int irq, -struct scsi_host_template *sht); +int ahci_host_activate (struct ata_host *host,struct scsi_host_template *sht);void ahci_error_handler (struct ata_port *ap); static inline void __iomem *__ahci_port_base(struct ata_host *host,
drivers/ata/libahci.c +7 -9
@@ -2344,7 +2344,7 @@ static int ahci_port_start(struct ata_port *ap)/* * Switch to per-port locking in case each port has its own MSI vector. */ -if ((hpriv->flags &AHCI_HFLAG_MULTI_MSI )) { +if (hpriv->flags &AHCI_HFLAG_MULTI_MSI ) {spin_lock_init (&pp->lock); ap->lock = &pp->lock; }
@@ -2472,7 +2472,10 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host, int irq, rc =ata_host_start (host);if (rc)return rc; - +/* +* Requests IRQs according to AHCI-1.1 when multiple MSIs were +* allocated. That is one MSI per port, starting from @irq. +*/ for (i =0 ; i < host->n_ports; i++) {struct ahci_port_priv *pp = host->ports[i]->private_data;
@@ -2511,23 +2514,18 @@ static int ahci_host_activate_multi_irqs(struct ata_host *host, int irq,/** * ahci_host_activate - start AHCI host, request IRQs and register it * @host: target ATA host -* @irq: base IRQ number to request * @sht: scsi_host_template to use when registering the host * -* Similar to ata_host_activate, but requests IRQs according to AHCI-1.1 -* when multiple MSIs were allocated. That is one MSI per port, starting -* from @irq. -* * LOCKING: * Inherited from calling layer (may sleep). * * RETURNS: * 0 on success, -errno otherwise. */ -int ahci_host_activate (struct ata_host *host,int irq, -struct scsi_host_template *sht) +int ahci_host_activate (struct ata_host *host,struct scsi_host_template *sht) {struct ahci_host_priv *hpriv = host->private_data; +int irq = hpriv->irq;int rc;if (hpriv->flags &AHCI_HFLAG_MULTI_MSI )
drivers/ata/libahci_platform.c +3 -1
@@ -518,6 +518,8 @@ int ahci_platform_init_host(struct platform_device *pdev,return -EINVAL ; } + hpriv->irq = irq; +/* prepare host */ pi.private_data = (void *)(unsigned long )hpriv->flags;
@@ -588,7 +590,7 @@ int ahci_platform_init_host(struct platform_device *pdev,ahci_init_controller (host);ahci_print_info (host,"platform" ); -return ahci_host_activate (host, irq, sht); +return ahci_host_activate (host, sht); }EXPORT_SYMBOL_GPL (ahci_platform_init_host);
drivers/ata/sata_highbank.c +2 -1
@@ -499,6 +499,7 @@ static int ahci_highbank_probe(struct platform_device *pdev)return -ENOMEM ; } + hpriv->irq = irq; hpriv->flags |= (unsigned long )pi.private_data; hpriv->mmio =devm_ioremap (dev, mem->start,resource_size (mem));
@@ -568,7 +569,7 @@ static int ahci_highbank_probe(struct platform_device *pdev)ahci_init_controller (host);ahci_print_info (host,"platform" ); - rc =ahci_host_activate (host, irq, &ahci_highbank_platform_sht); + rc =ahci_host_activate (host, &ahci_highbank_platform_sht);if (rc) goto err0;
Signature · 1 line
2.1.1
Re: [PATCH v4 2/3] ahci: Store irq number in struct ahci_host_priv
Quoted 14 lines · Robert Richter
From: Robert Richter <[email protected]>
Currently, ahci supports only msi and intx. To also support msix the handling of the irq number need to be changed. The irq number for msix devices is taken from
msi_listinstead ofpci_dev. Thus, the irq number of a device needs to be stored in structahci_host_privnow. This allows the host controller to be activated in a generic way.This change is only intended for ahci drivers. For that reason the irq number is stored in struct
ahci_host_privused only by ahci drivers. Thus, the ABI changes only forahci_host_activate(), but existing ata drivers (about 50) are unaffected and keep unchanged. All users ofahci_host_activate()have been updated.
Quoted 4 lines
While touching
drivers/ata/libahci.c, doing a small code cleanup inahci_port_start().Signed-off-by Robert Richter <[email protected]>
Applied 1-2 to libata/for-4.2.
Thanks.
Signature · 1 line
tejun
PATCH v4 3/3 AHCI: Add generic MSI-X interrupt support to SATA PCI driver
In reply to Robert RichterFrom: Robert Richter <[email protected]>
This patch adds generic support for MSI-X interrupts to the SATA PCI driver. MSI-X support is needed for host controller that only have MSI-X support implemented, such as the controller on Cavium's ThunderX SoC. Only support for single interrupts is added, multiple per-port MSI-X interrupts are not yet implemented.
The new implementation still initializes MSIs first. Only if that fails, the code tries to enable MSI-X. If that fails too, setup is continued with intx interrupts.
To not break other chips by this generic code change, there are the following precautions:
* Interrupt ranges are not enabled at all.
* Only single interrupt mode is enabled for msix cap devices. These devices require a single port only or a total number of int entries less than the total number of ports. In this case only one interrupt will be enabled.
* During the discussion with Tejun we agreed to change the init sequence from msix-msi-intx to msi-msix-intx. Thus, if a device offers msi and init does not fail, the msix init code will not be executed. This is equivalent to current code.
With this, the code only setups single mode msix as a last resort if msi fails. No interrupt range is enabled at all. Only one interrupt will be enabled.
v4:
* removed implementation of ahci_init_intx()
* improved patch descriptions
* rebased onto libata/for-4.2
v3:
* store irq number in struct ahci_host_priv
* change initialization order from msix-msi-intx to msi-msix-intx
* improve comments in ahci_init_msix()
* improve error message in ahci_init_msix()
* do not enable MSI-X if MSI is actively disabled for the device
v2:
* determine irq vector from pci_dev->msi_list
Based on a patch from Sunil Goutham <[email protected]>.
--- drivers/ata/ahci.c | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++- 1 file changed, 86 insertions(+), 1 deletion(-)
drivers/ata/ahci.c +86 -1
@@ -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
Signature · 1 line
2.1.1
Hello, Robert.
Maybe qualifying the subject that it's only for single IRQ would be a good idea?
drivers/ata/ahci.c line 58
Quoted 7 lines
drivers/ata/ahci.c +1
-0@@ -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.
drivers/ata/ahci.c static inline void ahci_gtf_filter_workaround(struct ata_host *host) · line 1223
Quoted 3 lines
drivers/ata/ahci.c +3
-0+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.
...
Quoted 4 lines
drivers/ata/ahci.c +4
-0+/* +* Per-port msix interrupts are not supported. Assume single +* port interrupts for: +*
drivers/ata/ahci.c static inline void ahci_gtf_filter_workaround(struct ata_host *host) · line 1254
Quoted 10 lines
drivers/ata/ahci.c +10
-0+* 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?
drivers/ata/ahci.c static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports, · line 1344
Quoted 7 lines
drivers/ata/ahci.c +3
-0@@ -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.
Signature · 1 line
tejun
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.
Quoted 9 lines · Tejun Heo
Quoted 9 lines
drivers/ata/ahci.c +9
-0+/* +* 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.
Quoted 8 lines
drivers/ata/ahci.c static inline void ahci_gtf_filter_workaround(struct ata_host *host) · line 1254
Quoted 5 lines
drivers/ata/ahci.c +5
-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
Hello, Robert.
Quoted 4 lines · Robert Richter
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.
Signature · 1 line
tejun