lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
Open Source and information security mailing list archives
| ||
|
Message-ID: <87r0r4l1v6.fsf@nvidia.com> Date: Thu, 25 May 2023 11:09:17 -0700 From: Rahul Rameshbabu <rrameshbabu@...dia.com> To: Paolo Abeni <pabeni@...hat.com> Cc: netdev@...r.kernel.org, "David S. Miller" <davem@...emloft.net>, Jacob Keller <jacob.e.keller@...el.com>, Gal Pressman <gal@...dia.com>, Tariq Toukan <tariqt@...dia.com>, Saeed Mahameed <saeed@...nel.org>, Richard Cochran <richardcochran@...il.com>, Vincent Cheng <vincent.cheng.xh@...esas.com> Subject: Re: [PATCH net-next v2 7/9] ptp: ptp_clockmatrix: Add .getmaxphase ptp_clock_info callback On Thu, 25 May, 2023 14:11:51 +0200 Paolo Abeni <pabeni@...hat.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@...il.com> >> > Cc: Vincent Cheng <vincent.cheng.xh@...esas.com> >> > Signed-off-by: Rahul Rameshbabu <rrameshbabu@...dia.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
Powered by blists - more mailing lists