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:09 AM
To: Keller, Jacob E <jacob.e.keller@intel.com>
Cc: Nguyen, Anthony L <anthony.l.nguyen@intel.com>; Richard Cochran
[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 09:43:17 -0700 Jacob Keller wrote:

quoted

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?

Large ppb values are sometimes used when you want to slew a clock to bring it in sync when its a few milliseconds to seconds off, without performing a time jump (so that you maintain monotonic increasing time).

That being said, we are supposed to be checking max_adj, except that you're right the conversion to ppb could overflow, and there's no check prior to the conversion from scaled_ppm to ppb.

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

quoted

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?