Re: [PATCH net-next 3/3] net: phy: air_en8811h: 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] The PHY driver calls the global `clk_save_context()` and `clk_restore_context()` APIs in its suspend and resume paths, forcefully saving and restoring all clocks in the entire system.
--

--- Patch [3]: [PATCH net-next 3/3] net: phy: air_en8811h: Make sure clk_init_data is fully initialized ---
commit 681a33786ef724d10dc7ce4f726e8ec894967ea4
Author: Geert Uytterhoeven <geert+renesas@glider.be>

net: phy: air_en8811h: Make sure clk_init_data is fully initialized

The clk_init_data structure contains several mutually-exclusive members
for different methods to specify the possible parents of a clock,
prompting drivers to initialize only the members they need. However,
not initializing all members may cause subtle issues. This commit makes
sure all members are fully initialized.

> diff --git a/drivers/net/phy/air_en8811h.c b/drivers/net/phy/air_en8811h.c
> index edd49c193e476..d1ab8744288bb 100644
> --- a/drivers/net/phy/air_en8811h.c
> +++ b/drivers/net/phy/air_en8811h.c

[ ... ]

> @@ -1031,7 +1031,7 @@ static const struct clk_ops en8811h_clk_ops = {
>  
>  static int en8811h_clk_provider_setup(struct device *dev, struct clk_hw *hw)
>  {
> -	struct clk_init_data init;
> +	struct clk_init_data init = {};
>  	int ret;
>  
>  	if (!IS_ENABLED(CONFIG_COMMON_CLK))

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing this driver's 
clock handling I noticed a potential power management issue further down in 
the file.

In drivers/net/phy/air_en8811h.c:

static int en8811h_resume(struct phy_device *phydev)
{
	clk_restore_context();

	return genphy_resume(phydev);
}

static int en8811h_suspend(struct phy_device *phydev)
{
	clk_save_context();

	return genphy_suspend(phydev);
}

Is it safe for the PHY driver to call the global clk_save_context() and 
clk_restore_context() APIs during its suspend and resume handlers?

These APIs are intended for core PM operations and iterate over the entire 
system's clock tree to save or restore state for all clocks system-wide.

Could restoring all clocks globally whenever this single leaf PHY resumes
forcefully reset or disrupt the clocks of completely unrelated devices,
potentially causing system lockups or crashes?

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

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