[PATCH v3 0/3] AHCI: Add generic MSI-X interrupt support to SATA PCI driver

[PATCH v3 0/3] AHCI: Add generic MSI-X interrupt support to SATA PCI driver

[Robert Richter]

From: Robert Richter <rrichter@cavium.com>

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.

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 separate functions
  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             | 138 ++++++++++++++++++++++++++++++++++++-----
 drivers/ata/ahci.h             |   4 +-
 drivers/ata/libahci.c          |  25 +++-----
 drivers/ata/libahci_platform.c |   4 +-
 drivers/ata/sata_highbank.c    |   3 +-
 6 files changed, 143 insertions(+), 35 deletions(-)

-- 
2.1.1

[PATCH v3 1/3] ahci: Move interrupt enablement code to separate functions

[Robert Richter]

From: Robert Richter <rrichter@cavium.com>

This patch refactors ahci_init_interrupts() and moves code to separate
functions for msi and intx. Needed since we add msix initialization in
a later patch. The initialization for msix is done after msi but
before intx.

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

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 33bb06e006c9..c8aedd836dc9 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -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,15 +1240,33 @@ 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_intx(struct pci_dev *pdev, unsigned int n_ports,
+			struct ahci_host_priv *hpriv)
+{
 	pci_intx(pdev, 1);
+
 	return 0;
 }
 
+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;
+
+	return ahci_init_intx(pdev, n_ports, hpriv);
+}
+
 static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	unsigned int board_id = ent->driver_data;
-- 
2.1.1

[PATCH v3 2/3] ahci: Store irq number in struct ahci_host_priv

[Robert Richter]

From: Robert Richter <rrichter@cavium.com>

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.  Host controller can be activated then in a
generic way.

Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 drivers/ata/acard-ahci.c       |  4 +++-
 drivers/ata/ahci.c             | 15 ++++++++++-----
 drivers/ata/ahci.h             |  4 ++--
 drivers/ata/libahci.c          | 25 ++++++++++---------------
 drivers/ata/libahci_platform.c |  4 +++-
 drivers/ata/sata_highbank.c    |  3 ++-
 6 files changed, 30 insertions(+), 25 deletions(-)

diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
index c962886d7e71..c67fce2a4cbf 100644
--- a/drivers/ata/acard-ahci.c
+++ b/drivers/ata/acard-ahci.c
@@ -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_id
 	acard_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);
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index c8aedd836dc9..8cdc9ebbbc43 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1237,20 +1237,25 @@ 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_intx(struct pci_dev *pdev, unsigned int n_ports,
 			struct ahci_host_priv *hpriv)
 {
 	pci_intx(pdev, 1);
+	hpriv->irq = pdev->irq;
 
 	return 0;
 }
@@ -1428,13 +1433,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
@@ -1480,7 +1485,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);
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 71262e08648e..f219507b1fbc 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -341,6 +341,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
@@ -393,8 +394,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,
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 61a9c07e0dff..022da7ce77fb 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -2298,7 +2298,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;
 	}
@@ -2426,7 +2426,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;
 
@@ -2465,31 +2468,23 @@ 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 rc;
 
 	if (hpriv->flags & AHCI_HFLAG_MULTI_MSI)
-		rc = ahci_host_activate_multi_irqs(host, irq, sht);
-	else
-		rc = ata_host_activate(host, irq, ahci_single_irq_intr,
-				       IRQF_SHARED, sht);
-	return rc;
+		return ahci_host_activate_multi_irqs(host, hpriv->irq, sht);
+
+	return ata_host_activate(host, hpriv->irq, ahci_single_irq_intr,
+				IRQF_SHARED, sht);
 }
 EXPORT_SYMBOL_GPL(ahci_host_activate);
 
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index d89305d289f6..aaa761b9081c 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -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);
 
diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index 24e311fe2c1c..8638d575b2b9 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -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;
 
-- 
2.1.1

[PATCH v3 3/3] AHCI: Add generic MSI-X interrupt support to SATA PCI driver

[Robert Richter]

From: Robert Richter <rrichter@cavium.com>

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.

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 | 87 +++++++++++++++++++++++++++++++++++++++++++++++++++++-
 1 file changed, 86 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 8cdc9ebbbc43..8245f75be6b6 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)
 {
@@ -1269,6 +1348,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;
+
 	return ahci_init_intx(pdev, n_ports, hpriv);
 }
 
@@ -1307,11 +1390,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

Re: [PATCH v3 0/3] AHCI: Add generic MSI-X interrupt support to SATA PCI driver

[Tejun Heo]

On Wed, May 27, 2015 at 10:01:30AM +0200, Robert Richter wrote:
> From: Robert Richter <rrichter@cavium.com>
> 
> 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.

Patch #2 doesn't apply to libata/for-4.2 due to the recently added
edge triggered irq support.  Can you please refresh the patch series?

Thank you.

-- 
tejun

[PATCH] ahci: Store irq number in struct ahci_host_priv

[Robert Richter]

On 27.05.15 10:01:32, Robert Richter wrote:
> From: Robert Richter <rrichter@cavium.com>
> 
> 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.  Host controller can be activated then in a
> generic way.
> 
> Signed-off-by: Robert Richter <rrichter@cavium.com>

Rebased version onto libata/for-4.2 below. Please apply 1/3 and 3/3
from the original patch set before and after.

Thanks,

-Robert



>From b50a5e478b8fce17603a91a5d272bb49527239af Mon Sep 17 00:00:00 2001
From: Robert Richter <rrichter@cavium.com>
Date: Tue, 12 May 2015 13:57:27 +0200
Subject: [PATCH] ahci: Store irq number in struct ahci_host_priv

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.  Host controller can be activated then in a
generic way.

Signed-off-by: Robert Richter <rrichter@cavium.com>
---
 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(-)

diff --git a/drivers/ata/acard-ahci.c b/drivers/ata/acard-ahci.c
index 12489ce863c4..ed6a30cd681a 100644
--- a/drivers/ata/acard-ahci.c
+++ b/drivers/ata/acard-ahci.c
@@ -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_id
 	acard_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);
diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 903ceed88aff..c40bb5670e88 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1237,20 +1237,25 @@ 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_intx(struct pci_dev *pdev, unsigned int n_ports,
 			struct ahci_host_priv *hpriv)
 {
 	pci_intx(pdev, 1);
+	hpriv->irq = pdev->irq;
 
 	return 0;
 }
@@ -1428,13 +1433,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
@@ -1480,7 +1485,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);
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index f4429600e2bf..5b8e8a0fab48 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -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,
diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
index 1add5baec584..1c99402a1017 100644
--- a/drivers/ata/libahci.c
+++ b/drivers/ata/libahci.c
@@ -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)
diff --git a/drivers/ata/libahci_platform.c b/drivers/ata/libahci_platform.c
index d89305d289f6..aaa761b9081c 100644
--- a/drivers/ata/libahci_platform.c
+++ b/drivers/ata/libahci_platform.c
@@ -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);
 
diff --git a/drivers/ata/sata_highbank.c b/drivers/ata/sata_highbank.c
index 24e311fe2c1c..8638d575b2b9 100644
--- a/drivers/ata/sata_highbank.c
+++ b/drivers/ata/sata_highbank.c
@@ -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;
 
-- 
2.1.1

Re: [PATCH v3 1/3] ahci: Move interrupt enablement code to separate functions

[Tejun Heo]

Hello,

On Wed, May 27, 2015 at 10:01:31AM +0200, Robert Richter wrote:
> +static int ahci_init_intx(struct pci_dev *pdev, unsigned int n_ports,
> +			struct ahci_host_priv *hpriv)
> +{
>  	pci_intx(pdev, 1);
> +
>  	return 0;
>  }

Let's please not factor out the above as a separate function.  An one
lin function which always returns 0 doesn't really serve any purpose.

Thanks.

-- 
tejun

Re: [PATCH] ahci: Store irq number in struct ahci_host_priv

[Tejun Heo]

Hello,

On Wed, May 27, 2015 at 06:16:03PM +0200, Robert Richter wrote:
> From b50a5e478b8fce17603a91a5d272bb49527239af Mon Sep 17 00:00:00 2001
> From: Robert Richter <rrichter@cavium.com>
> Date: Tue, 12 May 2015 13:57:27 +0200
> Subject: [PATCH] ahci: Store irq number in struct ahci_host_priv
> 

Currently, ahci supports only msi and intx; however, msix support is
planned for ....

> 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.  Host controller can be activated then in a
> generic way.

The above paragraph doesn't really explain why it needs to be moved to
host_priv, does it?  Can you please elaborate a bit further?

> diff --git a/drivers/ata/libahci.c b/drivers/ata/libahci.c
> index 1add5baec584..1c99402a1017 100644
> --- a/drivers/ata/libahci.c
> +++ b/drivers/ata/libahci.c
> @@ -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) {

This sort of cleanups are fine but please mention them in the patch
description.

Thanks.

-- 
tejun