* CAN message timestamping @ 2016-03-23 5:12 Austin Schuh 2016-03-23 7:00 ` Oliver Hartkopp 0 siblings, 1 reply; 10+ messages in thread From: Austin Schuh @ 2016-03-23 5:12 UTC (permalink / raw) To: linux-can, Philipp Schrader 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CAN message timestamping 2016-03-23 5:12 CAN message timestamping Austin Schuh @ 2016-03-23 7:00 ` Oliver Hartkopp 2016-03-29 1:51 ` Austin Schuh 0 siblings, 1 reply; 10+ messages in thread From: Oliver Hartkopp @ 2016-03-23 7:00 UTC (permalink / raw) To: Austin Schuh, linux-can, Philipp Schrader 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CAN message timestamping 2016-03-23 7:00 ` Oliver Hartkopp @ 2016-03-29 1:51 ` Austin Schuh 2016-03-29 3:42 ` Austin Schuh 0 siblings, 1 reply; 10+ messages in thread From: Austin Schuh @ 2016-03-29 1:51 UTC (permalink / raw) To: Oliver Hartkopp, linux-can, Philipp Schrader 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 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CAN message timestamping 2016-03-29 1:51 ` Austin Schuh @ 2016-03-29 3:42 ` Austin Schuh 2016-03-29 4:28 ` Austin Schuh 0 siblings, 1 reply; 10+ messages in thread From: Austin Schuh @ 2016-03-29 3:42 UTC (permalink / raw) To: Oliver Hartkopp, linux-can, Philipp Schrader 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 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: CAN message timestamping 2016-03-29 3:42 ` Austin Schuh @ 2016-03-29 4:28 ` Austin Schuh 2016-03-30 12:17 ` Oliver Hartkopp 0 siblings, 1 reply; 10+ messages in thread From: Austin Schuh @ 2016-03-29 4:28 UTC (permalink / raw) To: Oliver Hartkopp, linux-can, Philipp Schrader 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); ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CAN message timestamping 2016-03-29 4:28 ` Austin Schuh @ 2016-03-30 12:17 ` Oliver Hartkopp 2016-03-30 16:50 ` Austin Schuh 0 siblings, 1 reply; 10+ messages in thread From: Oliver Hartkopp @ 2016-03-30 12:17 UTC (permalink / raw) To: Austin Schuh, linux-can, Philipp Schrader 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CAN message timestamping 2016-03-30 12:17 ` Oliver Hartkopp @ 2016-03-30 16:50 ` Austin Schuh 2016-04-04 19:45 ` Oliver Hartkopp 0 siblings, 1 reply; 10+ messages in thread From: Austin Schuh @ 2016-03-30 16:50 UTC (permalink / raw) To: Oliver Hartkopp; +Cc: linux-can, Philipp Schrader 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CAN message timestamping 2016-03-30 16:50 ` Austin Schuh @ 2016-04-04 19:45 ` Oliver Hartkopp 2016-04-04 20:32 ` Austin Schuh 0 siblings, 1 reply; 10+ messages in thread From: Oliver Hartkopp @ 2016-04-04 19:45 UTC (permalink / raw) To: Austin Schuh; +Cc: linux-can, Philipp Schrader 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CAN message timestamping 2016-04-04 19:45 ` Oliver Hartkopp @ 2016-04-04 20:32 ` Austin Schuh 2016-05-12 2:07 ` Austin Schuh 0 siblings, 1 reply; 10+ messages in thread From: Austin Schuh @ 2016-04-04 20:32 UTC (permalink / raw) To: Oliver Hartkopp; +Cc: linux-can, Philipp Schrader 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: CAN message timestamping 2016-04-04 20:32 ` Austin Schuh @ 2016-05-12 2:07 ` Austin Schuh 0 siblings, 0 replies; 10+ messages in thread From: Austin Schuh @ 2016-05-12 2:07 UTC (permalink / raw) To: Oliver Hartkopp; +Cc: linux-can, Philipp Schrader 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 ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2016-05-12 2:07 UTC | newest] Thread overview: 10+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2016-03-23 5:12 CAN message timestamping Austin Schuh 2016-03-23 7:00 ` Oliver Hartkopp 2016-03-29 1:51 ` Austin Schuh 2016-03-29 3:42 ` Austin Schuh 2016-03-29 4:28 ` Austin Schuh 2016-03-30 12:17 ` Oliver Hartkopp 2016-03-30 16:50 ` Austin Schuh 2016-04-04 19:45 ` Oliver Hartkopp 2016-04-04 20:32 ` Austin Schuh 2016-05-12 2:07 ` Austin Schuh
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).