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

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

quoted

quoted

+static int ice_ptp_adjfine(struct ptp_clock_info *info, long scaled_ppm)
+{
+	struct ice_pf *pf = ptp_info_to_pf(info);
+	u64 freq, divisor = 1000000ULL;
+	struct ice_hw *hw = &pf->hw;
+	s64 incval, diff;
+	int neg_adj = 0;
+	int err;
+
+	incval = ICE_PTP_NOMINAL_INCVAL_E810;
+
+	if (scaled_ppm < 0) {
+		neg_adj = 1;
+		scaled_ppm = -scaled_ppm;
+	}
+
+	while ((u64)scaled_ppm > div_u64(U64_MAX, incval)) {
+		/* handle overflow by scaling down the scaled_ppm and
+		 * the divisor, losing some precision
+		 */
+		scaled_ppm >>= 2;
+		divisor >>= 2;
+	}  

I have a question regarding ppm overflows.

We have the max_adj field in struct ptp_clock_info which is checked
against ppb, but ppb is a signed 32 bit and scaled_ppm is a long,
meaning values larger than S32_MAX << 16 / 1000 will overflow 
the ppb calculation, and therefore the check.

Hmmm.. I thought ppb was a s64, not an s32.

In general, I believe max_adj is usually capped at 1 billion anyways,
since it doesn't make sense to slow a clock by more than 1billioln ppb,
and increasing it more than that isn't really useful either.

Do you mean it's capped somewhere in the code to 1B?

I'm no time expert but this is not probability where 1 is a magic
value, adjusting clock by 1 - 1ppb vs 1 + 1ppb makes little difference,
no? Both mean something is super fishy with the nominal or expected
frequency, but the hardware can do that and more.

Flipping the question, if adjusting by large ppb values is not correct,
why not cap the adjustment at the value which would prevent the u64
overflow?

I don't really have a preferences here, I'm mostly disturbed by 
the overflow in the ppb vs max_adj check.

quoted

Are we okay with that? Is my math off? Did I miss some part 
of the kernel which filters crazy high scaled_ppm/freq?

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?