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.
