Re: [PATCH net-next 7/7] net: create a netdev notifier for DSA to reject PTP on DSA master

[Florian Fainelli <f.fainelli@gmail.com>]

On 4/2/2023 5:37 AM, Vladimir Oltean wrote:
> The fact that PTP 2-step TX timestamping is broken on DSA switches if
> the master also timestamps the same packets is documented by commit
> f685e609a301 ("net: dsa: Deny PTP on master if switch supports it").
> We attempt to help the users avoid shooting themselves in the foot by
> making DSA reject the timestamping ioctls on an interface that is a DSA
> master, and the switch tree beneath it contains switches which are aware
> of PTP.
> 
> The only problem is that there isn't an established way of intercepting
> ndo_eth_ioctl calls, so DSA creates avoidable burden upon the network
> stack by creating a struct dsa_netdevice_ops with overlaid function
> pointers that are manually checked from the relevant call sites. There
> used to be 2 such dsa_netdevice_ops, but now, ndo_eth_ioctl is the only
> one left.
> 
> There is an ongoing effort to migrate driver-visible hardware timestamping
> control from the ndo_eth_ioctl() based API to a new ndo_hwtstamp_set()
> model, but DSA actively prevents that migration, since dsa_master_ioctl()
> is currently coded to manually call the master's legacy ndo_eth_ioctl(),
> and so, whenever a network device driver would be converted to the new
> API, DSA's restrictions would be circumvented, because any device could
> be used as a DSA master.
> 
> The established way for unrelated modules to react on a net device event
> is via netdevice notifiers. So we create a new notifier which gets
> called whenever there is an attempt to change hardware timestamping
> settings on a device.
> 
> Finally, there is another reason why a netdev notifier will be a good
> idea, besides strictly DSA, and this has to do with PHY timestamping.
> 
> With ndo_eth_ioctl(), all MAC drivers must manually call
> phy_has_hwtstamp() before deciding whether to act upon SIOCSHWTSTAMP,
> otherwise they must pass this ioctl to the PHY driver via
> phy_mii_ioctl().
> 
> With the new ndo_hwtstamp_set() API, it will be desirable to simply not
> make any calls into the MAC device driver when timestamping should be
> performed at the PHY level.
> 
> But there exist drivers, such as the lan966x switch, which need to
> install packet traps for PTP regardless of whether they are the layer
> that provides the hardware timestamps, or the PHY is. That would be
> impossible to support with the new API.
> 
> The proposal there, too, is to introduce a netdev notifier which acts as
> a better cue for switching drivers to add or remove PTP packet traps,
> than ndo_hwtstamp_set(). The one introduced here "almost" works there as
> well, except for the fact that packet traps should only be installed if
> the PHY driver succeeded to enable hardware timestamping, whereas here,
> we need to deny hardware timestamping on the DSA master before it
> actually gets enabled. This is why this notifier is called "PRE_", and
> the notifier that would get used for PHY timestamping and packet traps
> would be called NETDEV_CHANGE_HWTSTAMP. This isn't a new concept, for
> example NETDEV_CHANGEUPPER and NETDEV_PRECHANGEUPPER do the same thing.
> 
> In expectation of future netlink UAPI, we also pass a non-NULL extack
> pointer to the netdev notifier, and we make DSA populate it with an
> informative reason for the rejection. To avoid making it go to waste, we
> make the ioctl-based dev_set_hwtstamp() create a fake extack and print
> the message to the kernel log.
> 
> Link: https://lore.kernel.org/netdev/20230401191215.tvveoi3lkawgg6g4@skbuf/
> Link: https://lore.kernel.org/netdev/20230310164451.ls7bbs6pdzs4m6pw@skbuf/
> Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian