| Message ID | 20210307132339.2320009-2-olteanv@gmail.com |
|---|---|
| State | New |
| Headers | show |
| Series | [net,1/2] net: enetc: set MAC RX FIFO to recommended value | expand |
Vladimir Oltean <olteanv@gmail.com> writes: > 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_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. Great write up :-) I know first person the time/effort that went on debugging this. What do you think of introducing a helper, something like: skb_txtime_consumed() to make it even clearer what's going on. > > 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> > --- Anyway, Acked-by: Vinicius Costa Gomes <vinicius.gomes@intel.com> Cheers, -- Vinicius
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); } }