Hi
Can it just move
skb->tstamp = ktime_set(0, 0);
into
skb_tstamp_tx(skb, &shhwtstamps);
if it always need to clear for HW tstamp setting.
> -----Original Message-----
> From: Vladimir Oltean <olte...@gmail.com>
> Sent: 2021年3月10日 22:51
> To: Jakub Kicinski <k...@kernel.org>; netdev@vger.kernel.org; Po Liu
> <po....@nxp.com>; Claudiu Manoil <claudiu.man...@nxp.com>
> Cc: Vinicius Costa Gomes <vinicius.go...@intel.com>; Richard Cochran
> <richardcoch...@gmail.com>
> Subject: [PATCH net-next] net: add a helper to avoid issues with HW TX
> timestamping and SO_TXTIME
>
> Caution: EXT Email
>
> As explained in commit 29d98f54a4fe ("net: enetc: allow hardware
> timestamping on TX queues with tc-etf enabled"), hardware TX timestamping
> requires an skb with skb->tstamp = 0. When a packet is sent with SO_TXTIME,
> the skb->skb_mstamp_ns corrupts the value of skb->tstamp, so the drivers need
> to explicitly reset skb->tstamp to zero after consuming the TX time.
>
> Create a helper named skb_txtime_consumed() which does just that. All drivers
> which offload TC_SETUP_QDISC_ETF should implement it, and it would make it
> easier to assess during review whether they do the right thing in order to be
> compatible with hardware timestamping or not.
>
> Suggested-by: Vinicius Costa Gomes <vinicius.go...@intel.com>
> Signed-off-by: Vladimir Oltean <vladimir.olt...@nxp.com>
> ---
> drivers/net/ethernet/freescale/enetc/enetc.c | 8 ++------
> drivers/net/ethernet/intel/igb/igb_main.c | 2 +-
> drivers/net/ethernet/intel/igc/igc_main.c | 2 +-
> include/net/pkt_sched.h | 9 +++++++++
> 4 files changed, 13 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/net/ethernet/freescale/enetc/enetc.c
> b/drivers/net/ethernet/freescale/enetc/enetc.c
> index e85dfccb9ed1..5a54976e6a28 100644
> --- a/drivers/net/ethernet/freescale/enetc/enetc.c
> +++ b/drivers/net/ethernet/freescale/enetc/enetc.c
> @@ -5,6 +5,7 @@
> #include <linux/tcp.h>
> #include <linux/udp.h>
> #include <linux/vmalloc.h>
> +#include <net/pkt_sched.h>
>
> /* ENETC overhead: optional extension BD + 1 BD gap */
> #define ENETC_TXBDS_NEEDED(val) ((val) + 2)
> @@ -293,12 +294,7 @@ 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_txtime_consumed(skb);
> skb_tstamp_tx(skb, &shhwtstamps);
> }
> }
> diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
> b/drivers/net/ethernet/intel/igb/igb_main.c
> index 878b31d534ec..369533feb4f2 100644
> --- a/drivers/net/ethernet/intel/igb/igb_main.c
> +++ b/drivers/net/ethernet/intel/igb/igb_main.c
> @@ -5856,7 +5856,7 @@ static void igb_tx_ctxtdesc(struct igb_ring *tx_ring,
> */
> if (tx_ring->launchtime_enable) {
> ts = ktime_to_timespec64(first->skb->tstamp);
> - first->skb->tstamp = ktime_set(0, 0);
> + skb_txtime_consumed(first->skb);
> context_desc->seqnum_seed = cpu_to_le32(ts.tv_nsec / 32);
> } else {
> context_desc->seqnum_seed = 0; diff --git
> a/drivers/net/ethernet/intel/igc/igc_main.c
> b/drivers/net/ethernet/intel/igc/igc_main.c
> index 7ac9597ddb84..059ffcfb0bda 100644
> --- a/drivers/net/ethernet/intel/igc/igc_main.c
> +++ b/drivers/net/ethernet/intel/igc/igc_main.c
> @@ -941,7 +941,7 @@ static void igc_tx_ctxtdesc(struct igc_ring *tx_ring,
> struct igc_adapter *adapter = netdev_priv(tx_ring->netdev);
> ktime_t txtime = first->skb->tstamp;
>
> - first->skb->tstamp = ktime_set(0, 0);
> + skb_txtime_consumed(first->skb);
> context_desc->launch_time = igc_tx_launchtime(adapter,
> txtime);
> } else {
> diff --git a/include/net/pkt_sched.h b/include/net/pkt_sched.h index
> 15b1b30f454e..f5c1bee0cd6a 100644
> --- a/include/net/pkt_sched.h
> +++ b/include/net/pkt_sched.h
> @@ -188,4 +188,13 @@ struct tc_taprio_qopt_offload
> *taprio_offload_get(struct tc_taprio_qopt_offload
> *offload); void
> taprio_offload_free(struct
> tc_taprio_qopt_offload *offload);
>
> +/* Ensure skb_mstamp_ns, which might have been populated with the
> +txtime, is
> + * not mistaken for a software timestamp, because this will otherwise
> +prevent
> + * the dispatch of hardware timestamps to the socket.
> + */
> +static inline void skb_txtime_consumed(struct sk_buff *skb) {
> + skb->tstamp = ktime_set(0, 0);
> +}
> +
> #endif
> --
> 2.25.1
Br,
Po Liu
