CAN message timestamping

CAN message timestamping

[Austin Schuh]

Hi,

We recently tried doing CAN timestamping with the ioctl(s, SIOCGSTAMP,
&tv) call.  It worked well, but all our timestamps internally are
based off the monotonic clock (since the realtime clock can jump and
go backwards in time.)  The conversion to the monotonic clock has been
tricky.  We already caught one bug today where it looks like our
simplistic sample both clocks and use that to compute the offset
algorithm was interrupted in the middle and resulted in the monotonic
time going backwards (which is wrong by definition).

Is there any way to get CAN timestamping with both clocks, or at least
with just the monotonic clock?  Or otherwise reliably get monotonic
time?

Thanks,
  Austin

Re: CAN message timestamping

[Oliver Hartkopp]

Hi Austin,

On 23.03.2016 06:12, Austin Schuh wrote:
> We recently tried doing CAN timestamping with the ioctl(s, SIOCGSTAMP,
> &tv) call.

Did you ever try to use the recvmsg() call with enabled SO_TIMESTAMPING socket 
option?

See:

https://www.kernel.org/doc/Documentation/networking/timestamping.txt
https://www.kernel.org/doc/Documentation/networking/timestamping/

The candump tool also uses recvmsg() - but only with SO_TIMESTAMP so far which 
does not provide the other RX timestamps (link HW TS).

Regards,
Oliver

Re: CAN message timestamping

[Austin Schuh]

Hi Oliver,

I spent some time trying recvmsg, but it still only gives me
timestamps with the real-time clock.  I do like the interface much
better.  Thanks!

I was able to get
  setsockopt(socket_, SOL_SOCKET, SO_TIMESTAMPNS,
      &enabled, sizeof(enabled))
and
  const int stamping_val = SOF_TIMESTAMPING_SOFTWARE |
      SOF_TIMESTAMPING_SYS_HARDWARE |
      SOF_TIMESTAMPING_RAW_HARDWARE;
  setsockopt(socket_, SOL_SOCKET, SO_TIMESTAMPING, &stamping_val,
      sizeof(stamping_val))
to successfully timestamp values with recvmsg.

I0328 18:25:09.927222 12739 can_data_source.cc:144] 0: 1459214709 927191637
I0328 18:25:09.927227 12739 can_data_source.cc:146] 1: 0 0
I0328 18:25:09.927233 12739 can_data_source.cc:148] 2: 0 0
I0328 18:25:09.927240 12739 can_data_source.cc:169] Capture time is
1459214709927191637ns

The time-stamp is definitely using the real-time clock.  The hardware
timestamps are not filled in.  Maybe I could modify the sja1000-peak
driver to abuse one of the hardware timestamps to return the timestamp
I am looking for?  I'd really rather not have to run a custom
kernel...

Austin

On Wed, Mar 23, 2016 at 12:00 AM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>
> Hi Austin,
>
> On 23.03.2016 06:12, Austin Schuh wrote:
> > We recently tried doing CAN timestamping with the ioctl(s, SIOCGSTAMP,
> > &tv) call.
>
> Did you ever try to use the recvmsg() call with enabled SO_TIMESTAMPING socket
> option?
>
> See:
>
> https://www.kernel.org/doc/Documentation/networking/timestamping.txt
> https://www.kernel.org/doc/Documentation/networking/timestamping/
>
> The candump tool also uses recvmsg() - but only with SO_TIMESTAMP so far which
> does not provide the other RX timestamps (link HW TS).
>
> Regards,
> Oliver
>

Re: CAN message timestamping

[Austin Schuh]

On Mon, Mar 28, 2016 at 6:51 PM, Austin Schuh <austin@peloton-tech.com> wrote:
> Hi Oliver,
>
> I spent some time trying recvmsg, but it still only gives me
> timestamps with the real-time clock.  I do like the interface much
> better.  Thanks!
>
> I was able to get
>   setsockopt(socket_, SOL_SOCKET, SO_TIMESTAMPNS,
>       &enabled, sizeof(enabled))
> and
>   const int stamping_val = SOF_TIMESTAMPING_SOFTWARE |
>       SOF_TIMESTAMPING_SYS_HARDWARE |
>       SOF_TIMESTAMPING_RAW_HARDWARE;
>   setsockopt(socket_, SOL_SOCKET, SO_TIMESTAMPING, &stamping_val,
>       sizeof(stamping_val))
> to successfully timestamp values with recvmsg.
>
> I0328 18:25:09.927222 12739 can_data_source.cc:144] 0: 1459214709 927191637
> I0328 18:25:09.927227 12739 can_data_source.cc:146] 1: 0 0
> I0328 18:25:09.927233 12739 can_data_source.cc:148] 2: 0 0
> I0328 18:25:09.927240 12739 can_data_source.cc:169] Capture time is
> 1459214709927191637ns
>
> The time-stamp is definitely using the real-time clock.  The hardware
> timestamps are not filled in.  Maybe I could modify the sja1000-peak
> driver to abuse one of the hardware timestamps to return the timestamp
> I am looking for?  I'd really rather not have to run a custom
> kernel...
>
> Austin
>
> On Wed, Mar 23, 2016 at 12:00 AM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>
>> Hi Austin,
>>
>> On 23.03.2016 06:12, Austin Schuh wrote:
>> > We recently tried doing CAN timestamping with the ioctl(s, SIOCGSTAMP,
>> > &tv) call.
>>
>> Did you ever try to use the recvmsg() call with enabled SO_TIMESTAMPING socket
>> option?
>>
>> See:
>>
>> https://www.kernel.org/doc/Documentation/networking/timestamping.txt
>> https://www.kernel.org/doc/Documentation/networking/timestamping/
>>
>> The candump tool also uses recvmsg() - but only with SO_TIMESTAMP so far which
>> does not provide the other RX timestamps (link HW TS).
>>
>> Regards,
>> Oliver
>>

Turns out timestamping in the driver is pretty easy.  The following
seems to be working for me.  (comments welcome!)  I don't think this
is something that should be up streamed, but I'm including it here in
case there is other interest.  I'm reading both clocks in the ISR to
reduce the amount of time difference between when they are both read.

$ git diff
diff --git a/drivers/net/can/sja1000/sja1000.c
b/drivers/net/can/sja1000/sja1000.c
index 76ef900..55d6583 100644
--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -370,6 +370,10 @@ static void sja1000_rx(struct net_device *dev)
        /* release receive buffer */
        sja1000_write_cmdreg(priv, CMD_RRB);

+       struct skb_shared_hwtstamps *shhwtstamps =
+           skb_hwtstamps(skb);
+       shhwtstamps->syststamp = ktime_get();
+       skb->tstamp = ktime_get_real();
        netif_rx(skb);

        stats->rx_packets++;

Austin

Re: CAN message timestamping

[Austin Schuh]

On Mon, Mar 28, 2016 at 8:42 PM, Austin Schuh <austin@peloton-tech.com> wrote:
> On Mon, Mar 28, 2016 at 6:51 PM, Austin Schuh <austin@peloton-tech.com> wrote:
>> Hi Oliver,
>>
>> I spent some time trying recvmsg, but it still only gives me
>> timestamps with the real-time clock.  I do like the interface much
>> better.  Thanks!
>>
>> I was able to get
>>   setsockopt(socket_, SOL_SOCKET, SO_TIMESTAMPNS,
>>       &enabled, sizeof(enabled))
>> and
>>   const int stamping_val = SOF_TIMESTAMPING_SOFTWARE |
>>       SOF_TIMESTAMPING_SYS_HARDWARE |
>>       SOF_TIMESTAMPING_RAW_HARDWARE;
>>   setsockopt(socket_, SOL_SOCKET, SO_TIMESTAMPING, &stamping_val,
>>       sizeof(stamping_val))
>> to successfully timestamp values with recvmsg.
>>
>> I0328 18:25:09.927222 12739 can_data_source.cc:144] 0: 1459214709 927191637
>> I0328 18:25:09.927227 12739 can_data_source.cc:146] 1: 0 0
>> I0328 18:25:09.927233 12739 can_data_source.cc:148] 2: 0 0
>> I0328 18:25:09.927240 12739 can_data_source.cc:169] Capture time is
>> 1459214709927191637ns
>>
>> The time-stamp is definitely using the real-time clock.  The hardware
>> timestamps are not filled in.  Maybe I could modify the sja1000-peak
>> driver to abuse one of the hardware timestamps to return the timestamp
>> I am looking for?  I'd really rather not have to run a custom
>> kernel...
>>
>> Austin
>>
>> On Wed, Mar 23, 2016 at 12:00 AM Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>>>
>>> Hi Austin,
>>>
>>> On 23.03.2016 06:12, Austin Schuh wrote:
>>> > We recently tried doing CAN timestamping with the ioctl(s, SIOCGSTAMP,
>>> > &tv) call.
>>>
>>> Did you ever try to use the recvmsg() call with enabled SO_TIMESTAMPING socket
>>> option?
>>>
>>> See:
>>>
>>> https://www.kernel.org/doc/Documentation/networking/timestamping.txt
>>> https://www.kernel.org/doc/Documentation/networking/timestamping/
>>>
>>> The candump tool also uses recvmsg() - but only with SO_TIMESTAMP so far which
>>> does not provide the other RX timestamps (link HW TS).
>>>
>>> Regards,
>>> Oliver
>>>
>
> Turns out timestamping in the driver is pretty easy.  The following
> seems to be working for me.  (comments welcome!)  I don't think this
> is something that should be up streamed, but I'm including it here in
> case there is other interest.  I'm reading both clocks in the ISR to
> reduce the amount of time difference between when they are both read.
>
> $ git diff
> diff --git a/drivers/net/can/sja1000/sja1000.c
> b/drivers/net/can/sja1000/sja1000.c
> index 76ef900..55d6583 100644
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -370,6 +370,10 @@ static void sja1000_rx(struct net_device *dev)
>         /* release receive buffer */
>         sja1000_write_cmdreg(priv, CMD_RRB);
>
> +       struct skb_shared_hwtstamps *shhwtstamps =
> +           skb_hwtstamps(skb);
> +       shhwtstamps->syststamp = ktime_get();
> +       skb->tstamp = ktime_get_real();
>         netif_rx(skb);
>
>         stats->rx_packets++;
>
> Austin

I missed the TX timestamping.  I didn't see a clean way to get access
to the echo skb.

--- a/drivers/net/can/sja1000/sja1000.c
+++ b/drivers/net/can/sja1000/sja1000.c
@@ -370,6 +370,12 @@ static void sja1000_rx(struct net_device *dev)
        /* release receive buffer */
        sja1000_write_cmdreg(priv, CMD_RRB);

+       {
+               struct skb_shared_hwtstamps *shhwtstamps =
+                       skb_hwtstamps(skb);
+               shhwtstamps->syststamp = ktime_get();
+               skb->tstamp = ktime_get_real();
+       }
        netif_rx(skb);

        stats->rx_packets++;
@@ -518,10 +524,19 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
                                stats->tx_errors++;
                                can_free_echo_skb(dev, 0);
                        } else {
+                               struct can_priv *can_priv_struct =
netdev_priv(dev);
                                /* transmission complete */
                                stats->tx_bytes +=
                                        priv->read_reg(priv, SJA1000_FI) & 0xf;
                                stats->tx_packets++;
+                               if (can_priv_struct->echo_skb[0]) {
+                                       struct sk_buff *skb =
can_priv_struct->echo_skb[0];
+                                       struct skb_shared_hwtstamps
*shhwtstamps =
+                                               skb_hwtstamps(skb);
+                                       shhwtstamps->syststamp = ktime_get();
+                                       skb->tstamp = ktime_get_real();
+                               }
+
                                can_get_echo_skb(dev, 0);
                        }
                        netif_wake_queue(dev);

Re: CAN message timestamping

[Oliver Hartkopp]

Hi Austin,

On 03/29/2016 06:28 AM, Austin Schuh wrote:
> On Mon, Mar 28, 2016 at 8:42 PM, Austin Schuh <austin@peloton-tech.com> wrote:
>> On Mon, Mar 28, 2016 at 6:51 PM, Austin Schuh <austin@peloton-tech.com> wrote:
>>> Hi Oliver,
>>>
>>> I spent some time trying recvmsg, but it still only gives me
>>> timestamps with the real-time clock.  I do like the interface much
>>> better.  Thanks!
>>>
>>> I was able to get
>>>    setsockopt(socket_, SOL_SOCKET, SO_TIMESTAMPNS,
>>>        &enabled, sizeof(enabled))
>>> and
>>>    const int stamping_val = SOF_TIMESTAMPING_SOFTWARE |
>>>        SOF_TIMESTAMPING_SYS_HARDWARE |
>>>        SOF_TIMESTAMPING_RAW_HARDWARE;
>>>    setsockopt(socket_, SOL_SOCKET, SO_TIMESTAMPING, &stamping_val,
>>>        sizeof(stamping_val))
>>> to successfully timestamp values with recvmsg.

Great!

>> Turns out timestamping in the driver is pretty easy.  The following
>> seems to be working for me.  (comments welcome!)  I don't think this
>> is something that should be up streamed, but I'm including it here in
>> case there is other interest.  I'm reading both clocks in the ISR to
>> reduce the amount of time difference between when they are both read.
>>
>> $ git diff
>> diff --git a/drivers/net/can/sja1000/sja1000.c
>> b/drivers/net/can/sja1000/sja1000.c
>> index 76ef900..55d6583 100644
>> --- a/drivers/net/can/sja1000/sja1000.c
>> +++ b/drivers/net/can/sja1000/sja1000.c
>> @@ -370,6 +370,10 @@ static void sja1000_rx(struct net_device *dev)
>>          /* release receive buffer */
>>          sja1000_write_cmdreg(priv, CMD_RRB);
>>
>> +       struct skb_shared_hwtstamps *shhwtstamps =
>> +           skb_hwtstamps(skb);
>> +       shhwtstamps->syststamp = ktime_get();
>> +       skb->tstamp = ktime_get_real();
>>          netif_rx(skb);
>>
>>          stats->rx_packets++;

Yes. I was also thinking about doing the timestamping directly at hw 
interrupt time.

The point is, that timestamping is an option.
The timestamping is only done if someone requires timestamps - an then 
it is done in the net-rx softirq (which is not that precise which is 
probably the reason for rx hardware timestamping).

> I missed the TX timestamping.  I didn't see a clean way to get access
> to the echo skb.
>
> --- a/drivers/net/can/sja1000/sja1000.c
> +++ b/drivers/net/can/sja1000/sja1000.c
> @@ -518,10 +524,19 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>                                  stats->tx_errors++;
>                                  can_free_echo_skb(dev, 0);
>                          } else {
> +                               struct can_priv *can_priv_struct =
> netdev_priv(dev);
>                                  /* transmission complete */
>                                  stats->tx_bytes +=
>                                          priv->read_reg(priv, SJA1000_FI) & 0xf;
>                                  stats->tx_packets++;
> +                               if (can_priv_struct->echo_skb[0]) {
> +                                       struct sk_buff *skb =
> can_priv_struct->echo_skb[0];
> +                                       struct skb_shared_hwtstamps
> *shhwtstamps =
> +                                               skb_hwtstamps(skb);
> +                                       shhwtstamps->syststamp = ktime_get();
> +                                       skb->tstamp = ktime_get_real();
> +                               }
> +
>                                  can_get_echo_skb(dev, 0);

I think a proper way would be to put this directly in 
can_get_echo_skb(), as poking into can_priv_struct->echo_skb[0] can just 
be a proof of concept for tx timstamping.

I'm fine with adding a more precise timestamping into CAN drivers but I 
wonder how other drivers implement this feature taking without killing 
the 'optional' feature.

Best regards,
Oliver

Re: CAN message timestamping

[Austin Schuh]

Hi Oliver,

On Wed, Mar 30, 2016 at 5:17 AM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> Hi Austin,
>
> On 03/29/2016 06:28 AM, Austin Schuh wrote:
>>
>> On Mon, Mar 28, 2016 at 8:42 PM, Austin Schuh <austin@peloton-tech.com>
>> wrote:
>>>
>>> On Mon, Mar 28, 2016 at 6:51 PM, Austin Schuh <austin@peloton-tech.com>
>>> wrote:
>>> Turns out timestamping in the driver is pretty easy.  The following
>>> seems to be working for me.  (comments welcome!)  I don't think this
>>> is something that should be up streamed, but I'm including it here in
>>> case there is other interest.  I'm reading both clocks in the ISR to
>>> reduce the amount of time difference between when they are both read.
>>>
>>> $ git diff
>>> diff --git a/drivers/net/can/sja1000/sja1000.c
>>> b/drivers/net/can/sja1000/sja1000.c
>>> index 76ef900..55d6583 100644
>>> --- a/drivers/net/can/sja1000/sja1000.c
>>> +++ b/drivers/net/can/sja1000/sja1000.c
>>> @@ -370,6 +370,10 @@ static void sja1000_rx(struct net_device *dev)
>>>          /* release receive buffer */
>>>          sja1000_write_cmdreg(priv, CMD_RRB);
>>>
>>> +       struct skb_shared_hwtstamps *shhwtstamps =
>>> +           skb_hwtstamps(skb);
>>> +       shhwtstamps->syststamp = ktime_get();
>>> +       skb->tstamp = ktime_get_real();
>>>          netif_rx(skb);
>>>
>>>          stats->rx_packets++;
>
>
> Yes. I was also thinking about doing the timestamping directly at hw
> interrupt time.
>
> The point is, that timestamping is an option.
> The timestamping is only done if someone requires timestamps - an then it is
> done in the net-rx softirq (which is not that precise which is probably the
> reason for rx hardware timestamping).

From what I see in the two drivers I've looked at so far, hardware
timestamping is done unconditionally.  Normal timestamping seems to be
done conditional on if SOCK_RCVTSTAMP is set on the sock and if it has
not already been timestamped.

drivers/net/can/usb/peak_usb/pcan_usb.c, in pcan_usb_decode_data, the
hardware timestamp is populated unconditionally.  I saw something
similar in the tg3 driver, though it was hidden behind an if statement
that I had trouble figuring out for sure when it was triggered.

I haven't checked to see if the bits are available where we need them,
but there are option bits attached to struct sock in net/core/sock.c
that signal if timestamping is required.

What do you think makes sense here?  I'm nervous about breaking things...

>
>> I missed the TX timestamping.  I didn't see a clean way to get access
>> to the echo skb.
>>
>> --- a/drivers/net/can/sja1000/sja1000.c
>> +++ b/drivers/net/can/sja1000/sja1000.c
>> @@ -518,10 +524,19 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>                                  stats->tx_errors++;
>>                                  can_free_echo_skb(dev, 0);
>>                          } else {
>> +                               struct can_priv *can_priv_struct =
>> netdev_priv(dev);
>>                                  /* transmission complete */
>>                                  stats->tx_bytes +=
>>                                          priv->read_reg(priv, SJA1000_FI)
>> & 0xf;
>>                                  stats->tx_packets++;
>> +                               if (can_priv_struct->echo_skb[0]) {
>> +                                       struct sk_buff *skb =
>> can_priv_struct->echo_skb[0];
>> +                                       struct skb_shared_hwtstamps
>> *shhwtstamps =
>> +                                               skb_hwtstamps(skb);
>> +                                       shhwtstamps->syststamp =
>> ktime_get();
>> +                                       skb->tstamp = ktime_get_real();
>> +                               }
>> +
>>                                  can_get_echo_skb(dev, 0);
>
>
> I think a proper way would be to put this directly in can_get_echo_skb(), as
> poking into can_priv_struct->echo_skb[0] can just be a proof of concept for
> tx timstamping.
>
> I'm fine with adding a more precise timestamping into CAN drivers but I
> wonder how other drivers implement this feature taking without killing the
> 'optional' feature.
>
> Best regards,
> Oliver

Works for me.  I don't like running a custom kernel when I don't need
to.  Before I put together a patch, let's figure out what makes sense.
I'm more than capable of writing the software, but my kernel internals
background and knowledge of best practices isn't very good.

Austin

Re: CAN message timestamping

[Oliver Hartkopp]

Hi Austin,

On 03/30/2016 06:50 PM, Austin Schuh wrote:
> Hi Oliver,
>
> On Wed, Mar 30, 2016 at 5:17 AM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
>> Hi Austin,
>>
>> On 03/29/2016 06:28 AM, Austin Schuh wrote:
>>>
>>> On Mon, Mar 28, 2016 at 8:42 PM, Austin Schuh <austin@peloton-tech.com>
>>> wrote:
>>>>
>>>> On Mon, Mar 28, 2016 at 6:51 PM, Austin Schuh <austin@peloton-tech.com>
>>>> wrote:
>>>> Turns out timestamping in the driver is pretty easy.  The following
>>>> seems to be working for me.  (comments welcome!)  I don't think this
>>>> is something that should be up streamed, but I'm including it here in
>>>> case there is other interest.  I'm reading both clocks in the ISR to
>>>> reduce the amount of time difference between when they are both read.
>>>>
>>>> $ git diff
>>>> diff --git a/drivers/net/can/sja1000/sja1000.c
>>>> b/drivers/net/can/sja1000/sja1000.c
>>>> index 76ef900..55d6583 100644
>>>> --- a/drivers/net/can/sja1000/sja1000.c
>>>> +++ b/drivers/net/can/sja1000/sja1000.c
>>>> @@ -370,6 +370,10 @@ static void sja1000_rx(struct net_device *dev)
>>>>           /* release receive buffer */
>>>>           sja1000_write_cmdreg(priv, CMD_RRB);
>>>>
>>>> +       struct skb_shared_hwtstamps *shhwtstamps =
>>>> +           skb_hwtstamps(skb);
>>>> +       shhwtstamps->syststamp = ktime_get();
>>>> +       skb->tstamp = ktime_get_real();
>>>>           netif_rx(skb);
>>>>
>>>>           stats->rx_packets++;
>>
>>
>> Yes. I was also thinking about doing the timestamping directly at hw
>> interrupt time.
>>
>> The point is, that timestamping is an option.
>> The timestamping is only done if someone requires timestamps - an then it is
>> done in the net-rx softirq (which is not that precise which is probably the
>> reason for rx hardware timestamping).
>
>  From what I see in the two drivers I've looked at so far, hardware
> timestamping is done unconditionally.  Normal timestamping seems to be
> done conditional on if SOCK_RCVTSTAMP is set on the sock and if it has
> not already been timestamped.

The fact that HW timstamping is done unconditionally should be discussed 
separately. E.g. by some net_timestamp_needed() function which would 
have to be introduced in linux/net/core/dev.c ...

> drivers/net/can/usb/peak_usb/pcan_usb.c, in pcan_usb_decode_data, the
> hardware timestamp is populated unconditionally.  I saw something
> similar in the tg3 driver, though it was hidden behind an if statement
> that I had trouble figuring out for sure when it was triggered.
>
> I haven't checked to see if the bits are available where we need them,
> but there are option bits attached to struct sock in net/core/sock.c
> that signal if timestamping is required.
>
> What do you think makes sense here?  I'm nervous about breaking things...

See above - I would vote for a local solution first.

>>
>>> I missed the TX timestamping.  I didn't see a clean way to get access
>>> to the echo skb.
>>>
>>> --- a/drivers/net/can/sja1000/sja1000.c
>>> +++ b/drivers/net/can/sja1000/sja1000.c
>>> @@ -518,10 +524,19 @@ irqreturn_t sja1000_interrupt(int irq, void *dev_id)
>>>                                   stats->tx_errors++;
>>>                                   can_free_echo_skb(dev, 0);
>>>                           } else {
>>> +                               struct can_priv *can_priv_struct =
>>> netdev_priv(dev);
>>>                                   /* transmission complete */
>>>                                   stats->tx_bytes +=
>>>                                           priv->read_reg(priv, SJA1000_FI)
>>> & 0xf;
>>>                                   stats->tx_packets++;
>>> +                               if (can_priv_struct->echo_skb[0]) {
>>> +                                       struct sk_buff *skb =
>>> can_priv_struct->echo_skb[0];
>>> +                                       struct skb_shared_hwtstamps
>>> *shhwtstamps =
>>> +                                               skb_hwtstamps(skb);
>>> +                                       shhwtstamps->syststamp =
>>> ktime_get();
>>> +                                       skb->tstamp = ktime_get_real();
>>> +                               }
>>> +
>>>                                   can_get_echo_skb(dev, 0);
>>
>>
>> I think a proper way would be to put this directly in can_get_echo_skb(), as
>> poking into can_priv_struct->echo_skb[0] can just be a proof of concept for
>> tx timstamping.
>>

(..)

> Works for me.

Not for me :-)

> I don't like running a custom kernel when I don't need
> to.  Before I put together a patch, let's figure out what makes sense.
> I'm more than capable of writing the software, but my kernel internals
> background and knowledge of best practices isn't very good.

I wonder if it makes sense to create a helper function to set the 
timestamps following your suggestion and call this function in 
can_get_echo_skb().

Regards,
Oliver

Re: CAN message timestamping

[Austin Schuh]

On Mon, Apr 4, 2016 at 12:45 PM, Oliver Hartkopp <socketcan@hartkopp.net> wrote:
> Hi Austin,
>
>
> On 03/30/2016 06:50 PM, Austin Schuh wrote:
>>
>> Hi Oliver,
>>
>> On Wed, Mar 30, 2016 at 5:17 AM, Oliver Hartkopp <socketcan@hartkopp.net>
>> wrote:
>>>
>>> Hi Austin,
>>>
>>> On 03/29/2016 06:28 AM, Austin Schuh wrote:
>>>>
>>>>
>>>> On Mon, Mar 28, 2016 at 8:42 PM, Austin Schuh <austin@peloton-tech.com>
>>>> wrote:
>>>>>
>>>>>
>>>>> On Mon, Mar 28, 2016 at 6:51 PM, Austin Schuh <austin@peloton-tech.com>
>>>>> wrote:
>>>>> Turns out timestamping in the driver is pretty easy.  The following
>>>>> seems to be working for me.  (comments welcome!)  I don't think this
>>>>> is something that should be up streamed, but I'm including it here in
>>>>> case there is other interest.  I'm reading both clocks in the ISR to
>>>>> reduce the amount of time difference between when they are both read.
>>>>>
>>>>> $ git diff
>>>>> diff --git a/drivers/net/can/sja1000/sja1000.c
>>>>> b/drivers/net/can/sja1000/sja1000.c
>>>>> index 76ef900..55d6583 100644
>>>>> --- a/drivers/net/can/sja1000/sja1000.c
>>>>> +++ b/drivers/net/can/sja1000/sja1000.c
>>>>> @@ -370,6 +370,10 @@ static void sja1000_rx(struct net_device *dev)
>>>>>           /* release receive buffer */
>>>>>           sja1000_write_cmdreg(priv, CMD_RRB);
>>>>>
>>>>> +       struct skb_shared_hwtstamps *shhwtstamps =
>>>>> +           skb_hwtstamps(skb);
>>>>> +       shhwtstamps->syststamp = ktime_get();
>>>>> +       skb->tstamp = ktime_get_real();
>>>>>           netif_rx(skb);
>>>>>
>>>>>           stats->rx_packets++;
>>>
>>>
>>>
>>> Yes. I was also thinking about doing the timestamping directly at hw
>>> interrupt time.
>>>
>>> The point is, that timestamping is an option.
>>> The timestamping is only done if someone requires timestamps - an then it
>>> is
>>> done in the net-rx softirq (which is not that precise which is probably
>>> the
>>> reason for rx hardware timestamping).
>>
>>
>>  From what I see in the two drivers I've looked at so far, hardware
>> timestamping is done unconditionally.  Normal timestamping seems to be
>> done conditional on if SOCK_RCVTSTAMP is set on the sock and if it has
>> not already been timestamped.
>
>
> The fact that HW timstamping is done unconditionally should be discussed
> separately. E.g. by some net_timestamp_needed() function which would have to
> be introduced in linux/net/core/dev.c ...

Sounds reasonable.  I'll implement a RFC without this part in the next
week or so and send it out.  Once I've got something that's a bit
better than what I have now, I'll send it out.

>> drivers/net/can/usb/peak_usb/pcan_usb.c, in pcan_usb_decode_data, the
>> hardware timestamp is populated unconditionally.  I saw something
>> similar in the tg3 driver, though it was hidden behind an if statement
>> that I had trouble figuring out for sure when it was triggered.
>>
>> I haven't checked to see if the bits are available where we need them,
>> but there are option bits attached to struct sock in net/core/sock.c
>> that signal if timestamping is required.
>>
>> What do you think makes sense here?  I'm nervous about breaking things...
>
>
> See above - I would vote for a local solution first.
>
>
>>>
>>>> I missed the TX timestamping.  I didn't see a clean way to get access
>>>> to the echo skb.
>>>>
>>>> --- a/drivers/net/can/sja1000/sja1000.c
>>>> +++ b/drivers/net/can/sja1000/sja1000.c
>>>> @@ -518,10 +524,19 @@ irqreturn_t sja1000_interrupt(int irq, void
>>>> *dev_id)
>>>>                                   stats->tx_errors++;
>>>>                                   can_free_echo_skb(dev, 0);
>>>>                           } else {
>>>> +                               struct can_priv *can_priv_struct =
>>>> netdev_priv(dev);
>>>>                                   /* transmission complete */
>>>>                                   stats->tx_bytes +=
>>>>                                           priv->read_reg(priv,
>>>> SJA1000_FI)
>>>> & 0xf;
>>>>                                   stats->tx_packets++;
>>>> +                               if (can_priv_struct->echo_skb[0]) {
>>>> +                                       struct sk_buff *skb =
>>>> can_priv_struct->echo_skb[0];
>>>> +                                       struct skb_shared_hwtstamps
>>>> *shhwtstamps =
>>>> +                                               skb_hwtstamps(skb);
>>>> +                                       shhwtstamps->syststamp =
>>>> ktime_get();
>>>> +                                       skb->tstamp = ktime_get_real();
>>>> +                               }
>>>> +
>>>>                                   can_get_echo_skb(dev, 0);
>>>
>>>
>>>
>>> I think a proper way would be to put this directly in can_get_echo_skb(),
>>> as
>>> poking into can_priv_struct->echo_skb[0] can just be a proof of concept
>>> for
>>> tx timstamping.
>>>
>
> (..)
>
>> Works for me.
>
>
> Not for me :-)
>
>> I don't like running a custom kernel when I don't need
>> to.  Before I put together a patch, let's figure out what makes sense.
>> I'm more than capable of writing the software, but my kernel internals
>> background and knowledge of best practices isn't very good.
>
>
> I wonder if it makes sense to create a helper function to set the timestamps
> following your suggestion and call this function in can_get_echo_skb().
>
> Regards,
> Oliver
>

Ok!  I *think* I have enough to put together a first patch for
comments and test it on my SJA1000 and on vcan.  Thanks!  If you have
other thoughts, please let me know.

Austin

Re: CAN message timestamping

[Austin Schuh]

On Mon, Apr 4, 2016 at 1:32 PM, Austin Schuh <austin@peloton-tech.com> wrote:
> Ok!  I *think* I have enough to put together a first patch for
> comments and test it on my SJA1000 and on vcan.  Thanks!  If you have
> other thoughts, please let me know.

Turns out  the entire infrastructure has been deprecated in newer
kernels, and we didn't notice on our old 3.14 kernel (RT patches and
the validation associated with change have kept us from upgrading).
We had to roll back the 2 patches which removed support for the
timestamps from our 4.1 RT kernel in order to make this work there.

I think that kind of kills the idea, unless you are more clever than us.

Austin