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:51 AM
> To: Richard Cochran <richardcochran@gmail.com>
> Cc: Keller, Jacob E <jacob.e.keller@intel.com>; Nguyen, Anthony L
> <anthony.l.nguyen@intel.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 11:12:20 -0700 Richard Cochran wrote:
> > On Mon, Jun 14, 2021 at 09:43:17AM -0700, Jacob Keller wrote:
> > > > 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)

This looks correct to me.