[PATCH] can: c_can: keep controller in init mode until bittiming is configured

[PATCH] can: c_can: keep controller in init mode until bittiming is configured

[Lucas Martins Alves]

From: Lucas Martins Alves <lucas.alves@lumal21.com.br>

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.

That creates a short timing window where the peripheral can interact with the
bus using a different/default bitrate, potentially generating bus errors and
corrupting traffic.

Set CONTROL_INIT together with the control-mode writes in c_can_chip_config()
(normal, loopback and listen-only paths), so the controller stays halted until
bit timing is fully programmed.

This prevents transient bus disturbance during startup when the configured
bitrate differs from the active bus bitrate.

Signed-off-by: Lucas Martins Alves <lucas.alves@lumal21.com.br>
---
 drivers/net/can/c_can/c_can_main.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/can/c_can/c_can_main.c b/drivers/net/can/c_can/c_can_main.c
index 3702cac7fbf0..32a3975b9d69 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);
 	}
 
-- 
2.53.0

Re: [PATCH] can: c_can: keep controller in init mode until bittiming is configured

[sashiko-bot]

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

Pre-existing issues:
- [High] Configuring bit timing temporarily clears the CONTROL_TEST bit, which silently resets the TEST register and breaks loopback/listen-only modes.
--

--- Patch [1]: [PATCH] can: c_can: keep controller in init mode until bittiming is configured ---
commit b7bd06d474febb092c757ff6270ed7de9b22644e
Author: Lucas Martins Alves <lucas.alves@lumal21.com.br>

can: c_can: 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. This patch adds CONTROL_INIT to the writes in c_can_chip_config()
so the controller stays halted until bit timing is fully programmed.

> diff --git a/drivers/net/can/c_can/c_can_main.c b/drivers/net/can/c_can/c_can_main.c
> index 3702cac7fbf0f..32a3975b9d692 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)
[ ... ]
>  	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) {

[Severity: High]
This is a pre-existing issue, but does the loopback or listen-only test mode
actually survive the initialization process?

When we configure C_CAN_TEST_REG here in c_can_chip_config(), we set the test
bits. However, shortly after this block, the function calls
c_can_set_bittiming() to configure the timing:

drivers/net/can/c_can/c_can_main.c:c_can_set_bittiming() {
    ...
    ctrl_save = priv->read_reg(priv, C_CAN_CTRL_REG);
    ctrl_save &= ~CONTROL_INIT;
    priv->write_reg(priv, C_CAN_CTRL_REG, CONTROL_CCE | CONTROL_INIT);
    ...
    priv->write_reg(priv, C_CAN_CTRL_REG, ctrl_save);
}

When writing CONTROL_CCE | CONTROL_INIT to the control register, the
CONTROL_TEST bit is explicitly omitted. Doesn't clearing the CONTROL_TEST bit
silently reset the C_CAN_TEST_REG back to 0 in the hardware, destroying the
configuration we just set?

Even though ctrl_save eventually restores the CONTROL_TEST bit in
C_CAN_CTRL_REG, the configuration in C_CAN_TEST_REG appears permanently lost.
Should c_can_set_bittiming() preserve the CONTROL_TEST bit when writing to
C_CAN_CTRL_REG?

>  		/* 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);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714164839.771123-1-lucas.alves@lumal21.com.br?part=1