Re: [PATCH v6 09/16] ASoC: rsnd: ssui: Add RZ/G3E SSIU BUSIF support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 8 potential issue(s) to consider:
- [High] Out-of-bounds memory access triggered by malformed Device Tree.
- [High] Permanent resource leak of shared reset control on error path.
- [High] Silent PIO fallback will cause hardware misbehavior on RZ/G3E.
- [Medium] Hardcoded SoC check contradicts the capability flags design.
- [Medium] Undocumented functional change regarding hardware reset.
- [Low] Unnecessary architectural complexity with dynamic allocation.
- [Low] Variable shadowing in rsnd_ssiu_busif_err_status_clear().
- [Low] Incorrect placement of ssiu_ctrl struct member comment.
--

commit 376b6d769102c3df2fb5467964293ac39d8601d0
Author: John Madieu <john.madieu.xa@bp.renesas.com>

ASoC: rsnd: ssui: Add RZ/G3E SSIU BUSIF support

Add support for the SSIU found on the Renesas RZ/G3E SoC, which
provides a different BUSIF layout compared to earlier generations.

> Rather than scattering SoC-specific checks across functional code,
> introduce an extra capability flags in the match data:
> 
>  - RSND_SSIU_BUSIF_STATUS_COUNT_2: only two BUSIF error-status
>    register pairs are present. Used in rsnd_ssiu_busif_err_irq_ctrl()
>    and rsnd_ssiu_busif_err_status_clear() to limit register iteration.
> 
> Future SoCs sharing these constraints can set the flags without
> requiring code changes.

This isn't a bug, but the patch later adds a hardcoded check for
rsnd_is_rzg3e(priv) in rsnd_ssiu_init() to skip PIO initialization. Would it
be better to introduce an additional capability flag like
RSND_SSIU_NO_PIO_FALLBACK instead of hardcoding the SoC check, to keep with
this stated design?

Also, the commit message doesn't mention the addition of the "ssi-all" shared
reset control or the resulting changes to the hardware initialization
sequence. Could this functional change be documented here?

> diff --git a/sound/soc/renesas/rcar/rsnd.h b/sound/soc/renesas/rcar/rsnd.h
> index e917aa12fa80c..186468a584fed 100644
> --- a/sound/soc/renesas/rcar/rsnd.h
> +++ b/sound/soc/renesas/rcar/rsnd.h
> @@ -684,6 +685,7 @@ struct rsnd_priv {
>  	/*
>  	 * below value will be filled on rsnd_ssi_probe()
>  	 */
> +	void *ssiu_ctrl;
>  	void *ssi;

This isn't a bug, but this variable is allocated and populated in
rsnd_ssiu_probe(). Should it be grouped under the rsnd_ssiu_probe() comment
block further down instead of rsnd_ssi_probe()?

> diff --git a/sound/soc/renesas/rcar/ssiu.c b/sound/soc/renesas/rcar/ssiu.c
> index 0cfa84fe5ea86..f483389868d2c 100644
> --- a/sound/soc/renesas/rcar/ssiu.c
> +++ b/sound/soc/renesas/rcar/ssiu.c
[ ... ]
> @@ -87,10 +95,10 @@ static void rsnd_ssiu_busif_err_irq_ctrl(struct rsnd_mod *mod, int enable)
>  
>  bool rsnd_ssiu_busif_err_status_clear(struct rsnd_mod *mod)
>  {
> +	struct rsnd_priv *priv = rsnd_mod_to_priv(mod);
>  	bool error = false;
>  	int id = rsnd_mod_id(mod);

Does this cause a compiler warning for variable shadowing?

There is an existing local declaration of struct rsnd_priv *priv inside the
if (status) block further down in this function:

	status &= val;
	if (status) {
		struct rsnd_priv *priv = rsnd_mod_to_priv(mod);
		struct device *dev = rsnd_priv_to_dev(priv);

which will shadow this newly added variable.

[ ... ]
> @@ -160,7 +168,8 @@ static int rsnd_ssiu_init(struct rsnd_mod *mod,
>  	/*
>  	 * SSI_MODE0
>  	 */
> -	rsnd_mod_bset(mod, SSI_MODE0, (1 << id), !use_busif << id);
> +	if (!rsnd_is_rzg3e(priv))
> +		rsnd_mod_bset(mod, SSI_MODE0, (1 << id), !use_busif << id);

If the DMA channel request fails with -EAGAIN, rsnd_rdai_continuance_probe()
will attempt a PIO fallback by replacing mod->ops with rsnd_ssi_pio_ops.

Since the RZ/G3E hardware and register configuration do not support PIO,
will this fallback silently fail and lead to hardware misbehavior or a
stream hang?

[ ... ]
> @@ -534,8 +545,15 @@ int rsnd_ssiu_probe(struct rsnd_priv *priv)
>  	if (!ssiu)
>  		return -ENOMEM;
>  
> +	ctrl = devm_kzalloc(dev, sizeof(*ctrl), GFP_KERNEL);
> +	if (!ctrl)
> +		return -ENOMEM;
> +
> +	ctrl->busif_status_count = rsnd_flags_has(priv, RSND_SSIU_BUSIF_STATUS_COUNT_2) ? 2 : 4;

Is it necessary to dynamically allocate a new structure just to store the
busif_status_count integer?

Could this status count be evaluated directly from the capability flag when
needed, or stored as an integer field in an existing structure to reduce
allocation overhead?

>  	priv->ssiu	= ssiu;
>  	priv->ssiu_nr	= nr;
> +	priv->ssiu_ctrl	= ctrl;
>  
>  	if (rsnd_is_gen1(priv))
>  		ops = &rsnd_ssiu_ops_gen1;
> @@ -558,12 +576,21 @@ int rsnd_ssiu_probe(struct rsnd_priv *priv)
>  		} else if (rsnd_is_gen4(priv)) {
>  			list	= gen4_id;
>  			nr	= ARRAY_SIZE(gen4_id);
> +		} else if (rsnd_is_rzg3e(priv)) {
> +			list	= rzg3e_id;
> +			nr	= ARRAY_SIZE(rzg3e_id);

Can a malformed device tree trigger an out-of-bounds memory access here?

The ssiu array is initially allocated based on the device tree node count,
which is saved in priv->ssiu_nr. However, nr is overwritten here with
ARRAY_SIZE(rzg3e_id).

If the device tree contains more nodes than expected, the later
for_each_rsnd_ssiu() loop will iterate up to the original node count,
causing the inner j loop to terminate without breaking. This results in an
incorrect id_sub value that can lead to an out-of-bounds access in
rsnd_ssiu_get_status().

>  		} else {
>  			dev_err(dev, "unknown SSIU\n");
>  			return -ENODEV;
>  		}
>  	}
>  
> +	/* Acquire shared reset once for all SSIU modules */
> +	rstc = devm_reset_control_get_optional_shared(dev, "ssi-all");
> +	if (IS_ERR(rstc))
> +		return dev_err_probe(dev, PTR_ERR(rstc),
> +				     "failed to get ssi-all reset\n");
> +
>  	for_each_rsnd_ssiu(ssiu, priv, i) {
>  		int ret;
>  
> @@ -586,7 +613,7 @@ int rsnd_ssiu_probe(struct rsnd_priv *priv)
>  		}
>  
>  		ret = rsnd_mod_init(priv, rsnd_mod_get(ssiu),
> -				    ops, NULL, NULL, RSND_MOD_SSIU, i);
> +				    ops, NULL, rstc, RSND_MOD_SSIU, i);
>  		if (ret)
>  			return ret;

If a subsequent iteration of this loop fails, returning ret directly leaves
the previously initialized modules active.

Since rsnd_mod_init() calls reset_control_deassert(), returning early without
rolling back the successful initializations will permanently leak the deassert
reference count of the shared ssi-all reset line.

Does this error path need to call rsnd_mod_quit() for the previously
initialized modules?

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