| Message ID | 20210110124903.109773-2-mailhol.vincent@wanadoo.fr |
|---|---|
| State | New |
| Headers | show |
| Series | Add software TX timestamps to the CAN devices | expand |
On 1/10/21 1:49 PM, Vincent Mailhol wrote: > 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@wanadoo.fr> Applied to linux-can-next/testing (ontop of my dev infrastructure cleanup). regards, Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung West/Dortmund | Phone: +49-231-2826-924 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Sun, Jan 10, 2021 at 09:49:03PM +0900, Vincent Mailhol wrote: > * 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. I can't agree with that statement. The local loopback is a special "feature" of CAN sockets, and some programs turn it off. Furthermore, requiring user space to handle CAN sockets differently WRT Tx time stamps is user-unfriendly. So I would strongly support adding SOF_TIMESTAMPING_TX_HARDWARE to the CAN layer in the future. (This isn't a criticism of the current patch, though.) Thanks, Richard
On Tue. 12 Jan 2021 at 02:11, Richard Cochran <richardcochran@gmail.com> wrote: > > On Sun, Jan 10, 2021 at 09:49:03PM +0900, Vincent Mailhol wrote: > > * 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. > > I can't agree with that statement. The local loopback is a special > "feature" of CAN sockets, and some programs turn it off. Furthermore, > requiring user space to handle CAN sockets differently WRT Tx time > stamps is user-unfriendly. So I would strongly support adding > SOF_TIMESTAMPING_TX_HARDWARE to the CAN layer in the future. > > (This isn't a criticism of the current patch, though.) Fair enough. Implementing SOF_TIMESTAMPING_TX_HARDWARE would result into having the timestamp being duplicated for the loopback frames but allowing existing programs to work as with no modifications is a good enough reason. Out of curiosity, which programs do you use? I guess wireshark but please let me know if you use any other programs (I just use to write a small C program to do the stuff). Mark: do you want me to send a v4 of that patch with above comment removed or can you directly do the change in your testing branch? Yours sincerely, Vincent
On Tue, Jan 12, 2021 at 09:00:33AM +0900, Vincent MAILHOL wrote: > Out of curiosity, which programs do you use? I guess wireshark > but please let me know if you use any other programs (I just use > to write a small C program to do the stuff). I was thinking of PTP over DeviceNET (which, in turn, is over CAN). This is specified in Annex G of IEEE 1588. The linuxptp stack has modular design and could one day support DeviceNET. It would be much easier for linuxptp if CAN interfaces support hardware time stamping in the same way as other network interfaces. Thanks, Richard
On Tue. 12 Jan 2021 at 11:14, Richard Cochran <richardcochran@gmail.com> wrote: > > On Tue, Jan 12, 2021 at 09:00:33AM +0900, Vincent MAILHOL wrote: > > Out of curiosity, which programs do you use? I guess wireshark > > but please let me know if you use any other programs (I just use > > to write a small C program to do the stuff). > > I was thinking of PTP over DeviceNET (which, in turn, is over CAN). > This is specified in Annex G of IEEE 1588. > > The linuxptp stack has modular design and could one day support > DeviceNET. It would be much easier for linuxptp if CAN interfaces > support hardware time stamping in the same way as other network > interfaces. I actually also thought of implementing PTP but for a slightly different goal: synchronise the clock of the different CAN controllers connected to a same Linux host. But so far, it is just a rough idea and I never looked really deep into the technical details of linuxptp. However, I did not know about DeviceNET. I am not really aware of the use cases for industrial automation applications. Glad to have asked, learnt something :) Yours sincerely, Vincent
On 1/12/21 1:00 AM, Vincent MAILHOL wrote: [...] > Mark: do you want me to send a v4 of that patch with above > comment removed or can you directly do the change in your testing > branch? Please send a patch on-top of linux-can-next/testing Marc -- Pengutronix e.K. | Marc Kleine-Budde | Embedded Linux | https://www.pengutronix.de | Vertretung West/Dortmund | Phone: +49-231-2826-924 | Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
On Tue. 12 Jan 2021 at 16:58, Marc Kleine-Budde <mkl@pengutronix.de> wrote: > > On 1/12/21 1:00 AM, Vincent MAILHOL wrote: > [...] > > > Mark: do you want me to send a v4 of that patch with above > > comment removed or can you directly do the change in your testing > > branch? > > Please send a patch on-top of linux-can-next/testing Done: https://lore.kernel.org/linux-can/20210112095437.6488-1-mailhol.vincent@wanadoo.fr/ Yours sincerely, Vincent
diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c index 3486704c8a95..850759c7677f 100644 --- a/drivers/net/can/dev.c +++ b/drivers/net/can/dev.c @@ -481,6 +481,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 this skb for tx interrupt echo handling */ priv->echo_skb[idx] = skb;
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@wanadoo.fr> --- drivers/net/can/dev.c | 1 + 1 file changed, 1 insertion(+)