On Tue, Feb 11, 2025 at 6:40 AM Martin KaFai Lau <martin.lau@xxxxxxxxx> wrote: > > On 2/5/25 8:03 AM, Jason Xing wrote: > >>> @@ -5574,9 +5575,9 @@ static void skb_tstamp_tx_bpf(struct sk_buff *skb, struct sock *sk, > >>> op = BPF_SOCK_OPS_TS_SCHED_OPT_CB; > >>> break; > >>> case SCM_TSTAMP_SND: > >>> - if (!sw) > >>> - return; > >>> - op = BPF_SOCK_OPS_TS_SW_OPT_CB; > >>> + op = sw ? BPF_SOCK_OPS_TS_SW_OPT_CB : BPF_SOCK_OPS_TS_HW_OPT_CB; > >>> + if (!sw && hwtstamps) > >>> + *skb_hwtstamps(skb) = *hwtstamps; > >> Isn't this called by drivers that have actually set skb_hwtstamps? > > Oops, somehow my mind has gone blank 🙁 Will remove it. Thanks for > > correcting me! > > I just noticed I missed this thread when reviewing v9. > > I looked at a few drivers, e.g. the mlx5e_consume_skb(). It does not necessarily There are indeed many drivers behaving like you said: 1. xgbe_tx_tstamp() 2. aq_ptp_tx_hwtstamp() 3. bnx2x_ptp_task 4. i40e_ptp_tx_hwtstamp ... I really doubt that I've checked this a long time ago and then left this memory behind in V9, after all we've discussed this a lot of times... > set the skb_hwtstamps(skb) before calling skb_tstamp_tx(). The __skb_tstamp_tx() > is also setting skb_hwtstamps(skb) after testing "if (hwtstamps)", so I think This assignment is used to assign a cloned or newly allocated skb instead of the orig_skb passing from the driver side. > this assignment is still needed here? Right. Thanks, Jason
