All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH net-next 0/5] Virtual PTP clock improvements and fix
@ 2022-01-27 11:45 Miroslav Lichvar
  2022-01-27 11:45 ` [PATCH net-next 1/5] ptp: unregister virtual clocks when unregistering physical clock Miroslav Lichvar
                   ` (5 more replies)
  0 siblings, 6 replies; 17+ messages in thread
From: Miroslav Lichvar @ 2022-01-27 11:45 UTC (permalink / raw)
  To: netdev; +Cc: Miroslav Lichvar

The first patch fixes an oops when unloading a driver with PTP clock and
enabled virtual clocks.

The other patches add missing features to make synchronization with
virtual clocks work as well as with the physical clock.

Miroslav Lichvar (5):
  ptp: unregister virtual clocks when unregistering physical clock.
  ptp: increase maximum adjustment of virtual clocks.
  ptp: add gettimex64() to virtual clocks.
  ptp: add getcrosststamp() to virtual clocks.
  ptp: start virtual clocks at current system time.

 drivers/ptp/ptp_clock.c  | 11 ++++++--
 drivers/ptp/ptp_vclock.c | 59 +++++++++++++++++++++++++++++++++++++---
 2 files changed, 64 insertions(+), 6 deletions(-)

-- 
2.34.1


^ permalink raw reply	[flat|nested] 17+ messages in thread

* [PATCH net-next 1/5] ptp: unregister virtual clocks when unregistering physical clock.
  2022-01-27 11:45 [PATCH net-next 0/5] Virtual PTP clock improvements and fix Miroslav Lichvar
@ 2022-01-27 11:45 ` Miroslav Lichvar
  2022-01-27 23:58   ` Vinicius Costa Gomes
  2022-01-27 11:45 ` [PATCH net-next 2/5] ptp: increase maximum adjustment of virtual clocks Miroslav Lichvar
                   ` (4 subsequent siblings)
  5 siblings, 1 reply; 17+ messages in thread
From: Miroslav Lichvar @ 2022-01-27 11:45 UTC (permalink / raw)
  To: netdev; +Cc: Miroslav Lichvar, Yangbo Lu, Yang Yingliang, Richard Cochran

When unregistering a physical clock which has some virtual clocks,
unregister the virtual clocks with it.

This fixes the following oops, which can be triggered by unloading
a driver providing a PTP clock when it has enabled virtual clocks:

BUG: unable to handle page fault for address: ffffffffc04fc4d8
Oops: 0000 [#1] PREEMPT SMP NOPTI
RIP: 0010:ptp_vclock_read+0x31/0xb0
Call Trace:
 timecounter_read+0xf/0x50
 ptp_vclock_refresh+0x2c/0x50
 ? ptp_clock_release+0x40/0x40
 ptp_aux_kworker+0x17/0x30
 kthread_worker_fn+0x9b/0x240
 ? kthread_should_park+0x30/0x30
 kthread+0xe2/0x110
 ? kthread_complete_and_exit+0x20/0x20
 ret_from_fork+0x22/0x30

Fixes: 73f37068d540 ("ptp: support ptp physical/virtual clocks conversion")
Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Cc: Yang Yingliang <yangyingliang@huawei.com>
Cc: Richard Cochran <richardcochran@gmail.com>
---
 drivers/ptp/ptp_clock.c | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 0e4bc8b9329d..b6f2cfd15dd2 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -317,11 +317,18 @@ struct ptp_clock *ptp_clock_register(struct ptp_clock_info *info,
 }
 EXPORT_SYMBOL(ptp_clock_register);
 
+static int unregister_vclock(struct device *dev, void *data)
+{
+	struct ptp_clock *ptp = dev_get_drvdata(dev);
+
+	ptp_vclock_unregister(info_to_vclock(ptp->info));
+	return 0;
+}
+
 int ptp_clock_unregister(struct ptp_clock *ptp)
 {
 	if (ptp_vclock_in_use(ptp)) {
-		pr_err("ptp: virtual clock in use\n");
-		return -EBUSY;
+		device_for_each_child(&ptp->dev, NULL, unregister_vclock);
 	}
 
 	ptp->defunct = 1;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 2/5] ptp: increase maximum adjustment of virtual clocks.
  2022-01-27 11:45 [PATCH net-next 0/5] Virtual PTP clock improvements and fix Miroslav Lichvar
  2022-01-27 11:45 ` [PATCH net-next 1/5] ptp: unregister virtual clocks when unregistering physical clock Miroslav Lichvar
@ 2022-01-27 11:45 ` Miroslav Lichvar
  2022-01-27 11:45 ` [PATCH net-next 3/5] ptp: add gettimex64() to " Miroslav Lichvar
                   ` (3 subsequent siblings)
  5 siblings, 0 replies; 17+ messages in thread
From: Miroslav Lichvar @ 2022-01-27 11:45 UTC (permalink / raw)
  To: netdev; +Cc: Miroslav Lichvar, Yangbo Lu, Richard Cochran

Increase the maximum frequency offset of virtual clocks to 50% to enable
faster slewing corrections.

This value cannot be represented as scaled ppm when long has 32 bits,
but that is already the case for other drivers, even those that provide
the adjfine() function, i.e. 32-bit applications are expected to check
for the limit.

Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Cc: Richard Cochran <richardcochran@gmail.com>
---
 drivers/ptp/ptp_vclock.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c
index ab1d233173e1..5aa2b32d9dc7 100644
--- a/drivers/ptp/ptp_vclock.c
+++ b/drivers/ptp/ptp_vclock.c
@@ -84,8 +84,7 @@ static long ptp_vclock_refresh(struct ptp_clock_info *ptp)
 static const struct ptp_clock_info ptp_vclock_info = {
 	.owner		= THIS_MODULE,
 	.name		= "ptp virtual clock",
-	/* The maximum ppb value that long scaled_ppm can support */
-	.max_adj	= 32767999,
+	.max_adj	= 500000000,
 	.adjfine	= ptp_vclock_adjfine,
 	.adjtime	= ptp_vclock_adjtime,
 	.gettime64	= ptp_vclock_gettime,
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 3/5] ptp: add gettimex64() to virtual clocks.
  2022-01-27 11:45 [PATCH net-next 0/5] Virtual PTP clock improvements and fix Miroslav Lichvar
  2022-01-27 11:45 ` [PATCH net-next 1/5] ptp: unregister virtual clocks when unregistering physical clock Miroslav Lichvar
  2022-01-27 11:45 ` [PATCH net-next 2/5] ptp: increase maximum adjustment of virtual clocks Miroslav Lichvar
@ 2022-01-27 11:45 ` Miroslav Lichvar
  2022-01-27 11:45 ` [PATCH net-next 4/5] ptp: add getcrosststamp() " Miroslav Lichvar
                   ` (2 subsequent siblings)
  5 siblings, 0 replies; 17+ messages in thread
From: Miroslav Lichvar @ 2022-01-27 11:45 UTC (permalink / raw)
  To: netdev; +Cc: Miroslav Lichvar, Yangbo Lu, Richard Cochran

If the physical clock has the gettimex64() function, provide a
gettimex64() wrapper in the virtual clock to enable more accurate
and stable synchronization.

This adds support for the PTP_SYS_OFFSET_EXTENDED ioctl.

Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Cc: Richard Cochran <richardcochran@gmail.com>
---
 drivers/ptp/ptp_vclock.c | 29 ++++++++++++++++++++++++++++-
 1 file changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c
index 5aa2b32d9dc7..2f0b46386176 100644
--- a/drivers/ptp/ptp_vclock.c
+++ b/drivers/ptp/ptp_vclock.c
@@ -57,6 +57,30 @@ static int ptp_vclock_gettime(struct ptp_clock_info *ptp,
 	return 0;
 }
 
+static int ptp_vclock_gettimex(struct ptp_clock_info *ptp,
+			       struct timespec64 *ts,
+			       struct ptp_system_timestamp *sts)
+{
+	struct ptp_vclock *vclock = info_to_vclock(ptp);
+	struct ptp_clock *pptp = vclock->pclock;
+	struct timespec64 pts;
+	unsigned long flags;
+	int err;
+	u64 ns;
+
+	err = pptp->info->gettimex64(pptp->info, &pts, sts);
+	if (err)
+		return err;
+
+	spin_lock_irqsave(&vclock->lock, flags);
+	ns = timecounter_cyc2time(&vclock->tc, timespec64_to_ns(&pts));
+	spin_unlock_irqrestore(&vclock->lock, flags);
+
+	*ts = ns_to_timespec64(ns);
+
+	return 0;
+}
+
 static int ptp_vclock_settime(struct ptp_clock_info *ptp,
 			      const struct timespec64 *ts)
 {
@@ -87,7 +111,6 @@ static const struct ptp_clock_info ptp_vclock_info = {
 	.max_adj	= 500000000,
 	.adjfine	= ptp_vclock_adjfine,
 	.adjtime	= ptp_vclock_adjtime,
-	.gettime64	= ptp_vclock_gettime,
 	.settime64	= ptp_vclock_settime,
 	.do_aux_work	= ptp_vclock_refresh,
 };
@@ -123,6 +146,10 @@ struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock)
 
 	vclock->pclock = pclock;
 	vclock->info = ptp_vclock_info;
+	if (pclock->info->gettimex64)
+		vclock->info.gettimex64 = ptp_vclock_gettimex;
+	else
+		vclock->info.gettime64 = ptp_vclock_gettime;
 	vclock->cc = ptp_vclock_cc;
 
 	snprintf(vclock->info.name, PTP_CLOCK_NAME_LEN, "ptp%d_virt",
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 4/5] ptp: add getcrosststamp() to virtual clocks.
  2022-01-27 11:45 [PATCH net-next 0/5] Virtual PTP clock improvements and fix Miroslav Lichvar
                   ` (2 preceding siblings ...)
  2022-01-27 11:45 ` [PATCH net-next 3/5] ptp: add gettimex64() to " Miroslav Lichvar
@ 2022-01-27 11:45 ` Miroslav Lichvar
  2022-01-27 11:45 ` [PATCH net-next 5/5] ptp: start virtual clocks at current system time Miroslav Lichvar
  2022-01-27 22:02 ` [PATCH net-next 0/5] Virtual PTP clock improvements and fix Richard Cochran
  5 siblings, 0 replies; 17+ messages in thread
From: Miroslav Lichvar @ 2022-01-27 11:45 UTC (permalink / raw)
  To: netdev; +Cc: Miroslav Lichvar, Yangbo Lu, Richard Cochran

If the physical clock supports cross timestamping (it has the
getcrosststamp() function), provide a wrapper in the virtual clock to
enable cross timestamping.

This adds support for the PTP_SYS_OFFSET_PRECISE ioctl.

Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Cc: Richard Cochran <richardcochran@gmail.com>
---
 drivers/ptp/ptp_vclock.c | 24 ++++++++++++++++++++++++
 1 file changed, 24 insertions(+)

diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c
index 2f0b46386176..cb179a3ea508 100644
--- a/drivers/ptp/ptp_vclock.c
+++ b/drivers/ptp/ptp_vclock.c
@@ -95,6 +95,28 @@ static int ptp_vclock_settime(struct ptp_clock_info *ptp,
 	return 0;
 }
 
+static int ptp_vclock_getcrosststamp(struct ptp_clock_info *ptp,
+				     struct system_device_crosststamp *xtstamp)
+{
+	struct ptp_vclock *vclock = info_to_vclock(ptp);
+	struct ptp_clock *pptp = vclock->pclock;
+	unsigned long flags;
+	int err;
+	u64 ns;
+
+	err = pptp->info->getcrosststamp(pptp->info, xtstamp);
+	if (err)
+		return err;
+
+	spin_lock_irqsave(&vclock->lock, flags);
+	ns = timecounter_cyc2time(&vclock->tc, ktime_to_ns(xtstamp->device));
+	spin_unlock_irqrestore(&vclock->lock, flags);
+
+	xtstamp->device = ns_to_ktime(ns);
+
+	return 0;
+}
+
 static long ptp_vclock_refresh(struct ptp_clock_info *ptp)
 {
 	struct ptp_vclock *vclock = info_to_vclock(ptp);
@@ -150,6 +172,8 @@ struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock)
 		vclock->info.gettimex64 = ptp_vclock_gettimex;
 	else
 		vclock->info.gettime64 = ptp_vclock_gettime;
+	if (pclock->info->getcrosststamp)
+		vclock->info.getcrosststamp = ptp_vclock_getcrosststamp;
 	vclock->cc = ptp_vclock_cc;
 
 	snprintf(vclock->info.name, PTP_CLOCK_NAME_LEN, "ptp%d_virt",
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* [PATCH net-next 5/5] ptp: start virtual clocks at current system time.
  2022-01-27 11:45 [PATCH net-next 0/5] Virtual PTP clock improvements and fix Miroslav Lichvar
                   ` (3 preceding siblings ...)
  2022-01-27 11:45 ` [PATCH net-next 4/5] ptp: add getcrosststamp() " Miroslav Lichvar
@ 2022-01-27 11:45 ` Miroslav Lichvar
  2022-01-27 22:01   ` Richard Cochran
  2022-01-27 22:02 ` [PATCH net-next 0/5] Virtual PTP clock improvements and fix Richard Cochran
  5 siblings, 1 reply; 17+ messages in thread
From: Miroslav Lichvar @ 2022-01-27 11:45 UTC (permalink / raw)
  To: netdev; +Cc: Miroslav Lichvar, Yangbo Lu, Richard Cochran

When a virtual clock is being created, initialize the timecounter to the
current system time instead of the Unix epoch to avoid very large steps
when the clock will be synchronized.

Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
Cc: Yangbo Lu <yangbo.lu@nxp.com>
Cc: Richard Cochran <richardcochran@gmail.com>
---
 drivers/ptp/ptp_vclock.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_vclock.c b/drivers/ptp/ptp_vclock.c
index cb179a3ea508..5a24a5128013 100644
--- a/drivers/ptp/ptp_vclock.c
+++ b/drivers/ptp/ptp_vclock.c
@@ -187,7 +187,8 @@ struct ptp_vclock *ptp_vclock_register(struct ptp_clock *pclock)
 		return NULL;
 	}
 
-	timecounter_init(&vclock->tc, &vclock->cc, 0);
+	timecounter_init(&vclock->tc, &vclock->cc,
+			 ktime_to_ns(ktime_get_real()));
 	ptp_schedule_worker(vclock->clock, PTP_VCLOCK_REFRESH_INTERVAL);
 
 	return vclock;
-- 
2.34.1


^ permalink raw reply related	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 5/5] ptp: start virtual clocks at current system time.
  2022-01-27 11:45 ` [PATCH net-next 5/5] ptp: start virtual clocks at current system time Miroslav Lichvar
@ 2022-01-27 22:01   ` Richard Cochran
  2022-01-31 10:21     ` Miroslav Lichvar
  0 siblings, 1 reply; 17+ messages in thread
From: Richard Cochran @ 2022-01-27 22:01 UTC (permalink / raw)
  To: Miroslav Lichvar; +Cc: netdev, Yangbo Lu

On Thu, Jan 27, 2022 at 12:45:36PM +0100, Miroslav Lichvar wrote:
> When a virtual clock is being created, initialize the timecounter to the
> current system time instead of the Unix epoch to avoid very large steps
> when the clock will be synchronized.

I think we agreed that, going forward, new PHC drivers should start at
zero (1970) instead of TAI - 37.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 0/5] Virtual PTP clock improvements and fix
  2022-01-27 11:45 [PATCH net-next 0/5] Virtual PTP clock improvements and fix Miroslav Lichvar
                   ` (4 preceding siblings ...)
  2022-01-27 11:45 ` [PATCH net-next 5/5] ptp: start virtual clocks at current system time Miroslav Lichvar
@ 2022-01-27 22:02 ` Richard Cochran
  5 siblings, 0 replies; 17+ messages in thread
From: Richard Cochran @ 2022-01-27 22:02 UTC (permalink / raw)
  To: Miroslav Lichvar; +Cc: netdev

On Thu, Jan 27, 2022 at 12:45:31PM +0100, Miroslav Lichvar wrote:
> The first patch fixes an oops when unloading a driver with PTP clock and
> enabled virtual clocks.
> 
> The other patches add missing features to make synchronization with
> virtual clocks work as well as with the physical clock.
> 
> Miroslav Lichvar (5):
>   ptp: unregister virtual clocks when unregistering physical clock.
>   ptp: increase maximum adjustment of virtual clocks.
>   ptp: add gettimex64() to virtual clocks.
>   ptp: add getcrosststamp() to virtual clocks.

For patches 1-4:

Acked-by: Richard Cochran <richardcochran@gmail.com>

>   ptp: start virtual clocks at current system time.

(I don't agree with this last one)

Thanks,
Richard



^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 1/5] ptp: unregister virtual clocks when unregistering physical clock.
  2022-01-27 11:45 ` [PATCH net-next 1/5] ptp: unregister virtual clocks when unregistering physical clock Miroslav Lichvar
@ 2022-01-27 23:58   ` Vinicius Costa Gomes
  2022-01-31 10:07     ` Miroslav Lichvar
  0 siblings, 1 reply; 17+ messages in thread
From: Vinicius Costa Gomes @ 2022-01-27 23:58 UTC (permalink / raw)
  To: Miroslav Lichvar, netdev
  Cc: Miroslav Lichvar, Yangbo Lu, Yang Yingliang, Richard Cochran

Miroslav Lichvar <mlichvar@redhat.com> writes:

> When unregistering a physical clock which has some virtual clocks,
> unregister the virtual clocks with it.
>
> This fixes the following oops, which can be triggered by unloading
> a driver providing a PTP clock when it has enabled virtual clocks:
>
> BUG: unable to handle page fault for address: ffffffffc04fc4d8
> Oops: 0000 [#1] PREEMPT SMP NOPTI
> RIP: 0010:ptp_vclock_read+0x31/0xb0
> Call Trace:
>  timecounter_read+0xf/0x50
>  ptp_vclock_refresh+0x2c/0x50
>  ? ptp_clock_release+0x40/0x40
>  ptp_aux_kworker+0x17/0x30
>  kthread_worker_fn+0x9b/0x240
>  ? kthread_should_park+0x30/0x30
>  kthread+0xe2/0x110
>  ? kthread_complete_and_exit+0x20/0x20
>  ret_from_fork+0x22/0x30
>
> Fixes: 73f37068d540 ("ptp: support ptp physical/virtual clocks conversion")
> Signed-off-by: Miroslav Lichvar <mlichvar@redhat.com>
> Cc: Yangbo Lu <yangbo.lu@nxp.com>
> Cc: Yang Yingliang <yangyingliang@huawei.com>
> Cc: Richard Cochran <richardcochran@gmail.com>
> ---

I am not against this change, but I think this problem was discussed
before and the suggestions were to fix it differently:

https://lore.kernel.org/all/20210807144332.szyazdfl42abwzmd@skbuf/


Cheers,
-- 
Vinicius

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 1/5] ptp: unregister virtual clocks when unregistering physical clock.
  2022-01-27 23:58   ` Vinicius Costa Gomes
@ 2022-01-31 10:07     ` Miroslav Lichvar
  2022-01-31 12:41       ` Vladimir Oltean
  0 siblings, 1 reply; 17+ messages in thread
From: Miroslav Lichvar @ 2022-01-31 10:07 UTC (permalink / raw)
  To: Vinicius Costa Gomes
  Cc: netdev, Yangbo Lu, Yang Yingliang, Richard Cochran, Vladimir Oltean

On Thu, Jan 27, 2022 at 03:58:02PM -0800, Vinicius Costa Gomes wrote:
> Miroslav Lichvar <mlichvar@redhat.com> writes:
> 
> > When unregistering a physical clock which has some virtual clocks,
> > unregister the virtual clocks with it.

> I am not against this change, but I think this problem was discussed
> before and the suggestions were to fix it differently:
> 
> https://lore.kernel.org/all/20210807144332.szyazdfl42abwzmd@skbuf/

Is a linked device supposed to be unregistered automatically before
the parent? The referenced document mentions only suspending
and resuming, nothing about unregistering.

I tried

	device_link_add(parent, &ptp->dev, DL_FLAG_AUTOREMOVE_CONSUMER);

and also with no flags specified, but it didn't seem to do anything
for the vclock. It was still oopsing.

Any hints?

-- 
Miroslav Lichvar


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 5/5] ptp: start virtual clocks at current system time.
  2022-01-27 22:01   ` Richard Cochran
@ 2022-01-31 10:21     ` Miroslav Lichvar
  2022-01-31 16:32       ` Richard Cochran
  2022-02-01 19:03       ` Richard Cochran
  0 siblings, 2 replies; 17+ messages in thread
From: Miroslav Lichvar @ 2022-01-31 10:21 UTC (permalink / raw)
  To: Richard Cochran; +Cc: netdev, Yangbo Lu

On Thu, Jan 27, 2022 at 02:01:16PM -0800, Richard Cochran wrote:
> On Thu, Jan 27, 2022 at 12:45:36PM +0100, Miroslav Lichvar wrote:
> > When a virtual clock is being created, initialize the timecounter to the
> > current system time instead of the Unix epoch to avoid very large steps
> > when the clock will be synchronized.
> 
> I think we agreed that, going forward, new PHC drivers should start at
> zero (1970) instead of TAI - 37.

I tried to find the discussion around this decision, but failed. Do
you have a link?

To me, it seems very strange to start the PHC at 0. It makes the
initial clock correction unnecessarily larger by ~7 orders of
magnitude. The system clock is initialized from the RTC, which can
have an error comparable to the TAI-UTC offset, especially if the
machine was turned off for a longer period of time, so why not
initialize the PHC from the system time? The error is much smaller
than billions of seconds.

-- 
Miroslav Lichvar


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 1/5] ptp: unregister virtual clocks when unregistering physical clock.
  2022-01-31 10:07     ` Miroslav Lichvar
@ 2022-01-31 12:41       ` Vladimir Oltean
  0 siblings, 0 replies; 17+ messages in thread
From: Vladimir Oltean @ 2022-01-31 12:41 UTC (permalink / raw)
  To: Miroslav Lichvar
  Cc: Vinicius Costa Gomes, netdev, Yangbo Lu, Yang Yingliang, Richard Cochran

Hi,

On Mon, Jan 31, 2022 at 11:07:52AM +0100, Miroslav Lichvar wrote:
> On Thu, Jan 27, 2022 at 03:58:02PM -0800, Vinicius Costa Gomes wrote:
> > Miroslav Lichvar <mlichvar@redhat.com> writes:
> > 
> > > When unregistering a physical clock which has some virtual clocks,
> > > unregister the virtual clocks with it.
> 
> > I am not against this change, but I think this problem was discussed
> > before and the suggestions were to fix it differently:
> > 
> > https://lore.kernel.org/all/20210807144332.szyazdfl42abwzmd@skbuf/
> 
> Is a linked device supposed to be unregistered automatically before
> the parent? The referenced document mentions only suspending
> and resuming, nothing about unregistering.
> 
> I tried
> 
> 	device_link_add(parent, &ptp->dev, DL_FLAG_AUTOREMOVE_CONSUMER);
> 
> and also with no flags specified, but it didn't seem to do anything
> for the vclock. It was still oopsing.
> 
> Any hints?
> 
> -- 
> Miroslav Lichvar
> 

First of all, I was wrong about the device hierarchy created by the PTP
subsystem, and as such, device links won't work.

When you unbind a physical PTP clock, what you actually unbind is the
driver from its parent device (the parent->parent, i.e. what is passed
as "parent" to the parent's ptp_clock_register call).

But since the PTP subsystem doesn't register its devices with the device
hierarchy (of device_register(), it only calls the first half:
device_initialize(), it doesn't call device_add), so the dev->kobj
doesn't get added to the devices_kset. The documentation says:

| The earliest point in time when device links can be added is after
| device_add() has been called for the supplier and device_initialize()
| has been called for the consumer.

Furthermore, PTP devices don't have a driver bound to them, with probe()
and remove() callbacks. The only driver is that of the parent of the
physical PHC. So no driver for the vclock => no hook to call
ptp_vclock_unregister() from, even if a device link from ptp->dev to
parent->parent can be added.

So your solution appears to be the correct one given the structure - call
unregister_vclock() manually.

Secondly, you got the arguments in reverse: the consumer is the first
argument, and that is &ptp->dev, and the supplier is "parent".

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 5/5] ptp: start virtual clocks at current system time.
  2022-01-31 10:21     ` Miroslav Lichvar
@ 2022-01-31 16:32       ` Richard Cochran
  2022-02-01  8:42         ` Miroslav Lichvar
  2022-02-01 19:03       ` Richard Cochran
  1 sibling, 1 reply; 17+ messages in thread
From: Richard Cochran @ 2022-01-31 16:32 UTC (permalink / raw)
  To: Miroslav Lichvar; +Cc: netdev, Yangbo Lu

On Mon, Jan 31, 2022 at 11:21:08AM +0100, Miroslav Lichvar wrote:
> I tried to find the discussion around this decision, but failed. Do
> you have a link?

I'll dig it up for you.
 
> To me, it seems very strange to start the PHC at 0. It makes the
> initial clock correction unnecessarily larger by ~7 orders of
> magnitude. The system clock is initialized from the RTC, which can
> have an error comparable to the TAI-UTC offset, especially if the
> machine was turned off for a longer period of time, so why not
> initialize the PHC from the system time? The error is much smaller
> than billions of seconds.

When the clock reads Jan 1, 1970, then that is clearly wrong, and so a
user might suspect that it is uninititalized.

When the clock is off by 37 seconds, the user will likely post a vague
complaint to linuxptp-users asking why linuxptp doesn't handle leap
seconds.

I prefer the clarity of the first case.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 5/5] ptp: start virtual clocks at current system time.
  2022-01-31 16:32       ` Richard Cochran
@ 2022-02-01  8:42         ` Miroslav Lichvar
  2022-02-01 19:10           ` Richard Cochran
  0 siblings, 1 reply; 17+ messages in thread
From: Miroslav Lichvar @ 2022-02-01  8:42 UTC (permalink / raw)
  To: Richard Cochran; +Cc: netdev, Yangbo Lu

On Mon, Jan 31, 2022 at 08:32:40AM -0800, Richard Cochran wrote:
> On Mon, Jan 31, 2022 at 11:21:08AM +0100, Miroslav Lichvar wrote:
> > To me, it seems very strange to start the PHC at 0. It makes the
> > initial clock correction unnecessarily larger by ~7 orders of
> > magnitude. The system clock is initialized from the RTC, which can
> > have an error comparable to the TAI-UTC offset, especially if the
> > machine was turned off for a longer period of time, so why not
> > initialize the PHC from the system time? The error is much smaller
> > than billions of seconds.
> 
> When the clock reads Jan 1, 1970, then that is clearly wrong, and so a
> user might suspect that it is uninititalized.

FWIW, my first thought when I saw the huge offset in ptp4l was that
something is horribly broken. 

> I prefer the clarity of the first case.

I'd prefer smaller initial error and consistency. The vast majority of
existing drivers seem to initialize the clock at current system time.
Drivers starting at 0 now create confusion. If this is the right way,
shouldn't be all existing drivers patched to follow that?

-- 
Miroslav Lichvar


^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 5/5] ptp: start virtual clocks at current system time.
  2022-01-31 10:21     ` Miroslav Lichvar
  2022-01-31 16:32       ` Richard Cochran
@ 2022-02-01 19:03       ` Richard Cochran
  2022-02-02  9:07         ` Miroslav Lichvar
  1 sibling, 1 reply; 17+ messages in thread
From: Richard Cochran @ 2022-02-01 19:03 UTC (permalink / raw)
  To: Miroslav Lichvar; +Cc: netdev, Yangbo Lu

On Mon, Jan 31, 2022 at 11:21:08AM +0100, Miroslav Lichvar wrote:
> On Thu, Jan 27, 2022 at 02:01:16PM -0800, Richard Cochran wrote:
> > On Thu, Jan 27, 2022 at 12:45:36PM +0100, Miroslav Lichvar wrote:
> > > When a virtual clock is being created, initialize the timecounter to the
> > > current system time instead of the Unix epoch to avoid very large steps
> > > when the clock will be synchronized.
> > 
> > I think we agreed that, going forward, new PHC drivers should start at
> > zero (1970) instead of TAI - 37.
> 
> I tried to find the discussion around this decision, but failed. Do
> you have a link?

Here is one of the discussions.  It didn't lay down a law or anything,
but my arguments still stand.

   https://lore.kernel.org/all/20180815175040.3736548-1-arnd@arndb.de/

I've been pushing back on new drivers that use ktime_get isntead of
zero, but some still sneak through, now and again.
 
> To me, it seems very strange to start the PHC at 0. It makes the
> initial clock correction unnecessarily larger by ~7 orders of
> magnitude. The system clock is initialized from the RTC, which can
> have an error comparable to the TAI-UTC offset, especially if the
> machine was turned off for a longer period of time, so why not
> initialize the PHC from the system time? The error is much smaller
> than billions of seconds.

Who cares?  The size of the error is irrelevant, because the initial
offset is wrong, and the action of the PTP will correct it, whether
large or small.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 5/5] ptp: start virtual clocks at current system time.
  2022-02-01  8:42         ` Miroslav Lichvar
@ 2022-02-01 19:10           ` Richard Cochran
  0 siblings, 0 replies; 17+ messages in thread
From: Richard Cochran @ 2022-02-01 19:10 UTC (permalink / raw)
  To: Miroslav Lichvar; +Cc: netdev, Yangbo Lu

On Tue, Feb 01, 2022 at 09:42:07AM +0100, Miroslav Lichvar wrote:
> On Mon, Jan 31, 2022 at 08:32:40AM -0800, Richard Cochran wrote:
> > On Mon, Jan 31, 2022 at 11:21:08AM +0100, Miroslav Lichvar wrote:
> > > To me, it seems very strange to start the PHC at 0. It makes the
> > > initial clock correction unnecessarily larger by ~7 orders of
> > > magnitude. The system clock is initialized from the RTC, which can
> > > have an error comparable to the TAI-UTC offset, especially if the
> > > machine was turned off for a longer period of time, so why not
> > > initialize the PHC from the system time? The error is much smaller
> > > than billions of seconds.
> > 
> > When the clock reads Jan 1, 1970, then that is clearly wrong, and so a
> > user might suspect that it is uninititalized.
> 
> FWIW, my first thought when I saw the huge offset in ptp4l was that
> something is horribly broken. 

Yes, that is my point!  Although you may have jumped to conclusions
about the root cause, still the zero value got your attention.

It is just too easy for people to see the correct date and time (down
to the minute) and assume all is okay.
 
> I'd prefer smaller initial error and consistency. The vast majority of
> existing drivers seem to initialize the clock at current system time.
> Drivers starting at 0 now create confusion. If this is the right way,
> shouldn't be all existing drivers patched to follow that?

I agree that consistency is good, and I would love to get rid of all
that ktime_get usage, but maybe people will argue against it for their
beloved driver.

Going forward, I'm asking that new drivers start from zero for an
"uninitialized" clock.

Thanks,
Richard

^ permalink raw reply	[flat|nested] 17+ messages in thread

* Re: [PATCH net-next 5/5] ptp: start virtual clocks at current system time.
  2022-02-01 19:03       ` Richard Cochran
@ 2022-02-02  9:07         ` Miroslav Lichvar
  0 siblings, 0 replies; 17+ messages in thread
From: Miroslav Lichvar @ 2022-02-02  9:07 UTC (permalink / raw)
  To: Richard Cochran; +Cc: netdev, Yangbo Lu

On Tue, Feb 01, 2022 at 11:03:51AM -0800, Richard Cochran wrote:
> On Mon, Jan 31, 2022 at 11:21:08AM +0100, Miroslav Lichvar wrote:
> > On Thu, Jan 27, 2022 at 02:01:16PM -0800, Richard Cochran wrote:
> > > I think we agreed that, going forward, new PHC drivers should start at
> > > zero (1970) instead of TAI - 37.
> > 
> > I tried to find the discussion around this decision, but failed. Do
> > you have a link?
> 
> Here is one of the discussions.  It didn't lay down a law or anything,
> but my arguments still stand.
> 
>    https://lore.kernel.org/all/20180815175040.3736548-1-arnd@arndb.de/

I don't see strong arguments for either option (0, UTC, TAI) and would
prefer consistency among drivers.

It would be nice if the drivers called a common function to get the
initial time, so it could be changed for all in one place if
necessary. Anyway, I'll leave this for another time and drop the patch
from the series.

Thanks,

-- 
Miroslav Lichvar


^ permalink raw reply	[flat|nested] 17+ messages in thread

end of thread, other threads:[~2022-02-02  9:08 UTC | newest]

Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-01-27 11:45 [PATCH net-next 0/5] Virtual PTP clock improvements and fix Miroslav Lichvar
2022-01-27 11:45 ` [PATCH net-next 1/5] ptp: unregister virtual clocks when unregistering physical clock Miroslav Lichvar
2022-01-27 23:58   ` Vinicius Costa Gomes
2022-01-31 10:07     ` Miroslav Lichvar
2022-01-31 12:41       ` Vladimir Oltean
2022-01-27 11:45 ` [PATCH net-next 2/5] ptp: increase maximum adjustment of virtual clocks Miroslav Lichvar
2022-01-27 11:45 ` [PATCH net-next 3/5] ptp: add gettimex64() to " Miroslav Lichvar
2022-01-27 11:45 ` [PATCH net-next 4/5] ptp: add getcrosststamp() " Miroslav Lichvar
2022-01-27 11:45 ` [PATCH net-next 5/5] ptp: start virtual clocks at current system time Miroslav Lichvar
2022-01-27 22:01   ` Richard Cochran
2022-01-31 10:21     ` Miroslav Lichvar
2022-01-31 16:32       ` Richard Cochran
2022-02-01  8:42         ` Miroslav Lichvar
2022-02-01 19:10           ` Richard Cochran
2022-02-01 19:03       ` Richard Cochran
2022-02-02  9:07         ` Miroslav Lichvar
2022-01-27 22:02 ` [PATCH net-next 0/5] Virtual PTP clock improvements and fix Richard Cochran

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.