Re: [PATCH net-next RFC] Add NDOs for hardware timestamp get/set

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Fri, Mar 31, 2023 at 11:10:41AM -0700, Jakub Kicinski wrote:
> > We still will need a version of net_hwtstamp_validate(struct ifreq *ifr)
> > to do validation for drivers not implementing ndo_hwtstamp_set().
> > Also we'll need to implement validation for dsa_ndo_eth_ioctl() which
> > usually has an empty implementation, but can do something
> > meaningful depending on kernel configuration if I understand
> > it correctly. I'm not sure where to insert the validation code for
> > the DSA code path, would greatly appreciate some advice here.
> 
> You can copy from user space onto stack at the start of the new
> dev_set_hwtstamp(), make validation run on the already-copied
> version, and then either proceed to call the NDO with the on-stack
> config which was validated or the legacy and DSA path with ifr.

Actually, and here is the problem, DSA will want to see the timestamping
request with the new code path too, not just with the legacy one.
But, in this form, the dsa_ndo_eth_ioctl() -> dsa_master_ioctl() code
path wants to do one of two things: it either denies the configuration,
or passes it further, unchanged, to the master's netdev_ops->ndo_eth_ioctl().

By being written around the legacy ndo_eth_ioctl(), dsa_ndo_eth_ioctl()
places a requirement which conflicts with any attempt to convert any
kernel driver to the new API, because basically any net device can serve
as a DSA master, and simply put, DSA wants to see timestamping requests
to the DSA master, old or new API.

The only "useful" piece of logic from dsa_master_ioctl() is to deny the
hwtstamp_set operation in some cases, so it's clear that it's useless
for dsa_master_ioctl() to have to call the master's netdev_ops->ndo_eth_ioctl()
when dev_eth_ioctl() already would have done it anyway.

I can make dsa_ndo_eth_ioctl() disappear and replace it with a netdev
notifier as per this patch:
https://lore.kernel.org/netdev/20220317225035.3475538-1-vladimir.oltean@nxp.com/

My understanding of Jakub's objection is that the scope of the
NETDEV_ETH_IOCTL is too wide, and as such, it would need to change to
something like NETDEV_HWTSTAMP_SET. I can make that change if that is
the only objection, and resubmit that as preparation work for the
ndo_hwtstamp_set() effort.