Re: [PATCH v8 5/7] clk: nuvoton: ma35d1: Reparent SYSPLL correctly

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] clk: nuvoton: ma35d1: Modification of global static state during probe
--

commit ee5a5a1eb9fdc7516949d10347d8380f083a7848
Author: Miquel Raynal <miquel.raynal@bootlin.com>

clk: nuvoton: ma35d1: Reparent SYSPLL correctly

This commit reparents SYSPLL under HXT_GATE to properly define the clock tree
topology. It also reuses a devm_ helper for fixed clocks to make them
device managed.

> diff --git a/drivers/clk/nuvoton/clk-ma35d1.c b/drivers/clk/nuvoton/clk-ma35d1.c
> index 2e6e1c65d01e5..6763f0fddc944 100644
> --- a/drivers/clk/nuvoton/clk-ma35d1.c
> +++ b/drivers/clk/nuvoton/clk-ma35d1.c
> @@ -73,6 +73,8 @@ static const struct clk_parent_data lxtg_parent_data[] = {
>  	{ .fw_name = "lxt", .name = "lxt" },
>  };
>  
> +static struct clk_parent_data syspll_parent_data[1] = {};
> +
>  /* Mux parent selection, by index into the controller's clock table */
>  static const int ca35clk_parent_idx[] = { HXT_GATE, CAPLL, DDRPLL };

[ ... ]

> @@ -275,22 +283,26 @@ static int ma35d1_clocks_probe(struct platform_device *pdev)

[ ... ]

>  	hws[CAPLL] = ma35d1_reg_clk_pll(dev, CAPLL, pllmode[0], "capll",
>  					hws[HXT_GATE], clk_base + REG_CLK_PLL0CTL0);
> -	hws[SYSPLL] = ma35d1_clk_fixed("syspll", 180000000);
> +	syspll_parent_data[0].hw = hws[HXT_GATE];
> +	hws[SYSPLL] = ma35d1_clk_fixed_parent_data(dev, "syspll", 180000000, syspll_parent_data);

[Severity: Low]
Is it safe to modify this global static array in the probe function?

In drivers/clk/nuvoton/clk-ma35d1.c, syspll_parent_data is declared as a
global static array and then modified here in ma35d1_clocks_probe() by
assigning it an instance-specific dynamically allocated hardware pointer.

Since the clock framework copies clk_parent_data during registration, could
syspll_parent_data be safely declared as a local stack variable inside
ma35d1_clocks_probe() to avoid modifying global state?

Modifying global state here violates device model isolation principles, which
could lead to data races or dangling pointers if multiple driver instances
were to exist.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261004-perso-ma35d1-upstream-clk-v8-0-e1a106a128c0@bootlin.com?part=5