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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The driver misuses the internal `__clk_is_enabled()` API and includes `<linux/clk-provider.h>` to manage clock toggling, leading to reference count underflows and unintentional shutdown of shared clocks.
- [High] Missing `.remove` callback leads to leaked clock enable counts when the PHY device is unbound.
- [Medium] Unbalanced resource release in the error path of TX clock enablement causes reference underflows.
- [Medium] The driver asserts the hardware FIFO reset while the RX and TX clocks are disabled, failing to properly handle error paths and potentially leaving the hardware in an inconsistent state.
--

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

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

This commit adds RX and TX clock management for the IPQ5018 PHY. It manages
the clocks based on link state changes to improve power efficiency.

> diff --git a/drivers/net/phy/qcom/at803x.c b/drivers/net/phy/qcom/at803x.c
> index 63726cf98cd42..99bc710531a48 100644
> --- a/drivers/net/phy/qcom/at803x.c
> +++ b/drivers/net/phy/qcom/at803x.c
> @@ -19,6 +19,8 @@
>  #include <linux/regulator/consumer.h>
>  #include <linux/of.h>
>  #include <linux/phylink.h>
> +#include <linux/clk.h>
> +#include <linux/clk-provider.h>

[Severity: High]
Is it appropriate to include linux/clk-provider.h here? This appears to be
a layering violation just to access the internal __clk_is_enabled API.

>  #include <linux/reset.h>
>  #include <linux/phy_port.h>
>  #include <dt-bindings/net/qca-ar803x.h>
[ ... ]
> @@ -1062,6 +1066,35 @@ 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;
> +	int ret;
> +
> +	if (phydev->link) {
> +		if (!__clk_is_enabled(priv->rx_clk)) {

[Severity: High]
Could checking __clk_is_enabled() cause a reference count underflow?

Since __clk_is_enabled() returns the global hardware state, it might return
true if the clock is shared or left enabled by the bootloader. If the driver
skips calling clk_prepare_enable() here, it fails to acquire its own
reference. Later on link-down, it might call clk_disable_unprepare() and
underflow the reference count.

> +			ret = clk_prepare_enable(priv->rx_clk);
> +			if (ret) {
> +				dev_err(&phydev->mdio.dev,
> +					"failed to enable RX clock\n");
> +				goto reset_fifo;
> +			}
> +		}
> +
> +		if (!__clk_is_enabled(priv->tx_clk)) {
> +			ret = clk_prepare_enable(priv->tx_clk);
> +			if (ret) {
> +				dev_err(&phydev->mdio.dev,
> +					"failed to enable TX clock\n");
> +				clk_disable_unprepare(priv->rx_clk);

[Severity: Medium]
Will this error path unconditionally drop a reference to the RX clock even
if the driver bypassed enabling it just above?

If rx_clk was already enabled in hardware, the driver didn't take a
reference, so this call to clk_disable_unprepare() could result in an
immediate clock reference count underflow.

> +			}
> +		}
> +	} else {
> +		if (__clk_is_enabled(priv->rx_clk))
> +			clk_disable_unprepare(priv->rx_clk);
> +		if (__clk_is_enabled(priv->tx_clk))
> +			clk_disable_unprepare(priv->tx_clk);
> +	}
> +
> +reset_fifo:
>  	/*
>  	 * Reset the FIFO buffer upon link disconnects to clear any residual data
>  	 * which may cause issues with the FIFO which it cannot recover from.
>  	 */
>  	mdiobus_modify_changed(phydev->mdio.bus, phydev->mdio.addr,
>  			       IPQ5018_PHY_FIFO_CONTROL, IPQ5018_PHY_FIFO_RESET,
>  			       phydev->link ? IPQ5018_PHY_FIFO_RESET : 0);

[Severity: Medium]
Can the hardware FIFO reset take effect if the underlying datapath clocks are
not running? 

During link down, the RX and TX clocks are disabled right before this
register write. Additionally, if clock enablement fails during link-up,
the driver jumps to reset_fifo and writes to the control register with the
clocks disabled, potentially leaving the hardware in an inconsistent state.

>  }
>  
>  static int ipq5018_probe(struct phy_device *phydev)
>  {
>  	struct device *dev = &phydev->mdio.dev;
>  	struct ipq5018_priv *priv;
[ ... ]
> +	priv->rx_clk = devm_clk_get(dev, "rx");
> +	if (IS_ERR(priv->rx_clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->rx_clk),
> +				     "failed to get RX clock\n");
> +
> +	priv->tx_clk = devm_clk_get(dev, "tx");
> +	if (IS_ERR(priv->tx_clk))
> +		return dev_err_probe(dev, PTR_ERR(priv->tx_clk),
> +				     "failed to get TX clock\n");
> +
>  	priv->rst = devm_reset_control_array_get_exclusive(dev);
>  	if (IS_ERR(priv->rst))
>  		return dev_err_probe(dev, PTR_ERR(priv->rst),
>  				     "failed to acquire reset\n");
>  
>  	ret = reset_control_reset(priv->rst);
>  	if (ret)
>  		return dev_err_probe(dev, ret, "failed to reset\n");
>  
>  	phydev->priv = priv;
>  
>  	return 0;
>  }

[Severity: High]
Does this driver leak clock enable counts on unbind?

The clocks are dynamically prepared and enabled during link changes, but
the PHY driver structure lacks a .remove callback to clean them up.
If the PHY device is forcefully unbound, generic phy_remove() clears the
state, and the devm framework drops the clock handles, but the hardware
enable counts are never decremented, permanently leaking the clocks.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-ipq5018-gephy-clocks-v3-0-f232d9ca0966@outlook.com?part=4