[RFC 0/3] ptp: Add esterror support

[RFC 0/3] ptp: Add esterror support

[Maciek Machnikowski]

This patch series implements handling of timex esterror field
by ptp devices.

Esterror field can be used to return or set the estimated error
of the clock. This is useful for devices containing a hardware
clock that is controlled and synchronized internally (such as
a time card) or when the synchronization is pushed to the embedded
CPU of a DPU.

Current implementation of ADJ_ESTERROR can enable pushing
current offset of the clock calculated by a userspace app
to the device, which can act upon this information by enabling
or disabling time-related functions when certain boundaries
are not met (eg. packet launchtime)


Maciek Machnikowski (3):
  ptp: Implement timex esterror support
  ptp: Implement support for esterror in ptp_mock
  ptp: Add setting esterror and reading timex structure

 drivers/ptp/ptp_clock.c               | 14 +++++++++-
 drivers/ptp/ptp_mock.c                | 30 +++++++++++++++++++++
 include/linux/ptp_clock_kernel.h      | 11 ++++++++
 tools/testing/selftests/ptp/testptp.c | 39 +++++++++++++++++++++++++--
 4 files changed, 91 insertions(+), 3 deletions(-)

-- 
2.34.1

[RFC 1/3] ptp: Implement timex esterror support

[Maciek Machnikowski]

The Timex structure returned by the clock_adjtime() POSIX API allows
the clock to return the estimated error. Implement getesterror
and setesterror functions in the ptp_clock_info to enable drivers
to interact with the hardware to get the error information.

getesterror additionally implements returning hw_ts and sys_ts
to enable upper layers to estimate the maximum error of the clock
based on the last time of correction. This functionality is not
directly implemented in the clock_adjtime and will require
a separate interface in the future.

Signed-off-by: Maciek Machnikowski <maciek@machnikowski.net>
---
 drivers/ptp/ptp_clock.c          | 18 +++++++++++++++++-
 include/linux/ptp_clock_kernel.h | 11 +++++++++++
 2 files changed, 28 insertions(+), 1 deletion(-)

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index c56cd0f63909..2cb1f6af60ea 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c
@@ -164,9 +164,25 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct __kernel_timex *tx)
 
 			err = ops->adjphase(ops, offset);
 		}
+	} else if (tx->modes & ADJ_ESTERROR) {
+		if (ops->setesterror)
+			if (tx->modes & ADJ_NANO)
+				err = ops->setesterror(ops, tx->esterror * 1000);
+			else
+				err = ops->setesterror(ops, tx->esterror);
 	} else if (tx->modes == 0) {
+		long esterror;
+
 		tx->freq = ptp->dialed_frequency;
-		err = 0;
+		if (ops->getesterror) {
+			err = ops->getesterror(ops, &esterror, NULL, NULL);
+			if (err)
+				return err;
+			tx->modes &= ADJ_NANO;
+			tx->esterror = esterror;
+		} else {
+			err = 0;
+		}
 	}
 
 	return err;
diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
index 6e4b8206c7d0..e78ea81fc4cf 100644
--- a/include/linux/ptp_clock_kernel.h
+++ b/include/linux/ptp_clock_kernel.h
@@ -136,6 +136,14 @@ struct ptp_system_timestamp {
  *                   parameter cts: Contains timestamp (device,system) pair,
  *                   where system time is realtime and monotonic.
  *
+ * @getesterror: Reads the current error estimate of the hardware clock.
+ *               parameter phase: Holds the error estimate in nanoseconds.
+ *               parameter hw_ts: If not NULL, holds the timestamp of the hardware clock.
+ *               parameter sw_ts: If not NULL, holds the timestamp of the CPU clock.
+ *
+ * @setesterror:  Set the error estimate of the hardware clock.
+ *                parameter phase: Desired error estimate in nanoseconds.
+ *
  * @enable:   Request driver to enable or disable an ancillary feature.
  *            parameter request: Desired resource to enable or disable.
  *            parameter on: Caller passes one to enable or zero to disable.
@@ -188,6 +196,9 @@ struct ptp_clock_info {
 			    struct ptp_system_timestamp *sts);
 	int (*getcrosscycles)(struct ptp_clock_info *ptp,
 			      struct system_device_crosststamp *cts);
+	int (*getesterror)(struct ptp_clock_info *ptp, long *phase,
+			   struct timespec64 *hw_ts, struct timespec64 *sys_ts);
+	int (*setesterror)(struct ptp_clock_info *ptp, long phase);
 	int (*enable)(struct ptp_clock_info *ptp,
 		      struct ptp_clock_request *request, int on);
 	int (*verify)(struct ptp_clock_info *ptp, unsigned int pin,
-- 
2.34.1

Re: [RFC 1/3] ptp: Implement timex esterror support

[Vadim Fedorenko]

On 13/08/2024 13:56, Maciek Machnikowski wrote:
> !-------------------------------------------------------------------|
>    This Message Is From an Untrusted Sender
>    You have not previously corresponded with this sender.
> |-------------------------------------------------------------------!
> 
> The Timex structure returned by the clock_adjtime() POSIX API allows
> the clock to return the estimated error. Implement getesterror
> and setesterror functions in the ptp_clock_info to enable drivers
> to interact with the hardware to get the error information.
> 
> getesterror additionally implements returning hw_ts and sys_ts
> to enable upper layers to estimate the maximum error of the clock
> based on the last time of correction. This functionality is not
> directly implemented in the clock_adjtime and will require
> a separate interface in the future.
> 
> Signed-off-by: Maciek Machnikowski <maciek@machnikowski.net>
> ---
>   drivers/ptp/ptp_clock.c          | 18 +++++++++++++++++-
>   include/linux/ptp_clock_kernel.h | 11 +++++++++++
>   2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index c56cd0f63909..2cb1f6af60ea 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -164,9 +164,25 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct __kernel_timex *tx)
>   
>   			err = ops->adjphase(ops, offset);
>   		}
> +	} else if (tx->modes & ADJ_ESTERROR) {
> +		if (ops->setesterror)
> +			if (tx->modes & ADJ_NANO)
> +				err = ops->setesterror(ops, tx->esterror * 1000);

Looks like some miscoding here. The callback doc later says that
setesterror expects nanoseconds. ADJ_NANO switches the structure to
provide nanoseconds. But the code here expects tx->esterror to be in
microseconds when ADJ_NANO is set, which is confusing.

> +			else
> +				err = ops->setesterror(ops, tx->esterror);
>   	} else if (tx->modes == 0) {
> +		long esterror;
> +
>   		tx->freq = ptp->dialed_frequency;
> -		err = 0;
> +		if (ops->getesterror) {
> +			err = ops->getesterror(ops, &esterror, NULL, NULL);
> +			if (err)
> +				return err;
> +			tx->modes &= ADJ_NANO;

Here all the flags except possible ADJ_NANO is cleaned, but why?

> +			tx->esterror = esterror
And here it doesn't matter what is set in flags, you return nanoseconds 
directly...

> +		} else {
> +			err = 0;
> +		}
>   	}
>   
>   	return err;
> diff --git a/include/linux/ptp_clock_kernel.h b/include/linux/ptp_clock_kernel.h
> index 6e4b8206c7d0..e78ea81fc4cf 100644
> --- a/include/linux/ptp_clock_kernel.h
> +++ b/include/linux/ptp_clock_kernel.h
> @@ -136,6 +136,14 @@ struct ptp_system_timestamp {
>    *                   parameter cts: Contains timestamp (device,system) pair,
>    *                   where system time is realtime and monotonic.
>    *
> + * @getesterror: Reads the current error estimate of the hardware clock.
> + *               parameter phase: Holds the error estimate in nanoseconds.
> + *               parameter hw_ts: If not NULL, holds the timestamp of the hardware clock.
> + *               parameter sw_ts: If not NULL, holds the timestamp of the CPU clock.
> + *
> + * @setesterror:  Set the error estimate of the hardware clock.
> + *                parameter phase: Desired error estimate in nanoseconds.
> + *

The man pages says that esterror is in microseconds. It makes total
sense to make it nanoseconds, but we have to adjust the documentation.

>    * @enable:   Request driver to enable or disable an ancillary feature.
>    *            parameter request: Desired resource to enable or disable.
>    *            parameter on: Caller passes one to enable or zero to disable.
> @@ -188,6 +196,9 @@ struct ptp_clock_info {
>   			    struct ptp_system_timestamp *sts);
>   	int (*getcrosscycles)(struct ptp_clock_info *ptp,
>   			      struct system_device_crosststamp *cts);
> +	int (*getesterror)(struct ptp_clock_info *ptp, long *phase,
> +			   struct timespec64 *hw_ts, struct timespec64 *sys_ts);
> +	int (*setesterror)(struct ptp_clock_info *ptp, long phase);
>   	int (*enable)(struct ptp_clock_info *ptp,
>   		      struct ptp_clock_request *request, int on);
>   	int (*verify)(struct ptp_clock_info *ptp, unsigned int pin,

Re: [RFC 1/3] ptp: Implement timex esterror support

[Maciek Machnikowski]

On 14/08/2024 11:29, Vadim Fedorenko wrote:
> On 13/08/2024 13:56, Maciek Machnikowski wrote:
>> !-------------------------------------------------------------------|
>>    This Message Is From an Untrusted Sender
>>    You have not previously corresponded with this sender.
>> |-------------------------------------------------------------------!
>>
>> The Timex structure returned by the clock_adjtime() POSIX API allows
>> the clock to return the estimated error. Implement getesterror
>> and setesterror functions in the ptp_clock_info to enable drivers
>> to interact with the hardware to get the error information.
>>
>> getesterror additionally implements returning hw_ts and sys_ts
>> to enable upper layers to estimate the maximum error of the clock
>> based on the last time of correction. This functionality is not
>> directly implemented in the clock_adjtime and will require
>> a separate interface in the future.
>>
>> Signed-off-by: Maciek Machnikowski <maciek@machnikowski.net>
>> ---
>>   drivers/ptp/ptp_clock.c          | 18 +++++++++++++++++-
>>   include/linux/ptp_clock_kernel.h | 11 +++++++++++
>>   2 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
>> index c56cd0f63909..2cb1f6af60ea 100644
>> --- a/drivers/ptp/ptp_clock.c
>> +++ b/drivers/ptp/ptp_clock.c
>> @@ -164,9 +164,25 @@ static int ptp_clock_adjtime(struct posix_clock
>> *pc, struct __kernel_timex *tx)
>>                 err = ops->adjphase(ops, offset);
>>           }
>> +    } else if (tx->modes & ADJ_ESTERROR) {
>> +        if (ops->setesterror)
>> +            if (tx->modes & ADJ_NANO)
>> +                err = ops->setesterror(ops, tx->esterror * 1000);
> 
> Looks like some miscoding here. The callback doc later says that
> setesterror expects nanoseconds. ADJ_NANO switches the structure to
> provide nanoseconds. But the code here expects tx->esterror to be in
> microseconds when ADJ_NANO is set, which is confusing.
> 
>> +            else
>> +                err = ops->setesterror(ops, tx->esterror);
>>       } else if (tx->modes == 0) {
>> +        long esterror;
>> +
>>           tx->freq = ptp->dialed_frequency;
>> -        err = 0;
>> +        if (ops->getesterror) {
>> +            err = ops->getesterror(ops, &esterror, NULL, NULL);
>> +            if (err)
>> +                return err;
>> +            tx->modes &= ADJ_NANO;
> 
> Here all the flags except possible ADJ_NANO is cleaned, but why?
> 
>> +            tx->esterror = esterror
> And here it doesn't matter what is set in flags, you return nanoseconds
> directly...
> 
You're right - seems I made an inverted logic. The PTP API should work
on nanoseconds and the layer above should convert different units to
nanoseconds.

>> +        } else {
>> +            err = 0;
>> +        }
>>       }
>>         return err;
>> diff --git a/include/linux/ptp_clock_kernel.h
>> b/include/linux/ptp_clock_kernel.h
>> index 6e4b8206c7d0..e78ea81fc4cf 100644
>> --- a/include/linux/ptp_clock_kernel.h
>> +++ b/include/linux/ptp_clock_kernel.h
>> @@ -136,6 +136,14 @@ struct ptp_system_timestamp {
>>    *                   parameter cts: Contains timestamp
>> (device,system) pair,
>>    *                   where system time is realtime and monotonic.
>>    *
>> + * @getesterror: Reads the current error estimate of the hardware clock.
>> + *               parameter phase: Holds the error estimate in
>> nanoseconds.
>> + *               parameter hw_ts: If not NULL, holds the timestamp of
>> the hardware clock.
>> + *               parameter sw_ts: If not NULL, holds the timestamp of
>> the CPU clock.
>> + *
>> + * @setesterror:  Set the error estimate of the hardware clock.
>> + *                parameter phase: Desired error estimate in
>> nanoseconds.
>> + *
> 
> The man pages says that esterror is in microseconds. It makes total
> sense to make it nanoseconds, but we have to adjust the documentation.
> 
>>    * @enable:   Request driver to enable or disable an ancillary feature.
>>    *            parameter request: Desired resource to enable or disable.
>>    *            parameter on: Caller passes one to enable or zero to
>> disable.
>> @@ -188,6 +196,9 @@ struct ptp_clock_info {
>>                   struct ptp_system_timestamp *sts);
>>       int (*getcrosscycles)(struct ptp_clock_info *ptp,
>>                     struct system_device_crosststamp *cts);
>> +    int (*getesterror)(struct ptp_clock_info *ptp, long *phase,
>> +               struct timespec64 *hw_ts, struct timespec64 *sys_ts);
>> +    int (*setesterror)(struct ptp_clock_info *ptp, long phase);
>>       int (*enable)(struct ptp_clock_info *ptp,
>>                 struct ptp_clock_request *request, int on);
>>       int (*verify)(struct ptp_clock_info *ptp, unsigned int pin,
> 

Re: [RFC 1/3] ptp: Implement timex esterror support

[Simon Horman]

On Tue, Aug 13, 2024 at 12:56:00PM +0000, Maciek Machnikowski wrote:
> The Timex structure returned by the clock_adjtime() POSIX API allows
> the clock to return the estimated error. Implement getesterror
> and setesterror functions in the ptp_clock_info to enable drivers
> to interact with the hardware to get the error information.
> 
> getesterror additionally implements returning hw_ts and sys_ts
> to enable upper layers to estimate the maximum error of the clock
> based on the last time of correction. This functionality is not
> directly implemented in the clock_adjtime and will require
> a separate interface in the future.
> 
> Signed-off-by: Maciek Machnikowski <maciek@machnikowski.net>
> ---
>  drivers/ptp/ptp_clock.c          | 18 +++++++++++++++++-
>  include/linux/ptp_clock_kernel.h | 11 +++++++++++
>  2 files changed, 28 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
> index c56cd0f63909..2cb1f6af60ea 100644
> --- a/drivers/ptp/ptp_clock.c
> +++ b/drivers/ptp/ptp_clock.c
> @@ -164,9 +164,25 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct __kernel_timex *tx)
>  
>  			err = ops->adjphase(ops, offset);
>  		}
> +	} else if (tx->modes & ADJ_ESTERROR) {
> +		if (ops->setesterror)
> +			if (tx->modes & ADJ_NANO)
> +				err = ops->setesterror(ops, tx->esterror * 1000);
> +			else
> +				err = ops->setesterror(ops, tx->esterror);

Both GCC-14 and Clang-18 complain when compiling with W=1 that the relation
of the else to the two if's is ambiguous. Based on indentation I suggest
adding a { } to the outer-if.

>  	} else if (tx->modes == 0) {
> +		long esterror;
> +
>  		tx->freq = ptp->dialed_frequency;
> -		err = 0;
> +		if (ops->getesterror) {
> +			err = ops->getesterror(ops, &esterror, NULL, NULL);
> +			if (err)
> +				return err;
> +			tx->modes &= ADJ_NANO;
> +			tx->esterror = esterror;
> +		} else {
> +			err = 0;
> +		}
>  	}
>  
>  	return err;

...

Re: [RFC 1/3] ptp: Implement timex esterror support

[Maciek Machnikowski]

On 14/08/2024 13:46, Simon Horman wrote:
> On Tue, Aug 13, 2024 at 12:56:00PM +0000, Maciek Machnikowski wrote:
>> The Timex structure returned by the clock_adjtime() POSIX API allows
>> the clock to return the estimated error. Implement getesterror
>> and setesterror functions in the ptp_clock_info to enable drivers
>> to interact with the hardware to get the error information.
>>
>> getesterror additionally implements returning hw_ts and sys_ts
>> to enable upper layers to estimate the maximum error of the clock
>> based on the last time of correction. This functionality is not
>> directly implemented in the clock_adjtime and will require
>> a separate interface in the future.
>>
>> Signed-off-by: Maciek Machnikowski <maciek@machnikowski.net>
>> ---
>>  drivers/ptp/ptp_clock.c          | 18 +++++++++++++++++-
>>  include/linux/ptp_clock_kernel.h | 11 +++++++++++
>>  2 files changed, 28 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
>> index c56cd0f63909..2cb1f6af60ea 100644
>> --- a/drivers/ptp/ptp_clock.c
>> +++ b/drivers/ptp/ptp_clock.c
>> @@ -164,9 +164,25 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct __kernel_timex *tx)
>>  
>>  			err = ops->adjphase(ops, offset);
>>  		}
>> +	} else if (tx->modes & ADJ_ESTERROR) {
>> +		if (ops->setesterror)
>> +			if (tx->modes & ADJ_NANO)
>> +				err = ops->setesterror(ops, tx->esterror * 1000);
>> +			else
>> +				err = ops->setesterror(ops, tx->esterror);
> 
> Both GCC-14 and Clang-18 complain when compiling with W=1 that the relation
> of the else to the two if's is ambiguous. Based on indentation I suggest
> adding a { } to the outer-if.

Thanks for catching this one - will fix in upcoming release!

Re: [RFC 1/3] ptp: Implement timex esterror support

[Richard Cochran]

On Tue, Aug 13, 2024 at 12:56:00PM +0000, Maciek Machnikowski wrote:
> The Timex structure returned by the clock_adjtime() POSIX API allows
> the clock to return the estimated error. Implement getesterror
> and setesterror functions in the ptp_clock_info to enable drivers
> to interact with the hardware to get the error information.

So this can be implemented in the PTP class layer directly.  No need
for driver callbacks.
 
> getesterror additionally implements returning hw_ts and sys_ts
> to enable upper layers to estimate the maximum error of the clock
> based on the last time of correction.

But where are the upper layers?

> This functionality is not
> directly implemented in the clock_adjtime and will require
> a separate interface in the future.

We don't add interfaces that have no users.

Thanks,
Richard

Re: [RFC 1/3] ptp: Implement timex esterror support

[Maciek Machnikowski]

On 15/08/2024 06:23, Richard Cochran wrote:
> On Tue, Aug 13, 2024 at 12:56:00PM +0000, Maciek Machnikowski wrote:
>> The Timex structure returned by the clock_adjtime() POSIX API allows
>> the clock to return the estimated error. Implement getesterror
>> and setesterror functions in the ptp_clock_info to enable drivers
>> to interact with the hardware to get the error information.
> 
> So this can be implemented in the PTP class layer directly.  No need
> for driver callbacks.
Not if there is no connection between PTP layer and the consumer of this
information (TimeCard that uses PPS from embedded GNSS receiver, or a
PTP stack running on the infrastructure function of the multi-host
adapter or a DPU)

>  
>> getesterror additionally implements returning hw_ts and sys_ts
>> to enable upper layers to estimate the maximum error of the clock
>> based on the last time of correction.
> 
> But where are the upper layers?
They will be used to calculate the estimate of maxerror - don't want to
implement an API just to change it in a next RFC

> 
>> This functionality is not
>> directly implemented in the clock_adjtime and will require
>> a separate interface in the future.
> 
> We don't add interfaces that have no users.
> 
> Thanks,
> Richard

Noted - I'll try to explain it better next time

Re: [RFC 1/3] ptp: Implement timex esterror support

[Maciek Machnikowski]

On 15/08/2024 06:23, Richard Cochran wrote:
> On Tue, Aug 13, 2024 at 12:56:00PM +0000, Maciek Machnikowski wrote:
>> The Timex structure returned by the clock_adjtime() POSIX API allows
>> the clock to return the estimated error. Implement getesterror
>> and setesterror functions in the ptp_clock_info to enable drivers
>> to interact with the hardware to get the error information.
> 
> So this can be implemented in the PTP class layer directly.  No need
> for driver callbacks.
Sorry - missed that one in the previous answer.

I can implement a generic support in case all the information are
exchanged locally, but the driver hooks are needed for a use case of
separate function owning the synchronization, or an autonomous sync
devices (Time Cards). Would a fallback if getesterror==NULL be enough?

Regards,
Maciek

>  
>> getesterror additionally implements returning hw_ts and sys_ts
>> to enable upper layers to estimate the maximum error of the clock
>> based on the last time of correction.
> 
> But where are the upper layers?
> 
>> This functionality is not
>> directly implemented in the clock_adjtime and will require
>> a separate interface in the future.
> 
> We don't add interfaces that have no users.
> 
> Thanks,
> Richard

Re: [RFC 1/3] ptp: Implement timex esterror support

[Richard Cochran]

On Thu, Aug 15, 2024 at 11:47:45AM +0200, Maciek Machnikowski wrote:
> I can implement a generic support in case all the information are
> exchanged locally, but the driver hooks are needed for a use case of
> separate function owning the synchronization, or an autonomous sync
> devices (Time Cards). Would a fallback if getesterror==NULL be enough?

Well, you really need to show us how this will work with the Mythical
Time Cards!

Thanks,
Richard

[RFC 2/3] ptp: Implement support for esterror in ptp_mock

[Maciek Machnikowski]

Implement basic example of handling the esterror using
getesterror/setesterror functions of the ptp_clock_info

Signed-off-by: Maciek Machnikowski <maciek@machnikowski.net>
---
 drivers/ptp/ptp_mock.c | 30 ++++++++++++++++++++++++++++++
 1 file changed, 30 insertions(+)

diff --git a/drivers/ptp/ptp_mock.c b/drivers/ptp/ptp_mock.c
index e7b459c846a2..1786da790f82 100644
--- a/drivers/ptp/ptp_mock.c
+++ b/drivers/ptp/ptp_mock.c
@@ -35,10 +35,13 @@
 
 struct mock_phc {
 	struct ptp_clock_info info;
+	struct timespec64 sys_ts;
+	struct timespec64 hw_ts;
 	struct ptp_clock *clock;
 	struct timecounter tc;
 	struct cyclecounter cc;
 	spinlock_t lock;
+	long esterror;
 };
 
 static u64 mock_phc_cc_read(const struct cyclecounter *cc)
@@ -100,6 +103,31 @@ static int mock_phc_gettime64(struct ptp_clock_info *info, struct timespec64 *ts
 	return 0;
 }
 
+static int mock_phc_getesterror(struct ptp_clock_info *info, long *esterror,
+				struct timespec64 *hw_ts, struct timespec64 *sys_ts)
+{
+	struct mock_phc *phc = info_to_phc(info);
+
+	*esterror = phc->esterror;
+	if (hw_ts)
+		*hw_ts = phc->hw_ts;
+	if (sys_ts)
+		*sys_ts = phc->sys_ts;
+
+	return 0;
+}
+
+static int mock_phc_setesterror(struct ptp_clock_info *info, long esterror)
+{
+	struct mock_phc *phc = info_to_phc(info);
+
+	phc->esterror = esterror;
+	phc->hw_ts = ns_to_timespec64(timecounter_read(&phc->tc));
+	phc->sys_ts = ns_to_timespec64(ktime_get_raw_ns());
+
+	return 0;
+}
+
 static long mock_phc_refresh(struct ptp_clock_info *info)
 {
 	struct timespec64 ts;
@@ -134,6 +162,8 @@ struct mock_phc *mock_phc_create(struct device *dev)
 		.adjtime	= mock_phc_adjtime,
 		.gettime64	= mock_phc_gettime64,
 		.settime64	= mock_phc_settime64,
+		.getesterror	= mock_phc_getesterror,
+		.setesterror	= mock_phc_setesterror,
 		.do_aux_work	= mock_phc_refresh,
 	};
 
-- 
2.34.1

[RFC 3/3] ptp: Add setting esterror and reading timex structure

[Maciek Machnikowski]

Implement setting the esterror using clock_adjtime for ptp clocks
and reading the clock setting using timex structure and
clock_adjtime

Signed-off-by: Maciek Machnikowski <maciek@machnikowski.net>
---
 tools/testing/selftests/ptp/testptp.c | 39 +++++++++++++++++++++++++--
 1 file changed, 37 insertions(+), 2 deletions(-)

diff --git a/tools/testing/selftests/ptp/testptp.c b/tools/testing/selftests/ptp/testptp.c
index 011252fe238c..38405803b881 100644
--- a/tools/testing/selftests/ptp/testptp.c
+++ b/tools/testing/selftests/ptp/testptp.c
@@ -117,6 +117,7 @@ static void usage(char *progname)
 {
 	fprintf(stderr,
 		"usage: %s [options]\n"
+		" -a val     adjust the estimated error by 'val' ns\n"
 		" -c         query the ptp clock's capabilities\n"
 		" -d name    device to open\n"
 		" -e val     read 'val' external time stamp events\n"
@@ -140,6 +141,7 @@ static void usage(char *progname)
 		" -H val     set output phase to 'val' nanoseconds (requires -p)\n"
 		" -w val     set output pulse width to 'val' nanoseconds (requires -p)\n"
 		" -P val     enable or disable (val=1|0) the system clock PPS\n"
+		" -r         read clock info in the  timex structure using clock_adjtime\n"
 		" -s         set the ptp clock time from the system time\n"
 		" -S         set the system time from the ptp clock time\n"
 		" -t val     shift the ptp clock time by 'val' seconds\n"
@@ -175,12 +177,14 @@ int main(int argc, char *argv[])
 	int adjns = 0;
 	int adjphase = 0;
 	int capabilities = 0;
+	long esterror = 0;
 	int extts = 0;
 	int flagtest = 0;
 	int gettime = 0;
 	int index = 0;
 	int list_pins = 0;
 	int pct_offset = 0;
+	int readclk = 0;
 	int getextended = 0;
 	int getcross = 0;
 	int n_samples = 0;
@@ -198,8 +202,11 @@ int main(int argc, char *argv[])
 
 	progname = strrchr(argv[0], '/');
 	progname = progname ? 1+progname : argv[0];
-	while (EOF != (c = getopt(argc, argv, "cd:e:f:F:ghH:i:k:lL:n:o:p:P:sSt:T:w:x:Xz"))) {
+	while (EOF != (c = getopt(argc, argv, "a:cd:e:f:F:ghH:i:k:lL:n:o:p:P:rsSt:T:w:x:Xz"))) {
 		switch (c) {
+		case 'a':
+			esterror = atoi(optarg);
+			break;
 		case 'c':
 			capabilities = 1;
 			break;
@@ -250,6 +257,9 @@ int main(int argc, char *argv[])
 		case 'P':
 			pps = atoi(optarg);
 			break;
+		case 'r':
+			readclk = 1;
+			break;
 		case 's':
 			settime = 1;
 			break;
@@ -290,7 +300,6 @@ int main(int argc, char *argv[])
 			return -1;
 		}
 	}
-
 	fd = open(device, O_RDWR);
 	if (fd < 0) {
 		fprintf(stderr, "opening %s: %s\n", device, strerror(errno));
@@ -621,6 +630,32 @@ int main(int argc, char *argv[])
 		}
 	}
 
+	if (esterror) {
+		memset(&tx, 0, sizeof(tx));
+		tx.modes = ADJ_ESTERROR;
+		tx.esterror = esterror;
+		if (clock_adjtime(clkid, &tx))
+			perror("clock_adjtime");
+		else
+			puts("esterror adjustment okay");
+	}
+
+	if (readclk) {
+		struct timex clk_info = {0};
+
+		memset(&tx, 0, sizeof(tx));
+		if (clock_adjtime(clkid, &tx)) {
+			perror("clock_adjtime");
+		} else {
+			printf("clock_adjtime:\n"
+			       "\tstatus %d,\n"
+			       "\toffset %ld,\n"
+			       "\tfreq %ld,\n"
+			       "\testerror %ld\n",
+			       tx.status, tx.offset, tx.freq, tx.esterror);
+		}
+	}
+
 	close(fd);
 	return 0;
 }
-- 
2.34.1

Re: [RFC 0/3] ptp: Add esterror support

[Andrew Lunn]

On Tue, Aug 13, 2024 at 12:55:59PM +0000, Maciek Machnikowski wrote:
> This patch series implements handling of timex esterror field
> by ptp devices.
> 
> Esterror field can be used to return or set the estimated error
> of the clock. This is useful for devices containing a hardware
> clock that is controlled and synchronized internally (such as
> a time card) or when the synchronization is pushed to the embedded
> CPU of a DPU.

How can you set the estimated error of a clock? Isn't it a properties
of the hardware, and maybe the network link? A 10BaseT/Half duplex
link is going to have a bigger error than a 1000BaseT link because the
bits take longer on the wire etc.

What is the device supposed to do with the set value?

     Andrew

Re: [RFC 0/3] ptp: Add esterror support

[Vadim Fedorenko]

On 13/08/2024 21:05, Andrew Lunn wrote:
> On Tue, Aug 13, 2024 at 12:55:59PM +0000, Maciek Machnikowski wrote:
>> This patch series implements handling of timex esterror field
>> by ptp devices.
>>
>> Esterror field can be used to return or set the estimated error
>> of the clock. This is useful for devices containing a hardware
>> clock that is controlled and synchronized internally (such as
>> a time card) or when the synchronization is pushed to the embedded
>> CPU of a DPU.
> 
> How can you set the estimated error of a clock? Isn't it a properties
> of the hardware, and maybe the network link? A 10BaseT/Half duplex
> link is going to have a bigger error than a 1000BaseT link because the
> bits take longer on the wire etc.

AFAIU, it's in the spec of the hardware, but it can change depending on
the environment, like temperature. The link speed doesn't matter here,
this property can be used to calculate possible drift of the clock in
the micro holdover mode (between sync points).

> What is the device supposed to do with the set value?

It can be used to report the value back to user-space to calculate the
boundaries of "true time" returned by the hardware.

Thanks!

Re: [RFC 0/3] ptp: Add esterror support

[Andrew Lunn]

On Wed, Aug 14, 2024 at 09:44:29AM +0100, Vadim Fedorenko wrote:
> On 13/08/2024 21:05, Andrew Lunn wrote:
> > On Tue, Aug 13, 2024 at 12:55:59PM +0000, Maciek Machnikowski wrote:
> > > This patch series implements handling of timex esterror field
> > > by ptp devices.
> > > 
> > > Esterror field can be used to return or set the estimated error
> > > of the clock. This is useful for devices containing a hardware
> > > clock that is controlled and synchronized internally (such as
> > > a time card) or when the synchronization is pushed to the embedded
> > > CPU of a DPU.
> > 
> > How can you set the estimated error of a clock? Isn't it a properties
> > of the hardware, and maybe the network link? A 10BaseT/Half duplex
> > link is going to have a bigger error than a 1000BaseT link because the
> > bits take longer on the wire etc.
> 
> AFAIU, it's in the spec of the hardware, but it can change depending on
> the environment, like temperature. The link speed doesn't matter here,
> this property can be used to calculate possible drift of the clock in
> the micro holdover mode (between sync points).

Is there a clear definition then? Could you reference a standard
indicating what is included and excluded from this?

> > What is the device supposed to do with the set value?
> 
> It can be used to report the value back to user-space to calculate the
> boundaries of "true time" returned by the hardware.

So the driver itself does not know its own error? It has to be told
it, so it can report it back to user space. Then why bother, just put
it directly into the ptp4l configuration file?

Maybe this is all obvious to somebody who knows PTP inside out, but to
me it is not. Please could you put a better explanation and
justification into the commit message. We need PHY driver writers who
have limited idea about PTP can implement these new calls.

	Andrew

Re: [RFC 0/3] ptp: Add esterror support

[Maciek Machnikowski]

On 14/08/2024 15:08, Andrew Lunn wrote:
> On Wed, Aug 14, 2024 at 09:44:29AM +0100, Vadim Fedorenko wrote:
>> On 13/08/2024 21:05, Andrew Lunn wrote:
>>> On Tue, Aug 13, 2024 at 12:55:59PM +0000, Maciek Machnikowski wrote:
>>>> This patch series implements handling of timex esterror field
>>>> by ptp devices.
>>>>
>>>> Esterror field can be used to return or set the estimated error
>>>> of the clock. This is useful for devices containing a hardware
>>>> clock that is controlled and synchronized internally (such as
>>>> a time card) or when the synchronization is pushed to the embedded
>>>> CPU of a DPU.
>>>
>>> How can you set the estimated error of a clock? Isn't it a properties
>>> of the hardware, and maybe the network link? A 10BaseT/Half duplex
>>> link is going to have a bigger error than a 1000BaseT link because the
>>> bits take longer on the wire etc.
>>
>> AFAIU, it's in the spec of the hardware, but it can change depending on
>> the environment, like temperature. The link speed doesn't matter here,
>> this property can be used to calculate possible drift of the clock in
>> the micro holdover mode (between sync points).
> 
> Is there a clear definition then? Could you reference a standard
> indicating what is included and excluded from this?
> 
The esterror should return the error calculated by the device. There is
no standard defining this, but the simplest implementation can put the
offset calculated by the ptp daemon, or the offset to the nearest PPS in
cases where PPS is used as a source of time


>>> What is the device supposed to do with the set value?
>>
>> It can be used to report the value back to user-space to calculate the
>> boundaries of "true time" returned by the hardware.
> 
> So the driver itself does not know its own error? It has to be told
> it, so it can report it back to user space. Then why bother, just put
> it directly into the ptp4l configuration file?
> 
> Maybe this is all obvious to somebody who knows PTP inside out, but to
> me it is not. Please could you put a better explanation and
> justification into the commit message. We need PHY driver writers who
> have limited idea about PTP can implement these new calls.
>
> Andrew

It's designed to enable devices that either synchronize its own hw clock
to some source of time on a hardware layer (e.g. a Timecard that uses a
PPS signal from the GNSS), or a device in which the PTP is done on a
different function (multifunction NIC, or a DPU) to convey its best
estimate of the error (see above). In case of a multifunction NIC the
ptp daemon running on a function that synchronizes the clock will use
the ADJ_ESTERROR to push the error estimate to the device. The device
will then be able to
a. provide that information to hardware clocks on different functions
b. prevent time-dependent functionalities from acting when a clock
shifts beyond a predefined limit.

Also esterror is not maxerror and is not designed to include all errors,
but the best estimate

Thanks
Maciek

Re: [RFC 0/3] ptp: Add esterror support

[Richard Cochran]

On Wed, Aug 14, 2024 at 05:08:24PM +0200, Maciek Machnikowski wrote:

> The esterror should return the error calculated by the device. There is
> no standard defining this, but the simplest implementation can put the
> offset calculated by the ptp daemon, or the offset to the nearest PPS in
> cases where PPS is used as a source of time

So user space produces the number, and other user space consumes it?

Sounds like it should say in user space, shared over some IPC, like
PTP management messages for example.

> the ADJ_ESTERROR to push the error estimate to the device. The device
> will then be able to
> a. provide that information to hardware clocks on different functions

Not really, because there no in-kernel API for one device to query
another.  At least you didn't provide one.  Looks like you mean that
some user space program sets the value, and another program reads it
to share it with other devices?

> b. prevent time-dependent functionalities from acting when a clock
> shifts beyond a predefined limit.

This can be controlled by the user space stack.  For example, "if
estimated error exceeds threshold, disable PPS output".

Thanks,
Richard

Re: [RFC 0/3] ptp: Add esterror support

[Maciek Machnikowski]

On 15/08/2024 05:53, Richard Cochran wrote:
> On Wed, Aug 14, 2024 at 05:08:24PM +0200, Maciek Machnikowski wrote:
> 
>> The esterror should return the error calculated by the device. There is
>> no standard defining this, but the simplest implementation can put the
>> offset calculated by the ptp daemon, or the offset to the nearest PPS in
>> cases where PPS is used as a source of time
> 
> So user space produces the number, and other user space consumes it?
> 
> Sounds like it should say in user space, shared over some IPC, like
> PTP management messages for example.

The user spaces may run on completely isolated platforms in isolated
network with no direct path to communicate that.
I'm well aware of different solutions on the same platform (libpmc, AWS
Nitro or Clock Manager) , but this patchset tries to address different
use case
> 
>> the ADJ_ESTERROR to push the error estimate to the device. The device
>> will then be able to
>> a. provide that information to hardware clocks on different functions
> 
> Not really, because there no in-kernel API for one device to query
> another.  At least you didn't provide one.  Looks like you mean that
> some user space program sets the value, and another program reads it
> to share it with other devices?
> 
>> b. prevent time-dependent functionalities from acting when a clock
>> shifts beyond a predefined limit.
> 
> This can be controlled by the user space stack.  For example, "if
> estimated error exceeds threshold, disable PPS output".
> 
> Thanks,
> Richard
It's doable if everything runs on the same OS, but there are cases where
time sync is part of management and we will want to disable features on
different PFs/VFs as a result of bad time synchronizations. Such
features include launchtime or time-based congestion control which are
part of infrastructure.

Re: [RFC 0/3] ptp: Add esterror support

[Andrew Lunn]

On Thu, Aug 15, 2024 at 11:40:28AM +0200, Maciek Machnikowski wrote:
> 
> 
> On 15/08/2024 05:53, Richard Cochran wrote:
> > On Wed, Aug 14, 2024 at 05:08:24PM +0200, Maciek Machnikowski wrote:
> > 
> >> The esterror should return the error calculated by the device. There is
> >> no standard defining this, but the simplest implementation can put the
> >> offset calculated by the ptp daemon, or the offset to the nearest PPS in
> >> cases where PPS is used as a source of time
> > 
> > So user space produces the number, and other user space consumes it?
> > 
> > Sounds like it should say in user space, shared over some IPC, like
> > PTP management messages for example.
> 
> The user spaces may run on completely isolated platforms in isolated
> network with no direct path to communicate that.
> I'm well aware of different solutions on the same platform (libpmc, AWS
> Nitro or Clock Manager) , but this patchset tries to address different
> use case

So this in effect is just a communication mechanism between two user
space processes. The device itself does not know its own error, and
when told about its error, it does nothing. So why add new driver API
calls? It seems like the core should be able to handle this. You then
don't need a details explanation of the API which a PHY driver writer
can understand...

       Andrew

Re: [RFC 0/3] ptp: Add esterror support

[Maciek Machnikowski]

On 15/08/2024 16:26, Andrew Lunn wrote:
> On Thu, Aug 15, 2024 at 11:40:28AM +0200, Maciek Machnikowski wrote:
>>
>>
>> On 15/08/2024 05:53, Richard Cochran wrote:
>>> On Wed, Aug 14, 2024 at 05:08:24PM +0200, Maciek Machnikowski wrote:
>>>
>>>> The esterror should return the error calculated by the device. There is
>>>> no standard defining this, but the simplest implementation can put the
>>>> offset calculated by the ptp daemon, or the offset to the nearest PPS in
>>>> cases where PPS is used as a source of time
>>>
>>> So user space produces the number, and other user space consumes it?
>>>
>>> Sounds like it should say in user space, shared over some IPC, like
>>> PTP management messages for example.
>>
>> The user spaces may run on completely isolated platforms in isolated
>> network with no direct path to communicate that.
>> I'm well aware of different solutions on the same platform (libpmc, AWS
>> Nitro or Clock Manager) , but this patchset tries to address different
>> use case
> 
> So this in effect is just a communication mechanism between two user
> space processes. The device itself does not know its own error, and
> when told about its error, it does nothing. So why add new driver API
> calls? It seems like the core should be able to handle this. You then
> don't need a details explanation of the API which a PHY driver writer
> can understand...
> 
>        Andrew

No - it is not the main use case. The easiest one to understand would be
the following:

Think about a Time Card
(https://opencomputeproject.github.io/Time-Appliance-Project/docs/time-card/introduction).

It is a device that exposes the precise time to the user space using the
PTP subsystem, but it is an autonomous device and the synchronization is
implemented on the hardware layer.
In this case no user space process is now aware of what is the expected
estimated error, because that is only known to the HW and its control loop.
And this information is needed for the aforementioned userspace
processes to calculate error boundaries (time uncertaninty) of a given
clock.

-Maciek

Re: [RFC 0/3] ptp: Add esterror support

[Richard Cochran]

On Thu, Aug 15, 2024 at 05:00:24PM +0200, Maciek Machnikowski wrote:

> Think about a Time Card
> (https://opencomputeproject.github.io/Time-Appliance-Project/docs/time-card/introduction).

No, I won't think about that!

You need to present the technical details in the form of patches.

Hand-wavey hints don't cut it.

Thanks,
Richard

Re: [RFC 0/3] ptp: Add esterror support

[Maciek Machnikowski]

On 15/08/2024 23:08, Richard Cochran wrote:
> On Thu, Aug 15, 2024 at 05:00:24PM +0200, Maciek Machnikowski wrote:
> 
>> Think about a Time Card
>> (https://opencomputeproject.github.io/Time-Appliance-Project/docs/time-card/introduction).
> 
> No, I won't think about that!
> 
> You need to present the technical details in the form of patches.
> 
> Hand-wavey hints don't cut it.
> 
> Thanks,
> Richard

This implementation addresses 3 use cases:

1. Autonomous devices that synchronize themselves to some external
sources (GNSS, NTP, dedicated time sync networks) and have the ability
to return the estimated error from the HW or FW loop to users

2. Multi function devices that may have a single isolated function
synchronizing the device clock (by means of PTP, or PPS or any other)
and letting other functions access the uncertainty information

3. Create a common interface to read the uncertainty from a device
(currently you can use PMC for PTP, but there is no way of receiving
that information from ts2phc)

#1 and #2 requires an interface to the driver to retrieve the error from
a device
#2 requires an interface to adjust the esterror and push it to the device
#3 can be terminated locally in the ptp class driver using the same
principle as the presented ptp_mock implementation, or something more
sophisticated

Also this is an RFC to help align work on this functionality across
different devices ] and validate if that's the right direction. If it is
- there will be a patch series with real drivers returning uncertainty
information using that interface. If it's not - I'd like to understand
what should I improve in the interface.

HTH
Maciek

Re: [RFC 0/3] ptp: Add esterror support

[Andrew Lunn]

On Fri, Aug 16, 2024 at 12:06:51AM +0200, Maciek Machnikowski wrote:
> 
> 
> On 15/08/2024 23:08, Richard Cochran wrote:
> > On Thu, Aug 15, 2024 at 05:00:24PM +0200, Maciek Machnikowski wrote:
> > 
> >> Think about a Time Card
> >> (https://opencomputeproject.github.io/Time-Appliance-Project/docs/time-card/introduction).
> > 
> > No, I won't think about that!
> > 
> > You need to present the technical details in the form of patches.
> > 
> > Hand-wavey hints don't cut it.
> > 
> > Thanks,
> > Richard
> 
> This implementation addresses 3 use cases:
> 
> 1. Autonomous devices that synchronize themselves to some external
> sources (GNSS, NTP, dedicated time sync networks) and have the ability
> to return the estimated error from the HW or FW loop to users

So this contradicts what you said earlier, when you said the device
does not know its own error, it has to be told it.

So what is user space supposed to do with this error? And given that
you said it is undefined what this error includes and excludes, how is
user space supposed to deal with the error in the error? Given how
poorly this is defined, what is user space supposed to do when the
device changes the definition of the error?

The message Richard has always given is that those who care about
errors freeze their kernel and do measurement campaign to determine
what the real error is and then configure user space to deal with
it. Does this error value negate the need for this?

> 2. Multi function devices that may have a single isolated function
> synchronizing the device clock (by means of PTP, or PPS or any other)
> and letting other functions access the uncertainty information

So this is the simple message passing API, which could be implemented
purely in the core? This sounds like it should be a patch of its own,
explaining the use case. 

> 3. Create a common interface to read the uncertainty from a device
> (currently you can use PMC for PTP, but there is no way of receiving
> that information from ts2phc)

That sounds like a problem with ts2phc? Please could you expand on why
the kernel should be involved in feature deficits of user space tools?

> Also this is an RFC to help align work on this functionality across
> different devices ] and validate if that's the right direction. If it is
> - there will be a patch series with real drivers returning uncertainty
> information using that interface. If it's not - I'd like to understand
> what should I improve in the interface.

I think you took the wrong approach. You should first state in detail
the use cases. Then show how you solve each use cases, both the user
and kernel space parts, and include the needed changes to a real
device driver.

	Andrew

Re: [RFC 0/3] ptp: Add esterror support

[Maciek Machnikowski]

On 16/08/2024 01:11, Andrew Lunn wrote:
> On Fri, Aug 16, 2024 at 12:06:51AM +0200, Maciek Machnikowski wrote:
>>
>>
>> On 15/08/2024 23:08, Richard Cochran wrote:
>>> On Thu, Aug 15, 2024 at 05:00:24PM +0200, Maciek Machnikowski wrote:
>>>
>>>> Think about a Time Card
>>>> (https://opencomputeproject.github.io/Time-Appliance-Project/docs/time-card/introduction).
>>>
>>> No, I won't think about that!
>>>
>>> You need to present the technical details in the form of patches.
>>>
>>> Hand-wavey hints don't cut it.
>>>
>>> Thanks,
>>> Richard
>>
>> This implementation addresses 3 use cases:
>>
>> 1. Autonomous devices that synchronize themselves to some external
>> sources (GNSS, NTP, dedicated time sync networks) and have the ability
>> to return the estimated error from the HW or FW loop to users
> 
> So this contradicts what you said earlier, when you said the device
> does not know its own error, it has to be told it.

No - it’s a different type of device.

> So what is user space supposed to do with this error? And given that
> you said it is undefined what this error includes and excludes, how is
> user space supposed to deal with the error in the error? Given how
> poorly this is defined, what is user space supposed to do when the
> device changes the definition of the error?

Esterror returns the last error to the master clock that the device
synchronizes to.
In the case of PPS - is the last error registered on the top of the second.
In the case of PTP - the last error is calculated based on a transaction.

> The message Richard has always given is that those who care about
> errors freeze their kernel and do measurement campaign to determine
> what the real error is and then configure user space to deal with
> it. Does this error value negate the need for this?

AFIR, this comment was relevant to measuring errors coming from delays
inside the system.

>> 2. Multi function devices that may have a single isolated function
>> synchronizing the device clock (by means of PTP, or PPS or any other)
>> and letting other functions access the uncertainty information
> 
> So this is the simple message passing API, which could be implemented
> purely in the core? This sounds like it should be a patch of its own,
> explaining the use case. 

If functions are isolated then there is no path for passing the messages
other than through the device.
The trusted function that can control the clock will push the last error
and control the clock to synchronize it as best as it can, other
functions will get the time from the clock and additional info to
calculate the uncertainty.

>> 3. Create a common interface to read the uncertainty from a device
>> (currently you can use PMC for PTP, but there is no way of receiving
>> that information from ts2phc)
> 
> That sounds like a problem with ts2phc? Please could you expand on why
> the kernel should be involved in feature deficits of user space tools?

Not really. Why would all userspace processes need to understand what
synchronizes the time currently and talk to the relevant tool?
All it cares is what the time is and the primitives for calculating
error boundaries and understand if the clock is synchronized good enough
for a given application.

>> Also this is an RFC to help align work on this functionality across
>> different devices ] and validate if that's the right direction. If it is
>> - there will be a patch series with real drivers returning uncertainty
>> information using that interface. If it's not - I'd like to understand
>> what should I improve in the interface.
> 
> I think you took the wrong approach. You should first state in detail
> the use cases. Then show how you solve each use cases, both the user
> and kernel space parts, and include the needed changes to a real
> device driver.
> 
> 	Andrew

Also there is one more use case I missed in that summary:
4. Device need to consume the information about the uncertainty to act
upon it. This is the case when you want to allow certain features to
work only if error boundaries requirements are met. For example you
don't want to allow launch time to work when the error of the clock is
huge, as your packets will launch precisely at unprecise time.

The current adjtime() API call without any flags can fill the time of
day and the last frequency correction. At the same time, the API's timex
structure consists of many other helpful information that are not
populated for PTP hardware clocks. This information consist of esterror
field which is there since kernel 0.99.13k. I'm not inventing a new API,
just trying to add hooks to implement existing APIs inside the PTP
subsystem.

Re: [RFC 0/3] ptp: Add esterror support

[Richard Cochran]

On Fri, Aug 16, 2024 at 12:06:51AM +0200, Maciek Machnikowski wrote:

> Also this is an RFC to help align work on this functionality across
> different devices ] and validate if that's the right direction. If it is
> - there will be a patch series with real drivers returning uncertainty
> information using that interface.

Please post a real world example with those real drivers.  That will
help us understand what you are trying to accomplish.

Thanks,
Richard

Re: [RFC 0/3] ptp: Add esterror support

[Richard Cochran]

On Wed, Aug 14, 2024 at 03:08:07PM +0200, Andrew Lunn wrote:

> So the driver itself does not know its own error? It has to be told
> it, so it can report it back to user space. Then why bother, just put
> it directly into the ptp4l configuration file?

This way my first reaction as well.

Thanks,
Richard

Re: [RFC 0/3] ptp: Add esterror support

[Richard Cochran]

On Wed, Aug 14, 2024 at 08:33:26PM -0700, Richard Cochran wrote:
> On Wed, Aug 14, 2024 at 03:08:07PM +0200, Andrew Lunn wrote:
> 
> > So the driver itself does not know its own error? It has to be told
> > it, so it can report it back to user space. Then why bother, just put
> > it directly into the ptp4l configuration file?
> 
> This way my first reaction as well.

Actually, looking at the NTP code, we have:

void process_adjtimex_modes(const struct __kernel_timex *txc,)
{
	...
	if (txc->modes & ADJ_ESTERROR)
		time_esterror = txc->esterror;
	...
}

So I guess PHCs should also support setting this from user space?

adding CC: Miroslav

At least it would be consistent.


Thanks,
Richard

Re: [RFC 0/3] ptp: Add esterror support

[Miroslav Lichvar]

On Wed, Aug 14, 2024 at 09:16:12PM -0700, Richard Cochran wrote:
> Actually, looking at the NTP code, we have:
> 
> void process_adjtimex_modes(const struct __kernel_timex *txc,)
> {
> 	...
> 	if (txc->modes & ADJ_ESTERROR)
> 		time_esterror = txc->esterror;
> 	...
> }
> 
> So I guess PHCs should also support setting this from user space?

Yes, I'd like that very much. It would allow other applications to get
the estimated error of the clock they are using. Maxerror would be
nice too, even if it didn't increase automatically at 500 ppm as the
system clock. IIRC this was proposed before for the cross-timestamping
PHC between VM host and guests, but there wasn't much interest at the
time.

-- 
Miroslav Lichvar

Re: [RFC 0/3] ptp: Add esterror support

[Maciek Machnikowski]

On 15/08/2024 07:24, Miroslav Lichvar wrote:
> On Wed, Aug 14, 2024 at 09:16:12PM -0700, Richard Cochran wrote:
>> Actually, looking at the NTP code, we have:
>>
>> void process_adjtimex_modes(const struct __kernel_timex *txc,)
>> {
>> 	...
>> 	if (txc->modes & ADJ_ESTERROR)
>> 		time_esterror = txc->esterror;
>> 	...
>> }
>>
>> So I guess PHCs should also support setting this from user space?
> 
> Yes, I'd like that very much. It would allow other applications to get
> the estimated error of the clock they are using. Maxerror would be
> nice too, even if it didn't increase automatically at 500 ppm as the
> system clock. IIRC this was proposed before for the cross-timestamping
> PHC between VM host and guests, but there wasn't much interest at the
> time.
> 
Thanks, I plan to include maxerror later (not sure if it should be in
the same RFC, or a separate one)

Re: [RFC 0/3] ptp: Add esterror support

[Maciek Machnikowski]

On 15/08/2024 06:16, Richard Cochran wrote:
> On Wed, Aug 14, 2024 at 08:33:26PM -0700, Richard Cochran wrote:
>> On Wed, Aug 14, 2024 at 03:08:07PM +0200, Andrew Lunn wrote:
>>
>>> So the driver itself does not know its own error? It has to be told
>>> it, so it can report it back to user space. Then why bother, just put
>>> it directly into the ptp4l configuration file?
>>
>> This way my first reaction as well.
> 
> Actually, looking at the NTP code, we have:
> 
> void process_adjtimex_modes(const struct __kernel_timex *txc,)
> {
> 	...
> 	if (txc->modes & ADJ_ESTERROR)
> 		time_esterror = txc->esterror;
> 	...
> }
> 
> So I guess PHCs should also support setting this from user space?
> 
> adding CC: Miroslav
> 
> At least it would be consistent.
> 
> 
> Thanks,
> Richard

Yes! It's exactly what inspired this patchset :)

Re: [RFC 0/3] ptp: Add esterror support

[Jakub Kicinski]

On Tue, 13 Aug 2024 12:55:59 +0000 Maciek Machnikowski wrote:
> This patch series implements handling of timex esterror field
> by ptp devices.
> 
> Esterror field can be used to return or set the estimated error
> of the clock. This is useful for devices containing a hardware
> clock that is controlled and synchronized internally (such as
> a time card) or when the synchronization is pushed to the embedded
> CPU of a DPU.
> 
> Current implementation of ADJ_ESTERROR can enable pushing
> current offset of the clock calculated by a userspace app
> to the device, which can act upon this information by enabling
> or disabling time-related functions when certain boundaries
> are not met (eg. packet launchtime)

Please do CC people who are working on the VM / PTP / vdso thing,
and time people like tglx. And an implementation for a real device
would be nice to establish attention.

Re: [RFC 0/3] ptp: Add esterror support

[Richard Cochran]

On Wed, Aug 14, 2024 at 05:41:47PM -0700, Jakub Kicinski wrote:

> Please do CC people who are working on the VM / PTP / vdso thing,
> and time people like tglx.

and CC John Stultz please

Thanks,
Richard

Re: [RFC 0/3] ptp: Add esterror support

[Richard Cochran]

On Wed, Aug 14, 2024 at 08:42:57PM -0700, Richard Cochran wrote:
> and CC John Stultz please

and Miroslav.

Thanks,
Richard

Re: [RFC 0/3] ptp: Add esterror support

[Maciek Machnikowski]

On 15/08/2024 02:41, Jakub Kicinski wrote:
> On Tue, 13 Aug 2024 12:55:59 +0000 Maciek Machnikowski wrote:
>> This patch series implements handling of timex esterror field
>> by ptp devices.
>>
>> Esterror field can be used to return or set the estimated error
>> of the clock. This is useful for devices containing a hardware
>> clock that is controlled and synchronized internally (such as
>> a time card) or when the synchronization is pushed to the embedded
>> CPU of a DPU.
>>
>> Current implementation of ADJ_ESTERROR can enable pushing
>> current offset of the clock calculated by a userspace app
>> to the device, which can act upon this information by enabling
>> or disabling time-related functions when certain boundaries
>> are not met (eg. packet launchtime)
> 
> Please do CC people who are working on the VM / PTP / vdso thing,
> and time people like tglx. And an implementation for a real device
> would be nice to establish attention.

Noted - will CC in future releases.

AWS planned the implementation in ENA driver - will work with David on
making it part of the patchset once we "upgrade" from an RFC - but
wanted to discuss the approach first.

Re: [RFC 0/3] ptp: Add esterror support

[Vadim Fedorenko]

On 15/08/2024 10:35, Maciek Machnikowski wrote:
> > 
> 
> 
> On 15/08/2024 02:41, Jakub Kicinski wrote:
>> On Tue, 13 Aug 2024 12:55:59 +0000 Maciek Machnikowski wrote:
>>> This patch series implements handling of timex esterror field
>>> by ptp devices.
>>>
>>> Esterror field can be used to return or set the estimated error
>>> of the clock. This is useful for devices containing a hardware
>>> clock that is controlled and synchronized internally (such as
>>> a time card) or when the synchronization is pushed to the embedded
>>> CPU of a DPU.
>>>
>>> Current implementation of ADJ_ESTERROR can enable pushing
>>> current offset of the clock calculated by a userspace app
>>> to the device, which can act upon this information by enabling
>>> or disabling time-related functions when certain boundaries
>>> are not met (eg. packet launchtime)
>>
>> Please do CC people who are working on the VM / PTP / vdso thing,
>> and time people like tglx. And an implementation for a real device
>> would be nice to establish attention.
> 
> Noted - will CC in future releases.
> 
> AWS planned the implementation in ENA driver - will work with David on
> making it part of the patchset once we "upgrade" from an RFC - but
> wanted to discuss the approach first.

I can implement the interface for OCP TimeCard too, I think we have HW
information about it already.

RE: [RFC 0/3] ptp: Add esterror support

[Arinzon, David]

> >
> >
> > On 15/08/2024 02:41, Jakub Kicinski wrote:
> >> On Tue, 13 Aug 2024 12:55:59 +0000 Maciek Machnikowski wrote:
> >>> This patch series implements handling of timex esterror field by ptp
> >>> devices.
> >>>
> >>> Esterror field can be used to return or set the estimated error of
> >>> the clock. This is useful for devices containing a hardware clock
> >>> that is controlled and synchronized internally (such as a time card)
> >>> or when the synchronization is pushed to the embedded CPU of a DPU.
> >>>
> >>> Current implementation of ADJ_ESTERROR can enable pushing current
> >>> offset of the clock calculated by a userspace app to the device,
> >>> which can act upon this information by enabling or disabling
> >>> time-related functions when certain boundaries are not met (eg.
> >>> packet launchtime)
> >>
> >> Please do CC people who are working on the VM / PTP / vdso thing, and
> >> time people like tglx. And an implementation for a real device would
> >> be nice to establish attention.
> >
> > Noted - will CC in future releases.
> >
> > AWS planned the implementation in ENA driver - will work with David on
> > making it part of the patchset once we "upgrade" from an RFC - but
> > wanted to discuss the approach first.
> 
> I can implement the interface for OCP TimeCard too, I think we have HW
> information about it already.

CC Amit Bernstein (AWS, ENA driver).