Re: [PATCH v2 3/3] can: rockchip_canfd: serialize TX state and command writes

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The `tx_lock` fails to fully serialize `RKCANFD_REG_MODE` writes against the interrupt path, leaving a race that can break the bus-off state.
--

--- Patch [3]: [PATCH v2 3/3] can: rockchip_canfd: serialize TX state and command writes ---
commit 2a5b1a8235e0f295b950d48868b11a6ee290c521
Author: Cunhao Lu <1579567540@qq.com>

can: rockchip_canfd: serialize TX state and command writes

This patch adds a TX state lock to protect tx_head, tx_tail, and the
echo skb ring. It also serializes the MODE/CMD/MODE sequence between
the transmit and interrupt paths to address errata 6 and 12.

Fixes: ae002cc32ec4 ("can: rockchip_canfd: prepare to use full TX-FIFO depth")
Fixes: 83f9bd6bf39d ("can: rockchip_canfd: implement workaround for erratum 12")
Cc: stable@vger.kernel.org
Signed-off-by: Cunhao Lu <1579567540@qq.com>

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/can/rockchip/rockchip_canfd-tx.c
> index c4ecdc9411cc6..c3811d3179b61 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-tx.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-tx.c

[ ... ]

> @@ -141,6 +158,7 @@ netdev_tx_t rkcanfd_start_xmit(struct sk_buff *skb, struct net_device *ndev)
>  	WRITE_ONCE(priv->tx_head, priv->tx_head + 1);
>  
>  	rkcanfd_start_xmit_write_cmd(priv, reg_cmd);
> +	spin_unlock_irqrestore(&priv->tx_lock, flags);

[Severity: High]
This is a pre-existing issue and was not introduced by this patch, but since 
the commit message mentions using tx_lock to serialize the MODE/CMD/MODE 
sequence between the transmit and interrupt paths, does this leave a race with 
the bus-off error handler?

Looking at rkcanfd_chip_set_reset_mode() in 
drivers/net/can/rockchip/rockchip_canfd-core.c, it places the controller in 
reset during a bus-off event by writing 0x0 to RKCANFD_REG_MODE, but it does 
so without holding tx_lock.

If a bus-off event occurs concurrently with a packet transmission, could the 
following sequence occur?

CPU1 (Interrupt path):
rkcanfd_irq() -> rkcanfd_handle_state_error_int() -> rkcanfd_chip_stop() -> 
rkcanfd_chip_set_reset_mode()
    rkcanfd_write(priv, RKCANFD_REG_MODE, 0x0);

CPU2 (TX path, holding tx_lock):
rkcanfd_start_xmit_write_cmd()
    rkcanfd_write(priv, RKCANFD_REG_MODE, priv->reg_mode_default);

Could this overwrite the bus-off reset mode and bring the controller back out 
of reset prematurely, potentially causing the node to continuously disrupt the 
CAN bus?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-master-v2-0-572974052400@qq.com?part=3