Re: [PATCH v2 2/6] mmc: sdhci-of-at91: add option to keep clocks enabled

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Return values of mandatory clk_prepare_enable() calls are completely ignored in probe and resume paths
--

commit 6269754970c40ae19a0c5916bfacc745090e47c2
Author: Robert Marko <robert.marko@sartura.hr>

mmc: sdhci-of-at91: add option to keep clocks enabled
    
sdhci_at91_set_clks_presets() both enables the controller clocks and
programs its capabilities and preset registers. This prevents callers from
restoring the registers without changing the clock enable counts.
    
Move clock enablement to callers and add a SoC data flag for controllers
that must keep their clocks enabled. Use it in the runtime PM paths while
keeping register restoration separate from clock enablement.

> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> index 7c4ac65f247d3..b7e2a89da348a 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
[ ... ]
> @@ -269,14 +266,23 @@ static int sdhci_at91_runtime_resume(struct device *dev)
>  	int ret;
>  
>  	if (priv->restore_needed) {
> -		ret = sdhci_at91_set_clks_presets(dev);
> -		if (ret)
> -			return ret;
> +		if (!priv->soc_data->keep_clks_on)
> +			clk_prepare_enable(priv->hclock);
> +
> +		sdhci_at91_set_clks_presets(dev);
> +
> +		if (!priv->soc_data->keep_clks_on) {
> +			clk_prepare_enable(priv->mainck);
> +			clk_prepare_enable(priv->gck);
> +		}

[Severity: High]
This isn't a bug introduced by this patch, as the old implementation in
sdhci_at91_set_clks_presets() also ignored the return values. However, could
this cause a kernel panic if the clock enablement fails?

In drivers/mmc/host/sdhci-of-at91.c:sdhci_at91_runtime_resume(), if
clk_prepare_enable(priv->hclock) fails, the subsequent call to
sdhci_at91_set_clks_presets() will perform MMIO accesses to the SDHCI
controller without the clock being enabled. This can trigger a synchronous
external abort.

>  
>  		priv->restore_needed = false;
>  		goto out;
>  	}
[ ... ]
> @@ -344,9 +350,10 @@ static int sdhci_at91_probe(struct platform_device *pdev)
>  		return dev_err_probe(&pdev->dev, PTR_ERR(priv->gck),
>  				     "failed to get multclk\n");
>  
> -	ret = sdhci_at91_set_clks_presets(&pdev->dev);
> -	if (ret)
> -		return ret;
> +	clk_prepare_enable(priv->hclock);
> +	sdhci_at91_set_clks_presets(&pdev->dev);
> +	clk_prepare_enable(priv->mainck);
> +	clk_prepare_enable(priv->gck);

[Severity: High]
This is a pre-existing issue, but this patch spreads the pattern to new
locations without adding error handling. Is it possible to check the return
values of clk_prepare_enable() here?

In drivers/mmc/host/sdhci-of-at91.c:sdhci_at91_probe(), if
clk_prepare_enable(priv->hclock) fails, the initialization will continue
and the MMIO accesses in sdhci_at91_set_clks_presets() will lead to a
system crash rather than a graceful failure of the probe function.

>  
>  	priv->restore_needed = false;
>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260907132617.1198548-1-robert.marko@sartura.hr?part=2