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 [off-list ref] wrote:

quoted

On Thu, 2023-05-25 at 14:08 +0200, Paolo Abeni wrote:

quoted

quoted

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