Re: [PATCH 3/3] net: phy: qca,at803x: add RX and TX clock management for IPQ5018 PHY

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Unbalanced clock reference counting causing CCF underflow warnings
--

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

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

This patch acquires and enables the RX and TX clocks for the IPQ5018
PHY datapath. It also gates these clocks upon link state changes to
improve 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]
Will this lead to unbalanced clock reference counts and Common Clock
Framework (CCF) underflow warnings?

Because ipq5018_link_change_notify() is invoked on every PHY state
transition, it can be called multiple times consecutively while
phydev->link remains false (for example, transitioning from PHY_NOLINK
to PHY_HALTED). This would result in multiple calls to clk_disable()
without corresponding clk_enable() calls.

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

[ ... ]

> @@ -1084,6 +1097,16 @@ static int ipq5018_probe(struct phy_device *phydev)
>  	priv->set_short_cable_dac = of_property_read_bool(dev->of_node,
>  							  "qcom,dac-preset-short-cable");
>  
> +	priv->rx_clk = devm_clk_get_enabled(dev, "rx");
> +	if (IS_ERR(priv->rx_clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->rx_clk),
> +				     "failed to get and enable RX clock\n");
> +

[Severity: High]
If the driver unbinds while the link is down, won't the unconditional
cleanup registered by devm_clk_get_enabled() trigger another CCF
underflow warning?

The initial devm_clk_get_enabled() sets the enable count to 1. If the
link goes down, the count drops to 0. When devres cleans up on unbind,
it will attempt to disable the clock again. Should state tracking be
added, or should this use devm_clk_get() instead if the clock state
is going to be managed dynamically?

> +	priv->tx_clk = devm_clk_get_enabled(dev, "tx");
> +	if (IS_ERR(priv->tx_clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->tx_clk),
> +				     "failed to get and enable TX clock\n");
> +

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260601-ipq5018-gephy-clocks-v1-0-2df8287712c3@outlook.com?part=3