Re: [PATCH 4/5] ata: sata_dwc_460ex: enable SATA interrupts only after IRQ handler is registered

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing SATA interrupt disable in remove path leads to potential interrupt storm.
- [High] Lifecycle inversion: `phy_power_off()` is executed on an already exited PHY during teardown.
- [High] Data race on `hsdev->sactive_issued` causing state corruption.
--

commit 6c390f90accf9771f2e071160279ee0671a1ecfe
Author: Rosen Penev <rosenp@gmail.com>

ata: sata_dwc_460ex: enable SATA interrupts only after IRQ handler is registered

This patch moves sata_dwc_enable_interrupts() after ata_host_activate()
in sata_dwc_probe(). This ensures that the controller's interrupt mask is
only enabled once the interrupt handler is successfully registered and the
core is fully initialized.

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 34d6d0534e308..fb6042b9ac273 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -1168,9 +1168,6 @@ static int sata_dwc_probe(struct platform_device *ofdev)
>  	/* Save dev for later use in dev_xxx() routines */
>  	hsdev->dev = dev;
>  
> -	/* Enable SATA Interrupts */
> -	sata_dwc_enable_interrupts(hsdev);
> -
>  	/* Get SATA interrupt number */
>  	irq = platform_get_irq(ofdev, 0);
>  	if (irq < 0)
> @@ -1204,6 +1201,8 @@ static int sata_dwc_probe(struct platform_device *ofdev)
>  		return err;
>  	}
>  
> +	/* Enable SATA Interrupts */
> +	sata_dwc_enable_interrupts(hsdev);
>  	return 0;
>  }

While reviewing the interrupt handling changes in sata_dwc_probe(), I
noticed a few other pre-existing issues in the driver that might be worth
addressing.

[Severity: High]
This isn't a bug introduced by this patch, but looking at sata_dwc_remove(),
is there a missing SATA interrupt disable call?

drivers/ata/sata_dwc_460ex.c:sata_dwc_remove() {
        ...
        ata_host_detach(host);
        
        phy_exit(hsdev->phy);
        ...
}

The driver detaches the host and exits the PHY, but fails to mask the
controller's interrupts (intmr and errmr registers). When remove() returns,
the driver core cleans up devres allocations, which unregisters the IRQ
handler.

If the hardware asserts an interrupt during teardown, can this lead to an
unhandled interrupt storm and potentially lock up the system or disable a
shared IRQ line?

[Severity: High]
This is a pre-existing issue, but does sata_dwc_remove() have a PHY lifecycle
inversion?

drivers/ata/sata_dwc_460ex.c:sata_dwc_remove() {
        ...
        phy_exit(hsdev->phy);
        ...
}

The driver explicitly calls phy_exit() here. However, phy_power_off() is
handled inside sata_dwc_port_stop(), which is executed later by devres
cleanup:

drivers/ata/sata_dwc_460ex.c:sata_dwc_port_stop() {
        ...
        phy_power_off(hsdev->phy);
        ...
}

Because ata_host_stop() is registered as a devres callback (via
ata_host_start()), it runs after sata_dwc_remove() returns. Does this result
in phy_power_off() operating on a PHY that has already been exited?

[Severity: High]
This isn't a bug introduced by this patch, but is there a data race on
hsdev->sactive_issued in sata_dwc_isr()?

drivers/ata/sata_dwc_460ex.c:sata_dwc_isr() {
        ...
        hsdev->sactive_issued = 0;
        
        spin_lock_irqsave(&host->lock, flags);
        ...
}

The assignment to hsdev->sactive_issued is executed locklessly before
acquiring host->lock. If a DMA completion interrupt fires concurrently on
another CPU, dma_dwc_xfer_done() acquires host->lock and calls
sata_dwc_qc_complete().

Since sata_dwc_qc_complete() performs a read-modify-write on this same
state under the lock, can this race corrupt the NCQ tag tracking state
and desynchronize the state machine?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260630202415.234463-1-rosenp@gmail.com?part=4