Re: [PATCH v2 4/5] clk: sunxi-ng: add support for the A523/T527 CPU CCU

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential issue(s) to consider:
- [High] clk: sunxi-ng: probe modifies active CPU PLL dividers without reparenting
- [High] clk: sunxi-ng: missing mux notifier for DSU PLL rate changes
- [High] clk: sunxi-ng: UAF on module unload due to unregistered notifiers
- [Medium] kconfig: SUN55I_A523_CPU_CCU inappropriately uses default y
- [Medium] clk: sunxi-ng: modeling non-existent divider for DSU mux
- [Medium] clk: sunxi-ng: probe enables PLLs without polling the lock bit
- [Medium] clk: sunxi-ng: ignored return values from ccu_mux_notifier_register()
- [Medium] clk: sunxi-ng: race condition exposing clocks before notifiers are attached
- [Medium] clk: sunxi-ng: missing MODULE_DEVICE_TABLE breaks module autoloading
--

commit 719a9a0732629f9d936e50d92e8a19aea0aef6c5
Author: Andre Przywara <andre.przywara@arm.com>

clk: sunxi-ng: add support for the A523/T527 CPU CCU

This commit adds support for the Allwinner A523/T527 CPU CCU, managing the 
clock controllers for the CPU clusters and the DSU. It defines the PLLs 
used to drive the core frequencies and their relations, allowing the kernel 
to adjust the core frequency dynamically.

> diff --git a/drivers/clk/sunxi-ng/Kconfig b/drivers/clk/sunxi-ng/Kconfig
> index 6af2d020e03e9..c34af60139124 100644
> --- a/drivers/clk/sunxi-ng/Kconfig
> +++ b/drivers/clk/sunxi-ng/Kconfig
> @@ -67,6 +67,11 @@ config SUN55I_A523_R_CCU
>  	default ARCH_SUNXI
>  	depends on ARM64 || COMPILE_TEST
>  
> +config SUN55I_A523_CPU_CCU
> +	tristate "Support for the Allwinner A523/T527 CPU CCU"
> +	default y
> +	depends on ARM64 || COMPILE_TEST

[Severity: Medium]
Is it intended to use "default y" here? 

Normally, SoC-specific drivers follow the subsystem convention of 
"default ARCH_SUNXI". This will unconditionally enable the driver on all 
ARM64 builds, causing defconfig pollution on arm64 multi-platform builds.

[ ... ]

> diff --git a/drivers/clk/sunxi-ng/ccu-sun55i-a523-cpu.c b/drivers/clk/sunxi-ng/ccu-sun55i-a523-cpu.c
> new file mode 100644
> index 0000000000000..e505aefc92370
> --- /dev/null
> +++ b/drivers/clk/sunxi-ng/ccu-sun55i-a523-cpu.c

[ ... ]

> +static SUNXI_CCU_MP_DATA_WITH_MUX(pll_cpu_l_div_clk, "pll-cpu-l-div",
> +				  pll_cpu_l_hws, 0x060,
> +				  0, 0,		/* no M */
> +				  16, 2,	/* P */
> +				  0, 0,		/* no mux */
> +				  CLK_SET_RATE_PARENT);		/* flags */
> +static SUNXI_CCU_MP_DATA_WITH_MUX(pll_cpu_b_div_clk, "pll-cpu-b-div",
> +				  pll_cpu_b_hws, 0x064,
> +				  0, 0,		/* no M */
> +				  16, 2,	/* P */
> +				  0, 0,		/* no mux */
> +				  CLK_SET_RATE_PARENT);		/* flags */
> +static SUNXI_CCU_MP_DATA_WITH_MUX(pll_cpu_dsu_div_clk, "pll-cpu-dsu-div",
> +				  pll_cpu_dsu_hws, 0x06c,
> +				  0, 0,		/* no M */
> +				  16, 2,	/* P */
> +				  0, 0,		/* no mux */
> +				  CLK_SET_RATE_PARENT);		/* flags */

[Severity: Medium]
Is it correct to model a P divider here extracting bits 17:16 for the DSU?

The block comment at the top of the file explicitly states "For PLL1 and PLL3
there is an additional shift field... although that applies only to the PLL...",
which implies it does not exist for the DSU (PLL2). Manipulating a non-existent
hardware field could cause issues in frequency calculations.

[ ... ]

> +static int sun55i_a523_cpu_ccu_probe(struct platform_device *pdev)
> +{
> +	const struct sunxi_ccu_desc *desc;
> +	void __iomem *reg;
> +	int i, ret;
> +	u32 val;
> +
> +	desc = of_device_get_match_data(&pdev->dev);
> +	if (!desc)
> +		return -EINVAL;
> +
> +	reg = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(reg))
> +		return PTR_ERR(reg);
> +
> +	/*
> +	 * The user manual recommends to keep the PLLs running, and just
> +	 * gate their output if they are not needed, to avoid interference
> +	 * with other PLLs, since they share a power domain.
> +	 * To comply with this, we use this output gate as the CCF enable bit,
> +	 * so we need to enable all PLLs here. Chances are the bootloader has
> +	 * already enabled at least one PLL, so check if it's already running
> +	 * and locked, before touching it.
> +	 * We set the enable, the LDO and the lock bits, and clear dividers.
> +	 */
> +	for (i = 0; i < ARRAY_SIZE(pll_regs); i++) {
> +		val = readl(reg + pll_regs[i]);
> +		if ((val & GENMASK(31, 28)) != GENMASK(31, 28)) {
> +			val |= BIT(31) | BIT(30) | BIT(29) | BIT(26);
> +			val &= ~GENMASK(21, 16); /* covering PLL_P and PLL_M0 */

[Severity: High]
Could modifying these dividers cause a hardware crash? 

If the bootloader leaves the PLL active but misses a control bit (e.g., bit 30),
this code instantly clears the P and M0 dividers. Modifying active dividers 
without first reparenting the CPU clock to a bypass might hang the system. 
Also, if the bootloader setup completely matches the lock mask, this block is 
skipped, leaving P and M0 uncleared, but the software CCF model blindly 
assumes they are 1.

> +			writel(val, reg + pll_regs[i]);
> +		}

[Severity: Medium]
Does this code need to poll the lock bit (BIT(28)) before proceeding?

The code sets BIT(31) to physically enable the PLLs in the CCU but doesn't
wait for the hardware lock to assert. The CCF enable op only toggles the output
gate, which could silently pass an unlocked PLL to a CPU core.

> +	}
> +
> +	ret = devm_sunxi_ccu_probe(&pdev->dev, reg, desc);
> +	if (ret)
> +		return ret;
> +
> +	/* Reparent CPU during CPU PLL rate changes */
> +	ccu_mux_notifier_register(pll_cpu_l_clk.common.hw.clk,
> +				  &sun55i_a523_cpu_l_nb);

[Severity: Medium]
Is there a race condition exposing clocks before the notifiers are attached?

devm_sunxi_ccu_probe() registers the clocks globally, making them accessible.
Since the reparenting notifiers are attached after this step, a fast consumer
could modify the frequency before the reparenting protection is active.

[Severity: Medium]
Is it safe to ignore the return values of ccu_mux_notifier_register()?

If the internal memory allocation fails, a subsequent cpufreq rate change
could attempt to scale the PLL without reparenting, which could lock up
the CPU.

[Severity: High]
Since this module registers static notifiers, could it cause a use-after-free
on module unload?

There doesn't appear to be a .remove callback to call
clk_notifier_unregister(). If the module is unloaded, the CCF notifier list
would retain dangling pointers.

> +	ccu_mux_notifier_register(pll_cpu_b_clk.common.hw.clk,
> +				  &sun55i_a523_cpu_b_nb);

[Severity: High]
Should there be a mux notifier registered for the DSU PLL as well?

If a DVFS rate change forces the DSU clock to relock, it could output an
unstable clock. CPU clusters are protected by the dynamic reparenting
notifiers above, but the DSU seems to be missing its corresponding notifier.

> +
> +	return ret;
> +}
> +
> +static const struct of_device_id sun55i_a523_cpu_ccu_ids[] = {
> +	{
> +		.compatible = "allwinner,sun55i-a523-cpu-ccu",
> +		.data = &sun55i_a523_cpu_ccu_desc,
> +	},
> +	{ }
> +};

[Severity: Medium]
Is this missing a MODULE_DEVICE_TABLE(of, sun55i_a523_cpu_ccu_ids) declaration?

Compiling the driver as a tristate module without exporting the device ID table
prevents the module alias from being emitted, breaking udev autoloading.

> +
> +static struct platform_driver sun55i_a523_cpu_ccu_driver = {
> +	.probe	= sun55i_a523_cpu_ccu_probe,
> +	.driver	= {
> +		.name			= "sun55i-a523-cpu-ccu",
> +		.suppress_bind_attrs	= true,
> +		.of_match_table		= sun55i_a523_cpu_ccu_ids,
> +	},
> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260917223913.1855301-1-andre.przywara@arm.com?part=4