Message-ID: <20230511205435.pu6dwhkdnpcdu3v2@skbuf>
Date: Thu, 11 May 2023 23:54:35 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Jakub Kicinski <kuba@...nel.org>
Cc: Köry Maincent <kory.maincent@...tlin.com>,
	netdev@...r.kernel.org, glipus@...il.com,
	maxime.chevallier@...tlin.com, vadim.fedorenko@...ux.dev,
	richardcochran@...il.com, gerhard@...leder-embedded.com,
	thomas.petazzoni@...tlin.com, krzysztof.kozlowski+dt@...aro.org,
	robh+dt@...nel.org, linux@...linux.org.uk
Subject: Re: [PATCH net-next RFC v4 4/5] net: Let the active time stamping
 layer be selectable.

On Thu, May 11, 2023 at 09:25:39AM -0700, Jakub Kicinski wrote:
> On Thu, 11 May 2023 18:56:40 +0300 Vladimir Oltean wrote:
> > > More importantly "monolithic" drivers have DMA/MAC/PHY all under 
> > > the NDO so assuming that SOF_PHY_TIMESTAMPING implies a phylib PHY
> > > is not going to work.
> > > 
> > > We need a more complex calling convention for the NDO.  
> > 
> > It's the first time I become aware of the issue of PHY timestamping in
> > monolithic drivers that don't use phylib, and it's actually a very good
> > point. I guess that input gives a clearer set of constraints for Köry to
> > design an API where the selected timestamping layer is maybe passed to
> > ndo_hwtstamp_set() and MAC drivers are obliged to look at it.
> > 
> > OTOH, a complaint about the current ndo_eth_ioctl() -> phy_mii_ioctl()
> > code path was that phylib PHY drivers need to have the explicit blessing
> > from the MAC driver in order to enable timestamping. This patch set
> > attempts to circumvent that, and you're basically saying that it shouldn't.
> 
> Yes, we don't want to lose the simplification benefit for the common
> cases.

While I'm all for simplification in general, let's not take that for
granted and see what it implies, first.

If the new default behavior for the common case is going to be to bypass
the MAC's ndo_hwtstamp_set(), then MAC drivers which didn't explicitly
allow phylib PHY timestamping will now do.

Let's group them into:

(A) MAC drivers where that is perfectly fine

(B) MAC drivers with forwarding offload, which would forward/flood PTP
    frames carrying PHY timestamps

(C) "solipsistic" MAC drivers which assume that any skb with
    SKBTX_HW_TSTAMP is a skb for *me* to timestamp

Going for the simplification would mean making sure that cases (B)
and (C) are well handled, and have a reasonable chance of not getting
reintroduced in the future.

For case (B) it would mean patching all existing switch drivers which
don't allow PHY timestamping to still not allow PHY timestamping, and
fixing those switch drivers which allow PHY timestamping but don't set
up traps (probably those weren't tested in a bridging configuration).

For case (C) it would mean scanning all MAC drivers for bugs akin to the
one fixed in commit c26a2c2ddc01 ("gianfar: Fix TX timestamping with a
stacked DSA driver"). I did that once, but it was years ago and I can't
guarantee what is the current state or that I didn't miss anything.
For example, I missed the minor fact that igc would count skbs
timestamped by a non-igc entity in its TX path as 'tx_hwtstamp_skipped'
packets, visible in ethtool -S:
https://lore.kernel.org/intel-wired-lan/20230504235233.1850428-2-vinicius.gomes@intel.com/

It has to be said that nowadays, Documentation/networking/timestamping.rst
does warn about stacked PHCs, and those who read will find it. Also,
ptp4l nowadays warns if there are multiple TX timestamps received for
the same skb, and that's a major user of this functionality. So I don't
mean to point this case out as a form of discouragement, but it is going
to be a bit of a roll of dice.


The alternative (ditching the simplification) is that someone still
has to code up the glue logic from ndo_hwtstamp_set() -> phy_mii_ioctl(),
and that presumes some minimal level of testing, which we are now
"simplifying" away.

The counter-argument against ditching the simplification is that DSA
is also vulnerable to the bugs from case (C), but as opposed to PHY
timestamping where currently MACs need to voluntarily pass the
phy_mii_ioctl() call themselves, MACs don't get to choose if they want
to act as DSA masters or not. That gives some more confidence that bugs
in this area might not be so common, and leaves just (B) a concern.

To analyze how common is the common case is a simple matter of counting
how many drivers among those with SIOCSHWTSTAMP implementations also
have some form of forwarding offload, OR, as you point out, how many
don't use phylib.

> I think we should make the "please call me for PHY requests" an opt in.
> 
> Annoyingly the "please call me for PHY/all requests" needs to be
> separate from "this MAC driver supports PHY timestamps". Because in
> your example the switch driver may not in fact implement PHY stamping,
> it just wants to know about the configuration.
> 
> So we need a bit somewhere (in ops? in some other struct? in output 
> of get_ts?) to let the driver declare that it wants to see all TS
> requests. (I've been using bits in ops, IDK if people find that
> repulsive or neat :))

It's either repulsive or neat, depending on the context.

Last time you suggested something like this in an ops structure was IIRC
something like whether "MAC Merge is supported". My objection was that
DSA has a common set of ops structures (dsa_slave_ethtool_ops,
dsa_slave_netdev_ops) behind which lie different switch families from
at least 13 vendors. A shared const ops structure is not an appropriate
means to communicate whether 13 switch vendors support a TSN MAC Merge
layer or not.

With declaring interest in all hardware timestamping requests in the
data path of a MAC, be they MAC-level requests or otherwise, it's a bit
different, because all DSA switches have one thing in common, which is
that they're switches, and that is relevant here. So I'm not opposed to
setting a bit in the ethtool ops structure, at least for DSA that could
work just fine.

> Then if bit is not set or NDO returns -EOPNOTSUPP for PHY requests we
> still try to call the PHY in the core?

Well, if there is no interest for special behavior from the MAC driver,
then I guess the memo is "yolo"...

But OTOH, if a macro-driver like DSA declares its interest in receiving
all timestamping requests, but then that particular DSA switch returns
-EOPNOTSUPP in the ndo_hwtstamp_set() handler, it would be silly for the
core to still "force the entry" and call phy_mii_ioctl() anyway - because
we know that's going to be broken.

So with "NDO returns -EOPNOTSUPP", I hope you don't mean *that* NDO
(ndo_hwtstamp_set()) but a previously unnamed one that serves the same
purpose as the capability bit - ndo_hey_are_you_interested_in_this_hwtstamp_request().
In that case, yes - with -EOPNOTSUPP we're back to "yolo".

> Separately the MAC driver needs to be able to report what stamping 
> it supports (DMA, MAC, PHY, as mentioned in reply to patch 2).

I'm a bit unclear on that - responded there.

Powered by blists - more mailing lists