[PATCH v5 0/2] ahci: Add generic MSI-X support for single interrupts to SATA PCI

[PATCH v5 0/2] ahci: Add generic MSI-X support for single interrupts to SATA PCI

[Robert Richter]

From: Robert Richter <rrichter@cavium.com>

This patch set adds generic support for MSI-X interrupts to the SATA
PCI driver.

One patch does the generic change, the other add support for Cavium's
ThunderX host controller.


Robert Richter (2):
  ahci: Add generic MSI-X support for single interrupts to SATA PCI
    driver
  ahci: Add support for Cavium's ThunderX host controller

 drivers/ata/ahci.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 81 insertions(+), 1 deletion(-)

-- 
2.1.1

[PATCH v5 1/2] ahci: Add generic MSI-X support for single interrupts to SATA PCI driver

[Robert Richter]

From: Robert Richter <rrichter@cavium.com>

This patch adds generic MSI-X support for single interrupts to the
SATA PCI driver. MSI-X support is needed for host controller that only
have MSI-X support implemented, but no MSI or intx. This patch only
adds support for single interrupts, 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. Thus,
   only one interrupt will be setup.

 * 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.

Changes of the patch series:

v5:
 * updated patch subject that the patch only implements single IRQ
 * moved Cavium specific code to a separate patch
 * detect Cavium ThunderX device with PCI_CLASS_STORAGE_SATA_AHCI
   instead of vendor/dev id
 * added more comments to the code
 * enable single msix support for all kind of devices (removing strict
   check)
 * rebased onto update libata/for-4.2 with patch 1, 2 applied

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 <sgoutham@cavium.com>.

Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/ata/ahci.c | 77 ++++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 insertions(+)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index a3c66c3bb76e..f300aa583678 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>
@@ -1201,6 +1202,71 @@ 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;
+}
+
+/*
+ * MSI-X support is needed for host controller that only have MSI-X
+ * support implemented, but no MSI or intx. For now, function
+ * ahci_init_msix() only implements single MSI-X support, but not
+ * multiple MSI-X per-port interrupts.
+ */
+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;
+	}
+
+	/*
+	 * 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 +1326,17 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
 	if (nvec >= 0)
 		return nvec;
 
+	/*
+	 * Only setup single mode MSI-X as a last resort if MSI fails:
+	 * Initialize MSI-X after MSI and only if that fails, continue
+	 * with intx interrupts on failure. Thus, MSI-X code is not
+	 * executed if a device offers MSI and its initialization does
+	 * not fail.
+	 */
+	nvec = ahci_init_msix(pdev, n_ports, hpriv);
+	if (nvec >= 0)
+		return nvec;
+
 	/* lagacy intx interrupts */
 	pci_intx(pdev, 1);
 	hpriv->irq = pdev->irq;
-- 
2.1.1

[PATCH v5 2/2] ahci: Add support for Cavium's ThunderX host controller

[Robert Richter]

From: Robert Richter <rrichter@cavium.com>

This patch adds support for Cavium's ThunderX host controller. The
controller resides on the SoC and is a AHCI compatible SATA controller
with one port, compliant with Serial ATA 3.1 and AHCI Revision 1.31.
There can exists multiple SATA controllers on the SoC.

The controller depends on MSI-X support since the PCI ECAM controller
on the SoC does not implement MSI nor lagacy intx interrupt support.
Thus, during device initialization, if MSI fails MSI-X will be used to
enable the device's interrupts.

The controller uses non-standard BAR0 for its register range. The
already existing device lookup (vendor and device id) that is already
implemented for other host controllers is used to change the PCI BAR.

Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/ata/ahci.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index f300aa583678..094649db3330 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -53,6 +53,7 @@
 
 enum {
 	AHCI_PCI_BAR_STA2X11	= 0,
+	AHCI_PCI_BAR_CAVIUM	= 0,
 	AHCI_PCI_BAR_ENMOTUS	= 2,
 	AHCI_PCI_BAR_STANDARD	= 5,
 };
@@ -1379,11 +1380,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
-- 
2.1.1

[PATCH v5 0/2] ahci: Add generic MSI-X support for single interrupts to SATA PCI

[Robert Richter]

On 05.06.15 19:49:24, Robert Richter wrote:
> From: Robert Richter <rrichter@cavium.com>
> 
> This patch set adds generic support for MSI-X interrupts to the SATA
> PCI driver.
> 
> One patch does the generic change, the other add support for Cavium's
> ThunderX host controller.

Tejun,

any comment on this?

Thanks,

-Robert

> 
> 
> Robert Richter (2):
>   ahci: Add generic MSI-X support for single interrupts to SATA PCI
>     driver
>   ahci: Add support for Cavium's ThunderX host controller
> 
>  drivers/ata/ahci.c | 82 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 81 insertions(+), 1 deletion(-)
> 
> -- 
> 2.1.1
> 

[PATCH v5 0/2] ahci: Add generic MSI-X support for single interrupts to SATA PCI

[Tejun Heo]

On Tue, Jun 16, 2015 at 03:24:34PM +0200, Robert Richter wrote:
> On 05.06.15 19:49:24, Robert Richter wrote:
> > From: Robert Richter <rrichter@cavium.com>
> > 
> > This patch set adds generic support for MSI-X interrupts to the SATA
> > PCI driver.
> > 
> > One patch does the generic change, the other add support for Cavium's
> > ThunderX host controller.
> 
> Tejun,
> 
> any comment on this?

Oops, sorry, the thread fell through the crack somehow.  I applied the
two patches to libata/for-4.2 with some comment edits on the first
patch.

Thanks!

-- 
tejun

[PATCH] ahci, msix: Fix build error for !PCI_MSI

[Robert Richter]

>From fd984f3be22f27b8d3c4cf577dbbf0a39792ea89 Mon Sep 17 00:00:00 2001
From: Robert Richter <rrichter@cavium.com>
Date: Wed, 17 Jun 2015 10:33:22 +0200
Subject: [PATCH] ahci, msix: Fix build error for !PCI_MSI
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Fixing a build error if PCI_MSI is unset:

 drivers/ata/ahci.c: In function ?msix_get_desc?:
 drivers/ata/ahci.c:1210:2: error: ?struct pci_dev? has no member named ?msi_list?

Catched by Fengguang's build bot.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/ata/ahci.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index bdedaa4f9d7b..99cc9526ae95 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1205,13 +1205,14 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host)
 
 static struct msi_desc *msix_get_desc(struct pci_dev *dev, u16 entry)
 {
+#ifdef CONFIG_PCI_MSI
 	struct msi_desc *desc;
 
 	list_for_each_entry(desc, &dev->msi_list, list) {
 		if (desc->msi_attrib.entry_nr == entry)
 			return desc;
 	}
-
+#endif
 	return NULL;
 }
 
-- 
2.1.1

[PATCH] ahci, msix: Fix build error for !PCI_MSI

[Jiang Liu]

On 2015/6/17 16:48, Robert Richter wrote:
> From fd984f3be22f27b8d3c4cf577dbbf0a39792ea89 Mon Sep 17 00:00:00 2001
> From: Robert Richter <rrichter@cavium.com>
> Date: Wed, 17 Jun 2015 10:33:22 +0200
> Subject: [PATCH] ahci, msix: Fix build error for !PCI_MSI
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> Fixing a build error if PCI_MSI is unset:
> 
>  drivers/ata/ahci.c: In function ?msix_get_desc?:
>  drivers/ata/ahci.c:1210:2: error: ?struct pci_dev? has no member named ?msi_list?
> 
> Catched by Fengguang's build bot.
> 
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: Robert Richter <rrichter@cavium.com>
> ---
>  drivers/ata/ahci.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
> index bdedaa4f9d7b..99cc9526ae95 100644
> --- a/drivers/ata/ahci.c
> +++ b/drivers/ata/ahci.c
> @@ -1205,13 +1205,14 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host)
>  
>  static struct msi_desc *msix_get_desc(struct pci_dev *dev, u16 entry)
>  {
> +#ifdef CONFIG_PCI_MSI
>  	struct msi_desc *desc;
>  
>  	list_for_each_entry(desc, &dev->msi_list, list) {
>  		if (desc->msi_attrib.entry_nr == entry)
>  			return desc;
>  	}
> -
> +#endif
Hi Robert,
	While at it, could you please help to use  for_each_pci_msi_entry()
from include/linux/msi.h instead of hard-coding? We are trying to refine
the msi_list related code,
so we don't need to touch this code again later.
Thanks!
Gerry

>  	return NULL;
>  }
>  
> 

[PATCH v2] ahci, msix: Fix build error for !PCI_MSI

[Robert Richter]

Tejun, Gerry,

On 17.06.15 17:02:20, Jiang Liu wrote:
> On 2015/6/17 16:48, Robert Richter wrote:

> >  static struct msi_desc *msix_get_desc(struct pci_dev *dev, u16 entry)
> >  {
> > +#ifdef CONFIG_PCI_MSI
> >  	struct msi_desc *desc;
> >  
> >  	list_for_each_entry(desc, &dev->msi_list, list) {
> >  		if (desc->msi_attrib.entry_nr == entry)
> >  			return desc;
> >  	}
> > -
> > +#endif

> Hi Robert,
> 	While at it, could you please help to use  for_each_pci_msi_entry()
> from include/linux/msi.h instead of hard-coding? We are trying to refine
> the msi_list related code,
> so we don't need to touch this code again later.

While looking into this I realized the code can be much more
simplified so that the desc lookup function can be removed at all.
See below.

Sorry for the patch noise.

-Robert


>From e43b79f0afd794d3e37ec0922c75e44b1c3f2e22 Mon Sep 17 00:00:00 2001
From: Robert Richter <rrichter@cavium.com>
Date: Wed, 17 Jun 2015 10:33:22 +0200
Subject: [PATCH v2] ahci, msix: Fix build error for !PCI_MSI
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

It turned out the irq vector of the msix can be obtained from struct
msix_entry. This makes the lookup function for msi_desc obsolete.

This fixes a build error if PCI_MSI is unset:

 drivers/ata/ahci.c: In function ?msix_get_desc?:
 drivers/ata/ahci.c:1210:2: error: ?struct pci_dev? has no member named ?msi_list?

Catched by Fengguang's build bot.

Reported-by: kbuild test robot <fengguang.wu@intel.com>
Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/ata/ahci.c | 21 +--------------------
 1 file changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index bdedaa4f9d7b..c478a40e32c6 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1203,18 +1203,6 @@ 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;
-}
-
 /*
  * ahci_init_msix() only implements single MSI-X support, not multiple
  * MSI-X per-port interrupts. This is needed for host controllers that only
@@ -1223,7 +1211,6 @@ static struct msi_desc *msix_get_desc(struct pci_dev *dev, u16 entry)
 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 = {};
 
@@ -1248,13 +1235,7 @@ static int ahci_init_msix(struct pci_dev *pdev, unsigned int n_ports,
 	if (rc < 0)
 		goto fail;
 
-	desc = msix_get_desc(pdev, 0);	/* first entry */
-	if (!desc) {
-		rc = -EINVAL;
-		goto fail;
-	}
-
-	hpriv->irq = desc->irq;
+	hpriv->irq = entry.vector;
 
 	return 1;
 fail:
-- 
2.1.1

[PATCH v2] ahci, msix: Fix build error for !PCI_MSI

[Tejun Heo]

On Wed, Jun 17, 2015 at 03:30:02PM +0200, Robert Richter wrote:
> From e43b79f0afd794d3e37ec0922c75e44b1c3f2e22 Mon Sep 17 00:00:00 2001
> From: Robert Richter <rrichter@cavium.com>
> Date: Wed, 17 Jun 2015 10:33:22 +0200
> Subject: [PATCH v2] ahci, msix: Fix build error for !PCI_MSI
> MIME-Version: 1.0
> Content-Type: text/plain; charset=UTF-8
> Content-Transfer-Encoding: 8bit
> 
> It turned out the irq vector of the msix can be obtained from struct
> msix_entry. This makes the lookup function for msi_desc obsolete.
> 
> This fixes a build error if PCI_MSI is unset:
> 
>  drivers/ata/ahci.c: In function ?msix_get_desc?:
>  drivers/ata/ahci.c:1210:2: error: ?struct pci_dev? has no member named ?msi_list?
> 
> Catched by Fengguang's build bot.
> 
> Reported-by: kbuild test robot <fengguang.wu@intel.com>
> Signed-off-by: Robert Richter <rrichter@cavium.com>

Applied to libata/for-4.2.

Thanks.

-- 
tejun