On 7/24/25 8:26 PM, Simon Horman wrote:

On Thu, Jul 24, 2025 at 11:24:55AM +0200, Jimmy Assarsson wrote:

quoted

Add support to turn on/off CAN LEDs on device.

Signed-off-by: Jimmy Assarsson <redacted>

...

quoted

diff --git a/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c b/drivers/net/can/usb/kvaser_usb/kvaser_usb_hydra.c

...

quoted

+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 (local)

...