Re: [PATCH v6 08/16] ASoC: rsnd: Add RZ/G3E DMA address calculation support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unchecked NULL pointer dereference in rsnd_rzg3e_dma_addr() when SSIU module is not present.
- [Low] Missing parentheses around macro arguments in RZ/G3E DMA address calculation macros.
--

commit 8ac44d32c9e0b39cce390dd7dede7d192b0f5ad7
Author: John Madieu <john.madieu.xa@bp.renesas.com>

ASoC: rsnd: Add RZ/G3E DMA address calculation support

This commit adds a dedicated DMA address calculation function for the
RZ/G3E platform, which has different DMA register base addresses and offsets
compared to other R-Car platforms.

> diff --git a/sound/soc/renesas/rcar/dma.c b/sound/soc/renesas/rcar/dma.c
> index 24c23cb893906..ede0123d40eb5 100644
> --- a/sound/soc/renesas/rcar/dma.c
> +++ b/sound/soc/renesas/rcar/dma.c

[ ... ]

> +/* RZ/G3E DMA address macros */
> +#define RDMA_SSI_I_N_G3E(addr, i)	(addr ##_reg + 0x0000F000 + (0x1000 * i))
> +#define RDMA_SSI_O_N_G3E(addr, i)	(addr ##_reg + 0x0000F000 + (0x1000 * i))

This isn't a bug, but could the 'i' parameter be wrapped in parentheses?

While 'i' is currently passed as a single variable ('id'), using (0x1000 * (i))
would prevent incorrect address calculations due to operator precedence if an
expression is ever passed as an argument in the future.

Does it make sense to update the SRC and CMD macros below as well?

> +
> +#define RDMA_SSIU_I_N_G3E(addr, i, j)	(addr ##_reg + 0x0000F000 + (0x1000 * (i)) + (((j) / 4) * 0xA000) + (((j) % 4) * 0x400) - (0x4000 * ((i) / 9) * ((j) / 4)))
> +#define RDMA_SSIU_O_N_G3E(addr, i, j)	RDMA_SSIU_I_N_G3E(addr, i, j)

[ ... ]

> +static dma_addr_t
> +rsnd_rzg3e_dma_addr(struct rsnd_dai_stream *io,
> +		    struct rsnd_mod *mod, int is_play, int is_from)
> +{
> +	struct rsnd_priv *priv = rsnd_io_to_priv(io);
> +	phys_addr_t ssi_reg = rsnd_gen_get_phy_addr(priv, RSND_BASE_SSI);
> +	phys_addr_t src_reg = rsnd_gen_get_phy_addr(priv, RSND_BASE_SCU);
> +	int id    = rsnd_mod_id(mod);
> +	int busif = rsnd_mod_id_sub(rsnd_io_to_mod_ssiu(io));

Can this result in a NULL pointer dereference?

If the ALSA stream does not have an SSIU module attached (like when the
device tree omits the SSIU node, or the SSI is configured for PIO mode via
the pio-transfer property), rsnd_io_to_mod_ssiu(io) will return NULL.

Looking at rsnd_mod_id_sub(), it unconditionally dereferences the pointer:

sound/soc/renesas/rcar/core.c:rsnd_mod_id_sub() {
        if ((mod)->ops->id_sub)
                return (mod)->ops->id_sub(mod);
        
        return 0;
}

Could this trigger a panic on stream open in valid configurations like
[SRC] -> [SSI (PIO)]?

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