On 2022-01-07 10:18, Johannes Berg wrote:
On Fri, 2022-01-07 at 10:36 +0300, Dan Carpenter wrote:
Unfortunately this code has stumbled into some deep C standards
nonsense. These two structs have a 3 byte struct hole at the end. If
you partially initialize a struct then the C standard specifies that
all the struct holes are zeroed out. But when you initialize all the
members of the struct, as this code does, then struct holes may be left
with uninitialized stack data. This is from C11 section 6.7.9 and how
it is implemented in GCC.
Wow, nice find ...
+ memset(&data, 0, sizeof(data));
+ data.cmd = cpu_to_le32(MURU_SET_TXC_TX_STATS_EN);
+ data.enable = enabled;
Maybe add a comment? This is not going to be obvious in the future.
return mt76_mcu_send_msg(&dev->mt76, MCU_EXT_CMD(MURU_CTRL),
&data,
sizeof(data), false);
Or maybe instead just mark the thing __packed (and/or explicitly add the
padding if needed), it seems weird that we'd send something to the
*firmware* that has a struct layout subject to compiler/arch padding
rules.
I would also prefer explicitly adding the padding and leaving the rest
of the code as-is.
- Felix
