On Mon, 2023-06-12 at 05:16 +0000, Keller, Jacob E wrote:

quoted

-----Original Message-----
From: Rahul Rameshbabu <rrameshbabu@nvidia.com>
Sent: Friday, June 9, 2023 12:48 PM
To: Paolo Abeni <pabeni@redhat.com>
Cc: netdev@vger.kernel.org; David S. Miller <davem@davemloft.net>; Keller,
Jacob E [off-list ref]; Gal Pressman [off-list ref]; Tariq
Toukan [off-list ref]; Saeed Mahameed [off-list ref]; Richard
Cochran [off-list ref]; Vincent Cheng
[off-list ref]
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 [off-list ref] wrote:

quoted

On Thu, 2023-05-25 at 11:09 -0700, Rahul Rameshbabu wrote:

quoted

On Thu, 25 May, 2023 14:11:51 +0200 Paolo Abeni [off-list ref] 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