> > Hi Tomas, > > Please, see my comments below... > > On 7/22/20 14:04, Winkler, Tomas wrote: > > > >> > >> Hi all, > >> > >> Friendly ping: who can take this? :) > >> > >> Thanks > >> -- > >> Gustavo > >> > >> On 7/14/20 16:45, Gustavo A. R. Silva wrote: > >>> One-element arrays are being deprecated[1]. Replace the one-element > >>> arrays with a simple value type u8 reserved, once this is just a > >>> placeholder for alignment. > >>> > >>> 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://github.com/KSPP/linux/issues/79 > >>> > >>> Signed-off-by: Gustavo A. R. Silva <gustavo...@kernel.org> > >>> --- > >>> Changes in v2: > >>> - Use a more concise changelog text. > >>> > >>> 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; > > > > This should be probably in a different patch it's not related to the second > part.
Frankly I will post other version of this that cleans the whole file. > > > >>> 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 > > I have second thoughts of this part as all reserved fields in this > > file are of form u8 reserved[X], so we will lose that uniformity with > > this change, you have to look at the file as whole not just at the patch. > > So I > prefer we drop that part of the patch. > > > > This is actually the main point of this patch: the removal of one-element > arrays. > And yeah, every place in the kernel that uses the form that you mention will > see it's uniformity slightly modified, and that's for a good cause: the > removal > of one-element arrays, so we can enable bounds checking. I was going over https://github.com/KSPP/linux/issues/79, I'm not sure this all related to flexible arrays, those are just reserved struct members. So because it's hard to identify a legitimate usage of single element arrays we are going to kill them all? It's more esthetic / readability issue here but there might be some legit use case for one element array, no? > > Thanks > -- > Gustavo > > >>> @@ -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; > >>> > >>> /** > >>>
