Re: [PATCH v6 16/16] ASoC: rsnd: Add system suspend/resume support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Incorrect clock and reset sequencing in `rsnd_resume_clk_reset()` prevents proper module resume.
- [Medium] Silent failure on clock preparation and enable errors during system resume.
- [Medium] The `scu_supply` clock is not disabled during system suspend, causing a power leak.
--

commit c780dd50f4c396b19ac5b1cf3e1ec1ec5c422765
Author: John Madieu <john.madieu.xa@bp.renesas.com>
ASoC: rsnd: Add system suspend/resume support

This commit adds system suspend/resume support for the ASoC rsnd driver,
required for RZ/G3E platforms. The per-module suspend/resume logic is
distributed across the relevant component files.

> diff --git a/sound/soc/renesas/rcar/core.c b/sound/soc/renesas/rcar/core.c
> index 679c833fd0014..26cd908299e6d 100644
> --- a/sound/soc/renesas/rcar/core.c
> +++ b/sound/soc/renesas/rcar/core.c
[ ... ]
> @@ -2160,11 +2161,35 @@ static void rsnd_remove(struct platform_device *pdev)
>  		remove_func[i](priv);
>  }
>  
> +void rsnd_suspend_clk_reset(struct clk *clk, struct reset_control *rstc)
> +{
> +	clk_unprepare(clk);
> +	reset_control_assert(rstc);
> +}
> +
> +void rsnd_resume_clk_reset(struct clk *clk, struct reset_control *rstc)
> +{
> +	reset_control_deassert(rstc);
> +	clk_prepare(clk);

Does this sequence deassert the reset before the clock is active, and omit
clk_enable() entirely?

In rsnd_mod_init(), the driver establishes that the hardware requires the
module clock to be active before deasserting its reset:

	ret = clk_prepare_enable(clk);
	if (ret)
		return ret;

	ret = reset_control_deassert(rstc);

Will deasserting the reset while the clocks are stopped fail to propagate
synchronous resets for IP modules using this helper?

Also, does this code silently ignore the return value of clk_prepare()?

If a clock fails to initialize, the subsystem might pretend the resume
succeeded.

> +}
[ ... ]
> @@ -2173,7 +2198,21 @@ static int rsnd_resume(struct device *dev)
>  {
>  	struct rsnd_priv *priv = dev_get_drvdata(dev);
>  
> -	return rsnd_adg_clk_enable(priv);
> +	/*
> +	 * Same order as probe:
> +	 * DMA -> SSI -> SSIU -> SRC -> CTU -> MIX -> DVC -> ADG
> +	 */
> +	rsnd_dma_resume(priv);
[ ... ]
> +	rsnd_adg_clk_enable(priv);
> +
> +	return 0;

Does this code silently ignore the return value of rsnd_adg_clk_enable() and
unconditionally return 0?

The pre-patch code properly returned the error code.

>  }
>  
>  static const struct dev_pm_ops rsnd_pm_ops = {
> diff --git a/sound/soc/renesas/rcar/dma.c b/sound/soc/renesas/rcar/dma.c
> index 0bca0b3031918..0bf97a12a9a72 100644
> --- a/sound/soc/renesas/rcar/dma.c
> +++ b/sound/soc/renesas/rcar/dma.c
[ ... ]
> @@ -1035,3 +1035,25 @@ int rsnd_dma_probe(struct rsnd_priv *priv)
[ ... ]
> +void rsnd_dma_resume(struct rsnd_priv *priv)
> +{
> +	struct rsnd_dma_ctrl *dmac = rsnd_priv_to_dmac(priv);
> +
> +	if (dmac) {
> +		/* Clock must be stable before reset is deasserted */
> +		clk_prepare_enable(dmac->audmapp_clk);

Is it possible to check the return value of clk_prepare_enable() here?

If the clock fails to initialize during resume, this could silently fail.

> +		rsnd_resume_clk_reset(NULL, dmac->audmapp_rstc);
> +	}
> +}
> diff --git a/sound/soc/renesas/rcar/src.c b/sound/soc/renesas/rcar/src.c
> index 0237b5d2e79e6..a844255879787 100644
> --- a/sound/soc/renesas/rcar/src.c
> +++ b/sound/soc/renesas/rcar/src.c
[ ... ]
> @@ -834,3 +834,37 @@ void rsnd_src_remove(struct rsnd_priv *priv)
>  		rsnd_mod_quit(rsnd_mod_get(src));
>  	}
>  }
> +
> +void rsnd_src_suspend(struct rsnd_priv *priv)
> +{
> +	struct rsnd_src_ctrl *src_ctrl = rsnd_priv_to_src_ctrl(priv);
> +	struct rsnd_src *src;
> +	int i;
> +
> +	if (!src_ctrl)
> +		return;
> +
> +	for_each_rsnd_src(src, priv, i)
> +		rsnd_suspend_clk_reset(rsnd_mod_get(src)->clk,
> +				       rsnd_mod_get(src)->rstc);
> +
> +	clk_disable_unprepare(src_ctrl->scu_x2);
> +	clk_disable_unprepare(src_ctrl->scu);

Does this code miss disabling the scu_supply clock?

It appears to be enabled during probe alongside scu and scu_x2, but omitted
here.

Could this leave the supply clock enabled across a system suspend cycle?

> +}
> +
> +void rsnd_src_resume(struct rsnd_priv *priv)
> +{
> +	struct rsnd_src_ctrl *src_ctrl = rsnd_priv_to_src_ctrl(priv);
> +	struct rsnd_src *src;
> +	int i;
> +
> +	if (!src_ctrl)
> +		return;
> +
> +	clk_prepare_enable(src_ctrl->scu);
> +	clk_prepare_enable(src_ctrl->scu_x2);

Are the return values from clk_prepare_enable() intentionally ignored here
as well?

> +
> +	for_each_rsnd_src(src, priv, i)
> +		rsnd_resume_clk_reset(rsnd_mod_get(src)->clk,
> +				      rsnd_mod_get(src)->rstc);
> +}

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