Re: [PATCH net 1/2] ptp: fix breakage after ptp_vclock_in_use() rework

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Mon, Jun 16, 2025 at 12:34:59AM +0900, Jeongjun Park wrote:
> However, I don't think it is appropriate to fix ptp_vclock_in_use().
> I agree that ptp->n_vclocks should be checked in the path where
> ptp_clock_freerun() is called, but there are many drivers that do not
> have any contact with ptp->n_vclocks in the path where
> ptp_clock_unregister() is called.

What do you mean there are many drivers that do not have any contact
with ptp->n_vclocks? It is a feature visible only to the core, and
transparent to the drivers. All drivers have contact with it, or none
do. It all depends solely upon user configuration, and not dependent at
all upon the specific driver.

> The reason I removed the ptp->n_vclocks check logic from the
> ptp_vclock_in_use() function is to prevent false positives from lockdep,
> but also to prevent the performance overhead caused by locking
> ptp->n_vclocks_mux and checking ptp->n_vclocks when calling
> ptp_vclock_in_use() from a driver that has nothing to do with
> ptp->n_vclocks.

Can you quantify the performance overhead caused by acquiring
ptp->n_vclocks_mux on unregistering physical clocks?

> 
> Therefore, I think it would be appropriate to modify ptp_clock_freerun()
> like this instead of ptp_vclock_in_use():
> ---
>  drivers/ptp/ptp_private.h | 14 ++++++++++++--
>  1 file changed, 12 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h
> index 528d86a33f37..abd99087f0ca 100644
> --- a/drivers/ptp/ptp_private.h
> +++ b/drivers/ptp/ptp_private.h
> @@ -104,10 +104,20 @@ 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 ret = false;
> +
>     if (ptp->has_cycles)
> -       return false;
> +       return ret;
> +
> +   if (mutex_lock_interruptible(&ptp->n_vclocks_mux))
> +       return true;
> +
> +   if (ptp_vclock_in_use(ptp) && ptp->n_vclocks)
> +       ret = true;
> +
> +   mutex_unlock(&ptp->n_vclocks_mux);
> 
> -   return ptp_vclock_in_use(ptp);
> +   return ret;
>  }
> 
>  extern const struct class ptp_class;
> -- 

If we leave the ptp_vclock_in_use() implementation as
"return !ptp->is_virtual_clock;", then a physical PTP clock with
n_vclocks=0 will have ptp_vclock_in_use() return true.
Do you consider that expected behavior? What does "vclocks in use"
even mean?

In any case, I do agree with the fact that we shouldn't need to acquire
a mutex in ptp_clock_unregister() to avoid racing with the sysfs device
attributes. This seems avoidable with better operation ordering
(unregister child virtual clocks when sysfs calls are no longer
possible), or the use of pre-existing "ptp->defunct", or some other
mechanism.

You can certainly expand on that idea in net-next. The lockdep splat is
a completely unrelated issue, and only that part is 'net' material.