Re: [PATCHv3 5/5] 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] Disabling hardware interrupts before `ata_host_detach()` prevents pending I/O and device unload commands from completing, resulting in command timeouts during teardown.
- [High] `sata_dwc_disable_interrupts()` is called in the remove path without synchronization, creating a race with the Error Handling (EH) thread that can cause interrupts to be incorrectly re-enabled.

Pre-existing issues:
- [High] Pre-existing race condition missing IRQ synchronization before freeing DMA and PHY resources during device removal, risking Use-After-Free.
- [High] `clear_interrupt_bit()` unintentionally clears all pending interrupts, causing lost interrupts due to a flawed read-modify-write on a W1C register.
--

--- Patch [1]: [PATCHv3 5/5] ata: sata_dwc_460ex: disable SATA interrupts on device removal ---
commit 65387714eba3b122c58a0cdaf1a76af59f78680c
Author: Rosen Penev <[email protected]>
Subject: 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() to prevent unhandled interrupt storms
during host teardown.

Fixes: 62936009f35a ("[libata] Add 460EX on-chip SATA driver, sata_dwc_460ex"

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 4fc22ce4bd9aa..7c858a7992c9c 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c

[ ... ]

> @@ -1217,6 +1225,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 this cause teardown commands to time out?

When sata_dwc_remove() calls ata_host_detach(), the SCSI devices are
detached, which typically issues teardown commands like SYNCHRONIZE CACHE.
Because interrupts are already masked by sata_dwc_disable_interrupts(), will
these commands fail to receive their completion IRQs and hang until they time
out?

[Severity: High]
Does masking interrupts here also create a race with the libata error handler?

If the teardown commands time out as mentioned above, the libata error
handling (EH) thread will execute. The EH thread calls sata_dwc_hardreset(),
which explicitly calls sata_dwc_enable_interrupts(). Could this inadvertently
re-enable interrupts while sata_dwc_remove() is still running, recreating the
interrupt storm this patch is trying to fix?

>  	ata_host_detach(host);
>  
>  	phy_exit(hsdev->phy);

[Severity: High]
This is a pre-existing issue, but does device removal leave a use-after-free
window?

In sata_dwc_remove(), the PHY and DMA resources are destroyed before the
devres-managed IRQ handler is unregistered. If an unexpected interrupt fires
and sata_dwc_isr() executes, could it access these freed resources before
devres cleans up the IRQ?

[ ... ]

While reviewing this driver, I noticed another pre-existing issue not touched
by this patch:

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));
}

[Severity: High]
This is a pre-existing issue, but does clear_interrupt_bit() unintentionally
clear all pending interrupts?

Looking at clear_interrupt_bit(), the function ignores its 'bit' argument and
reads then writes back the intpr register. Because intpr is a write-1-to-clear
(W1C) register, does writing back the read value clear every currently pending
interrupt, including those that have arrived since the ISR started but haven't
been processed yet?

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