Re: [PATCH v24 07/15] mmc: renesas_sdhi: Add tuning delay support for RZ/G2L

[Wolfram Sang <wsa+renesas@sang-engineering.com>]

[-- Attachment #1: Type: text/plain, Size: 2345 bytes --]


> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The new function renesas_sdhi_set_hw_adjustment_delay() accesses SCC registers via sd_scc_write32() without checking if priv->scc_ctl is initialized.
> --
> 
> --- Patch [7]: [PATCH v24 07/15] mmc: renesas_sdhi: Add tuning delay support for RZ/G2L ---
> commit 4e31e96fb980d0bb6400d3a60dfcc29dca208dcc
> Author: Biju Das <biju.das.jz@bp.renesas.com>
> 
> mmc: renesas_sdhi: Add tuning delay support for RZ/G2L
> 
> This commit adds hardware adjustment delay support for RZ/G2L SoCs during
> signal-voltage switch. It introduces the TMIO_MMC_TUNING_DELAY flag and
> applies a delay by writing to SCC_TMPPORT2 based on the regulator voltage.
> 
> > diff --git a/drivers/mmc/host/renesas_sdhi_core.c b/drivers/mmc/host/renesas_sdhi_core.c
> > --- a/drivers/mmc/host/renesas_sdhi_core.c
> > +++ b/drivers/mmc/host/renesas_sdhi_core.c
> [ ... ]
> > @@ -350,6 +316,55 @@ static inline void sd_scc_write32(struct tmio_mmc_host *host,
> >  	writel(val, priv->scc_ctl + (addr << host->bus_shift));
> >  }
> >  
> > +static void renesas_sdhi_set_hw_adjustment_delay(struct tmio_mmc_host *host)
> > +{
> > +	struct renesas_sdhi *priv = host_to_priv(host);
> > +
> > +	if (!(host->pdata->flags & TMIO_MMC_TUNING_DELAY))
> > +		return;
> > +
> > +	if (host->mmc->ios.signal_voltage == MMC_SIGNAL_VOLTAGE_330)
> > +		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TMPPORT2, 0x0);
> > +	else
> > +		sd_scc_write32(host, priv, SH_MOBILE_SDHI_SCC_TMPPORT2, 0x1);
> 
> [Severity: High]
> Is it possible for priv->scc_ctl to be NULL here?
> 
> If an incorrect device tree binding or a hardware fault causes the
> controller to report a version older than SDHI_VER_GEN2_SDR104,
> priv->scc_ctl might not be mapped during probe.
> 
> However, renesas_sdhi_start_signal_voltage_switch() is registered based
> on TMIO_MMC_MIN_RCAR2 and calls this new function, which writes directly
> to the SCC register via sd_scc_write32().
> 
> Could this cause a NULL pointer dereference during card insertion?

I agree. We should ensure in probe() that either

a) TMIO_MMC_TUNING_DELAY has a valid SCC or bail out otherwise

or

b) clear the TMIO_MMC_TUNING_DELAY flag if there is no SCC present

What do you think?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]