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

[sashiko-bot@kernel.org]

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