Date:   Mon, 21 Mar 2022 22:48:32 +0200
From:   Vladimir Oltean <olteanv@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Vladimir Oltean <vladimir.oltean@....com>, netdev@...r.kernel.org,
        Paolo Abeni <pabeni@...hat.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        Andrew Lunn <andrew@...n.ch>,
        Vivien Didelot <vivien.didelot@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Richard Cochran <richardcochran@...il.com>
Subject: Re: [RFC PATCH net-next] net: create a NETDEV_ETH_IOCTL notifier for
 DSA to reject PTP on DSA master

On Mon, Mar 21, 2022 at 01:28:29PM -0700, Jakub Kicinski wrote:
> On Fri, 18 Mar 2022 00:50:35 +0200 Vladimir Oltean wrote:
> > The fact that PTP 2-step TX timestamping is deeply broken on DSA
> > switches if the master also timestamps the same packets is well
> > 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.
> 
> Remind me - are the DSA CPU-side interfaces linked as lower devices 
> of the ports?

DSA CPU-side interfaces have no representation towards user space.
We overlay some port counters through the ethtool_ops of the host
controller, such that when the user runs "ethtool -S eth0" they see the
counters of the switch port and of the host port back to back, and
that's about it. The ioctl for which we have a special case, and which
I'm now trying to remove, is just to enforce a restriction.

> > In fact, the underlying reason which is prompting me to make this change
> > is that I'd like to hide as many DSA data structures from public API as
> > I can. But struct net_device :: dsa_ptr is a struct dsa_port (which is a
> > huge structure), and I'd like to create a smaller structure. I'd like
> > struct dsa_netdevice_ops to not be a part of this, so this is how the
> > need to delete it arose.
> 
> Isn't it enough to move the implementation to a C source instead 
> of having it be a static inline?

Assuming you mean to make that C source part of dsa_core.o:

obj-$(CONFIG_NET_DSA) += dsa_core.o

CONFIG_NET_DSA can be module, so it couldn't be called from built-in code.

Or do you mean adding something like:

obj-y := dsa_extra.o

which only contains dsa_master_ioctl(), basically?

> > The established way for unrelated modules to react on a net device event
> > is via netdevice notifiers. These have the advantage of loose coupling,
> > i.e. they work even when DSA is built as module, without resorting to
> > static inline functions (which cannot offer the desired data structure
> > encapsulation).
> > 
> > Signed-off-by: Vladimir Oltean <vladimir.oltean@....com>
> > ---
> > I'd mostly like to take this opportunity to raise a discussion about how
> > to handle this. It's clear that calling the notifier chain is less
> > efficient than having some dev->dsa_ptr checks, but I'm not sure if the
> > ndo_eth_ioctl can tolerate the extra performance hit at the expense of
> > some code cleanliness.
> > 
> > Of course, what would be great is if we didn't have the limitation to
> > begin with, but the effort to add UAPI for multiple TX timestamps per
> > packet isn't proportional to the stated goal here, which is to hide some
> > DSA data structures.
> 
> Was there a reason we haven't converted the timestamping to ndos?
> Just a matter of finding someone with enough cycles to go thru all 
> the drivers?

So you're saying that if SIOCSHWTSTAMP and SIOCGHWTSTAMP had their own
ndo, a netdev notifier would be easier to swallow?

Maybe, I haven't explored that avenue, but on the other hand,
ndo_eth_ioctl seems to only be used for PTP, plus PHY user-space access
(SIOCGMIIPHY, SIOCGMIIREG, SIOCSMIIREG). There isn't an active desire
from PHY maintainers to keep that UAPI in the best shape, to my
knowledge, since it is feared that if it works too well, vendors might
end up with user space PHY drivers for their SDKs. Those ioctls are
currently a best-effort debugging tool.

Powered by blists - more mailing lists