Re: [PATCH net v4 5/6] can: rockchip_canfd: serialize TX with bus-off and restart

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] can: rockchip_canfd: echo frames leaked as RX frames on bus-off
- [Medium] can: rockchip_canfd: data race on priv->bec berr statistics
--

commit 15287fe0f7062cc51e49632555a904f260da449d
Author: Cunhao Lu <1579567540@qq.com>

can: rockchip_canfd: serialize TX with bus-off and restart

This commit serializes TX submission, RX-side completion, and retry using a
private lock and an enabled flag. It disables TX before publishing a bus-off
state to prevent concurrent senders from modifying echo slots.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-rx.c b/drivers/net/can/rockchip/rockchip_canfd-rx.c
> index 24e87daa1df00..9b357bb8fc70e 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-rx.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-rx.c
[ ... ]
> @@ -237,15 +240,18 @@ static int rkcanfd_handle_rx_int_one(struct rkcanfd_priv *priv)
>  		return 0;
>  	}
>  
> -	if (rkcanfd_get_tx_pending(priv)) {
> -		bool tx_done = false;
> -
> +	/* Do not consume echo SKBs, retry TX or wake the queue after bus-off
> +	 * has handed ownership of the echo ring to the CAN restart path.
> +	 */
> +	spin_lock_irqsave(&priv->tx_lock, flags);
> +	err = 0;
> +	if (priv->tx_enabled && rkcanfd_get_tx_pending(priv))
>  		err = rkcanfd_rxstx_filter(priv, cfd, header->ts, &tx_done);
> -		if (err)
> -			return err;
> -		if (tx_done && !(priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK))
> -			return 0;
> -	}
> +	spin_unlock_irqrestore(&priv->tx_lock, flags);
> +	if (err)
> +		return err;
> +	if (tx_done && !(priv->can.ctrlmode & CAN_CTRLMODE_LOOPBACK))
> +		return 0;

[Severity: Medium]
Could this leak echo frames as RX frames on a bus-off event?

When priv->tx_enabled is false, the code skips calling rkcanfd_rxstx_filter(),
leaving tx_done as false. This bypasses the early return for echo frames.
Since the hardware pushes self-transmitted frames to the RX FIFO, wouldn't
any echo frames already present in the RX FIFO then fall through and be
allocated and queued as regular RX frames?

>  
>  	/* Manual handling of CAN Bus Error counters. See
>  	 * rkcanfd_get_corrected_berr_counter() for detailed

[Severity: Medium]
Is there a data race on the priv->bec statistics here?

This patch introduces tx_lock around parts of the IRQ handler, but leaves the
updates to priv->bec outside the lock. The netlink command
can_get_berr_counter() invokes rkcanfd_get_berr_counter_corrected(), which
accesses priv->bec from process context without driver-specific
serialization. Concurrently, NAPI loop execution decrements priv->bec.rxerr
and priv->bec.txerr right below this block.

Could this result in torn reads or lost updates to the error counters being
reported to userspace?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261015-rockchip-canfd-restart-close-v4-0-ad149ff79450@qq.com?part=5