2014-04-17 13:12:12

by Alexander Gordeev

[permalink] [raw]
Subject: [PATCH v2 0/2] ahci: Tweaks to the driver's PCI function

Changes since v1:
- ahci_is_mrsm() open-coded inside ahci_init_interrupt()
- changelog elaborated

Cc: [email protected]

Alexander Gordeev (2):
ahci: Ensure "MSI Revert to Single Message" mode is not enforced
ahci: Use pci_enable_msi_exact() instead of pci_enable_msi_range()

drivers/ata/ahci.c | 19 ++++++++++++++-----
drivers/ata/ahci.h | 1 +
2 files changed, 15 insertions(+), 5 deletions(-)

--
1.7.7.6


2014-04-17 13:12:16

by Alexander Gordeev

[permalink] [raw]
Subject: [PATCH v2 2/2] ahci: Use pci_enable_msi_exact() instead of pci_enable_msi_range()

The driver calls pci_enable_msi_range() function with the
range of [nvec..nvec] which what pci_enable_msi_exact()
function is for.

Signed-off-by: Alexander Gordeev <[email protected]>
Cc: [email protected]
---
drivers/ata/ahci.c | 8 ++++----
1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 649ac1c..0927d5f 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1166,7 +1166,7 @@ static inline void ahci_gtf_filter_workaround(struct ata_host *host)
static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
struct ahci_host_priv *hpriv)
{
- int nvec;
+ int rc, nvec;

if (hpriv->flags & AHCI_HFLAG_NO_MSI)
goto intx;
@@ -1183,10 +1183,10 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
if (nvec < n_ports)
goto single_msi;

- nvec = pci_enable_msi_range(pdev, nvec, nvec);
- if (nvec == -ENOSPC)
+ rc = pci_enable_msi_exact(pdev, nvec);
+ if (rc == -ENOSPC)
goto single_msi;
- else if (nvec < 0)
+ else if (rc < 0)
goto intx;

/*
--
1.7.7.6

2014-04-17 13:13:21

by Alexander Gordeev

[permalink] [raw]
Subject: [PATCH v2 1/2] ahci: Ensure "MSI Revert to Single Message" mode is not enforced

The AHCI specification allows hardware to choose to revert to
single MSI mode when fewer messages are allocated than requested.
Yet, at least ICH10 chipset reverts to single MSI mode even when
enough messages are allocated in some cases (see below).

This update forces the driver do not rely on initialization of
multiple MSIs mode alone and always check if "MSI Revert to
Single Message" (MRSM) mode was enforced by the controller and
fallback to the single MSI mode in case it did.

That prevents a situation when the driver configured multiple
per-port IRQ handlers, but the controller sends all port's
interrupts to a single IRQ, which could easily screw up the
interrupt handling and lead to delays and possibly crashes.

The fix was tested on a 6-port controller that successfully
reverted to the single MSI mode:

00:1f.2 SATA controller: Intel Corporation 82801JI (ICH10 Family) SATA
AHCI Controller (prog-if 01 [AHCI 1.0])
Subsystem: Super Micro Computer Inc Device 10a7
Flags: bus master, 66MHz, medium devsel, latency 0, IRQ 101
I/O ports at f110 [size=8]
I/O ports at f100 [size=4]
I/O ports at f0f0 [size=8]
I/O ports at f0e0 [size=4]
I/O ports at f020 [size=32]
Memory at fbf00000 (32-bit, non-prefetchable) [size=2K]
Capabilities: [80] MSI: Enable+ Count=1/16 Maskable- 64bit-
Capabilities: [70] Power Management version 3
Capabilities: [a8] SATA HBA v1.0
Capabilities: [b0] PCI Advanced Features
Kernel driver in use: ahci

With 6 ports just 8 MSI vectors should be enough, but the adapter
enforces the MRSM mode when less than 16 vectors are written to
the Multiple Messages Enable PCI register. I instigated MRSM mode
with the patch below:

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 1e493e6..14799f2 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1171,7 +1171,7 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
if (hpriv->flags & AHCI_HFLAG_NO_MSI)
goto intx;

- nvec = pci_msi_vec_count(pdev);
+ nvec = 8;
if (nvec < 0)
goto intx;

Signed-off-by: Alexander Gordeev <[email protected]>
Cc: [email protected]
---
drivers/ata/ahci.c | 11 ++++++++++-
drivers/ata/ahci.h | 1 +
2 files changed, 11 insertions(+), 1 deletions(-)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
index 5a0bf8e..649ac1c 100644
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1164,7 +1164,7 @@ 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)
+ struct ahci_host_priv *hpriv)
{
int nvec;

@@ -1189,6 +1189,15 @@ static int ahci_init_interrupts(struct pci_dev *pdev, unsigned int n_ports,
else if (nvec < 0)
goto intx;

+ /*
+ * Fallback to single MSI mode if the controller enforced MRSM mode
+ */
+ if (readl(hpriv->mmio + HOST_CTL) & HOST_MRSM) {
+ pci_disable_msi(pdev);
+ printk(KERN_INFO "ahci: MRSM is on, fallback to single MSI\n");
+ goto single_msi;
+ }
+
return nvec;

single_msi:
diff --git a/drivers/ata/ahci.h b/drivers/ata/ahci.h
index 51af275..b5eb886 100644
--- a/drivers/ata/ahci.h
+++ b/drivers/ata/ahci.h
@@ -94,6 +94,7 @@ enum {
/* HOST_CTL bits */
HOST_RESET = (1 << 0), /* reset controller; self-clear */
HOST_IRQ_EN = (1 << 1), /* global IRQ enable */
+ HOST_MRSM = (1 << 2), /* MSI Revert to Single Message */
HOST_AHCI_EN = (1 << 31), /* AHCI enabled */

/* HOST_CAP bits */
--
1.7.7.6

2014-04-17 13:28:15

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH v2 0/2] ahci: Tweaks to the driver's PCI function

On Thu, Apr 17, 2014 at 02:13:48PM +0200, Alexander Gordeev wrote:
> Changes since v1:
> - ahci_is_mrsm() open-coded inside ahci_init_interrupt()
> - changelog elaborated
>
> Cc: [email protected]
>
> Alexander Gordeev (2):
> ahci: Ensure "MSI Revert to Single Message" mode is not enforced
> ahci: Use pci_enable_msi_exact() instead of pci_enable_msi_range()
>
> drivers/ata/ahci.c | 19 ++++++++++++++-----
> drivers/ata/ahci.h | 1 +
> 2 files changed, 15 insertions(+), 5 deletions(-)

Applied to libata/for-3.15-fixes.

Thanks.

--
tejun

2014-04-17 13:34:44

by Alexander Gordeev

[permalink] [raw]
Subject: Re: [PATCH v2 0/2] ahci: Tweaks to the driver's PCI function

On Thu, Apr 17, 2014 at 09:28:06AM -0400, Tejun Heo wrote:
> On Thu, Apr 17, 2014 at 02:13:48PM +0200, Alexander Gordeev wrote:
> > Changes since v1:
> > - ahci_is_mrsm() open-coded inside ahci_init_interrupt()
> > - changelog elaborated
> >
> > Cc: [email protected]
> >
> > Alexander Gordeev (2):
> > ahci: Ensure "MSI Revert to Single Message" mode is not enforced
> > ahci: Use pci_enable_msi_exact() instead of pci_enable_msi_range()
> >
> > drivers/ata/ahci.c | 19 ++++++++++++++-----
> > drivers/ata/ahci.h | 1 +
> > 2 files changed, 15 insertions(+), 5 deletions(-)
>
> Applied to libata/for-3.15-fixes.

Oh-oh.
It seems the changelog's demo patch portion applied to the source.


> Thanks.
>
> --
> tejun

--
Regards,
Alexander Gordeev
[email protected]

2014-04-17 14:00:48

by Tejun Heo

[permalink] [raw]
Subject: Re: [PATCH v2 0/2] ahci: Tweaks to the driver's PCI function

On Thu, Apr 17, 2014 at 02:36:18PM +0200, Alexander Gordeev wrote:
> > Applied to libata/for-3.15-fixes.
>
> Oh-oh.
> It seems the changelog's demo patch portion applied to the source.

Urgh... fixed it. Please don't include proper patch in the
description. It'll mess up a lot of people's workflow.

Thanks.

--
tejun