Re: [PATCH 5/5] arm64: dts: altera: agilex72: add clock manager support

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Clock gate operations (.enable, .disable, .is_enabled) are missing from gateclk_ops, rendering clock gating non-functional.
- [High] Probe error path leaves non-devm clocks registered with CCF while their backing MMIO regions are unmapped, leading to a use-after-free panic.
--

--- Patch [5]: [PATCH 5/5] arm64: dts: altera: agilex72: add clock manager support ---
commit 9ede61d9f11826e0bfb7cdf0bab36155fec8e509
Author: Chiam, Evon <evon.chiam@altera.com>

arm64: dts: altera: agilex72: add clock manager support

Add the Agilex72 clock manager node and input reference clocks, and
wire the UART clocks through the clock manager instead of a fixed
clock. Set the SoCDK board-specific input clock frequencies.

> diff --git a/drivers/clk/socfpga/clk-agilex72.c b/drivers/clk/socfpga/clk-agilex72.c
> new file mode 100644
> index 0000000000000..e350d8a9bb4d9
> --- /dev/null
> +++ b/drivers/clk/socfpga/clk-agilex72.c
> @@ -0,0 +1,1181 @@
[ ... ]
> +static const struct clk_ops peri_cnt_clk_ops = {
> +	.recalc_rate = agilex72_clk_peri_cnt_clk_recalc_rate,
> +	.get_parent = agilex72_clk_periclk_get_parent,
> +};
> +
> +static const struct clk_ops gateclk_ops = {
> +	.recalc_rate = agilex72_gate_clk_recalc_rate,
> +	.get_parent  = agilex72_gate_get_parent,
> +};

[Severity: High]
Since gateclk_ops omits the standard clock gating callbacks like .enable,
.disable, and .is_enabled, won't this prevent the kernel from toggling hardware
gates?

If a peripheral's clock was disabled by the bootloader, calling
clk_prepare_enable() from a driver will silently return success without
modifying the hardware. Subsequent register accesses to that peripheral could
trigger a synchronous external abort.

[ ... ]
> +	init.ops = &gateclk_ops;
> +
> +	init.name        = clks->name;
> +	init.flags       = 0;
> +	init.num_parents = clks->num_parents;
> +	init.parent_names = clks->parent_names;
> +	socfpga_clk->hw.hw.init = &init;
> +
> +	hw_clk = &socfpga_clk->hw.hw;
> +
> +	ret = clk_hw_register(NULL, &socfpga_clk->hw.hw);
> +	if (ret) {
> +		kfree(socfpga_clk);
> +		return ERR_PTR(ret);
> +	}
> +	return hw_clk;
> +}

[Severity: High]
Individual clocks are registered into the Common Clock Framework using
clk_hw_register() here without being tied to the device's devres lifecycle.

If devm_clk_hw_register_fixed_factor() fails later during
agilex72_clkmgr_init(), the probe will abort and the devres framework will
automatically unmap the MMIO bases.

However, these non-devm clocks registered earlier will not be unregistered.
Could this leave dangling pointers in the CCF?

[ ... ]
> +	/*
> +	 * usb31_ref_clk is a 1:1 alias of usb31_suspend_clk. The DWC3
> +	 * controller uses the same source for both its suspend and reference
> +	 * clock inputs on this SoC. Register it as a fixed-factor (passthrough)
> +	 * clock so that enabling it propagates to the parent gate without
> +	 * touching any gate register of its own.
> +	 */
> +	clk_data->clk_data.hws[AGILEX72_USB31_REF_CLK] =
> +		devm_clk_hw_register_fixed_factor(dev, "usb31_ref_clk",
> +						  "usb31_suspend_clk", 0, 1, 1);
> +	if (IS_ERR(clk_data->clk_data.hws[AGILEX72_USB31_REF_CLK]))
> +		return dev_err_probe(dev,
> +			PTR_ERR(clk_data->clk_data.hws[AGILEX72_USB31_REF_CLK]),
> +			"failed to register clock usb31_ref_clk\n");

[Severity: High]
This error path fails the probe without unregistering the previously registered
non-devm clocks.

If CCF later walks the clock tree (e.g., via a read of
/sys/kernel/debug/clk/clk_summary), it will invoke the .recalc_rate callbacks
of these leaked clocks. Those callbacks will dereference the now-unmapped memory
pointers, causing an external abort/kernel panic.

Is there a way to unregister them to avoid leaving dangling pointers, or should
devm_clk_hw_register() be used for all clocks?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1786006669.git.evon.chiam@altera.com?part=5