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

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issue(s) to consider.

Critical: 0 · High: 0 · Medium: 2 · Low: 3

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch adds an RX-path change that the commit message never
  mentions.
- [Medium] gs_usb_timestamp_start()
  (drivers/net/can/usb/gs_usb.c:504-536) always sends the…
- [Low] The commit message describes the old behaviour inaccurately and
  understates the impact.
- [Low] The fix relies on disable_delayed_work(), enable_delayed_work()
  and disable_delayed_work_sync().
- [Low] The new counter struct gs_usb::active_timestamp_channels
  (drivers/net/can/usb/gs_usb.c:345), and the rewritten tc_lock comment…

--

can: gs_usb: fix hardware timestamp state for mixed channels

This patch initializes the HW timestamping infrastructure in
gs_usb_probe() if any channel has GS_CAN_FEATURE_HW_TIMESTAMP set. It
reworks gs_usb_timestamp_start()/gs_usb_timestamp_stop() to count opens
and to init the timecounter on the first one, reading the initial
timestamp before taking tc_lock. The delayed worker is disabled and
enabled to prevent rescheduling while stop runs.

Closes: https://sashiko.dev/#/bug/linux-b39360f9-6cd8-4f47-83bb-f0f78331f308
Closes: https://sashiko.dev/#/bug/linux-5f8ecd4e-344e-4ceb-9b46-886f4942a5f6

> Currently both the initialization and shutdown of the HW timestamping
> infrastructure is tied to the flags of the first channel opened
> respectively closed. This leads to problems if the first channel opened
> doesn't have the timestamping feature, but the second channel has.

[Severity: Low]

Is "first channel ... closed" accurate? Before this patch, gs_can_close()
only called gs_usb_timestamp_stop() once active_channels reached zero. So
shutdown depended on the flags of the last channel closed:

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

Could the message also say what "leads to problems" means? The old code
seems to have two crash cases.

If a non-TS channel opens first and a TS channel opens second,
gs_usb_timestamp_init() never runs. The TS channel's RX frames then reach
gs_usb_skb_set_timestamp()->timecounter_cyc2time() while parent->tc.cc is
still NULL.

If a TS channel opens first and a non-TS channel closes last,
cancel_delayed_work_sync() is never called. The self-rearming
gs_usb_timestamp_work() can then run after gs_usb_disconnect() frees
parent.

The Cc: stable tag rests on these, so would it help to name them?

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index 6327069f63c95..9eafdf63a4ef0 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c

[ ... ]

> @@ -332,11 +336,14 @@ struct gs_usb {
>  	/* time counter for hardware timestamps */
>  	struct cyclecounter cc;
>  	struct timecounter tc;
> -	spinlock_t tc_lock; /* spinlock to guard access tc->cycle_last */
> +	spinlock_t tc_lock; /* spinlock to guard access tc and active_timestamp_channels */
>  	struct delayed_work timestamp;
> +	u32 initial_timestamp;
>  
>  	unsigned int hf_size_rx;
>  	u8 active_channels;
> +	u8 active_timestamp_channels;

[Severity: Low]

This isn't a bug, but does active_timestamp_channels really count
timestamping channels? Three places call gs_usb_timestamp_start() or
gs_usb_timestamp_stop(): gs_can_open(), its error path, and
gs_can_close(). All three check only the parent-wide
GS_USB_PARENT_FLAGS_HW_TIMESTAMP flag, never dev->feature:

	if (parent->flags & GS_USB_PARENT_FLAGS_HW_TIMESTAMP) {
		rc = gs_usb_timestamp_start(parent);

So on a TS-capable interface the counter just mirrors active_channels.
With only non-TS channels open, gs_usb_timestamp_work() still sends
GS_USB_BREQ_TIMESTAMP every 30 minutes. Opening a non-TS channel also
does a synchronous timestamp read.

The commit message says the functions "track the number of active
timestamp channels", which doesn't match this. Would a different name or
comment be clearer?

[ ... ]

> @@ -475,24 +489,61 @@ static void gs_usb_timestamp_init(struct gs_usb *parent)

[ ... ]

>  	INIT_DELAYED_WORK(&parent->timestamp, gs_usb_timestamp_work);
> +	disable_delayed_work(&parent->timestamp);

[Severity: Low]

Will this backport to the stable trees covered by the Fixes: tag?
disable_delayed_work(), enable_delayed_work() and
disable_delayed_work_sync() arrived in v6.10 with commit 86898fa6b8cd
("workqueue: Implement disable/enable for (delayed) work items").
5886e4d5ecec dates from July 2023 and is in older stable trees such as
6.6.y and 6.1.y.

A mechanical backport would fail to build there. Swapping in
cancel_delayed_work_sync() by hand would silently drop the protection
against gs_usb_timestamp_work() re-arming itself. The commit message
relies on that protection ("Enabling and disabling the delayed worker
prevents concurrent rescheduling while gs_usb_timestamp_stop() is
running").

Could a stable version annotation or a note for backporters be added?

> +
> +	parent->flags |= GS_USB_PARENT_FLAGS_HW_TIMESTAMP;
> +}
> +
> +static int gs_usb_timestamp_start(struct gs_usb *parent)
> +{
> +	u32 timestamp = 0;
> +	int err;
> +
> +	/* read current timestamp, might be used later for initial time stamp */
> +	err = gs_usb_get_timestamp(parent, &timestamp);
> +	if (err)
> +		return err;
> +
> +	scoped_guard(spinlock_bh, &parent->tc_lock) {
> +		struct cyclecounter *cc = &parent->cc;
> +
> +		parent->active_timestamp_channels++;
> +		if (parent->active_timestamp_channels > 1)
> +			return 0;

[Severity: Medium]

Should a failed GS_USB_BREQ_TIMESTAMP read make every channel's open
fail? gs_usb_get_timestamp() runs before the active_timestamp_channels
check. For every open after the first, the value is thrown away. An error
such as a timeout, -EPIPE or -EPROTO still reaches gs_can_open():

	if (parent->flags & GS_USB_PARENT_FLAGS_HW_TIMESTAMP) {
		rc = gs_usb_timestamp_start(parent);
		if (rc)
			goto out_can_rx_offload_disable;
	}

On a mixed device, opening a channel without
GS_CAN_FEATURE_HW_TIMESTAMP now also depends on this request succeeding.

Before the patch, only the first open of a TS-capable channel read the
timestamp, through gs_usb_timestamp_read() inside timecounter_init(). A
failure there was only logged with dev_err() and did not fail the open.
Later opens sent no request at all.

Open and close are serialized by RTNL. Could the count be checked first,
with the read done only on the 0->1 transition? Or could the old
non-fatal behaviour be kept?

[ ... ]

> @@ -654,7 +705,7 @@ static void gs_usb_receive_bulk_callback(struct urb *urb)
>  	if (!netif_device_present(netdev))
>  		return;
>  
> -	if (!netif_running(netdev))
> +	if (!netif_running(netdev) || !(netdev->flags & IFF_UP))
>  		goto resubmit_urb;

[Severity: Medium]

Is this hunk part of the timestamp fix? The commit message doesn't
mention it.

__dev_open() sets __LINK_STATE_START before ndo_open(), and sets IFF_UP
only after ndo_open() succeeds. On close, __dev_close_many() clears
__LINK_STATE_START before ndo_stop(). So the new check only has an effect
while this channel's gs_can_open() is running.

On a multi-channel interface with another channel already up, the RX URBs
are active. Any frame for this channel that completes in that window is
now dropped and the URB resubmitted. That includes frames received after
the GS_USB_BREQ_MODE start request at the end of gs_can_open() but before
the core sets IFF_UP.

With this patch, every open on a TS-capable interface calls
gs_usb_timestamp_start() before the RX URBs are submitted, so the
timecounter should already be valid. If this hunk fixes a different
problem, perhaps one of the Closes: reports, could it be described in the
commit message or split into its own patch?

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261113-gs_usb-fixes-v6-0-0c039e1bfd74%40pengutronix.de