| Message ID | ZgG8bbEzhmX5nGRE@neat (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | [next] rpmsg: glink: Avoid -Wflex-array-member-not-at-end warnings | expand |
On Mon, Mar 25, 2024 at 12:03:25PM -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. > > There is currently an object (`msg`) in multiple structures that > contains a flexible structure (`struct glink_msg`), for example: > > struct glink_defer_cmd { > ... > > struct glink_msg msg; > u8 data[]; > }; > > So, in order to avoid ending up with a flexible-array member in the > middle of another structure, we use the `__struct_group()` helper > to separate the flexible array from the rest of the members in the > flexible structure: > > struct glink_msg { > __struct_group(glink_msg_hdr, hdr, __packed, > > ... the rest of members > > ); > u8 data[]; > } __packed; > > With the change described above, we now declare objects of the type of > the tagged struct, in this case `struct glink_msg_hdr`, without > embedding flexible arrays in the middle of other structures: > > struct glink_defer_cmd { > ... > > struct glink_msg_hdr msg; > u8 data[]; > }; > > Also, use `container_of()` to retrieve a pointer to the flexible structure. > > We also use the `DEFINE_RAW_FLEX()` helper for an in-stack definition of > a flexible structure where the size of the flexible-array member is known > at compile-time. > > So, with these changes, fix the following warnings: > drivers/rpmsg/qcom_glink_native.c:51:26: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/rpmsg/qcom_glink_native.c:459:34: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/rpmsg/qcom_glink_native.c:846:34: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/rpmsg/qcom_glink_native.c:968:34: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] > drivers/rpmsg/qcom_glink_native.c:1380:34: 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/rpmsg/qcom_glink_native.c | 38 ++++++++++++++++--------------- > 1 file changed, 20 insertions(+), 18 deletions(-) > > diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c > index 82d460ff4777..878e3461bce1 100644 > --- a/drivers/rpmsg/qcom_glink_native.c > +++ b/drivers/rpmsg/qcom_glink_native.c > @@ -30,9 +30,12 @@ > #define RPM_GLINK_CID_MAX 65536 > > struct glink_msg { > - __le16 cmd; > - __le16 param1; > - __le32 param2; > + /* New members must be added within the __struct_group() macro below. */ > + __struct_group(glink_msg_hdr, hdr, __packed, > + __le16 cmd; > + __le16 param1; > + __le32 param2; > + ); > u8 data[]; > } __packed; > > @@ -48,7 +51,7 @@ struct glink_msg { > struct glink_defer_cmd { > struct list_head node; > > - struct glink_msg msg; > + struct glink_msg_hdr msg; > u8 data[]; > }; Instead of this change (and the container_of() uses below), I think you can just simply drop "data" here. I don't see anything using it except the struct_size()s which can all change their "data" argument to msg.data. e.g.: - dcmd = kzalloc(struct_size(dcmd, data, extra), GFP_ATOMIC); + dcmd = kzalloc(struct_size(dcmd, msg.data, extra), GFP_ATOMIC); With those changed, I think this patch becomes more readable. -Kees
>> @@ -48,7 +51,7 @@ struct glink_msg { >> struct glink_defer_cmd { >> struct list_head node; >> >> - struct glink_msg msg; >> + struct glink_msg_hdr msg; >> u8 data[]; >> }; > > Instead of this change (and the container_of() uses below), I think you > can just simply drop "data" here. I don't see anything using it except > the struct_size()s which can all change their "data" argument to > msg.data. e.g.: Whaa.. I'm sorry, I totally missed this response. I think I was traveling a lot back then. > > - dcmd = kzalloc(struct_size(dcmd, data, extra), GFP_ATOMIC); > + dcmd = kzalloc(struct_size(dcmd, msg.data, extra), GFP_ATOMIC); > > With those changed, I think this patch becomes more readable. Yes; I think I can change the code like this. :) Thanks! -- Gustavo
diff --git a/drivers/rpmsg/qcom_glink_native.c b/drivers/rpmsg/qcom_glink_native.c index 82d460ff4777..878e3461bce1 100644 --- a/drivers/rpmsg/qcom_glink_native.c +++ b/drivers/rpmsg/qcom_glink_native.c @@ -30,9 +30,12 @@ #define RPM_GLINK_CID_MAX 65536 struct glink_msg { - __le16 cmd; - __le16 param1; - __le32 param2; + /* New members must be added within the __struct_group() macro below. */ + __struct_group(glink_msg_hdr, hdr, __packed, + __le16 cmd; + __le16 param1; + __le32 param2; + ); u8 data[]; } __packed; @@ -48,7 +51,7 @@ struct glink_msg { struct glink_defer_cmd { struct list_head node; - struct glink_msg msg; + struct glink_msg_hdr msg; u8 data[]; }; @@ -455,12 +458,9 @@ static void qcom_glink_intent_req_abort(struct glink_channel *channel) static int qcom_glink_send_open_req(struct qcom_glink *glink, struct glink_channel *channel) { - struct { - struct glink_msg msg; - u8 name[GLINK_NAME_SIZE]; - } __packed req; + DEFINE_RAW_FLEX(struct glink_msg, req, data, GLINK_NAME_SIZE); int name_len = strlen(channel->name) + 1; - int req_len = ALIGN(sizeof(req.msg) + name_len, 8); + int req_len = ALIGN(sizeof(*req) + name_len, 8); int ret; unsigned long flags; @@ -476,10 +476,10 @@ static int qcom_glink_send_open_req(struct qcom_glink *glink, channel->lcid = ret; - req.msg.cmd = cpu_to_le16(GLINK_CMD_OPEN); - req.msg.param1 = cpu_to_le16(channel->lcid); - req.msg.param2 = cpu_to_le32(name_len); - strcpy(req.name, channel->name); + req->cmd = cpu_to_le16(GLINK_CMD_OPEN); + req->param1 = cpu_to_le16(channel->lcid); + req->param2 = cpu_to_le32(name_len); + strcpy(req->data, channel->name); ret = qcom_glink_tx(glink, &req, req_len, NULL, 0, true); if (ret) @@ -826,7 +826,9 @@ static int qcom_glink_rx_defer(struct qcom_glink *glink, size_t extra) INIT_LIST_HEAD(&dcmd->node); - qcom_glink_rx_peek(glink, &dcmd->msg, 0, sizeof(dcmd->msg) + extra); + qcom_glink_rx_peek(glink, + container_of(&dcmd->msg, struct glink_msg, hdr), 0, + sizeof(dcmd->msg) + extra); spin_lock(&glink->rx_lock); list_add_tail(&dcmd->node, &glink->rx_queue); @@ -843,7 +845,7 @@ static int qcom_glink_rx_data(struct qcom_glink *glink, size_t avail) struct glink_core_rx_intent *intent; struct glink_channel *channel; struct { - struct glink_msg msg; + struct glink_msg_hdr msg; __le32 chunk_size; __le32 left_size; } __packed hdr; @@ -965,7 +967,7 @@ static void qcom_glink_handle_intent(struct qcom_glink *glink, }; struct { - struct glink_msg msg; + struct glink_msg_hdr msg; struct intent_pair intents[]; } __packed * msg; @@ -1377,7 +1379,7 @@ static int __qcom_glink_send(struct glink_channel *channel, struct glink_core_rx_intent *tmp; int iid = 0; struct { - struct glink_msg msg; + struct glink_msg_hdr msg; __le32 chunk_size; __le32 left_size; } __packed req; @@ -1685,7 +1687,7 @@ static void qcom_glink_work(struct work_struct *work) list_del(&dcmd->node); spin_unlock_irqrestore(&glink->rx_lock, flags); - msg = &dcmd->msg; + msg = container_of(&dcmd->msg, struct glink_msg, hdr); cmd = le16_to_cpu(msg->cmd); param1 = le16_to_cpu(msg->param1); param2 = le32_to_cpu(msg->param2);
-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 (`msg`) in multiple structures that contains a flexible structure (`struct glink_msg`), for example: struct glink_defer_cmd { ... struct glink_msg msg; u8 data[]; }; So, in order to avoid ending up with a flexible-array member in the middle of another structure, we use the `__struct_group()` helper to separate the flexible array from the rest of the members in the flexible structure: struct glink_msg { __struct_group(glink_msg_hdr, hdr, __packed, ... the rest of members ); u8 data[]; } __packed; With the change described above, we now declare objects of the type of the tagged struct, in this case `struct glink_msg_hdr`, without embedding flexible arrays in the middle of other structures: struct glink_defer_cmd { ... struct glink_msg_hdr msg; u8 data[]; }; Also, use `container_of()` to retrieve a pointer to the flexible structure. We also use the `DEFINE_RAW_FLEX()` helper for an in-stack definition of a flexible structure where the size of the flexible-array member is known at compile-time. So, with these changes, fix the following warnings: drivers/rpmsg/qcom_glink_native.c:51:26: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/rpmsg/qcom_glink_native.c:459:34: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/rpmsg/qcom_glink_native.c:846:34: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/rpmsg/qcom_glink_native.c:968:34: warning: structure containing a flexible array member is not at the end of another structure [-Wflex-array-member-not-at-end] drivers/rpmsg/qcom_glink_native.c:1380:34: 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/rpmsg/qcom_glink_native.c | 38 ++++++++++++++++--------------- 1 file changed, 20 insertions(+), 18 deletions(-)