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

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Data race and lock corruption due to unprotected initialization of shared timestamp lock
--

--- Patch [1]: [PATCH can v2] can: gs_usb: fix hardware timestamp state for mixed channels ---
commit 5294b98eba495191f8e49a8e183658cbb1702465
Author: Shuangpeng Bai <[email protected]>

can: gs_usb: fix hardware timestamp state for mixed channels

This patch decouples the hardware timestamp state from active_channels. It
tracks timestamp-capable channels separately to fix initialization for
mixed-channel devices.

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> index ec9a7cbbbc696..9cc197803e0d7 100644
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c
[ ... ]
> @@ -980,10 +981,13 @@ 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)
> +	if (dev->feature & GS_CAN_FEATURE_HW_TIMESTAMP) {
> +		if (!parent->active_timestamp_channels)
>  			gs_usb_timestamp_init(parent);
> +		parent->active_timestamp_channels++;
> +	}
>  
> +	if (!parent->active_channels) {
>  		for (i = 0; i < GS_MAX_RX_URBS; i++) {
>  			u8 *buf;

[Severity: High]
Can this corrupt the tc_lock spinlock if an RX callback executes concurrently?

Since gs_usb_timestamp_init() is now called independently of
!parent->active_channels, it can execute while RX URBs are already polling (if
a non-timestamp channel was opened first).

gs_usb_timestamp_init() re-initializes the lock and timecounter:

    spin_lock_init(&parent->tc_lock);
    spin_lock_bh(&parent->tc_lock);
    timecounter_init(&parent->tc, &parent->cc, ktime_get_real_ns());

If a USB device injects an RX frame during this window, the active RX URB
completion handler (gs_usb_receive_bulk_callback) can process it. Because
__dev_open() sets __LINK_STATE_START before calling ndo_open(), the
netif_running() check in the callback will pass, and
gs_usb_skb_set_timestamp() could attempt to acquire parent->tc_lock at the
same time it is being re-initialized.

Is there a way to safely initialize this lock before URBs are active, or
initialize it only once during device probe?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1