diff mbox series

[next] nfp: Avoid -Wflex-array-member-not-at-end warnings

Message ID ZgYWlkxdrrieDYIu@neat (mailing list archive)
State Accepted
Commit d88cabfd9abcd01c7729e5383919357da732ada9
Delegated to: Netdev Maintainers
Headers show
Series [next] nfp: Avoid -Wflex-array-member-not-at-end warnings | expand

Checks

Context Check Description
netdev/tree_selection success Not a local patch

Commit Message

Gustavo A. R. Silva March 29, 2024, 1:17 a.m. UTC 
-Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
ready to enable it globally.

There is currently an object (`tl`), at the beginning of multiple
structures, that contains a flexible structure (`struct nfp_dump_tl`),
for example:

struct nfp_dumpspec_csr {
        struct nfp_dump_tl tl;

        ...

        __be32 register_width;  /* in bits */
};

So, in order to avoid ending up with flexible-array members in the
middle of multiple other structs, we use the `struct_group_tagged()`
helper to separate the flexible array from the rest of the members
in the flexible structure:

struct nfp_dump_tl {
	struct_group_tagged(nfp_dump_tl_hdr, hdr,

	... the rest of members

	);
        char data[];
};

With the change described above, we now declare objects of the type of
the tagged struct, in this case `struct nfp_dump_tl_hdr`, without
embedding flexible arrays in the middle of another struct:

struct nfp_dumpspec_csr {
        struct nfp_dump_tl_hdr tl;

	...

        __be32 register_width;  /* in bits */
};

Also, use `container_of()` whenever we need to retrieve a pointer to
the flexible structure, through which we can access the flexible
array if needed.

So, with these changes, fix 33 of the following warnings:
drivers/net/ethernet/netronome/nfp/nfp_net_debugdump.c:58:28: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
drivers/net/ethernet/netronome/nfp/nfp_net_debugdump.c:64:28: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
drivers/net/ethernet/netronome/nfp/nfp_net_debugdump.c:70:28: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
drivers/net/ethernet/netronome/nfp/nfp_net_debugdump.c:78:28: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
drivers/net/ethernet/netronome/nfp/nfp_net_debugdump.c:87:28: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]
drivers/net/ethernet/netronome/nfp/nfp_net_debugdump.c:92:28: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end]

Link: https://github.com/KSPP/linux/issues/202
Signed-off-by: Gustavo A. R. Silva <gustavoars@kernel.org>
---
 .../netronome/nfp/nfp_net_debugdump.c         | 41 +++++++++++--------
 1 file changed, 25 insertions(+), 16 deletions(-)

Comments

Jakub Kicinski April 2, 2024, 4:24 a.m. UTC | #1 
On Thu, 28 Mar 2024 19:17:10 -0600 Gustavo A. R. Silva wrote:
> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_debugdump.c
> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_debugdump.c
> @@ -34,8 +34,11 @@ enum nfp_dumpspec_type {
>  
>  /* generic type plus length */
>  struct nfp_dump_tl {
> -	__be32 type;
> -	__be32 length;	/* chunk length to follow, aligned to 8 bytes */
> +	/* New members must be added within the struct_group() macro below. */
> +	struct_group_tagged(nfp_dump_tl_hdr, hdr,
> +		__be32 type;
> +		__be32 length;	/* chunk length to follow, aligned to 8 bytes */
> +	);
>  	char data[];
>  };

I counted 9 references to nfp_dump_tl->data.
Better to add:

static void *nfp_dump_tl_data(struct nfp_dump_tl *spec)
{
	return &spec[1];
}

and delete the flex array completely.
Gustavo A. R. Silva April 3, 2024, 12:49 a.m. UTC | #2 
On 01/04/24 22:24, Jakub Kicinski wrote:
> On Thu, 28 Mar 2024 19:17:10 -0600 Gustavo A. R. Silva wrote:
>> --- a/drivers/net/ethernet/netronome/nfp/nfp_net_debugdump.c
>> +++ b/drivers/net/ethernet/netronome/nfp/nfp_net_debugdump.c
>> @@ -34,8 +34,11 @@ enum nfp_dumpspec_type {
>>   
>>   /* generic type plus length */
>>   struct nfp_dump_tl {
>> -	__be32 type;
>> -	__be32 length;	/* chunk length to follow, aligned to 8 bytes */
>> +	/* New members must be added within the struct_group() macro below. */
>> +	struct_group_tagged(nfp_dump_tl_hdr, hdr,
>> +		__be32 type;
>> +		__be32 length;	/* chunk length to follow, aligned to 8 bytes */
>> +	);
>>   	char data[];
>>   };
> 
> I counted 9 references to nfp_dump_tl->data.
> Better to add:
> 
> static void *nfp_dump_tl_data(struct nfp_dump_tl *spec)
> {
> 	return &spec[1];
> }

Unfortunately, that's out-of-bounds for the compiler, and well, basically
the reason why flex-array members were created in the first place.

I was looking into implementing two separate structs:

struct nfp_dump_tl_hdr {
         __be32 type;
         __be32 length;  /* chunk length to follow, aligned to 8 bytes */
};

struct nfp_dump_tl {
         __be32 type;
         __be32 length;  /* chunk length to follow, aligned to 8 bytes */
	char data[];
};

and at least for structs nfp_dumpspec_csr, nfp_dumpspec_rtsym, nfp_dump_csr, and
nfp_dump_rtsym it'd be a clean change (no need for container_of()), but not for
structs nfp_dumpspec_csr and nfp_dumpspec_rtsym because of some casts from
the flex struct:

nfp_add_tlv_size():
         case NFP_DUMPSPEC_TYPE_ME_CSR:
                 spec_csr = (struct nfp_dumpspec_csr *)tl;
                 if (!nfp_csr_spec_valid(spec_csr))
		...

         case NFP_DUMPSPEC_TYPE_INDIRECT_ME_CSR:
                 spec_csr = (struct nfp_dumpspec_csr *)tl;
                 if (!nfp_csr_spec_valid(spec_csr))
		...

	case NFP_DUMPSPEC_TYPE_RTSYM:
                 spec_rtsym = (struct nfp_dumpspec_rtsym *)tl;
                 err = nfp_dump_single_rtsym(pf, spec_rtsym, dump);

nfp_calc_rtsym_dump_sz():
         spec_rtsym = (struct nfp_dumpspec_rtsym *)spec;


At least for those two structs, it's probably more straightforward to use
struct_group_tagged() and container_of().

--
Gustavo
patchwork-bot+netdevbpf@kernel.org April 3, 2024, 1:10 a.m. UTC | #3 
Hello:

This patch was applied to netdev/net-next.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Thu, 28 Mar 2024 19:17:10 -0600 you wrote:
> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting
> ready to enable it globally.
> 
> There is currently an object (`tl`), at the beginning of multiple
> structures, that contains a flexible structure (`struct nfp_dump_tl`),
> for example:
> 
> [...]

Here is the summary with links:
  - [next] nfp: Avoid -Wflex-array-member-not-at-end warnings
    https://git.kernel.org/netdev/net-next/c/d88cabfd9abc

You are awesome, thank you!
diff mbox series

Patch

diff --git a/drivers/net/ethernet/netronome/nfp/nfp_net_debugdump.c b/drivers/net/ethernet/netronome/nfp/nfp_net_debugdump.c
index a614df095b08..2dd37557185e 100644
--- a/drivers/net/ethernet/netronome/nfp/nfp_net_debugdump.c
+++ b/drivers/net/ethernet/netronome/nfp/nfp_net_debugdump.c
@@ -34,8 +34,11 @@  enum nfp_dumpspec_type {
 
 /* generic type plus length */
 struct nfp_dump_tl {
-	__be32 type;
-	__be32 length;	/* chunk length to follow, aligned to 8 bytes */
+	/* New members must be added within the struct_group() macro below. */
+	struct_group_tagged(nfp_dump_tl_hdr, hdr,
+		__be32 type;
+		__be32 length;	/* chunk length to follow, aligned to 8 bytes */
+	);
 	char data[];
 };
 
@@ -55,19 +58,19 @@  struct nfp_dump_common_cpp {
 
 /* CSR dumpables */
 struct nfp_dumpspec_csr {
-	struct nfp_dump_tl tl;
+	struct nfp_dump_tl_hdr tl;
 	struct nfp_dump_common_cpp cpp;
 	__be32 register_width;	/* in bits */
 };
 
 struct nfp_dumpspec_rtsym {
-	struct nfp_dump_tl tl;
+	struct nfp_dump_tl_hdr tl;
 	char rtsym[];
 };
 
 /* header for register dumpable */
 struct nfp_dump_csr {
-	struct nfp_dump_tl tl;
+	struct nfp_dump_tl_hdr tl;
 	struct nfp_dump_common_cpp cpp;
 	__be32 register_width;	/* in bits */
 	__be32 error;		/* error code encountered while reading */
@@ -75,7 +78,7 @@  struct nfp_dump_csr {
 };
 
 struct nfp_dump_rtsym {
-	struct nfp_dump_tl tl;
+	struct nfp_dump_tl_hdr tl;
 	struct nfp_dump_common_cpp cpp;
 	__be32 error;		/* error code encountered while reading */
 	u8 padded_name_length;	/* pad so data starts at 8 byte boundary */
@@ -84,12 +87,12 @@  struct nfp_dump_rtsym {
 };
 
 struct nfp_dump_prolog {
-	struct nfp_dump_tl tl;
+	struct nfp_dump_tl_hdr tl;
 	__be32 dump_level;
 };
 
 struct nfp_dump_error {
-	struct nfp_dump_tl tl;
+	struct nfp_dump_tl_hdr tl;
 	__be32 error;
 	char padding[4];
 	char spec[];
@@ -449,6 +452,8 @@  static int
 nfp_dump_csr_range(struct nfp_pf *pf, struct nfp_dumpspec_csr *spec_csr,
 		   struct nfp_dump_state *dump)
 {
+	struct nfp_dump_tl *spec_csr_tl =
+			container_of(&spec_csr->tl, struct nfp_dump_tl, hdr);
 	struct nfp_dump_csr *dump_header = dump->p;
 	u32 reg_sz, header_size, total_size;
 	u32 cpp_rd_addr, max_rd_addr;
@@ -458,7 +463,7 @@  nfp_dump_csr_range(struct nfp_pf *pf, struct nfp_dumpspec_csr *spec_csr,
 	int err;
 
 	if (!nfp_csr_spec_valid(spec_csr))
-		return nfp_dump_error_tlv(&spec_csr->tl, -EINVAL, dump);
+		return nfp_dump_error_tlv(spec_csr_tl, -EINVAL, dump);
 
 	reg_sz = be32_to_cpu(spec_csr->register_width) / BITS_PER_BYTE;
 	header_size = ALIGN8(sizeof(*dump_header));
@@ -466,7 +471,7 @@  nfp_dump_csr_range(struct nfp_pf *pf, struct nfp_dumpspec_csr *spec_csr,
 		     ALIGN8(be32_to_cpu(spec_csr->cpp.dump_length));
 	dest = dump->p + header_size;
 
-	err = nfp_add_tlv(be32_to_cpu(spec_csr->tl.type), total_size, dump);
+	err = nfp_add_tlv(be32_to_cpu(spec_csr_tl->type), total_size, dump);
 	if (err)
 		return err;
 
@@ -552,6 +557,8 @@  nfp_dump_indirect_csr_range(struct nfp_pf *pf,
 			    struct nfp_dumpspec_csr *spec_csr,
 			    struct nfp_dump_state *dump)
 {
+	struct nfp_dump_tl *spec_csr_tl =
+			container_of(&spec_csr->tl, struct nfp_dump_tl, hdr);
 	struct nfp_dump_csr *dump_header = dump->p;
 	u32 reg_sz, header_size, total_size;
 	u32 cpp_rd_addr, max_rd_addr;
@@ -560,7 +567,7 @@  nfp_dump_indirect_csr_range(struct nfp_pf *pf,
 	int err;
 
 	if (!nfp_csr_spec_valid(spec_csr))
-		return nfp_dump_error_tlv(&spec_csr->tl, -EINVAL, dump);
+		return nfp_dump_error_tlv(spec_csr_tl, -EINVAL, dump);
 
 	reg_sz = be32_to_cpu(spec_csr->register_width) / BITS_PER_BYTE;
 	header_size = ALIGN8(sizeof(*dump_header));
@@ -569,7 +576,7 @@  nfp_dump_indirect_csr_range(struct nfp_pf *pf,
 	total_size = header_size + ALIGN8(reg_data_length);
 	dest = dump->p + header_size;
 
-	err = nfp_add_tlv(be32_to_cpu(spec_csr->tl.type), total_size, dump);
+	err = nfp_add_tlv(be32_to_cpu(spec_csr_tl->type), total_size, dump);
 	if (err)
 		return err;
 
@@ -597,6 +604,8 @@  static int
 nfp_dump_single_rtsym(struct nfp_pf *pf, struct nfp_dumpspec_rtsym *spec,
 		      struct nfp_dump_state *dump)
 {
+	struct nfp_dump_tl *spec_tl =
+			container_of(&spec->tl, struct nfp_dump_tl, hdr);
 	struct nfp_dump_rtsym *dump_header = dump->p;
 	struct nfp_dumpspec_cpp_isl_id cpp_params;
 	struct nfp_rtsym_table *rtbl = pf->rtbl;
@@ -607,14 +616,14 @@  nfp_dump_single_rtsym(struct nfp_pf *pf, struct nfp_dumpspec_rtsym *spec,
 	void *dest;
 	int err;
 
-	tl_len = be32_to_cpu(spec->tl.length);
+	tl_len = be32_to_cpu(spec_tl->length);
 	key_len = strnlen(spec->rtsym, tl_len);
 	if (key_len == tl_len)
-		return nfp_dump_error_tlv(&spec->tl, -EINVAL, dump);
+		return nfp_dump_error_tlv(spec_tl, -EINVAL, dump);
 
 	sym = nfp_rtsym_lookup(rtbl, spec->rtsym);
 	if (!sym)
-		return nfp_dump_error_tlv(&spec->tl, -ENOENT, dump);
+		return nfp_dump_error_tlv(spec_tl, -ENOENT, dump);
 
 	sym_size = nfp_rtsym_size(sym);
 	header_size =
@@ -622,7 +631,7 @@  nfp_dump_single_rtsym(struct nfp_pf *pf, struct nfp_dumpspec_rtsym *spec,
 	total_size = header_size + ALIGN8(sym_size);
 	dest = dump->p + header_size;
 
-	err = nfp_add_tlv(be32_to_cpu(spec->tl.type), total_size, dump);
+	err = nfp_add_tlv(be32_to_cpu(spec_tl->type), total_size, dump);
 	if (err)
 		return err;