| Message ID | 20240813125602.155827-2-maciek@machnikowski.net (mailing list archive) |
|---|---|
| State | RFC |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ptp: Add esterror support | expand |
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,
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; ...
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
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!
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, >
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
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
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
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,
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(-)