| Message ID | e9ccb0cd7e490bfa270a7c20979e16ff84ac91e2.1729536776.git.gustavoars@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | UAPI: net/ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings | expand |
> struct ethtool_link_settings { > - __u32 cmd; > - __u32 speed; > - __u8 duplex; > - __u8 port; > - __u8 phy_address; > - __u8 autoneg; > - __u8 mdio_support; > - __u8 eth_tp_mdix; > - __u8 eth_tp_mdix_ctrl; > - __s8 link_mode_masks_nwords; > - __u8 transceiver; > - __u8 master_slave_cfg; > - __u8 master_slave_state; > - __u8 rate_matching; > - __u32 reserved[7]; > + /* New members MUST be added within the __struct_group() macro below. */ > + __struct_group(ethtool_link_settings_hdr, hdr, /* no attrs */, > + __u32 cmd; > + __u32 speed; > + __u8 duplex; > + __u8 port; > + __u8 phy_address; > + __u8 autoneg; > + __u8 mdio_support; > + __u8 eth_tp_mdix; > + __u8 eth_tp_mdix_ctrl; > + __s8 link_mode_masks_nwords; > + __u8 transceiver; > + __u8 master_slave_cfg; > + __u8 master_slave_state; > + __u8 rate_matching; > + __u32 reserved[7]; > + ); > __u32 link_mode_masks[]; Dumb C question. What are the padding rules for a union, compared to base types? Do we know for sure the compiler is not going pad this structure differently because of the union? It is however nicely constructed. The 12 __u8 making 3 32bit words, so we have a total of 12 32bit words, or 6 64bit words, before the link_mode_masks[], so i don't think padding is technically an issue, but it would be nice to know the C standard guarantees this. Andrew
On 21/10/24 14:11, Andrew Lunn wrote: >> struct ethtool_link_settings { >> - __u32 cmd; >> - __u32 speed; >> - __u8 duplex; >> - __u8 port; >> - __u8 phy_address; >> - __u8 autoneg; >> - __u8 mdio_support; >> - __u8 eth_tp_mdix; >> - __u8 eth_tp_mdix_ctrl; >> - __s8 link_mode_masks_nwords; >> - __u8 transceiver; >> - __u8 master_slave_cfg; >> - __u8 master_slave_state; >> - __u8 rate_matching; >> - __u32 reserved[7]; >> + /* New members MUST be added within the __struct_group() macro below. */ >> + __struct_group(ethtool_link_settings_hdr, hdr, /* no attrs */, >> + __u32 cmd; >> + __u32 speed; >> + __u8 duplex; >> + __u8 port; >> + __u8 phy_address; >> + __u8 autoneg; >> + __u8 mdio_support; >> + __u8 eth_tp_mdix; >> + __u8 eth_tp_mdix_ctrl; >> + __s8 link_mode_masks_nwords; >> + __u8 transceiver; >> + __u8 master_slave_cfg; >> + __u8 master_slave_state; >> + __u8 rate_matching; >> + __u32 reserved[7]; >> + ); >> __u32 link_mode_masks[]; > > Dumb C question. What are the padding rules for a union, compared to > base types? Do we know for sure the compiler is not going pad this > structure differently because of the union? We've been using the struct_group() family of helpers in Linux for years, and we haven't seen any issues with padding an alignment. So, it seems to do its job just fine. :) Thanks -- Gustavo > > It is however nicely constructed. The 12 __u8 making 3 32bit words, so > we have a total of 12 32bit words, or 6 64bit words, before the > link_mode_masks[], so i don't think padding is technically an issue, > but it would be nice to know the C standard guarantees this. > > Andrew
diff --git a/include/uapi/linux/ethtool.h b/include/uapi/linux/ethtool.h index c405ed63acfa..fc1f54b065f9 100644 --- a/include/uapi/linux/ethtool.h +++ b/include/uapi/linux/ethtool.h @@ -2511,21 +2511,24 @@ enum ethtool_reset_flags { * autonegotiation; 0 if unknown or not applicable. Read-only. */ struct ethtool_link_settings { - __u32 cmd; - __u32 speed; - __u8 duplex; - __u8 port; - __u8 phy_address; - __u8 autoneg; - __u8 mdio_support; - __u8 eth_tp_mdix; - __u8 eth_tp_mdix_ctrl; - __s8 link_mode_masks_nwords; - __u8 transceiver; - __u8 master_slave_cfg; - __u8 master_slave_state; - __u8 rate_matching; - __u32 reserved[7]; + /* New members MUST be added within the __struct_group() macro below. */ + __struct_group(ethtool_link_settings_hdr, hdr, /* no attrs */, + __u32 cmd; + __u32 speed; + __u8 duplex; + __u8 port; + __u8 phy_address; + __u8 autoneg; + __u8 mdio_support; + __u8 eth_tp_mdix; + __u8 eth_tp_mdix_ctrl; + __s8 link_mode_masks_nwords; + __u8 transceiver; + __u8 master_slave_cfg; + __u8 master_slave_state; + __u8 rate_matching; + __u32 reserved[7]; + ); __u32 link_mode_masks[]; /* layout of link_mode_masks fields: * __u32 map_supported[link_mode_masks_nwords];
Use the `__struct_group()` helper to create a new tagged `struct ethtool_link_settings_hdr`. This structure groups together all the members of the flexible `struct ethtool_link_settings` except the flexible array. As a result, the array is effectively separated from the rest of the members without modifying the memory layout of the flexible structure. This new tagged struct will be used to fix problematic declarations of middle-flex-arrays in composite structs[1]. [1] https://git.kernel.org/linus/d88cabfd9abc Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org> --- include/uapi/linux/ethtool.h | 33 ++++++++++++++++++--------------- 1 file changed, 18 insertions(+), 15 deletions(-)