Re: [PATCH net v2 1/2] ptp: drain virtual clock sysfs operations before unregistering children

[netdev-bot+sashiko]

Thank you for your contribution! Sashiko AI review found more severe
issue(s) in other patches of this series. This mail is only an FYI, in case
you want to address the nit picks below when you respin.

Low priority / nit picks for your consideration:

- [Low] The commit message gives the wrong reason for the
  is_virtual_clock guard in ptp_vclock_remove_sysfs()…

--

ptp: drain virtual clock sysfs operations before unregistering children

This commit removes the n_vclocks sysfs attribute at the start of
ptp_clock_unregister(), before the virtual clock children are walked. Doing
so blocks new n_vclocks stores and drains any that are already running, so a
concurrent store can no longer unregister the same child a second time.
Virtual clocks skip the removal through an is_virtual_clock check.

> diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c
> index dc398c6b75284..53388b1231980 100644
> --- a/drivers/ptp/ptp_sysfs.c
> +++ b/drivers/ptp/ptp_sysfs.c
> @@ -263,6 +263,12 @@ static ssize_t n_vclocks_store(struct device *dev,
>  }
>  static DEVICE_ATTR_RW(n_vclocks);
>  
> +void ptp_vclock_remove_sysfs(struct ptp_clock *ptp)
> +{
> +	if (!ptp->is_virtual_clock)
> +		device_remove_file(&ptp->dev, &dev_attr_n_vclocks);
> +}

[Severity: Low]
This isn't a bug, but the commit message gives what looks like the wrong
reason for this guard:

  Virtual clocks have no such attribute and must not remove the parent
  attribute when being deleted by its active store.

When a vclock is deleted from the parent's active store, the path is:

n_vclocks_store()->unregister_vclock()->ptp_vclock_unregister()->
  ptp_clock_unregister(vclock->clock)->ptp_vclock_remove_sysfs()

Here ptp->dev is the virtual clock's own device, not the parent's.
device_remove_file() only acts on that device's directory. It cannot reach
the parent's n_vclocks attribute.

ptp_is_attribute_visible() also never creates the attribute for virtual
clocks:

	} else if (attr == &dev_attr_n_vclocks.attr ||
		   attr == &dev_attr_max_vclocks.attr) {
		if (ptp->is_virtual_clock)
			mode = 0;

Without the guard, device_remove_file() would just find nothing and
kernfs_remove_by_name_ns() would return -ENOENT. There is no self-drain of
the parent's attribute for the guard to prevent.

Could the commit message say instead that the guard only skips a no-op
removal on virtual clocks, which never have n_vclocks? As written, it
reads as though the guard is what stops the parent's active store from
draining itself.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261005-ptp-vclock-sampling-v2-0-8ed12d4d10af%40gmail.com