Re: [PATCH v3 3/5] net: Let the active time stamping layer be selectable.

From: Vladimir Oltean <vladimir.oltean@nxp.com>
To: Jakub Kicinski <kuba@kernel.org>
Cc: "Richard Cochran" <richardcochran@gmail.com>,
	"Köry Maincent" <kory.maincent@bootlin.com>,
	"Oleksij Rempel" <o.rempel@pengutronix.de>,
	"Horatiu Vultur" <horatiu.vultur@microchip.com>,
	"Michael Walle" <michael@walle.cc>,
	"Andrew Lunn" <andrew@lunn.ch>,
	"Florian Fainelli" <f.fainelli@gmail.com>,
	"Heiner Kallweit" <hkallweit1@gmail.com>,
	netdev@vger.kernel.org, devicetree@vger.kernel.org,
	linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
	linux-omap@vger.kernel.org,
	"Maxime Chevallier" <maxime.chevallier@bootlin.com>,
	thomas.petazzoni@bootlin.com,
	"Russell King" <linux@armlinux.org.uk>,
	"David S. Miller" <davem@davemloft.net>,
	"Eric Dumazet" <edumazet@google.com>,
	"Paolo Abeni" <pabeni@redhat.com>,
	"Rob Herring" <robh+dt@kernel.org>,
	"Krzysztof Kozlowski" <krzysztof.kozlowski+dt@linaro.org>,
	"Jonathan Corbet" <corbet@lwn.net>,
	"Jay Vosburgh" <j.vosburgh@gmail.com>,
	"Veaceslav Falico" <vfalico@gmail.com>,
	"Andy Gospodarek" <andy@greyhouse.net>,
	"Claudiu Manoil" <claudiu.manoil@nxp.com>,
	"Alexandre Belloni" <alexandre.belloni@bootlin.com>,
	UNGLinuxDriver@microchip.com,
	"Minghao Chi" <chi.minghao@zte.com.cn>,
	"Jie Wang" <wangjie125@huawei.com>,
	"Oleksij Rempel" <linux@rempel-privat.de>,
	"Sean Anderson" <sean.anderson@seco.com>,
	"Gustavo A. R. Silva" <gustavoars@kernel.org>,
	"Wolfram Sang" <wsa+renesas@sang-engineering.com>,
	"Alexander Lobakin" <alexandr.lobakin@intel.com>,
	"Marco Bonelli" <marco@mebeim.net>
Subject: Re: [PATCH v3 3/5] net: Let the active time stamping layer be selectable.
Date: Sat, 18 Mar 2023 13:54:57 +0200	[thread overview]
Message-ID: <20230318115457.gtfvq6gom3jew2qc@skbuf> (raw)
In-Reply-To: <20230317210306.346e80ea@kernel.org>

On Fri, Mar 17, 2023 at 09:03:06PM -0700, Jakub Kicinski wrote:
> On Fri, 17 Mar 2023 20:38:49 -0700 Richard Cochran wrote:
> > On Fri, Mar 17, 2023 at 05:21:50PM +0200, Vladimir Oltean wrote:
> > > On Thu, Mar 16, 2023 at 04:09:20PM +0100, Köry Maincent wrote:  
> > > > Was there any useful work that could be continued on managing timestamp through
> > > > NDOs. As it seem we will made some change to the timestamp API, maybe it is a
> > > > good time to also take care of this.  
> > > 
> > > Not to my knowledge. Yes, I agree that it would be a good time to add an
> > > NDO for hwtimestamping (while keeping the ioctl fallback), then
> > > transitioning as many devices as we can, and removing the fallback when
> > > the transition is complete.  
> > 
> > Um, user space ABI cannot be removed.
> 
> NDO meaning a dedicated callback in struct net_device_ops, so at least
> for netdevs we can copy the data from user space, validate in the core
> and then call the driver with a normal kernel pointer. So just an
> internal refactoring, no uAPI changes.

Yes, I was talking about the current handling via net_device_ops :: ndo_eth_ioctl()
(internal driver-facing kernel API) that should eventually get removed.
The new ndo_hwtstamp_get() and ndo_hwtstamp_set() should also have
slightly different (clearer) semantics IMO, like for example they should
only get called if the selected timestamping layer is the MAC. The MAC
driver would no longer be concerned with marshalling these calls down to
the PHY for PHY timestamping with this new API.

This is also the reason why the conversion can't be realistically done
all at once, because in some cases, as pointed out by Horatiu, simply
marshalling the ndo_eth_ioctl() to phy_mii_ioctl() isn't the only thing
that's necessary - sometimes the MAC driver may need to add filters or
traps for PTP frames itself, even if it doesn't provide the timestamps
per se. That will be solved not via the ndo_hwtstamp_set(), but via a
new (listen-only) NETDEV_HWTSTAMP_SET notifier, where interested drivers
can figure out that timestamping was enabled somewhere along the data
path of their netdev (not necessarily at their MAC layer) and program
those filters or traps accordingly, so that either MAC, or PHY,
timestamping works properly e.g. on a switch.

Also, the ndo_hwtstamp_get() and ndo_hwtstamp_set() API should not need
to explicitly call copy_from_user() and copy_to_user(), those are
especially error-prone w.r.t. their error code - non-zero means "bytes
left to copy IIRC", but -EFAULT should be returned to user space,
instead of blindly propagating what copy_from_user() has returned.