Re: [PATCHv2 5/8] ata: sata_dwc_460ex: fix data race on hsdev->sactive_issued in interrupt handler

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The NCQ tag completion logic contains an infinite loop due to a left shift instead of a right shift, which will cause a hard CPU hang inside a spinlock-protected hardirq context.
- [High] Unconditionally zeroing `hsdev->sactive_issued` on every interrupt destroys persistent NCQ state, breaking NCQ completion tracking.
- [High] The `clear_interrupt_bit()` function ignores the `bit` argument and inadvertently clears all pending hardware interrupts.
--

commit 2746dc992432ebc7a9253d28cee91b6dd821a51d
Author: Rosen Penev <[email protected]>

ata: sata_dwc_460ex: fix data race on hsdev->sactive_issued in interrupt handler
    
hsdev->sactive_issued is written locklessly in sata_dwc_isr() before
acquiring host->lock, while sata_dwc_qc_complete() performs a
read-modify-write on the same field under the lock.  This creates a
data race that can corrupt NCQ tag tracking state.
    
Move the zero assignment inside the critical section so all accesses
to sactive_issued are serialized by host->lock.

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 4fc22ce4bd9aa..9a4fb911d3215 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -470,9 +470,9 @@ static irqreturn_t sata_dwc_isr(int irq, void *dev_instance)
>  	int handled, port = 0;
>  	uint intpr, sactive, sactive2, tag_mask;
>  	struct sata_dwc_device_port *hsdevp;
> -	hsdev->sactive_issued = 0;
>  
>  	spin_lock_irqsave(&host->lock, flags);
> +	hsdev->sactive_issued = 0;

[Severity: High]
This is a pre-existing issue, but does unconditionally zeroing
hsdev->sactive_issued on every entry into sata_dwc_isr() destroy persistent
NCQ state?

It looks like this breaks NCQ completion tracking by erasing tags set during
SATA_DWC_INTPR_NEWFP handling.

Would this force a fallback to a non-standard NCQ completion path that handles
commands individually?

>  
>  	/* Read the interrupt register */
>  	intpr = sata_dwc_readl(&hsdev->sata_dwc_regs->intpr);

[Severity: Critical]
This is a pre-existing issue, but does the NCQ tag completion logic contain an
infinite loop when processing completions?

Inside the NCQ completion block of sata_dwc_isr():

        while (!(tag_mask & 0x00000001)) {
                tag++;
                tag_mask <<= 1;
        }

Because tag_mask is left-shifted, bit 0 will remain 0 and tag_mask eventually
evaluates to 0. At that point, !(0 & 1) is always true.

Would this cause the loop to spin forever inside a spinlock-protected hardirq
context?

[Severity: High]
This isn't a bug introduced by this patch, but does the clear_interrupt_bit()
function ignore the bit argument and inadvertently clear all pending hardware
interrupts?

drivers/ata/sata_dwc_460ex.c:clear_interrupt_bit()
        sata_dwc_writel(&hsdev->sata_dwc_regs->intpr,
                        sata_dwc_readl(&hsdev->sata_dwc_regs->intpr));

Since it reads the intpr register and writes the exact read value back,
assuming a standard Write-1-to-Clear (W1C) register model, would this clear
all pending interrupts?

Could hardware interrupts that occur simultaneously with NEWFP interrupts be
permanently lost?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1