| Message ID | 50ebd35a-086f-40fb-887e-576e36e7a2b8@noerenberg.de |
|---|---|
| State | New |
| Headers | show |
| Series | ata: ahci: clear HOST_IRQ_STAT before the ports on Marvell 88SE61xx | expand |
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, 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
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.
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))
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(+)