| Message ID | 20210615094517.48752-3-yangbo.lu@nxp.com (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
| Series | ptp: support virtual clocks and timestamping | expand |
On Tue, Jun 15, 2021 at 05:45:09PM +0800, Yangbo Lu wrote: > diff --git a/Documentation/ABI/testing/sysfs-ptp b/Documentation/ABI/testing/sysfs-ptp > index 2363ad810ddb..2ef11b775f47 100644 > --- a/Documentation/ABI/testing/sysfs-ptp > +++ b/Documentation/ABI/testing/sysfs-ptp > @@ -61,6 +61,19 @@ Description: > This file contains the number of programmable pins > offered by the PTP hardware clock. > > +What: /sys/class/ptp/ptpN/n_vclocks > +Date: May 2021 > +Contact: Yangbo Lu <yangbo.lu@nxp.com> > +Description: > + This file contains the ptp virtual clocks number in use, > + based on current ptp physical clock. In default, the > + value is 0 meaning only ptp physical clock is in use. > + Setting the value can create corresponding number of ptp > + virtual clocks to use. But current ptp physical clock is > + guaranteed to stay free running. Setting the value back > + to 0 can delete ptp virtual clocks and back use ptp > + physical clock again. The native speaker in me suggests: This file contains the number of virtual PTP clocks in use. By default, the value is 0 meaning that only the physical clock is in use. Setting the value creates the corresponding number of virtual clocks and causes the physical clock to become free running. Setting the value back to 0 deletes the virtual clocks and switches the physical clock back to normal, adjustable operation. Thanks, Richard
On Tue, Jun 15, 2021 at 05:45:09PM +0800, Yangbo Lu wrote: > diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h > index 3f388d63904c..6949afc9d733 100644 > --- a/drivers/ptp/ptp_private.h > +++ b/drivers/ptp/ptp_private.h > @@ -46,6 +46,9 @@ struct ptp_clock { > const struct attribute_group *pin_attr_groups[2]; > struct kthread_worker *kworker; > struct kthread_delayed_work aux_work; > + u8 n_vclocks; Hm, type is u8, but ... > + struct mutex n_vclocks_mux; /* protect concurrent n_vclocks access */ > + bool vclock_flag; > }; > > #define info_to_vclock(d) container_of((d), struct ptp_vclock, info) > diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h > index 1d108d597f66..4b933dc1b81b 100644 > --- a/include/uapi/linux/ptp_clock.h > +++ b/include/uapi/linux/ptp_clock.h > @@ -69,6 +69,11 @@ > */ > #define PTP_PEROUT_V1_VALID_FLAGS (0) > > +/* > + * Max number of PTP virtual clocks per PTP physical clock > + */ > +#define PTP_MAX_VCLOCKS 20 Only 20 clocks are allowed? Why? Thanks, Richard
On Tue, Jun 15, 2021 at 05:45:09PM +0800, Yangbo Lu wrote: > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c > index a780435331c8..78414b3e16dd 100644 > --- a/drivers/ptp/ptp_clock.c > +++ b/drivers/ptp/ptp_clock.c > @@ -76,6 +76,11 @@ static int ptp_clock_settime(struct posix_clock *pc, const struct timespec64 *tp > { > struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock); > > + if (ptp_guaranteed_pclock(ptp)) { Can we please invent a more descriptive name for this method? The word "guaranteed" suggests much more. > + pr_err("ptp: virtual clock in use, guarantee physical clock free running\n"); This is good: ^^^^^^^^^^^^^^^^^^^^^^^^^ You can drop this part: ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ So, please rename ptp_guaranteed_pclock() to ptp_vclock_in_use(); > + return -EBUSY; > + } > + > return ptp->info->settime64(ptp->info, tp); > } > diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h > index 3f388d63904c..6949afc9d733 100644 > --- a/drivers/ptp/ptp_private.h > +++ b/drivers/ptp/ptp_private.h > @@ -46,6 +46,9 @@ struct ptp_clock { > const struct attribute_group *pin_attr_groups[2]; > struct kthread_worker *kworker; > struct kthread_delayed_work aux_work; > + u8 n_vclocks; Why not use "unsigned int" type? I don't see a need to set an artificial limit. > + struct mutex n_vclocks_mux; /* protect concurrent n_vclocks access */ > + bool vclock_flag; "flag" is vague. How about "is_virtual_clock" instead? > }; > > #define info_to_vclock(d) container_of((d), struct ptp_vclock, info) > @@ -75,6 +78,18 @@ static inline int queue_cnt(struct timestamp_event_queue *q) > return cnt < 0 ? PTP_MAX_TIMESTAMPS + cnt : cnt; > } > > +/* > + * Guarantee physical clock to stay free running, if ptp virtual clocks > + * on it are in use. > + */ > +static inline bool ptp_guaranteed_pclock(struct ptp_clock *ptp) > +{ > + if (!ptp->vclock_flag && ptp->n_vclocks) Need to take mutex for n_vclocks to prevent load tearing. > + return true; > + > + return false; > +} > + > /* > * see ptp_chardev.c > */ > @@ -148,6 +149,90 @@ static ssize_t pps_enable_store(struct device *dev, > } > static DEVICE_ATTR(pps_enable, 0220, NULL, pps_enable_store); > > +static int unregister_vclock(struct device *dev, void *data) > +{ > + struct ptp_clock *ptp = dev_get_drvdata(dev); > + struct ptp_clock_info *info = ptp->info; > + struct ptp_vclock *vclock; > + u8 *num = data; > + > + vclock = info_to_vclock(info); > + dev_info(dev->parent, "delete virtual clock ptp%d\n", > + vclock->clock->index); > + > + ptp_vclock_unregister(vclock); > + (*num)--; > + > + /* For break. Not error. */ > + if (*num == 0) > + return -EINVAL; > + > + return 0; > +} > + > +static ssize_t n_vclocks_show(struct device *dev, > + struct device_attribute *attr, char *page) > +{ > + struct ptp_clock *ptp = dev_get_drvdata(dev); > + > + return snprintf(page, PAGE_SIZE-1, "%d\n", ptp->n_vclocks); Take mutex. > +} > + > +static ssize_t n_vclocks_store(struct device *dev, > + struct device_attribute *attr, > + const char *buf, size_t count) > +{ > + struct ptp_clock *ptp = dev_get_drvdata(dev); > + struct ptp_vclock *vclock; > + int err = -EINVAL; > + u8 num, i; > + > + if (kstrtou8(buf, 0, &num)) > + goto out; > + > + if (num > PTP_MAX_VCLOCKS) { > + dev_err(dev, "max value is %d\n", PTP_MAX_VCLOCKS); > + goto out; > + } > + > + if (mutex_lock_interruptible(&ptp->n_vclocks_mux)) > + return -ERESTARTSYS; > + > + /* Need to create more vclocks */ > + if (num > ptp->n_vclocks) { > + for (i = 0; i < num - ptp->n_vclocks; i++) { > + vclock = ptp_vclock_register(ptp); > + if (!vclock) { > + mutex_unlock(&ptp->n_vclocks_mux); > + goto out; > + } > + > + dev_info(dev, "new virtual clock ptp%d\n", > + vclock->clock->index); > + } > + } > + > + /* Need to delete vclocks */ > + if (num < ptp->n_vclocks) { > + i = ptp->n_vclocks - num; > + device_for_each_child_reverse(dev, &i, > + unregister_vclock); > + } > + > + if (num == 0) > + dev_info(dev, "only physical clock in use now\n"); > + else > + dev_info(dev, "guarantee physical clock free running\n"); > + > + ptp->n_vclocks = num; > + mutex_unlock(&ptp->n_vclocks_mux); > + > + return count; > +out: > + return err; > +} > +static DEVICE_ATTR_RW(n_vclocks); > + > static struct attribute *ptp_attrs[] = { > &dev_attr_clock_name.attr, > > diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h > index 1d108d597f66..4b933dc1b81b 100644 > --- a/include/uapi/linux/ptp_clock.h > +++ b/include/uapi/linux/ptp_clock.h > @@ -69,6 +69,11 @@ > */ > #define PTP_PEROUT_V1_VALID_FLAGS (0) > > +/* > + * Max number of PTP virtual clocks per PTP physical clock > + */ > +#define PTP_MAX_VCLOCKS 20 Why limit this to twenty clocks? Thanks, Richard
Hi Richard, > -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: 2021年6月18日 1:43 > To: Y.b. Lu <yangbo.lu@nxp.com> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-kselftest@vger.kernel.org; mptcp@lists.linux.dev; David S . Miller > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Mat Martineau > <mathew.j.martineau@linux.intel.com>; Matthieu Baerts > <matthieu.baerts@tessares.net>; Shuah Khan <shuah@kernel.org>; Michal > Kubecek <mkubecek@suse.cz>; Florian Fainelli <f.fainelli@gmail.com>; > Andrew Lunn <andrew@lunn.ch>; Rui Sousa <rui.sousa@nxp.com>; Sebastien > Laveze <sebastien.laveze@nxp.com> > Subject: Re: [net-next, v3, 02/10] ptp: support ptp physical/virtual clocks > conversion > > On Tue, Jun 15, 2021 at 05:45:09PM +0800, Yangbo Lu wrote: > > > diff --git a/Documentation/ABI/testing/sysfs-ptp > b/Documentation/ABI/testing/sysfs-ptp > > index 2363ad810ddb..2ef11b775f47 100644 > > --- a/Documentation/ABI/testing/sysfs-ptp > > +++ b/Documentation/ABI/testing/sysfs-ptp > > @@ -61,6 +61,19 @@ Description: > > This file contains the number of programmable pins > > offered by the PTP hardware clock. > > > > +What: /sys/class/ptp/ptpN/n_vclocks > > +Date: May 2021 > > +Contact: Yangbo Lu <yangbo.lu@nxp.com> > > +Description: > > + This file contains the ptp virtual clocks number in use, > > + based on current ptp physical clock. In default, the > > + value is 0 meaning only ptp physical clock is in use. > > + Setting the value can create corresponding number of ptp > > + virtual clocks to use. But current ptp physical clock is > > + guaranteed to stay free running. Setting the value back > > + to 0 can delete ptp virtual clocks and back use ptp > > + physical clock again. > > The native speaker in me suggests: > > This file contains the number of virtual PTP clocks in > use. By default, the value is 0 meaning that only the > physical clock is in use. Setting the value creates > the corresponding number of virtual clocks and causes > the physical clock to become free running. Setting the > value back to 0 deletes the virtual clocks and > switches the physical clock back to normal, adjustable > operation. Thank you! Will update. That's better than mine. > > Thanks, > Richard
Hi Richard, > -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: 2021年6月18日 2:28 > To: Y.b. Lu <yangbo.lu@nxp.com> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-kselftest@vger.kernel.org; mptcp@lists.linux.dev; David S . Miller > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Mat Martineau > <mathew.j.martineau@linux.intel.com>; Matthieu Baerts > <matthieu.baerts@tessares.net>; Shuah Khan <shuah@kernel.org>; Michal > Kubecek <mkubecek@suse.cz>; Florian Fainelli <f.fainelli@gmail.com>; > Andrew Lunn <andrew@lunn.ch>; Rui Sousa <rui.sousa@nxp.com>; Sebastien > Laveze <sebastien.laveze@nxp.com> > Subject: Re: [net-next, v3, 02/10] ptp: support ptp physical/virtual clocks > conversion > > On Tue, Jun 15, 2021 at 05:45:09PM +0800, Yangbo Lu wrote: > > > diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h > > index 3f388d63904c..6949afc9d733 100644 > > --- a/drivers/ptp/ptp_private.h > > +++ b/drivers/ptp/ptp_private.h > > @@ -46,6 +46,9 @@ struct ptp_clock { > > const struct attribute_group *pin_attr_groups[2]; > > struct kthread_worker *kworker; > > struct kthread_delayed_work aux_work; > > + u8 n_vclocks; > > Hm, type is u8, but ... > > > + struct mutex n_vclocks_mux; /* protect concurrent n_vclocks access */ > > + bool vclock_flag; > > }; > > > > > #define info_to_vclock(d) container_of((d), struct ptp_vclock, info) > > diff --git a/include/uapi/linux/ptp_clock.h > > b/include/uapi/linux/ptp_clock.h index 1d108d597f66..4b933dc1b81b > > 100644 > > --- a/include/uapi/linux/ptp_clock.h > > +++ b/include/uapi/linux/ptp_clock.h > > @@ -69,6 +69,11 @@ > > */ > > #define PTP_PEROUT_V1_VALID_FLAGS (0) > > > > +/* > > + * Max number of PTP virtual clocks per PTP physical clock */ > > +#define PTP_MAX_VCLOCKS 20 > > Only 20 clocks are allowed? Why? Initially I think vclock can be used for ptp multiple domains synchronization. Since the PTP domainValue is u8, u8 vclock number is large enough. This is not a good idea to hard-code a PTP_MAX_VCLOCKS value. But it looks a little crazy to create numbers of vclocks via one command (echo n > /sys/class/ptp/ptp0/n_vclocks). Maybe a typo creates hundreds of vclocks we don’t need. Do you think we should be care about setting a limitation of vclock number? Any suggestion for implementation? Thanks. > > Thanks, > Richard
Hi Richard, > -----Original Message----- > From: Richard Cochran <richardcochran@gmail.com> > Sent: 2021年6月19日 12:06 > To: Y.b. Lu <yangbo.lu@nxp.com> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-kselftest@vger.kernel.org; mptcp@lists.linux.dev; David S . Miller > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Mat Martineau > <mathew.j.martineau@linux.intel.com>; Matthieu Baerts > <matthieu.baerts@tessares.net>; Shuah Khan <shuah@kernel.org>; Michal > Kubecek <mkubecek@suse.cz>; Florian Fainelli <f.fainelli@gmail.com>; > Andrew Lunn <andrew@lunn.ch>; Rui Sousa <rui.sousa@nxp.com>; Sebastien > Laveze <sebastien.laveze@nxp.com> > Subject: Re: [net-next, v3, 02/10] ptp: support ptp physical/virtual clocks > conversion > > On Tue, Jun 15, 2021 at 05:45:09PM +0800, Yangbo Lu wrote: > > > diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index > > a780435331c8..78414b3e16dd 100644 > > --- a/drivers/ptp/ptp_clock.c > > +++ b/drivers/ptp/ptp_clock.c > > @@ -76,6 +76,11 @@ static int ptp_clock_settime(struct posix_clock > > *pc, const struct timespec64 *tp { > > struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock); > > > > + if (ptp_guaranteed_pclock(ptp)) { > > Can we please invent a more descriptive name for this method? > The word "guaranteed" suggests much more. > > > + pr_err("ptp: virtual clock in use, guarantee physical clock free > > +running\n"); > > This is good: ^^^^^^^^^^^^^^^^^^^^^^^^^ > You can drop this part: > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > So, please rename ptp_guaranteed_pclock() to ptp_vclock_in_use(); > Thank you. Will convert to that. > > + return -EBUSY; > > + } > > + > > return ptp->info->settime64(ptp->info, tp); } > > > > diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h > > index 3f388d63904c..6949afc9d733 100644 > > --- a/drivers/ptp/ptp_private.h > > +++ b/drivers/ptp/ptp_private.h > > @@ -46,6 +46,9 @@ struct ptp_clock { > > const struct attribute_group *pin_attr_groups[2]; > > struct kthread_worker *kworker; > > struct kthread_delayed_work aux_work; > > + u8 n_vclocks; > > Why not use "unsigned int" type? I don't see a need to set an artificial limit. Please see my explain in another email thread. Thanks. > > > + struct mutex n_vclocks_mux; /* protect concurrent n_vclocks access */ > > + bool vclock_flag; > > "flag" is vague. How about "is_virtual_clock" instead? That's better. Will use it. Thank you. > > > }; > > > > #define info_to_vclock(d) container_of((d), struct ptp_vclock, info) > > @@ -75,6 +78,18 @@ static inline int queue_cnt(struct > timestamp_event_queue *q) > > return cnt < 0 ? PTP_MAX_TIMESTAMPS + cnt : cnt; } > > > > +/* > > + * Guarantee physical clock to stay free running, if ptp virtual > > +clocks > > + * on it are in use. > > + */ > > +static inline bool ptp_guaranteed_pclock(struct ptp_clock *ptp) { > > + if (!ptp->vclock_flag && ptp->n_vclocks) > > Need to take mutex for n_vclocks to prevent load tearing. > > > + return true; > > + > > + return false; > > +} > > + > > /* > > * see ptp_chardev.c > > */ > > > @@ -148,6 +149,90 @@ static ssize_t pps_enable_store(struct device > > *dev, } static DEVICE_ATTR(pps_enable, 0220, NULL, > > pps_enable_store); > > > > +static int unregister_vclock(struct device *dev, void *data) { > > + struct ptp_clock *ptp = dev_get_drvdata(dev); > > + struct ptp_clock_info *info = ptp->info; > > + struct ptp_vclock *vclock; > > + u8 *num = data; > > + > > + vclock = info_to_vclock(info); > > + dev_info(dev->parent, "delete virtual clock ptp%d\n", > > + vclock->clock->index); > > + > > + ptp_vclock_unregister(vclock); > > + (*num)--; > > + > > + /* For break. Not error. */ > > + if (*num == 0) > > + return -EINVAL; > > + > > + return 0; > > +} > > + > > +static ssize_t n_vclocks_show(struct device *dev, > > + struct device_attribute *attr, char *page) { > > + struct ptp_clock *ptp = dev_get_drvdata(dev); > > + > > + return snprintf(page, PAGE_SIZE-1, "%d\n", ptp->n_vclocks); > > Take mutex. Will take mutex everywhere to access it. > > > +} > > + > > +static ssize_t n_vclocks_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t count) { > > + struct ptp_clock *ptp = dev_get_drvdata(dev); > > + struct ptp_vclock *vclock; > > + int err = -EINVAL; > > + u8 num, i; > > + > > + if (kstrtou8(buf, 0, &num)) > > + goto out; > > + > > + if (num > PTP_MAX_VCLOCKS) { > > + dev_err(dev, "max value is %d\n", PTP_MAX_VCLOCKS); > > + goto out; > > + } > > + > > + if (mutex_lock_interruptible(&ptp->n_vclocks_mux)) > > + return -ERESTARTSYS; > > + > > + /* Need to create more vclocks */ > > + if (num > ptp->n_vclocks) { > > + for (i = 0; i < num - ptp->n_vclocks; i++) { > > + vclock = ptp_vclock_register(ptp); > > + if (!vclock) { > > + mutex_unlock(&ptp->n_vclocks_mux); > > + goto out; > > + } > > + > > + dev_info(dev, "new virtual clock ptp%d\n", > > + vclock->clock->index); > > + } > > + } > > + > > + /* Need to delete vclocks */ > > + if (num < ptp->n_vclocks) { > > + i = ptp->n_vclocks - num; > > + device_for_each_child_reverse(dev, &i, > > + unregister_vclock); > > + } > > + > > + if (num == 0) > > + dev_info(dev, "only physical clock in use now\n"); > > + else > > + dev_info(dev, "guarantee physical clock free running\n"); > > + > > + ptp->n_vclocks = num; > > + mutex_unlock(&ptp->n_vclocks_mux); > > + > > + return count; > > +out: > > + return err; > > +} > > +static DEVICE_ATTR_RW(n_vclocks); > > + > > static struct attribute *ptp_attrs[] = { > > &dev_attr_clock_name.attr, > > > > > > diff --git a/include/uapi/linux/ptp_clock.h > > b/include/uapi/linux/ptp_clock.h index 1d108d597f66..4b933dc1b81b > > 100644 > > --- a/include/uapi/linux/ptp_clock.h > > +++ b/include/uapi/linux/ptp_clock.h > > @@ -69,6 +69,11 @@ > > */ > > #define PTP_PEROUT_V1_VALID_FLAGS (0) > > > > +/* > > + * Max number of PTP virtual clocks per PTP physical clock */ > > +#define PTP_MAX_VCLOCKS 20 > > Why limit this to twenty clocks? Please see my explain in another email thread. Thanks. > > Thanks, > Richard
Hi Richard, > -----Original Message----- > From: Y.b. Lu > Sent: 2021年6月22日 18:35 > To: Richard Cochran <richardcochran@gmail.com> > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > linux-kselftest@vger.kernel.org; mptcp@lists.linux.dev; David S . Miller > <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Mat Martineau > <mathew.j.martineau@linux.intel.com>; Matthieu Baerts > <matthieu.baerts@tessares.net>; Shuah Khan <shuah@kernel.org>; Michal > Kubecek <mkubecek@suse.cz>; Florian Fainelli <f.fainelli@gmail.com>; > Andrew Lunn <andrew@lunn.ch>; Rui Sousa <rui.sousa@nxp.com>; Sebastien > Laveze <sebastien.laveze@nxp.com> > Subject: RE: [net-next, v3, 02/10] ptp: support ptp physical/virtual clocks > conversion > > Hi Richard, > > > -----Original Message----- > > From: Richard Cochran <richardcochran@gmail.com> > > Sent: 2021年6月18日 2:28 > > To: Y.b. Lu <yangbo.lu@nxp.com> > > Cc: netdev@vger.kernel.org; linux-kernel@vger.kernel.org; > > linux-kselftest@vger.kernel.org; mptcp@lists.linux.dev; David S . > > Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>; Mat > > Martineau <mathew.j.martineau@linux.intel.com>; Matthieu Baerts > > <matthieu.baerts@tessares.net>; Shuah Khan <shuah@kernel.org>; Michal > > Kubecek <mkubecek@suse.cz>; Florian Fainelli <f.fainelli@gmail.com>; > > Andrew Lunn <andrew@lunn.ch>; Rui Sousa <rui.sousa@nxp.com>; > Sebastien > > Laveze <sebastien.laveze@nxp.com> > > Subject: Re: [net-next, v3, 02/10] ptp: support ptp physical/virtual > > clocks conversion > > > > On Tue, Jun 15, 2021 at 05:45:09PM +0800, Yangbo Lu wrote: > > > > > diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h > > > index 3f388d63904c..6949afc9d733 100644 > > > --- a/drivers/ptp/ptp_private.h > > > +++ b/drivers/ptp/ptp_private.h > > > @@ -46,6 +46,9 @@ struct ptp_clock { > > > const struct attribute_group *pin_attr_groups[2]; > > > struct kthread_worker *kworker; > > > struct kthread_delayed_work aux_work; > > > + u8 n_vclocks; > > > > Hm, type is u8, but ... > > > > > + struct mutex n_vclocks_mux; /* protect concurrent n_vclocks access */ > > > + bool vclock_flag; > > > }; > > > > > > > > #define info_to_vclock(d) container_of((d), struct ptp_vclock, > > > info) diff --git a/include/uapi/linux/ptp_clock.h > > > b/include/uapi/linux/ptp_clock.h index 1d108d597f66..4b933dc1b81b > > > 100644 > > > --- a/include/uapi/linux/ptp_clock.h > > > +++ b/include/uapi/linux/ptp_clock.h > > > @@ -69,6 +69,11 @@ > > > */ > > > #define PTP_PEROUT_V1_VALID_FLAGS (0) > > > > > > +/* > > > + * Max number of PTP virtual clocks per PTP physical clock */ > > > +#define PTP_MAX_VCLOCKS 20 > > > > Only 20 clocks are allowed? Why? > > Initially I think vclock can be used for ptp multiple domains synchronization. > Since the PTP domainValue is u8, u8 vclock number is large enough. > This is not a good idea to hard-code a PTP_MAX_VCLOCKS value. But it looks a > little crazy to create numbers of vclocks via one command (echo n > > /sys/class/ptp/ptp0/n_vclocks). > Maybe a typo creates hundreds of vclocks we don’t need. > Do you think we should be care about setting a limitation of vclock number? > Any suggestion for implementation? > Thanks. I sent v4. I removed the u8 limitation for vclocks number, using unsigned int instead. I introduced max_vclocks attribute which could be re-configured. Please help to review. Thanks. > > > > > Thanks, > > Richard
diff --git a/Documentation/ABI/testing/sysfs-ptp b/Documentation/ABI/testing/sysfs-ptp index 2363ad810ddb..2ef11b775f47 100644 --- a/Documentation/ABI/testing/sysfs-ptp +++ b/Documentation/ABI/testing/sysfs-ptp @@ -61,6 +61,19 @@ Description: This file contains the number of programmable pins offered by the PTP hardware clock. +What: /sys/class/ptp/ptpN/n_vclocks +Date: May 2021 +Contact: Yangbo Lu <yangbo.lu@nxp.com> +Description: + This file contains the ptp virtual clocks number in use, + based on current ptp physical clock. In default, the + value is 0 meaning only ptp physical clock is in use. + Setting the value can create corresponding number of ptp + virtual clocks to use. But current ptp physical clock is + guaranteed to stay free running. Setting the value back + to 0 can delete ptp virtual clocks and back use ptp + physical clock again. + What: /sys/class/ptp/ptpN/pins Date: March 2014 Contact: Richard Cochran <richardcochran@gmail.com> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c index a780435331c8..78414b3e16dd 100644 --- a/drivers/ptp/ptp_clock.c +++ b/drivers/ptp/ptp_clock.c @@ -76,6 +76,11 @@ static int ptp_clock_settime(struct posix_clock *pc, const struct timespec64 *tp { struct ptp_clock *ptp = container_of(pc, struct ptp_clock, clock); + if (ptp_guaranteed_pclock(ptp)) { + pr_err("ptp: virtual clock in use, guarantee physical clock free running\n"); + return -EBUSY; + } + return ptp->info->settime64(ptp->info, tp); } @@ -97,6 +102,11 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct __kernel_timex *tx) struct ptp_clock_info *ops; int err = -EOPNOTSUPP; + if (ptp_guaranteed_pclock(ptp)) { + pr_err("ptp: virtual clock in use, guarantee physical clock free running\n"); + return -EBUSY; + } + ops = ptp->info; if (tx->modes & ADJ_SETOFFSET) { @@ -161,6 +171,7 @@ static void ptp_clock_release(struct device *dev) ptp_cleanup_pin_groups(ptp); mutex_destroy(&ptp->tsevq_mux); mutex_destroy(&ptp->pincfg_mux); + mutex_destroy(&ptp->n_vclocks_mux); ida_simple_remove(&ptp_clocks_map, ptp->index); kfree(ptp); } @@ -208,6 +219,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, spin_lock_init(&ptp->tsevq.lock); mutex_init(&ptp->tsevq_mux); mutex_init(&ptp->pincfg_mux); + mutex_init(&ptp->n_vclocks_mux); init_waitqueue_head(&ptp->tsev_wq); if (ptp->info->do_aux_work) { @@ -220,6 +232,10 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, } } + /* PTP virtual clock is being registered under physical clock */ + if (parent->class && strcmp(parent->class->name, "ptp") == 0) + ptp->vclock_flag = true; + err = ptp_populate_pin_groups(ptp); if (err) goto no_pin_groups; @@ -269,6 +285,7 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info, kworker_err: mutex_destroy(&ptp->tsevq_mux); mutex_destroy(&ptp->pincfg_mux); + mutex_destroy(&ptp->n_vclocks_mux); ida_simple_remove(&ptp_clocks_map, index); no_slot: kfree(ptp); @@ -279,6 +296,11 @@ EXPORT_SYMBOL(ptp_clock_register); int ptp_clock_unregister(struct ptp_clock *ptp) { + if (ptp_guaranteed_pclock(ptp)) { + pr_err("ptp: virtual clock in use, guarantee physical clock free running\n"); + return -EBUSY; + } + ptp->defunct = 1; wake_up_interruptible(&ptp->tsev_wq); diff --git a/drivers/ptp/ptp_private.h b/drivers/ptp/ptp_private.h index 3f388d63904c..6949afc9d733 100644 --- a/drivers/ptp/ptp_private.h +++ b/drivers/ptp/ptp_private.h @@ -46,6 +46,9 @@ struct ptp_clock { const struct attribute_group *pin_attr_groups[2]; struct kthread_worker *kworker; struct kthread_delayed_work aux_work; + u8 n_vclocks; + struct mutex n_vclocks_mux; /* protect concurrent n_vclocks access */ + bool vclock_flag; }; #define info_to_vclock(d) container_of((d), struct ptp_vclock, info) @@ -75,6 +78,18 @@ static inline int queue_cnt(struct timestamp_event_queue *q) return cnt < 0 ? PTP_MAX_TIMESTAMPS + cnt : cnt; } +/* + * Guarantee physical clock to stay free running, if ptp virtual clocks + * on it are in use. + */ +static inline bool ptp_guaranteed_pclock(struct ptp_clock *ptp) +{ + if (!ptp->vclock_flag && ptp->n_vclocks) + return true; + + return false; +} + /* * see ptp_chardev.c */ diff --git a/drivers/ptp/ptp_sysfs.c b/drivers/ptp/ptp_sysfs.c index be076a91e20e..600edd7a90af 100644 --- a/drivers/ptp/ptp_sysfs.c +++ b/drivers/ptp/ptp_sysfs.c @@ -3,6 +3,7 @@ * PTP 1588 clock support - sysfs interface. * * Copyright (C) 2010 OMICRON electronics GmbH + * Copyright 2021 NXP */ #include <linux/capability.h> #include <linux/slab.h> @@ -148,6 +149,90 @@ static ssize_t pps_enable_store(struct device *dev, } static DEVICE_ATTR(pps_enable, 0220, NULL, pps_enable_store); +static int unregister_vclock(struct device *dev, void *data) +{ + struct ptp_clock *ptp = dev_get_drvdata(dev); + struct ptp_clock_info *info = ptp->info; + struct ptp_vclock *vclock; + u8 *num = data; + + vclock = info_to_vclock(info); + dev_info(dev->parent, "delete virtual clock ptp%d\n", + vclock->clock->index); + + ptp_vclock_unregister(vclock); + (*num)--; + + /* For break. Not error. */ + if (*num == 0) + return -EINVAL; + + return 0; +} + +static ssize_t n_vclocks_show(struct device *dev, + struct device_attribute *attr, char *page) +{ + struct ptp_clock *ptp = dev_get_drvdata(dev); + + return snprintf(page, PAGE_SIZE-1, "%d\n", ptp->n_vclocks); +} + +static ssize_t n_vclocks_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + struct ptp_clock *ptp = dev_get_drvdata(dev); + struct ptp_vclock *vclock; + int err = -EINVAL; + u8 num, i; + + if (kstrtou8(buf, 0, &num)) + goto out; + + if (num > PTP_MAX_VCLOCKS) { + dev_err(dev, "max value is %d\n", PTP_MAX_VCLOCKS); + goto out; + } + + if (mutex_lock_interruptible(&ptp->n_vclocks_mux)) + return -ERESTARTSYS; + + /* Need to create more vclocks */ + if (num > ptp->n_vclocks) { + for (i = 0; i < num - ptp->n_vclocks; i++) { + vclock = ptp_vclock_register(ptp); + if (!vclock) { + mutex_unlock(&ptp->n_vclocks_mux); + goto out; + } + + dev_info(dev, "new virtual clock ptp%d\n", + vclock->clock->index); + } + } + + /* Need to delete vclocks */ + if (num < ptp->n_vclocks) { + i = ptp->n_vclocks - num; + device_for_each_child_reverse(dev, &i, + unregister_vclock); + } + + if (num == 0) + dev_info(dev, "only physical clock in use now\n"); + else + dev_info(dev, "guarantee physical clock free running\n"); + + ptp->n_vclocks = num; + mutex_unlock(&ptp->n_vclocks_mux); + + return count; +out: + return err; +} +static DEVICE_ATTR_RW(n_vclocks); + static struct attribute *ptp_attrs[] = { &dev_attr_clock_name.attr, @@ -162,6 +247,7 @@ static struct attribute *ptp_attrs[] = { &dev_attr_fifo.attr, &dev_attr_period.attr, &dev_attr_pps_enable.attr, + &dev_attr_n_vclocks.attr, NULL }; @@ -183,6 +269,9 @@ static umode_t ptp_is_attribute_visible(struct kobject *kobj, } else if (attr == &dev_attr_pps_enable.attr) { if (!info->pps) mode = 0; + } else if (attr == &dev_attr_n_vclocks.attr) { + if (ptp->vclock_flag) + mode = 0; } return mode; diff --git a/include/uapi/linux/ptp_clock.h b/include/uapi/linux/ptp_clock.h index 1d108d597f66..4b933dc1b81b 100644 --- a/include/uapi/linux/ptp_clock.h +++ b/include/uapi/linux/ptp_clock.h @@ -69,6 +69,11 @@ */ #define PTP_PEROUT_V1_VALID_FLAGS (0) +/* + * Max number of PTP virtual clocks per PTP physical clock + */ +#define PTP_MAX_VCLOCKS 20 + /* * struct ptp_clock_time - represents a time value *
Support ptp physical/virtual clocks conversion via sysfs. There will be a new attribute n_vclocks under ptp physical clock sysfs. - In default, the value is 0 meaning only ptp physical clock is in use. - Setting the value can create corresponding number of ptp virtual clocks to use. But current physical clock is guaranteed to stay free running. - Setting the value back to 0 can delete virtual clocks and back use physical clock again. Signed-off-by: Yangbo Lu <yangbo.lu@nxp.com> --- Changes for v2: - Split from v1 patch #2. - Converted to num_vclocks for creating virtual clocks. - Guranteed physical clock free running when using virtual clocks. - Fixed build warning. - Updated copyright. Changes for v3: - Protected concurrency of ptp->num_vclocks accessing. --- Documentation/ABI/testing/sysfs-ptp | 13 +++++ drivers/ptp/ptp_clock.c | 22 +++++++ drivers/ptp/ptp_private.h | 15 +++++ drivers/ptp/ptp_sysfs.c | 89 +++++++++++++++++++++++++++++ include/uapi/linux/ptp_clock.h | 5 ++ 5 files changed, 144 insertions(+)