(no subject)

[Marc Kleine-Budde]

Subject: [PATCH 0/3] can: gs_usb: fix hardware timestamping issues
In-Reply-To:

Hello,

while playing around with the hardware timestamping code, I found some
issues, which are fixed in this series.

John, can you test if timestamping still works on your hardware.

regards,
Marc

[PATCH 1/3] can: gs_usb: gs_usb_get_timestamp(): fix endpoint parameter for usb_control_msg_recv()

[Marc Kleine-Budde]

The 2nd argument of usb_control_msg_recv() is the "endpoint",
usb_control_msg_recv() will internally convert the endpoint into a
pipe with usb_rcvctrlpipe().

In gs_usb_get_timestamp() not the endpoint "0" is passed, but the
pipe. This worked by accident as endpoint is a __u8 and the lowest 8
bits of the pipe are 0. Fix this copy/paste error by using the correct
endpoint of "0".

Fixes: 45dfa45f52e6 ("can: gs_usb: add RX and TX hardware timestamp support")
Cc: John Whittington <git@jbrengineering.co.uk>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 5ba159f49c4e..12e7437a9496 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -386,8 +386,7 @@ static inline int gs_usb_get_timestamp(const struct gs_can *dev,
 	__le32 timestamp;
 	int rc;
 
-	rc = usb_control_msg_recv(interface_to_usbdev(dev->iface),
-				  usb_sndctrlpipe(interface_to_usbdev(dev->iface), 0),
+	rc = usb_control_msg_recv(interface_to_usbdev(dev->iface), 0,
 				  GS_USB_BREQ_TIMESTAMP,
 				  USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_INTERFACE,
 				  dev->channel, 0,
-- 
2.35.1

[PATCH 2/3] can: gs_usb: add missing lock to protect struct timecounter::cycle_last

[Marc Kleine-Budde]

The struct timecounter::cycle_last is a 64 bit variable, read by
timecounter_cyc2time(), and written by timecounter_read(). On 32 bit
architectures this is not atomic.

Add a spinlock to protect access to struct timecounter::cycle_last. In
the gs_usb_timestamp_read() callback the lock is dropped to execute a
sleeping synchronous USB transfer. This is safe, as the variable we
want to protect is accessed during this call.

Fixes: 45dfa45f52e6 ("can: gs_usb: add RX and TX hardware timestamp support")
Cc: John Whittington <git@jbrengineering.co.uk>
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index 12e7437a9496..fe4116bf925b 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -286,6 +286,7 @@ struct gs_can {
 	/* time counter for hardware timestamps */
 	struct cyclecounter cc;
 	struct timecounter tc;
+	spinlock_t tc_lock; /* spinlock to guard access tc->cycle_last */
 	struct delayed_work timestamp;
 
 	u32 feature;
@@ -401,14 +402,18 @@ static inline int gs_usb_get_timestamp(const struct gs_can *dev,
 	return 0;
 }
 
-static u64 gs_usb_timestamp_read(const struct cyclecounter *cc)
+static u64 gs_usb_timestamp_read(const struct cyclecounter *cc) __must_hold(&dev->tc_lock)
 {
-	const struct gs_can *dev;
+	struct gs_can *dev = container_of(cc, struct gs_can, cc);
 	u32 timestamp = 0;
 	int err;
 
-	dev = container_of(cc, struct gs_can, cc);
+	lockdep_assert_held(&dev->tc_lock);
+
+	/* drop lock for synchronous USB transfer */
+	spin_unlock_bh(&dev->tc_lock);
 	err = gs_usb_get_timestamp(dev, &timestamp);
+	spin_lock_bh(&dev->tc_lock);
 	if (err)
 		netdev_err(dev->netdev,
 			   "Error %d while reading timestamp. HW timestamps may be inaccurate.",
@@ -423,19 +428,24 @@ static void gs_usb_timestamp_work(struct work_struct *work)
 	struct gs_can *dev;
 
 	dev = container_of(delayed_work, struct gs_can, timestamp);
+	spin_lock_bh(&dev->tc_lock);
 	timecounter_read(&dev->tc);
+	spin_unlock_bh(&dev->tc_lock);
 
 	schedule_delayed_work(&dev->timestamp,
 			      GS_USB_TIMESTAMP_WORK_DELAY_SEC * HZ);
 }
 
-static void gs_usb_skb_set_timestamp(const struct gs_can *dev,
+static void gs_usb_skb_set_timestamp(struct gs_can *dev,
 				     struct sk_buff *skb, u32 timestamp)
 {
 	struct skb_shared_hwtstamps *hwtstamps = skb_hwtstamps(skb);
 	u64 ns;
 
+	spin_lock_bh(&dev->tc_lock);
 	ns = timecounter_cyc2time(&dev->tc, timestamp);
+	spin_unlock_bh(&dev->tc_lock);
+
 	hwtstamps->hwtstamp = ns_to_ktime(ns);
 }
 
@@ -448,7 +458,10 @@ static void gs_usb_timestamp_init(struct gs_can *dev)
 	cc->shift = 32 - bits_per(NSEC_PER_SEC / GS_USB_TIMESTAMP_TIMER_HZ);
 	cc->mult = clocksource_hz2mult(GS_USB_TIMESTAMP_TIMER_HZ, cc->shift);
 
+	spin_lock_init(&dev->tc_lock);
+	spin_lock_bh(&dev->tc_lock);
 	timecounter_init(&dev->tc, &dev->cc, ktime_get_real_ns());
+	spin_unlock_bh(&dev->tc_lock);
 
 	INIT_DELAYED_WORK(&dev->timestamp, gs_usb_timestamp_work);
 	schedule_delayed_work(&dev->timestamp,
@@ -485,7 +498,7 @@ static void gs_update_state(struct gs_can *dev, struct can_frame *cf)
 	}
 }
 
-static void gs_usb_set_timestamp(const struct gs_can *dev, struct sk_buff *skb,
+static void gs_usb_set_timestamp(struct gs_can *dev, struct sk_buff *skb,
 				 const struct gs_host_frame *hf)
 {
 	u32 timestamp;
-- 
2.35.1

[PATCH 3/3] can: gs_usb: gs_can_open(): initialize time counter before starting device

[Marc Kleine-Budde]

On busy networks the CAN controller might receive CAN frames directly
after starting it but before the timecounter is setup. This will lead
to NULL pointer deref while converting the converting the CAN frame's
timestamp with the timecounter.

Close the race window by setting up the timecounter before starting
the CAN controller.

Fixes: 45dfa45f52e6 ("can: gs_usb: add RX and TX hardware timestamp support")
Cc: John Whittington <git@jbrengineering.co.uk
Signed-off-by: Marc Kleine-Budde <mkl@pengutronix.de>
---
 drivers/net/can/usb/gs_usb.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
index fe4116bf925b..d3e229c62389 100644
--- a/drivers/net/can/usb/gs_usb.c
+++ b/drivers/net/can/usb/gs_usb.c
@@ -972,6 +972,10 @@ static int gs_can_open(struct net_device *netdev)
 	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
 		flags |= GS_CAN_MODE_HW_TIMESTAMP;
 
+	/* start polling timestamp */
+	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
+		gs_usb_timestamp_init(dev);
+
 	/* finally start device */
 	dev->can.state = CAN_STATE_ERROR_ACTIVE;
 	dm->mode = cpu_to_le32(GS_CAN_MODE_START);
@@ -991,10 +995,6 @@ static int gs_can_open(struct net_device *netdev)
 
 	kfree(dm);
 
-	/* start polling timestamp */
-	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
-		gs_usb_timestamp_init(dev);
-
 	parent->active_channels++;
 	if (!(dev->can.ctrlmode & CAN_CTRLMODE_LISTENONLY))
 		netif_start_queue(netdev);
-- 
2.35.1

Re: (No Subject)

[john]

> while playing around with the hardware timestamping code, I found some
> issues, which are fixed in this series.
> 
> John, can you test if timestamping still works on your hardware.

I've just tested the 3 patches and yes it still works. Thanks for picking up on those.

I see a potential issue with 3/3: the timer not being stopped if the USB start errors but I've replied in that thread.

Re: [PATCH 3/3] can: gs_usb: gs_can_open(): initialize time counter before starting device

[john]

> On busy networks the CAN controller might receive CAN frames directly
> after starting it but before the timecounter is setup. This will lead
> to NULL pointer deref while converting the converting the CAN frame's
> timestamp with the timecounter.
> 
> Close the race window by setting up the timecounter before starting
> the CAN controller.

My logic of starting the timer after the USB request to start was due to the function returning before if the USB start request returns an error. With this change, the timer will be started and poll the USB timestamp request even if the device is not started - I tested and confirmed this is the case.

I agree with the issue this patch solves and flaw previously but believe it requires a check of the GS_CAN_FEATURE_HW_TIMESTAMP and gs_usb_timestamp_stop(dev) in the rc < 0 check.

if (rc < 0) {
  netdev_err(netdev, "Couldn't start device (err=%d)\n", rc);
  kfree(dm);
  /* stop polling timestamp */
  if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
    gs_usb_timestamp_stop(dev);
  return rc;
}

John.

Re: [PATCH 3/3] can: gs_usb: gs_can_open(): initialize time counter before starting device

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 1496 bytes --]

On 21.09.2022 07:07:42, john@jbrengineering.co.uk wrote:
> > On busy networks the CAN controller might receive CAN frames directly
> > after starting it but before the timecounter is setup. This will lead
> > to NULL pointer deref while converting the converting the CAN frame's
> > timestamp with the timecounter.
> > 
> > Close the race window by setting up the timecounter before starting
> > the CAN controller.
> 
> My logic of starting the timer after the USB request to start was due
> to the function returning before if the USB start request returns an
> error. With this change, the timer will be started and poll the USB
> timestamp request even if the device is not started - I tested and
> confirmed this is the case.
> 
> I agree with the issue this patch solves and flaw previously but
> believe it requires a check of the GS_CAN_FEATURE_HW_TIMESTAMP and
> gs_usb_timestamp_stop(dev) in the rc < 0 check.
> 
> if (rc < 0) {
>   netdev_err(netdev, "Couldn't start device (err=%d)\n", rc);
>   kfree(dm);
>   /* stop polling timestamp */
>   if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
>     gs_usb_timestamp_stop(dev);
>   return rc;
> }

Right! Will fix that.

Thanks,
Marc

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (No Subject)

[Marc Kleine-Budde]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

On 21.09.2022 07:07:26, john@jbrengineering.co.uk wrote:
> > while playing around with the hardware timestamping code, I found some
> > issues, which are fixed in this series.
> > 
> > John, can you test if timestamping still works on your hardware.
> 
> I've just tested the 3 patches and yes it still works. Thanks for
> picking up on those.

Thanks, can I add your Tested-by?

> I see a potential issue with 3/3: the timer not being stopped if the
> USB start errors but I've replied in that thread.

Fixed, see:

| https://lore.kernel.org/all/20220921081329.385509-1-mkl@pengutronix.de

regards,
Marc

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: (No Subject)

[john]

> Thanks, can I add your Tested-by?

Yep.

John.