Re: [PATCH v7 8/9] mmc: sdhci-cadence: add Cadence SD6HC support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Type Confusion and Out-of-Bounds Read due to relying on SD Association's host->version
- [Medium] Memory Barrier Misuse masks hardware timing requirements
- [Medium] Hardcoded of_device_is_compatible checks bypass driver match data
--

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

mmc: sdhci-cadence: add Cadence SD6HC support

This commit adds support for the Cadence SD6HC host controller and its
integrated combo-PHY. It splits the existing driver into a core module and
a PHY-specific module for v6, using a shared header for private state.

> diff --git a/drivers/mmc/host/sdhci-cadence.c b/drivers/mmc/host/sdhci-cadence-core.c
> similarity index 85%
> rename from drivers/mmc/host/sdhci-cadence.c
> rename to drivers/mmc/host/sdhci-cadence-core.c
> index 1fb23051a047b..ea2517e8f72a9 100644
> --- a/drivers/mmc/host/sdhci-cadence.c
> +++ b/drivers/mmc/host/sdhci-cadence-core.c
[ ... ]
> @@ -569,13 +583,24 @@ static int sdhci_cdns_probe(struct platform_device *pdev)
[ ... ]
> +	if (of_device_is_compatible(dev->of_node, "cdns,sd6hc")) {
> +		clk = devm_clk_get_enabled(dev, "ciu");

[Severity: Medium]
Will this hardcoded check fail to match hardware variants that omit the
generic fallback string?

The code branches by checking specifically for "cdns,sd6hc", bypassing the
driver match data. If a valid device tree specifies a variant string without
the generic fallback, this check will fail, leading to missing clock
initializations.

Could this logic be moved to a flag or PHY type identifier within
sdhci_cdns_drv_data instead of hardcoding compatible string comparisons?

[ ... ]
> +	if (of_device_is_compatible(dev->of_node, "cdns,sd4hc")) {
> +		__sdhci_read_caps(host, &version, NULL, NULL);
> +		ret = sdhci_cdns4_phy_probe(pdev, priv);

[Severity: Medium]
Similarly, will this check misidentify specific SD4HC variants as SD6HC if
the generic string is omitted?

If "cdns,sd4hc" is missing from the device tree node but a variant compatible
string is present, the driver will mistakenly execute the SD6HC PHY probe
block below.

[ ... ]
> @@ -636,7 +677,11 @@ static int sdhci_cdns_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> -	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]
Can this type confusion lead to an out-of-bounds read?

The PHY structure pointer is cast and accessed based on the hardware-reported
SDHCI version rather than the actual type allocated during probe. If an SD6HC
device reports a specification version less than 4.20 in its registers, it
will fall back to sdhci_cdns4_phy_init().

Because sdhci_cdns4_phy_init() casts the PHY state to struct sdhci_cdns4_phy,
it misinterprets the first field of struct sdhci_cdns6_phy (t_cmd_output_min)
as the nr_phy_params field. This will result in an out-of-bounds loop
reading unmapped memory.

Should the PHY version be determined by the driver data set during probe
instead of relying on the hardware specification version register?

> diff --git a/drivers/mmc/host/sdhci-cadence-phy-v6.c b/drivers/mmc/host/sdhci-cadence-phy-v6.c
> new file mode 100644
> index 0000000000000..22d56bb46d75a
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-cadence-phy-v6.c
[ ... ]
> +	sdhci_cdns6_write_phy_reg(priv, SDHCI_CDNS6_PHY_CTRL_REG, reg);
> +
> +	/*
> +	 * Ensure all preceding PHY register writes complete and reach the controller before
> +	 * releasing the PHY from reset. Without this, SDR104 has been observed to fail
> +	 * intermittently on some boards.
> +	 */
> +	wmb();

[Severity: Medium]
Is this memory barrier masking a hardware timing requirement?

The wmb() macro orders local CPU memory stores, but it does not flush posted
MMIO writes over the interconnect to the device. Its success in preventing
intermittent failures is likely due to it acting as an accidental micro-delay
on the CPU. On a faster CPU or a different interconnect architecture, this
will not guarantee the writes have settled.

Would a dummy readl() to flush posted writes or a small udelay() be more
appropriate here?

> +
> +	ret = sdhci_cdns6_dll_reset(priv, false);
[ ... ]
> +	sdhci_cdns6_write_phy_reg(priv, SDHCI_CDNS6_PHY_DQ_TIMING_REG, reg);
> +
> +	/* Ensure DQ timing programming is visible before HRS09 follow-up writes */
> +	wmb();

[Severity: Medium]
Does this second wmb() suffer from the same MMIO flushing issue?

Like the previous barrier, this orders CPU-side memory stores but does not
ensure the device has actually received and processed the DQ timing
configuration before the subsequent HRS09 writes occur.

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