| Message ID | ZgHdZ15cQ7MIHsGL@neat (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [next] RDMA/cm: Avoid -Wflex-array-member-not-at-end warning | expand |
On Mon, Mar 25, 2024 at 02:24:07PM -0600, Gustavo A. R. Silva wrote: > -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting > ready to enable it globally. > > Use the `struct_group_tagged()` helper to separate the flexible array > from the rest of the members in flexible `struct cm_work`, and avoid > embedding the flexible-array member in `struct cm_timewait_info`. > > Also, use `container_of()` to retrieve a pointer to the flexible > structure. > > So, with these changes, fix the following warning: > drivers/infiniband/core/cm.c:196:24: 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/infiniband/core/cm.c | 21 ++++++++++++--------- > 1 file changed, 12 insertions(+), 9 deletions(-) > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > index bf0df6ee4f78..80c87085499c 100644 > --- a/drivers/infiniband/core/cm.c > +++ b/drivers/infiniband/core/cm.c > @@ -182,18 +182,21 @@ struct cm_av { > }; > > struct cm_work { > - struct delayed_work work; > - struct list_head list; > - struct cm_port *port; > - struct ib_mad_recv_wc *mad_recv_wc; /* Received MADs */ > - __be32 local_id; /* Established / timewait */ > - __be32 remote_id; > - struct ib_cm_event cm_event; > + /* New members must be added within the struct_group() macro below. */ > + struct_group_tagged(cm_work_hdr, hdr, > + struct delayed_work work; > + struct list_head list; > + struct cm_port *port; > + struct ib_mad_recv_wc *mad_recv_wc; /* Received MADs */ > + __be32 local_id; /* Established / timewait */ > + __be32 remote_id; > + struct ib_cm_event cm_event; > + ); > struct sa_path_rec path[]; > }; I didn't look, but does it make more sense to break out the path side into its own type and avoid the struct_group_tagged? I seem to remember only one thing used it. Jason
On 3/25/24 16:47, Jason Gunthorpe wrote: > On Mon, Mar 25, 2024 at 02:24:07PM -0600, Gustavo A. R. Silva wrote: >> -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting >> ready to enable it globally. >> >> Use the `struct_group_tagged()` helper to separate the flexible array >> from the rest of the members in flexible `struct cm_work`, and avoid >> embedding the flexible-array member in `struct cm_timewait_info`. >> >> Also, use `container_of()` to retrieve a pointer to the flexible >> structure. >> >> So, with these changes, fix the following warning: >> drivers/infiniband/core/cm.c:196:24: 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/infiniband/core/cm.c | 21 ++++++++++++--------- >> 1 file changed, 12 insertions(+), 9 deletions(-) >> >> diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c >> index bf0df6ee4f78..80c87085499c 100644 >> --- a/drivers/infiniband/core/cm.c >> +++ b/drivers/infiniband/core/cm.c >> @@ -182,18 +182,21 @@ struct cm_av { >> }; >> >> struct cm_work { >> - struct delayed_work work; >> - struct list_head list; >> - struct cm_port *port; >> - struct ib_mad_recv_wc *mad_recv_wc; /* Received MADs */ >> - __be32 local_id; /* Established / timewait */ >> - __be32 remote_id; >> - struct ib_cm_event cm_event; >> + /* New members must be added within the struct_group() macro below. */ >> + struct_group_tagged(cm_work_hdr, hdr, >> + struct delayed_work work; >> + struct list_head list; >> + struct cm_port *port; >> + struct ib_mad_recv_wc *mad_recv_wc; /* Received MADs */ >> + __be32 local_id; /* Established / timewait */ >> + __be32 remote_id; >> + struct ib_cm_event cm_event; >> + ); >> struct sa_path_rec path[]; >> }; > > I didn't look, but does it make more sense to break out the path side > into its own type and avoid the struct_group_tagged? I seem to > remember only one thing used it. > I thought about that, but I'd have to change the parameter type of `static int cm_timewait_handler(struct cm_work *work)`, and that would imply also modifying the internals of function `cm_work_handler()` (and then I didn't look much into it). So, the `struct_group_tagged()` strategy is in general more cleaner and straightforward. -- Gustavo
On Mon, Mar 25, 2024 at 08:57:08PM -0600, Gustavo A. R. Silva wrote: > > > On 3/25/24 16:47, Jason Gunthorpe wrote: > > On Mon, Mar 25, 2024 at 02:24:07PM -0600, Gustavo A. R. Silva wrote: > > > -Wflex-array-member-not-at-end is coming in GCC-14, and we are getting > > > ready to enable it globally. > > > > > > Use the `struct_group_tagged()` helper to separate the flexible array > > > from the rest of the members in flexible `struct cm_work`, and avoid > > > embedding the flexible-array member in `struct cm_timewait_info`. > > > > > > Also, use `container_of()` to retrieve a pointer to the flexible > > > structure. > > > > > > So, with these changes, fix the following warning: > > > drivers/infiniband/core/cm.c:196:24: 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/infiniband/core/cm.c | 21 ++++++++++++--------- > > > 1 file changed, 12 insertions(+), 9 deletions(-) > > > > > > diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c > > > index bf0df6ee4f78..80c87085499c 100644 > > > --- a/drivers/infiniband/core/cm.c > > > +++ b/drivers/infiniband/core/cm.c > > > @@ -182,18 +182,21 @@ struct cm_av { > > > }; > > > struct cm_work { > > > - struct delayed_work work; > > > - struct list_head list; > > > - struct cm_port *port; > > > - struct ib_mad_recv_wc *mad_recv_wc; /* Received MADs */ > > > - __be32 local_id; /* Established / timewait */ > > > - __be32 remote_id; > > > - struct ib_cm_event cm_event; > > > + /* New members must be added within the struct_group() macro below. */ > > > + struct_group_tagged(cm_work_hdr, hdr, > > > + struct delayed_work work; > > > + struct list_head list; > > > + struct cm_port *port; > > > + struct ib_mad_recv_wc *mad_recv_wc; /* Received MADs */ > > > + __be32 local_id; /* Established / timewait */ > > > + __be32 remote_id; > > > + struct ib_cm_event cm_event; > > > + ); > > > struct sa_path_rec path[]; > > > }; > > > > I didn't look, but does it make more sense to break out the path side > > into its own type and avoid the struct_group_tagged? I seem to > > remember only one thing used it. > > > > I thought about that, but I'd have to change the parameter type of > `static int cm_timewait_handler(struct cm_work *work)`, and that would > imply also modifying the internals of function `cm_work_handler()` (and > then I didn't look much into it). So let's try to invest in this direction first before we add obfuscation with magic words to the code. Thanks > So, the `struct_group_tagged()` strategy is in general more cleaner and straightforward. > > -- > Gustavo > >
diff --git a/drivers/infiniband/core/cm.c b/drivers/infiniband/core/cm.c index bf0df6ee4f78..80c87085499c 100644 --- a/drivers/infiniband/core/cm.c +++ b/drivers/infiniband/core/cm.c @@ -182,18 +182,21 @@ struct cm_av { }; struct cm_work { - struct delayed_work work; - struct list_head list; - struct cm_port *port; - struct ib_mad_recv_wc *mad_recv_wc; /* Received MADs */ - __be32 local_id; /* Established / timewait */ - __be32 remote_id; - struct ib_cm_event cm_event; + /* New members must be added within the struct_group() macro below. */ + struct_group_tagged(cm_work_hdr, hdr, + struct delayed_work work; + struct list_head list; + struct cm_port *port; + struct ib_mad_recv_wc *mad_recv_wc; /* Received MADs */ + __be32 local_id; /* Established / timewait */ + __be32 remote_id; + struct ib_cm_event cm_event; + ); struct sa_path_rec path[]; }; struct cm_timewait_info { - struct cm_work work; + struct cm_work_hdr work; struct list_head list; struct rb_node remote_qp_node; struct rb_node remote_id_node; @@ -3440,7 +3443,7 @@ static int cm_timewait_handler(struct cm_work *work) struct cm_timewait_info *timewait_info; struct cm_id_private *cm_id_priv; - timewait_info = container_of(work, struct cm_timewait_info, work); + timewait_info = container_of(&work->hdr, struct cm_timewait_info, work); spin_lock_irq(&cm.lock); list_del(&timewait_info->list); spin_unlock_irq(&cm.lock);
-Wflex-array-member-not-at-end is coming in GCC-14, and we are getting ready to enable it globally. Use the `struct_group_tagged()` helper to separate the flexible array from the rest of the members in flexible `struct cm_work`, and avoid embedding the flexible-array member in `struct cm_timewait_info`. Also, use `container_of()` to retrieve a pointer to the flexible structure. So, with these changes, fix the following warning: drivers/infiniband/core/cm.c:196:24: 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/infiniband/core/cm.c | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-)