| Message ID | 20230330192049.18549-1-jesse.brandeburg@intel.com |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [net,v2] ice: refactor to remove not needed packing | expand |
From: Brandeburg, Jesse <jesse.brandeburg@intel.com> Date: Thu, 30 Mar 2023 12:20:49 -0700 > After the changes to the structures to make them flex array safe, > packing is no longer necessary. > > to reproduce: > make EXTRA_CFLAGS=-Wpacked drivers/net/ethernet/intel/ice/ice.ko When have we started to fix hypothetical custom compiler flags? `-Wpacked` is enabled only on `W=3`[0], which's comment clearly says: "lots of noise, you can safely ignore those"[1]. Anyway, see below. > > In file included from drivers/net/ethernet/intel/ice/ice_controlq.h:7, > from drivers/net/ethernet/intel/ice/ice_type.h:14, > from drivers/net/ethernet/intel/ice/ice.h:59: > drivers/net/ethernet/intel/ice/ice_adminq_cmd.h:618:1: warning: packed attribute is unnecessary for ‘ice_aqc_sw_rules_elem_hdr’ [-Wpacked] > 618 | } __packed __aligned(sizeof(__le16)); > | ^ > drivers/net/ethernet/intel/ice/ice_adminq_cmd.h:705:1: warning: packed attribute is unnecessary for ‘ice_sw_rule_lkup_rx_tx’ [-Wpacked] > 705 | } __packed __aligned(sizeof(__le16)); > | ^ > drivers/net/ethernet/intel/ice/ice_adminq_cmd.h:767:1: warning: packed attribute is unnecessary for ‘ice_sw_rule_lg_act’ [-Wpacked] > 767 | } __packed __aligned(sizeof(__le16)); > | ^ > drivers/net/ethernet/intel/ice/ice_adminq_cmd.h:779:1: warning: packed attribute is unnecessary for ‘ice_sw_rule_vsi_list’ [-Wpacked] > 779 | } __packed __aligned(sizeof(__le16)); > | ^ Packing here is completely unrelated to zero-fake-flex-arrays. It's added because those structures are shared between HW and SW. In such cases, you need 1:1 match, so you can't allow the compiler do whatever it wants with paddings and stuff. Each `__packed` is balanced here by `__aligned` to not blow up the object code. The `-Wpacked` warnings come from the fact that there's nothing to pack -- the layout are organized the way that each field has natural alignment. It doesn't mean there's something wrong with the code at all. But if you have a really good reason to remove those, please add static assertions to make sure each structure's size is precisely the one that is described in the specs, otherwise you may face obscure bugs one day on some weird arch/compiler setup. And provide bloat-o-meter stats, which would clearly show that removing `__packed` improves codegen. > > Fixes: 6e1ff618737a ("ice: fix access-beyond-end in the switch code") Definitely incorrect, this change doesn't "fix" anything. > Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> > --- > v2: send to net instead of net-next [...] [0] https://elixir.bootlin.com/linux/latest/source/scripts/Makefile.extrawarn#L99 [1] https://elixir.bootlin.com/linux/latest/source/scripts/Makefile.extrawarn#L91 Thanks, Olek
diff --git a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h index 838d9b274d68..7460dcaf6473 100644 --- a/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h +++ b/drivers/net/ethernet/intel/ice/ice_adminq_cmd.h @@ -615,7 +615,7 @@ struct ice_aqc_sw_rules_elem_hdr { #define ICE_AQC_SW_RULES_T_PRUNE_LIST_SET 0x5 #define ICE_AQC_SW_RULES_T_PRUNE_LIST_CLEAR 0x6 __le16 status; -} __packed __aligned(sizeof(__le16)); +} __aligned(sizeof(__le16)); /* Add/Update/Get/Remove lookup Rx/Tx command/response entry * This structures describes the lookup rules and associated actions. "index" @@ -702,7 +702,7 @@ struct ice_sw_rule_lkup_rx_tx { */ __le16 hdr_len; u8 hdr_data[]; -} __packed __aligned(sizeof(__le16)); +} __aligned(sizeof(__le16)); /* Add/Update/Remove large action command/response entry * "index" is returned as part of a response to a successful Add command, and @@ -764,7 +764,7 @@ struct ice_sw_rule_lg_act { #define ICE_LG_ACT_STAT_COUNT_S 3 #define ICE_LG_ACT_STAT_COUNT_M (0x7F << ICE_LG_ACT_STAT_COUNT_S) __le32 act[]; /* array of size for actions */ -} __packed __aligned(sizeof(__le16)); +} __aligned(sizeof(__le16)); /* Add/Update/Remove VSI list command/response entry * "index" is returned as part of a response to a successful Add command, and @@ -776,7 +776,7 @@ struct ice_sw_rule_vsi_list { __le16 index; /* Index of VSI/Prune list */ __le16 number_vsi; __le16 vsi[]; /* Array of number_vsi VSI numbers */ -} __packed __aligned(sizeof(__le16)); +} __aligned(sizeof(__le16)); /* Query PFC Mode (direct 0x0302) * Set PFC Mode (direct 0x0303)
After the changes to the structures to make them flex array safe, packing is no longer necessary. to reproduce: make EXTRA_CFLAGS=-Wpacked drivers/net/ethernet/intel/ice/ice.ko In file included from drivers/net/ethernet/intel/ice/ice_controlq.h:7, from drivers/net/ethernet/intel/ice/ice_type.h:14, from drivers/net/ethernet/intel/ice/ice.h:59: drivers/net/ethernet/intel/ice/ice_adminq_cmd.h:618:1: warning: packed attribute is unnecessary for ‘ice_aqc_sw_rules_elem_hdr’ [-Wpacked] 618 | } __packed __aligned(sizeof(__le16)); | ^ drivers/net/ethernet/intel/ice/ice_adminq_cmd.h:705:1: warning: packed attribute is unnecessary for ‘ice_sw_rule_lkup_rx_tx’ [-Wpacked] 705 | } __packed __aligned(sizeof(__le16)); | ^ drivers/net/ethernet/intel/ice/ice_adminq_cmd.h:767:1: warning: packed attribute is unnecessary for ‘ice_sw_rule_lg_act’ [-Wpacked] 767 | } __packed __aligned(sizeof(__le16)); | ^ drivers/net/ethernet/intel/ice/ice_adminq_cmd.h:779:1: warning: packed attribute is unnecessary for ‘ice_sw_rule_vsi_list’ [-Wpacked] 779 | } __packed __aligned(sizeof(__le16)); | ^ Fixes: 6e1ff618737a ("ice: fix access-beyond-end in the switch code") Signed-off-by: Jesse Brandeburg <jesse.brandeburg@intel.com> --- v2: send to net instead of net-next --- drivers/net/ethernet/intel/ice/ice_adminq_cmd.h | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)