Re: [PATCH v2 4/4] net: phy: at803x: add RX and TX clock management for IPQ5018 PHY

[Jakub Kicinski]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
net: phy: at803x: add RX and TX clock management for IPQ5018 PHY

This commit adds RX and TX clock acquisition for the IPQ5018 PHY using
devm_clk_get_enabled(). It also attempts to manage power by manually toggling
these clocks in ipq5018_link_change_notify() based on the link state.

> 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);

[Severity: Medium]
Should the return values of clk_enable() be checked here?

If clk_enable(priv->rx_clk) succeeds but clk_enable(priv->tx_clk) fails,
the hardware might be left in a partially enabled state without any logged
warning. Would it be better to explicitly rollback and disable the
previously enabled clocks if an error occurs to maintain a consistent state?

> +	} else {
> +		clk_disable(priv->rx_clk);
> +		clk_disable(priv->tx_clk);
> +	}
> +
>  	/*
>  	 * 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");

[Severity: High]
Does mixing devm_clk_get_enabled() with manual clk_enable() and clk_disable()
cause enable count imbalances?

Since devm_clk_get_enabled() establishes a base enable count of 1 and
registers an unconditional devres teardown action to call
clk_disable_unprepare(), manually toggling the clocks in
ipq5018_link_change_notify() could lead to a few unintended behaviors:

1. The count initializes to 1. If the link comes up, the count increases
   to 2. When the link goes down, it drops to 1. The hardware clocks are
   never actually gated since the count never reaches 0.

2. If the driver is unbound while the link is down (after a manual
   clk_disable()), devres will call clk_disable() again, dropping the
   enable_count below zero and triggering a kernel WARN.

3. If phy_stop() halts the PHY, it invokes the notify callback before
   phydev->link is cleared. If the link was up, clk_enable() is called
   again, inflating the count to 3. Devres cleanup will then only drop
   the count by 1, permanently leaking the clock reference.

Could this be resolved by using devm_clk_get() instead, and handling the
prepare/enable states directly?

> +	if (IS_ERR(priv->rx_clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->rx_clk),
> +				     "failed to get and enable RX clock\n");
> +
> +	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");
> +
>  	priv->rst = devm_reset_control_array_get_exclusive(dev);

[ ... ]
-- 
pw-bot: cr