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

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