UAPI: net/ethtool: Avoid thousands of -Wflex-array-member-not-at-end warnings

struct ethtool_link_settings {

include/uapi/linux/ethtool.h +9 -15

-	__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;

Quoted 31 lines

  struct ethtool_link_settings {

include/uapi/linux/ethtool.h +15 -15

-	__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;

include/uapi/linux/ethtool.h enum ethtool_reset_flags { · line 2532

Quoted 4 lines

include/uapi/linux/ethtool.h +3 -0

+		__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

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]

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.

 include/linux/ethtool.h                            |  2 +-

Quoted 3 lines · 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?

Quoted 4 lines

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.

Quoted 2 lines

  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.

Quoted 5 lines

Quoted 3 lines

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.

Quoted 6 lines · On Mon, 28 Oct 2024 17:32:53 -0600 Gustavo A. R. Silva wrote:

Quoted 5 lines

Quoted 3 lines

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.

Quoted 4 lines

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.

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.

drivers/net/ethernet/cisco/enic/enic_ethtool.c +0 -0

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

drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c static int bnxt_set_link_ksettings(struct net_device *dev, · line 3028

Quoted 6 lines · On Mon, 21 Oct 2024 13:02:27 -0600 Gustavo A. R. Silva wrote:

drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +1 -1

@@ -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.

Quoted 9 lines

drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +0 -0

  	bool set_pause = false;
  	u32 speed, lanes = 0;
  	int rc = 0;

drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c +0 -0

index 7f3f5afa864f..cc43294bdc96 100644

drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c static int get_link_ksettings(struct net_device *dev, · line 666

Quoted 5 lines

drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c +1 -1

@@ -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

drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c static int set_link_ksettings(struct net_device *dev, · line 722

Quoted 8 lines

drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c +1 -1

  	/* 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

drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c static int cxgb4vf_get_link_ksettings(struct net_device *dev, · line 1440

Quoted 13 lines

drivers/net/ethernet/chelsio/cxgb4/cxgb4_ethtool.c +0 -0

  	struct link_config old_lc;
  	unsigned int fw_caps;
  	int ret = 0;

drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c +1 -1

index 2fbe0f059a0b..0d85ac342ac7 100644
@@ -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

drivers/net/ethernet/cisco/enic/enic_ethtool.c static int enic_get_ksettings(struct net_device *netdev, · line 133

Quoted 12 lines

drivers/net/ethernet/chelsio/cxgb4vf/cxgb4vf_main.c +0 -0

  	/* For the nonce, the Firmware doesn't send up Port State changes
  	 * when the Virtual Interface attached to the Port is down.  So

drivers/net/ethernet/cisco/enic/enic_ethtool.c +1 -1

index f7986f2b6a17..8670eb394fad 100644
@@ -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

Quoted 2 lines

drivers/net/ethernet/cisco/enic/enic_ethtool.c +0 -0

  	ethtool_link_ksettings_add_link_mode(ecmd, supported,
  					     10000baseT_Full);

drivers/net/ethernet/cisco/enic/enic_ethtool.c static int linkmodes_reply_size(const struct ethnl_req_info *req_base, · line 65

Quoted 6 lines

drivers/net/ethernet/cisco/enic/enic_ethtool.c +1 -1

@@ -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

drivers/net/ethernet/cisco/enic/enic_ethtool.c static int linkmodes_fill_reply(struct sk_buff *skb, · line 106

Quoted 10 lines

drivers/net/ethernet/cisco/enic/enic_ethtool.c +1 -1

  	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

Quoted 2 lines

drivers/net/ethernet/cisco/enic/enic_ethtool.c +0 -0

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

Quoted 10 lines · Jakub Kicinski

drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c static int bnxt_set_link_ksettings(struct net_device *dev, · line 3028

Quoted 6 lines · On Mon, 21 Oct 2024 13:02:27 -0600 Gustavo A. R. Silva wrote:

drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +1 -1

@@ -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;

Quoted 8 lines

net/ethtool/linkmodes.c static int linkmodes_reply_size(const struct ethnl_req_info *req_base, · line 65

Quoted 6 lines

net/ethtool/linkmodes.c +1 -1

@@ -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.

Quoted 12 lines · On Tue, 29 Oct 2024 10:55:14 -0600 Gustavo A. R. Silva wrote:

Quoted 10 lines · Jakub Kicinski

drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c static int bnxt_set_link_ksettings(struct net_device *dev, · line 3028

Quoted 6 lines · On Mon, 21 Oct 2024 13:02:27 -0600 Gustavo A. R. Silva wrote:

drivers/net/ethernet/broadcom/bnxt/bnxt_ethtool.c +1 -1

@@ -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.

Quoted 5 lines

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.

Quoted 9 lines

Quoted 8 lines

net/ethtool/linkmodes.c static int linkmodes_reply_size(const struct ethnl_req_info *req_base, · line 65

Quoted 6 lines

net/ethtool/linkmodes.c +1 -1

@@ -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

Quoted 2 lines

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.

Quoted 4 lines

Quoted 2 lines

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?

Quoted 10 lines · On Tue, 29 Oct 2024 12:18:56 -0600 Gustavo A. R. Silva wrote:

Quoted 4 lines

Quoted 2 lines

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.

Quoted 1 line

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.

Quoted 5 lines

Quoted 1 line

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;

(quoted diff) +1 -1

-    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?

Quoted 16 lines · On Tue, 29 Oct 2024 12:48:32 -0600 Gustavo A. R. Silva wrote:

Quoted 5 lines

Quoted 1 line

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;

(quoted diff) +1 -1

-    const struct ethtool_link_settings *lsettings = &ksettings->base;
+    const struct ethtool_link_settings_hdr *lsettings = &ksettings->base;

Quoted 7 lines

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.

Quoted 2 lines

       lsettings = &ksettings->base;

Quoted 2 lines

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:

(quoted diff) +1 -1

+       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.

Quoted 12 lines · 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:

(quoted diff) +1 -1

+       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;

Quoted 2 lines

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.

Quoted 4 lines

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.

Quoted 2 lines

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.

Quoted 10 lines

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.