peak_usb: fix hwtstamp assignment

PATCH In review Oliver Hartkopp 2 participants 2 messages Nov 22, 2012 CAN network drivers

Share & tools

Oliver Hartkopp

author

2012-11-22 18:44 UTC #0 lore raw

PATCH 1/1 peak_usb: fix hwtstamp assignment

The skb->tstamp is set to the hardware timestamp when available in the USB urb message. This leads to user visible timestamps which contain the 'uptime' of the USB adapter - and not the usual system generated timestamp.

Fix this wrong assignment by applying the available hardware timestamp to the skb_shared_hwtstamps data structure - which is intended for this purpose.

Signed-off-by Oliver Hartkopp <[email protected]>

---

Hello Marc, hello Stephane,

i think we would generally need to support a hwtstamp_ioctl() for CAN interfaces which are controlled with a struct hwtstamp_config in the future.

For now this patch fixes the current issue much better than a simple removal of the skb->tstamp assignment.

I'm currently on a business trip and i accidentally missed to take two PEAK USB interfaces with me to test this patch. It's compile tested ...

So let's wait for an Acked-by from Stephane.

Regards,
Oliver

drivers/net/can/usb/peak_usb/pcan_usb.c +6 -2

index 86f26a1..25723d8 100644
@@ -519,8 +519,10 @@ static int pcan_usb_decode_error(struct pcan_usb_msg_context *mc, u8 n,
 	mc->pdev->dev.can.state = new_state;
 
 	if (status_len & PCAN_USB_STATUSLEN_TIMESTAMP) {
+		struct skb_shared_hwtstamps *hwts = skb_hwtstamps(skb);
+
 		peak_usb_get_ts_tv(&mc->pdev->time_ref, mc->ts16, &tv);
-		skb->tstamp = timeval_to_ktime(tv);
+		hwts->hwtstamp = timeval_to_ktime(tv);
 	}
 
 	netif_rx(skb);

@@ -605,6 +607,7 @@ static int pcan_usb_decode_data(struct pcan_usb_msg_context *mc, u8 status_len)
 	struct sk_buff *skb;
 	struct can_frame *cf;
 	struct timeval tv;
+	struct skb_shared_hwtstamps *hwts;
 
 	skb = alloc_can_skb(mc->netdev, &cf);
 	if (!skb)

@@ -652,7 +655,8 @@ static int pcan_usb_decode_data(struct pcan_usb_msg_context *mc, u8 status_len)
 
 	/* convert timestamp into kernel time */
 	peak_usb_get_ts_tv(&mc->pdev->time_ref, mc->ts16, &tv);
-	skb->tstamp = timeval_to_ktime(tv);
+	hwts = skb_hwtstamps(skb);
+	hwts->hwtstamp = timeval_to_ktime(tv);
 
 	/* push the skb */
 	netif_rx(skb);

drivers/net/can/usb/peak_usb/pcan_usb_pro.c +6 -2

index e1626d9..30d79bf 100644
@@ -532,6 +532,7 @@ static int pcan_usb_pro_handle_canmsg(struct pcan_usb_pro_interface *usb_if,
 	struct can_frame *can_frame;
 	struct sk_buff *skb;
 	struct timeval tv;
+	struct skb_shared_hwtstamps *hwts;
 
 	skb = alloc_can_skb(netdev, &can_frame);
 	if (!skb)

@@ -549,7 +550,8 @@ static int pcan_usb_pro_handle_canmsg(struct pcan_usb_pro_interface *usb_if,
 		memcpy(can_frame->data, rx->data, can_frame->can_dlc);
 
 	peak_usb_get_ts_tv(&usb_if->time_ref, le32_to_cpu(rx->ts32), &tv);
-	skb->tstamp = timeval_to_ktime(tv);
+	hwts = skb_hwtstamps(skb);
+	hwts->hwtstamp = timeval_to_ktime(tv);
 
 	netif_rx(skb);
 	netdev->stats.rx_packets++;

@@ -570,6 +572,7 @@ static int pcan_usb_pro_handle_error(struct pcan_usb_pro_interface *usb_if,
 	u8 err_mask = 0;
 	struct sk_buff *skb;
 	struct timeval tv;
+	struct skb_shared_hwtstamps *hwts;
 
 	/* nothing should be sent while in BUS_OFF state */
 	if (dev->can.state == CAN_STATE_BUS_OFF)

@@ -664,7 +667,8 @@ static int pcan_usb_pro_handle_error(struct pcan_usb_pro_interface *usb_if,
 	dev->can.state = new_state;
 
 	peak_usb_get_ts_tv(&usb_if->time_ref, le32_to_cpu(er->ts32), &tv);
-	skb->tstamp = timeval_to_ktime(tv);
+	hwts = skb_hwtstamps(skb);
+	hwts->hwtstamp = timeval_to_ktime(tv);
 	netif_rx(skb);
 	netdev->stats.rx_packets++;
 	netdev->stats.rx_bytes += can_frame->can_dlc;

Marc Kleine-Budde

maintainer

2012-11-22 19:19 UTC #1 lore raw

Quoted 11 lines · Oliver Hartkopp

The skb->tstamp is set to the hardware timestamp when available in the USB urb message. This leads to user visible timestamps which contain the 'uptime' of the USB adapter - and not the usual system generated timestamp.
Fix this wrong assignment by applying the available hardware timestamp to the skb_shared_hwtstamps data structure - which is intended for this purpose.
Signed-off-by Oliver Hartkopp <[email protected]>
---

Quoted 7 lines

Hello Marc, hello Stephane,
i think we would generally need to support a hwtstamp_ioctl() for CAN interfaces which are controlled with a struct hwtstamp_config in the future.
For now this patch fixes the current issue much better than a simple removal of the skb->tstamp assignment.

Sure, making it work correctly is better than removing the broken stuff. But removing the broken code is better than having wrong code in the driver.

Quoted 2 lines

I'm currently on a business trip and i accidentally missed to take two PEAK USB interfaces with me to test this patch. It's compile tested ...

Quoted 1 line

So let's wait for an Acked-by from Stephane.

yep,
Marc

Signature · 4 lines

Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |