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

[Shuangpeng]

> On Jul 20, 2026, at 18:35, Vadim Fedorenko <vadim.fedorenko@xxxxxxxxx> 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@xxxxxxxxxxxxxxx
>> Signed-off-by: Shuangpeng Bai <shuangpeng.kernel@xxxxxxxxx>
>> ---
>> 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?

> 
>>  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;