On Mon. 11 Jan 2021 at 19:45, Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> wrote: > > From: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > > Call skb_tx_timestamp() within can_put_echo_skb() so that a software > tx timestamp gets attached on the skb. > > There two main reasons to include this call in can_put_echo_skb(): > > * It easily allow to enable the tx timestamp on all devices with > just one small change. > > * According to Documentation/networking/timestamping.rst, the tx > timestamps should be generated in the device driver as close as > possible, but always prior to passing the packet to the network > interface. During the call to can_put_echo_skb(), the skb gets > cloned meaning that the driver should not dereference the skb > variable anymore after can_put_echo_skb() returns. This makes > can_put_echo_skb() the very last place we can use the skb without > having to access the echo_skb[] array. > > Remarks: > > * By default, skb_tx_timestamp() does nothing. It needs to be > activated by passing the SOF_TIMESTAMPING_TX_SOFTWARE flag either > through socket options or control messages. > > * The hardware rx timestamp of a local loopback message is the > hardware tx timestamp. This means that there are no needs to > implement SOF_TIMESTAMPING_TX_HARDWARE for CAN sockets. > > References: > > Support for the error queue in CAN RAW sockets (which is needed for tx > timestamps) was introduced in: > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=eb88531bdbfaafb827192d1fc6c5a3fcc4fadd96 > > Put the call to skb_tx_timestamp() just before adding it to the array: > https://lkml.org/lkml/2021/1/10/54 > > Signed-off-by: Vincent Mailhol <mailhol.vincent@xxxxxxxxxx> > Link: https://lore.kernel.org/r/20210110124903.109773-2-mailhol.vincent@xxxxxxxxxx > Signed-off-by: Marc Kleine-Budde <mkl@xxxxxxxxxxxxxx> > --- > drivers/net/can/dev/skb.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/net/can/dev/skb.c b/drivers/net/can/dev/skb.c > index 53683d4312f1..f858f994cd49 100644 > --- a/drivers/net/can/dev/skb.c > +++ b/drivers/net/can/dev/skb.c > @@ -61,6 +61,7 @@ int can_put_echo_skb(struct sk_buff *skb, struct net_device *dev, > skb->pkt_type = PACKET_BROADCAST; > skb->ip_summed = CHECKSUM_UNNECESSARY; > skb->dev = dev; > + skb_tx_timestamp(skb); > > /* save frame_len to reuse it when transmission is completed */ > can_skb_prv(skb)->frame_len = frame_len; > -- > 2.29.2 > > A small detail, I would prefer to see the call to skb_tx_timestamp() at the bottom just before we put the skb in the array. The reason is that the later we do the timestamp, the better it is. /* make settings for echo to reduce code in irq context */ skb->pkt_type = PACKET_BROADCAST; skb->ip_summed = CHECKSUM_UNNECESSARY; skb->dev = dev; /* save frame_len to reuse it when transmission is completed */ can_skb_prv(skb)->frame_len = frame_len; + skb_tx_timestamp(skb); /* save this skb for tx interrupt echo handling */ priv->echo_skb[idx] = skb; But it does not really matter. Yours sincerely, Vincent
