Re: [PATCH can v3] can: gs_usb: fix hardware timestamp state for mixed channels

[Vadim Fedorenko <vadim.fedorenko@linux.dev>]

On 23/07/2026 06:17, Shuangpeng wrote:
> 
> 
>> On Jul 20, 2026, at 18:35, Vadim Fedorenko <vadim.fedorenko@linux.dev> wrote:
>>
>> On 20.07.2026 19:40, Shuangpeng Bai wrote:
>>> The hardware timestamp state is shared by struct gs_usb, but
>>> gs_can_open() and gs_can_close() tie its initialization and teardown to
>>> active_channels and to the feature bits of the channel being opened or
>>> closed.
>>> This is wrong for mixed-channel devices in both directions. If a
>>> non-timestamp channel opens first, a later timestamp-capable channel does
>>> not initialize the shared cyclecounter/timecounter because active_channels
>>> is already non-zero. Timestamp RX then calls timecounter_cyc2time() with
>>> parent->tc.cc unset.
>>> Conversely, if a timestamp-capable channel opens first and starts the
>>> shared delayed work, then closes while a non-timestamp channel remains
>>> active, disconnect may close the non-timestamp channel last. The old
>>> teardown check skips gs_usb_timestamp_stop() in that case and frees
>>> struct gs_usb while the delayed work timer is still queued.
>>> Count the number of active timestamp-capable channels instead. Start the
>>> shared timestamp state when the first such channel opens, stop it when the
>>> last such channel closes, and unwind the count if open fails.
>>> Initialize the shared timestamp lock and delayed work once during probe.
>>> The first timestamp-capable channel may be opened while RX URBs are
>>> already active, so guard the count and timecounter access with the same
>>> lock. Keep the count zero until timecounter_init() completes, and make RX
>>> and delayed work skip the timecounter while the count is zero.
>>> Fixes: 45dfa45f52e6 ("can: gs_usb: add RX and TX hardware timestamp support")
>>> Cc: stable@vger.kernel.org
>>> Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@gmail.com>
>>> ---
>>> Changes in v3:
>>> - Drop Suggested-by tag.
>>> - Move active_timestamp_channels management into the timestamp init/stop
>>>    helpers.
>>> - Protect active_timestamp_channels with tc_lock, and keep it zero until
>>>    timecounter_init() has completed.
>>> - Make RX timestamp conversion and the delayed work skip the timecounter
>>>    while no timestamp-capable channel is active.
>>> - Initialize the shared timestamp lock and delayed work once during probe.
>>> Changes in v2:
>>> - Count active timestamp-capable channels instead of tracking only whether
>>>    the shared timestamp worker has been started.
>>> - Stop the shared timestamp worker when the last timestamp-capable channel
>>>    closes, even if non-timestamp channels remain open.
>>> - Unwind the timestamp-capable channel count on gs_can_open() failures.
>>>   drivers/net/can/usb/gs_usb.c | 81 ++++++++++++++++++++++++------------
>>>   1 file changed, 54 insertions(+), 27 deletions(-)
>>> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
>>> index ec9a7cbbbc69..be25d82cedcb 100644
>>> --- a/drivers/net/can/usb/gs_usb.c
>>> +++ b/drivers/net/can/usb/gs_usb.c
>>> @@ -337,6 +337,7 @@ struct gs_usb {
>>>      unsigned int hf_size_rx;
>>>    u8 active_channels;
>>> + u8 active_timestamp_channels;
>>>    u8 channel_cnt;
>>>      unsigned int pipe_in;
>>> @@ -447,14 +448,20 @@ static void gs_usb_timestamp_work(struct work_struct *work)
>>>   {
>>>    struct delayed_work *delayed_work = to_delayed_work(work);
>>>    struct gs_usb *parent;
>>> + bool active;
>>>      parent = container_of(delayed_work, struct gs_usb, timestamp);
>>>    spin_lock_bh(&parent->tc_lock);
>>> - timecounter_read(&parent->tc);
>>> + active = parent->active_timestamp_channels;
>>> + if (active) {
>>> + timecounter_read(&parent->tc);
>>> + active = parent->active_timestamp_channels;
>>> + }
>>>    spin_unlock_bh(&parent->tc_lock);
>>>   - schedule_delayed_work(&parent->timestamp,
>>> -       GS_USB_TIMESTAMP_WORK_DELAY_SEC * HZ);
>>> + if (active)
>>> + schedule_delayed_work(&parent->timestamp,
>>> +       GS_USB_TIMESTAMP_WORK_DELAY_SEC * HZ);
>>>   }
>>>     static void gs_usb_skb_set_timestamp(struct gs_can *dev,
>>> @@ -465,6 +472,11 @@ static void gs_usb_skb_set_timestamp(struct gs_can *dev,
>>>    u64 ns;
>>>      spin_lock_bh(&parent->tc_lock);
>>> + if (!parent->active_timestamp_channels) {
>>> + spin_unlock_bh(&parent->tc_lock);
>>> + return;
>>> + }
>>> +
>>>    ns = timecounter_cyc2time(&parent->tc, timestamp);
>>>    spin_unlock_bh(&parent->tc_lock);
>>>   @@ -474,25 +486,40 @@ static void gs_usb_skb_set_timestamp(struct gs_can *dev,
>>>   static void gs_usb_timestamp_init(struct gs_usb *parent)
>>>   {
>>>    struct cyclecounter *cc = &parent->cc;
>>> + bool first = false;
>>>   - cc->read = gs_usb_timestamp_read;
>>> - cc->mask = CYCLECOUNTER_MASK(32);
>>> - 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(&parent->tc_lock);
>>>    spin_lock_bh(&parent->tc_lock);
>>> - timecounter_init(&parent->tc, &parent->cc, ktime_get_real_ns());
>>> + if (!parent->active_timestamp_channels) {
>>> + cc->read = gs_usb_timestamp_read;
>>> + cc->mask = CYCLECOUNTER_MASK(32);
>>> + cc->shift = 32 - bits_per(NSEC_PER_SEC /
>>> +       GS_USB_TIMESTAMP_TIMER_HZ);
>>> + cc->mult = clocksource_hz2mult(GS_USB_TIMESTAMP_TIMER_HZ,
>>> +        cc->shift);
>>> +
>>> + timecounter_init(&parent->tc, &parent->cc,
>>> +  ktime_get_real_ns());
>>> + first = true;
>>> + }
>>> + parent->active_timestamp_channels++;
>>>    spin_unlock_bh(&parent->tc_lock);
>>>   - INIT_DELAYED_WORK(&parent->timestamp, gs_usb_timestamp_work);
>>> - schedule_delayed_work(&parent->timestamp,
>>> -       GS_USB_TIMESTAMP_WORK_DELAY_SEC * HZ);
>>> + if (first)
>>> + schedule_delayed_work(&parent->timestamp,
>>> +       GS_USB_TIMESTAMP_WORK_DELAY_SEC * HZ);
>>>   }
>>>     static void gs_usb_timestamp_stop(struct gs_usb *parent)
>>>   {
>>> - cancel_delayed_work_sync(&parent->timestamp);
>>> + bool last;
>>> +
>>> + spin_lock_bh(&parent->tc_lock);
>>> + parent->active_timestamp_channels--;
>>> + last = !parent->active_timestamp_channels;
>>> + spin_unlock_bh(&parent->tc_lock);
>>> +
>>> + if (last)
>>> + cancel_delayed_work_sync(&parent->timestamp);
>>>   }
>>>   
>>
>> tc_lock serializes active_timestamp_channels updates, but this work manipulation
>> is not protected. Imagine CPU0 doing gs_usb_timestamp_stop while CPU1 is doing
>> gs_can_open:
>>
>>     CPU0     CPU1
>>
>> gs_usb_timestamp_stop() gs_usb_timestamp_init()
>>   spin_lock_bh(&tc_lock);
>>   active_timestamp_channels--
>>   last = true
>>   spin_unlock_bh(&tc_lock);
>> ... spin_lock_bh(&tc_lock)
>> ...   timecounter_init()
>> ...   first = true;
>> ... spin_unlock_bh(&tc_lock)
>> ... schedule_delayed_work()
>>   cancel_delayed_work_sync()
>>
>> And the device will have no worker while active_timestamp_channels is not 0.
>>
>> I think we have to find another way of synchronization here.
>>
> 
> Thanks for your check.
> 
> I thought these paths were already serialized by RTNL through
> ndo_open/ndo_stop. If additional synchronization is needed,
> do you have any suggestions?

Ok, fair, but we are slowly moving towards per-device locks. May need to
revisit this part later.

Reviewed-by: Vadim Fedorenko <vadim.fedorenko@linux.dev>

> 
>>
>>>   static void gs_update_state(struct gs_can *dev, struct can_frame *cf)
>>> @@ -980,10 +1007,10 @@ static int gs_can_open(struct net_device *netdev)
>>>      can_rx_offload_enable(&dev->offload);
>>>   - if (!parent->active_channels) {
>>> - if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
>>> - gs_usb_timestamp_init(parent);
>>> + if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
>>> + gs_usb_timestamp_init(parent);
>>>   + if (!parent->active_channels) {
>>>    for (i = 0; i < GS_MAX_RX_URBS; i++) {
>>>    u8 *buf;
>>>   @@ -1094,12 +1121,11 @@ static int gs_can_open(struct net_device *netdev)
>>>   out_usb_free_urb:
>>>    usb_free_urb(urb);
>>>   out_usb_kill_anchored_urbs:
>>> - if (!parent->active_channels) {
>>> - usb_kill_anchored_urbs(&parent->rx_submitted);
>>> + if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
>>> + gs_usb_timestamp_stop(parent);
>>>   - if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
>>> - gs_usb_timestamp_stop(parent);
>>> - }
>>> + if (!parent->active_channels)
>>> + usb_kill_anchored_urbs(&parent->rx_submitted);
>>>      can_rx_offload_disable(&dev->offload);
>>>    close_candev(netdev);
>>> @@ -1152,12 +1178,11 @@ static int gs_can_close(struct net_device *netdev)
>>>      /* Stop polling */
>>>    parent->active_channels--;
>>> - if (!parent->active_channels) {
>>> - usb_kill_anchored_urbs(&parent->rx_submitted);
>>> + if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
>>> + gs_usb_timestamp_stop(parent);
>>>   - if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP)
>>> - gs_usb_timestamp_stop(parent);
>>> - }
>>> + if (!parent->active_channels)
>>> + usb_kill_anchored_urbs(&parent->rx_submitted);
>>>      /* Stop sending URBs */
>>>    usb_kill_anchored_urbs(&dev->tx_submitted);
>>> @@ -1577,6 +1602,8 @@ static int gs_usb_probe(struct usb_interface *intf,
>>>    parent->channel_cnt = icount;
>>>      init_usb_anchor(&parent->rx_submitted);
>>> + spin_lock_init(&parent->tc_lock);
>>> + INIT_DELAYED_WORK(&parent->timestamp, gs_usb_timestamp_work);
>>>      usb_set_intfdata(intf, parent);
>>>    parent->udev = udev;
> 
>