Re: [PATCH] ARM: dts: marvell: add missing SDHCI clock-names

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unchecked return value of clk_prepare_enable() leading to potential unclocked hardware accesses.
- [High] Suppression of legitimate errors (e.g., -EPROBE_DEFER) when fetching the optional 'core' clock.
--

--- Patch [1]: [PATCH] ARM: dts: marvell: add missing SDHCI clock-names ---
commit 4be703994612e3151cc7dd6a9b229ae270b6b4da
Author: Rosen Penev <rosenp@gmail.com>

ARM: dts: marvell: add missing SDHCI clock-names

The marvell,armada-380-sdhci binding requires "clock-names", and
the sdhci-pxav3 driver requests its io clock by name before falling
back to an unnamed lookup. Both Armada 38x and 39x nodes only carry
an unnamed clocks reference, so dtbs_check reports "'clock-names'
is a required property" on every board.

Add clock-names = "io" to both nodes. No functional change: the
driver already obtains the clock through its fallback.

> diff --git a/arch/arm/boot/dts/marvell/armada-38x.dtsi b/arch/arm/boot/dts/marvell/armada-38x.dtsi
> index 1d616edda322d..0618779090d04 100644
> --- a/arch/arm/boot/dts/marvell/armada-38x.dtsi
> +++ b/arch/arm/boot/dts/marvell/armada-38x.dtsi

[ ... ]

> @@ -627,6 +627,7 @@ sdhci: sdhci@d8000 {
>  					<0x18454 0x4>;
>  				interrupts = <GIC_SPI 25 IRQ_TYPE_LEVEL_HIGH>;
>  				clocks = <&gateclk 17>;
> +				clock-names = "io";

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the
sdhci-pxav3 driver mentioned in the commit message, I noticed a pre-existing
issue in how these clocks are handled.

Looking at sdhci_pxav3_probe() in drivers/mmc/host/sdhci-pxav3.c:

drivers/mmc/host/sdhci-pxav3.c:sdhci_pxav3_probe() {
    ...
	pltfm_host->clk = pxa->clk_io;
	clk_prepare_enable(pxa->clk_io);
    ...
	pxa->clk_core = devm_clk_get(dev, "core");
	if (!IS_ERR(pxa->clk_core))
		clk_prepare_enable(pxa->clk_core);
    ...
}

Are we ignoring the return values of clk_prepare_enable() here? If the clock
subsystem fails to enable the IO or core clocks during probe, the driver
might access hardware registers while the clock is disabled or failed to
enable, which could lead to a synchronous external abort.

[Severity: High]
This is also a pre-existing issue, but does this code suppress legitimate
errors like -EPROBE_DEFER when fetching the optional 'core' clock?

drivers/mmc/host/sdhci-pxav3.c:sdhci_pxav3_probe() {
    ...
	pxa->clk_core = devm_clk_get(dev, "core");
	if (!IS_ERR(pxa->clk_core))
		clk_prepare_enable(pxa->clk_core);
    ...
}

If devm_clk_get() returns -EPROBE_DEFER because the clock provider is not
yet ready, the if (!IS_ERR(pxa->clk_core)) condition treats this as if the
clock is absent and proceeds silently without it. Could this break device
initialization if the clock is actually required but deferred?

The unchecked clk_prepare_enable() issue also appears in the runtime
resume path:

drivers/mmc/host/sdhci-pxav3.c:sdhci_pxav3_runtime_resume() {
    ...
	clk_prepare_enable(pxa->clk_io);
	if (!IS_ERR(pxa->clk_core))
		clk_prepare_enable(pxa->clk_core);

	sdhci_runtime_resume_host(host, 0);
	return 0;
}

Can the clocks fail to enable during runtime resume, leading to the same
risk of unclocked hardware access when sdhci_runtime_resume_host() is
called?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260823234721.72903-1-rosenp@gmail.com?part=1