Re: [PATCH 3/4] can: usb: ixxat_usb: add CL2/V2 protocol layer with CAN

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] The CAN FD Transmitter Delay Compensation offset (tdo) calculation overflows its 16-bit type at low bitrates, leading to silent truncation and incorrect hardware configuration.
--

--- Patch [3]: [PATCH 3/4] can: usb: ixxat_usb: add CL2/V2 protocol layer with CAN ---
commit 41a98ff00a0f9a2ad35a45bd14b45d0c0f12d57f
Author: Stéphane Grosjean <[email protected]>

can: usb: ixxat_usb: add CL2/V2 protocol layer with CAN

This commit adds the ixxat_usb_cl2.c file to implement the CL2 transfer
protocol used by IXXAT USB-to-CAN V2 and CAN FD adapters. It exports
descriptors and initialization routines for standard and CAN FD bittiming
configurations.

> diff --git a/drivers/net/can/usb/ixxat_usb/ixxat_usb_cl2.c b/drivers/net/can/usb/ixxat_usb/ixxat_usb_cl2.c
> new file mode 100644
> index 0000000000000..6a56a2bbdbbf7
> --- /dev/null
> +++ b/drivers/net/can/usb/ixxat_usb/ixxat_usb_cl2.c
[ ... ]
> +static const struct can_bittiming_const usb2can_fd_btd = {
> +	.name = KBUILD_MODNAME,
> +	.tseg1_min = IXXAT_USB2CANFD_TSEG1_MIN_DATA,
> +	.tseg1_max = IXXAT_USB2CANFD_TSEG1_MAX_DATA,
> +	.tseg2_min = IXXAT_USB2CANFD_TSEG2_MIN_DATA,
> +	.tseg2_max = IXXAT_USB2CANFD_TSEG2_MAX_DATA,
> +	.sjw_max = IXXAT_USB2CANFD_SJW_MAX_DATA,
> +	.brp_min = IXXAT_USB2CANFD_BRP_MIN_DATA,
> +	.brp_max = IXXAT_USB2CANFD_BRP_MAX_DATA,
> +	.brp_inc = IXXAT_USB2CANFD_BRP_INC_DATA,
> +};
[ ... ]
> +static int ixxat_usb_init_ctrl(struct ixxat_usb_candevice *dev)
> +{
[ ... ]
> +	if (exmode) {
> +		u16 tdo = btd->brp * (btd->phase_seg1 + 1 + btd->prop_seg);

[Severity: Medium]
Could this Transmitter Delay Compensation offset (tdo) calculation overflow
the 16-bit unsigned integer type?

Since IXXAT_USB2CANFD_BRP_MAX_DATA is 513 and IXXAT_USB2CANFD_TSEG1_MAX_DATA
is 256, the product of btd->brp and (btd->phase_seg1 + 1 + btd->prop_seg) can
be up to 513 * (256 + 1) = 131841. This exceeds the maximum value of a u16
(65535). 

When an administrator configures a low CAN FD data bitrate, will this result
in a silent truncation and an incorrect hardware configuration being sent to
the device?

> +
> +		cmd.fdr.mode = cpu_to_le32(btmode);
> +		cmd.fdr.bps = cpu_to_le32(btd->brp);
> +		cmd.fdr.ts1 = cpu_to_le16(btd->prop_seg + btd->phase_seg1);
> +		cmd.fdr.ts2 = cpu_to_le16(btd->phase_seg2);
> +		cmd.fdr.sjw = cpu_to_le16(btd->sjw);
> +		cmd.fdr.tdo = cpu_to_le16(tdo);
> +	}
> +
> +	return ixxat_usb_send_cmd(dev, port, &cmd, snd_size, &cmd.res,
> +				  rcv_size, IXXAT_USB_CMD_TIMEOUT);
> +}

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