RE: [PATCH net 2/2] net: enetc: allow hardware timestamping on TX queues with tc-etf enabled

-----Original Message-----
From: Vladimir Oltean <olteanv@gmail.com>
Sent: 2021年3月7日 21:24
To: David S . Miller <davem@davemloft.net>; Jakub Kicinski <kuba@kernel.org>;
netdev@vger.kernel.org; Po Liu [off-list ref]
Cc: Alexandru Marginean <redacted>; Claudiu Manoil
[off-list ref]; Michael Walle [off-list ref]; Vladimir
Oltean [off-list ref]; Vinicius Costa Gomes
[off-list ref]
Subject: [PATCH net 2/2] net: enetc: allow hardware timestamping on TX
queues with tc-etf enabled

Caution: EXT Email

From: Vladimir Oltean <vladimir.oltean@nxp.com>

The txtime is passed to the driver in skb->skb_mstamp_ns, which is actually in a
union with skb->tstamp (the place where software timestamps are kept).

Since commit b50a5c70ffa4 ("net: allow simultaneous SW and HW transmit
timestamping"), __sock_recv_timestamp has some logic for making sure that
the two calls to skb_tstamp_tx:

skb_tx_timestamp(skb) # Software timestamp in the driver
-> skb_tstamp_tx(skb, NULL)

and

skb_tstamp_tx(skb, &shhwtstamps) # Hardware timestamp in the driver

will both do the right thing and in a race-free manner, meaning that
skb_tx_timestamp will deliver a cmsg with the software timestamp only, and
skb_tstamp_tx with a non-NULL hwtstamps argument will deliver a cmsg with
the hardware timestamp only.

Why are races even possible? Well, because although the software timestamp
skb->tstamp is private per skb, the hardware timestamp
skb->skb_hwtstamps(skb)
lives in skb_shinfo(skb), an area which is shared between skbs and their clones.
And skb_tstamp_tx works by cloning the packets when timestamping them,
therefore attempting to perform hardware timestamping on an skb's clone will
also change the hardware timestamp of the original skb. And the original skb
might have been yet again cloned for software timestamping, at an earlier stage.

So the logic in __sock_recv_timestamp can't be as simple as saying "does this
skb have a hardware timestamp? if yes I'll send the hardware timestamp to the
socket, otherwise I'll send the software timestamp", precisely because the
hardware timestamp is shared.
Instead, it's quite the other way around: __sock_recv_timestamp says "does this
skb have a software timestamp? if yes, I'll send the software timestamp,
otherwise the hardware one". This works because the software timestamp is not
shared with clones.

But that means we have a problem when we attempt hardware timestamping
with skbs that don't have the skb->tstamp == 0. __sock_recv_timestamp will say
"oh, yeah, this must be some sort of odd clone" and will not deliver the
hardware timestamp to the socket. And this is exactly what is happening when
we have txtime enabled on the socket: as mentioned, that is put in a union with
skb->tstamp, so it is quite easy to mistake it.

Do what other drivers do (intel igb/igc) and write zero to skb->tstamp before
taking the hardware timestamp. It's of no use to us now (we're already on the
TX confirmation path).

Fixes: 0d08c9ec7d6e ("enetc: add support time specific departure base on the
qos etf")
Cc: Vinicius Costa Gomes <vinicius.gomes@intel.com>
Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com>
---
 drivers/net/ethernet/freescale/enetc/enetc.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c

b/drivers/net/ethernet/freescale/enetc/enetc.c
index 30d7d4e83900..09471329f3a3 100644

--- a/drivers/net/ethernet/freescale/enetc/enetc.c
+++ b/drivers/net/ethernet/freescale/enetc/enetc.c

@@ -344,6 +344,12 @@ static void enetc_tstamp_tx(struct sk_buff *skb, u64

tstamp)
        if (skb_shinfo(skb)->tx_flags & SKBTX_IN_PROGRESS) {
                memset(&shhwtstamps, 0, sizeof(shhwtstamps));
                shhwtstamps.hwtstamp = ns_to_ktime(tstamp);
+               /* Ensure skb_mstamp_ns, which might have been populated with
+                * the txtime, is not mistaken for a software timestamp,
+                * because this will prevent the dispatch of our hardware
+                * timestamp to the socket.
+                */
+               skb->tstamp = ktime_set(0, 0);
                skb_tstamp_tx(skb, &shhwtstamps);
        }
 }
--
2.25.1