Re: [PATCH v1 2/2] mmc: sdhci-of-dwcmshc: Add support for StarFive JHB100

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] mmc: sdhci-of-dwcmshc: memory corruption due to 36-bit DMA mask with fixed offset
- [Medium] mmc: sdhci-of-dwcmshc: power leak due to unconditional cclk_tx re-enable
--

commit e46c590ca0a75d2a6f76120b26e10916c916ffa1
Author: Changhuang Liang <changhuang.liang@starfivetech.com>

mmc: sdhci-of-dwcmshc: Add support for StarFive JHB100

Add support for the DWCMSHC controller found on the StarFive JHB100 SoC.
This configures the higher DMA address bits [35:32] through the PER1 SYSCON
and implements a clock wrapper to disable/enable cclk_tx around clock changes.

> diff --git a/drivers/mmc/host/sdhci-of-dwcmshc.c b/drivers/mmc/host/sdhci-of-dwcmshc.c
> index 1baa2379c9c39..021b03c173127 100644
> --- a/drivers/mmc/host/sdhci-of-dwcmshc.c
> +++ b/drivers/mmc/host/sdhci-of-dwcmshc.c
[ ... ]
> @@ -2257,6 +2266,145 @@ static const struct dwcmshc_pltfm_data sdhci_dwcmshc_hpe_gsc_pdata = {
>  	.init = dwcmshc_hpe_gsc_init,
>  };
>  
> +static int sdhci_jhb100_set_dma_mask(struct sdhci_host *host)
> +{
> +	struct mmc_host *mmc = host->mmc;
> +	struct device *dev = mmc_dev(mmc);
> +	int ret;
> +
> +	/*
> +	 * The StarFive JHB100's CMSHC controller only has 32-bit DMA
> +	 * capability by default, but the higher DMA address bits [35:32]
> +	 * can be configured through SYSCON, thus supporting up to 36-bit
> +	 * DMA capability.
> +	 */
> +	ret = dma_set_mask_and_coherent(dev, DMA_BIT_MASK(36));
[Severity: High]
Does this falsely advertise full 36-bit dynamic addressability to the DMA API?

Looking at dwcmshc_starfive_jhb100_init(), it hardcodes the upper 4 bits
(DWCMSHC_ARWADDR_DEFAULT = 4):

    ret = regmap_update_bits(syscon, reg_offset, DWCMSHC_ARWADDR,
                             DWCMSHC_ARWADDR_DEFAULT);

This proves the hardware only accesses a fixed 32-bit physical memory window
and cannot dynamically utilize the advertised 36-bit range.

If the kernel allocates a DMA buffer at any physical address outside the
0x400000000 - 0x4FFFFFFFF window, will the hardware blindly apply the hardcoded
SYSCON offset to the transaction, redirecting the DMA access and silently
corrupting memory inside the 0x400000000 window instead?

> +	if (ret)
> +		dev_err(dev, "Failed to set 36-bit DMA mask.\n");
> +
> +	return ret;
> +}
> +
> +static void sdhci_jhb100_set_clock(struct sdhci_host *host, unsigned int clock)
> +{
> +	struct sdhci_pltfm_host *pltfm_host = sdhci_priv(host);
> +	struct dwcmshc_priv *dwc_priv = sdhci_pltfm_priv(pltfm_host);
> +	struct jhb100_priv *priv = dwc_priv->priv;
> +
> +	/* the cclk_tx clock is disabled before sdhci_set_clock() to eliminate glitches */
> +	if (priv->cclk_tx_on) {
> +		clk_disable_unprepare(priv->cclk_tx);
> +		priv->cclk_tx_on = false;
> +	}
> +
> +	fsleep(15);
> +
> +	sdhci_set_clock(host, clock);
> +
> +	if (clk_prepare_enable(priv->cclk_tx))
[Severity: Medium]
Will this unconditionally reactivate the clock regardless of whether the MMC
core requested a zero clock frequency?

When the MMC subsystem enters suspend or becomes idle, it requests the clock
to be disabled by calling host->ops->set_clock(host, 0). 

The sdhci_jhb100_set_clock() wrapper processes this by passing clock = 0 to
sdhci_set_clock(), which returns early and turns off the core clocks.

However, the wrapper then unconditionally executes
clk_prepare_enable(priv->cclk_tx), ignoring the fact that the MMC core
requested a full clock shutdown.

Could this leave the cclk_tx clock enabled indefinitely during system runtime
suspend and deep sleep, potentially preventing the SoC clock controller from
powering down the associated PLL or power domain?

> +		dev_err(mmc_dev(host->mmc), "failed to re-enable cclk_tx\n");
> +	else
> +		priv->cclk_tx_on = true;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260919092950.178617-1-changhuang.liang@starfivetech.com?part=2