Re: [PATCH can-next v5 6/6] can: gs_usb: fix hardware timestamp state for mixed channels

[Marc Kleine-Budde]

On 09.11.2026 12:40:14, Vadim Fedorenko wrote:
> > diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> > index 6327069f63c9..0a1cd14c940c 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;
> > @@ -481,18 +482,35 @@ static void gs_usb_timestamp_init(struct gs_usb *parent)
> >   	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());
> > -	spin_unlock_bh(&parent->tc_lock);
> >   	INIT_DELAYED_WORK(&parent->timestamp, gs_usb_timestamp_work);
> > +	disable_delayed_work(&parent->timestamp);
> > +}
> > +
> > +static void gs_usb_timestamp_start(struct gs_usb *parent)
> > +{
> > +	scoped_guard(spinlock_bh, &parent->tc_lock) {
> > +		parent->active_timestamp_channels++;
> > +		if (parent->active_timestamp_channels > 1)
> > +			return;
> > +
> > +		timecounter_init(&parent->tc, &parent->cc, ktime_get_real_ns());
> > +	}
> > +
> > +	enable_delayed_work(&parent->timestamp);
> >   	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);
> > +	scoped_guard(spinlock_bh, &parent->tc_lock) {
> > +		parent->active_timestamp_channels--;
> > +		if (parent->active_timestamp_channels > 0)
> > +			return;
> > +	}
> > +
> > +	disable_delayed_work_sync(&parent->timestamp);
> >   }
>
> That's quite defensive code. You have both disable/enable delayed_work
> and active_timestamp_channels counter just to be sure to schedule
> timestamp worker only once.

disable is needed to stop the worker from self-scheduling.

Sadly workers only support multiple levels of disable, but not of
enable. So we have use active_timestamp_channels, too.

> Just wonder cannot we keep this worker running regardless of channels?

For proper power management (on the Linux and on the µC side) I don't
want to keep it running if the interface is down.

There are also (more or less) gs_usb compatible devices which implement
(more or less) independent USB interfaces. All of those interfaces have
a common clock, so I want to share the HW timestamp infrastructure
between them.

regards,
Marc

-- 
Pengutronix e.K.                 | Marc Kleine-Budde          |
Embedded Linux                   | https://www.pengutronix.de |
Vertretung Nürnberg              | Phone: +49-5121-206917-129 |
Amtsgericht Hildesheim, HRA 2686 | Fax:   +49-5121-206917-9   |

Attachment: signature.asc
Description: PGP signature