[PATCH] ata: ahci: clear HOST_IRQ_STAT before the ports on Marvell 88SE61xx

[PATCH] ata: ahci: clear HOST_IRQ_STAT before the ports on Marvell 88SE61xx

[Hajo Noerenberg]

ahci_single_level_irq_intr() services the ports first and clears the global
HOST_IRQ_STAT afterwards, as recommended by AHCI 1.1 section 10.6.2.  The
Marvell 88SE6111/6121/6145 family stops reporting interrupts for a port when
HOST_IRQ_STAT is cleared while PxIS still holds bits: PxIS keeps its content,
HOST_IRQ_STAT reads back as 0, the port is never looked at again, and the
command in flight only ends in a timeout.

Measured on a Seagate Blackarmor NAS440 (Marvell 88F6281 Kirkwood, 88SE6121
rev B2 behind PCIe) by polling the AHCI registers from userspace while an
IDENTIFY was outstanding:

  t=303.046  irq count 127  PxIS 0x00000000  PxCI 0x00000001   IDENTIFY issued
  t=303.057  irq count 128  PxIS 0x00000020  PxCI 0x00000000   CI cleared, DPS
                                                               set, one
                                                               interrupt taken
             ... PxIS stays 0x00000020, HOST_IRQ_STAT stays 0 ...
  t~308.05                                                     qc timeout after
                                                               5000 msecs

The command had completed - PxCI was clear and PxIS had DPS set - so
ahci_qc_complete() would have completed it.  It never got the chance because
the handler read HOST_IRQ_STAT as 0 and returned IRQ_NONE.

Marvell's own driver for these chips clears the two registers in the opposite
order and says so ("clear global before channel"), and ahci_xgene handles its
broken edge latch the same way.  Since the reordering costs at most one
spurious interrupt per valid one on conforming controllers, do it in a private
interrupt handler selected for board_ahci_mv instead of changing libahci for
everyone.

With this applied, SATA-2 and SATA-3 disks work at 3.0 Gbps on the 88SE6121
without the drive-side 1.5 Gbps jumper that was needed before:

  WDC WD5000AADS-00S9B0  port 0  identified   7 ms after link up (never before)
  WDC WD3202ABYS-01B7A0  port 1  identified  28 ms after link up
  WDC WD30EFRX-68EUZN0   port 1  identified 200 ms after link up, 3 TB, HPA ok

Only the 88SE6121 was tested; board_ahci_mv also covers the 88SE6145, which
Marvell's driver treats identically.

Link: https://lore.kernel.org/linux-ide/db6b48b7-d69a-564b-24f0-75fbd6a9e543@noerenberg.de/
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216094
Signed-off-by: Hajo Noerenberg <hajo-linux-ide@noerenberg.de>

---
Damien,

this is the resolution of the thread we had in 2024 [1], sent as a new mail
rather than as a reply so that it does not get lost at the end of a long
thread.

To summarize the problem again: Gen2/3 HDDs only worked with the 88SE6121
controller in the Seagate Blackarmor NAS440 [2] when they were jumpered down
to Gen1 (1.5 Gbit/s).  That was unsatisfactory, because the very same drives
work at Gen2 speed (3 Gbit/s) without any jumper under the U-Boot bootloader.
With the patch below they work at 3 Gbit/s under Linux as well.

Both of your guesses back then turned out to be right:

> That is very odd. sata_lpm_ignore_phy_events() is only a couple of "if"
> statements and there are no register accesses in there. So if the few CPU
> cycles that takes make a difference, I would suspect that there is something
> odd going on with the marvell adapter interrupts.

> This hunk may not be directly related to the issue and commenting it out
> simply changes the timing making things better.

Exactly that.  On this machine CAP.SALP is 0, so link->lpm_policy never leaves
ATA_LPM_UNKNOWN and sata_lpm_ignore_phy_events() always returns false - the
hunk I had commented out was dead code there.  All it did was shorten the
window in which the interrupt goes missing.  Sorry for sending you down that
path.

How it was finally pinned down: I polled the AHCI registers from userspace
while an IDENTIFY was outstanding, which produced the trace in the commit
message above.  Seeing PxCI clear and PxIS set while HOST_IRQ_STAT read 0 was
the moment it became obvious.  Marvell's own driver for this chip family
("Thor", 1.0.0.9, 2007) then confirmed it: it acknowledges the two registers
in the opposite order from libahci, in three places, each with the comment

        /* clear global before channel */

and it does not use PxIS for completion at all, but derives it from
PORT_CMD_ISSUE / PORT_SCR_ACT.

Why U-Boot always worked, which puzzled me in 2024: it drives AHCI purely by
polling PORT_CMD_ISSUE in waiting_for_cmd_completed(), there is no
request_irq() anywhere in that driver, so a lost interrupt cannot affect it.
And why 2.x/3.x kernels often worked: the hazard is timing dependent and the
interrupt handler has grown over the years.  In the 3.2 log attached to the
bug the first IDENTIFY times out as well - only the retry after the hard reset
succeeds there.

One more thing worth recording, because bug 216094 spent years on it:
pci-mvebu is not involved.  The PCIe configuration is identical in the working
and the failing case - same MaxPayload, MaxReadReq and LnkCap, and the same
ABAR address with and without the DTB - there are no AER errors and PxSERR
stays 0.  So the INTx and AER work Pali did on pci-mvebu during that
discussion was never the missing piece.  libata was doing exactly what AHCI
1.1 prescribes all along; this controller simply does not behave that way.

Tested on 6.10-rc6 and on Debian's 6.1.0-50-marvell (armel), the latter across
a reboot with the module replaced, so the fix also works on the boot path that
had been broken since 3.16.

Not tested on the 88SE6145, which board_ahci_mv also covers; Marvell's driver
treats the family identically, but someone with that chip may want to confirm.

[1] https://lore.kernel.org/linux-ide/db6b48b7-d69a-564b-24f0-75fbd6a9e543@noerenberg.de/
[2] https://github.com/hn/seagate-blackarmor-nas

 drivers/ata/ahci.c | 49 +++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 49 insertions(+)

diff --git a/drivers/ata/ahci.c b/drivers/ata/ahci.c
--- a/drivers/ata/ahci.c
+++ b/drivers/ata/ahci.c
@@ -1618,6 +1618,51 @@
 }
 #endif

+/*
+ * The Marvell 88SE6111/6121/6145 ("Thor") family stops reporting interrupts
+ * for a port when HOST_IRQ_STAT is cleared while PxIS still holds bits: PxIS
+ * keeps its content, HOST_IRQ_STAT reads back as 0, the port is never looked
+ * at again and the command in flight only ends in a timeout.  On a 88SE6121
+ * this makes every SATA-2 or SATA-3 disk fail to IDENTIFY, while SATA-1 disks
+ * happen to win the race often enough to work.
+ *
+ * Clearing the host status before servicing the ports avoids it.  Marvell's
+ * own driver for these chips does the same and says so ("clear global before
+ * channel"), and ahci_xgene handles its broken edge latch the same way.  The
+ * price is at most one spurious interrupt per valid one, which is why this is
+ * not the generic behaviour - see AHCI 1.1 section 10.6.2.
+ *
+ * Link: https://bugzilla.kernel.org/show_bug.cgi?id=216094
+ */
+static irqreturn_t ahci_mv_irq_handler(int irq, void *dev_instance)
+{
+       struct ata_host *host = dev_instance;
+       struct ahci_host_priv *hpriv = host->private_data;
+       void __iomem *mmio = hpriv->mmio;
+       unsigned int rc;
+       u32 irq_stat, irq_masked;
+
+       irq_stat = readl(mmio + HOST_IRQ_STAT);
+       if (!irq_stat)
+               return IRQ_NONE;
+
+       irq_masked = irq_stat & hpriv->port_map;
+
+       spin_lock(&host->lock);
+
+       /*
+        * Use the unmasked value to clear the interrupt, as a spurious pending
+        * event on a dummy port might cause a screaming IRQ.
+        */
+       writel(irq_stat, mmio + HOST_IRQ_STAT);
+
+       rc = ahci_handle_port_intr(host, irq_masked);
+
+       spin_unlock(&host->lock);
+
+       return IRQ_RETVAL(rc);
+}
+
 static void ahci_remap_check(struct pci_dev *pdev, int bar,
                struct ahci_host_priv *hpriv)
 {
@@ -1878,6 +1923,10 @@
                return -ENOMEM;
        hpriv->flags |= (unsigned long)pi.private_data;

+       /* the Marvell "Thor" family needs HOST_IRQ_STAT cleared first */
+       if (board_id == board_ahci_mv)
+               hpriv->irq_handler = ahci_mv_irq_handler;
+
        /* MCP65 revision A1 and A2 can't do MSI */
        if (board_id == board_ahci_mcp65 &&
            (pdev->revision == 0xa1 || pdev->revision == 0xa2))

Re: [PATCH] ata: ahci: clear HOST_IRQ_STAT before the ports on Marvell 88SE61xx

[Damien Le Moal]

On 8/24/26 20:26, Hajo Noerenberg wrote:
> 
> ahci_single_level_irq_intr() services the ports first and clears the global
> HOST_IRQ_STAT afterwards, as recommended by AHCI 1.1 section 10.6.2.  The
> Marvell 88SE6111/6121/6145 family stops reporting interrupts for a port when
> HOST_IRQ_STAT is cleared while PxIS still holds bits: PxIS keeps its content,
> HOST_IRQ_STAT reads back as 0, the port is never looked at again, and the
> command in flight only ends in a timeout.
> 
> Measured on a Seagate Blackarmor NAS440 (Marvell 88F6281 Kirkwood, 88SE6121
> rev B2 behind PCIe) by polling the AHCI registers from userspace while an
> IDENTIFY was outstanding:

The commit title is not great as "before the ports" is not very clear.
What does "clear the ports" mean? Though I do not have a better alternative
title (yet).

[...]

> +static irqreturn_t ahci_mv_irq_handler(int irq, void *dev_instance)
> +{
> +       struct ata_host *host = dev_instance;
> +       struct ahci_host_priv *hpriv = host->private_data;
> +       void __iomem *mmio = hpriv->mmio;
> +       unsigned int rc;
> +       u32 irq_stat, irq_masked;
> +
> +       irq_stat = readl(mmio + HOST_IRQ_STAT);
> +       if (!irq_stat)
> +               return IRQ_NONE;
> +
> +       irq_masked = irq_stat & hpriv->port_map;
> +
> +       spin_lock(&host->lock);
> +
> +       /*
> +        * Use the unmasked value to clear the interrupt, as a spurious pending
> +        * event on a dummy port might cause a screaming IRQ.
> +        */
> +       writel(irq_stat, mmio + HOST_IRQ_STAT);
> +
> +       rc = ahci_handle_port_intr(host, irq_masked);
> +
> +       spin_unlock(&host->lock);
> +
> +       return IRQ_RETVAL(rc);
> +}

This looks very similar to what ahci_thunderx_irq_handler() does, minus the
loop. That loop does make sense to me though, so shouldn't this be the same?
Maybe try using that same function to see if it works?
If it does, we can rename that function to something more generic and reuse it.

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH] ata: ahci: clear HOST_IRQ_STAT before the ports on Marvell 88SE61xx

[Hajo Noerenberg]

Damien,

thank you for the quick review.

On 8/25/26 02:51, Damien Le Moal wrote:
> The commit title is not great as "before the ports" is not very clear.
> What does "clear the ports" mean? Though I do not have a better alternative
> title (yet).

Agreed, and "the ports" was meant to be the ports' PxIS registers, which the
title does not say.  Would this be better?

  ata: ahci: work around lost interrupts on Marvell 88SE61xx

That names the symptom rather than the mechanism.  Happy to take any other
suggestion.

> This looks very similar to what ahci_thunderx_irq_handler() does, minus the
> loop. That loop does make sense to me though, so shouldn't this be the same?
> Maybe try using that same function to see if it works?
> If it does, we can rename that function to something more generic and reuse it.

I tried exactly that: a build that registers the existing
ahci_thunderx_irq_handler() unchanged for board_ahci_mv, taken out of the
#ifdef CONFIG_ARM64, with a dev_info() so I could be certain which handler was
really active.  It does not work.  The controller fails exactly as it does
without any patch.

Both of the following are from one and the same boot of the same machine, on
the same drive in the same slot, 86 seconds apart.  I only swapped the module
with rmmod/insmod in between, nothing else changed:

  [  853.732695] ahci 0000:01:00.0: TEST BUILD: setting hpriv->irq_handler =
                                    ahci_thunderx_irq_handler
  [  854.155483] ata6: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
  [  859.313008] ata6.00: qc timeout after 5000 msecs (cmd 0xec)
  [  859.318692] ata6.00: failed to IDENTIFY (I/O error, err_mask=0x4)
  [  869.808790] ata6.00: qc timeout after 10000 msecs (cmd 0xec)
  [  869.814554] ata6.00: failed to IDENTIFY (I/O error, err_mask=0x4)
  [  869.820702] ata6: limiting SATA link speed to 1.5 Gbps
  [  901.808101] ata6.00: qc timeout after 30000 msecs (cmd 0xec)
  [  901.813867] ata6.00: failed to IDENTIFY (I/O error, err_mask=0x4)

  ... rmmod ahci ; insmod ahci.ko with the patch from this thread ...

  [  940.249339] ahci 0000:01:00.0: version 3.0-mv1
  [  940.661512] ata9: SATA link up 3.0 Gbps (SStatus 123 SControl 300)
  [  940.668285] ata9.00: HPA detected: current 5860531055, native 5860533168
  [  940.675076] ata9.00: ATA-9: WDC WD30EFRX-68EUZN0, 80.00A80, max UDMA/133
  [  940.694325] ata9.00: configured for UDMA/133

With the ThunderX handler the drive is given up after 48 seconds and three
timeouts.  With the patch from this thread it is identified 7 ms after link
up.

The reason is that the difference between the two handlers is not really the
loop -- it is the order of the two accesses, and the loop cannot substitute
for it:

  ThunderX:  rc = ahci_handle_port_intr(host, irq_masked);
             writel(irq_stat, mmio + HOST_IRQ_STAT);     /* IS after ports */

  Marvell:   writel(irq_stat, mmio + HOST_IRQ_STAT);     /* IS before ports */
             rc = ahci_handle_port_intr(host, irq_masked);

The 88SE61xx stops reflecting PxIS in IS once IS is written while PxIS still
holds bits.  With the ThunderX order, ahci_handle_port_intr() clears PxIS and
then IS is written; if the device raised a new event in that window, the write
to IS discards the pending indication, and -- this is the part the loop cannot
fix -- the controller never re-asserts it.  The re-read of IS therefore
returns 0, the loop exits, and the completion is lost.  The loop only catches
events that manage to set IS *after* the write, which is precisely the case
that is not broken here.  Clearing IS first means IS is never written while an
unserviced PxIS bit is standing.

So for this chip the ordering is load-bearing and the loop is not.

I think the function this patch resembles is not the ThunderX one but
xgene_ahci_irq_intr() in ahci_xgene.c, which does the same reordering for the
same kind of reason and says so:

  /*
   * HOST_IRQ_STAT behaves as edge triggered latch meaning that
   * it should be cleared before all the port events are cleared.
   */
  writel(irq_stat, mmio + HOST_IRQ_STAT);

  rc = xgene_ahci_handle_broken_edge_irq(host, irq_masked);

That is this patch, except that X-Gene needs a second workaround on top: its
wrapper re-reads HOST_IRQ_STAT and, if it reads back as zero, checks
PORT_IRQ_STAT by hand on the ports that IS did not flag and folds those in.
The 88SE61xx does not need that part -- I tried the equivalent rescan on its
own, without the reordering, and it did not help.  Marvell's own 6121 driver
also clears in this order, with the comment "clear global before channel".

As for sharing the code: a single handler that clears IS first *and* loops
would in theory serve both chips, but I would rather not propose it.  Each of
the three users needs something different -- X-Gene the port rescan, ThunderX
the loop, the 88SE61xx neither -- so the merged version would be a superset
that nobody actually wants, and I would be changing ThunderX behaviour on
hardware I do not have and cannot test.  Keeping the handlers separate, as in
this patch, costs about fifteen duplicated lines and no risk.

For the record, the fix has meanwhile carried a full mdadm RAID10 resync
across four 2 TB drives -- two of them on the 88SE6121, two on the Kirkwood
SoC controller -- with no errors, so it holds up under sustained parallel load
and not just during probe.  All of this on a machine whose SoC was released in
2008 -- eighteen years is a long time to wait for an interrupt :)

Thanks,
Hajo

Re: [PATCH] ata: ahci: clear HOST_IRQ_STAT before the ports on Marvell 88SE61xx

[Damien Le Moal]

On 8/25/26 16:58, Hajo Noerenberg wrote:
> 
> Damien,
> 
> thank you for the quick review.
> 
> On 8/25/26 02:51, Damien Le Moal wrote:
>> The commit title is not great as "before the ports" is not very clear.
>> What does "clear the ports" mean? Though I do not have a better alternative
>> title (yet).
> 
> Agreed, and "the ports" was meant to be the ports' PxIS registers, which the
> title does not say.  Would this be better?
> 
>   ata: ahci: work around lost interrupts on Marvell 88SE61xx

That is better!

> I tried exactly that: a build that registers the existing
> ahci_thunderx_irq_handler() unchanged for board_ahci_mv, taken out of the
> #ifdef CONFIG_ARM64, with a dev_info() so I could be certain which handler was
> really active.  It does not work.  The controller fails exactly as it does
> without any patch.

OK. Understood. Please resend the patch with the fixed-up commit title and we'll
apply it.

-- 
Damien Le Moal
Western Digital Research