Re: [PATCH can-next 13/13] can: gs_usb: implement CAN bus off recovery

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The CAN_ERR_RESTARTED bit check in gs_update_state() is always false because the bit is unconditionally cleared at the beginning of the function.
--

--- Patch [13]: [PATCH can-next 13/13] can: gs_usb: implement CAN bus off recovery ---
commit 7b5dd8a47b695ccb806d9d65e91f427368f07215
Author: Marc Kleine-Budde <[email protected]>

can: gs_usb: implement CAN bus off recovery

This patch implements manual CAN bus off recovery for gs_usb devices that
support the GS_CAN_FEATURE_BUS_OFF_RECOVERY feature. It allows the USB host
to control bus off recovery instead of relying on the hardware's automatic
recovery.

Link: https://github.com/candle-usb/candleLight_fw/pull/317

> diff --git a/drivers/net/can/usb/gs_usb.c b/drivers/net/can/usb/gs_usb.c
> --- a/drivers/net/can/usb/gs_usb.c
> +++ b/drivers/net/can/usb/gs_usb.c

[ ... ]

> @@ -573,14 +579,22 @@ static void gs_update_state(struct gs_can *dev, struct can_frame *cf,
>  		return;
>  
>  	/* some firmware does automatically CAN bus off recovery, account for this */
> -	if (cf->can_id & CAN_ERR_RESTARTED ||
> -	    (dev->can.state == CAN_STATE_BUS_OFF && new_state < CAN_STATE_BUS_OFF)) {
> +	if (!(dev->feature & GS_CAN_FEATURE_BUS_OFF_RECOVERY) &&
> +	    (cf->can_id & CAN_ERR_RESTARTED ||
> +	     (dev->can.state == CAN_STATE_BUS_OFF && new_state < CAN_STATE_BUS_OFF))) {

[Severity: Medium]
Will the check for CAN_ERR_RESTARTED ever evaluate to true in this
condition?

Looking earlier in gs_update_state(), the CAN_ERR_RESTARTED bit is
unconditionally cleared from cf->can_id:

	cf->can_id &= ~(CAN_ERR_CRTL | CAN_ERR_BUSOFF | CAN_ERR_RESTARTED);

Because of this, the check for CAN_ERR_RESTARTED here appears to be
structurally guaranteed to be false. Could this cause the driver to miss
bus-off restart events if the cached state was not already CAN_STATE_BUS_OFF?

>  		can_stats->restarts++;
>  		/* some firmware doesn't set CAN_ERR_RESTARTED, fixup */
>  		cf->can_id |= CAN_ERR_RESTARTED;
>  	}

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