diff mbox series

[v3,1/1] can: dev: add software tx timestamps

Message ID 20210110124903.109773-2-mailhol.vincent@wanadoo.fr
State New
Headers show
Series Add software TX timestamps to the CAN devices | expand

Commit Message

Vincent MAILHOL Jan. 10, 2021, 12:49 p.m. UTC
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(+)

Comments

Marc Kleine-Budde Jan. 11, 2021, 8:10 a.m. UTC | #1
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 |
Richard Cochran Jan. 11, 2021, 5:11 p.m. UTC | #2
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
Vincent MAILHOL Jan. 12, 2021, midnight UTC | #3
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
Richard Cochran Jan. 12, 2021, 2:14 a.m. UTC | #4
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
Vincent MAILHOL Jan. 12, 2021, 6:38 a.m. UTC | #5
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
Marc Kleine-Budde Jan. 12, 2021, 7:58 a.m. UTC | #6
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 |
Vincent MAILHOL Jan. 12, 2021, 9:58 a.m. UTC | #7
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 mbox series

Patch

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;