Re: [PATCHv2 6/8] ata: sata_dwc_460ex: disable SATA interrupts on device removal

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

New issues:
- [High] Masking hardware interrupts before calling ata_host_detach() breaks the SCSI teardown sequence, leading to command timeouts, shutdown hangs, and potentially unwritten disk caches.
- [Medium] Missing read-back to flush posted writes when disabling hardware interrupts in sata_dwc_disable_interrupts().

Pre-existing issues:
- [High] sata_dwc_probe() incorrectly returns 0 (success) even if ata_host_activate() fails, leading to unhandled error paths and crashes on remove.
- [High] Masking interrupts without calling synchronize_irq() allows an in-flight interrupt handler to execute concurrently with hardware teardown.
--

commit df81e2008992f864aa17e308c42b08931c2b48f5
Author: Rosen Penev <[email protected]>

ata: sata_dwc_460ex: disable SATA interrupts on device removal

This commit adds sata_dwc_disable_interrupts() and calls it at the beginning
of sata_dwc_remove(). It masks the controller's INTMR and ERRMR registers to
prevent unhandled interrupt storms during teardown.

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 4fc22ce4bd9aa..62580a58ad76e 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -762,6 +762,12 @@ static int sata_dwc_qc_complete(struct ata_port *ap, struct ata_queued_cmd *qc)
>  	return 0;
>  }
>  
> +static void sata_dwc_disable_interrupts(struct sata_dwc_device *hsdev)
> +{
> +	sata_dwc_writel(&hsdev->sata_dwc_regs->intmr, 0);
> +	sata_dwc_writel(&hsdev->sata_dwc_regs->errmr, 0);

[Severity: Medium]
Will these posted writes reach the hardware predictably before the CPU moves
on to tear down the device?

Since sata_dwc_writel() wraps writel_relaxed(), writes to the memory-mapped
registers over the system bus can be posted. Without a subsequent dummy read
to a controller register to flush them, the interrupt mask configuration might
be delayed in write buffers. This could allow an interrupt to fire during
teardown, which is exactly what this change is intended to prevent.

> +}
> +

[ ... ]

> @@ -1217,6 +1223,8 @@ static void sata_dwc_remove(struct platform_device *ofdev)
>  	struct ata_host *host = dev_get_drvdata(dev);
>  	struct sata_dwc_device *hsdev = host->private_data;
>  
> +	sata_dwc_disable_interrupts(hsdev);
> +

[Severity: High]
Does masking interrupts here break the SCSI teardown sequence?

When ata_host_detach() is called, it instructs the SCSI layer to issue cache
flush and spindown commands. If hardware interrupts are already masked, won't
these commands fail to complete and eventually time out? This could lead to a
system hang during device removal and potential data loss if caches are not
successfully flushed.

[Severity: High]
This isn't a bug introduced by this patch, but does the teardown sequence
leave a race window with the still-registered ISR?

The interrupt handler is registered using devm_request_irq() and is only
freed by devres after sata_dwc_remove() returns. While the new masking stops
new interrupts, it does not wait for an already-running ISR on another CPU to
finish. Since synchronize_irq() isn't called, could the ISR execute
concurrently with ata_host_detach(), phy_exit(), and sata_dwc_dma_exit_old(),
accessing freed memory or hardware in an invalid state?

[Severity: High]
This is a pre-existing issue, but can sata_dwc_remove() crash if the initial
device probe failed?

Looking at sata_dwc_probe():

    err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
    if (err)
        dev_err(dev, "failed to activate host");

    return 0;

If ata_host_activate() fails, probe still returns 0 (success). When the
driver is later unbound, sata_dwc_remove() will unconditionally call
ata_host_detach() and attempt to tear down host structures that were never
successfully initialized, potentially causing a NULL pointer dereference or
list corruption.

>  	ata_host_detach(host);

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