RE: [PATCH net-next 5/8] ice: register 1588 PTP clock device object for E810 devices

-----Original Message-----
From: Jakub Kicinski <kuba@kernel.org>
Sent: Monday, June 14, 2021 11:51 AM
To: Richard Cochran <richardcochran@gmail.com>
Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Nguyen, Anthony L
[off-list ref]; davem@davemloft.net;
netdev@vger.kernel.org; sassmann@redhat.com; Brelinski, TonyX
[off-list ref]
Subject: Re: [PATCH net-next 5/8] ice: register 1588 PTP clock device object for
E810 devices

On Mon, 14 Jun 2021 11:12:20 -0700 Richard Cochran wrote:

quoted

On Mon, Jun 14, 2021 at 09:43:17AM -0700, Jacob Keller wrote:

quoted

quoted

Since dialed_freq is updated regardless of return value of .adjfine
the driver has no clear way to reject bad scaled_ppm>

I'm not sure. +Richard?

The driver advertises "max_adj".  The PHC layer checks user space inputs:

ptp_clock.c line 140:
	} else if (tx->modes & ADJ_FREQUENCY) {
		s32 ppb = scaled_ppm_to_ppb(tx->freq);
		if (ppb > ops->max_adj || ppb < -ops->max_adj)
			return -ERANGE;

So, if the max_adj is correct for the driver/HW, then all is well.

tx->freq is a long, and the conversion to ppb can overflow the s32 type.
E.g. 281474976645 will become -2 AFAICT. I hacked up phc_ctl to not do
range checking and kernel happily accepted that value. Shall we do this?

--->8----

diff --git a/drivers/ptp/ptp_clock.c b/drivers/ptp/ptp_clock.c
index 03a246e60fd9..ed32fc98d9d8 100644
--- a/drivers/ptp/ptp_clock.c
+++ b/drivers/ptp/ptp_clock.c

@@ -63,7 +63,7 @@ static void enqueue_external_timestamp(struct

timestamp_event_queue *queue,
        spin_unlock_irqrestore(&queue->lock, flags);
 }

-s32 scaled_ppm_to_ppb(long ppm)
+s64 scaled_ppm_to_ppb(long ppm)
 {
        /*
         * The 'freq' field in the 'struct timex' is in parts per

@@ -80,7 +80,7 @@ s32 scaled_ppm_to_ppb(long ppm)
        s64 ppb = 1 + ppm;
        ppb *= 125;
        ppb >>= 13;
-       return (s32) ppb;
+       return ppb;
 }
 EXPORT_SYMBOL(scaled_ppm_to_ppb);

@@ -138,7 +138,7 @@ static int ptp_clock_adjtime(struct posix_clock *pc, struct

__kernel_timex *tx)
                delta = ktime_to_ns(kt);
                err = ops->adjtime(ops, delta);
        } else if (tx->modes & ADJ_FREQUENCY) {
-               s32 ppb = scaled_ppm_to_ppb(tx->freq);
+               s64 ppb = scaled_ppm_to_ppb(tx->freq);
                if (ppb > ops->max_adj || ppb < -ops->max_adj)
                        return -ERANGE;
                if (ops->adjfine)