Message-ID: <20250512210942.2bvv466abjdswhay@skbuf>
Date: Tue, 13 May 2025 00:09:42 +0300
From: Vladimir Oltean <vladimir.oltean@....com>
To: Gerhard Engleder <gerhard@...leder-embedded.com>
Cc: Andrew Lunn <andrew@...n.ch>, "David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
	Simon Horman <horms@...nel.org>,
	Vadim Fedorenko <vadim.fedorenko@...ux.dev>,
	Richard Cochran <richardcochran@...il.com>, netdev@...r.kernel.org
Subject: Re: [PATCH net] net: tsnep: fix timestamping with a stacked DSA
 driver

On Mon, May 12, 2025 at 10:07:52PM +0200, Gerhard Engleder wrote:
> On 12.05.25 15:24, Vladimir Oltean wrote:
> > This driver seems susceptible to a form of the bug explained in commit
> > c26a2c2ddc01 ("gianfar: Fix TX timestamping with a stacked DSA driver")
> > and in Documentation/networking/timestamping.rst section "Other caveats
> > for MAC drivers", specifically it timestamps any skb which has
> > SKBTX_HW_TSTAMP, and does not consider adapter->hwtstamp_config.tx_type.
> 
> Is it necessary in general to check adapter->hwtstamp_config.tx_type for
> HWTSTAMP_TX_ON or only to fix this bug?

I'll start with the problem description and work my way towards an answer.

The socket UAPI doesn't support presenting multiple TX hardware timestamps
for the same packet, or better said, user space has no way of distinguishing
the source of a hardware timestamp other than assuming it comes from the
PHC reported by the ethtool get_ts_info() of the driver. If the kernel
delivers multiple hardware timestamps to the socket error queue for the
same packet, nothing good comes out of that, so we expect there to be
filtering somewhere to avoid that.

The way DSA switches are hooked up is by presenting themselves as a
collection of N MAC interfaces (possibly with hardware timestamping
capabilities of their own) stacked on top of the host port like VLANs,
with their ndo_start_xmit() doing some packet processing and ultimately
calling the ndo_start_xmit() of the tsnep driver.

DSA is supposed to work with any MAC driver as host port as long as it
is fairly well behaved, and as a MAC driver you don't really get to
choose if you support it or not. One of the expectations is that the
host port driver should only provide hardware TX timestamps only if it
is the active TX timestamping layer for the packet. That is one level of
filtering.

DSA will prevent adapter->hwtstamp_config.tx_type from ever being
changed from the default value by blocking driver calls to
SIOCSHWTSTAMP, and that is another level of filtering. See
dev_set_hwtstamp() -> dsa_conduit_hwtstamp_validate() -> ... ->
__dsa_conduit_hwtstamp_validate().

Another case where the packet visits your ndo_start_xmit() but the
timestamping duty is "not for you" is with PHY timestamping. To user
space, it looks the same (for better or for worse), so SKBTX_HW_TSTAMP
is set by the usual place in __sock_tx_timestamp(), and can be found set
during ndo_start_xmit().

I didn't mention this because as opposed to DSA, PHY timestamping needs
explicit MAC driver cooperation, and the tsnep driver does not currently
fulfill the requirements for supporting PHY timestamping - so no user
visible bug exists there.
(1) the control path operations, SIOCSHWTSTAMP and SIOCGHWTSTAMP, are
    always processed by tsnep_ptp_ioctl() and never passed on to
    phy_mii_ioctl() -> phydev->mii_ts->hwtstamp(). So it never gives
    timestamping PHY drivers a chance to set themselves up.
(2) the data path hook is in skb_tx_timestamp(), which tsnep calls from
    tsnep_xmit_frame_ring(). That part is fine, but in the future it may
    result in one of the drivers from drivers/net/phy/ setting
    SKBTX_IN_PROGRESS even when tsnep_xmit_frame_ring() didn't do that.

(1) is something that in the new API based on ndo_hwtstamp_set() changes
radically. After converting to the new API, the core, in dev_set_hwtstamp(),
decides now whether to pass the SIOCSHWTSTAMP call to the MAC driver or
to the PHY driver, it is no longer the MAC driver's choice. So it
becomes more like DSA, you need to comply with a basic set of principles
and then you may support stacked timestamping layers without even
knowing it. It boils down to the same thing, only provide a timestamp
if you're the active layer.

> In Documentation/networking/timestamping.rst section "Hardware
> Timestamping Implementation: Device Drivers" only the check of
> (skb_shinfo(skb)->tx_flags & SKBTX_HW_TSTAMP) is mentioned.

Yeah, a more complete picture is built later on in the document, in the
section immediately following the one you quoted.

> > Evaluate the proper TX timestamping condition only once on the TX
> > path (in tsnep_netdev_xmit_frame()) and pass it down to
> > tsnep_xmit_frame_ring() and tsnep_tx_activate() through a bool variable.
> 
> What about also storing the TX timestamping condition in TX entry and
> evaluating it in tsnep_tx_poll() instead of checking
> adapter->hwtstamp_config.tx_type again? This way the timestamping
> decision is only done in tsnep_netdev_xmit_frame() and tsnep_tx_poll()
> cannot decide differently e.g. if hardware timestamping was deactivated
> in between.

That would be a lot better indeed. I didn't want to make a clumsy
attempt at that.

> Also this means that SKBTX_IN_PROGRESS is only set but never evaluated
> by tsnep, which should fix this bug AFAIU.

SKBTX_IN_PROGRESS is part of the problem (and in case of gianfar was the
only problem), but tsnep has the additional problem of not evaluating
adapter->hwtstamp_config.tx_type even once, in the TX path. I've explained
above the results I expect.

> > Also evaluate it again in the TX confirmation path, in tsnep_tx_poll(),
> > since I don't know whether TSNEP_DESC_EXTENDED_WRITEBACK_FLAG is a
> > confounding condition and may be set for other reasons than hardware
> > timestamping too.
> 
> Yes, there is also some DMA statistic included besides timestamping so
> it can be set for other reasons too in the future.
> 
> I can take over this patch and test it when I understand more clearly
> what needs to be done.
> 
> Gerhard

It would be great if you could take over this patch. After the net ->
net-next merge I can then submit the ndo_hwtstamp_get()/ndo_hwtstamp_set()
conversion patch for tsnep, the one which initially prompted me to look
into how this driver uses the provided configuration.

Powered by blists - more mailing lists