From mboxrd@z Thu Jan 1 00:00:00 1970 From: Oliver Hartkopp Subject: Re: CAN message timestamping Date: Mon, 4 Apr 2016 21:45:00 +0200 Message-ID: <5702C43C.4000506@hartkopp.net> References: <56F23F1F.6040101@hartkopp.net> <56FBC3C4.80400@hartkopp.net> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Return-path: Received: from mo4-p00-ob.smtp.rzone.de ([81.169.146.220]:30954 "EHLO mo4-p00-ob.smtp.rzone.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754749AbcDDTpO (ORCPT ); Mon, 4 Apr 2016 15:45:14 -0400 In-Reply-To: Sender: linux-can-owner@vger.kernel.org List-ID: 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 wrote: >> Hi Austin, >> >> On 03/29/2016 06:28 AM, Austin Schuh wrote: >>> >>> On Mon, Mar 28, 2016 at 8:42 PM, Austin Schuh >>> wrote: >>>> >>>> On Mon, Mar 28, 2016 at 6:51 PM, Austin Schuh >>>> 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