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

[PATCH] 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>
---

This was discovered during review of an internal backport by some internal
LLM-based tool but the fix is entirely cooked up by me.

 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..c3df2fce4af7 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 err;
+	}
 
 	/* Enable SATA Interrupts */
 	sata_dwc_enable_interrupts(hsdev);
--
2.55.0

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

[sashiko-bot]

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

New issues:
- [Low] The patch introduces a `goto err;` statement in `sata_dwc_probe()`, but there is no label named `err:` defined in the function, resulting in a compilation error.

Pre-existing issues:
- [High] Missing cleanup of legacy DMA controller registration on error paths in `sata_dwc_probe()` leads to leaked DMA resources and a Use-After-Free in the dmaengine subsystem.
- [High] The error path unconditionally calls `phy_exit()` even if `phy_init()` fails, corrupting the PHY initialization reference count.
- [High] The allocated DMA channel is not released if `phy_power_on()` fails in `sata_dwc_port_start()`.
- [Medium] The host is registered and exposed to libata before hardware interrupts are enabled, causing a potential race condition with the asynchronous probe thread.
--

commit 3b0d1baec2d2645cdd4b2283d5c10d4927e37dd2
Author: Nikolay Borisov <nik.borisov@suse.com>
Subject: ata: sata_dwc_460ex: Don't enable interrupt if libATA core registration fails

This commit updates the probe function to fail the registration process if
ata_host_activate() fails. It aims to prevent interrupts from being enabled
when the device is not registered with the libATA core.

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 8e3fc713891af..c3df2fce4af73 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 err;

[Severity: Low]
Does this code attempt to jump to an undefined label? There is no label
named "err" in this function, which will cause a compilation failure.
Should this jump to "error_out" instead?

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

[Severity: Medium]
This is a pre-existing issue, but does this order of operations expose the
port to the asynchronous EH worker before the hardware interrupt mask is
configured?

ata_host_activate() registers the host with libata and schedules the
async_port_probe thread, which issues a softreset. If the async thread
issues a command relying on hardware interrupts before
sata_dwc_enable_interrupts() is called, could this result in a command
timeout during device detection?

[Severity: High]
This is a pre-existing issue, but does the error exit path in
sata_dwc_probe() miss cleaning up the legacy DMA controller?

If sata_dwc_dma_init_old() succeeds, the controller is registered via
dw_dma_probe(). If a subsequent initialization step fails and the code
jumps to the error path:

drivers/ata/sata_dwc_460ex.c:sata_dwc_probe()
    ...
error_out:
	phy_exit(hsdev->phy);
	return err;

Can bypassing sata_dwc_dma_exit_old() cause a use-after-free when devres
frees the memory but the dmaengine device remains registered?

[Severity: High]
This is a pre-existing issue, but does the error path unconditionally exit
the PHY even if phy_init() fails?

drivers/ata/sata_dwc_460ex.c:sata_dwc_probe()
	err = phy_init(hsdev->phy);
	if (err)
		goto error_out;
    ...
error_out:
	phy_exit(hsdev->phy);
	return err;

If phy_init() fails, it leaves phy->init_count at 0. Calling phy_exit() will
decrement the count to -1, which could lead to a permanent initialization
failure on subsequent probe attempts since phy_init() will see a non-zero
count and skip initialization.

[Severity: High]
This is a pre-existing issue, but is there a potential resource leak in
sata_dwc_port_start() if phy_power_on() fails?

drivers/ata/sata_dwc_460ex.c:sata_dwc_port_start()
	err = sata_dwc_dma_get_channel(hsdevp);
	if (err)
		goto CLEANUP_ALLOC;
    ...
	err = phy_power_on(hsdev->phy);
	if (err)
		goto CLEANUP_ALLOC;
    ...
CLEANUP_ALLOC:
	kfree(hsdevp);

If phy_power_on() fails, the code jumps to CLEANUP_ALLOC but misses a call
to dma_release_channel(), which could eventually exhaust all available DMA
channels.

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

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

[Nikolay Borisov]

On 8.09.26 г. 17:30 ч., sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> 
> New issues:
> - [Low] The patch introduces a `goto err;` statement in `sata_dwc_probe()`, but there is no label named `err:` defined in the function, resulting in a compilation error.

Fair point, however I'd like to hear from an expert whether enabling the 
interrupt without properly registering with libata core is fine or not? 
I.e whether this particular fix makes sense?

<snip>

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

[Niklas Cassel]

On Tue, Sep 08, 2026 at 05:34:51PM +0300, Nikolay Borisov wrote:
> 
> 
> On 8.09.26 г. 17:30 ч., sashiko-bot@kernel.org wrote:
> > Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
> > 
> > New issues:
> > - [Low] The patch introduces a `goto err;` statement in `sata_dwc_probe()`, but there is no label named `err:` defined in the function, resulting in a compilation error.
> 
> Fair point, however I'd like to hear from an expert whether enabling the
> interrupt without properly registering with libata core is fine or not? I.e
> whether this particular fix makes sense?

Your fix makes sense.


Kind regards,
Niklas