[PATCH v2] ata: sata_dwc_460ex: Don't enable interrupt if libATA core registration fails

[PATCH v2] ata: sata_dwc_460ex: Don't enable interrupt if libATA core registration fails

[Nikolay Borisov]

Recent 4bbc16a353a9 commit changed the timing when interrupts are
enabled. However it didn't account for the libATA registration step. So
it's possible that interrupts are enabled when the device is not
registered with libATA core.

Resolve the issue by failing the entire registration process in case of
failure to register with libATA.

Fixes: 4bbc16a353a9 ("ata: sata_dwc_460ex: enable SATA interrupts only after IRQ handler is registered")
Signed-off-by: Nikolay Borisov <nik.borisov@suse.com>
---

Changes since v1:
	* Added proper goto label

 drivers/ata/sata_dwc_460ex.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 8e3fc713891a..db39843c5f0f 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -1185,8 +1185,10 @@ static int sata_dwc_probe(struct platform_device *ofdev)
 	 * error_handler() to execute a dummy Softreset EH session
 	 */
 	err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
-	if (err)
+	if (err) {
 		dev_err(dev, "failed to activate host");
+		goto error_out;
+	}
 
 	/* Enable SATA Interrupts */
 	sata_dwc_enable_interrupts(hsdev);
--
2.55.0

Re: [PATCH v2] ata: sata_dwc_460ex: Don't enable interrupt if libATA core registration fails

[sashiko-bot]

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

New issues:
- [High] Inverted teardown sequence for the PHY on probe error path, calling phy_power_off() after phy_exit().

Pre-existing issues:
- [Critical] Missing cleanup of DMA resources on probe error paths leads to Use-After-Free of DW DMA controller structures.
--

commit fb665db1a3c329dd76a0b668e1948fec56aec15e
Author: Nikolay Borisov <nik.borisov@suse.com>

ata: sata_dwc_460ex: Don't enable interrupt if libATA core registration fails

Recent 4bbc16a353a9 commit changed the timing when interrupts are enabled.
However it didn't account for the libATA registration step. Resolve the
issue by failing the entire registration process in case of failure to
register with libATA.

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 8e3fc713891af..db39843c5f0fc 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
> @@ -1185,8 +1185,10 @@ static int sata_dwc_probe(struct platform_device *ofdev)
>  	 * error_handler() to execute a dummy Softreset EH session
>  	 */
>  	err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
> -	if (err)
> +	if (err) {
>  		dev_err(dev, "failed to activate host");
> +		goto error_out;

[Severity: High]
If ata_host_activate() fails, does this new failure path invert the PHY
teardown sequence?

When ata_host_activate() fails, it has already called ata_host_start(),
which powers on the PHY and registers a devres cleanup action for
ata_host_release(). Jumping to the error_out block executes a manual
phy_exit():

error_out:
	phy_exit(hsdev->phy);
	return err;

When devres later handles the cleanup, it cascades to sata_dwc_port_stop()
and executes phy_power_off(hsdev->phy) on the already-exited PHY. Could
this sequence violate the PHY API and potentially cause system lockups or
synchronous external aborts?

[Severity: Critical]
This is a pre-existing issue, but does this error path expose a missing
cleanup of legacy DMA resources?

When sata_dwc_probe() fails and jumps to error_out, the error handling
block skips calling sata_dwc_dma_exit_old(). Because
sata_dwc_dma_init_old() uses non-devm functions like request_irq() and
dma_async_device_register(), these globally registered resources remain
active and hold pointers to the freed devm-allocated structures.

If the shared IRQ fires or the DMA subsystem accesses the controller after
sata_dwc_probe() fails, could this lead to a use-after-free?

> +	}
>  
>  	/* Enable SATA Interrupts */
>  	sata_dwc_enable_interrupts(hsdev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909112426.811456-1-nik.borisov@suse.com?part=1

Re: [PATCH v2] ata: sata_dwc_460ex: Don't enable interrupt if libATA core registration fails

[Niklas Cassel]

On Wed, Sep 09, 2026 at 11:38:21AM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [High] Inverted teardown sequence for the PHY on probe error path, calling phy_power_off() after phy_exit().

Looking at ahci_platform.c:ahci_probe()

ahci_probe() calls
ahci_platform_enable_resources() which calls ahci_platform_enable_phys()
which calls
phy_init() and phy_power_on().

ahci_probe() later calls
ahci_platform_init_host() which calls ahci_host_activate()

if ahci_platform_init_host() fails, ahci_probe() calls

calls ahci_platform_disable_resources(), which calls
phy_power_off() and and phy_exit().


So the problem is that sata_dwc_460ex calls phy_power_on() in
->port_stop.
(And that it calls phy_power_on() in ->port_start()).


These calls should just be in sata_dwc_probe().
I don't see any other driver doing phy_power_on()/phy_power_off()
in ->port_start()/->port_stop().

It seems very wrong... Because sata_dwc_460ex will call phy_power_off()
for the same phy, when having multiple ports. phy_power_off() should
not be called multiple times on the same phy.


This driver is quite broken.

You could just send a patch that does:

diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
index 8e3fc713891a..0690e31719d2 100644
--- a/drivers/ata/sata_dwc_460ex.c
+++ b/drivers/ata/sata_dwc_460ex.c
@@ -1185,11 +1185,12 @@ static int sata_dwc_probe(struct platform_device *ofdev)
         * error_handler() to execute a dummy Softreset EH session
         */
        err = ata_host_activate(host, irq, sata_dwc_isr, 0, &sata_dwc_sht);
-       if (err)
+       if (err) {
                dev_err(dev, "failed to activate host");
-
-       /* Enable SATA Interrupts */
-       sata_dwc_enable_interrupts(hsdev);
+       } else {
+               /* Enable SATA Interrupts */
+               sata_dwc_enable_interrupts(hsdev);
+       }
        return 0;
 
 error_out:


Or if you want to clean this up, create a patch that moves
the phy_power_on() from ->port_start() to sata_dwc_probe()
and phy_power_off() from ->port_stop() to sata_dwc_probe().

Then you will be able to call goto error_out; on ata_host_activate() failure.


Kind regards,
Niklas