Re: [PATCH 3/5] clk: socfpga: agilex72: add clock manager driver for Agilex72

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] The `struct clk_init_data init` variable is declared on the stack without zero-initialization, leaving fields like `parent_data` and `parent_hws` containing uninitialized garbage.
- [High] The `gateclk_ops` structure lacks `.enable`, `.disable`, and `.is_enabled` callbacks, rendering software clock gating completely non-functional.
- [High] Independent clocks `dma_0_core_clk` and `dma_0_hs_clk` are registered with the exact same hardware gate register and bit, leading to uncoordinated hardware state changes.
- [High] Probe error paths leak `kzalloc_obj` memory and leave non-devm clocks registered with the CCF, containing dangling `__iomem` pointers that lead to Use-After-Free.
--

--- Patch [3]: [PATCH 3/5] clk: socfpga: agilex72: add clock manager driver for Agilex72 ---
commit fb18cfcc77d7e1e9f2bfe18c6b8c94bdd0fbad23
Author: Chiam, Evon <evon.chiam@altera.com>
Subject: clk: socfpga: agilex72: add clock manager driver for Agilex72

This commit adds a new clock manager driver to support the Altera Agilex72
platform.

> 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

[ ... ]

> +static const struct clk_ops gateclk_ops = {
> +	.recalc_rate = agilex72_gate_clk_recalc_rate,
> +	.get_parent  = agilex72_gate_get_parent,
> +};

[Severity: High]
Are the .enable, .disable, and .is_enabled callbacks intentionally missing
here?

Without these hooks, clk_enable() and clk_disable() calls will return
success without actually writing to the hardware gate registers, leaving the
physical clock ungated or gated incorrectly.

[ ... ]

> +	{ AGILEX72_DMA_0_CORE_CLK, "dma_0_core_clk", lsp_main_parent,
> +	  1, 0xd0, 15, 0, 0, 0, 0, 0, 0 },
> +	{ AGILEX72_DMA_0_HS_CLK, "dma_0_hs_clk", lsp_mp_parent,
> +	  1, 0xd0, 15, 0, 0, 0, 0, 0, 0 },
> +	{ AGILEX72_DMA_1_CORE_CLK, "dma_1_core_clk", lsp_main_parent,
> +	  1, 0xd0, 16, 0, 0, 0, 0, 0, 0 },
> +	{ AGILEX72_DMA_1_HS_CLK, "dma_1_hs_clk", lsp_mp_parent,
> +	  1, 0xd0, 16, 0, 0, 0, 0, 0, 0 },

[Severity: High]
Will mapping independent clocks to the exact same hardware gate register
and bit cause uncoordinated state changes?

The dma_0_core_clk and dma_0_hs_clk both map to register 0xd0, bit index 15.
Because they are registered as entirely distinct clk_hw instances, if
two consumer paths enable both clocks and one path later disables its clock,
the reference count for that specific clock drops to 0. This clears bit 15,
instantaneously cutting off the clock for the other active consumer.

The same conflict appears to occur for dma_1_core_clk and dma_1_hs_clk on
bit 16.

[ ... ]

> +static struct clk_hw *
> +agilex72_register_pll(const struct agilex72_pll_clock *clks,
> +		      void __iomem *base, void __iomem *pll_base)
> +{
> +	struct clk_hw *hw_clk;
> +	struct agilex72_pll *pll_clk;
> +	struct clk_init_data init;
> +	const char *name = clks->name;
> +	int ret;

[Severity: Critical]
Does this leave the init structure partially uninitialized on the stack?

Because init is not zero-initialized, fields like parent_data and parent_hws
might contain uninitialized stack memory. When passed to clk_hw_register(),
the clock framework might check these garbage values and attempt to dereference
them.

This same pattern appears in agilex72_register_periph(),
agilex72_register_cnt_periph(), and agilex72_register_gate().

[ ... ]

> +static struct clk_hw *
> +agilex72_register_gate(const struct agilex72_gate_clock *clks,
> +		       void __iomem *regbase)
> +{
> +	struct clk_hw *hw_clk;
> +	struct agilex72_gate_clk *socfpga_clk;
> +	struct clk_init_data init;
> +	int ret;
> +
> +	socfpga_clk = kzalloc_obj(*socfpga_clk);
> +	if (!socfpga_clk)
> +		return ERR_PTR(-ENOMEM);
> +
> +	socfpga_clk->hw.reg = regbase + clks->gate_reg;
> +	socfpga_clk->hw.bit_idx = clks->gate_idx;

[Severity: High]
If the gateclk_ops are updated to use standard gate operations, will this
cause a null pointer dereference?

The hw.lock spinlock assignment for struct clk_gate is missing here.

[ ... ]

> +	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]
Does this error path leak memory and leave dangling clock registrations
if devm_clk_hw_register_fixed_factor() fails?

The earlier clocks are allocated with kzalloc_obj() and registered using
clk_hw_register(NULL, ...), which bypasses automatic devres cleanup. If this
returns an error, devres will automatically unmap the MMIO resources, but
the clocks remain registered.

If a tool like debugfs clk_summary attempts to access the still-registered
clocks, the framework will invoke driver callbacks that read from the unmapped
memory, resulting in a synchronous external abort. Could we add proper
unregister calls on the error path, or use devm_clk_hw_register() for all
clocks?

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