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

[PATCH net v2] 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 the
driver allocates its per-PHY data and registers the PTP clock. If either
operation fails, no PHY is bound and the remove callback cannot release
the clock, leaking the clock, its pin configuration, and the MII bus
device reference.

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

Use the PHY package infrastructure for the per-bus clock. The package
table is scoped to each MII bus and holds the shared object until the
last joined PHY leaves. Serialize the one-time clock initialization with
the bus shared lock because phy_package_probe_once() elects an
initializer but does not wait for initialization to finish.

Manage both the package reference and the per-PHY state with devres.
This is needed because dp83640_probe() may succeed before later PHY core
initialization fails, and the driver remove callback is not called for
that failure. Register the per-PHY cleanup action after the package
reference so probe unwinding first unregisters the PTP clock or removes
the PHY from the clock list, then releases the shared clock. Release the
same action from the normal remove path.

Embed the pin configuration in the package private data so the final
package leave releases all clock storage.

Fixes: 42e2a9e11a1d ("net: phy: dp83640: improve phydev and driver removal handling")
Suggested-by: Jakub Kicinski <kuba@kernel.org>
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
Changes:
v2:
  - Replace the driver-local clock list and kref with the PHY package API.
    (Suggested by Jakub.)
  - Use devres to avoid leaking the new package reference on later PHY core
    initialization failures.
  - Embed the pin configuration in the package private data to simplify
    lifetime management.

v1: https://lore.kernel.org/all/20260724023532.14583-1-xuanqiang.luo@linux.dev/

 drivers/net/phy/dp83640.c | 184 ++++++++++++++------------------------
 drivers/ptp/Kconfig       |   1 +
 2 files changed, 70 insertions(+), 115 deletions(-)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 98472abdd3920..53786c38edb80 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -8,6 +8,7 @@
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
 #include <linux/crc32.h>
+#include <linux/device/devres.h>
 #include <linux/ethtool.h>
 #include <linux/kernel.h>
 #include <linux/list.h>
@@ -21,6 +22,7 @@
 #include <linux/ptp_clock_kernel.h>
 
 #include "dp83640_reg.h"
+#include "phylib.h"
 
 #define DP83640_PHY_ID	0x20005ce1
 #define PAGESEL		0x13
@@ -128,10 +130,6 @@ struct dp83640_private {
 };
 
 struct dp83640_clock {
-	/* keeps the instance in the 'phyter_clocks' list */
-	struct list_head list;
-	/* we create one clock instance per MII bus */
-	struct mii_bus *bus;
 	/* protects extended registers from concurrent access */
 	struct mutex extreg_lock;
 	/* remembers which page was last selected */
@@ -146,6 +144,7 @@ struct dp83640_clock {
 	struct list_head phylist;
 	/* reference to our PTP hardware clock */
 	struct ptp_clock *ptp_clock;
+	struct ptp_pin_desc pin_config[DP83640_N_PINS];
 };
 
 /* globals */
@@ -206,10 +205,6 @@ static void dp83640_gpio_defaults(struct ptp_pin_desc *pd)
 	}
 }
 
-/* a list of clocks and a mutex to protect it */
-static LIST_HEAD(phyter_clocks);
-static DEFINE_MUTEX(phyter_clocks_lock);
-
 static void rx_timestamp_work(struct work_struct *work);
 
 /* extended register access functions */
@@ -953,13 +948,12 @@ static void decode_status_frame(struct dp83640_private *dp83640,
 	}
 }
 
-static void dp83640_clock_init(struct dp83640_clock *clock, struct mii_bus *bus)
+static void dp83640_clock_init(struct dp83640_clock *clock)
 {
-	INIT_LIST_HEAD(&clock->list);
-	clock->bus = bus;
 	mutex_init(&clock->extreg_lock);
 	mutex_init(&clock->clock_lock);
 	INIT_LIST_HEAD(&clock->phylist);
+	clock->caps.pin_config = clock->pin_config;
 	clock->caps.owner = THIS_MODULE;
 	sprintf(clock->caps.name, "dp83640 timer");
 	clock->caps.max_adj	= 1953124;
@@ -977,14 +971,8 @@ static void dp83640_clock_init(struct dp83640_clock *clock, struct mii_bus *bus)
 	clock->caps.settime64	= ptp_dp83640_settime;
 	clock->caps.enable	= ptp_dp83640_enable;
 	clock->caps.verify	= ptp_dp83640_verify;
-	/*
-	 * Convert the module param defaults into a dynamic pin configuration.
-	 */
+	/* Convert the module param defaults into the pin configuration. */
 	dp83640_gpio_defaults(clock->caps.pin_config);
-	/*
-	 * Get a reference to this bus instance.
-	 */
-	get_device(&bus->dev);
 }
 
 static int choose_this_phy(struct dp83640_clock *clock,
@@ -999,58 +987,6 @@ static int choose_this_phy(struct dp83640_clock *clock,
 	return 0;
 }
 
-static struct dp83640_clock *dp83640_clock_get(struct dp83640_clock *clock)
-{
-	if (clock)
-		mutex_lock(&clock->clock_lock);
-	return clock;
-}
-
-/*
- * Look up and lock a clock by bus instance.
- * If there is no clock for this bus, then create it first.
- */
-static struct dp83640_clock *dp83640_clock_get_bus(struct mii_bus *bus)
-{
-	struct dp83640_clock *clock = NULL, *tmp;
-	struct list_head *this;
-
-	mutex_lock(&phyter_clocks_lock);
-
-	list_for_each(this, &phyter_clocks) {
-		tmp = list_entry(this, struct dp83640_clock, list);
-		if (tmp->bus == bus) {
-			clock = tmp;
-			break;
-		}
-	}
-	if (clock)
-		goto out;
-
-	clock = kzalloc_obj(struct dp83640_clock);
-	if (!clock)
-		goto out;
-
-	clock->caps.pin_config = kzalloc_objs(struct ptp_pin_desc,
-					      DP83640_N_PINS);
-	if (!clock->caps.pin_config) {
-		kfree(clock);
-		clock = NULL;
-		goto out;
-	}
-	dp83640_clock_init(clock, bus);
-	list_add_tail(&clock->list, &phyter_clocks);
-out:
-	mutex_unlock(&phyter_clocks_lock);
-
-	return dp83640_clock_get(clock);
-}
-
-static void dp83640_clock_put(struct dp83640_clock *clock)
-{
-	mutex_unlock(&clock->clock_lock);
-}
-
 static int dp83640_soft_reset(struct phy_device *phydev)
 {
 	int ret;
@@ -1398,19 +1334,65 @@ static int dp83640_ts_info(struct mii_timestamper *mii_ts,
 	return 0;
 }
 
-static int dp83640_probe(struct phy_device *phydev)
+static void dp83640_phy_release(void *data)
 {
+	struct dp83640_private *dp83640 = data;
+	struct dp83640_private *tmp;
+	struct list_head *this, *next;
 	struct dp83640_clock *clock;
+	struct phy_device *phydev;
+
+	clock = dp83640->clock;
+	phydev = dp83640->phydev;
+	phydev->mii_ts = NULL;
+	cancel_delayed_work_sync(&dp83640->ts_work);
+	skb_queue_purge(&dp83640->rx_queue);
+	skb_queue_purge(&dp83640->tx_queue);
+
+	mutex_lock(&clock->clock_lock);
+	if (dp83640 == clock->chosen) {
+		ptp_clock_unregister(clock->ptp_clock);
+		clock->ptp_clock = NULL;
+		clock->chosen = NULL;
+	} else {
+		list_for_each_safe(this, next, &clock->phylist) {
+			tmp = list_entry(this, struct dp83640_private, list);
+			if (tmp == dp83640) {
+				list_del_init(&tmp->list);
+				break;
+			}
+		}
+	}
+	mutex_unlock(&clock->clock_lock);
+
+	phydev->default_timestamp = false;
+	phydev->priv = NULL;
+	kfree(dp83640);
+}
+
+static int dp83640_probe(struct phy_device *phydev)
+{
 	struct dp83640_private *dp83640;
+	struct dp83640_clock *clock;
 	int err = -ENOMEM, i;
 
 	if (phydev->mdio.addr == BROADCAST_ADDR)
 		return 0;
 
-	clock = dp83640_clock_get_bus(phydev->mdio.bus);
-	if (!clock)
+	err = devm_phy_package_join(&phydev->mdio.dev, phydev,
+				    BROADCAST_ADDR, sizeof(*clock));
+	if (err)
 		goto no_clock;
 
+	clock = phy_package_get_priv(phydev);
+	/* Ensure other PHY probes wait for shared clock initialization. */
+	mutex_lock(&phydev->mdio.bus->shared_lock);
+	if (phy_package_probe_once(phydev))
+		dp83640_clock_init(clock);
+	mutex_unlock(&phydev->mdio.bus->shared_lock);
+
+	mutex_lock(&clock->clock_lock);
+
 	dp83640 = kzalloc_obj(struct dp83640_private);
 	if (!dp83640)
 		goto no_memory;
@@ -1450,24 +1432,31 @@ static int dp83640_probe(struct phy_device *phydev)
 	} else
 		list_add_tail(&dp83640->list, &clock->phylist);
 
-	dp83640_clock_put(clock);
+	mutex_unlock(&clock->clock_lock);
+
+	err = devm_add_action_or_reset(&phydev->mdio.dev,
+				       dp83640_phy_release, dp83640);
+	if (err)
+		return err;
+
 	return 0;
 
 no_register:
 	clock->chosen = NULL;
+	clock->ptp_clock = NULL;
+	phydev->default_timestamp = false;
+	phydev->mii_ts = NULL;
+	phydev->priv = NULL;
 	kfree(dp83640);
 no_memory:
-	dp83640_clock_put(clock);
+	mutex_unlock(&clock->clock_lock);
 no_clock:
 	return err;
 }
 
 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;
+	struct dp83640_private *dp83640 = phydev->priv;
 
 	if (phydev->mdio.addr == BROADCAST_ADDR)
 		return;
@@ -1475,43 +1464,8 @@ static void dp83640_remove(struct phy_device *phydev)
 	phydev->mii_ts = NULL;
 
 	enable_status_frames(phydev, false);
-	cancel_delayed_work_sync(&dp83640->ts_work);
-
-	skb_queue_purge(&dp83640->rx_queue);
-	skb_queue_purge(&dp83640->tx_queue);
-
-	clock = dp83640_clock_get(dp83640->clock);
 
-	if (dp83640 == clock->chosen) {
-		ptp_clock_unregister(clock->ptp_clock);
-		clock->chosen = NULL;
-	} else {
-		list_for_each_safe(this, next, &clock->phylist) {
-			tmp = list_entry(this, struct dp83640_private, list);
-			if (tmp == dp83640) {
-				list_del_init(&tmp->list);
-				break;
-			}
-		}
-	}
-
-	if (!clock->chosen && list_empty(&clock->phylist))
-		remove_clock = true;
-
-	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);
-	}
+	devm_release_action(&phydev->mdio.dev, dp83640_phy_release, dp83640);
 }
 
 static struct phy_driver dp83640_driver[] = {
diff --git a/drivers/ptp/Kconfig b/drivers/ptp/Kconfig
index b93640ca08b72..feb50f8cc406a 100644
--- a/drivers/ptp/Kconfig
+++ b/drivers/ptp/Kconfig
@@ -78,6 +78,7 @@ config DP83640_PHY
 	depends on PHYLIB
 	depends on PTP_1588_CLOCK
 	select CRC32
+	select PHY_PACKAGE
 	help
 	  Supports the DP83640 PHYTER with IEEE 1588 features.
 
-- 
2.43.0

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

[Jakub Kicinski]

On Thu, 30 Jul 2026 14:44:51 +0800 xuanqiang.luo@linux.dev wrote:
> 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 the
> driver allocates its per-PHY data and registers the PTP clock. If either
> operation fails, no PHY is bound and the remove callback cannot release
> the clock, leaking the clock, its pin configuration, and the MII bus
> device reference.
> 
> The remove path can also free a clock after dropping clock_lock. A
> concurrent probe may already have found the clock under
> phyter_clocks_lock and be waiting for clock_lock, allowing it to acquire
> a freed mutex and access the freed clock.
> 
> Use the PHY package infrastructure for the per-bus clock. The package
> table is scoped to each MII bus and holds the shared object until the
> last joined PHY leaves. Serialize the one-time clock initialization with
> the bus shared lock because phy_package_probe_once() elects an
> initializer but does not wait for initialization to finish.
> 
> Manage both the package reference and the per-PHY state with devres.
> This is needed because dp83640_probe() may succeed before later PHY core
> initialization fails, and the driver remove callback is not called for
> that failure. Register the per-PHY cleanup action after the package
> reference so probe unwinding first unregisters the PTP clock or removes
> the PHY from the clock list, then releases the shared clock. Release the
> same action from the normal remove path.
> 
> Embed the pin configuration in the package private data so the final
> package leave releases all clock storage.
> 
> Fixes: 42e2a9e11a1d ("net: phy: dp83640: improve phydev and driver removal handling")
> Suggested-by: Jakub Kicinski <kuba@kernel.org>

No need to add Suggested-by tags for review comments.
Only if the v1 was suggested by the person.

> +static int dp83640_probe(struct phy_device *phydev)
> +{
>  	struct dp83640_private *dp83640;
> +	struct dp83640_clock *clock;
>  	int err = -ENOMEM, i;
>  
>  	if (phydev->mdio.addr == BROADCAST_ADDR)
>  		return 0;
>  
> -	clock = dp83640_clock_get_bus(phydev->mdio.bus);
> -	if (!clock)
> +	err = devm_phy_package_join(&phydev->mdio.dev, phydev,
> +				    BROADCAST_ADDR, sizeof(*clock));
> +	if (err)
>  		goto no_clock;
>  
> +	clock = phy_package_get_priv(phydev);
> +	/* Ensure other PHY probes wait for shared clock initialization. */
> +	mutex_lock(&phydev->mdio.bus->shared_lock);
> +	if (phy_package_probe_once(phydev))
> +		dp83640_clock_init(clock);
> +	mutex_unlock(&phydev->mdio.bus->shared_lock);
> +
> +	mutex_lock(&clock->clock_lock);
> +
>  	dp83640 = kzalloc_obj(struct dp83640_private);
>  	if (!dp83640)

AI reviewer points out that this error path is now missing setting
the err variable. Please clean this up as part of your change.
Remove the init to -ENOMEM and have every goto x; path set err
if it needs to

>  		goto no_memory;
> @@ -1450,24 +1432,31 @@ static int dp83640_probe(struct phy_device *phydev)
>  	} else
>  		list_add_tail(&dp83640->list, &clock->phylist);
>  
> -	dp83640_clock_put(clock);
> +	mutex_unlock(&clock->clock_lock);
> +
> +	err = devm_add_action_or_reset(&phydev->mdio.dev,
> +				       dp83640_phy_release, dp83640);
> +	if (err)
> +		return err;
> +
>  	return 0;
>  
>  no_register:
>  	clock->chosen = NULL;
> +	clock->ptp_clock = NULL;
> +	phydev->default_timestamp = false;
> +	phydev->mii_ts = NULL;
> +	phydev->priv = NULL;
>  	kfree(dp83640);
>  no_memory:
> -	dp83640_clock_put(clock);
> +	mutex_unlock(&clock->clock_lock);
>  no_clock:
>  	return err;
>  }
>  
>  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;
> +	struct dp83640_private *dp83640 = phydev->priv;
>  
>  	if (phydev->mdio.addr == BROADCAST_ADDR)
>  		return;
> @@ -1475,43 +1464,8 @@ static void dp83640_remove(struct phy_device *phydev)
>  	phydev->mii_ts = NULL;
>  
>  	enable_status_frames(phydev, false);
> -	cancel_delayed_work_sync(&dp83640->ts_work);
> -
> -	skb_queue_purge(&dp83640->rx_queue);
> -	skb_queue_purge(&dp83640->tx_queue);
> -
> -	clock = dp83640_clock_get(dp83640->clock);
>  
> -	if (dp83640 == clock->chosen) {
> -		ptp_clock_unregister(clock->ptp_clock);
> -		clock->chosen = NULL;
> -	} else {
> -		list_for_each_safe(this, next, &clock->phylist) {
> -			tmp = list_entry(this, struct dp83640_private, list);
> -			if (tmp == dp83640) {
> -				list_del_init(&tmp->list);
> -				break;
> -			}
> -		}
> -	}
> -
> -	if (!clock->chosen && list_empty(&clock->phylist))
> -		remove_clock = true;
> -
> -	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);
> -	}
> +	devm_release_action(&phydev->mdio.dev, dp83640_phy_release, dp83640);

Why the explicit call? if you're using devm_ you should let it handle
the unwind.. If you want to have an explicit call, don't use dev_ APIs.
-- 
pw-bot: cr

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

[luoxuanqiang]

在 2026/8/4 10:00, Jakub Kicinski 写道:

> On Thu, 30 Jul 2026 14:44:51 +0800 xuanqiang.luo@linux.dev wrote:
>>
>> Embed the pin configuration in the package private data so the final
>> package leave releases all clock storage.
>>
>> Fixes: 42e2a9e11a1d ("net: phy: dp83640: improve phydev and driver removal handling")
>> Suggested-by: Jakub Kicinski <kuba@kernel.org>
> No need to add Suggested-by tags for review comments.
> Only if the v1 was suggested by the person.
>
Understood, I will drop it in v3.

>> +static int dp83640_probe(struct phy_device *phydev)
>> +{
>>   	struct dp83640_private *dp83640;
>> +	struct dp83640_clock *clock;
>>   	int err = -ENOMEM, i;
>>   
>>   	if (phydev->mdio.addr == BROADCAST_ADDR)
>>   		return 0;
>>   
>> -	clock = dp83640_clock_get_bus(phydev->mdio.bus);
>> -	if (!clock)
>> +	err = devm_phy_package_join(&phydev->mdio.dev, phydev,
>> +				    BROADCAST_ADDR, sizeof(*clock));
>> +	if (err)
>>   		goto no_clock;
>>   
>> +	clock = phy_package_get_priv(phydev);
>> +	/* Ensure other PHY probes wait for shared clock initialization. */
>> +	mutex_lock(&phydev->mdio.bus->shared_lock);
>> +	if (phy_package_probe_once(phydev))
>> +		dp83640_clock_init(clock);
>> +	mutex_unlock(&phydev->mdio.bus->shared_lock);
>> +
>> +	mutex_lock(&clock->clock_lock);
>> +
>>   	dp83640 = kzalloc_obj(struct dp83640_private);
>>   	if (!dp83640)
> AI reviewer points out that this error path is now missing setting
> the err variable. Please clean this up as part of your change.
> Remove the init to -ENOMEM and have every goto x; path set err
> if it needs to

Thanks, this is a serious oversight. I will fix it in v3.

>>   	
>> -	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);
>> -	}
>> +	devm_release_action(&phydev->mdio.dev, dp83640_phy_release, dp83640);
> Why the explicit call? if you're using devm_ you should let it handle
> the unwind.. If you want to have an explicit call, don't use dev_ APIs.

I used the explicit call because I was concerned about leaving the
entire unwind to devres. After the driver's .remove() callback returns,
phylib asserts reset, and the subsequent devres cleanup calls
ptp_clock_unregister(), which may issue MDIO writes while disabling PTP
events.

However, the same ordering can also occur when the driver .probe()
succeeds but later PHY core initialization fails: phy_probe() asserts
reset before devres cleanup unregisters the PTP clock.

As you point out, explicitly releasing a managed action makes the
cleanup semantics awkward, and I currently have no evidence that the
post-reset MDIO writes cause an actual problem. I will therefore drop
the explicit release in v3 and let devres handle the cleanup.

Thanks,
Xuanqiang