There is a regular need in the kernel to provide a way to declare
having a dynamically sized set of trailing elements in a structure.
Kernel code should always use “flexible array members”[1] for these
cases or, as in this particular case, replace the one-element array
with a simple value type u8 reserved once this is just a placeholder
for alignment. The older style of one-element or zero-length arrays
should no longer be used[2].
Also, while there, use the preferred form for passing a size of a struct.
The alternative form where struct name is spelled out hurts readability
and introduces an opportunity for a bug when the variable type is changed
but the corresponding sizeof that is passed as argument is not.
[1] https://en.wikipedia.org/wiki/Flexible_array_member
[2] https://github.com/KSPP/linux/issues/79
Signed-off-by: Gustavo A. R. Silva <[email protected]>
---
drivers/misc/mei/hbm.c | 4 ++--
drivers/misc/mei/hw.h | 6 +++---
2 files changed, 5 insertions(+), 5 deletions(-)
diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c
index a44094cdbc36..f020d5594154 100644
--- a/drivers/misc/mei/hbm.c
+++ b/drivers/misc/mei/hbm.c
@@ -408,14 +408,14 @@ static int mei_hbm_add_cl_resp(struct mei_device *dev, u8 addr, u8 status)
{
struct mei_msg_hdr mei_hdr;
struct hbm_add_client_response resp;
- const size_t len = sizeof(struct hbm_add_client_response);
+ const size_t len = sizeof(resp);
int ret;
dev_dbg(dev->dev, "adding client response\n");
mei_hbm_hdr(&mei_hdr, len);
- memset(&resp, 0, sizeof(struct hbm_add_client_response));
+ memset(&resp, 0, len);
resp.hbm_cmd = MEI_HBM_ADD_CLIENT_RES_CMD;
resp.me_addr = addr;
resp.status = status;
diff --git a/drivers/misc/mei/hw.h b/drivers/misc/mei/hw.h
index b1a8d5ec88b3..8c0297f0e7f3 100644
--- a/drivers/misc/mei/hw.h
+++ b/drivers/misc/mei/hw.h
@@ -346,13 +346,13 @@ struct hbm_add_client_request {
* @hbm_cmd: bus message command header
* @me_addr: address of the client in ME
* @status: if HBMS_SUCCESS then the client can now accept connections.
- * @reserved: reserved
+ * @reserved: reserved for alignment.
*/
struct hbm_add_client_response {
u8 hbm_cmd;
u8 me_addr;
u8 status;
- u8 reserved[1];
+ u8 reserved;
} __packed;
/**
@@ -461,7 +461,7 @@ struct hbm_notification {
u8 hbm_cmd;
u8 me_addr;
u8 host_addr;
- u8 reserved[1];
+ u8 reserved;
} __packed;
/**
--
2.27.0
>
> There is a regular need in the kernel to provide a way to declare having a
> dynamically sized set of trailing elements in a structure.
> Kernel code should always use “flexible array members”[1] for these cases
> or, as in this particular case, replace the one-element array with a simple
> value type u8 reserved once this is just a placeholder for alignment. The older
> style of one-element or zero-length arrays should no longer be used[2].
>
> Also, while there, use the preferred form for passing a size of a struct.
> The alternative form where struct name is spelled out hurts readability and
> introduces an opportunity for a bug when the variable type is changed but
> the corresponding sizeof that is passed as argument is not.
>
> [1] https://en.wikipedia.org/wiki/Flexible_array_member
> [2] https://github.com/KSPP/linux/issues/79
>
> Signed-off-by: Gustavo A. R. Silva <[email protected]>
I'm okay with the patch but in this case the description is a bit off.
In this case there was no intention for a flexible arrays its just a reserved field.
> ---
> drivers/misc/mei/hbm.c | 4 ++--
> drivers/misc/mei/hw.h | 6 +++---
> 2 files changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c index
> a44094cdbc36..f020d5594154 100644
> --- a/drivers/misc/mei/hbm.c
> +++ b/drivers/misc/mei/hbm.c
> @@ -408,14 +408,14 @@ static int mei_hbm_add_cl_resp(struct mei_device
> *dev, u8 addr, u8 status) {
> struct mei_msg_hdr mei_hdr;
> struct hbm_add_client_response resp;
> - const size_t len = sizeof(struct hbm_add_client_response);
> + const size_t len = sizeof(resp);
> int ret;
>
> dev_dbg(dev->dev, "adding client response\n");
>
> mei_hbm_hdr(&mei_hdr, len);
>
> - memset(&resp, 0, sizeof(struct hbm_add_client_response));
> + memset(&resp, 0, len);
> resp.hbm_cmd = MEI_HBM_ADD_CLIENT_RES_CMD;
> resp.me_addr = addr;
> resp.status = status;
> diff --git a/drivers/misc/mei/hw.h b/drivers/misc/mei/hw.h index
> b1a8d5ec88b3..8c0297f0e7f3 100644
> --- a/drivers/misc/mei/hw.h
> +++ b/drivers/misc/mei/hw.h
> @@ -346,13 +346,13 @@ struct hbm_add_client_request {
> * @hbm_cmd: bus message command header
> * @me_addr: address of the client in ME
> * @status: if HBMS_SUCCESS then the client can now accept connections.
> - * @reserved: reserved
> + * @reserved: reserved for alignment.
> */
> struct hbm_add_client_response {
> u8 hbm_cmd;
> u8 me_addr;
> u8 status;
> - u8 reserved[1];
> + u8 reserved;
> } __packed;
>
> /**
> @@ -461,7 +461,7 @@ struct hbm_notification {
> u8 hbm_cmd;
> u8 me_addr;
> u8 host_addr;
> - u8 reserved[1];
> + u8 reserved;
> } __packed;
>
> /**
> --
> 2.27.0
On Tue, Jul 14, 2020 at 05:54:32PM +0000, Winkler, Tomas wrote:
> >
> > There is a regular need in the kernel to provide a way to declare having a
> > dynamically sized set of trailing elements in a structure.
> > Kernel code should always use “flexible array members”[1] for these cases
> > or, as in this particular case, replace the one-element array with a simple
> > value type u8 reserved once this is just a placeholder for alignment. The older
> > style of one-element or zero-length arrays should no longer be used[2].
> >
> > Also, while there, use the preferred form for passing a size of a struct.
> > The alternative form where struct name is spelled out hurts readability and
> > introduces an opportunity for a bug when the variable type is changed but
> > the corresponding sizeof that is passed as argument is not.
> >
> > [1] https://en.wikipedia.org/wiki/Flexible_array_member
> > [2] https://github.com/KSPP/linux/issues/79
> >
> > Signed-off-by: Gustavo A. R. Silva <[email protected]>
> I'm okay with the patch but in this case the description is a bit off.
> In this case there was no intention for a flexible arrays its just a reserved field.
>
The reserved field is actually mentioned in the description:
"... or, as in this particular case, replace the one-element array with a simple
value type u8 reserved once this is just a placeholder for alignment."
Thanks
--
Gustavo
> > ---
> > drivers/misc/mei/hbm.c | 4 ++--
> > drivers/misc/mei/hw.h | 6 +++---
> > 2 files changed, 5 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c index
> > a44094cdbc36..f020d5594154 100644
> > --- a/drivers/misc/mei/hbm.c
> > +++ b/drivers/misc/mei/hbm.c
> > @@ -408,14 +408,14 @@ static int mei_hbm_add_cl_resp(struct mei_device
> > *dev, u8 addr, u8 status) {
> > struct mei_msg_hdr mei_hdr;
> > struct hbm_add_client_response resp;
> > - const size_t len = sizeof(struct hbm_add_client_response);
> > + const size_t len = sizeof(resp);
> > int ret;
> >
> > dev_dbg(dev->dev, "adding client response\n");
> >
> > mei_hbm_hdr(&mei_hdr, len);
> >
> > - memset(&resp, 0, sizeof(struct hbm_add_client_response));
> > + memset(&resp, 0, len);
> > resp.hbm_cmd = MEI_HBM_ADD_CLIENT_RES_CMD;
> > resp.me_addr = addr;
> > resp.status = status;
> > diff --git a/drivers/misc/mei/hw.h b/drivers/misc/mei/hw.h index
> > b1a8d5ec88b3..8c0297f0e7f3 100644
> > --- a/drivers/misc/mei/hw.h
> > +++ b/drivers/misc/mei/hw.h
> > @@ -346,13 +346,13 @@ struct hbm_add_client_request {
> > * @hbm_cmd: bus message command header
> > * @me_addr: address of the client in ME
> > * @status: if HBMS_SUCCESS then the client can now accept connections.
> > - * @reserved: reserved
> > + * @reserved: reserved for alignment.
> > */
> > struct hbm_add_client_response {
> > u8 hbm_cmd;
> > u8 me_addr;
> > u8 status;
> > - u8 reserved[1];
> > + u8 reserved;
> > } __packed;
> >
> > /**
> > @@ -461,7 +461,7 @@ struct hbm_notification {
> > u8 hbm_cmd;
> > u8 me_addr;
> > u8 host_addr;
> > - u8 reserved[1];
> > + u8 reserved;
> > } __packed;
> >
> > /**
> > --
> > 2.27.0
>
>
> On Tue, Jul 14, 2020 at 05:54:32PM +0000, Winkler, Tomas wrote:
> > >
> > > There is a regular need in the kernel to provide a way to declare
> > > having a dynamically sized set of trailing elements in a structure.
> > > Kernel code should always use “flexible array members”[1] for these
> > > cases or, as in this particular case, replace the one-element array
> > > with a simple value type u8 reserved once this is just a placeholder
> > > for alignment. The older style of one-element or zero-length arrays
> should no longer be used[2].
> > >
> > > Also, while there, use the preferred form for passing a size of a struct.
> > > The alternative form where struct name is spelled out hurts
> > > readability and introduces an opportunity for a bug when the
> > > variable type is changed but the corresponding sizeof that is passed as
> argument is not.
> > >
> > > [1] https://en.wikipedia.org/wiki/Flexible_array_member
> > > [2] https://github.com/KSPP/linux/issues/79
> > >
> > > Signed-off-by: Gustavo A. R. Silva <[email protected]>
> > I'm okay with the patch but in this case the description is a bit off.
> > In this case there was no intention for a flexible arrays its just a reserved
> field.
> >
>
> The reserved field is actually mentioned in the description:
>
> "... or, as in this particular case, replace the one-element array with a simple
> value type u8 reserved once this is just a placeholder for alignment."
Right, but it looks not connected to overall context, it looks like not very clean reuse of a commit message.
I would say that this reserved[1] rather had confused the detection scripts you are using for the cleanup you are doing.
Again, I'm okay with the patch, but if you can reword the commit message it would be even more okay.
>
> Thanks
> --
> Gustavo
>
> > > ---
> > > drivers/misc/mei/hbm.c | 4 ++--
> > > drivers/misc/mei/hw.h | 6 +++---
> > > 2 files changed, 5 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/misc/mei/hbm.c b/drivers/misc/mei/hbm.c index
> > > a44094cdbc36..f020d5594154 100644
> > > --- a/drivers/misc/mei/hbm.c
> > > +++ b/drivers/misc/mei/hbm.c
> > > @@ -408,14 +408,14 @@ static int mei_hbm_add_cl_resp(struct
> > > mei_device *dev, u8 addr, u8 status) {
> > > struct mei_msg_hdr mei_hdr;
> > > struct hbm_add_client_response resp;
> > > - const size_t len = sizeof(struct hbm_add_client_response);
> > > + const size_t len = sizeof(resp);
> > > int ret;
> > >
> > > dev_dbg(dev->dev, "adding client response\n");
> > >
> > > mei_hbm_hdr(&mei_hdr, len);
> > >
> > > - memset(&resp, 0, sizeof(struct hbm_add_client_response));
> > > + memset(&resp, 0, len);
> > > resp.hbm_cmd = MEI_HBM_ADD_CLIENT_RES_CMD;
> > > resp.me_addr = addr;
> > > resp.status = status;
> > > diff --git a/drivers/misc/mei/hw.h b/drivers/misc/mei/hw.h index
> > > b1a8d5ec88b3..8c0297f0e7f3 100644
> > > --- a/drivers/misc/mei/hw.h
> > > +++ b/drivers/misc/mei/hw.h
> > > @@ -346,13 +346,13 @@ struct hbm_add_client_request {
> > > * @hbm_cmd: bus message command header
> > > * @me_addr: address of the client in ME
> > > * @status: if HBMS_SUCCESS then the client can now accept
> connections.
> > > - * @reserved: reserved
> > > + * @reserved: reserved for alignment.
> > > */
> > > struct hbm_add_client_response {
> > > u8 hbm_cmd;
> > > u8 me_addr;
> > > u8 status;
> > > - u8 reserved[1];
> > > + u8 reserved;
> > > } __packed;
> > >
> > > /**
> > > @@ -461,7 +461,7 @@ struct hbm_notification {
> > > u8 hbm_cmd;
> > > u8 me_addr;
> > > u8 host_addr;
> > > - u8 reserved[1];
> > > + u8 reserved;
> > > } __packed;
> > >
> > > /**
> > > --
> > > 2.27.0
> >
On Tue, Jul 14, 2020 at 07:57:51PM +0000, Winkler, Tomas wrote:
> >
> > On Tue, Jul 14, 2020 at 05:54:32PM +0000, Winkler, Tomas wrote:
> > > >
> > > > There is a regular need in the kernel to provide a way to declare
> > > > having a dynamically sized set of trailing elements in a structure.
> > > > Kernel code should always use “flexible array members”[1] for these
> > > > cases or, as in this particular case, replace the one-element array
> > > > with a simple value type u8 reserved once this is just a placeholder
> > > > for alignment. The older style of one-element or zero-length arrays
> > should no longer be used[2].
> > > >
> > > > Also, while there, use the preferred form for passing a size of a struct.
> > > > The alternative form where struct name is spelled out hurts
> > > > readability and introduces an opportunity for a bug when the
> > > > variable type is changed but the corresponding sizeof that is passed as
> > argument is not.
> > > >
> > > > [1] https://en.wikipedia.org/wiki/Flexible_array_member
> > > > [2] https://github.com/KSPP/linux/issues/79
> > > >
> > > > Signed-off-by: Gustavo A. R. Silva <[email protected]>
> > > I'm okay with the patch but in this case the description is a bit off.
> > > In this case there was no intention for a flexible arrays its just a reserved
> > field.
> > >
> >
> > The reserved field is actually mentioned in the description:
> >
> > "... or, as in this particular case, replace the one-element array with a simple
> > value type u8 reserved once this is just a placeholder for alignment."
>
> Right, but it looks not connected to overall context, it looks like not very clean reuse of a commit message.
> I would say that this reserved[1] rather had confused the detection scripts you are using for the cleanup you are doing.
> Again, I'm okay with the patch, but if you can reword the commit message it would be even more okay.
>
Yep; I've come up with a more concise text for these sorts of cases:
"One-element arrays are being deprecated[1]. Replace the one-element
array with a simple value type u8 reserved, once this is just a
placeholder for alignment.
[1] https://github.com/KSPP/linux/issues/79"
I'll send v2 with the text above, shortly.
Thanks for the feedback.
--
Gustavo