[PATCH net v1] net: phy: dp83640: fix per-bus clock lifetime

[PATCH net v1] net: phy: dp83640: fix per-bus clock lifetime

[xuanqiang.luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

Commit 42e2a9e11a1d ("net: phy: dp83640: improve phydev and driver
removal handling") moved per-bus clock cleanup from module exit to the
remove path. This leaves two lifetime problems.

dp83640_clock_get_bus() publishes a newly allocated clock before
dp83640_probe() allocates its per-PHY private data and registers the PTP
clock. If either operation fails, no PHY is bound and no remove callback
will release the clock, leaking the clock, its pin configuration, and the
reference to the MII bus device.

The remove path can also free a clock after dropping clock_lock. A
concurrent lookup may already have found that clock under
phyter_clocks_lock and be waiting for clock_lock, allowing it to acquire a
freed mutex and access the freed clock.

Use a kref to give each probing or successfully bound PHY a reference to
the per-bus clock. Acquire the reference under phyter_clocks_lock before
making the clock available to the caller, and drop it on probe failure or
when the PHY is removed. kref_put_mutex() serializes the last put with
lookups so that the release callback can remove the clock from the list
before freeing it. A lookup therefore either obtains a reference or cannot
find the clock.

Separate clock locking from the reference helpers so get and put have
their usual lifetime-management meaning.

Fixes: 42e2a9e11a1d ("net: phy: dp83640: improve phydev and driver removal handling")
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/phy/dp83640.c | 62 ++++++++++++++++++++++++---------------
 1 file changed, 38 insertions(+), 24 deletions(-)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 98472abdd3920..10587db95fb0b 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -10,6 +10,7 @@
 #include <linux/crc32.h>
 #include <linux/ethtool.h>
 #include <linux/kernel.h>
+#include <linux/kref.h>
 #include <linux/list.h>
 #include <linux/mii.h>
 #include <linux/module.h>
@@ -130,6 +131,8 @@ struct dp83640_private {
 struct dp83640_clock {
 	/* keeps the instance in the 'phyter_clocks' list */
 	struct list_head list;
+	/* tracks attached or probing PHYs */
+	struct kref kref;
 	/* we create one clock instance per MII bus */
 	struct mii_bus *bus;
 	/* protects extended registers from concurrent access */
@@ -956,6 +959,7 @@ static void decode_status_frame(struct dp83640_private *dp83640,
 static void dp83640_clock_init(struct dp83640_clock *clock, struct mii_bus *bus)
 {
 	INIT_LIST_HEAD(&clock->list);
+	kref_init(&clock->kref);
 	clock->bus = bus;
 	mutex_init(&clock->extreg_lock);
 	mutex_init(&clock->clock_lock);
@@ -999,11 +1003,9 @@ static int choose_this_phy(struct dp83640_clock *clock,
 	return 0;
 }
 
-static struct dp83640_clock *dp83640_clock_get(struct dp83640_clock *clock)
+static void dp83640_clock_lock(struct dp83640_clock *clock)
 {
-	if (clock)
-		mutex_lock(&clock->clock_lock);
-	return clock;
+	mutex_lock(&clock->clock_lock);
 }
 
 /*
@@ -1021,6 +1023,7 @@ static struct dp83640_clock *dp83640_clock_get_bus(struct mii_bus *bus)
 		tmp = list_entry(this, struct dp83640_clock, list);
 		if (tmp->bus == bus) {
 			clock = tmp;
+			kref_get(&clock->kref);
 			break;
 		}
 	}
@@ -1043,14 +1046,38 @@ static struct dp83640_clock *dp83640_clock_get_bus(struct mii_bus *bus)
 out:
 	mutex_unlock(&phyter_clocks_lock);
 
-	return dp83640_clock_get(clock);
+	if (clock)
+		dp83640_clock_lock(clock);
+
+	return clock;
 }
 
-static void dp83640_clock_put(struct dp83640_clock *clock)
+static void dp83640_clock_unlock(struct dp83640_clock *clock)
 {
 	mutex_unlock(&clock->clock_lock);
 }
 
+static void dp83640_clock_release(struct kref *kref)
+{
+	struct dp83640_clock *clock =
+		container_of(kref, struct dp83640_clock, kref);
+
+	list_del(&clock->list);
+	mutex_unlock(&phyter_clocks_lock);
+
+	mutex_destroy(&clock->extreg_lock);
+	mutex_destroy(&clock->clock_lock);
+	put_device(&clock->bus->dev);
+	kfree(clock->caps.pin_config);
+	kfree(clock);
+}
+
+static void dp83640_clock_put(struct dp83640_clock *clock)
+{
+	kref_put_mutex(&clock->kref, dp83640_clock_release,
+		       &phyter_clocks_lock);
+}
+
 static int dp83640_soft_reset(struct phy_device *phydev)
 {
 	int ret;
@@ -1450,13 +1477,14 @@ static int dp83640_probe(struct phy_device *phydev)
 	} else
 		list_add_tail(&dp83640->list, &clock->phylist);
 
-	dp83640_clock_put(clock);
+	dp83640_clock_unlock(clock);
 	return 0;
 
 no_register:
 	clock->chosen = NULL;
 	kfree(dp83640);
 no_memory:
+	dp83640_clock_unlock(clock);
 	dp83640_clock_put(clock);
 no_clock:
 	return err;
@@ -1467,7 +1495,6 @@ static void dp83640_remove(struct phy_device *phydev)
 	struct dp83640_clock *clock;
 	struct list_head *this, *next;
 	struct dp83640_private *tmp, *dp83640 = phydev->priv;
-	bool remove_clock = false;
 
 	if (phydev->mdio.addr == BROADCAST_ADDR)
 		return;
@@ -1480,7 +1507,8 @@ static void dp83640_remove(struct phy_device *phydev)
 	skb_queue_purge(&dp83640->rx_queue);
 	skb_queue_purge(&dp83640->tx_queue);
 
-	clock = dp83640_clock_get(dp83640->clock);
+	clock = dp83640->clock;
+	dp83640_clock_lock(clock);
 
 	if (dp83640 == clock->chosen) {
 		ptp_clock_unregister(clock->ptp_clock);
@@ -1495,23 +1523,9 @@ static void dp83640_remove(struct phy_device *phydev)
 		}
 	}
 
-	if (!clock->chosen && list_empty(&clock->phylist))
-		remove_clock = true;
-
+	dp83640_clock_unlock(clock);
 	dp83640_clock_put(clock);
 	kfree(dp83640);
-
-	if (remove_clock) {
-		mutex_lock(&phyter_clocks_lock);
-		list_del(&clock->list);
-		mutex_unlock(&phyter_clocks_lock);
-
-		mutex_destroy(&clock->extreg_lock);
-		mutex_destroy(&clock->clock_lock);
-		put_device(&clock->bus->dev);
-		kfree(clock->caps.pin_config);
-		kfree(clock);
-	}
 }
 
 static struct phy_driver dp83640_driver[] = {
-- 
2.43.0

Re: [PATCH net v1] net: phy: dp83640: fix per-bus clock lifetime

[Jakub Kicinski]

On Fri, 24 Jul 2026 10:35:32 +0800 xuanqiang.luo@linux.dev wrote:
> Commit 42e2a9e11a1d ("net: phy: dp83640: improve phydev and driver
> removal handling") moved per-bus clock cleanup from module exit to the
> remove path. This leaves two lifetime problems.

Isn't this sort of shared PHY resource problem supposed to be solved
using the PHY package API?

Re: [PATCH net v1] net: phy: dp83640: fix per-bus clock lifetime

[Andrew Lunn]

On Tue, Jul 28, 2026 at 07:03:40PM -0700, Jakub Kicinski wrote:
> On Fri, 24 Jul 2026 10:35:32 +0800 xuanqiang.luo@linux.dev wrote:
> > Commit 42e2a9e11a1d ("net: phy: dp83640: improve phydev and driver
> > removal handling") moved per-bus clock cleanup from module exit to the
> > remove path. This leaves two lifetime problems.
> 
> Isn't this sort of shared PHY resource problem supposed to be solved
> using the PHY package API?

I don't see anything in the description about it being shared by
multiple PHYs in one package. PHY Packages are about quad PHYs, where
the 4 PHYs share some additional registers, either in one of the PHYs
address space, or in a totally different address space.

Linux clocks are nice in that they are reference counted for enable
and start operations. So they natural share when there are multiple
consumers.

This patch is about something else. It was on my TODO list to take a
better look because it seemed overly complex to me.

       Andrew

Re: [PATCH net v1] net: phy: dp83640: fix per-bus clock lifetime

[Andrew Lunn]

On Tue, Jul 28, 2026 at 07:03:40PM -0700, Jakub Kicinski wrote:
> On Fri, 24 Jul 2026 10:35:32 +0800 xuanqiang.luo@linux.dev wrote:
> > Commit 42e2a9e11a1d ("net: phy: dp83640: improve phydev and driver
> > removal handling") moved per-bus clock cleanup from module exit to the
> > remove path. This leaves two lifetime problems.
> 
> Isn't this sort of shared PHY resource problem supposed to be solved
> using the PHY package API?

Ah! Sorry, my reply was completely wrong, now that i have looked at
the patch.

We have clocks and we have clocks. To me, a clock is something which
uses the common clock framework, because it is a clock. clk_get,
clk_enable, clk_disable.

And then we have these PTP things, which some people call clocks, but
they are something else. And in this patch, we are talking about this
something else.

And you are correct, it does appear to be one of these PTP things
which is shared by a number of PHYs. So yes, the PHY package API
should be used here.

Xuanqiang please take a look at drivers/net/phy/phy_package.c, and the
PHYs which use it. It should help solve your lifetime problems.

    Andrew

---
pw-bot: cr

      

Re: [PATCH net v1] net: phy: dp83640: fix per-bus clock lifetime

[luoxuanqiang]

在 2026/7/29 21:39, Andrew Lunn 写道:
> On Tue, Jul 28, 2026 at 07:03:40PM -0700, Jakub Kicinski wrote:
>> On Fri, 24 Jul 2026 10:35:32 +0800 xuanqiang.luo@linux.dev wrote:
>>> Commit 42e2a9e11a1d ("net: phy: dp83640: improve phydev and driver
>>> removal handling") moved per-bus clock cleanup from module exit to the
>>> remove path. This leaves two lifetime problems.
>> Isn't this sort of shared PHY resource problem supposed to be solved
>> using the PHY package API?
> Ah! Sorry, my reply was completely wrong, now that i have looked at
> the patch.
>
> We have clocks and we have clocks. To me, a clock is something which
> uses the common clock framework, because it is a clock. clk_get,
> clk_enable, clk_disable.
>
> And then we have these PTP things, which some people call clocks, but
> they are something else. And in this patch, we are talking about this
> something else.
>
> And you are correct, it does appear to be one of these PTP things
> which is shared by a number of PHYs. So yes, the PHY package API
> should be used here.
>
> Xuanqiang please take a look at drivers/net/phy/phy_package.c, and the
> PHYs which use it. It should help solve your lifetime problems.
>
>      Andrew
>
> ---
> pw-bot: cr
>
Thanks Jakub and Andrew for pointing this out.

That does look like a much cleaner way to handle the lifetime of the
shared PTP clock.

I'll rework the patch and send a v2.