Re: [RFC PATCH v3 3/5] Add ndo_hwtstamp_get/set support to vlan code path

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

On Wed, Apr 05, 2023 at 10:19:11AM -0600, Max Georgiev wrote:
> > I would recommend also making vlan_dev_hwtstamp() be called from the
> > VLAN driver's ndo_hwtstamp_set() rather than from ndo_eth_ioctl().
> 
> Vladimir, could you please elaborate here a bit?
> Are you saying that I should go all the way with vlan NDO conversion,
> implement ndo_hwtstamp_get/set() for vlan, and stop handling
> SIOCGHWTSTAMP/SIOCSHWTSTAMP in vlan_dev_ioctl()?

Yes, sorry for being unclear.

> > My understanding of Jakub's suggestion to (temporarily) stuff ifr
> > inside kernel_config was to do that from top-level net/core/dev_ioctl.c,
> > not from the VLAN driver.
> 
> [RFC PATCH v3 2/5] in this patch stack changes net/core/dev_ioctl.c
> to insert ifr inside kernel_config. I assumed that I should do it here too
> so underlying drivers could rely on ifr pointer in kernel_config being
> always initialized.
> If the plan is to stop supporting SIOCGHWTSTAMP/SIOCSHWTSTAMP
> in vlan_dev_ioctl() all together and move the hw timestamp handling
> logic to vlan_get/set_hwtstamp() functions, then this ifr initialization
> code will be removed from net/8021q/vlan_dev.c anyway.

Yes, correct, dev_set_hwtstamp() should provide it.

There's a small thing I don't like about stuffing "ifr" inside struct
kernel_hwtstamp_config, and that's that some drivers keep the last
configuration privately using memcpy(). If we put "ifr" there, they will
practically have access to a stale pointer, because "ifr" loses meaning
once the ioctl syscall is over.

Since we don't know how long it will take until the ndo_hwtstamp_set()
conversion is complete (experience says: possibly indefinitely), it
would be good if this wasn't possible, because who knows what ideas
people might get to do with it.

Options to avoid it are:
- keep doing what you're doing - let drivers memcpy() the struct
  hwtstamp_config and not the struct kernel_hwtstamp_config.
- pass the ifr as yet another argument to ndo_hwtstamp_set(), and don't
  stuff it inside struct kernel_hwtstamp_config.

Neither of these is particularly great, because at the end of the
conversion, some extra cleanup will be required to fix up the API again
(either to stop all drivers from using struct hwtstamp_config, or to
stop passing the argument which is no longer used by anybody).

I think, personally, I'd opt for the first bullet, keep doing what
you're doing. It should require a bit less cleanup, since not all
drivers do the memcpy() thing.