Re: CAN message timestamping

[Oliver Hartkopp <socketcan@hartkopp.net>]

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