Re: [PATCH v6 14/16] ASoC: rsnd: src: Add SRC reset and clock support for RZ/G3E

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] Missing error rollback in the probe loop leaves hardware permanently deasserted.
- [Low] Redundant devres allocations for a shared reset control inside a loop.
--

commit 987ecf3576f0681ad0809aeed224f14241a758ce
Author: John Madieu <john.madieu.xa@bp.renesas.com>

ASoC: rsnd: src: Add SRC reset and clock support for RZ/G3E

This commit introduces explicit Sampling Rate Converter Unit (SCU) reset and
clock management for the RZ/G3E SoC. It adds support for the shared SCU
reset controller used by all SRC modules and manages the SCU supply clock
with the same lifetime as the other SCU clocks.

> diff --git a/sound/soc/renesas/rcar/src.c b/sound/soc/renesas/rcar/src.c
> --- a/sound/soc/renesas/rcar/src.c
> +++ b/sound/soc/renesas/rcar/src.c

[ ... ]

> @@ -754,6 +792,16 @@ int rsnd_src_probe(struct rsnd_priv *priv)
>  			goto rsnd_src_probe_done;
>  		}
>  
> +		/*
> +		 * RZ/G3E uses a shared SCU reset controller for all SRC modules.
> +		 * R-Car platforms typically don't have SRC reset controls.
> +		 */
> +		rstc = devm_reset_control_get_optional_shared(dev, "scu");

Is it intended to fetch the shared reset control inside the loop?

Since it queries the parent device using a constant string, it resolves to
the exact same shared reset control on every iteration. Could this be moved
outside the loop to avoid duplicate devres allocations and repeated refcount
increments for the same resource?

The resulting single rstc instance could then be safely passed to
rsnd_mod_init() during each iteration, as the reset subsystem correctly
handles multiple deassertions on the same shared wrapper.

> +		if (IS_ERR(rstc)) {
> +			ret = PTR_ERR(rstc);
> +			goto rsnd_src_probe_done;
> +		}
> +
>  		clk = rsnd_devm_clk_get_indexed(dev, SRC_NAME, i);
>  		if (IS_ERR(clk)) {
>  			ret = PTR_ERR(clk);
>  			goto rsnd_src_probe_done;
>  		}

Does this error path leave the hardware permanently deasserted for modules
initialized in previous iterations?

When an error occurs here, the function returns without rolling back the
modules successfully initialized earlier in the loop. rsnd_mod_init()
explicitly deasserts the reset control:

rsnd_mod_init()
    ...
    ret = reset_control_deassert(rstc);
    ...

Since devres only drops object references via reset_control_put() and does
not assert the reset line on failure, the hardware reset might remain
permanently deasserted.

While the failure to unwind the clock prepare count on this error path appears
to be a pre-existing condition, the reset deassert state leak is introduced
here. Could an explicit rollback be added to handle this?

>  		ret = rsnd_mod_init(priv, rsnd_mod_get(src),
> -				    &rsnd_src_ops, clk, NULL, RSND_MOD_SRC, i);
> +				    &rsnd_src_ops, clk, rstc, RSND_MOD_SRC, i);
>  		if (ret)
>  			goto rsnd_src_probe_done;

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