netdev - Re: [PATCH net v1] dim: initialize all struct fields

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Thu, 5 May 2022 18:40:33 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Jesse Brandeburg <jesse.brandeburg@...el.com>
Cc:     netdev@...r.kernel.org, alexandr.lobakin@...el.com
Subject: Re: [PATCH net v1] dim: initialize all struct fields

On Wed,  4 May 2022 11:58:32 -0700 Jesse Brandeburg wrote:
> The W=2 build pointed out that the code wasn't initializing all the
> variables in the dim_cq_moder declarations with the struct initializers.
> The net change here is zero since these structs were already static
> const globals and were initialized with zeros by the compiler, but
> removing compiler warnings has value in and of itself.
> 
> lib/dim/net_dim.c: At top level:
> lib/dim/net_dim.c:54:9: warning: missing initializer for field ‘comps’ of ‘const struct dim_cq_moder’ [-Wmissing-field-initializers]
>    54 |         NET_DIM_RX_EQE_PROFILES,
>       |         ^~~~~~~~~~~~~~~~~~~~~~~
> In file included from lib/dim/net_dim.c:6:
> ./include/linux/dim.h:45:13: note: ‘comps’ declared here
>    45 |         u16 comps;
>       |             ^~~~~
> 
> and repeats for the tx struct, and once you fix the comps entry then
> the cq_period_mode field needs the same treatment.
> 
> Add the necessary initializers so that the fields in the struct all have
> explicit values.
> 
> While here and fixing these lines, clean up the code slightly with
> a conversion to explicit field initializers from anonymous ones, and fix
> the super long lines by removing the word "_MODERATION" from a couple
> defines only used in this file.
> anon to explicit conversion example similar to used in this patch:
> - struct foo foo_struct = { a, b}
> + struct foo foo_struct = { .foo_a = a, .foo_b = b)
> 
> Fixes: f8be17b81d44 ("lib/dim: Fix -Wunused-const-variable warnings")
> Signed-off-by: Jesse Brandeburg <jesse.brandeburg@...el.com>
> ---
>  lib/dim/net_dim.c | 55 ++++++++++++++++++++++++++---------------------
>  1 file changed, 31 insertions(+), 24 deletions(-)
> 
> diff --git a/lib/dim/net_dim.c b/lib/dim/net_dim.c
> index 06811d866775..286b5220e360 100644
> --- a/lib/dim/net_dim.c
> +++ b/lib/dim/net_dim.c
> @@ -12,41 +12,48 @@
>   *        Each profile size must be of NET_DIM_PARAMS_NUM_PROFILES
>   */
>  #define NET_DIM_PARAMS_NUM_PROFILES 5
> -#define NET_DIM_DEFAULT_RX_CQ_MODERATION_PKTS_FROM_EQE 256
> -#define NET_DIM_DEFAULT_TX_CQ_MODERATION_PKTS_FROM_EQE 128
> +#define NET_DIM_DEFAULT_RX_CQ_PKTS_FROM_EQE 256
> +#define NET_DIM_DEFAULT_TX_CQ_PKTS_FROM_EQE 128
>  #define NET_DIM_DEF_PROFILE_CQE 1
>  #define NET_DIM_DEF_PROFILE_EQE 1
>  
> +#define DIM_CQ_MODER(u, p, c, m) { \
> +	.usec = (u),		   \
> +	.pkts = (p),		   \
> +	.comps = (c),		   \
> +	.cq_period_mode = (m)	   \
> +}
> +
>  #define NET_DIM_RX_EQE_PROFILES { \
> -	{1,   NET_DIM_DEFAULT_RX_CQ_MODERATION_PKTS_FROM_EQE}, \
> -	{8,   NET_DIM_DEFAULT_RX_CQ_MODERATION_PKTS_FROM_EQE}, \
> -	{64,  NET_DIM_DEFAULT_RX_CQ_MODERATION_PKTS_FROM_EQE}, \
> -	{128, NET_DIM_DEFAULT_RX_CQ_MODERATION_PKTS_FROM_EQE}, \
> -	{256, NET_DIM_DEFAULT_RX_CQ_MODERATION_PKTS_FROM_EQE}, \
> +	DIM_CQ_MODER(1,   NET_DIM_DEFAULT_RX_CQ_PKTS_FROM_EQE, 0, 0), \
> +	DIM_CQ_MODER(8,   NET_DIM_DEFAULT_RX_CQ_PKTS_FROM_EQE, 0, 0), \
> +	DIM_CQ_MODER(64,  NET_DIM_DEFAULT_RX_CQ_PKTS_FROM_EQE, 0, 0), \
> +	DIM_CQ_MODER(128, NET_DIM_DEFAULT_RX_CQ_PKTS_FROM_EQE, 0, 0), \
> +	DIM_CQ_MODER(256, NET_DIM_DEFAULT_RX_CQ_PKTS_FROM_EQE, 0, 0)  \
>  }

That may give people the false impression that we always have 
to initialize all the fields to appease W=2. The most common
way of fixing this warning is to tell the compiler that we know
what we're doing and add a comma after the last member:

-	{2,  256},             \
+	{2,  256,},             \

The commit message needs to at least discuss why this direction 
was not taken. My preference would actually be to do it, tho.

Also please CC maintainers and authors of patches under Fixes:.