Re: [PATCH v24 03/15] mmc: renesas_sdhi: Add clk_mask field to support flexible clock divider widths

[Wolfram Sang]

Hi,

Sashiko found issues with your patch. I curated the list and left only those
which I see as valid. Please have a look at these issues.

Thank you and happy hacking,

   Wolfram

> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] A very low or zero clock rate from the clock provider causes an infinite loop in `renesas_sdhi_set_clock`.
> --
> 
> --- Patch [3]: [PATCH v24 03/15] mmc: renesas_sdhi: Add clk_mask field to support flexible clock divider widths ---
> commit 1c7318b14c028686fc8ca703eb8ac2be269937b6
> Author: Biju Das <biju.das.jz@xxxxxxxxxxxxxx>
> 
> mmc: renesas_sdhi: Add clk_mask field to support flexible clock divider widths
> 
> This patch introduces a clk_mask field in platform data to replace a hardcoded
> 32-bit clock mask, supporting 11-bit clock dividers on newer SoCs like the
> RZ/G3L. It updates renesas_sdhi_set_clock() to use the dynamic mask and sets
> up backward-compatible default mask values.
> 
> > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> > index f9ec78d699f45..604d886e483c7 100644
> > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> [ ... ]
> > @@ -213,7 +213,7 @@ static void renesas_sdhi_set_clock(struct tmio_mmc_host *host,
> >  	 * provided for actual_clock in renesas_sdhi_clk_update().
> >  	 */
> >  	clk_margin = new_clock >> 10;
> > -	for (clk = 0x80000080; new_clock + clk_margin >= (clock << 1); clk >>= 1)
> > +	for (clk = host->pdata->clk_mask; new_clock + clk_margin >= (clock << 1); clk >>= 1)
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but does this loop risk an
> infinite hang if the clock provider returns a very low rate?
> 
> In renesas_sdhi_set_clock(), if the clock provider supplies a rate smaller
> than the maximum divider, the division to calculate the initial clock value
> can evaluate to 0. Since 0 << 1 is still 0, the loop condition
> new_clock + clk_margin >= (clock << 1) will remain permanently true for any
> strictly positive new_clock value.
> 
> Could this prevent the loop from ever terminating, causing an unrecoverable
> CPU hang? Should there be an additional safety check (such as clk != 0 or
> clock > 0) added to the loop condition to gracefully handle misbehaving
> clock providers?
> 
> >  		clock <<= 1;
> >  
> >  	/* 1/1 clock is option */
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260820171657.62815-1-biju.das.jz@xxxxxxxxxxxxxx?part=3

Attachment: signature.asc
Description: PGP signature