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

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 <[email protected]>

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/[email protected]?part=1