Re: [PATCH] ata: sata_mv: Use platform_get_irq() to get interrupt

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

New issues:
- [Low] The `if (!irq)` check after calling `platform_get_irq()` is dead code.

Pre-existing issues:
- [High] The sanity check `pdev->num_resources != 1` unconditionally breaks probing for legacy non-DT platforms.
- [High] Unconditional dereference of `mv_platform_data` for non-DT platform devices.
- [High] Port clock reference and enable state are leaked if PHY initialization fails.
--

--- Patch [1]: [PATCH] ata: sata_mv: Use platform_get_irq() to get interrupt ---
commit f1861b64daa8effebc2bb7261bbf40da10b32a2b
Author: Rosen Penev <[email protected]>

ata: sata_mv: Use platform_get_irq() to get interrupt

This commit simplifies the driver's probe logic by using platform_get_irq()
unconditionally to fetch the interrupt. It replaces the direct use of
irq_of_parse_and_map() on DT-based systems.

> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 41647a56a9f4e..b109eb4ef995d 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -4054,13 +4054,11 @@ static int mv_platform_probe(struct platform_device *pdev)
>  				n_ports);
>  			return -EINVAL;
>  		}
> -
> -		irq = irq_of_parse_and_map(pdev->dev.of_node, 0);
>  	} else {
>  		mv_platform_data = dev_get_platdata(&pdev->dev);
>  		n_ports = mv_platform_data->n_ports;

[Severity: High]
This isn't a bug introduced by this patch, but could mv_platform_data be NULL
here in mv_platform_probe()? If a non-DT device is probed without valid
platform data (e.g., bound manually via sysfs), dereferencing it
unconditionally would cause a NULL pointer dereference.

> -		irq = platform_get_irq(pdev, 0);
>  	}
> +	irq = platform_get_irq(pdev, 0);
>  	if (irq < 0)
>  		return irq;
>  	if (!irq)

[Severity: Low]
Since platform_get_irq() returns negative error codes on failure and intercepts
0 internally to return -EINVAL, is the if (!irq) check in mv_platform_probe()
now dead code?

>  		return -EINVAL;

While reviewing this probe function, I noticed a couple of other
pre-existing issues.

[Severity: High]
This is a pre-existing issue, but I am wondering if this sanity check
unconditionally breaks probing for legacy non-DT platforms:

drivers/ata/sata_mv.c:mv_platform_probe() {
	/*
	 * Simple resource validation ..
	 */
	if (unlikely(pdev->num_resources != 1)) {
		dev_err(&pdev->dev, "invalid number of resources\n");
		return -EINVAL;
	}
...
}

Legacy board files initializing the sata_mv device add both an IORESOURCE_MEM
and an IORESOURCE_IRQ resource, making num_resources == 2. It looks like this
would cause the probe to immediately abort with -EINVAL.

[Severity: High]
This is another pre-existing issue, but it appears that the clock reference
and enable state might be leaked if PHY initialization fails:

drivers/ata/sata_mv.c:mv_platform_probe() {
...
		hpriv->port_phys[port] = devm_phy_optional_get(&pdev->dev,
							       port_number);
		if (IS_ERR(hpriv->port_phys[port])) {
			rc = PTR_ERR(hpriv->port_phys[port]);
			hpriv->port_phys[port] = NULL;
			if (rc != -EPROBE_DEFER)
				dev_warn(&pdev->dev, "error getting phy %d", rc);

			/* Cleanup only the initialized ports */
			hpriv->n_ports = port;
			goto err;
		}
...
}

If clk_get() and clk_prepare_enable() succeed for a port, but the subsequent
devm_phy_optional_get() fails (e.g., returning -EPROBE_DEFER), the error
handler sets hpriv->n_ports = port.

Since the cleanup loop uses a for-loop up to hpriv->n_ports, the current
port index is excluded. Would this bypass clk_disable_unprepare() and
clk_put() for the currently failing port?

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