Re: [PATCH v5 5/8] mmc: sdhci-cadence: rename SD4HC symbols for SD6HC groundwork

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

Pre-existing issues:
- [High] Mismatch between counted and parsed PHY parameters causes unintended zero-writes to the PHY configuration register.
--

--- Patch [5]: [PATCH v5 5/8] mmc: sdhci-cadence: rename SD4HC symbols for SD6HC groundwork ---
commit 15f674da35101e267b2865b095a96414154f99a4
Author: Tanmay Kathpalia <[email protected]>

mmc: sdhci-cadence: rename SD4HC symbols for SD6HC groundwork

SD4HC PHY helpers and the default ops/drv_data are not marked as
version-specific, so it is unclear what is shared versus SD4HC-only
ahead of SD6HC support.

> diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence.c
> index 435603c8c00b2..8a6ef6d9d1dd8 100644
> --- a/drivers/mmc/host/sdhci-cadence.c
> +++ b/drivers/mmc/host/sdhci-cadence.c

[ ... ]

> @@ -156,44 +155,42 @@ static int sdhci_cdns_write_phy_reg(struct sdhci_cdns_priv *priv,
>  	return ret;
>  }
>  
> -static unsigned int sdhci_cdns_phy_param_count(struct device_node *np)
> +static unsigned int sdhci_cdns4_phy_param_count(struct device_node *np)
>  {
>  	unsigned int count = 0;
>  	int i;
>  
> -	for (i = 0; i < ARRAY_SIZE(sdhci_cdns_phy_cfgs); i++)
> -		if (of_property_present(np, sdhci_cdns_phy_cfgs[i].property))
> +	for (i = 0; i < ARRAY_SIZE(sdhci_cdns4_phy_cfgs); i++)
> +		if (of_property_present(np, sdhci_cdns4_phy_cfgs[i].property))
>  			count++;
>  
>  	return count;
>  }
>  
> -static void sdhci_cdns_phy_param_parse(struct device_node *np,
> -				       struct sdhci_cdns_priv *priv)
> +static void sdhci_cdns4_phy_param_parse(struct device_node *np, struct sdhci_cdns_priv *priv)
>  {
> -	struct sdhci_cdns_phy_param *p = priv->phy_params;
> +	struct sdhci_cdns4_phy_param *p = priv->phy_params;
>  	u32 val;
>  	int ret, i;
>  
> -	for (i = 0; i < ARRAY_SIZE(sdhci_cdns_phy_cfgs); i++) {
> -		ret = of_property_read_u32(np, sdhci_cdns_phy_cfgs[i].property,
> -					   &val);
> +	for (i = 0; i < ARRAY_SIZE(sdhci_cdns4_phy_cfgs); i++) {
> +		ret = of_property_read_u32(np, sdhci_cdns4_phy_cfgs[i].property, &val);
>  		if (ret)
>  			continue;

[Severity: High]
This isn't a bug introduced by this patch, but can this error handling logic
lead to uninitialized writes later?

If of_property_present() in sdhci_cdns4_phy_param_count() returns true for a
property (e.g., an empty boolean flag), the global count nr_phy_params is
incremented. However, if of_property_read_u32() fails here, the pointer p is
not incremented, but nr_phy_params is never decremented to reflect the
skipped entry.

>  
> -		p->addr = sdhci_cdns_phy_cfgs[i].addr;
> +		p->addr = sdhci_cdns4_phy_cfgs[i].addr;
>  		p->data = val;
>  		p++;
>  	}
>  }
>  
> -static int sdhci_cdns_phy_init(struct sdhci_cdns_priv *priv)
> +static int sdhci_cdns4_phy_init(struct sdhci_cdns_priv *priv)
>  {
>  	int ret, i;
>  
>  	for (i = 0; i < priv->nr_phy_params; i++) {
> -		ret = sdhci_cdns_write_phy_reg(priv, priv->phy_params[i].addr,
> -					       priv->phy_params[i].data);
> +		ret = sdhci_cdns4_write_phy_reg(priv, priv->phy_params[i].addr,
> +						priv->phy_params[i].data);

[Severity: High]
This is a pre-existing issue tied to the above parsing logic, but could this
loop process trailing zero-initialized elements?

Because it iterates strictly over the original nr_phy_params count, a parsing
failure earlier means the array will contain trailing zero-initialized
elements. This would result in writing a value of 0 to addr = 0 (which
corresponds to SDHCI_CDNS_PHY_DLY_SD_HS).

Could this unintentionally overwrite the High-Speed delay configuration and
potentially break SD card initialization?

>  		if (ret)
>  			return ret;
>  	}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5