AHCI: Add generic MSI-X interrupt support to SATA PCI driver

PATCH Superseded Robert Richter 4 participants 12 messages May 4, 2015 – May 18, 2015

Subsystems (2)

arm64 libata

lore

Share & tools

lore

Robert Richter

author

2015-05-04 07:45 UTC #0 lore raw

PATCH v2 1/1 AHCI: Add generic MSI-X interrupt support to SATA PCI driver

From: Robert Richter <[email protected]>

This patch adds generic support for MSI-X interrupts to the SATA PCI driver. Only single interrupt support is implemented. Thus, per-port interrupts can not yet be enabled.

The driver now checks the device for the existence of MSI-X and tries to enable the interrupt. Otherwise, if a device is not MSI-X capable, the initialization is skipped and MSI or intx interrupts are configured.

This patch also enables AHCI for Cavium Thunder SoCs that uses MSI-X.

v2:
* determine irq vector from pci_dev->msi_list

Based on a patch from Sunil Goutham <[email protected]>.

Signed-off-by Robert Richter <[email protected]>

---
 arch/arm64/Kconfig |  1 +
 drivers/ata/ahci.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 75 insertions(+), 5 deletions(-)

arch/arm64/Kconfig +1 -0

index b1f9a20a3677..2d95bbc0557e 100644
@@ -157,6 +157,7 @@ config ARCH_THUNDER
 	bool "Cavium Inc. Thunder SoC Family"
 	help
 	  This enables support for Cavium's Thunder Family of SoCs.
+	select SATA_AHCI
 
 config ARCH_VEXPRESS
 	bool "ARMv8 software model (Versatile Express)"

drivers/ata/ahci.c +74 -5

140 lines not on this page · raw message

Signature · 1 line

2.1.1

Tejun Heo

2015-05-04 16:06 UTC #1 lore raw

Hello, Robert. (cc'ing Alexander for ahci msi)

Quoted 6 lines · Robert Richter

From: Robert Richter <[email protected]>
This patch adds generic support for MSI-X interrupts to the SATA PCI driver. Only single interrupt support is implemented. Thus, per-port interrupts can not yet be enabled.

Quoted 6 lines

The driver now checks the device for the existence of MSI-X and tries to enable the interrupt. Otherwise, if a device is not MSI-X capable, the initialization is skipped and MSI or intx interrupts are configured.
This patch also enables AHCI for Cavium Thunder SoCs that uses MSI-X.

Please don't mix these two changes in the same patch.

Quoted 10 lines

drivers/ata/ahci.c +4 -2

@@ -1202,11 +1207,41 @@ 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_msix(struct pci_dev *pdev, unsigned int n_ports,
+			  struct ahci_host_priv *hpriv)
+{
+	int rc, nvec;

drivers/ata/ahci.c static inline void ahci_gtf_filter_workaround(struct ata_host *host) · line 1223

Quoted 10 lines

drivers/ata/ahci.c +10 -0

+	struct msix_entry entry = {};
+
+	/* check if msix is supported */
+	nvec = pci_msix_vec_count(pdev);
+	if (nvec <= 0)
+		return 0;
+
+	/* per-port msix interrupts are not supported */
+	if (n_ports > 1 && nvec >= n_ports)
+		return -ENOSYS;

Hmm... can you please elaborate why the condition isn't nvec > 1? Also, shouldn't we be printing a warning message here explaining why probing is failing?

drivers/ata/ahci.c static inline void ahci_gtf_filter_workaround(struct ata_host *host) · line 1226

Quoted 3 lines

drivers/ata/ahci.c +3 -0

+
+	/* only enable the first entry (entry.entry = 0) */
+	rc = pci_enable_msix_exact(pdev, &entry, 1);

So, enabling the first msix works if nvec > 1 && nvec < n_ports but not if nvec >= n_ports?

Quoted 43 lines

drivers/ata/ahci.c +33 -0

+	if (rc < 0)
+		return rc;
+
+	return 1;
+}
+
+static int __ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
+				  struct ahci_host_priv *hpriv)
 {
 	int rc, nvec;
 
+	nvec = ahci_init_msix(pdev, n_ports, hpriv);
+	if (nvec > 0)
+		return nvec;
+
+	if (nvec && nvec != -ENOSYS)
+		dev_err(&pdev->dev, "failed to enable MSI-X: %d", nvec);
+
 	if (hpriv->flags & AHCI_HFLAG_NO_MSI)
 		goto intx;

@@ -1250,6 +1285,35 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
 	return 0;
 }
 
+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_interrupts(struct pci_dev *pdev, unsigned int n_ports,
+				struct ahci_host_priv *hpriv)
+{
+	struct msi_desc *desc;
+
+	__ahci_init_interrupts(pdev, n_ports, hpriv);

drivers/ata/ahci.c static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports, · line 1315

Quoted 10 lines

drivers/ata/ahci.c +10 -0

+
+	if (!pdev->msix_enabled)
+		return pdev->irq;
+
+	desc = msix_get_desc(pdev, 0);	/* first entry */
+	if (!desc)
+		return -ENODEV;
+
+	return desc->irq;
+}

Can we please do this properly? We should be able to move port priv allocation to host allocaotion time and add and use pp->irq instead, right?

Thanks.

Signature · 1 line

tejun

Robert Richter

2015-05-11 17:18 UTC #2 lore raw

Tejun,

thanks for your review and answer.

Quoted 3 lines · Tejun Heo

Quoted 1 line
This patch also enables AHCI for Cavium Thunder SoCs that uses MSI-X.
Please don't mix these two changes in the same patch.

I will split the patch.

Quoted 5 lines

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
+   /* per-port msix interrupts are not supported */
+   if (n_ports > 1 && nvec >= n_ports)
+           return -ENOSYS;
Hmm... can you please elaborate why the condition isn't nvec > 1?

I slightly changed the check and added a comment that explains that's going on in the function. This is the new version:

static int ahci_init_msix(struct pci_dev *pdev, unsigned int n_ports,
struct ahci_host_priv *hpriv)

{
	int rc, nvec;
	struct msix_entry entry = {};

	/* check if msix is supported */
	nvec = pci_msix_vec_count(pdev);
	if (nvec <= 0)
		return 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.
	 */
	if (n_ports != 1 && nvec >= n_ports)
		return -ENOSYS;

	/*
	 * 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)
		return rc;

	return 1;
}

Note that the check changed to n_ports != 1 to also cover the case n_ports == 0 which should return -ENOSYS.

Quoted 2 lines

Also, shouldn't we be printing a warning message here explaining why probing is failing?

I didn't want to print a warning in case -ENOSYS for backward compatability. Only if msi-x code fails there is a message, see __ahci_init_interrupts(). In any other case the behaviour is as before, thus no message is printed.

Quoted 6 lines

drivers/ata/ahci.c static inline void ahci_gtf_filter_workaround(struct ata_host *host) · line 1226
Quoted 3 lines
drivers/ata/ahci.c +3 -0
+
+   /* only enable the first entry (entry.entry = 0) */
+   rc = pci_enable_msix_exact(pdev, &entry, 1);
So, enabling the first msix works if nvec > 1 && nvec < n_ports but not if nvec >= n_ports?

For n_ports > 1 && nvec >= n_ports we need to assume per-port interrupts. There are enough vectors for all ports then.

Quoted 49 lines

drivers/ata/ahci.c +39 -0

+	if (rc < 0)
+		return rc;
+
+	return 1;
+}
+
+static int __ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
+				  struct ahci_host_priv *hpriv)
 {
 	int rc, nvec;
 
+	nvec = ahci_init_msix(pdev, n_ports, hpriv);
+	if (nvec > 0)
+		return nvec;
+
+	if (nvec && nvec != -ENOSYS)
+		dev_err(&pdev->dev, "failed to enable MSI-X: %d", nvec);
+
 	if (hpriv->flags & AHCI_HFLAG_NO_MSI)
 		goto intx;

@@ -1250,6 +1285,35 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
 	return 0;
 }
 
+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_interrupts(struct pci_dev *pdev, unsigned int n_ports,
+                           struct ahci_host_priv *hpriv)
+{
+   struct msi_desc *desc;
+
+   __ahci_init_interrupts(pdev, n_ports, hpriv);
+
+   if (!pdev->msix_enabled)
+           return pdev->irq;
+
+   desc = msix_get_desc(pdev, 0);  /* first entry */
+   if (!desc)

Quoted 8 lines

drivers/ata/ahci.c static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports, · line 1315
Quoted 4 lines
drivers/ata/ahci.c +4 -0
+           return -ENODEV;
+
+   return desc->irq;
+}
Can we please do this properly? We should be able to move port priv allocation to host allocaotion time and add and use pp->irq instead, right?

I started working implementing this.

Will send an updated patch set once finished.

Thanks,

-Robert

Robert Richter

2015-05-12 11:46 UTC #3 lore raw

Tejun,

Quoted 21 lines · Robert Richter

Quoted 12 lines
Quoted 12 lines
drivers/ata/ahci.c +12 -0
+static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
+                           struct ahci_host_priv *hpriv)
+{
+   struct msi_desc *desc;
+
+   __ahci_init_interrupts(pdev, n_ports, hpriv);
+
+   if (!pdev->msix_enabled)
+           return pdev->irq;
+
+   desc = msix_get_desc(pdev, 0);  /* first entry */
+   if (!desc)
Quoted 8 lines
drivers/ata/ahci.c static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports, · line 1315
Quoted 4 lines
drivers/ata/ahci.c +4 -0
+           return -ENODEV;
+
+   return desc->irq;
+}
Can we please do this properly? We should be able to move port priv allocation to host allocaotion time and add and use pp->irq instead, right?

Quoted 3 lines

I started working implementing this.
Will send an updated patch set once finished.

A couple of questions here.

If we store the irq for each port separate in host->ports[i]->irq, then there are duplicates for !AHCI_HFLAG_MULTI_MSI. We can not iterate over all ports then to initialize the interrupt. Instead we need to check for !AHCI_HFLAG_MULTI_MSI and store in that case the irq in host->irq. This would avoid initializing duplicates and makes host->ports[i]->irq only useful for multi-msi. Right now multi-msi uses a base irq + index to register all irqs. This makes host->ports[i]->irq obsolete at all.

Now, adding host->irq would change the function interface of ahci_host_activate() to:

int ahci_host_activate(struct ata_host *host, struct scsi_host_template *sht);

I don't think this is worth the effort as all internal and external drivers need to be changed basically from:

ahci_host_activate(host, irq, &ahci_sht);

to:

host->irq = irq;
ahci_host_activate(host, &ahci_sht);

This looks not very useful to do. Since irq is used only a single time, there is no reason to store it in the host's data structure. It also makes the interface more error prone since host->irq might not be setup. Apart from that there is an abi change.

I agree that we will need the implemention of host->ports[i]->irq for the case there irqs are no longer in sequential order as this might be the case for per-port msi-x interrupts. But this is not the focus of my implementation and as long there is no hardware for this available, it wouldn't make sense to implement this at all.

So how to proceed? I could send you patches that implement host->irq for a single per-host interrupt, and also one that reworks multi-port interrupts to use host->ports[i]->irq. But I don't see any benefit here. That said, I would better keep my patch here as it is. That do you think?

Thanks,

-Robert

Tejun Heo

2015-05-13 14:39 UTC #4 lore raw

Hello,

Quoted 5 lines · Robert Richter

I don't think this is worth the effort as all internal and external drivers need to be changed basically from:
ahci_host_activate(host, irq, &ahci_sht);

Quoted 7 lines

to:
host->irq = irq;
ahci_host_activate(host, &ahci_sht);
This looks not very useful to do. Since irq is used only a single time, there is no reason to store it in the host's data structure. It

Doesn't really matter tho.

Quoted 2 lines

also makes the interface more error prone since host->irq might not be setup. Apart from that there is an abi change.

But large part of @host needs to be initialized before activation. I don't think moving irq to that pool changes much if anything.

Quoted 5 lines

I agree that we will need the implemention of host->ports[i]->irq for the case there irqs are no longer in sequential order as this might be the case for per-port msi-x interrupts. But this is not the focus of my implementation and as long there is no hardware for this available, it wouldn't make sense to implement this at all.

Why are we doing msix at all? I don't get it.

Quoted 5 lines

So how to proceed? I could send you patches that implement host->irq for a single per-host interrupt, and also one that reworks multi-port interrupts to use host->ports[i]->irq. But I don't see any benefit here. That said, I would better keep my patch here as it is. That do you think?

Let's start with why we're doing this in the first place.

Thanks.

Signature · 1 line

tejun

Tejun Heo

2015-05-13 14:33 UTC #5 lore raw

In reply to Robert Richter

Hello, Robert.

Quoted 15 lines · Robert Richter

static int ahci_init_msix(struct pci_dev *pdev, unsigned int n_ports,
struct ahci_host_priv *hpriv)

{
	int rc, nvec;
	struct msix_entry entry = {};

	/* check if msix is supported */
	nvec = pci_msix_vec_count(pdev);
	if (nvec <= 0)
		return 0;

	/*
	 * Per-port msix interrupts are not supported. Assume single
	 * port interrupts for:
	 *

Quoted 8 lines

	 *  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)
		return -ENOSYS;

Why are failing the whole thing when nvec >= n_ports? Can't we just print some warning and configure it for single interrupt mode?

Quoted 7 lines

Quoted 2 lines
Also, shouldn't we be printing a warning message here explaining why probing is failing?
I didn't want to print a warning in case -ENOSYS for backward compatability. Only if msi-x code fails there is a message, see __ahci_init_interrupts(). In any other case the behaviour is as before, thus no message is printed.

I'm confused here. Why are we implementing msix support at all if it only support single interrupt mode? I kinda assumed that that was because you're trying to support a controller which does only msix, no? At any rate, I don't think it's wrong to print an informational / warning message when a controller declares msix support but has wacko parameters.

Quoted 9 lines

Quoted 6 lines
drivers/ata/ahci.c static inline void ahci_gtf_filter_workaround(struct ata_host *host) · line 1226
Quoted 3 lines
drivers/ata/ahci.c +3 -0
+
+   /* only enable the first entry (entry.entry = 0) */
+   rc = pci_enable_msix_exact(pdev, &entry, 1);
So, enabling the first msix works if nvec > 1 && nvec < n_ports but not if nvec >= n_ports?
For n_ports > 1 && nvec >= n_ports we need to assume per-port interrupts. There are enough vectors for all ports then.

Again, and we fail irq init in that case?

Thanks.

Signature · 1 line

tejun

Robert Richter

2015-05-13 17:28 UTC #6 lore raw

Tejun,

Quoted 8 lines · Tejun Heo

Quoted 7 lines · Robert Richter
I don't think this is worth the effort as all internal and external drivers need to be changed basically from:
ahci_host_activate(host, irq, &ahci_sht);
to:

Quoted 7 lines

Quoted 5 lines
host->irq = irq;
ahci_host_activate(host, &ahci_sht);
This looks not very useful to do. Since irq is used only a single time, there is no reason to store it in the host's data structure. It
Doesn't really matter tho.

Since ahci_host_activate() is EXPORT_SYMBOL_GPL I really have concerns changing the i/f. But I will send you a patch for this.

Quoted 6 lines

Quoted 2 lines
also makes the interface more error prone since host->irq might not be setup. Apart from that there is an abi change.
But large part of @host needs to be initialized before activation. I don't think moving irq to that pool changes much if anything.

Quoted 7 lines

Quoted 5 lines
I agree that we will need the implemention of host->ports[i]->irq for the case there irqs are no longer in sequential order as this might be the case for per-port msi-x interrupts. But this is not the focus of my implementation and as long there is no hardware for this available, it wouldn't make sense to implement this at all.
Why are we doing msix at all? I don't get it.

See below.

Quoted 7 lines

Quoted 5 lines
So how to proceed? I could send you patches that implement host->irq for a single per-host interrupt, and also one that reworks multi-port interrupts to use host->ports[i]->irq. But I don't see any benefit here. That said, I would better keep my patch here as it is. That do you think?
Let's start with why we're doing this in the first place.

Right, the sata controller is connected to a pci ecam controller, both are on an SoC together with the processor. There are no external pci ports for the connection of external devices. Since all pci devices on the chip support msi-x, the controller is only capable to handle this and not INTx nor MSI. So for enabling of the sata hc we need msix support.

-Robert

Tejun Heo

2015-05-13 17:46 UTC #7 lore raw

Hello, Robert.

Quoted 7 lines · Robert Richter

Quoted 4 lines
Quoted 2 lines
This looks not very useful to do. Since irq is used only a single time, there is no reason to store it in the host's data structure. It
Doesn't really matter tho.
Since ahci_host_activate() is EXPORT_SYMBOL_GPL I really have concerns changing the i/f. But I will send you a patch for this.

It doesn't matter. Please go ahead and change it.

Quoted 8 lines

Quoted 1 line
Let's start with why we're doing this in the first place.
Right, the sata controller is connected to a pci ecam controller, both are on an SoC together with the processor. There are no external pci ports for the connection of external devices. Since all pci devices on the chip support msi-x, the controller is only capable to handle this and not INTx nor MSI. So for enabling of the sata hc we need msix support.

I see. If you can get hold of an ahci controller which actually can do multi-irq msix, it'd be the best. If not, let's make it super clear that this is a special case and use it as the last resort (which also clers up the warning issue).

Thanks.

Signature · 1 line

tejun

Robert Richter

2015-05-13 18:07 UTC #8 lore raw

Quoted 6 lines · Tejun Heo

Hello, Robert.
Quoted 3 lines · Robert Richter
Quoted 3 lines
Quoted 2 lines
This looks not very useful to do. Since irq is used only a single time, there is no reason to store it in the host's data structure. It

Quoted 6 lines

Quoted 4 lines
Quoted 1 line
Doesn't really matter tho.
Since ahci_host_activate() is EXPORT_SYMBOL_GPL I really have concerns changing the i/f. But I will send you a patch for this.
It doesn't matter. Please go ahead and change it.

Ok, np.

Quoted 9 lines

Quoted 8 lines
Quoted 1 line
Let's start with why we're doing this in the first place.
Right, the sata controller is connected to a pci ecam controller, both are on an SoC together with the processor. There are no external pci ports for the connection of external devices. Since all pci devices on the chip support msi-x, the controller is only capable to handle this and not INTx nor MSI. So for enabling of the sata hc we need msix support.

Quoted 4 lines

I see. If you can get hold of an ahci controller which actually can do multi-irq msix, it'd be the best. If not, let's make it super clear that this is a special case and use it as the last resort (which also clers up the warning issue).

This sounds good.

The device actually supports multi-irq msix, a single mode interrupt + additional interrupts for error or hotplug handling. But I don't have hardware for a per-port msix host controller.

So I will move msix after msi then and mark it as a special case if msi is not supported. I don't want to move it after intx since this is the fallback if nothing else works, there is no explicit check for intx, I even don't know if that is possible at all.

Thanks,

-Robert

Tejun Heo

2015-05-13 18:10 UTC #9 lore raw

Hello,

Quoted 4 lines · Robert Richter

So I will move msix after msi then and mark it as a special case if msi is not supported. I don't want to move it after intx since this is the fallback if nothing else works, there is no explicit check for intx, I even don't know if that is possible at all.

Sure, that sounds good to me.

Thanks.

Signature · 1 line

tejun

Alexander Gordeev

2015-05-17 07:19 UTC #10 lore raw

In reply to Robert Richter

Quoted 10 lines · Robert Richter

From: Robert Richter <[email protected]>
This patch adds generic support for MSI-X interrupts to the SATA PCI driver. Only single interrupt support is implemented. Thus, per-port interrupts can not yet be enabled.
The driver now checks the device for the existence of MSI-X and tries to enable the interrupt. Otherwise, if a device is not MSI-X capable, the initialization is skipped and MSI or intx interrupts are configured.

Hi Robert,

Sorry for jumping in late.

I have took a cursory look at your code and have a major concern.

You enable MSI-X for a single chip, but introduce a change to the generic AHCI code.

In general MSI-X case, what makes you believe that IRQ vectors are assigned continuously?

(Interface ahci_host_activate() kinda expects a contiguous vector range, but MSI-X does not guarantee that at all).

Thanks!

Quoted 186 lines

This patch also enables AHCI for Cavium Thunder SoCs that uses MSI-X.

v2:
* determine irq vector from pci_dev->msi_list

Based on a patch from Sunil Goutham <[email protected]>.

Signed-off-by Robert Richter <[email protected]>

---
 arch/arm64/Kconfig |  1 +
 drivers/ata/ahci.c | 79 ++++++++++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 75 insertions(+), 5 deletions(-)

arch/arm64/Kconfig +1 -0

index b1f9a20a3677..2d95bbc0557e 100644
@@ -157,6 +157,7 @@ config ARCH_THUNDER
 	bool "Cavium Inc. Thunder SoC Family"
 	help
 	  This enables support for Cavium's Thunder Family of SoCs.
+	select SATA_AHCI
 
 config ARCH_VEXPRESS
 	bool "ARMv8 software model (Versatile Express)"

drivers/ata/ahci.c +74 -5

index 49f1e6890587..36c88f6d8490 100644
@@ -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,
 };

@@ -500,6 +502,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 },

@@ -1202,11 +1207,41 @@ 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_msix(struct pci_dev *pdev, unsigned int n_ports,
+			  struct ahci_host_priv *hpriv)
+{
+	int rc, nvec;
+	struct msix_entry entry = {};
+
+	/* check if msix is supported */
+	nvec = pci_msix_vec_count(pdev);
+	if (nvec <= 0)
+		return 0;
+
+	/* per-port msix interrupts are not supported */
+	if (n_ports > 1 && nvec >= n_ports)
+		return -ENOSYS;
+
+	/* only enable the first entry (entry.entry = 0) */
+	rc = pci_enable_msix_exact(pdev, &entry, 1);
+	if (rc < 0)
+		return rc;
+
+	return 1;
+}
+
+static int __ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
+				  struct ahci_host_priv *hpriv)
 {
 	int rc, nvec;
 
+	nvec = ahci_init_msix(pdev, n_ports, hpriv);
+	if (nvec > 0)
+		return nvec;
+
+	if (nvec && nvec != -ENOSYS)
+		dev_err(&pdev->dev, "failed to enable MSI-X: %d", nvec);
+
 	if (hpriv->flags & AHCI_HFLAG_NO_MSI)
 		goto intx;

@@ -1250,6 +1285,35 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
 	return 0;
 }
 
+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_interrupts(struct pci_dev *pdev, unsigned int n_ports,
+				struct ahci_host_priv *hpriv)
+{
+	struct msi_desc *desc;
+
+	__ahci_init_interrupts(pdev, n_ports, hpriv);
+
+	if (!pdev->msix_enabled)
+		return pdev->irq;
+
+	desc = msix_get_desc(pdev, 0);	/* first entry */
+	if (!desc)
+		return -ENODEV;
+
+	return desc->irq;
+}
+
 static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 {
 	unsigned int board_id = ent->driver_data;

@@ -1260,6 +1324,7 @@ static int ahci_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	struct ata_host *host;
 	int n_ports, i, rc;
 	int ahci_pci_bar = AHCI_PCI_BAR_STANDARD;
+	int irq;
 
 	VPRINTK("ENTER\n");

@@ -1285,11 +1350,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

@@ -1411,7 +1478,9 @@ 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);
+	irq = ahci_init_interrupts(pdev, n_ports, hpriv);
+	if (irq < 0)
+		return irq;
 
 	host = ata_host_alloc_pinfo(&pdev->dev, ppi, n_ports);
 	if (!host)

@@ -1463,7 +1532,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, irq, &ahci_sht);
 }
 
 module_pci_driver(ahci_pci_driver);

Signature · 7 lines

2.1.1

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to [email protected]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Signature · 3 lines

Regards,
Alexander Gordeev
[email protected]

Robert Richter

2015-05-18 08:06 UTC #11 lore raw

Quoted 8 lines · Alexander Gordeev

You enable MSI-X for a single chip, but introduce a change to the generic AHCI code.
In general MSI-X case, what makes you believe that IRQ vectors are assigned continuously?
(Interface ahci_host_activate() kinda expects a contiguous vector range, but MSI-X does not guarantee that at all).

That's the reason why I only enable single interrupt mode which our hardware supports.

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. Considering all this I think your concerns are addressed.

Also, the code can be easily extended for other devices and thus should be generic from the beginning.

-Robert