Re: [PATCH v6 7/8] mmc: sdhci-cadence: add Cadence SD6HC support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Type confusion and out-of-bounds memory access due to split-brain PHY version detection
- [Medium] BIU clock is not disabled during system suspend, causing power inefficiency
--

commit ae6fe1c869dec5dbfd62f818d4b7f208b7357ea8
Author: Tanmay Kathpalia <tanmay.kathpalia@altera.com>

mmc: sdhci-cadence: add Cadence SD6HC support

The Cadence SD6HC is a sixth-generation SD/SDIO/eMMC host controller
with an integrated combo-PHY. This commit splits the existing driver to
support SD6HC, adding sdhci-cadence-phy-v6.c and updating the core.

> diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence-core.c
> index 1fb23051a047b..18a5329f27db9 100644
> --- a/drivers/mmc/host/sdhci-cadence.c
> +++ b/drivers/mmc/host/sdhci-cadence-core.c
[ ... ]
> @@ -569,6 +583,7 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
>  	if (IS_ERR(clk))
>  		return PTR_ERR(clk);
>  
> +	/* SD6HC requires a second clock, "biu", for the bus interface unit. */
> +	if (of_device_is_compatible(dev->of_node, "cdns,sd6hc")) {
> +		biu_clk = devm_clk_get_enabled(dev, "biu");

[Severity: Medium]
Is there a way to turn this clock off during suspend? Since biu_clk is
stored in a local variable, the power management callbacks won't be able
to access it to disable it later.

> +		if (IS_ERR(biu_clk))
> +			return dev_err_probe(dev, PTR_ERR(biu_clk), "failed to enable biu clock\n");
> +	}
[ ... ]
> +	if (of_device_is_compatible(dev->of_node, "cdns,sd4hc")) {
> +		__sdhci_read_caps(host, &version, NULL, NULL);
> +		ret = sdhci_cdns4_phy_probe(pdev, priv);
> +		if (ret)
> +			return ret;
[ ... ]
> +	} else {
> +		ret = sdhci_cdns6_phy_probe(pdev, priv);

[Severity: High]
If the device is bound using the "cdns,sd6hc" compatible string, the driver
allocates the struct sdhci_cdns6_phy type and assigns it to the opaque
priv->phy pointer here.

What happens during suspend/resume if the hardware itself reports an SDHCI
spec version lower than 4.20?

> +		if (ret)
> +			return ret;
>  	}
>  
>  	return sdhci_add_host(host);
>  }
>  
> +/*
> + * Only the CIU clock is gated on suspend. The SD6HC "biu" clock is not
> + * toggled here as it may be a shared bus clock; a dedicated biu clock
> + * would need explicit PM gating added here.
> + */

[Severity: Medium]
Does this assumption hold true under the Linux Common Clock Framework?

Since CCF handles shared clocks through reference counting, it will keep the
underlying hardware clock running as long as other consumers hold a reference,
and safely power it down when all consumers have disabled it. Could we safely
disable the BIU clock during suspend to improve power efficiency?

>  static int sdhci_cdns_resume(struct device *dev)
>  {
>  	struct sdhci_host *host = dev_get_drvdata(dev);
[ ... ]
> -	ret = sdhci_cdns4_phy_init(priv);
> +	if (host->version >= SDHCI_SPEC_420)
> +		ret = sdhci_cdns6_phy_init(priv);
> +	else
> +		ret = sdhci_cdns4_phy_init(priv);

[Severity: High]
Could this cause type confusion if we allocated sdhci_cdns6_phy in probe but
hit the fallback branch here? (e.g. due to a misconfigured device tree or
anomalous hardware reporting an older spec version)

If this falls back to sdhci_cdns4_phy_init() for an SD6HC PHY pointer:

sdhci_cdns4_phy_init(struct sdhci_cdns_priv *priv) {
    struct sdhci_cdns4_phy *phy = priv->phy;
    ...
    for (i = 0; i < phy->nr_phy_params; i++) {
        ret = sdhci_cdns4_write_phy_reg(priv, phy->phy_params[i].addr,
                                        phy->phy_params[i].data);
    }
}

This would misinterpret the fields from sdhci_cdns6_phy (such as treating
t_cmd_output_min as nr_phy_params), which could lead to out-of-bounds reads
and arbitrary MMIO writes. Should this check use the compatible string or a
saved flag rather than the host version to determine the PHY type?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260829184252.27644-1-tanmay.kathpalia@altera.com?part=7