Re: [PATCH v2 01/11] can: kvaser_usb: Add support to control CAN LEDs on device

[Jimmy Assarsson <jimmyassarsson@gmail.com>]

On 7/24/25 8:26 PM, Simon Horman wrote:
> On Thu, Jul 24, 2025 at 11:24:55AM +0200, Jimmy Assarsson wrote:
>> Add support to turn on/off CAN LEDs on device.
>>
>> Signed-off-by: Jimmy Assarsson <extja@kvaser.com>
> 
> ...
> 
>> diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c
> 
> ...
> 
>> +static int kvaser_usb_hydra_set_led(struct kvaser_usb_net_priv *priv,
>> +				    enum kvaser_usb_led_state state,
>> +				    u16 duration_ms)
>> +{
>> +	struct kvaser_usb *dev = priv->dev;
>> +	struct kvaser_cmd *cmd;
>> +	int ret;
>> +
>> +	cmd = kzalloc(sizeof(*cmd), GFP_KERNEL);
>> +	if (!cmd)
>> +		return -ENOMEM;
>> +
>> +	cmd->header.cmd_no = CMD_LED_ACTION_REQ;
>> +	kvaser_usb_hydra_set_cmd_dest_he(cmd, dev->card_data.hydra.sysdbg_he);
>> +	kvaser_usb_hydra_set_cmd_transid(cmd, kvaser_usb_hydra_get_next_transid(dev));
>> +
>> +	cmd->led_action_req.duration_ms = cpu_to_le16(duration_ms);
>> +	cmd->led_action_req.action = state |
>> +				     FIELD_PREP(KVASER_USB_HYDRA_LED_IDX_MASK,
>> +						KVASER_USB_HYDRA_LED_YELLOW_CH0_IDX +
>> +						KVASER_USB_HYDRA_LEDS_PER_CHANNEL * priv->channel);
>> +
>> +	ret = kvaser_usb_send_cmd(dev, cmd, kvaser_usb_hydra_cmd_size(cmd));
> 
> When building this file with GCC 15.1.0 with KCFLAGS=-Warray-bounds
> I see the following:
> 
>    In file included from ./include/linux/byteorder/little_endian.h:5,
>                     from ./arch/x86/include/uapi/asm/byteorder.h:5,
>                     from ./include/linux/bitfield.h:12,
>                     from drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:15:
>    In function 'kvaser_usb_hydra_cmd_size',
>        inlined from 'kvaser_usb_hydra_set_led' at drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:1993:38:
>    drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:532:65: warning: array subscript 'struct kvaser_cmd_ext[0]' is partly outside array bounds of 'unsigned char[32]' [-Warray-bounds=]
>      532 |                 ret = le16_to_cpu(((struct kvaser_cmd_ext *)cmd)->len);
>    ./include/uapi/linux/byteorder/little_endian.h:37:51: note: in definition of macro '__le16_to_cpu'
>       37 | #define __le16_to_cpu(x) ((__force __u16)(__le16)(x))
>          |                                                   ^
>    drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:532:23: note: in expansion of macro 'le16_to_cpu'
>      532 |                 ret = le16_to_cpu(((struct kvaser_cmd_ext *)cmd)->len);
>          |                       ^~~~~~~~~~~
>    In file included from ./include/linux/fs.h:46,
>                     from ./include/linux/compat.h:17,
>                     from ./arch/x86/include/asm/ia32.h:7,
>                     from ./arch/x86/include/asm/elf.h:10,
>                     from ./include/linux/elf.h:6,
>                     from ./include/linux/module.h:19,
>                     from ./include/linux/device/driver.h:21,
>                     from ./include/linux/device.h:32,
>                     from drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:17:
>    In function 'kmalloc_noprof',
>        inlined from 'kzalloc_noprof' at ./include/linux/slab.h:1039:9,
>        inlined from 'kvaser_usb_hydra_set_led' at drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c:1979:8:
>    ./include/linux/slab.h:905:24: note: object of size 32 allocated by '__kmalloc_cache_noprof'
>      905 |                 return __kmalloc_cache_noprof(
>          |                        ^~~~~~~~~~~~~~~~~~~~~~~
>      906 |                                 kmalloc_caches[kmalloc_type(flags, _RET_IP_)][index],
>          |                                 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
>      907 |                                 flags, size);
>          |                                 ~~~~~~~~~~~~
> 
> 	if (cmd->header.cmd_no == CMD_EXTENDED)
> 		ret = le16_to_cpu(((struct kvaser_cmd_ext *)cmd)->len);
> 
> GCC seems to know that:
> * cmd was allocated sizeof(*cmd) = 32 bytes
> * struct kvaser_cmd_ext is larger than this (96 bytes)
> 
> And it thinks that cmd->header.cmd_no might be CMD_EXTENDED.
> This is not true, becuae .cmd_no is set to CMD_LED_ACTION_REQ
> earlier in kvaser_usb_hydra_set_led. But still, GCC produces
> a big fat warning.
> 
> On the one hand we might say this is a shortcoming in GCC,
> a position I agree with. But on the other hand, we might follow
> the pattern used elsewhere in this file for similar functions,
> which seems to make GCC happy, I guess, and it is strictly a guess,
> because less context is needed for it to analyse things correctly.

Thanks for finding this!

Marc Kleine-Budde actually sorted this out for other commands some years ago [1],
but I had completely forgotten.

[1] https://lore.kernel.org/all/20221219110104.1073881-1-mkl@pengutronix.de

...