[PATCH net v4 0/4] net: phy: dp83640: fix shared clock lifetime and probe error cleanup

[PATCH net v4 0/4] net: phy: dp83640: fix shared clock lifetime and probe error cleanup

[xuanqiang.luo]

From: Xuanqiang Luo <xuanqiang.luo@linux.dev>

The DP83640 driver shares one PTP clock between all PHYs on the same MII
bus.

Its driver-local clock lookup and removal scheme can leak the shared clock
on probe failure or free it while another probe is acquiring it.

This series moves the shared clock to the PHY package infrastructure.

Patch 1 adds PHY package locking helpers.

Patch 2 embeds the pin configuration in the shared clock.

Patch 3 clears per-PHY state when PTP clock registration fails.

Patch 4 fixes the shared clock lifetime using the PHY package
infrastructure.

---
Changes:
v4:
  Patch 1 (new):
  - Add PHY package locking helpers for shared package data.
    (Andrew Lunn.)

  Patch 2 (new):
  - Split the pin configuration storage change into a separate patch and
    clarify that the pin functions remain configurable at runtime.
    (Andrew Lunn.)
  - Clarify the locking comments for pin_config. (Sashiko.)

  Patch 3 (new):
  - Split the PTP registration failure cleanup into a separate patch.

  Patch 4:
  - Use the PHY package locking helpers instead of accessing the MII bus
    shared lock directly. (Andrew Lunn.)
  - Adapt the lifetime fix to the preparatory changes split into separate
    patches. (Andrew Lunn.)

v3: https://lore.kernel.org/all/20260805072725.169431-1-xuanqiang.luo@linux.dev/
  - Drop the Suggested-by tag. (Jakub Kicinski.)
  - Set err to -ENOMEM on the kzalloc_obj() failure path. (Sashiko.)
  - Let devres handle cleanup on driver detach instead of explicitly
    releasing the action from .remove(). (Jakub Kicinski.)

v2: https://lore.kernel.org/all/20260730064451.32261-1-xuanqiang.luo@linux.dev/
  - Replace the driver-local clock list and kref with the PHY package API.
    (Jakub Kicinski.)
  - 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/

Xuanqiang Luo (4):
  net: phy: add PHY package locking helpers
  net: phy: dp83640: embed pin configuration in clock
  net: phy: dp83640: clear state after PTP registration failure
  net: phy: dp83640: fix per-bus clock lifetime

 drivers/net/phy/dp83640.c     | 191 +++++++++++++---------------------
 drivers/net/phy/phy_package.c |  23 ++++
 drivers/net/phy/phylib.h      |   2 +
 drivers/ptp/Kconfig           |   1 +
 4 files changed, 98 insertions(+), 119 deletions(-)

-- 
2.43.0

[PATCH net v4 1/4] net: phy: add PHY package locking helpers

[xuanqiang.luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

The PHY package API provides private data shared by all PHYs in a
package. Drivers are responsible for synchronizing access to this data,
but the API does not provide a lock for that purpose.

Add phy_package_lock() and phy_package_unlock() for drivers to serialize
access to package-private data, including its initialization.

Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/phy/phy_package.c | 23 +++++++++++++++++++++++
 drivers/net/phy/phylib.h      |  2 ++
 2 files changed, 25 insertions(+)

diff --git a/drivers/net/phy/phy_package.c b/drivers/net/phy/phy_package.c
index 16ae8d1c1f89a..735806c5bea88 100644
--- a/drivers/net/phy/phy_package.c
+++ b/drivers/net/phy/phy_package.c
@@ -52,6 +52,29 @@ void *phy_package_get_priv(struct phy_device *phydev)
 }
 EXPORT_SYMBOL_GPL(phy_package_get_priv);
 
+/**
+ * phy_package_lock - acquire the PHY package lock
+ * @phydev: PHY device that has joined the package
+ *
+ * Use this to serialize access to package-private data. Release the lock
+ * with phy_package_unlock().
+ */
+void phy_package_lock(struct phy_device *phydev)
+{
+	mutex_lock(&phydev->mdio.bus->shared_lock);
+}
+EXPORT_SYMBOL_GPL(phy_package_lock);
+
+/**
+ * phy_package_unlock - release the PHY package lock
+ * @phydev: PHY device that has joined the package
+ */
+void phy_package_unlock(struct phy_device *phydev)
+{
+	mutex_unlock(&phydev->mdio.bus->shared_lock);
+}
+EXPORT_SYMBOL_GPL(phy_package_unlock);
+
 static int phy_package_address(struct phy_device *phydev,
 			       unsigned int addr_offset)
 {
diff --git a/drivers/net/phy/phylib.h b/drivers/net/phy/phylib.h
index 0fba245f97458..c6e26ac6b28f0 100644
--- a/drivers/net/phy/phylib.h
+++ b/drivers/net/phy/phylib.h
@@ -12,6 +12,8 @@ struct mii_bus;
 
 struct device_node *phy_package_get_node(struct phy_device *phydev);
 void *phy_package_get_priv(struct phy_device *phydev);
+void phy_package_lock(struct phy_device *phydev);
+void phy_package_unlock(struct phy_device *phydev);
 int __phy_package_read(struct phy_device *phydev, unsigned int addr_offset,
 		       u32 regnum);
 int __phy_package_write(struct phy_device *phydev, unsigned int addr_offset,
-- 
2.43.0

[PATCH net v4 2/4] net: phy: dp83640: embed pin configuration in clock

[xuanqiang.luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

The DP83640 has a fixed number of PTP pins, and its pin configuration
has the same lifetime as the per-bus clock. Allocating the configuration
separately adds an allocation failure path and requires a separate free.

Embed the pin configuration in struct dp83640_clock and point the PTP
clock information at the embedded array. This changes only the storage;
the pin functions remain configurable at runtime. It also allows all
per-bus clock storage to be managed as one allocation.

Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/phy/dp83640.c | 15 ++++-----------
 1 file changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 98472abdd3920..ba39d30b74705 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -146,6 +146,8 @@ struct dp83640_clock {
 	struct list_head phylist;
 	/* reference to our PTP hardware clock */
 	struct ptp_clock *ptp_clock;
+	/* protected by the PTP core pin configuration lock */
+	struct ptp_pin_desc pin_config[DP83640_N_PINS];
 };
 
 /* globals */
@@ -960,6 +962,7 @@ static void dp83640_clock_init(struct dp83640_clock *clock, struct mii_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,9 +980,7 @@ 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.
-	 */
+	/* Initialize the runtime pin configuration from gpio_tab. */
 	dp83640_gpio_defaults(clock->caps.pin_config);
 	/*
 	 * Get a reference to this bus instance.
@@ -1031,13 +1032,6 @@ static struct dp83640_clock *dp83640_clock_get_bus(struct mii_bus *bus)
 	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:
@@ -1509,7 +1503,6 @@ static void dp83640_remove(struct phy_device *phydev)
 		mutex_destroy(&clock->extreg_lock);
 		mutex_destroy(&clock->clock_lock);
 		put_device(&clock->bus->dev);
-		kfree(clock->caps.pin_config);
 		kfree(clock);
 	}
 }
-- 
2.43.0

[PATCH net v4 3/4] net: phy: dp83640: clear state after PTP registration failure

[xuanqiang.luo]

From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

dp83640_probe() publishes its per-PHY state through phydev before
registering the PTP clock. If registration fails, the private data is
freed while phydev->mii_ts and phydev->priv still point to it, and
default_timestamp remains set.

Clear the published PHY state and reset the PTP clock pointer before
freeing the private data.

Fixes: 4715f65ffa05 ("net: Introduce a new MII time stamping interface.")
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/phy/dp83640.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index ba39d30b74705..7aa5cf0a7bb03 100644
--- a/drivers/net/phy/dp83640.c
+++ b/drivers/net/phy/dp83640.c
@@ -1449,6 +1449,10 @@ static int dp83640_probe(struct phy_device *phydev)
 
 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);
-- 
2.43.0

[PATCH net v4 4/4] 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 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 package 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. Let devres
run the same action on normal driver detach.

Fixes: 42e2a9e11a1d ("net: phy: dp83640: improve phydev and driver removal handling")
Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
---
 drivers/net/phy/dp83640.c | 172 ++++++++++++++------------------------
 drivers/ptp/Kconfig       |   1 +
 2 files changed, 65 insertions(+), 108 deletions(-)

diff --git a/drivers/net/phy/dp83640.c b/drivers/net/phy/dp83640.c
index 7aa5cf0a7bb03..543f63eac9caf 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 */
@@ -208,10 +206,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 */
@@ -955,10 +949,8 @@ 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);
@@ -982,10 +974,6 @@ static void dp83640_clock_init(struct dp83640_clock *clock, struct mii_bus *bus)
 	clock->caps.verify	= ptp_dp83640_verify;
 	/* Initialize the runtime pin configuration from gpio_tab. */
 	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,
@@ -1000,51 +988,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;
-
-	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;
@@ -1392,22 +1335,70 @@ 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;
-	int err = -ENOMEM, i;
+	struct dp83640_clock *clock;
+	int err, 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. */
+	phy_package_lock(phydev);
+	if (phy_package_probe_once(phydev))
+		dp83640_clock_init(clock);
+	phy_package_unlock(phydev);
+
+	mutex_lock(&clock->clock_lock);
+
 	dp83640 = kzalloc_obj(struct dp83640_private);
-	if (!dp83640)
+	if (!dp83640) {
+		err = -ENOMEM;
 		goto no_memory;
+	}
 
 	dp83640->phydev = phydev;
 	dp83640->mii_ts.rxtstamp = dp83640_rxtstamp;
@@ -1444,7 +1435,13 @@ 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:
@@ -1455,60 +1452,19 @@ static int dp83640_probe(struct phy_device *phydev)
 	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;
-
 	if (phydev->mdio.addr == BROADCAST_ADDR)
 		return;
 
 	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);
-	}
 }
 
 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 v4 1/4] net: phy: add PHY package locking helpers

[Andrew Lunn]

On Fri, Aug 07, 2026 at 03:07:26PM +0800, xuanqiang.luo@linux.dev wrote:
> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> 
> The PHY package API provides private data shared by all PHYs in a
> package. Drivers are responsible for synchronizing access to this data,
> but the API does not provide a lock for that purpose.
> 
> Add phy_package_lock() and phy_package_unlock() for drivers to serialize
> access to package-private data, including its initialization.
> 
> Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net v4 2/4] net: phy: dp83640: embed pin configuration in clock

[Andrew Lunn]

On Fri, Aug 07, 2026 at 03:07:27PM +0800, xuanqiang.luo@linux.dev wrote:
> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> 
> The DP83640 has a fixed number of PTP pins, and its pin configuration
> has the same lifetime as the per-bus clock. Allocating the configuration
> separately adds an allocation failure path and requires a separate free.
> 
> Embed the pin configuration in struct dp83640_clock and point the PTP
> clock information at the embedded array. This changes only the storage;
> the pin functions remain configurable at runtime. It also allows all
> per-bus clock storage to be managed as one allocation.
> 
> Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net v4 3/4] net: phy: dp83640: clear state after PTP registration failure

[Andrew Lunn]

On Fri, Aug 07, 2026 at 03:07:28PM +0800, xuanqiang.luo@linux.dev wrote:
> From: Xuanqiang Luo <luoxuanqiang@kylinos.cn>
> 
> dp83640_probe() publishes its per-PHY state through phydev before
> registering the PTP clock. If registration fails, the private data is
> freed while phydev->mii_ts and phydev->priv still point to it, and
> default_timestamp remains set.
> 
> Clear the published PHY state and reset the PTP clock pointer before
> freeing the private data.
> 
> Fixes: 4715f65ffa05 ("net: Introduce a new MII time stamping interface.")
> Signed-off-by: Xuanqiang Luo <luoxuanqiang@kylinos.cn>

Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew

Re: [PATCH net v4 4/4] net: phy: dp83640: fix per-bus clock lifetime

[Andrew Lunn]

> -static int dp83640_probe(struct phy_device *phydev)
> +static void dp83640_phy_release(void *data)
>  {

...

> +	mutex_lock(&clock->clock_lock);
> +	if (dp83640 == clock->chosen) {
> +		ptp_clock_unregister(clock->ptp_clock);
> +		clock->ptp_clock = NULL;
> +		clock->chosen = NULL;
> +	} else {

Probe has:

> +	/* Ensure other PHY probes wait for shared clock initialization. */
> +	phy_package_lock(phydev);
> +	if (phy_package_probe_once(phydev))
> +		dp83640_clock_init(clock);
> +	phy_package_unlock(phydev);

It seems like a phy_package_release_once(phydev) would help keep probe
and release being symmetric. The problem is getting the semantics
correct. phy_package_probe_once() will be true for the first PHY
probed. You want phy_package_release_once() to be true when the last
PHY is removed from the package. It probably needs to look at
phydev->shared->refcnt. However that probably also requires using
phy_package_join() not devm_phy_package_join().

I then _think_ all the list manipulation can go away, and the driver
will look cleaner.

    Andrew

---
pw-bot: cr

Re: [PATCH net v4 4/4] net: phy: dp83640: fix per-bus clock lifetime

[luoxuanqiang]

Hi Andrew,

在 2026/8/7 21:59, Andrew Lunn 写道:
>> -static int dp83640_probe(struct phy_device *phydev)
>> +static void dp83640_phy_release(void *data)
>>   {
> ...
>
>> +	mutex_lock(&clock->clock_lock);
>> +	if (dp83640 == clock->chosen) {
>> +		ptp_clock_unregister(clock->ptp_clock);
>> +		clock->ptp_clock = NULL;
>> +		clock->chosen = NULL;
>> +	} else {
> Probe has:
>
>> +	/* Ensure other PHY probes wait for shared clock initialization. */
>> +	phy_package_lock(phydev);
>> +	if (phy_package_probe_once(phydev))
>> +		dp83640_clock_init(clock);
>> +	phy_package_unlock(phydev);
> It seems like a phy_package_release_once(phydev) would help keep probe
> and release being symmetric. The problem is getting the semantics
> correct. phy_package_probe_once() will be true for the first PHY
> probed. You want phy_package_release_once() to be true when the last
> PHY is removed from the package. It probably needs to look at
> phydev->shared->refcnt. However that probably also requires using
> phy_package_join() not devm_phy_package_join().
>
> I then _think_ all the list manipulation can go away, and the driver
> will look cleaner.
>
>      Andrew
>
> ---
> pw-bot: cr

Thanks. I tried to work through the suggested design, but I am still not
sure I understand how the different lifetime requirements are meant to
fit together.

The reason v4 uses devm_phy_package_join() is that dp83640_probe() can
return successfully and phy_probe() can fail later. The driver's
.remove() callback is not called on that path, so devres is needed to
release both the per-PHY state and the package reference.

If I switch to phy_package_join() and release the package explicitly
during driver removal, that failure path will retain the package
reference. If I keep the managed join, however, the per-PHY devres action
runs before the managed package leave. A release_once check from that
action is therefore not atomic with dropping the package reference. Two
concurrent removals could both observe a non-final reference count before
either managed leave runs.

I do not see how to pair a phy_package_release_once() helper with package
leave without losing the managed probe unwind. Am I missing an existing
mechanism here?

There are also two DP83640-specific details I am unsure how to handle
under this model.

The PHY list is not only used during release. recalibrate() uses it to
enumerate the fully initialized non-chosen PHYs, read their timestamps,
and adjust their clock offsets. The package API does not currently
provide equivalent member enumeration. When you mentioned that all list
manipulation could go away, did you mean only the list handling for the
final member?

The PTP clock is also tied to clock->chosen rather than to the final
package member. For example, ptp_dp83640_gettime() obtains the PHY device
directly from clock->chosen->phydev and uses it for the subsequent MDIO
accesses. The chosen PHY is not necessarily removed last, so keeping the
PTP clock registered after that PHY has been removed would leave this
callback dereferencing stale per-PHY state.

Should release_once govern only the lifetime of the package-private
storage, or do you expect the PTP clock to remain registered until the
last package member leaves as well? The latter would also require
handling chosen-PHY migration, and I am not sure whether that is intended
to be part of this fix.

Thanks,
Xuanqiang

Re: [PATCH net v4 4/4] net: phy: dp83640: fix per-bus clock lifetime

[Andrew Lunn]

> Thanks. I tried to work through the suggested design, but I am still not
> sure I understand how the different lifetime requirements are meant to
> fit together.
> 
> The reason v4 uses devm_phy_package_join() is that dp83640_probe() can
> return successfully and phy_probe() can fail later. The driver's
> .remove() callback is not called on that path, so devres is needed to
> release both the per-PHY state and the package reference.

You need to be careful with devm_ it can be a footgun. devm_ works
best when all resources are devm_. It gets messy when you need to mix
devm_ resources with resources which are not devm_.

So the rules are, if .probe() fails, the probe needs to cleanup
whatever it did, because as you said, .remove is not called.

> If I switch to phy_package_join() and release the package explicitly
> during driver removal, that failure path will retain the package
> reference.

It should not, if you correctly undo what you did. So if the first PHY
to probe calls phy_package_join(), and creates the shared clock, it
needs to destroy the shared clock and do a phy_package_leave() if
probe fails. In this situation, when the second PHY probes, it will
call phy_package_join(), find that phy_package_init_once() is true,
and create the shared clock. Now, i've not audited the phy_package
code, it might be broken, and you might need to fix it. probe()
failing does not happen to often, so it is not tested well.

> I do not see how to pair a phy_package_release_once() helper with package
> leave without losing the managed probe unwind. Am I missing an existing
> mechanism here?

Maybe you actually need a devm_phy_package_init_once() where you pass
as a parameter the function to call as the release function.

I _think_ that will work, but i don't actually like it. I personally
would throw away of the devm_ calls and do the cleanup manually. It is
much easier to reason about.

> The PHY list is not only used during release. recalibrate() uses it to
> enumerate the fully initialized non-chosen PHYs, read their timestamps,
> and adjust their clock offsets. The package API does not currently
> provide equivalent member enumeration. When you mentioned that all list
> manipulation could go away, did you mean only the list handling for the
> final member?

I did not realise the list was used in other places. So yes, i don't
think you need it for create/destroy of the shared clock.

> The PTP clock is also tied to clock->chosen rather than to the final
> package member. For example, ptp_dp83640_gettime() obtains the PHY device
> directly from clock->chosen->phydev and uses it for the subsequent MDIO
> accesses.

This is wrong. They should be using __phy_package_write() &
__phy_package_read() since you are accessing a package resource, not
an individual PHY resources.

> Should release_once govern only the lifetime of the package-private
> storage, or do you expect the PTP clock to remain registered until the
> last package member leaves as well?

Yes, the clock should be a package resource, so should have the same
lifetime as the package.

	 Andrew

Re: [PATCH net v4 4/4] net: phy: dp83640: fix per-bus clock lifetime

[luoxuanqiang]

Hi Andrew,
在 2026/8/9 23:24, Andrew Lunn 写道:

>> Thanks. I tried to work through the suggested design, but I am still not
>> sure I understand how the different lifetime requirements are meant to
>> fit together.
>>
>> The reason v4 uses devm_phy_package_join() is that dp83640_probe() can
>> return successfully and phy_probe() can fail later. The driver's
>> .remove() callback is not called on that path, so devres is needed to
>> release both the per-PHY state and the package reference.
> You need to be careful with devm_ it can be a footgun. devm_ works
> best when all resources are devm_. It gets messy when you need to mix
> devm_ resources with resources which are not devm_.
>
> So the rules are, if .probe() fails, the probe needs to cleanup
> whatever it did, because as you said, .remove is not called.
>
>> If I switch to phy_package_join() and release the package explicitly
>> during driver removal, that failure path will retain the package
>> reference.
> It should not, if you correctly undo what you did. So if the first PHY
> to probe calls phy_package_join(), and creates the shared clock, it
> needs to destroy the shared clock and do a phy_package_leave() if
> probe fails. In this situation, when the second PHY probes, it will
> call phy_package_join(), find that phy_package_init_once() is true,
> and create the shared clock. Now, i've not audited the phy_package
> code, it might be broken, and you might need to fix it. probe()
> failing does not happen to often, so it is not tested well.
>
>> I do not see how to pair a phy_package_release_once() helper with package
>> leave without losing the managed probe unwind. Am I missing an existing
>> mechanism here?
> Maybe you actually need a devm_phy_package_init_once() where you pass
> as a parameter the function to call as the release function.
>
> I _think_ that will work, but i don't actually like it. I personally
> would throw away of the devm_ calls and do the cleanup manually. It is
> much easier to reason about.

I agree. In the next version, I will remove the devm_ calls and manage
the resources explicitly with phy_package_join() and
phy_package_leave().

If the DP83640 driver's .probe() returns successfully but a later PHY
core initialization step fails, the current PHY core does not call the
driver's .remove() callback. This is a generic PHY core rollback issue,
which I will address separately in a follow-up patch rather than include
in this series.

>> The PHY list is not only used during release. recalibrate() uses it to
>> enumerate the fully initialized non-chosen PHYs, read their timestamps,
>> and adjust their clock offsets. The package API does not currently
>> provide equivalent member enumeration. When you mentioned that all list
>> manipulation could go away, did you mean only the list handling for the
>> final member?
> I did not realise the list was used in other places. So yes, i don't
> think you need it for create/destroy of the shared clock.
>
>> The PTP clock is also tied to clock->chosen rather than to the final
>> package member. For example, ptp_dp83640_gettime() obtains the PHY device
>> directly from clock->chosen->phydev and uses it for the subsequent MDIO
>> accesses.
> This is wrong. They should be using __phy_package_write() &
> __phy_package_read() since you are accessing a package resource, not
> an individual PHY resources.
>
>> Should release_once govern only the lifetime of the package-private
>> storage, or do you expect the PTP clock to remain registered until the
>> last package member leaves as well?
> Yes, the clock should be a package resource, so should have the same
> lifetime as the package.
>
I revisited this part. Extending the lifetime of the registered PTP clock
until the last package member leaves would require broader changes.

The current PTP clock relies on clock->chosen to provide the actual PHY.
For example, ptp_dp83640_enable() and periodic_output() use
clock->chosen->phydev for non-broadcast writes that configure the EXTS
and PEROUT GPIOs.

If the chosen PHY leaves before the last package member while the PTP
clock remains registered, the driver would need to define the behavior of
all callbacks after the chosen PHY has left. clock->chosen is also used
by recalibrate() as the calibration reference, so this is more than a
change to the register accessors; it requires redefining the hardware
selection and synchronization semantics.

Given the additional scope, would it be acceptable to retain the existing
MDIO access paths and keep the PTP clock lifetime tied to the chosen PHY,
rather than keep it registered until the last package member leaves?

That would introduce the PHY package only to manage the lifetime of the
shared struct dp83640_clock. This would keep the change focused on the two
lifetime bugs described in the commit message.

I've sent v5 with these changes.

Thanks,
Xuanqiang