Re: [PATCH v2 2/4] dt-bindings: net: qca,ar803x: Add clocks property for IPQ5018 PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The PHY's link_change_notify callback naively toggles clocks based on link state without tracking its own enable state, leading to severe clock leaks and unbalanced disables.
--

commit b2bbbd0f6e2e237442a168520c225e85c25f2d4a
Author: George Moussalem <george.moussalem@outlook.com>

net: phy: at803x: add RX and TX clock management for IPQ5018 PHY

Acquire and enable the RX and TX clocks for the IPQ5018 PHY. These clocks
are required for the PHY's datapath to function correctly. In addition,
gate the clocks upon link state changes for improved power management.

> diff --git a/drivers/net/phy/qcom/at803x.c b/drivers/net/phy/qcom/at803x.c
> index 63726cf98cd42..b7361a14220d8 100644
> --- a/drivers/net/phy/qcom/at803x.c
> +++ b/drivers/net/phy/qcom/at803x.c
[ ... ]
> @@ -1062,6 +1065,16 @@ static int ipq5018_config_init(struct phy_device *phydev)
>  
>  static void ipq5018_link_change_notify(struct phy_device *phydev)
>  {
> +	struct ipq5018_priv *priv = phydev->priv;
> +
> +	if (phydev->link) {
> +		clk_enable(priv->rx_clk);
> +		clk_enable(priv->tx_clk);
> +	} else {
> +		clk_disable(priv->rx_clk);
> +		clk_disable(priv->tx_clk);
> +	}
> +

[Severity: High]
Can this logic in ipq5018_link_change_notify() lead to unbalanced clock
enable and disable operations?

The callback is executed by phy_process_state_change() for every change in
phydev->state.

If an interface is brought down manually, phy_stop() transitions the state
to PHY_HALTED and calls phy_process_state_change(). At this exact moment,
phydev->link has not been updated yet and remains 1 if the interface was
running. Consequently, this callback executes a duplicate clk_enable() on an
already enabled clock, incrementing the enable count to 2 and causing a
permanent clock leak.

Conversely, if the interface is brought down when the link is already down
(meaning phydev->link == 0 and the clock was already disabled during the
PHY_UP to PHY_NOLINK transition), phy_process_state_change() triggers the
callback again. The callback reads phydev->link == 0 and executes a
duplicate clk_disable() on an already disabled clock.

This triggers an unbalanced clock disable kernel warning and underflows the
clock's enable count, which can corrupt the system's clock tree and block
power state transitions.

Could we track the clock's enable state within struct ipq5018_priv to prevent
these duplicate operations during state machine transitions?

>  	/*
>  	 * Reset the FIFO buffer upon link disconnects to clear any residual data
>  	 * which may cause issues with the FIFO which it cannot recover from.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260602-ipq5018-gephy-clocks-v2-0-65a1f1d881f3@outlook.com?part=2