Re: [PATCH net-next 5/8] ice: register 1588 PTP clock device object for E810 devices
From: Jakub Kicinski <kuba@kernel.org>
Date: 2021-06-14 18:50:45
Subsystem:
networking drivers, ptp hardware clock support, the rest · Maintainers:
Andrew Lunn, "David S. Miller", Eric Dumazet, Jakub Kicinski, Paolo Abeni, Richard Cochran, Linus Torvalds
On Mon, 14 Jun 2021 11:12:20 -0700 Richard Cochran wrote:
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)