Ok to me.
> -----Original Message-----
> From: Vladimir Oltean <olte...@gmail.com>
> Sent: 2021年3月7日 21:24
> To: David S . Miller <da...@davemloft.net>; Jakub Kicinski <k...@kernel.org>;
> netdev@vger.kernel.org; Po Liu <po....@nxp.com>
> Cc: Alexandru Marginean <alexandru.margin...@nxp.com>; Claudiu Manoil
> <claudiu.man...@nxp.com>; Michael Walle <mich...@walle.cc>; Vladimir
> Oltean <vladimir.olt...@nxp.com>; Vinicius Costa Gomes
> <vinicius.go...@intel.com>
> Subject: [PATCH net 2/2] net: enetc: allow hardware timestamping on TX
> queues with tc-etf enabled
>
> Caution: EXT Email
>
> From: Vladimir Oltean <vladimir.olt...@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.go...@intel.com>
> Signed-off-by: Vladimir Oltean <vladimir.olt...@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
Reviewed-by: Po Liu <po....@nxp.com>
Br,
Po Liu
