Re: [PATCH net-next 2/3] net: mdio: mux-meson-g12a: Make sure clk_init_data is fully initialized

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Busy-wait loop of up to 1 second with interrupts disabled in atomic clock enable callback
--

--- Patch [2]: [PATCH net-next 2/3] net: mdio: mux-meson-g12a: Make sure clk_init_data is fully initialized ---
commit 061a72e517f35ad4bc41a274d2b768db77d84ad1
Author: Geert Uytterhoeven <geert+renesas@glider.be>
Subject: net: mdio: mux-meson-g12a: Make sure clk_init_data is fully initialized

This commit ensures the clk_init_data structure is fully initialized by
adding an empty initializer. This prevents subtle bugs related to mutually
exclusive members when different stack initialization patterns are enabled.

> diff --git a/drivers/net/mdio/mdio-mux-meson-g12a.c b/drivers/net/mdio/mdio-mux-meson-g12a.c
> --- a/drivers/net/mdio/mdio-mux-meson-g12a.c
> +++ b/drivers/net/mdio/mdio-mux-meson-g12a.c
> @@ -223,7 +223,7 @@ static int g12a_ephy_glue_clk_register(struct device *dev)
>  {
>  	struct g12a_mdio_mux *priv = dev_get_drvdata(dev);
>  	const char *parent_names[PLL_MUX_NUM_PARENT];
> -	struct clk_init_data init;
> +	struct clk_init_data init = {};

[Severity: High]
This is a pre-existing issue, but while reviewing this clock initialization, 
I noticed a potential lockup in the related PLL operations.

Does g12a_ephy_pll_enable() risk triggering the hard lockup detector?

The g12a_ephy_pll_ops struct is used later in this function to initialize 
the pll clock, and its .enable callback maps to g12a_ephy_pll_enable():

drivers/net/mdio/mdio-mux-meson-g12a.c:g12a_ephy_pll_enable() {
    ...
    return readl_poll_timeout(pll->base + ETH_PLL_CTL0, val,
                              val & PLL_CTL0_LOCK_DIG, 0, PLL_LOCK_TIMEOUT);
}

The clock framework calls the .enable callback with the enable_lock spinlock
held and local interrupts disabled. Since the delay parameter is 0 and 
PLL_LOCK_TIMEOUT is 1,000,000, readl_poll_timeout() evaluates to a tight
busy-wait loop. 

If the PLL hardware fails to lock or takes a long time, could this stall 
the CPU for up to 1 second in an interrupt-disabled context?

Would it be possible to move this hardware initialization sequence to the 
.prepare callback instead, which executes in a preemptible context where 
sleeping delays are permitted?

>  	struct g12a_ephy_pll *pll;
>  	struct clk_mux *mux;
>  	struct clk *clk;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1787240118.git.geert+renesas@glider.be?part=2

_______________________________________________
linux-amlogic mailing list
linux-amlogic@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-amlogic