Re: [PATCH] ata: sata_mv: use devm clock helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Direct returns inside the per-port initialization loop bypass the `err` cleanup block, leaking the power state of previously initialized PHYs.
--

commit 6348b987b12e84df8076e5646462dde25b2d8234
Author: Rosen Penev <rosenp@gmail.com>

ata: sata_mv: use devm clock helpers

This commit replaces manual clock management with devm_clk_get_optional_enabled
for the main and per-port clocks. The commit message states the err label is
retained for phy_power_off cleanup.

> diff --git a/drivers/ata/sata_mv.c b/drivers/ata/sata_mv.c
> --- a/drivers/ata/sata_mv.c
> +++ b/drivers/ata/sata_mv.c

[ ... ]

> @@ -4091,21 +4091,16 @@ static int mv_platform_probe(struct platform_device *pdev)

[ ... ]

>  	for (port = 0; port < n_ports; port++) {
>  		char port_number[16];
>  		sprintf(port_number, "%d", port);
> -		hpriv->port_clks[port] = clk_get(&pdev->dev, port_number);
> -		if (!IS_ERR(hpriv->port_clks[port]))
> -			clk_prepare_enable(hpriv->port_clks[port]);
> +		hpriv->port_clks[port] = devm_clk_get_optional_enabled(&pdev->dev, port_number);
> +		if (IS_ERR(hpriv->port_clks[port]))
> +			return PTR_ERR(hpriv->port_clks[port]);

[Severity: High]
Does this direct return skip the cleanup for PHYs powered on during previous
loop iterations?

If mv_platform_probe() fails to get the port clock on a subsequent port
(port > 0), returning directly bypasses the err label where phy_power_off()
is called. This could leave the earlier PHYs powered on.

>  		sprintf(port_number, "port%d", port);
>  		hpriv->port_phys[port] = devm_phy_optional_get(&pdev->dev,
> @@ -4118,9 +4113,9 @@ static int mv_platform_probe(struct platform_device *pdev)
>  
>  			/* Cleanup only the initialized ports */
>  			hpriv->n_ports = port;
> -			goto err;
> -		} else
> -			phy_power_on(hpriv->port_phys[port]);
> +			return rc;
> +		}

[Severity: High]
Is it intentional to replace the goto err with a direct return here?

The original code used goto err to ensure phy_power_off() would be called for
previously initialized ports when devm_phy_optional_get() failed. By
returning directly, could this leak the power state if probe is deferred
or fails for a later port?

> +		phy_power_on(hpriv->port_phys[port]);
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260610063512.109464-1-rosenp@gmail.com?part=1