Re: [PATCH v2 3/3] can: ixxat_usb: add support for USB-to-CAN FD and CAN-IDM devices

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] can: ixxat_usb: unregister_candev() called on register_candev() failure path
- [Medium] can: ixxat_usb: can_fd_idm adapter advertises CAN_CTRLMODE_TDC_AUTO without tdc_const
- [Medium] can: ixxat_usb: missing le32_to_cpu() conversion when evaluating device capabilities
--

commit 2f56f582fe161622a9b788d108e941b11e03caad
Author: Stéphane Grosjean <s.grosjean@peak-system.fr>
Subject: can: ixxat_usb: add support for USB-to-CAN FD and CAN-IDM devices

This commit introduces the ixxat_usb_cl2.c driver to support the CAN(-FD)
controller operations and bittiming constants for the IXXAT CL2
communication layer. It also wires up the new usb_device_id entries and
driver descriptors in the core. Legacy hardware with newer firmware is
upgraded to use the usb2can_v2 adapter.

> diff --git a/drivers/net/can/usb/ixxat_usb/ixxat_usb_cl2.c b/drivers/net/can/usb/ixxat_usb/ixxat_usb_cl2.c
> index 0000000000000..6c08f47915373
> --- /dev/null
> +++ b/drivers/net/can/usb/ixxat_usb/ixxat_usb_cl2.c
[ ... ]
> +static int ixxat_usb_init_ctrl(struct ixxat_usb_candevice *dev)
> +{
[ ... ]
> +	if (exmode) {
> +		cmd.fdr.tdo = cpu_to_le16(dev->can.fd.tdc.tdco);

[Severity: Medium]
Will this pass a 0 offset to the device? If the adapter is missing its
tdc_const (as seen below for can_fd_idm), can_calc_tdco() will abort
and leave tdco as 0, which could negate the requested auto delay
compensation functionality on high baud rate CAN FD networks.

> +		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);
[ ... ]
> +const struct ixxat_usb_adapter can_fd_idm = {
> +	.clock = IXXAT_USB_CLOCK,
> +	.bt = &canidm_bt,
> +	.btd = &canidm_btd,
> +	.modes = IXXAT_USB_MODES_FD,

[Severity: Medium]
Since IXXAT_USB_MODES_FD includes CAN_CTRLMODE_TDC_AUTO, does this definition
also need to set .tdc = &usb2can_fd_tdc? Without it, the tdc block is left
unconfigured despite advertising the capability.

> +	.buffer_size_rx = IXXAT_USBIDM_BUFFER_SIZE_RX,
> +	.buffer_size_tx = IXXAT_USBIDM_BUFFER_SIZE_TX,


Looking at ixxat_usb_create_ctrl() in drivers/net/can/usb/ixxat_usb/ixxat_usb_core.c:

> 	if (devdata->caps.features & IXXAT_USB_CAN_FEATURE_LISTONLY)
> 		dev->can.ctrlmode_supported |= CAN_CTRLMODE_LISTENONLY;
> 
> 	if (devdata->caps.features & IXXAT_USB_CAN_FEATURE_SSM)

[Severity: Medium]
Does devdata->caps.features need a le32_to_cpu() conversion here? Since
it is a 32-bit little-endian value retrieved from the USB device, evaluating
it directly against host-endian macros could cause the capability checks
to fail on big-endian architectures, preventing the use of features like
listen-only modes.


Also in ixxat_usb_create_ctrl() in drivers/net/can/usb/ixxat_usb/ixxat_usb_core.c:

> 	err = register_candev(netdev);
> 	if (err) {
> 		dev_err(&intf->dev, "Error %d: Failed to register can device\n",
> 			err);
> 		goto free_candev;
> 	}
[ ... ]
> free_candev:
> 	/* sashiko-bot v1: missing unregister_candev() */
> 	unregister_candev(netdev);
> 	usb_set_intfdata(intf, dev->prev_dev);
> 	free_candev(netdev);

[Severity: High]
Does this crash if register_candev() failed earlier? The error path jumps
here and unconditionally calls unregister_candev() on a network device
that was never successfully registered, which could cause list corruption
and kernel panics.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260916-ixxat_usb_can-v2-0-9b892e362d9e@peak-system.fr?part=3