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

["Keller, Jacob E" <jacob.e.keller@intel.com>]

> -----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
> <richardcochran@gmail.com>; davem@davemloft.net; netdev@vger.kernel.org;
> sassmann@redhat.com; Brelinski, TonyX <tonyx.brelinski@intel.com>
> 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:
> > >> +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.
> 
> > > 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?