* (no subject) @ 2022-09-20 10:04 Marc Kleine-Budde 2022-09-20 10:04 ` [PATCH 1/3] can: gs_usb: gs_usb_get_timestamp(): fix endpoint parameter for usb_control_msg_recv() Marc Kleine-Budde ` (3 more replies) 0 siblings, 4 replies; 9+ messages in thread From: Marc Kleine-Budde @ 2022-09-20 10:04 UTC (permalink / raw) To: linux-can; +Cc: John Whittington 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 1/3] can: gs_usb: gs_usb_get_timestamp(): fix endpoint parameter for usb_control_msg_recv() 2022-09-20 10:04 Marc Kleine-Budde @ 2022-09-20 10:04 ` Marc Kleine-Budde 2022-09-20 10:04 ` [PATCH 2/3] can: gs_usb: add missing lock to protect struct timecounter::cycle_last Marc Kleine-Budde ` (2 subsequent siblings) 3 siblings, 0 replies; 9+ messages in thread From: Marc Kleine-Budde @ 2022-09-20 10:04 UTC (permalink / raw) To: linux-can; +Cc: John Whittington, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/3] can: gs_usb: add missing lock to protect struct timecounter::cycle_last 2022-09-20 10:04 Marc Kleine-Budde 2022-09-20 10:04 ` [PATCH 1/3] can: gs_usb: gs_usb_get_timestamp(): fix endpoint parameter for usb_control_msg_recv() Marc Kleine-Budde @ 2022-09-20 10:04 ` Marc Kleine-Budde 2022-09-20 10:04 ` [PATCH 3/3] can: gs_usb: gs_can_open(): initialize time counter before starting device Marc Kleine-Budde 2022-09-21 7:07 ` (No Subject) john 3 siblings, 0 replies; 9+ messages in thread From: Marc Kleine-Budde @ 2022-09-20 10:04 UTC (permalink / raw) To: linux-can; +Cc: John Whittington, 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, ×tamp); + 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 3/3] can: gs_usb: gs_can_open(): initialize time counter before starting device 2022-09-20 10:04 Marc Kleine-Budde 2022-09-20 10:04 ` [PATCH 1/3] can: gs_usb: gs_usb_get_timestamp(): fix endpoint parameter for usb_control_msg_recv() Marc Kleine-Budde 2022-09-20 10:04 ` [PATCH 2/3] can: gs_usb: add missing lock to protect struct timecounter::cycle_last Marc Kleine-Budde @ 2022-09-20 10:04 ` Marc Kleine-Budde 2022-09-21 7:07 ` john 2022-09-21 7:07 ` (No Subject) john 3 siblings, 1 reply; 9+ messages in thread From: Marc Kleine-Budde @ 2022-09-20 10:04 UTC (permalink / raw) To: linux-can; +Cc: John Whittington, 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 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] can: gs_usb: gs_can_open(): initialize time counter before starting device 2022-09-20 10:04 ` [PATCH 3/3] can: gs_usb: gs_can_open(): initialize time counter before starting device Marc Kleine-Budde @ 2022-09-21 7:07 ` john 2022-09-21 7:40 ` Marc Kleine-Budde 0 siblings, 1 reply; 9+ messages in thread From: john @ 2022-09-21 7:07 UTC (permalink / raw) To: Marc Kleine-Budde; +Cc: linux-can, John Whittington > 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] can: gs_usb: gs_can_open(): initialize time counter before starting device 2022-09-21 7:07 ` john @ 2022-09-21 7:40 ` Marc Kleine-Budde 0 siblings, 0 replies; 9+ messages in thread From: Marc Kleine-Budde @ 2022-09-21 7:40 UTC (permalink / raw) To: john; +Cc: linux-can, John Whittington [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: (No Subject) 2022-09-20 10:04 Marc Kleine-Budde ` (2 preceding siblings ...) 2022-09-20 10:04 ` [PATCH 3/3] can: gs_usb: gs_can_open(): initialize time counter before starting device Marc Kleine-Budde @ 2022-09-21 7:07 ` john 2022-09-21 19:58 ` Marc Kleine-Budde 3 siblings, 1 reply; 9+ messages in thread From: john @ 2022-09-21 7:07 UTC (permalink / raw) To: Marc Kleine-Budde; +Cc: linux-can, John Whittington > 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. ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: (No Subject) 2022-09-21 7:07 ` (No Subject) john @ 2022-09-21 19:58 ` Marc Kleine-Budde 2022-09-23 7:37 ` john 0 siblings, 1 reply; 9+ messages in thread From: Marc Kleine-Budde @ 2022-09-21 19:58 UTC (permalink / raw) To: john; +Cc: linux-can, John Whittington [-- 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 --] ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: (No Subject) 2022-09-21 19:58 ` Marc Kleine-Budde @ 2022-09-23 7:37 ` john 0 siblings, 0 replies; 9+ messages in thread From: john @ 2022-09-23 7:37 UTC (permalink / raw) To: Marc Kleine-Budde; +Cc: linux-can, John Whittington > Thanks, can I add your Tested-by? Yep. John. ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-09-23 7:37 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-09-20 10:04 Marc Kleine-Budde 2022-09-20 10:04 ` [PATCH 1/3] can: gs_usb: gs_usb_get_timestamp(): fix endpoint parameter for usb_control_msg_recv() Marc Kleine-Budde 2022-09-20 10:04 ` [PATCH 2/3] can: gs_usb: add missing lock to protect struct timecounter::cycle_last Marc Kleine-Budde 2022-09-20 10:04 ` [PATCH 3/3] can: gs_usb: gs_can_open(): initialize time counter before starting device Marc Kleine-Budde 2022-09-21 7:07 ` john 2022-09-21 7:40 ` Marc Kleine-Budde 2022-09-21 7:07 ` (No Subject) john 2022-09-21 19:58 ` Marc Kleine-Budde 2022-09-23 7:37 ` john
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.