Re: [PATCHv4 1/4] ata: sata_dwc_460ex: use platform_get_irq()

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

Pre-existing issues:
- [High] sata_dwc_probe() ignores the return value from ata_host_activate() and incorrectly returns 0 (success) on failure.
- [High] sata_dwc_probe() fails to clean up DMA resources initialized by sata_dwc_dma_init_old() on error paths, leading to a use-after-free.
- [High] Hardware interrupts are enabled early in probe without being disabled on error paths, potentially causing unhandled interrupt storms.
--

--- Patch [1]: [PATCHv4 1/4] ata: sata_dwc_460ex: use platform_get_irq() ---
commit c2c0e5be0a165f590877664b3703065d59cc686e
Author: Rosen Penev <[email protected]>

ata: sata_dwc_460ex: use platform_get_irq()

This commit replaces irq_of_parse_and_map() with platform_get_irq() in both
sata_dwc_dma_init_old() and sata_dwc_probe() to improve error reporting. It
also removes the unnecessary of_irq.h include and fixes an unused variable
when CONFIG_SATA_DWC_OLD_DMA is disabled.

> diff --git a/drivers/ata/sata_dwc_460ex.c b/drivers/ata/sata_dwc_460ex.c
> index 4fc22ce4bd9aa..092272753749c 100644
> --- a/drivers/ata/sata_dwc_460ex.c
> +++ b/drivers/ata/sata_dwc_460ex.c
[ ... ]
> @@ -1173,14 +1168,12 @@ static int sata_dwc_probe(struct platform_device *ofdev)
>  	sata_dwc_enable_interrupts(hsdev);
>  
>  	/* Get SATA interrupt number */
> -	irq = irq_of_parse_and_map(np, 0);
> -	if (!irq) {
> -		dev_err(dev, "no SATA DMA irq\n");
> -		return -ENODEV;
> -	}
> +	irq = platform_get_irq(ofdev, 0);
> +	if (irq < 0)
> +		return irq;

[Severity: High]
This isn't a bug introduced by this patch, but hardware interrupts are enabled
just before this by calling sata_dwc_enable_interrupts(). 

If platform_get_irq() fails and returns an error code like -EPROBE_DEFER, the
function returns early without masking the hardware interrupts again.

Could this lead to an unhandled interrupt storm since the hardware registers
are unmasked but no handler is registered yet?

>  
>  #ifdef CONFIG_SATA_DWC_OLD_DMA
> -	if (!of_property_present(np, "dmas")) {
> +	if (!of_property_present(dev->of_node, "dmas")) {
>  		err = sata_dwc_dma_init_old(ofdev, hsdev);
>  		if (err)
>  			return err;

[Severity: High]
This is a pre-existing issue, but looking further down in sata_dwc_probe(),
there is a potential cleanup issue on error paths:

	hsdev->phy = devm_phy_optional_get(dev, "sata-phy");
	if (IS_ERR(hsdev->phy))
		return PTR_ERR(hsdev->phy);

	err = phy_init(hsdev->phy);
	if (err)
		goto error_out;

If getting or initializing the PHY fails, the function exits without cleaning
up the DMA resources that were just initialized by sata_dwc_dma_init_old().

Because sata_dwc_dma_init_old() sets up the DMA engine, does returning here
leave the DMA subsystem with dangling pointers, leading to a use-after-free
when devres cleans up hsdev->dma?

[Severity: High]
This is another pre-existing issue further down in sata_dwc_probe(). The
return value from ata_host_activate() is logged but then discarded:

	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, the function unconditionally returns 0, meaning
the driver core considers the device successfully bound. 

When the driver is unbound later, scsi_remove_host() will be called on an
uninitialized SCSI host. Does this return value need to be propagated instead
of returning 0 to prevent a NULL pointer dereference during teardown?

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