Message-ID: <13b7315446390d3a78d8f508937354f12778b68e.camel@redhat.com>
Date: Mon, 12 Jun 2023 16:15:29 +0200
From: Paolo Abeni <pabeni@...hat.com>
To: "Keller, Jacob E" <jacob.e.keller@...el.com>, Rahul Rameshbabu
	 <rrameshbabu@...dia.com>
Cc: "netdev@...r.kernel.org" <netdev@...r.kernel.org>, "David S. Miller"
	 <davem@...emloft.net>, Gal Pressman <gal@...dia.com>, Tariq Toukan
	 <tariqt@...dia.com>, Saeed Mahameed <saeed@...nel.org>, Richard Cochran
	 <richardcochran@...il.com>, Vincent Cheng <vincent.cheng.xh@...esas.com>
Subject: Re: [PATCH net-next v2 7/9] ptp: ptp_clockmatrix: Add .getmaxphase
 ptp_clock_info callback

On Mon, 2023-06-12 at 05:16 +0000, Keller, Jacob E wrote:
> > -----Original Message-----
> > From: Rahul Rameshbabu <rrameshbabu@...dia.com>
> > Sent: Friday, June 9, 2023 12:48 PM
> > To: Paolo Abeni <pabeni@...hat.com>
> > Cc: netdev@...r.kernel.org; David S. Miller <davem@...emloft.net>; Keller,
> > Jacob E <jacob.e.keller@...el.com>; Gal Pressman <gal@...dia.com>; Tariq
> > Toukan <tariqt@...dia.com>; Saeed Mahameed <saeed@...nel.org>; Richard
> > Cochran <richardcochran@...il.com>; Vincent Cheng
> > <vincent.cheng.xh@...esas.com>
> > Subject: Re: [PATCH net-next v2 7/9] ptp: ptp_clockmatrix: Add .getmaxphase
> > ptp_clock_info callback
> > 
> > On Fri, 09 Jun, 2023 08:38:11 +0200 Paolo Abeni <pabeni@...hat.com> wrote:
> > > On Thu, 2023-05-25 at 11:09 -0700, Rahul Rameshbabu wrote:
> > > > On Thu, 25 May, 2023 14:11:51 +0200 Paolo Abeni <pabeni@...hat.com> wrote:
> > > > 
> > > 
> > > I guess the user-space could pass such large delta (e.g. at boot
> > > time?!?). If so, with this patch we change an user-space observable
> > > behavior, and I think we should avoid that.
> > 
> > The point that you bring up here is about clamping (which is done by
> > idtcm_adjphase previously) versus throwing an error when out of range
> > (what is now done in ptp_clock_adjtime in this patch series). This was
> > something I was struggling with deciding on a unified behavior across
> > all drivers. For example, the mlx5_core driver chooses to return -ERANGE
> > when the delta landed on it is out of the range supported by the PHC of
> > the device. We chose to return an error because there was no mechanism
> > previously for the userspace to know what was the supported offset when
> > using ADJ_OFFSET with different PHC devices. If a user provides an
> > offset and no error is returned, the user would assume that offset had
> > been applied (there was no way to know that it was clamped from the
> > userspace). This patch series now adds the query for maximum supported
> > offset in the PTP_CLOCK_GETCAPS ioctl. In my opinion, I think we will
> > see an userspace observable behavior change either way unfortunately due
> > to the inconsistency among device drivers, which was one of the main
> > issues this patch submission targets. I am ok with making the common
> > behavior in ptp_clock_adjtime clamp the provided offset value instead of
> > throwing an error when out of range. In both cases, userspace programs
> > can handle the out-of-range case explicitly with a check against the
> > maximum offset value now advertised in PTP_CLOCK_GETCAPS. My personal
> > opinion is that since we have this inconsistency among device drivers
> > for handling out of range offsets that are currently provided as-is to
> > the driver-specific callback implementations, it makes sense to converge
> > to a version that returns an error when the userspace provides
> > out-of-range values rather than silently clamping these values. However,
> > I am open to either version as long as we have consistency and do not
> > leave this up to individual device-drivers to dictate since this adds
> > further complexity in the userspace when working with this syscall.
> 
> I'm in favor of throwing an error, since userspace that *doesn't*
> check for the max value and assumes it will apply without a clamp may
> be surprised when it starts clamping. Userspace which previously
> supplied a large value and it clamps now gets an error, which might
> be concerning, but they got driver defined behavior before, where it
> might error or it might clamp, so I think we're in a no-win scenario
> there.
> 
> I don't really see the value in clamping because that makes it hard
> to tell if an update was fully applied or not. Now software has to
> know to check the range in advance. I wouldn't view a partially
> applied update as a successful behavior in a timing application.
> Thus, on the principle of least surprise I would avoid clamping. I'm
> open to other opinions, and I think standardizing is much better than
> letting it be driver behavior.

Given that this is general agreement on throwing an error, I'll be ok
with that. Perhaps mention the behaviour change in the commit message?

Thanks!

Paolo

Powered by blists - more mailing lists