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

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 a=
gainst 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 c=
ommand writes ---
commit 2a5b1a8235e0f295b950d48868b11a6ee290c521
Author: Cunhao Lu <[email protected]>

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 dept=
h")
Fixes: 83f9bd6bf39d ("can: rockchip_canfd: implement workaround for erratum=
 12")
Cc: [email protected]
Signed-off-by: Cunhao Lu <[email protected]>

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-tx.c b/drivers/net/c=
an/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, s=
truct net_device *ndev)
>  	WRITE_ONCE(priv->tx_head, priv->tx_head + 1);
> =20
>  	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 sinc=
e=20
the commit message mentions using tx_lock to serialize the MODE/CMD/MODE=20
sequence between the transmit and interrupt paths, does this leave a race w=
ith=20
the bus-off error handler?

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

If a bus-off event occurs concurrently with a packet transmission, could th=
e=20
following sequence occur?

CPU1 (Interrupt path):
rkcanfd_irq() -> rkcanfd_handle_state_error_int() -> rkcanfd_chip_stop() ->=
=20
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 o=
ut=20
of reset prematurely, potentially causing the node to continuously disrupt =
the=20
CAN bus?

--=20
Sashiko AI review =C2=B7 https://sashiko.dev/#/patchset/20260730-master-v2-=
[email protected]?part=3D3