Re: [PATCH net-next v2 7/9] ptp: ptp_clockmatrix: Add .getmaxphase ptp_clock_info callback

[Rahul Rameshbabu <rrameshbabu@nvidia.com>]

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