[PATCH 0/2][next] UAPI: net/ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[PATCH 0/2][next] UAPI: net/ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Gustavo A. R. Silva]

Small patch series aimed at fixing thousands of -Wflex-array-member-not-at-end
warnings by creating a new tagged struct within a flexible structure. We then
use this new struct type to fix problematic middle-flex-array declarations in
multiple composite structs, as well as to update the type of some variables in
various functions.

Gustavo A. R. Silva (2):
  UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings
  net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end
    warnings

 .../net/ethernet/broadcom/bnxt/bnxt_ethtool.c |  6 ++--
 .../ethernet/chelsio/cxgb4/cxgb4_ethtool.c    |  4 +--
 .../ethernet/chelsio/cxgb4vf/cxgb4vf_main.c   |  2 +-
 .../net/ethernet/cisco/enic/enic_ethtool.c    |  2 +-
 .../net/ethernet/qlogic/qede/qede_ethtool.c   |  4 +--
 include/linux/ethtool.h                       |  2 +-
 include/uapi/linux/ethtool.h                  | 33 ++++++++++---------
 net/ethtool/ioctl.c                           |  2 +-
 net/ethtool/linkinfo.c                        |  8 ++---
 net/ethtool/linkmodes.c                       | 14 ++++----
 10 files changed, 40 insertions(+), 37 deletions(-)

-- 
2.34.1

[PATCH 1/2][next] UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings

[Gustavo A. R. Silva]

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

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];
-- 
2.34.1

Re: [PATCH 1/2][next] UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings

[Andrew Lunn]

>  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

Re: [PATCH 1/2][next] UAPI: ethtool: Use __struct_group() in struct ethtool_link_settings

[Gustavo A. R. Silva]

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

[PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Gustavo A. R. Silva]

-Wflex-array-member-not-at-end was introduced in GCC-14, and we are
getting ready to enable it, globally.

Change the type of the middle struct member currently causing
trouble from `struct ethtool_link_settings` to `struct
ethtool_link_settings_hdr`.

Additionally, update the type of some variables in various functions
that don't access the flexible-array member, changing them to the
newly created `struct ethtool_link_settings_hdr`.

Fix 3338 of the following -Wflex-array-member-not-at-end warnings:

include/linux/ethtool.h:214:38: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]

Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c  |  6 +++---
 drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c |  4 ++--
 .../net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c    |  2 +-
 drivers/net/ethernet/cisco/enic/enic_ethtool.c     |  2 +-
 drivers/net/ethernet/qlogic/qede/qede_ethtool.c    |  4 ++--
 include/linux/ethtool.h                            |  2 +-
 net/ethtool/ioctl.c                                |  2 +-
 net/ethtool/linkinfo.c                             |  8 ++++----
 net/ethtool/linkmodes.c                            | 14 +++++++-------
 9 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
index f71cc8188b4e..fed7fc38b0ff 100644
--- a/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
+++ b/drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c
@@ -2781,7 +2781,7 @@ u32 bnxt_fw_to_ethtool_speed(u16 fw_link_speed)
 static void bnxt_get_default_speeds(struct ethtool_link_ksettings *lk_ksettings,
 				    struct bnxt_link_info *link_info)
 {
-	struct ethtool_link_settings *base = &lk_ksettings->base;
+	struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
 
 	if (link_info->link_state == BNXT_LINK_STATE_UP) {
 		base->speed = bnxt_fw_to_ethtool_speed(link_info->link_speed);
@@ -2800,7 +2800,7 @@ static void bnxt_get_default_speeds(struct ethtool_link_ksettings *lk_ksettings,
 static int bnxt_get_link_ksettings(struct net_device *dev,
 				   struct ethtool_link_ksettings *lk_ksettings)
 {
-	struct ethtool_link_settings *base = &lk_ksettings->base;
+	struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
 	enum ethtool_link_mode_bit_indices link_mode;
 	struct bnxt *bp = netdev_priv(dev);
 	struct bnxt_link_info *link_info;
@@ -3025,7 +3025,7 @@ static int bnxt_set_link_ksettings(struct net_device *dev,
 {
 	struct bnxt *bp = netdev_priv(dev);
 	struct bnxt_link_info *link_info = &bp->link_info;
-	const struct ethtool_link_settings *base = &lk_ksettings->base;
+	const struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
 	bool set_pause = false;
 	u32 speed, lanes = 0;
 	int rc = 0;
diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
index 7f3f5afa864f..cc43294bdc96 100644
--- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
+++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
@@ -663,7 +663,7 @@ static int get_link_ksettings(struct net_device *dev,
 			      struct ethtool_link_ksettings *link_ksettings)
 {
 	struct port_info *pi = netdev_priv(dev);
-	struct ethtool_link_settings *base = &link_ksettings->base;
+	struct ethtool_link_settings_hdr *base = &link_ksettings->base;
 
 	/* For the nonce, the Firmware doesn't send up Port State changes
 	 * when the Virtual Interface attached to the Port is down.  So
@@ -719,7 +719,7 @@ static int set_link_ksettings(struct net_device *dev,
 {
 	struct port_info *pi = netdev_priv(dev);
 	struct link_config *lc = &pi->link_cfg;
-	const struct ethtool_link_settings *base = &link_ksettings->base;
+	const struct ethtool_link_settings_hdr *base = &link_ksettings->base;
 	struct link_config old_lc;
 	unsigned int fw_caps;
 	int ret = 0;
diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
index 2fbe0f059a0b..0d85ac342ac7 100644
--- a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
+++ b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
@@ -1437,7 +1437,7 @@ static int cxgb4vf_get_link_ksettings(struct net_device *dev,
 				  struct ethtool_link_ksettings *link_ksettings)
 {
 	struct port_info *pi = netdev_priv(dev);
-	struct ethtool_link_settings *base = &link_ksettings->base;
+	struct ethtool_link_settings_hdr *base = &link_ksettings->base;
 
 	/* For the nonce, the Firmware doesn't send up Port State changes
 	 * when the Virtual Interface attached to the Port is down.  So
diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
index f7986f2b6a17..8670eb394fad 100644
--- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
+++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
@@ -130,7 +130,7 @@ static int enic_get_ksettings(struct net_device *netdev,
 			      struct ethtool_link_ksettings *ecmd)
 {
 	struct enic *enic = netdev_priv(netdev);
-	struct ethtool_link_settings *base = &ecmd->base;
+	struct ethtool_link_settings_hdr *base = &ecmd->base;
 
 	ethtool_link_ksettings_add_link_mode(ecmd, supported,
 					     10000baseT_Full);
diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
index 97b059be1041..24ff154285ac 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
@@ -508,7 +508,7 @@ static int qede_get_link_ksettings(struct net_device *dev,
 				   struct ethtool_link_ksettings *cmd)
 {
 	typeof(cmd->link_modes) *link_modes = &cmd->link_modes;
-	struct ethtool_link_settings *base = &cmd->base;
+	struct ethtool_link_settings_hdr *base = &cmd->base;
 	struct qede_dev *edev = netdev_priv(dev);
 	struct qed_link_output current_link;
 
@@ -541,7 +541,7 @@ static int qede_get_link_ksettings(struct net_device *dev,
 static int qede_set_link_ksettings(struct net_device *dev,
 				   const struct ethtool_link_ksettings *cmd)
 {
-	const struct ethtool_link_settings *base = &cmd->base;
+	const struct ethtool_link_settings_hdr *base = &cmd->base;
 	const struct ethtool_forced_speed_map *map;
 	struct qede_dev *edev = netdev_priv(dev);
 	struct qed_link_output current_link;
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 12f6dc567598..1199e308c8dd 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -211,7 +211,7 @@ void ethtool_rxfh_context_lost(struct net_device *dev, u32 context_id);
  * fields, but they are allowed to overwrite them (will be ignored).
  */
 struct ethtool_link_ksettings {
-	struct ethtool_link_settings base;
+	struct ethtool_link_settings_hdr base;
 	struct {
 		__ETHTOOL_DECLARE_LINK_MODE_MASK(supported);
 		__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising);
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 04b34dc6b369..00f63640f02e 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -425,7 +425,7 @@ convert_link_ksettings_to_legacy_settings(
 
 /* layout of the struct passed from/to userland */
 struct ethtool_link_usettings {
-	struct ethtool_link_settings base;
+	struct ethtool_link_settings_hdr base;
 	struct {
 		__u32 supported[__ETHTOOL_LINK_MODE_MASK_NU32];
 		__u32 advertising[__ETHTOOL_LINK_MODE_MASK_NU32];
diff --git a/net/ethtool/linkinfo.c b/net/ethtool/linkinfo.c
index 30b8ce275159..2d5bc57160be 100644
--- a/net/ethtool/linkinfo.c
+++ b/net/ethtool/linkinfo.c
@@ -8,9 +8,9 @@ struct linkinfo_req_info {
 };
 
 struct linkinfo_reply_data {
-	struct ethnl_reply_data		base;
-	struct ethtool_link_ksettings	ksettings;
-	struct ethtool_link_settings	*lsettings;
+	struct ethnl_reply_data			base;
+	struct ethtool_link_ksettings		ksettings;
+	struct ethtool_link_settings_hdr	*lsettings;
 };
 
 #define LINKINFO_REPDATA(__reply_base) \
@@ -98,7 +98,7 @@ static int
 ethnl_set_linkinfo(struct ethnl_req_info *req_info, struct genl_info *info)
 {
 	struct ethtool_link_ksettings ksettings = {};
-	struct ethtool_link_settings *lsettings;
+	struct ethtool_link_settings_hdr *lsettings;
 	struct net_device *dev = req_info->dev;
 	struct nlattr **tb = info->attrs;
 	bool mod = false;
diff --git a/net/ethtool/linkmodes.c b/net/ethtool/linkmodes.c
index 259cd9ef1f2a..1eb7a4c604a8 100644
--- a/net/ethtool/linkmodes.c
+++ b/net/ethtool/linkmodes.c
@@ -11,10 +11,10 @@ struct linkmodes_req_info {
 };
 
 struct linkmodes_reply_data {
-	struct ethnl_reply_data		base;
-	struct ethtool_link_ksettings	ksettings;
-	struct ethtool_link_settings	*lsettings;
-	bool				peer_empty;
+	struct ethnl_reply_data			base;
+	struct ethtool_link_ksettings		ksettings;
+	struct ethtool_link_settings_hdr	*lsettings;
+	bool					peer_empty;
 };
 
 #define LINKMODES_REPDATA(__reply_base) \
@@ -62,7 +62,7 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base,
 {
 	const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
 	const struct ethtool_link_ksettings *ksettings = &data->ksettings;
-	const struct ethtool_link_settings *lsettings = &ksettings->base;
+	const struct ethtool_link_settings_hdr *lsettings = &ksettings->base;
 	bool compact = req_base->flags & ETHTOOL_FLAG_COMPACT_BITSETS;
 	int len, ret;
 
@@ -103,7 +103,7 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
 {
 	const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
 	const struct ethtool_link_ksettings *ksettings = &data->ksettings;
-	const struct ethtool_link_settings *lsettings = &ksettings->base;
+	const struct ethtool_link_settings_hdr *lsettings = &ksettings->base;
 	bool compact = req_base->flags & ETHTOOL_FLAG_COMPACT_BITSETS;
 	int ret;
 
@@ -237,7 +237,7 @@ static int ethnl_update_linkmodes(struct genl_info *info, struct nlattr **tb,
 				  struct ethtool_link_ksettings *ksettings,
 				  bool *mod, const struct net_device *dev)
 {
-	struct ethtool_link_settings *lsettings = &ksettings->base;
+	struct ethtool_link_settings_hdr *lsettings = &ksettings->base;
 	bool req_speed, req_lanes, req_duplex;
 	const struct nlattr *master_slave_cfg, *lanes_cfg;
 	int ret;
-- 
2.34.1

Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Jakub Kicinski]

On Mon, 21 Oct 2024 13:02:27 -0600 Gustavo A. R. Silva wrote:
> Fix 3338 of the following -Wflex-array-member-not-at-end warnings:
> 
> include/linux/ethtool.h:214:38: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]

I don't see any change in the number of warnings with W=1:
gcc (GCC) 14.2.1 20240912 (Red Hat 14.2.1-3)
Is it only enabled with W=2?

> Additionally, update the type of some variables in various functions
> that don't access the flexible-array member, changing them to the
> newly created `struct ethtool_link_settings_hdr`.

Why? Please avoid unnecessary code changes.

>  include/linux/ethtool.h                            |  2 +-

This is probably where most of the warnings come from.
Please split the changes to this header file as a separate patch 
for ease of review / validation.

Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Gustavo A. R. Silva]

On 28/10/24 17:21, Jakub Kicinski wrote:
> On Mon, 21 Oct 2024 13:02:27 -0600 Gustavo A. R. Silva wrote:
>> Fix 3338 of the following -Wflex-array-member-not-at-end warnings:
>>
>> include/linux/ethtool.h:214:38: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
> 
> I don't see any change in the number of warnings with W=1:
> gcc (GCC) 14.2.1 20240912 (Red Hat 14.2.1-3)
> Is it only enabled with W=2?

-Wfamnae is not currently part of any upstream build. We are working
to have it enabled. So, these warnings are the ones I see in my local
build with the following patch applied:

diff --git a/Makefile b/Makefile
index d4a41c44e0fc..08d18b5d01f5 100644
--- a/Makefile
+++ b/Makefile
@@ -1002,6 +1002,9 @@ NOSTDINC_FLAGS += -nostdinc
  # perform bounds checking.
  KBUILD_CFLAGS += $(call cc-option, -fstrict-flex-arrays=3)

+# Avoid flexible-array members not at the end of composite structure.
+KBUILD_CFLAGS += $(call cc-option, -Wflex-array-member-not-at-end)
+
  #Currently, disable -Wstringop-overflow for GCC 11, globally.
  KBUILD_CFLAGS-$(CONFIG_CC_NO_STRINGOP_OVERFLOW) += $(call cc-option, -Wno-stringop-overflow)
  KBUILD_CFLAGS-$(CONFIG_CC_STRINGOP_OVERFLOW) += $(call cc-option, -Wstringop-overflow)


> 
>> Additionally, update the type of some variables in various functions
>> that don't access the flexible-array member, changing them to the
>> newly created `struct ethtool_link_settings_hdr`.
> 
> Why? Please avoid unnecessary code changes.

This is actually necessary. As the type of the conflicting middle members
changed, those instances that expect the type to be `struct ethtool_link_settings`
should be adjusted to the new type. Another option is to leave the type
unchanged and instead use container_of. See below.

So, instead of this:

-	struct ethtool_link_settings *base = &link_ksettings->base;
+	struct ethtool_link_settings_hdr *base = &link_ksettings->base;

we would do something like this:

-	struct ethtool_link_settings *base = &link_ksettings->base;
+	struct ethtool_link_settings *base = container_of(&link_ksettings->base,
+							  struct struct ethtool_link_settings, hdr);

I think that in this case, we could avoid using `container_of()`, but
if you prefer that, I can update the patch.


> 
>>   include/linux/ethtool.h                            |  2 +-
> 
> This is probably where most of the warnings come from.
> Please split the changes to this header file as a separate patch
> for ease of review / validation.
> 

Sure thing!

Thanks
--
Gustavo

Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Jakub Kicinski]

On Mon, 28 Oct 2024 17:32:53 -0600 Gustavo A. R. Silva wrote:
> >> Additionally, update the type of some variables in various functions
> >> that don't access the flexible-array member, changing them to the
> >> newly created `struct ethtool_link_settings_hdr`.  
> > 
> > Why? Please avoid unnecessary code changes.  
> 
> This is actually necessary. As the type of the conflicting middle members
> changed, those instances that expect the type to be `struct ethtool_link_settings`
> should be adjusted to the new type. Another option is to leave the type
> unchanged and instead use container_of. See below.

Ah, that makes sense. So they need to be included int the newly split
patch. Please rephrase the commit message a bit, the current paragraph
reads as if this was a code cleanup.

Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Gustavo A. R. Silva]

On 28/10/24 18:32, Jakub Kicinski wrote:
> On Mon, 28 Oct 2024 17:32:53 -0600 Gustavo A. R. Silva wrote:
>>>> Additionally, update the type of some variables in various functions
>>>> that don't access the flexible-array member, changing them to the
>>>> newly created `struct ethtool_link_settings_hdr`.
>>>
>>> Why? Please avoid unnecessary code changes.
>>
>> This is actually necessary. As the type of the conflicting middle members
>> changed, those instances that expect the type to be `struct ethtool_link_settings`
>> should be adjusted to the new type. Another option is to leave the type
>> unchanged and instead use container_of. See below.
> 
> Ah, that makes sense. So they need to be included int the newly split
> patch. Please rephrase the commit message a bit, the current paragraph
> reads as if this was a code cleanup.

After double-checking, it turns out that the patch ends up being basically
the same. The only change that would be split in a separate patch would be
the following.

diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 5cc131cdb1bc..7da94e26ced6 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -425,7 +425,7 @@ convert_link_ksettings_to_legacy_settings(

  /* layout of the struct passed from/to userland */
  struct ethtool_link_usettings {
-       struct ethtool_link_settings base;
+       struct ethtool_link_settings_hdr base;
         struct {
                 __u32 supported[__ETHTOOL_LINK_MODE_MASK_NU32];
                 __u32 advertising[__ETHTOOL_LINK_MODE_MASK_NU32];

The rest will essentially remain the same as the change in
include/linux/ethtool.h triggers a cascade of changes across
the rest of the files in this patch.

So, you tell me if you still want me to split this patch. In any case
I'll update the changelog text.

Thanks
--
Gustavo

Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Jakub Kicinski]

On Mon, 28 Oct 2024 20:37:13 -0600 Gustavo A. R. Silva wrote:
> The rest will essentially remain the same as the change in
> include/linux/ethtool.h triggers a cascade of changes across
> the rest of the files in this patch.
> 
> So, you tell me if you still want me to split this patch. In any case
> I'll update the changelog text.

Unpleasantness. Okay. Update the commit message and I'll give you a few
more nit picks related to variable ordering.

Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Jakub Kicinski]

On Mon, 21 Oct 2024 13:02:27 -0600 Gustavo A. R. Silva wrote:
> @@ -3025,7 +3025,7 @@ static int bnxt_set_link_ksettings(struct net_device *dev,
>  {
>  	struct bnxt *bp = netdev_priv(dev);
>  	struct bnxt_link_info *link_info = &bp->link_info;
> -	const struct ethtool_link_settings *base = &lk_ksettings->base;
> +	const struct ethtool_link_settings_hdr *base = &lk_ksettings->base;

Please improve the variable ordering while at it. Longest list first,
so move the @base definition first.

>  	bool set_pause = false;
>  	u32 speed, lanes = 0;
>  	int rc = 0;
> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> index 7f3f5afa864f..cc43294bdc96 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
> @@ -663,7 +663,7 @@ static int get_link_ksettings(struct net_device *dev,
>  			      struct ethtool_link_ksettings *link_ksettings)
>  {
>  	struct port_info *pi = netdev_priv(dev);
> -	struct ethtool_link_settings *base = &link_ksettings->base;
> +	struct ethtool_link_settings_hdr *base = &link_ksettings->base;

ditto

>  	/* For the nonce, the Firmware doesn't send up Port State changes
>  	 * when the Virtual Interface attached to the Port is down.  So
> @@ -719,7 +719,7 @@ static int set_link_ksettings(struct net_device *dev,
>  {
>  	struct port_info *pi = netdev_priv(dev);
>  	struct link_config *lc = &pi->link_cfg;
> -	const struct ethtool_link_settings *base = &link_ksettings->base;
> +	const struct ethtool_link_settings_hdr *base = &link_ksettings->base;

and here

>  	struct link_config old_lc;
>  	unsigned int fw_caps;
>  	int ret = 0;
> diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
> index 2fbe0f059a0b..0d85ac342ac7 100644
> --- a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
> +++ b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
> @@ -1437,7 +1437,7 @@ static int cxgb4vf_get_link_ksettings(struct net_device *dev,
>  				  struct ethtool_link_ksettings *link_ksettings)
>  {
>  	struct port_info *pi = netdev_priv(dev);
> -	struct ethtool_link_settings *base = &link_ksettings->base;
> +	struct ethtool_link_settings_hdr *base = &link_ksettings->base;

and here

>  	/* For the nonce, the Firmware doesn't send up Port State changes
>  	 * when the Virtual Interface attached to the Port is down.  So
> diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> index f7986f2b6a17..8670eb394fad 100644
> --- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> +++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
> @@ -130,7 +130,7 @@ static int enic_get_ksettings(struct net_device *netdev,
>  			      struct ethtool_link_ksettings *ecmd)
>  {
>  	struct enic *enic = netdev_priv(netdev);
> -	struct ethtool_link_settings *base = &ecmd->base;
> +	struct ethtool_link_settings_hdr *base = &ecmd->base;

and here

>  	ethtool_link_ksettings_add_link_mode(ecmd, supported,
>  					     10000baseT_Full);

> @@ -62,7 +62,7 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base,
>  {
>  	const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
>  	const struct ethtool_link_ksettings *ksettings = &data->ksettings;
> -	const struct ethtool_link_settings *lsettings = &ksettings->base;
> +	const struct ethtool_link_settings_hdr *lsettings = &ksettings->base;

here it was correct and now its not

>  	bool compact = req_base->flags & ETHTOOL_FLAG_COMPACT_BITSETS;
>  	int len, ret;
>  
> @@ -103,7 +103,7 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
>  {
>  	const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
>  	const struct ethtool_link_ksettings *ksettings = &data->ksettings;
> -	const struct ethtool_link_settings *lsettings = &ksettings->base;
> +	const struct ethtool_link_settings_hdr *lsettings = &ksettings->base;

same

>  	bool compact = req_base->flags & ETHTOOL_FLAG_COMPACT_BITSETS;
>  	int ret;

Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Gustavo A. R. Silva]

On 29/10/24 07:58, Jakub Kicinski wrote:
> On Mon, 21 Oct 2024 13:02:27 -0600 Gustavo A. R. Silva wrote:
>> @@ -3025,7 +3025,7 @@ static int bnxt_set_link_ksettings(struct net_device *dev,
>>   {
>>   	struct bnxt *bp = netdev_priv(dev);
>>   	struct bnxt_link_info *link_info = &bp->link_info;
>> -	const struct ethtool_link_settings *base = &lk_ksettings->base;
>> +	const struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
> 
> Please improve the variable ordering while at it. Longest list first,
> so move the @base definition first.

OK. This would end up looking like:

	const struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
	struct bnxt *bp = netdev_priv(dev);
	struct bnxt_link_info *link_info = &bp->link_info;



> 
>>   	bool set_pause = false;
>>   	u32 speed, lanes = 0;
>>   	int rc = 0;
>> diff --git a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
>> index 7f3f5afa864f..cc43294bdc96 100644
>> --- a/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
>> +++ b/drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c
>> @@ -663,7 +663,7 @@ static int get_link_ksettings(struct net_device *dev,
>>   			      struct ethtool_link_ksettings *link_ksettings)
>>   {
>>   	struct port_info *pi = netdev_priv(dev);
>> -	struct ethtool_link_settings *base = &link_ksettings->base;
>> +	struct ethtool_link_settings_hdr *base = &link_ksettings->base;
> 
> ditto
> 
>>   	/* For the nonce, the Firmware doesn't send up Port State changes
>>   	 * when the Virtual Interface attached to the Port is down.  So
>> @@ -719,7 +719,7 @@ static int set_link_ksettings(struct net_device *dev,
>>   {
>>   	struct port_info *pi = netdev_priv(dev);
>>   	struct link_config *lc = &pi->link_cfg;
>> -	const struct ethtool_link_settings *base = &link_ksettings->base;
>> +	const struct ethtool_link_settings_hdr *base = &link_ksettings->base;
> 
> and here
> 
>>   	struct link_config old_lc;
>>   	unsigned int fw_caps;
>>   	int ret = 0;
>> diff --git a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
>> index 2fbe0f059a0b..0d85ac342ac7 100644
>> --- a/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
>> +++ b/drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c
>> @@ -1437,7 +1437,7 @@ static int cxgb4vf_get_link_ksettings(struct net_device *dev,
>>   				  struct ethtool_link_ksettings *link_ksettings)
>>   {
>>   	struct port_info *pi = netdev_priv(dev);
>> -	struct ethtool_link_settings *base = &link_ksettings->base;
>> +	struct ethtool_link_settings_hdr *base = &link_ksettings->base;
> 
> and here
> 
>>   	/* For the nonce, the Firmware doesn't send up Port State changes
>>   	 * when the Virtual Interface attached to the Port is down.  So
>> diff --git a/drivers/net/ethernet/cisco/enic/enic_ethtool.c b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
>> index f7986f2b6a17..8670eb394fad 100644
>> --- a/drivers/net/ethernet/cisco/enic/enic_ethtool.c
>> +++ b/drivers/net/ethernet/cisco/enic/enic_ethtool.c
>> @@ -130,7 +130,7 @@ static int enic_get_ksettings(struct net_device *netdev,
>>   			      struct ethtool_link_ksettings *ecmd)
>>   {
>>   	struct enic *enic = netdev_priv(netdev);
>> -	struct ethtool_link_settings *base = &ecmd->base;
>> +	struct ethtool_link_settings_hdr *base = &ecmd->base;
> 
> and here
> 
>>   	ethtool_link_ksettings_add_link_mode(ecmd, supported,
>>   					     10000baseT_Full);
> 
>> @@ -62,7 +62,7 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base,
>>   {
>>   	const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
>>   	const struct ethtool_link_ksettings *ksettings = &data->ksettings;
>> -	const struct ethtool_link_settings *lsettings = &ksettings->base;
>> +	const struct ethtool_link_settings_hdr *lsettings = &ksettings->base;
> 
> here it was correct and now its not

I don't think you want to change this. `lsettings` is based on `ksettings`. So,
`ksettings` should go first. The same scenario for the one below.

> 
>>   	bool compact = req_base->flags & ETHTOOL_FLAG_COMPACT_BITSETS;
>>   	int len, ret;
>>   
>> @@ -103,7 +103,7 @@ static int linkmodes_fill_reply(struct sk_buff *skb,
>>   {
>>   	const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
>>   	const struct ethtool_link_ksettings *ksettings = &data->ksettings;
>> -	const struct ethtool_link_settings *lsettings = &ksettings->base;
>> +	const struct ethtool_link_settings_hdr *lsettings = &ksettings->base;
> 
> same
> 
>>   	bool compact = req_base->flags & ETHTOOL_FLAG_COMPACT_BITSETS;
>>   	int ret;
> 

Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Jakub Kicinski]

On Tue, 29 Oct 2024 10:55:14 -0600 Gustavo A. R. Silva wrote:
> On 29/10/24 07:58, Jakub Kicinski wrote:
> > On Mon, 21 Oct 2024 13:02:27 -0600 Gustavo A. R. Silva wrote:  
> >> @@ -3025,7 +3025,7 @@ static int bnxt_set_link_ksettings(struct net_device *dev,
> >>   {
> >>   	struct bnxt *bp = netdev_priv(dev);
> >>   	struct bnxt_link_info *link_info = &bp->link_info;
> >> -	const struct ethtool_link_settings *base = &lk_ksettings->base;
> >> +	const struct ethtool_link_settings_hdr *base = &lk_ksettings->base;  
> > 
> > Please improve the variable ordering while at it. Longest list first,
> > so move the @base definition first.  
> 
> OK. This would end up looking like:
> 
> 	const struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
> 	struct bnxt *bp = netdev_priv(dev);
> 	struct bnxt_link_info *link_info = &bp->link_info;

Correct, one step at a time.

> >> @@ -62,7 +62,7 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base,
> >>   {
> >>   	const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
> >>   	const struct ethtool_link_ksettings *ksettings = &data->ksettings;
> >> -	const struct ethtool_link_settings *lsettings = &ksettings->base;
> >> +	const struct ethtool_link_settings_hdr *lsettings = &ksettings->base;  
> > 
> > here it was correct and now its not  
> 
> I don't think you want to change this. `lsettings` is based on `ksettings`. So,
> `ksettings` should go first. The same scenario for the one below.

In which case you need to move the init out of line.

Thanks.

Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Gustavo A. R. Silva]

On 29/10/24 12:08, Jakub Kicinski wrote:
> On Tue, 29 Oct 2024 10:55:14 -0600 Gustavo A. R. Silva wrote:
>> On 29/10/24 07:58, Jakub Kicinski wrote:
>>> On Mon, 21 Oct 2024 13:02:27 -0600 Gustavo A. R. Silva wrote:
>>>> @@ -3025,7 +3025,7 @@ static int bnxt_set_link_ksettings(struct net_device *dev,
>>>>    {
>>>>    	struct bnxt *bp = netdev_priv(dev);
>>>>    	struct bnxt_link_info *link_info = &bp->link_info;
>>>> -	const struct ethtool_link_settings *base = &lk_ksettings->base;
>>>> +	const struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
>>>
>>> Please improve the variable ordering while at it. Longest list first,
>>> so move the @base definition first.
>>
>> OK. This would end up looking like:
>>
>> 	const struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
>> 	struct bnxt *bp = netdev_priv(dev);
>> 	struct bnxt_link_info *link_info = &bp->link_info;
> 
> Correct, one step at a time.
> 
>>>> @@ -62,7 +62,7 @@ static int linkmodes_reply_size(const struct ethnl_req_info *req_base,
>>>>    {
>>>>    	const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
>>>>    	const struct ethtool_link_ksettings *ksettings = &data->ksettings;
>>>> -	const struct ethtool_link_settings *lsettings = &ksettings->base;
>>>> +	const struct ethtool_link_settings_hdr *lsettings = &ksettings->base;
>>>
>>> here it was correct and now its not
>>
>> I don't think you want to change this. `lsettings` is based on `ksettings`. So,
>> `ksettings` should go first. The same scenario for the one below.
> 
> In which case you need to move the init out of line.

So, the same applies to the case below?

	const struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
	struct bnxt *bp = netdev_priv(dev);
	struct bnxt_link_info *link_info = &bp->link_info;

Is this going to be a priority for any other netdev patches in the future?

--
Gustavo

Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Jakub Kicinski]

On Tue, 29 Oct 2024 12:18:56 -0600 Gustavo A. R. Silva wrote:
> >> I don't think you want to change this. `lsettings` is based on `ksettings`. So,
> >> `ksettings` should go first. The same scenario for the one below.  
> > 
> > In which case you need to move the init out of line.  
> 
> So, the same applies to the case below?
> 
> 	const struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
> 	struct bnxt *bp = netdev_priv(dev);
> 	struct bnxt_link_info *link_info = &bp->link_info;

Do you mean the bp and bp->link_info lines?
You're not touching them, so leave them be.

> Is this going to be a priority for any other netdev patches in the future?

It's been the preferred formatting for a decade or more.
Which is why the net/ethtool/ code you're touching follows
this convention. We're less strict about driver code.

Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Gustavo A. R. Silva]

On 29/10/24 12:39, Jakub Kicinski wrote:
> On Tue, 29 Oct 2024 12:18:56 -0600 Gustavo A. R. Silva wrote:
>>>> I don't think you want to change this. `lsettings` is based on `ksettings`. So,
>>>> `ksettings` should go first. The same scenario for the one below.
>>>
>>> In which case you need to move the init out of line.
>>
>> So, the same applies to the case below?
>>
>> 	const struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
>> 	struct bnxt *bp = netdev_priv(dev);
>> 	struct bnxt_link_info *link_info = &bp->link_info;
> 
> Do you mean the bp and bp->link_info lines?
> You're not touching them, so leave them be.
> 
>> Is this going to be a priority for any other netdev patches in the future?
> 
> It's been the preferred formatting for a decade or more.
> Which is why the net/ethtool/ code you're touching follows
> this convention. We're less strict about driver code.

I mean, the thing about moving the initialization out of line to accommodate
for the convention.

What I'm understanding is that now you're asking me to change the following

      const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
      const struct ethtool_link_ksettings *ksettings = &data->ksettings;
-    const struct ethtool_link_settings *lsettings = &ksettings->base;
+    const struct ethtool_link_settings_hdr *lsettings = &ksettings->base;

to this:

      const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
      const struct ethtool_link_settings_hdr *lsettings;
      const struct ethtool_link_ksettings *ksettings;

      ksettings = &data->ksettings;
      lsettings = &ksettings->base;

I just want to have clear if this is going to be a priority and in which scenarios
should I/others modify the code to accommodate for the convention?

--
Gustavo

Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Jakub Kicinski]

On Tue, 29 Oct 2024 12:48:32 -0600 Gustavo A. R. Silva wrote:
> >> Is this going to be a priority for any other netdev patches in the future?  
> > 
> > It's been the preferred formatting for a decade or more.
> > Which is why the net/ethtool/ code you're touching follows
> > this convention. We're less strict about driver code.  
> 
> I mean, the thing about moving the initialization out of line to accommodate
> for the convention.
> 
> What I'm understanding is that now you're asking me to change the following
> 
>       const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
>       const struct ethtool_link_ksettings *ksettings = &data->ksettings;
> -    const struct ethtool_link_settings *lsettings = &ksettings->base;
> +    const struct ethtool_link_settings_hdr *lsettings = &ksettings->base;
> 
> to this:
> 
>       const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
>       const struct ethtool_link_settings_hdr *lsettings;
>       const struct ethtool_link_ksettings *ksettings;
> 
>       ksettings = &data->ksettings;

You don't have to move this one out of line but either way is fine.

>       lsettings = &ksettings->base;
> 
> I just want to have clear if this is going to be a priority and in which scenarios
> should I/others modify the code to accommodate for the convention?

I don't understand what you mean by priority. If you see code under
net/ or drivers/net which follows the reverse xmas tree variable
sorting you should not be breaking this convention. And yes, if
there are dependencies between variables you should move the init
out of line.

Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Gustavo A. R. Silva]

On 29/10/24 12:54, Jakub Kicinski wrote:
> On Tue, 29 Oct 2024 12:48:32 -0600 Gustavo A. R. Silva wrote:
>>>> Is this going to be a priority for any other netdev patches in the future?
>>>
>>> It's been the preferred formatting for a decade or more.
>>> Which is why the net/ethtool/ code you're touching follows
>>> this convention. We're less strict about driver code.
>>
>> I mean, the thing about moving the initialization out of line to accommodate
>> for the convention.
>>
>> What I'm understanding is that now you're asking me to change the following
>>
>>        const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
>>        const struct ethtool_link_ksettings *ksettings = &data->ksettings;
>> -    const struct ethtool_link_settings *lsettings = &ksettings->base;
>> +    const struct ethtool_link_settings_hdr *lsettings = &ksettings->base;
>>
>> to this:
>>
>>        const struct linkmodes_reply_data *data = LINKMODES_REPDATA(reply_base);
>>        const struct ethtool_link_settings_hdr *lsettings;
>>        const struct ethtool_link_ksettings *ksettings;
>>
>>        ksettings = &data->ksettings;
> 
> You don't have to move this one out of line but either way is fine.
> 
>>        lsettings = &ksettings->base;
>>
>> I just want to have clear if this is going to be a priority and in which scenarios
>> should I/others modify the code to accommodate for the convention?
> 
> I don't understand what you mean by priority. If you see code under
> net/ or drivers/net which follows the reverse xmas tree variable
> sorting you should not be breaking this convention. And yes, if
> there are dependencies between variables you should move the init
> out of line.

By priority I mean if preserving the reverse xmas tree is a most
after any changes that mess in some way with it. As in the case below,
where things were already messed up:

+       const struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
         struct bnxt *bp = netdev_priv(dev);
         struct bnxt_link_info *link_info = &bp->link_info;
-       const struct ethtool_link_settings *base = &lk_ksettings->base;
         bool set_pause = false;
         u32 speed, lanes = 0;
         int rc = 0;

Should I leave the rest as-is, or should I now have to rearrange the whole
thing to accommodate for the convention?

How I see this, we can take a couple of directions:

a) when things are already messed up, just implement your changes and leave
the rest as-is.

b) when your changes mess things up, clean it up and accommodate for the
convention.

extra option:

c) this is probably going to be a case by case thing and we may ask you
    to do more changes as we see fit.

To be clear, I have no issue with c) (because it's basically how things
usually work), as long as maintainers don't expect v1 of any patch to
be in pristine form. In any other case, I would really like to be crystal
clear about what's expected and what's not.

Thanks!
--
Gustavo

Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Jakub Kicinski]

On Tue, 29 Oct 2024 13:18:56 -0600 Gustavo A. R. Silva wrote:
> By priority I mean if preserving the reverse xmas tree is a most
> after any changes that mess in some way with it. As in the case below,
> where things were already messed up:
> 
> +       const struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
>          struct bnxt *bp = netdev_priv(dev);
>          struct bnxt_link_info *link_info = &bp->link_info;
> -       const struct ethtool_link_settings *base = &lk_ksettings->base;
>          bool set_pause = false;
>          u32 speed, lanes = 0;
>          int rc = 0;
> 
> Should I leave the rest as-is, or should I now have to rearrange the whole
> thing to accommodate for the convention?

Don't rearrange the rest. The point is that if you touch a line you end
up with a delete and an add. So you can as well move it to get it closer
to the convention. But that's just nice to have, I brought the entire
thing up because of the net/ethtool/ code which previously followed the
convention and after changes it wouldn't.

> How I see this, we can take a couple of directions:
> 
> a) when things are already messed up, just implement your changes and leave
> the rest as-is.

This is acceptable, moving things closer to convention is nice to have.

> b) when your changes mess things up, clean it up and accommodate for the
> convention.

Yes, if by "your changes mess things up" you mean that the code follows
the convention exactly for a given function - then yes, the changes must
remain complaint. Not sure why you say "clean it up", if the code is
complaint you shouldn't break it. No touching of pre-existing code
(other than modified lines) should be necessary.

> extra option:
> 
> c) this is probably going to be a case by case thing and we may ask you
>     to do more changes as we see fit.
> 
> To be clear, I have no issue with c) (because it's basically how things
> usually work), as long as maintainers don't expect v1 of any patch to
> be in pristine form. In any other case, I would really like to be crystal
> clear about what's expected and what's not.

Re: [PATCH 2/2][next] net: ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

[Gustavo A. R. Silva]

On 29/10/24 14:00, Jakub Kicinski wrote:
> On Tue, 29 Oct 2024 13:18:56 -0600 Gustavo A. R. Silva wrote:
>> By priority I mean if preserving the reverse xmas tree is a most
>> after any changes that mess in some way with it. As in the case below,
>> where things were already messed up:
>>
>> +       const struct ethtool_link_settings_hdr *base = &lk_ksettings->base;
>>           struct bnxt *bp = netdev_priv(dev);
>>           struct bnxt_link_info *link_info = &bp->link_info;
>> -       const struct ethtool_link_settings *base = &lk_ksettings->base;
>>           bool set_pause = false;
>>           u32 speed, lanes = 0;
>>           int rc = 0;
>>
>> Should I leave the rest as-is, or should I now have to rearrange the whole
>> thing to accommodate for the convention?
> 
> Don't rearrange the rest. The point is that if you touch a line you end
> up with a delete and an add. So you can as well move it to get it closer
> to the convention. But that's just nice to have, I brought the entire
> thing up because of the net/ethtool/ code which previously followed the
> convention and after changes it wouldn't.
> 
>> How I see this, we can take a couple of directions:
>>
>> a) when things are already messed up, just implement your changes and leave
>> the rest as-is.
> 
> This is acceptable, moving things closer to convention is nice to have.
> 
>> b) when your changes mess things up, clean it up and accommodate for the
>> convention.
> 
> Yes, if by "your changes mess things up" you mean that the code follows
> the convention exactly for a given function - then yes, the changes must
> remain complaint. Not sure why you say "clean it up", if the code is
> complaint you shouldn't break it. No touching of pre-existing code
> (other than modified lines) should be necessary.

Gotcha. Hopefully, this v2 is just fine:

https://lore.kernel.org/linux-hardening/cover.1730238285.git.gustavoars@kernel.org/

Thanks!
-Gustavo

> 
>> extra option:
>>
>> c) this is probably going to be a case by case thing and we may ask you
>>      to do more changes as we see fit.
>>
>> To be clear, I have no issue with c) (because it's basically how things
>> usually work), as long as maintainers don't expect v1 of any patch to
>> be in pristine form. In any other case, I would really like to be crystal
>> clear about what's expected and what's not.