Re: [PATCH v2 6/8] can: rcar_canfd: Add support for Renesas RZ/G3S

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

New issues:
- [Low] The `max_aflpn` field is incorrectly configured to 127, modeling 2048 acceptance filter rules instead of the hardware's 128 rules.

Pre-existing issues:
- [High] Silent data truncation of received CAN FD frames when the interface is configured in Classical CAN mode.
--

--- Patch [6]: [PATCH v2 6/8] can: rcar_canfd: Add support for Renesas RZ/G3S ---
commit dd7ffbc8ce8271e811932ba34aecef0183db4270
Author: Claudiu Beznea <[email protected]>

can: rcar_canfd: Add support for Renesas RZ/G3S

The Renesas RZ/G3S CAN-FD controller is largely compatible with the
variant found on the Renesas RZ/G3E. The main differences are:
- the RZ/G3S provides 128 acceptance filters
- the RZ/G3S supports only two channels
- the RZ/G3S supports only CAN-FD operation and does not implement the
  bits used to select between classical CAN-only and CAN FD-only modes.
- the RZ/G3S includes an internal divider that allows the peripheral
  clock to be used as the CAN FD clock source.

Reviewed-by: Biju Das <[email protected]>
Signed-off-by: Claudiu Beznea <[email protected]>

> diff --git a/drivers/net/can/rcar/rcar_canfd.c b/drivers/net/can/rcar/rcar_canfd.c
> index 22f87491cd913..e74b72e60272e 100644
> --- a/drivers/net/can/rcar/rcar_canfd.c
> +++ b/drivers/net/can/rcar/rcar_canfd.c
> @@ -661,6 +661,26 @@ static const struct rcar_canfd_hw_info rzg2l_hw_info = {
>  	.mode_select_na = 0,
>  };
>  
> +static const struct rcar_canfd_hw_info r9a08g045_hw_info = {
> +	.nom_bittiming = &rcar_canfd_gen4_nom_bittiming_const,
> +	.data_bittiming = &rcar_canfd_gen4_data_bittiming_const,
> +	.tdc_const = &rcar_canfd_gen4_tdc_const,
> +	.regs = &rcar_gen4_regs,
> +	.sh = &rcar_gen4_shift_data,
> +	.rnc_field_width = 16,
> +	.max_aflpn = 127,

[Severity: Low]
Is this max_aflpn value correct for the RZ/G3S hardware?

The commit message mentions the RZ/G3S provides 128 acceptance filters.
Setting max_aflpn to 127 configures a mask for up to 128 pages (2048 rules).

Should this mask be 7 (for 8 pages / 128 filters) to match the hardware
specification?

> +	.max_cftml = 31,
> +	.max_channels = 2,
> +	.postdiv = 2,
> +	.shared_global_irqs = 0,
> +	.multi_channel_irqs = 1,
> +	.ch_interface_mode = 1,
> +	.shared_can_regs = 1,
> +	.external_clk = 1,
> +	.fcan_pclk = 1,
> +	.mode_select_na = 1,

[Severity: High]
This is a pre-existing issue, but does adding a controller with 
mode_select_na = 1 expose a silent data truncation issue?

If a user requests Classical CAN mode via the DT property renesas,no-can-fd,
the controller will still receive FD frames because mode_select_na prevents
hardware-level Classical CAN mode on this SoC.

When a CAN FD frame is received, rcar_canfd_rx_pkt() checks the ctrlmode:

drivers/net/can/rcar/rcar_canfd.c:rcar_canfd_rx_pkt() {
    if ((priv->can.ctrlmode & CAN_CTRLMODE_FD) &&
        sts & RCANFD_RFFDSTS_RFFDF)
        skb = alloc_canfd_skb(ndev, &cf);
    else
        skb = alloc_can_skb(ndev, (struct can_frame **)&cf);
    ...
}

Since CAN_CTRLMODE_FD is disabled, it allocates a Classical CAN skb instead
of dropping the unsupported FD frame. It then truncates the DLC and copies
up to 8 bytes:

drivers/net/can/rcar/rcar_canfd.c:rcar_canfd_rx_pkt() {
    ...
    } else {
        cf->len = can_cc_dlc2len(RCANFD_RFPTR_RFDLC(dlc));
        if (id & RCANFD_RFID_RFRTR)
            cf->can_id |= CAN_RTR_FLAG;
        else if (gpriv->info->shared_can_regs)
            rcar_canfd_get_data(priv, cf, RCANFD_F_RFDF(gpriv, ridx, 0));
    ...
}

Can this cause an FD frame to be silently truncated and passed to userspace
as a valid Classical CAN frame?

> +};
> +

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6