Re: [PATCH v6 2/4] can: rockchip: add RK3588 CAN support

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The rockchip_canfd driver unconditionally enables CAN-FD mode in the hardware, even if the user only requests Classic CAN mode. However, the data bit timing registers are only initialized if the user explicitly requests CAN-FD. This leaves the hardware configured to process CAN-FD frames but with uninitialized data bit timing, causing it to lose synchronization and transmit error frames, which disrupts valid CAN-FD traffic on the bus.
--

--- Patch [2]: [PATCH v6 2/4] can: rockchip: add RK3588 CAN support ---
commit c9bd120b397e60634c9f6a44ebe7ae2f872b4274
Author: Cunhao Lu <1579567540@qq.com>

can: rockchip: add RK3588 CAN support

Add support for the RK3588 CAN controller by introducing a dedicated
model ID and OF match entry. Enable existing erratum 5 and 6 workarounds
and enable CAN-FD for RK3588.

> diff --git a/drivers/net/can/rockchip/rockchip_canfd-core.c b/drivers/net/can/rockchip/rockchip_canfd-core.c
> index 29de0c01e4edc..37c1c22c40c9f 100644
> --- a/drivers/net/can/rockchip/rockchip_canfd-core.c
> +++ b/drivers/net/can/rockchip/rockchip_canfd-core.c
[ ... ]
> @@ -148,6 +156,12 @@ static int rkcanfd_set_bittiming(struct rkcanfd_priv *priv)
>  
>  	rkcanfd_write(priv, RKCANFD_REG_FD_DATA_BITTIMING, reg_dbt);
>  
> +	/* RK3588 CAN-FD BRS works with TDC disabled. */
> +	if (priv->devtype_data.model == RKCANFD_MODEL_RK3588) {
> +		rkcanfd_write(priv, RKCANFD_REG_TRANSMIT_DELAY_COMPENSATION, 0);
> +		return 0;
> +	}
> +

[Severity: High]
This isn't a bug introduced by this patch, but it appears there is a
pre-existing issue with how CAN-FD is enabled that affects both RK3568 and
the newly added RK3588 controllers.

In drivers/net/can/rockchip/rockchip_canfd-core.c:rkcanfd_chip_start(), the
hardware is unconditionally configured to enable CAN-FD mode by setting
RKCANFD_REG_MODE_CAN_FD_MODE_ENABLE.

However, in rkcanfd_set_bittiming() (just above the changes in this hunk),
if the user configures the interface in Classic CAN mode, the initialization
of data bit timing registers is skipped:

	if (!(priv->can.ctrlmode & CAN_CTRLMODE_FD))
		return 0;

Does this leave the hardware configured to process CAN-FD frames but with
uninitialized data bit timing?

If a valid CAN-FD frame with BRS is sent by another node on the bus, could
this controller attempt to switch to the uninitialized data bit timing, fail,
and actively transmit an error frame, disrupting valid CAN-FD traffic?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260730-master-v6-0-dfb63e234775@qq.com?part=2