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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [High] Inconsistent PHY generation identification leads to dangerous struct casting and massive out-of-bounds memory accesses.
- [Medium] The `biu` clock reference is not released during system suspend, blocking the shared bus clock from being gated.
- [Medium] The `wmb()` memory barrier is incorrectly used as a delay/flush mechanism to wait for indirect PHY register writes to complete.
- [Medium] Hardware reset sequence lacks a dummy read to flush the posted write, potentially causing a violation of the minimum eMMC reset pulse width.
- [Medium] The SD6HC PHY and hardware reset logic bypasses the `priv->priv_writel()` abstraction, using raw `writel()` instead.
--

--- Patch [7]: [PATCH v4 7/8] mmc: sdhci-cadence: add Cadence SD6HC support ---
commit 3dbe4a2148f59d518992019858523fa45d6808da
Author: Tanmay Kathpalia <tanmay.kathpalia@altera.com>
Subject: mmc: sdhci-cadence: add Cadence SD6HC support

Adds support for the sixth-generation Cadence SD/SDIO/eMMC host controller
(SD6HC), splitting the driver to support its new combo-PHY and separate
CIU/BIU clocks.

> diff --git a/drivers/mmc/host/sdhci-cadence-core.c b/drivers/mmc/host/sdhci-cadence-core.c
> --- a/drivers/mmc/host/sdhci-cadence-core.c
> +++ b/drivers/mmc/host/sdhci-cadence-core.c
[ ... ]
> @@ -577,6 +592,13 @@ 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");
> +		if (IS_ERR(biu_clk))
> +			return dev_err_probe(dev, PTR_ERR(biu_clk), "failed to enable biu clock\n");
> +	}

[Severity: Medium]
Is the biu_clk reference intentionally discarded here? By keeping biu_clk as a
local variable, there is no way to disable it during system suspend. This
prevents the clock and its parent power domains from powering down.

[ ... ]
> +/*
> + * 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 approach misinterpret how shared clocks work in the Common Clock
Framework?

The framework uses reference counting, which safely handles shared clocks by
physically disabling them only when all consumers have disabled their
references. By not keeping the biu_clk pointer and failing to call
clk_disable_unprepare() on suspend, this code leaks a prepare/enable reference
and keeps the shared bus clock unnecessarily active during suspend.

>  static int sdhci_cdns_resume(struct device *dev)
>  {
>  	struct sdhci_host *host = dev_get_drvdata(dev);
> @@ -636,7 +674,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 version check cause an out-of-bounds memory read?

In sdhci_cdns_probe(), the driver allocates struct sdhci_cdns6_phy for
priv->phy purely based on the cdns,sd6hc device tree compatible string. If
the hardware's SDHCI_HOST_VERSION register reports a version older than
4.20 (e.g., due to a hardware errata), this resume path falls back to
sdhci_cdns4_phy_init().

When sdhci_cdns4_phy_init() is called, it casts priv->phy to struct
sdhci_cdns4_phy and iterates over phy->nr_phy_params. Because the memory
actually holds a struct sdhci_cdns6_phy, nr_phy_params aliases with
t_cmd_output_min (which typically holds a value like 5000). This would
cause the loop in sdhci_cdns4_phy_init() to read thousands of elements
past the end of the heap allocation and write arbitrary memory contents
to the PHY registers.

> diff --git a/drivers/mmc/host/sdhci-cadence-phy-v6.c b/drivers/mmc/host/sdhci-cadence-phy-v6.c
> new file mode 100644
> --- /dev/null
> +++ b/drivers/mmc/host/sdhci-cadence-phy-v6.c
[ ... ]
> +static void sdhci_cdns6_write_phy_reg(struct sdhci_cdns_priv *priv, const u32 address,
> +				      const u32 value)
> +{
> +	writel(address, priv->hrs_addr + SDHCI_CDNS_HRS04);
> +	writel(value, priv->hrs_addr + SDHCI_CDNS_HRS05);
> +}

[Severity: Medium]
Does this bypass the priv->priv_writel() abstraction?

The Cadence core driver uses priv->priv_writel() to handle SoC-specific
write constraints for Host Register Set accesses, such as the spinlocks and
byte-lane logic needed for elba_priv_writel(). Hardcoding raw writel() calls
here violates that abstraction and could lead to state corruption on future
platforms that require custom write logic.

[ ... ]
> +int sdhci_cdns6_phy_init(struct sdhci_cdns_priv *priv)
> +{
[ ... ]
> +	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();
> +
> +	ret = sdhci_cdns6_dll_reset(priv, false);

[Severity: Medium]
Is wmb() the correct mechanism for this synchronization?

The wmb() macro is an ordering barrier for CPU memory accesses, but it does
not flush posted interconnect writes to the peripheral. Since the subsequent
readl() inside sdhci_cdns6_dll_reset() naturally flushes preceding writes,
this wmb() appears to be inadvertently acting as a short CPU delay loop.

If a delay is required by the hardware to process the writes before releasing
reset, should this be an explicit udelay() instead of relying on the side
effects of a memory barrier?

[ ... ]
> +void sdhci_cdns6_hw_reset(struct sdhci_host *host)
> +{
> +	struct sdhci_cdns_priv *priv = sdhci_cdns_priv(host);
> +	void __iomem *reg;
> +
> +	reg = priv->hrs_addr + SDHCI_CDNS_HRS11;
> +	writel(SDHCI_CDNS_HRS11_EMMC_RST, reg);
> +	/* eMMC HW reset assertion: spec requires >= 1us, give margin */
> +	usleep_range(10, 20);
> +	writel(0, reg);
> +	/* For eMMC, minimum is 200us but give it 300us for good measure */
> +	usleep_range(300, 1000);
> +}

[Severity: Medium]
Can the lack of a dummy read here truncate the eMMC reset pulse?

Because writel() is a posted write, the first write asserting the reset
could be buffered in the interconnect while the CPU sleeps in
usleep_range(). When the CPU wakes and issues the second writel() to
deassert reset, both writes might arrive at the peripheral back-to-back.
This could result in a physical reset pulse width much shorter than the
required 1us minimum. Adding a readl() after the first writel() would
ensure the write reaches the hardware before the sleep timer starts.

Also, should this function be using priv->priv_writel() instead of raw
writel() for the same abstraction reasons mentioned regarding
sdhci_cdns6_write_phy_reg()?

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