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

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/[email protected]/
Link: https://bugzilla.kernel.org/show_bug.cgi?id=216094
Signed-off-by: Hajo Noerenberg <[email protected]>

---
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/[email protected]/
[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))