Re: [PATCH v2 3/6] mmc: sdhci-of-at91: add LAN969x support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing return value checks for clk_prepare_enable() in probe and resume paths.
- [High] Potential kernel panic due to division by zero if clock rates evaluate to zero.
--

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

mmc: sdhci-of-at91: add LAN969x support
    
LAN969x uses the same internally generated base clock layout as SAM9X60,
but its SDMMC controller stops responding when runtime PM gates its clocks.
Add LAN969x-specific SoC data using the SAM9X60 clock layout and select the
option to leave its clocks enabled across runtime suspend.

> diff --git a/drivers/mmc/host/sdhci-of-at91.c b/drivers/mmc/host/sdhci-of-at91.c
> index b7e2a89da348a..e7affca8da25f 100644
> --- a/drivers/mmc/host/sdhci-of-at91.c
> +++ b/drivers/mmc/host/sdhci-of-at91.c
> @@ -158,9 +158,17 @@ static const struct sdhci_at91_soc_data soc_data_sam9x60 = {
>  	.divider_for_baseclk = 2,
>  };
>  
> +static const struct sdhci_at91_soc_data soc_data_lan969x = {
> +	.pdata = &sdhci_sama5d2_pdata,
> +	.baseclk_is_generated_internally = true,
> +	.keep_clks_on = true,
> +	.divider_for_baseclk = 2,
> +};

[Severity: High]
This isn't a bug introduced by this patch, but it exists in the baseline.

In sdhci_at91_probe() and sdhci_at91_runtime_resume(), the return values of
clk_prepare_enable() are ignored before accessing hardware registers.

For example, in sdhci_at91_probe():

	clk_prepare_enable(priv->hclock);
	sdhci_at91_set_clks_presets(&pdev->dev);
	clk_prepare_enable(priv->mainck);
	clk_prepare_enable(priv->gck);

And when restoring state in sdhci_at91_runtime_resume():

	if (priv->restore_needed) {
		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);
		}

If these clocks fail to enable, the driver silently proceeds to access hardware
registers via sdhci_at91_set_clks_presets() and subsequent MMIO operations.
Could this lead to a synchronous external abort and a kernel panic from
accessing unclocked peripheral registers? Furthermore, would ignoring these
failures cause unbalanced clock reference counts during cleanup?

[Severity: High]
This is a pre-existing issue, but the configuration here is used during clock
setup in sdhci_at91_set_clks_presets(), which might have a division by zero:

	gck_rate = clk_get_rate(priv->gck);
	if (priv->soc_data->baseclk_is_generated_internally)
		clk_base_rate = gck_rate / priv->soc_data->divider_for_baseclk;
	else
		clk_base_rate = clk_get_rate(priv->mainck);

	clk_base = clk_base_rate / 1000000;
	clk_mul = gck_rate / clk_base_rate - 1;

If the clock is misconfigured in the device tree or fails to initialize,
clk_get_rate() can legitimately return 0. If clk_base_rate evaluates to 0,
will the computation of clk_mul trigger a kernel panic due to an unhandled
division by zero exception?

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