| Message ID | f4f8ca5cd7f039bcab816194342c7b6101e891fe.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 |
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.
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
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.
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
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.
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;
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; >
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.
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
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.
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
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.
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
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.
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.
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;
-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(-)