Re: [PATCH net 17/20] can: c_can: c_can_chip_config(): keep controller in init mode until bittiming is configured

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] c_can_set_bittiming() inadvertently resets C_CAN_TEST_REG, breaking loopback and listen-only modes.
--

--- Patch [17]: [PATCH net 17/20] can: c_can: c_can_chip_config(): keep controller in init mode until bittiming is configured ---
commit 43c179b24ec630378edf5e7aee260bd073975b00
Author: Lucas Martins Alves <lucas.alves@lumal21.com.br>

can: c_can: c_can_chip_config(): keep controller in init mode until bittiming is configured

c_can_chip_config() was programming C_CAN_CTRL_REG without CONTROL_INIT,
which may allow the controller to become active before
c_can_set_bittiming() finishes.

Link: https://patch.msgid.link/20260714164839.771123-1-lucas.alves@lumal21.com.br

> diff --git a/drivers/net/can/c_can/c_can_main.c b/drivers/net/can/c_can/c_can_main.c
> index 3702cac7fbf0f..b3b321d9ce68d 100644
> --- a/drivers/net/can/c_can/c_can_main.c
> +++ b/drivers/net/can/c_can/c_can_main.c
> @@ -597,20 +597,20 @@ static int c_can_chip_config(struct net_device *dev)
>  		return err;
>  
>  	/* enable automatic retransmission */
> -	priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_ENABLE_AR);
> +	priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_ENABLE_AR | CONTROL_INIT);
>  
>  	if ((priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) &&
>  	    (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK)) {
>  		/* loopback + silent mode : useful for hot self-test */
> -		priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_TEST);
> +		priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_TEST | CONTROL_INIT);
>  		priv->write_reg(priv, C_CAN_TEST_REG, TEST_LBACK | TEST_SILENT);
>  	} else if (priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK) {
>  		/* loopback mode : useful for self-test function */
> -		priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_TEST);
> +		priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_TEST | CONTROL_INIT);
>  		priv->write_reg(priv, C_CAN_TEST_REG, TEST_LBACK);
>  	} else if (priv->can.ctrlmode & CAN_CTRLMODE_LISTENONLY) {
>  		/* silent mode : bus-monitoring mode */
> -		priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_TEST);
> +		priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_TEST | CONTROL_INIT);
>  		priv->write_reg(priv, C_CAN_TEST_REG, TEST_SILENT);
>  	}
>  

[Severity: High]
This isn't a bug introduced by this patch, but doesn't the subsequent call to
c_can_set_bittiming() at the end of c_can_chip_config() unconditionally clear
the CONTROL_TEST bit?

c_can_set_bittiming() executes this:
	priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_CCE | CONTROL_INIT);

Because this write does not include CONTROL_TEST, it gets cleared. According to
the Bosch C_CAN hardware manual, clearing the Test bit instantly resets the
Test Register to its default value (0x00).

Does this silently erase the loopback and listen-only modes configured here,
causing the controller to incorrectly operate in normal mode and potentially
cause active disruption on a live CAN bus?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260729102802.505168-1-mkl@pengutronix.de?part=17