Re: [PATCH] ata: sata_mv: accept 1 or 2 resources in platform probe

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

Pre-existing issues:
- [High] Missing error check for port clock clk_prepare_enable() leads to unbalanced disable and potential hardware faults.
--

--- Patch [1]: [PATCH] ata: sata_mv: accept 1 or 2 resources in platform probe ---
commit 079963d555bbd8a5581f9c80568deb335571afa8
Author: Rosen Penev <[email protected]>

ata: sata_mv: accept 1 or 2 resources in platform probe

This commit modifies mv_platform_probe() to accept either 1 or 2 resources,
allowing the SATA driver to probe successfully on legacy Marvell
Orion/Kirkwood-style boards.

> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> index 41647a56a9f4e..365454390d7e0 100644
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c
> @@ -4027,7 +4027,7 @@ static int mv_platform_probe(struct platform_device *pdev)
>  	/*
>  	 * Simple resource validation ..
>  	 */
> -	if (unlikely(pdev->num_resources != 1)) {
> +	if (unlikely(pdev->num_resources != 1 && pdev->num_resources != 2)) {
>  		dev_err(&pdev->dev, "invalid number of resources\n");
>  		return -EINVAL;
>  	}

[Severity: High]
This is a pre-existing issue, but while reviewing this function I noticed
the return value of clk_prepare_enable() for port clocks is ignored further
down in mv_platform_probe():

drivers/ata/sata_mv.c:mv_platform_probe() {
...
        hpriv->port_clks[port] = clk_get(&pdev->dev, port_number);
        if (!IS_ERR(hpriv->port_clks[port]))
            clk_prepare_enable(hpriv->port_clks[port]);
...
}

Does this code risk causing a synchronous external abort on ARM systems if
the port clock fails to enable? If the driver proceeds with hardware
initialization while the IP block is unclocked, it could lead to a kernel
panic upon register access.

Additionally, if clk_prepare_enable() fails or another error occurs during
probe, the cleanup loop unconditionally attempts to disable and unprepare
the clock:

drivers/ata/sata_mv.c:mv_platform_probe() {
...
err:
...
    for (port = 0; port < hpriv->n_ports; port++) {
        if (!IS_ERR(hpriv->port_clks[port])) {
            clk_disable_unprepare(hpriv->port_clks[port]);
            clk_put(hpriv->port_clks[port]);
        }
...
}

Could this unbalanced disable cause a WARN_ON during the error path since
the clock was never successfully enabled?

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