Messages in this thread | | | From | "Gustavo A. R. Silva" <> | Subject | Re: [PATCH v2] mei: Avoid the use of one-element arrays | Date | Wed, 22 Jul 2020 18:01:54 -0500 |
| |
On 7/22/20 17:40, Winkler, Tomas wrote: >> >> 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 <gustavoars@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. >
Sounds good. :)
>>> >>>>> 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,
I've opened a new issue for this:
https://github.com/KSPP/linux/issues/86
And I'm already including the link above in these sorts of patches; please, see:
https://lore.kernel.org/lkml/20200717215500.GA13910@embeddedor/
> 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? >
The code is continuously evolving and, if a one-element array is not intended to be used as a variable-length array at all, I frankly cannot think of any another use that is not merely esthetic/readability. :)
Thanks -- Gustavo
> >> >> 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; >>>>> >>>>> /** >>>>>
| |