[PATCH net-next] ptp: remove unnecessary mutex lock in ptp_clock_unregister()

[PATCH net-next] ptp: remove unnecessary mutex lock in ptp_clock_unregister()

[Jeongjun Park]

ptp_clock_unregister() is called by ptp core and several drivers that
require ptp clock feature. And in this function, ptp_vclock_in_use()
is called to check if ptp virtual clock is in use, and
ptp->is_virtual_clock, ptp->n_vclocks are checked.

It is true that you should always check ptp->is_virtual_clock to see if
you are using ptp virtual clock, but you do not necessarily need to
check ptp->n_vclocks.

ptp->n_vclocks is a feature need by ptp sysfs or some ptp cores, so in
most cases, except for these callers, it is not necessary to check.

The problem is that ptp_clock_unregister() checks ptp->n_vclocks even
when called by a driver other than the ptp core, and acquires
ptp->n_vclocks_mux to avoid concurrency issues when checking.

I think this logic is inefficient, so I think it would be appropriate to
modify the caller function that must check ptp->n_vclocks to check
ptp->n_vclocks in advance before calling ptp_clock_unregister().

Signed-off-by: Jeongjun Park <aha310510@gmail.com>
---
 drivers/ptp/ptp_clock.c   |  2 +-
 drivers/ptp/ptp_private.h | 34 +++++++++-------------------------
 2 files changed, 10 insertions(+), 26 deletions(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 36f57d7b4a66..db6e03072fba 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -413,7 +413,7 @@ static int unregister_vclock(struct device *dev, void *data)
 
 int ptp_clock_unregister(struct ptp_clock *ptp)
 {
-	if (ptp_vclock_in_use(ptp)) {
+	if (!ptp->is_virtual_clock) {
 		device_for_each_child(&ptp->dev, NULL, unregister_vclock);
 	}
 
diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
index a6aad743c282..9b308461fcc8 100644
--- a/drivers/ptp/ptp_private.h
+++ b/drivers/ptp/ptp_private.h
@@ -95,39 +95,23 @@ static inline int queue_cnt(const struct timestamp_event_queue *q)
 	return cnt < 0 ? PTP_MAX_TIMESTAMPS + cnt : cnt;
 }
 
-/* Check if ptp virtual clock is in use */
-static inline bool ptp_vclock_in_use(struct ptp_clock *ptp)
+/* Check if ptp clock shall be free running */
+static inline bool ptp_clock_freerun(struct ptp_clock *ptp)
 {
-	bool in_use = false;
-
-	/* Virtual clocks can't be stacked on top of virtual clocks.
-	 * Avoid acquiring the n_vclocks_mux on virtual clocks, to allow this
-	 * function to be called from code paths where the n_vclocks_mux of the
-	 * parent physical clock is already held. Functionally that's not an
-	 * issue, but lockdep would complain, because they have the same lock
-	 * class.
-	 */
-	if (ptp->is_virtual_clock)
-		return false;
+	bool ret = false;
+
+	if (ptp->has_cycles)
+		return ret;
 
 	if (mutex_lock_interruptible(&ptp->n_vclocks_mux))
 		return true;
 
-	if (ptp->n_vclocks)
-		in_use = true;
+	if (!ptp->is_virtual_clock && ptp->n_vclocks)
+		ret = true;
 
 	mutex_unlock(&ptp->n_vclocks_mux);
 
-	return in_use;
-}
-
-/* Check if ptp clock shall be free running */
-static inline bool ptp_clock_freerun(struct ptp_clock *ptp)
-{
-	if (ptp->has_cycles)
-		return false;
-
-	return ptp_vclock_in_use(ptp);
+	return ret;
 }
 
 extern const struct class ptp_class;
--

Re: [PATCH net-next] ptp: remove unnecessary mutex lock in ptp_clock_unregister()

[Jakub Kicinski]

On Wed,  2 Jul 2025 02:03:53 +0900 Jeongjun Park wrote:
> ptp_clock_unregister() is called by ptp core and several drivers that
> require ptp clock feature. And in this function, ptp_vclock_in_use()
> is called to check if ptp virtual clock is in use, and
> ptp->is_virtual_clock, ptp->n_vclocks are checked.
> 
> It is true that you should always check ptp->is_virtual_clock to see if
> you are using ptp virtual clock, but you do not necessarily need to
> check ptp->n_vclocks.
> 
> ptp->n_vclocks is a feature need by ptp sysfs or some ptp cores, so in
> most cases, except for these callers, it is not necessary to check.
> 
> The problem is that ptp_clock_unregister() checks ptp->n_vclocks even
> when called by a driver other than the ptp core, and acquires
> ptp->n_vclocks_mux to avoid concurrency issues when checking.
> 
> I think this logic is inefficient, so I think it would be appropriate to
> modify the caller function that must check ptp->n_vclocks to check
> ptp->n_vclocks in advance before calling ptp_clock_unregister().

Please repost this and CC Vladimir.
-- 
pw-bot: cr