Re: [PATCH v2 4/4] net: phy: at803x: add RX and TX clock management for IPQ5018 PHY
flat view
From: Jakub Kicinski <kuba@kernel.org>
Date: 2026-06-05 01:33:08
Also in:
linux-arm-msm, linux-devicetree, lkml
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
+ } 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.
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
+ 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