Re: [PATCH 2/3] arm64: qcom: ipq5018: Add GEPHY RX and TX clocks

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] DeviceTree ABI breakage due to requiring newly introduced clock properties on an existing PHY.
- [High] Unbalanced clock enable/disable calls in link state change notifications lead to enable count underflows and kernel warnings.
--

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

arm64: qcom: ipq5018: Add GEPHY RX and TX clocks

This commit adds rx and tx clocks for the IPQ5018 GEPHY in the device tree
to enable the datapath. It specifies the clock handles and their corresponding
names for the ethernet phy node.

> diff --git a/arch/arm64/boot/dts/qcom/ipq5018.dtsi b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> index 6f8004a22a1ff..60c27a6f2b10c 100644
> --- a/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> +++ b/arch/arm64/boot/dts/qcom/ipq5018.dtsi
> @@ -229,6 +229,9 @@ ge_phy: ethernet-phy@7 {
>  				compatible = "ethernet-phy-id004d.d0c0";
>  				reg = <7>;
>  
> +				clocks = <&gcc GCC_GEPHY_RX_CLK>,
> +					 <&gcc GCC_GEPHY_TX_CLK>;
> +				clock-names = "rx", "tx";

[Severity: Critical]
Can requiring these clock properties cause a device tree backward compatibility
regression?

Looking at ipq5018_probe() in drivers/net/phy/qcom/at803x.c:

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

The driver uses devm_clk_get_enabled() to acquire the new rx and tx clocks,
meaning these properties are strictly mandatory for the probe to succeed.

Since this phy was already supported in the kernel, existing device trees
do not contain these newly introduced clock properties.

When booting the new kernel with an older device tree, devm_clk_get_enabled()
will return -ENOENT, causing ipq5018_probe() to abort.

Should the driver gracefully handle missing clocks on older device trees by
using devm_clk_get_optional_enabled()?

[Severity: High]
Will gating these clocks based on link state result in an enable count underflow?

Looking at ipq5018_link_change_notify() in drivers/net/phy/qcom/at803x.c:

	if (phydev->link) {
		clk_enable(priv->rx_clk);
		clk_enable(priv->tx_clk);
	} else {
		clk_disable(priv->rx_clk);
		clk_disable(priv->tx_clk);
	}

Since phydev->link can be false in multiple consecutive states (like PHY_NOLINK
and PHY_HALTED), won't clk_disable() be called multiple times in a row without a
matching clk_enable(), causing the clock enable count to underflow and triggering
a kernel warning?

Also, transitioning to link up increments the count to 2. If the device is
unbound while the link is up, devres cleanup only reduces the count by 1.

Could this lead to a clock reference leak?

>  				resets = <&gcc GCC_GEPHY_MISC_ARES>;
>  			};
>  		};

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