| Message ID | 20230523205440.326934-8-rrameshbabu@nvidia.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | ptp .adjphase cleanups | expand |
| Context | Check | Description |
|---|---|---|
| netdev/series_format | success | Posting correctly formatted |
| netdev/tree_selection | success | Clearly marked for net-next |
| netdev/fixes_present | success | Fixes tag not required for -next series |
| netdev/header_inline | success | No static functions without inline keyword in header files |
| netdev/build_32bit | success | Errors and warnings before: 8 this patch: 8 |
| netdev/cc_maintainers | success | CCed 2 of 2 maintainers |
| netdev/build_clang | success | Errors and warnings before: 8 this patch: 8 |
| netdev/verify_signedoff | success | Signed-off-by tag matches author and committer |
| netdev/deprecated_api | success | None detected |
| netdev/check_selftest | success | No net selftest shell script |
| netdev/verify_fixes | success | No Fixes tag |
| netdev/build_allmodconfig_warn | success | Errors and warnings before: 8 this patch: 8 |
| netdev/checkpatch | success | total: 0 errors, 0 warnings, 0 checks, 77 lines checked |
| netdev/kdoc | success | Errors and warnings before: 0 this patch: 0 |
| netdev/source_inline | success | Was 0 now: 0 |
On Tue, 2023-05-23 at 13:54 -0700, Rahul Rameshbabu wrote: > Advertise the maximum offset the .adjphase callback is capable of > supporting in nanoseconds for IDT ClockMatrix devices. > > Cc: Richard Cochran <richardcochran@gmail.com> > Cc: Vincent Cheng <vincent.cheng.xh@renesas.com> > Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com> > --- > drivers/ptp/ptp_clockmatrix.c | 36 +++++++++++++++++------------------ > drivers/ptp/ptp_clockmatrix.h | 2 +- > 2 files changed, 18 insertions(+), 20 deletions(-) > > diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c > index c9d451bf89e2..f6f9d4adce04 100644 > --- a/drivers/ptp/ptp_clockmatrix.c > +++ b/drivers/ptp/ptp_clockmatrix.c > @@ -1692,14 +1692,23 @@ static int initialize_dco_operating_mode(struct idtcm_channel *channel) > /* PTP Hardware Clock interface */ > > /* > - * Maximum absolute value for write phase offset in picoseconds > - * > - * @channel: channel > - * @delta_ns: delta in nanoseconds > + * Maximum absolute value for write phase offset in nanoseconds > * > * Destination signed register is 32-bit register in resolution of 50ps > * > - * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 > + * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 ps > + * Represent 107374182350 ps as 107374182 ns > + */ > +static s32 idtcm_getmaxphase(struct ptp_clock_info *ptp __always_unused) > +{ > + return MAX_ABS_WRITE_PHASE_NANOSECONDS; > +} This introduces a functional change WRT the current code. Prior to this patch ClockMatrix tries to adjust phase delta even above MAX_ABS_WRITE_PHASE_NANOSECONDS, limiting the delta to such value. After this patch it will error out. Perhaps a more conservative approach would be keeping the existing logic in _idtcm_adjphase and let idtcm_getmaxphase return S32_MAX? Note that even that will error out for delta == S32_MIN so perhaps an API change to allow the driver specify unlimited delta would be useful (possibly regardless of the above). Cheers, Paolo
On Thu, 2023-05-25 at 14:08 +0200, Paolo Abeni wrote: > On Tue, 2023-05-23 at 13:54 -0700, Rahul Rameshbabu wrote: > > Advertise the maximum offset the .adjphase callback is capable of > > supporting in nanoseconds for IDT ClockMatrix devices. > > > > Cc: Richard Cochran <richardcochran@gmail.com> > > Cc: Vincent Cheng <vincent.cheng.xh@renesas.com> > > Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com> > > --- > > drivers/ptp/ptp_clockmatrix.c | 36 +++++++++++++++++------------------ > > drivers/ptp/ptp_clockmatrix.h | 2 +- > > 2 files changed, 18 insertions(+), 20 deletions(-) > > > > diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c > > index c9d451bf89e2..f6f9d4adce04 100644 > > --- a/drivers/ptp/ptp_clockmatrix.c > > +++ b/drivers/ptp/ptp_clockmatrix.c > > @@ -1692,14 +1692,23 @@ static int initialize_dco_operating_mode(struct idtcm_channel *channel) > > /* PTP Hardware Clock interface */ > > > > /* > > - * Maximum absolute value for write phase offset in picoseconds > > - * > > - * @channel: channel > > - * @delta_ns: delta in nanoseconds > > + * Maximum absolute value for write phase offset in nanoseconds > > * > > * Destination signed register is 32-bit register in resolution of 50ps > > * > > - * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 > > + * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 ps > > + * Represent 107374182350 ps as 107374182 ns > > + */ > > +static s32 idtcm_getmaxphase(struct ptp_clock_info *ptp __always_unused) > > +{ > > + return MAX_ABS_WRITE_PHASE_NANOSECONDS; > > +} > > This introduces a functional change WRT the current code. Prior to this > patch ClockMatrix tries to adjust phase delta even above > MAX_ABS_WRITE_PHASE_NANOSECONDS, limiting the delta to such value. > After this patch it will error out. > > Perhaps a more conservative approach would be keeping the existing > logic in _idtcm_adjphase and let idtcm_getmaxphase return > S32_MAX? > > Note that even that will error out for delta == S32_MIN so perhaps an > API change to allow the driver specify unlimited delta would be useful > (possibly regardless of the above). What about allowing drivers with no getmaxphase() callback, meaning such drivers allow adjusting unlimited phase delta? Thanks! Paolo
On Thu, 25 May, 2023 14:11:51 +0200 Paolo Abeni <pabeni@redhat.com> wrote: > On Thu, 2023-05-25 at 14:08 +0200, Paolo Abeni wrote: >> On Tue, 2023-05-23 at 13:54 -0700, Rahul Rameshbabu wrote: >> > Advertise the maximum offset the .adjphase callback is capable of >> > supporting in nanoseconds for IDT ClockMatrix devices. >> > >> > Cc: Richard Cochran <richardcochran@gmail.com> >> > Cc: Vincent Cheng <vincent.cheng.xh@renesas.com> >> > Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com> >> > --- >> > drivers/ptp/ptp_clockmatrix.c | 36 +++++++++++++++++------------------ >> > drivers/ptp/ptp_clockmatrix.h | 2 +- >> > 2 files changed, 18 insertions(+), 20 deletions(-) >> > >> > diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c >> > index c9d451bf89e2..f6f9d4adce04 100644 >> > --- a/drivers/ptp/ptp_clockmatrix.c >> > +++ b/drivers/ptp/ptp_clockmatrix.c >> > @@ -1692,14 +1692,23 @@ static int initialize_dco_operating_mode(struct idtcm_channel *channel) >> > /* PTP Hardware Clock interface */ >> > >> > /* >> > - * Maximum absolute value for write phase offset in picoseconds >> > - * >> > - * @channel: channel >> > - * @delta_ns: delta in nanoseconds >> > + * Maximum absolute value for write phase offset in nanoseconds >> > * >> > * Destination signed register is 32-bit register in resolution of 50ps >> > * >> > - * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 >> > + * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 ps >> > + * Represent 107374182350 ps as 107374182 ns >> > + */ >> > +static s32 idtcm_getmaxphase(struct ptp_clock_info *ptp __always_unused) >> > +{ >> > + return MAX_ABS_WRITE_PHASE_NANOSECONDS; >> > +} >> >> This introduces a functional change WRT the current code. Prior to this >> patch ClockMatrix tries to adjust phase delta even above >> MAX_ABS_WRITE_PHASE_NANOSECONDS, limiting the delta to such value. >> After this patch it will error out. My understanding is the syscall for adjphase, clock_adjtime, cannot represent an offset granularity smaller than nanoseconds using the struct timex offset member. To me, it seems that adjusting a delta above MAX_ABS_WRITE_PHASE_NANOSECONDS (due to support for higher precision units by the device), while supported by the device driver, would not be a capability utilized by any interface that would invoke the .adjphase callback implemented by ClockMatrix. The parameter doc comments even describe the delta provided is in nanoseconds, which is why the parameter was named delta_ns. Therefore, the increased precision in ps is lost either way. >> >> Perhaps a more conservative approach would be keeping the existing >> logic in _idtcm_adjphase and let idtcm_getmaxphase return >> S32_MAX? I personally do not like the idea of a device driver circumventing the PTP core stack for the check and implementing its own check. I can understand this choice potentially if the precision supported that is greater than nanosecond representation was utilized. I think this will depend on the outcome of the discussion of the previous point. >> >> Note that even that will error out for delta == S32_MIN so perhaps an >> API change to allow the driver specify unlimited delta would be useful >> (possibly regardless of the above). > > What about allowing drivers with no getmaxphase() callback, meaning > such drivers allow adjusting unlimited phase delta? I think this relates to the idea that even with "unlimited" adjustment support, the driver is still bound by the parameter value range for the .adjphase interface. Therefore, there really is not a way to support "unlimited" delta per-say. I understand the argument that the interface + check in the ptp core stack will limit the adjustment range to be [S32_MIN + 1, S32_MAX - 1] at most rather than [S32_MIN, S32_MAX]. However, I feel that if such large offset adjustments are needed, a difference of one nanosecond in either extreme is not a large loss. The reason I wanted to enforce device drivers to implement .getmaxphase was to discourage/avoid drivers from implementing their own range checks in .adjphase since there is a core check in the ptp_clock_adjtime function invoked when userspace calls the clock_adjtime syscall for the ADJ_OFFSET operation. Maybe this is just something to be discussed during each code review instead when implementers publish support to the mailing list? > > Thanks! > > Paolo Thanks for the feedback, Rahul Rameshbabu
Hi Paolo, Any comments on this follow-up? On Thu, 25 May, 2023 11:09:17 -0700 Rahul Rameshbabu <rrameshbabu@nvidia.com> wrote: > On Thu, 25 May, 2023 14:11:51 +0200 Paolo Abeni <pabeni@redhat.com> wrote: >> On Thu, 2023-05-25 at 14:08 +0200, Paolo Abeni wrote: >>> On Tue, 2023-05-23 at 13:54 -0700, Rahul Rameshbabu wrote: >>> > Advertise the maximum offset the .adjphase callback is capable of >>> > supporting in nanoseconds for IDT ClockMatrix devices. >>> > >>> > Cc: Richard Cochran <richardcochran@gmail.com> >>> > Cc: Vincent Cheng <vincent.cheng.xh@renesas.com> >>> > Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com> >>> > --- >>> > drivers/ptp/ptp_clockmatrix.c | 36 +++++++++++++++++------------------ >>> > drivers/ptp/ptp_clockmatrix.h | 2 +- >>> > 2 files changed, 18 insertions(+), 20 deletions(-) >>> > >>> > diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c >>> > index c9d451bf89e2..f6f9d4adce04 100644 >>> > --- a/drivers/ptp/ptp_clockmatrix.c >>> > +++ b/drivers/ptp/ptp_clockmatrix.c >>> > @@ -1692,14 +1692,23 @@ static int initialize_dco_operating_mode(struct idtcm_channel *channel) >>> > /* PTP Hardware Clock interface */ >>> > >>> > /* >>> > - * Maximum absolute value for write phase offset in picoseconds >>> > - * >>> > - * @channel: channel >>> > - * @delta_ns: delta in nanoseconds >>> > + * Maximum absolute value for write phase offset in nanoseconds >>> > * >>> > * Destination signed register is 32-bit register in resolution of 50ps >>> > * >>> > - * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 >>> > + * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 ps >>> > + * Represent 107374182350 ps as 107374182 ns >>> > + */ >>> > +static s32 idtcm_getmaxphase(struct ptp_clock_info *ptp __always_unused) >>> > +{ >>> > + return MAX_ABS_WRITE_PHASE_NANOSECONDS; >>> > +} >>> >>> This introduces a functional change WRT the current code. Prior to this >>> patch ClockMatrix tries to adjust phase delta even above >>> MAX_ABS_WRITE_PHASE_NANOSECONDS, limiting the delta to such value. >>> After this patch it will error out. > > My understanding is the syscall for adjphase, clock_adjtime, cannot > represent an offset granularity smaller than nanoseconds using the > struct timex offset member. To me, it seems that adjusting a delta above > MAX_ABS_WRITE_PHASE_NANOSECONDS (due to support for higher precision > units by the device), while supported by the device driver, would not be > a capability utilized by any interface that would invoke the .adjphase > callback implemented by ClockMatrix. The parameter doc comments even > describe the delta provided is in nanoseconds, which is why the > parameter was named delta_ns. Therefore, the increased precision in ps > is lost either way. > >>> >>> Perhaps a more conservative approach would be keeping the existing >>> logic in _idtcm_adjphase and let idtcm_getmaxphase return >>> S32_MAX? > > I personally do not like the idea of a device driver circumventing the > PTP core stack for the check and implementing its own check. I can > understand this choice potentially if the precision supported that is > greater than nanosecond representation was utilized. I think this will > depend on the outcome of the discussion of the previous point. > >>> >>> Note that even that will error out for delta == S32_MIN so perhaps an >>> API change to allow the driver specify unlimited delta would be useful >>> (possibly regardless of the above). >> >> What about allowing drivers with no getmaxphase() callback, meaning >> such drivers allow adjusting unlimited phase delta? > > I think this relates to the idea that even with "unlimited" adjustment > support, the driver is still bound by the parameter value range for the > .adjphase interface. Therefore, there really is not a way to support > "unlimited" delta per-say. I understand the argument that the interface > + check in the ptp core stack will limit the adjustment range to be > [S32_MIN + 1, S32_MAX - 1] at most rather than [S32_MIN, S32_MAX]. > However, I feel that if such large offset adjustments are needed, a > difference of one nanosecond in either extreme is not a large loss. > > The reason I wanted to enforce device drivers to implement .getmaxphase > was to discourage/avoid drivers from implementing their own range checks > in .adjphase since there is a core check in the ptp_clock_adjtime > function invoked when userspace calls the clock_adjtime syscall for the > ADJ_OFFSET operation. Maybe this is just something to be discussed > during each code review instead when implementers publish support to the > mailing list? > I am pretty open to revising this, but I wanted to know if your opinion is still the same based on this response I provided. >> >> Thanks! >> >> Paolo > > Thanks for the feedback, > > Rahul Rameshbabu Thanks, -- Rahul Rameshbabu
Hi, I'm sorry for the late reply. This fell under my radar. On Thu, 2023-05-25 at 11:09 -0700, Rahul Rameshbabu wrote: > On Thu, 25 May, 2023 14:11:51 +0200 Paolo Abeni <pabeni@redhat.com> wrote: > > On Thu, 2023-05-25 at 14:08 +0200, Paolo Abeni wrote: > > > > diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c > > > > index c9d451bf89e2..f6f9d4adce04 100644 > > > > --- a/drivers/ptp/ptp_clockmatrix.c > > > > +++ b/drivers/ptp/ptp_clockmatrix.c > > > > @@ -1692,14 +1692,23 @@ static int initialize_dco_operating_mode(struct idtcm_channel *channel) > > > > /* PTP Hardware Clock interface */ > > > > > > > > /* > > > > - * Maximum absolute value for write phase offset in picoseconds > > > > - * > > > > - * @channel: channel > > > > - * @delta_ns: delta in nanoseconds > > > > + * Maximum absolute value for write phase offset in nanoseconds > > > > * > > > > * Destination signed register is 32-bit register in resolution of 50ps > > > > * > > > > - * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 > > > > + * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 ps > > > > + * Represent 107374182350 ps as 107374182 ns > > > > + */ > > > > +static s32 idtcm_getmaxphase(struct ptp_clock_info *ptp __always_unused) > > > > +{ > > > > + return MAX_ABS_WRITE_PHASE_NANOSECONDS; > > > > +} > > > > > > This introduces a functional change WRT the current code. Prior to this > > > patch ClockMatrix tries to adjust phase delta even above > > > MAX_ABS_WRITE_PHASE_NANOSECONDS, limiting the delta to such value. > > > After this patch it will error out. > > My understanding is the syscall for adjphase, clock_adjtime, cannot > represent an offset granularity smaller than nanoseconds using the > struct timex offset member. Ok. > To me, it seems that adjusting a delta above > MAX_ABS_WRITE_PHASE_NANOSECONDS (due to support for higher precision > units by the device), while supported by the device driver, would not be > a capability utilized by any interface that would invoke the .adjphase > callback implemented by ClockMatrix. Here I don't follow. I must admit I know the ptp subsystem very little, but AFAICS, we could have e.g. clock_adjtime() // offset > 200 secs (200000000 usec) -> do_clock_adjtime -> kc->clock_adj -> clock_posix_dynamic -> pc_clock_adjtime -> ptp_clock_adjtime -> _idtcm_adjphase // delta land unmodified up here I guess the user-space could pass such large delta (e.g. at boot time?!?). If so, with this patch we change an user-space observable behavior, and I think we should avoid that. Thanks Paolo
On Fri, 09 Jun, 2023 08:38:11 +0200 Paolo Abeni <pabeni@redhat.com> wrote: > Hi, > > I'm sorry for the late reply. This fell under my radar. > > On Thu, 2023-05-25 at 11:09 -0700, Rahul Rameshbabu wrote: >> On Thu, 25 May, 2023 14:11:51 +0200 Paolo Abeni <pabeni@redhat.com> wrote: >> > On Thu, 2023-05-25 at 14:08 +0200, Paolo Abeni wrote: >> > > > diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c >> > > > index c9d451bf89e2..f6f9d4adce04 100644 >> > > > --- a/drivers/ptp/ptp_clockmatrix.c >> > > > +++ b/drivers/ptp/ptp_clockmatrix.c >> > > > @@ -1692,14 +1692,23 @@ static int initialize_dco_operating_mode(struct idtcm_channel *channel) >> > > > /* PTP Hardware Clock interface */ >> > > > >> > > > /* >> > > > - * Maximum absolute value for write phase offset in picoseconds >> > > > - * >> > > > - * @channel: channel >> > > > - * @delta_ns: delta in nanoseconds >> > > > + * Maximum absolute value for write phase offset in nanoseconds >> > > > * >> > > > * Destination signed register is 32-bit register in resolution of 50ps >> > > > * >> > > > - * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 >> > > > + * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 ps >> > > > + * Represent 107374182350 ps as 107374182 ns >> > > > + */ >> > > > +static s32 idtcm_getmaxphase(struct ptp_clock_info *ptp __always_unused) >> > > > +{ >> > > > + return MAX_ABS_WRITE_PHASE_NANOSECONDS; >> > > > +} >> > > >> > > This introduces a functional change WRT the current code. Prior to this >> > > patch ClockMatrix tries to adjust phase delta even above >> > > MAX_ABS_WRITE_PHASE_NANOSECONDS, limiting the delta to such value. >> > > After this patch it will error out. >> >> My understanding is the syscall for adjphase, clock_adjtime, cannot >> represent an offset granularity smaller than nanoseconds using the >> struct timex offset member. > > Ok. > >> To me, it seems that adjusting a delta above >> MAX_ABS_WRITE_PHASE_NANOSECONDS (due to support for higher precision >> units by the device), while supported by the device driver, would not be >> a capability utilized by any interface that would invoke the .adjphase >> callback implemented by ClockMatrix. I see I caused some confusion in terms of what I was focused on with this response. My main concern here was still about supporting precision units higher than nanoseconds. For example if a device was capable of supporting 107374182350 picoseconds for ADJ_OFFSET, it doesn't matter whether the driver advertises 107374182 nanoseconds as the maximum adjustment capability versus 107374182350 picoseconds even though 107374182 nanoseconds < 107374182350 picoseconds because the granularity of the parameter for the adjphase callback is in nanoseconds. I think we have converged on this topic but not the other point you brought up. > > Here I don't follow. I must admit I know the ptp subsystem very little, > but AFAICS, we could have e.g. > > clock_adjtime() // offset > 200 secs (200000000 usec) > -> do_clock_adjtime > -> kc->clock_adj > -> clock_posix_dynamic > -> pc_clock_adjtime > -> ptp_clock_adjtime > -> _idtcm_adjphase // delta land unmodified up here > > I guess the user-space could pass such large delta (e.g. at boot > time?!?). If so, with this patch we change an user-space observable > behavior, and I think we should avoid that. The point that you bring up here is about clamping (which is done by idtcm_adjphase previously) versus throwing an error when out of range (what is now done in ptp_clock_adjtime in this patch series). This was something I was struggling with deciding on a unified behavior across all drivers. For example, the mlx5_core driver chooses to return -ERANGE when the delta landed on it is out of the range supported by the PHC of the device. We chose to return an error because there was no mechanism previously for the userspace to know what was the supported offset when using ADJ_OFFSET with different PHC devices. If a user provides an offset and no error is returned, the user would assume that offset had been applied (there was no way to know that it was clamped from the userspace). This patch series now adds the query for maximum supported offset in the PTP_CLOCK_GETCAPS ioctl. In my opinion, I think we will see an userspace observable behavior change either way unfortunately due to the inconsistency among device drivers, which was one of the main issues this patch submission targets. I am ok with making the common behavior in ptp_clock_adjtime clamp the provided offset value instead of throwing an error when out of range. In both cases, userspace programs can handle the out-of-range case explicitly with a check against the maximum offset value now advertised in PTP_CLOCK_GETCAPS. My personal opinion is that since we have this inconsistency among device drivers for handling out of range offsets that are currently provided as-is to the driver-specific callback implementations, it makes sense to converge to a version that returns an error when the userspace provides out-of-range values rather than silently clamping these values. However, I am open to either version as long as we have consistency and do not leave this up to individual device-drivers to dictate since this adds further complexity in the userspace when working with this syscall. > > Thanks > > Paolo Thanks, Rahul Rameshbabu
> -----Original Message----- > From: Rahul Rameshbabu <rrameshbabu@nvidia.com> > Sent: Friday, June 9, 2023 12:48 PM > To: Paolo Abeni <pabeni@redhat.com> > Cc: netdev@vger.kernel.org; David S. Miller <davem@davemloft.net>; Keller, > Jacob E <jacob.e.keller@intel.com>; Gal Pressman <gal@nvidia.com>; Tariq > Toukan <tariqt@nvidia.com>; Saeed Mahameed <saeed@kernel.org>; Richard > Cochran <richardcochran@gmail.com>; Vincent Cheng > <vincent.cheng.xh@renesas.com> > Subject: Re: [PATCH net-next v2 7/9] ptp: ptp_clockmatrix: Add .getmaxphase > ptp_clock_info callback > > On Fri, 09 Jun, 2023 08:38:11 +0200 Paolo Abeni <pabeni@redhat.com> wrote: > > Hi, > > > > I'm sorry for the late reply. This fell under my radar. > > > > On Thu, 2023-05-25 at 11:09 -0700, Rahul Rameshbabu wrote: > >> On Thu, 25 May, 2023 14:11:51 +0200 Paolo Abeni <pabeni@redhat.com> > wrote: > >> > On Thu, 2023-05-25 at 14:08 +0200, Paolo Abeni wrote: > >> > > > diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c > >> > > > index c9d451bf89e2..f6f9d4adce04 100644 > >> > > > --- a/drivers/ptp/ptp_clockmatrix.c > >> > > > +++ b/drivers/ptp/ptp_clockmatrix.c > >> > > > @@ -1692,14 +1692,23 @@ static int > initialize_dco_operating_mode(struct idtcm_channel *channel) > >> > > > /* PTP Hardware Clock interface */ > >> > > > > >> > > > /* > >> > > > - * Maximum absolute value for write phase offset in picoseconds > >> > > > - * > >> > > > - * @channel: channel > >> > > > - * @delta_ns: delta in nanoseconds > >> > > > + * Maximum absolute value for write phase offset in nanoseconds > >> > > > * > >> > > > * Destination signed register is 32-bit register in resolution of 50ps > >> > > > * > >> > > > - * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 > >> > > > + * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 ps > >> > > > + * Represent 107374182350 ps as 107374182 ns > >> > > > + */ > >> > > > +static s32 idtcm_getmaxphase(struct ptp_clock_info *ptp > __always_unused) > >> > > > +{ > >> > > > + return MAX_ABS_WRITE_PHASE_NANOSECONDS; > >> > > > +} > >> > > > >> > > This introduces a functional change WRT the current code. Prior to this > >> > > patch ClockMatrix tries to adjust phase delta even above > >> > > MAX_ABS_WRITE_PHASE_NANOSECONDS, limiting the delta to such value. > >> > > After this patch it will error out. > >> > >> My understanding is the syscall for adjphase, clock_adjtime, cannot > >> represent an offset granularity smaller than nanoseconds using the > >> struct timex offset member. > > > > Ok. > > > >> To me, it seems that adjusting a delta above > >> MAX_ABS_WRITE_PHASE_NANOSECONDS (due to support for higher precision > >> units by the device), while supported by the device driver, would not be > >> a capability utilized by any interface that would invoke the .adjphase > >> callback implemented by ClockMatrix. > > I see I caused some confusion in terms of what I was focused on with > this response. My main concern here was still about supporting precision > units higher than nanoseconds. For example if a device was capable of > supporting 107374182350 picoseconds for ADJ_OFFSET, it doesn't matter > whether the driver advertises 107374182 nanoseconds as the maximum > adjustment capability versus 107374182350 picoseconds even though > 107374182 nanoseconds < 107374182350 picoseconds because the granularity > of the parameter for the adjphase callback is in nanoseconds. I think we > have converged on this topic but not the other point you brought up. > > > > > Here I don't follow. I must admit I know the ptp subsystem very little, > > but AFAICS, we could have e.g. > > > > clock_adjtime() // offset > 200 secs (200000000 usec) > > -> do_clock_adjtime > > -> kc->clock_adj > > -> clock_posix_dynamic > > -> pc_clock_adjtime > > -> ptp_clock_adjtime > > -> _idtcm_adjphase // delta land unmodified up here > > > > I guess the user-space could pass such large delta (e.g. at boot > > time?!?). If so, with this patch we change an user-space observable > > behavior, and I think we should avoid that. > > The point that you bring up here is about clamping (which is done by > idtcm_adjphase previously) versus throwing an error when out of range > (what is now done in ptp_clock_adjtime in this patch series). This was > something I was struggling with deciding on a unified behavior across > all drivers. For example, the mlx5_core driver chooses to return -ERANGE > when the delta landed on it is out of the range supported by the PHC of > the device. We chose to return an error because there was no mechanism > previously for the userspace to know what was the supported offset when > using ADJ_OFFSET with different PHC devices. If a user provides an > offset and no error is returned, the user would assume that offset had > been applied (there was no way to know that it was clamped from the > userspace). This patch series now adds the query for maximum supported > offset in the PTP_CLOCK_GETCAPS ioctl. In my opinion, I think we will > see an userspace observable behavior change either way unfortunately due > to the inconsistency among device drivers, which was one of the main > issues this patch submission targets. I am ok with making the common > behavior in ptp_clock_adjtime clamp the provided offset value instead of > throwing an error when out of range. In both cases, userspace programs > can handle the out-of-range case explicitly with a check against the > maximum offset value now advertised in PTP_CLOCK_GETCAPS. My personal > opinion is that since we have this inconsistency among device drivers > for handling out of range offsets that are currently provided as-is to > the driver-specific callback implementations, it makes sense to converge > to a version that returns an error when the userspace provides > out-of-range values rather than silently clamping these values. However, > I am open to either version as long as we have consistency and do not > leave this up to individual device-drivers to dictate since this adds > further complexity in the userspace when working with this syscall. > I'm in favor of throwing an error, since userspace that *doesn't* check for the max value and assumes it will apply without a clamp may be surprised when it starts clamping. Userspace which previously supplied a large value and it clamps now gets an error, which might be concerning, but they got driver defined behavior before, where it might error or it might clamp, so I think we're in a no-win scenario there. I don't really see the value in clamping because that makes it hard to tell if an update was fully applied or not. Now software has to know to check the range in advance. I wouldn't view a partially applied update as a successful behavior in a timing application. Thus, on the principle of least surprise I would avoid clamping. I'm open to other opinions, and I think standardizing is much better than letting it be driver behavior. > > > > Thanks > > > > Paolo > > Thanks, > > Rahul Rameshbabu
On Mon, 2023-06-12 at 05:16 +0000, Keller, Jacob E wrote: > > -----Original Message----- > > From: Rahul Rameshbabu <rrameshbabu@nvidia.com> > > Sent: Friday, June 9, 2023 12:48 PM > > To: Paolo Abeni <pabeni@redhat.com> > > Cc: netdev@vger.kernel.org; David S. Miller <davem@davemloft.net>; Keller, > > Jacob E <jacob.e.keller@intel.com>; Gal Pressman <gal@nvidia.com>; Tariq > > Toukan <tariqt@nvidia.com>; Saeed Mahameed <saeed@kernel.org>; Richard > > Cochran <richardcochran@gmail.com>; Vincent Cheng > > <vincent.cheng.xh@renesas.com> > > Subject: Re: [PATCH net-next v2 7/9] ptp: ptp_clockmatrix: Add .getmaxphase > > ptp_clock_info callback > > > > On Fri, 09 Jun, 2023 08:38:11 +0200 Paolo Abeni <pabeni@redhat.com> wrote: > > > On Thu, 2023-05-25 at 11:09 -0700, Rahul Rameshbabu wrote: > > > > On Thu, 25 May, 2023 14:11:51 +0200 Paolo Abeni <pabeni@redhat.com> wrote: > > > > > > > > > > I guess the user-space could pass such large delta (e.g. at boot > > > time?!?). If so, with this patch we change an user-space observable > > > behavior, and I think we should avoid that. > > > > The point that you bring up here is about clamping (which is done by > > idtcm_adjphase previously) versus throwing an error when out of range > > (what is now done in ptp_clock_adjtime in this patch series). This was > > something I was struggling with deciding on a unified behavior across > > all drivers. For example, the mlx5_core driver chooses to return -ERANGE > > when the delta landed on it is out of the range supported by the PHC of > > the device. We chose to return an error because there was no mechanism > > previously for the userspace to know what was the supported offset when > > using ADJ_OFFSET with different PHC devices. If a user provides an > > offset and no error is returned, the user would assume that offset had > > been applied (there was no way to know that it was clamped from the > > userspace). This patch series now adds the query for maximum supported > > offset in the PTP_CLOCK_GETCAPS ioctl. In my opinion, I think we will > > see an userspace observable behavior change either way unfortunately due > > to the inconsistency among device drivers, which was one of the main > > issues this patch submission targets. I am ok with making the common > > behavior in ptp_clock_adjtime clamp the provided offset value instead of > > throwing an error when out of range. In both cases, userspace programs > > can handle the out-of-range case explicitly with a check against the > > maximum offset value now advertised in PTP_CLOCK_GETCAPS. My personal > > opinion is that since we have this inconsistency among device drivers > > for handling out of range offsets that are currently provided as-is to > > the driver-specific callback implementations, it makes sense to converge > > to a version that returns an error when the userspace provides > > out-of-range values rather than silently clamping these values. However, > > I am open to either version as long as we have consistency and do not > > leave this up to individual device-drivers to dictate since this adds > > further complexity in the userspace when working with this syscall. > > I'm in favor of throwing an error, since userspace that *doesn't* > check for the max value and assumes it will apply without a clamp may > be surprised when it starts clamping. Userspace which previously > supplied a large value and it clamps now gets an error, which might > be concerning, but they got driver defined behavior before, where it > might error or it might clamp, so I think we're in a no-win scenario > there. > > I don't really see the value in clamping because that makes it hard > to tell if an update was fully applied or not. Now software has to > know to check the range in advance. I wouldn't view a partially > applied update as a successful behavior in a timing application. > Thus, on the principle of least surprise I would avoid clamping. I'm > open to other opinions, and I think standardizing is much better than > letting it be driver behavior. Given that this is general agreement on throwing an error, I'll be ok with that. Perhaps mention the behaviour change in the commit message? Thanks! Paolo
On Mon, 12 Jun, 2023 16:15:29 +0200 Paolo Abeni <pabeni@redhat.com> wrote: > Given that this is general agreement on throwing an error, I'll be ok > with that. Perhaps mention the behaviour change in the commit message? > > Thanks! > > Paolo Mentioned the behavior change in the following patches. Link: https://lore.kernel.org/netdev/20230612211500.309075-8-rrameshbabu@nvidia.com/ Link: https://lore.kernel.org/netdev/20230612211500.309075-9-rrameshbabu@nvidia.com/ Thanks, -- Rahul Rameshbabu
diff --git a/drivers/ptp/ptp_clockmatrix.c b/drivers/ptp/ptp_clockmatrix.c index c9d451bf89e2..f6f9d4adce04 100644 --- a/drivers/ptp/ptp_clockmatrix.c +++ b/drivers/ptp/ptp_clockmatrix.c @@ -1692,14 +1692,23 @@ static int initialize_dco_operating_mode(struct idtcm_channel *channel) /* PTP Hardware Clock interface */ /* - * Maximum absolute value for write phase offset in picoseconds - * - * @channel: channel - * @delta_ns: delta in nanoseconds + * Maximum absolute value for write phase offset in nanoseconds * * Destination signed register is 32-bit register in resolution of 50ps * - * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 + * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 ps + * Represent 107374182350 ps as 107374182 ns + */ +static s32 idtcm_getmaxphase(struct ptp_clock_info *ptp __always_unused) +{ + return MAX_ABS_WRITE_PHASE_NANOSECONDS; +} + +/* + * Internal function for implementing support for write phase offset + * + * @channel: channel + * @delta_ns: delta in nanoseconds */ static int _idtcm_adjphase(struct idtcm_channel *channel, s32 delta_ns) { @@ -1708,7 +1717,6 @@ static int _idtcm_adjphase(struct idtcm_channel *channel, s32 delta_ns) u8 i; u8 buf[4] = {0}; s32 phase_50ps; - s64 offset_ps; if (channel->mode != PTP_PLL_MODE_WRITE_PHASE) { err = channel->configure_write_phase(channel); @@ -1716,19 +1724,7 @@ static int _idtcm_adjphase(struct idtcm_channel *channel, s32 delta_ns) return err; } - offset_ps = (s64)delta_ns * 1000; - - /* - * Check for 32-bit signed max * 50: - * - * 0x7fffffff * 50 = 2147483647 * 50 = 107374182350 - */ - if (offset_ps > MAX_ABS_WRITE_PHASE_PICOSECONDS) - offset_ps = MAX_ABS_WRITE_PHASE_PICOSECONDS; - else if (offset_ps < -MAX_ABS_WRITE_PHASE_PICOSECONDS) - offset_ps = -MAX_ABS_WRITE_PHASE_PICOSECONDS; - - phase_50ps = div_s64(offset_ps, 50); + phase_50ps = div_s64((s64)delta_ns * 1000, 50); for (i = 0; i < 4; i++) { buf[i] = phase_50ps & 0xff; @@ -2048,6 +2044,7 @@ static const struct ptp_clock_info idtcm_caps = { .n_ext_ts = MAX_TOD, .n_pins = MAX_REF_CLK, .adjphase = &idtcm_adjphase, + .getmaxphase = &idtcm_getmaxphase, .adjfine = &idtcm_adjfine, .adjtime = &idtcm_adjtime, .gettime64 = &idtcm_gettime, @@ -2064,6 +2061,7 @@ static const struct ptp_clock_info idtcm_caps_deprecated = { .n_ext_ts = MAX_TOD, .n_pins = MAX_REF_CLK, .adjphase = &idtcm_adjphase, + .getmaxphase = &idtcm_getmaxphase, .adjfine = &idtcm_adjfine, .adjtime = &idtcm_adjtime_deprecated, .gettime64 = &idtcm_gettime, diff --git a/drivers/ptp/ptp_clockmatrix.h b/drivers/ptp/ptp_clockmatrix.h index bf1e49409844..7c17c4f7f573 100644 --- a/drivers/ptp/ptp_clockmatrix.h +++ b/drivers/ptp/ptp_clockmatrix.h @@ -18,7 +18,7 @@ #define MAX_PLL (8) #define MAX_REF_CLK (16) -#define MAX_ABS_WRITE_PHASE_PICOSECONDS (107374182350LL) +#define MAX_ABS_WRITE_PHASE_NANOSECONDS (107374182L) #define TOD_MASK_ADDR (0xFFA5) #define DEFAULT_TOD_MASK (0x04)
Advertise the maximum offset the .adjphase callback is capable of supporting in nanoseconds for IDT ClockMatrix devices. Cc: Richard Cochran <richardcochran@gmail.com> Cc: Vincent Cheng <vincent.cheng.xh@renesas.com> Signed-off-by: Rahul Rameshbabu <rrameshbabu@nvidia.com> --- drivers/ptp/ptp_clockmatrix.c | 36 +++++++++++++++++------------------ drivers/ptp/ptp_clockmatrix.h | 2 +- 2 files changed, 18 insertions(+), 20 deletions(-)