On Mon, 21 Mar 2022 22:48:32 +0200 Vladimir Oltean wrote:

On Mon, Mar 21, 2022 at 01:28:29PM -0700, Jakub Kicinski wrote:

quoted

On Fri, 18 Mar 2022 00:50:35 +0200 Vladimir Oltean wrote:  

quoted

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.

Sorry, I was trying to avoid saying "master". I meant the "master"
/ host MAC interface that's often a random driver.

quoted

quoted

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?

Yup, we can create a "y" object even if most of DSA is m. Like:

ifneq ($(CONFIG_NET_DSA),)
obj-y += dsa_something_something.o
endif

quoted

quoted

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@nxp.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?

Yes, having a "IOCTL" notifier really isn't great. At the very least
the notifier should be more narrow.

But TBH adding an NDO is one of my "I wish I had the time to clean this
up" tasks, so I thought I'd bring it up in case you have extra context.

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.

Right, would be pretty great to keep the "useful" stuff out of
ndo_eth_ioctl, then we can keep an eye out for new drivers trying 
to use it more easily. Or even add a patchwork check.