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

[Rahul Rameshbabu <rrameshbabu@nvidia.com>]

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