Re: [PATCH v6 12/16] ASoC: rsnd: adg: Add per-SSI ADG and SSIF supply clock management

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Bitwise OR error code corruption and unbalanced clock unprepare in rsnd_adg_clk_control()
- [High] Missing error handling cleanup in rsnd_adg_probe() leads to Use-After-Free
--

commit 96241f9dadb676a5e8a4489644874b25a85bb097
Author: John Madieu <john.madieu.xa@bp.renesas.com>
ASoC: rsnd: adg: Add per-SSI ADG and SSIF supply clock management

This commit introduces explicit clock management for SSI audio interfaces
on the RZ/G3E, which differs from previous R-Car generations. It adds
per-SSI ADG clocks and a shared SSIF supply clock, acquiring them via
optional APIs to remain transparent to other platforms.

> diff --git a/sound/soc/renesas/rcar/adg.c b/sound/soc/renesas/rcar/adg.c
> --- a/sound/soc/renesas/rcar/adg.c
> +++ b/sound/soc/renesas/rcar/adg.c
[ ... ]
> @@ -417,6 +448,29 @@ int rsnd_adg_clk_control(struct rsnd_priv *priv, int enable)
>  		}
>  	}
>  
> +	/*
> +	 * rsnd_adg_clk_enable() might return error (_disable() will not).
> +	 * We need to rollback in such case
> +	 */
> +	/*
> +	 * RZ/G3E per-SSI ADG and SSIF supply clocks.
> +	 *
> +	 * Follow the same style as for_each_rsnd_clkin() above: on enable,
> +	 * try to prepare every clock and accumulate the error. On disable
> +	 * (which is also used as the rollback path below), unprepare every
> +	 * clock. Absent optional clocks are NULL, for which clk_prepare()
> +	 * and clk_unprepare() are no-ops.
> +	 */
> +	if (enable) {
> +		for (i = 0; i < ADG_SSI_MAX; i++)
> +			ret |= clk_prepare(adg->clk_adg_ssi[i]);
> +		ret |= clk_prepare(adg->clk_ssif_supply);

Does using bitwise OR to accumulate error codes corrupt the returned error
value? clk_prepare() returns negative error codes like -ENOMEM, and
accumulating them with a bitwise OR could return a meaningless value to
the caller, breaking probe deferral.

Also, if one of these clocks fails to prepare, the loop does not break.
The subsequent rollback path will then unconditionally call clk_unprepare()
on all clocks. Does calling clk_unprepare() on a clock that failed to prepare
trigger a kernel warning and underflow its state?

Additionally, if the preceding for_each_rsnd_clkin loop failed, the ret
variable is already less than zero. This block will still execute because
enable is true, which will overwrite the valid error code from the first loop.

> +	} else {
> +		clk_unprepare(adg->clk_ssif_supply);
> +		for (i = 0; i < ADG_SSI_MAX; i++)
> +			clk_unprepare(adg->clk_adg_ssi[i]);
> +	}
> +
[ ... ]
> @@ -798,6 +877,11 @@ int rsnd_adg_probe(struct rsnd_priv *priv)
>  	if (ret)
>  		return ret;
>  
> +	/* RZ/G3E-specific: per-SSI ADG and SSIF supply clocks */
> +	ret = rsnd_adg_get_ssi_clks(priv);
> +	if (ret)
> +		return ret;

If rsnd_adg_get_ssi_clks() returns an error here (such as -EPROBE_DEFER),
does returning directly skip deregistering the clock provider?

Earlier in rsnd_adg_probe(), the clock provider is registered via
rsnd_adg_get_clkout() which ultimately adds the provider.

Since the memory for adg is managed by devm_kzalloc(), if this probe fails,
the memory is automatically freed. If the clock provider remains registered
with the subsystem, could this lead to a use-after-free when accessed?

> +
>  	ret = rsnd_adg_clk_enable(priv);
>  	if (ret)
>  		return ret;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512182631.3842065-1-john.madieu.xa@bp.renesas.com?part=12