Re: [PATCH v6 3/8] mmc: sdhci-esdhc-imx: fix esdhc_change_pinstate() to allow default state restore

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] On non-uSDHC boards, requesting high-speed UHS timings via a malformed device tree triggers a NULL pointer dereference in the pinctrl subsystem because `pins_100mhz` and `pins_200mhz` are checked with `IS_ERR()` instead of `IS_ERR_OR_NULL()`.
--

--- Patch [3]: [PATCH v6 3/8] mmc: sdhci-esdhc-imx: fix esdhc_change_pinstate() to allow default state restore ---
commit dd1c1cd99a70d068cd94016148812cfb2f68ca96
Author: Luke Wang <ziniu.wang_1@nxp.com>

mmc: sdhci-esdhc-imx: fix esdhc_change_pinstate() to allow default state restore

This commit moves the checks for pins_100mhz and pins_200mhz into their
respective switch cases in esdhc_change_pinstate(). This prevents the checks
from unconditionally blocking the default case and allows devices without
high-speed pin states to restore their default pin configuration.

> diff --git a/drivers/mmc/host/sdhci-esdhc-imx.c b/drivers/mmc/host/sdhci-esdhc-imx.c
> index 685e508c61279..2da5c8ab3e622 100644
> --- a/drivers/mmc/host/sdhci-esdhc-imx.c
> +++ b/drivers/mmc/host/sdhci-esdhc-imx.c
> @@ -1326,19 +1326,21 @@ static int esdhc_change_pinstate(struct sdhci_host *host,
>  
>  	dev_dbg(mmc_dev(host->mmc), "change pinctrl state for uhs %d\n", uhs);
>  
> -	if (IS_ERR(imx_data->pinctrl) ||
> -		IS_ERR(imx_data->pins_100mhz) ||
> -		IS_ERR(imx_data->pins_200mhz))
> +	if (IS_ERR(imx_data->pinctrl))
>  		return -EINVAL;
>  
>  	switch (uhs) {
>  	case MMC_TIMING_UHS_SDR50:
>  	case MMC_TIMING_UHS_DDR50:
> +		if (IS_ERR(imx_data->pins_100mhz))
> +			return -EINVAL;

[Severity: High]
This is a pre-existing issue, but will IS_ERR() properly catch a NULL
pointer if pins_100mhz is uninitialized?

For non-uSDHC boards, imx_data->pins_100mhz can be NULL. If a malformed
device tree specifies UHS timings, IS_ERR(NULL) evaluates to false. Does
this result in pinctrl being set to NULL and later passed to
pinctrl_select_state(), causing a kernel panic?

Would it be safer to use IS_ERR_OR_NULL() here and for pins_200mhz below?

>  		pinctrl = imx_data->pins_100mhz;
>  		break;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713043904.4144172-1-ziniu.wang_1@oss.nxp.com?part=3