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 <[email protected]>
---
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;
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
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 <[email protected]>
> ---
> 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;
> 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;
>
> /**
>
On Wed, Jul 22, 2020 at 01:27:10PM -0500, Gustavo A. R. Silva wrote:
> Hi all,
>
> Friendly ping: who can take this? :)
I will, give me a chance...
Hey Greg,
On 7/22/20 13:49, Greg Kroah-Hartman wrote:
> On Wed, Jul 22, 2020 at 01:27:10PM -0500, Gustavo A. R. Silva wrote:
>> Hi all,
>>
>> Friendly ping: who can take this? :)
>
> I will, give me a chance...
>
Oops, sorry... I just wanted to make sure this makes it to 5.9-rc1. :)
Thanks!
--
Gustavo
>
> 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 <[email protected]>
> > ---
> > 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.
> > 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.
> > @@ -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;
> >
> > /**
> >
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 <[email protected]>
>>> ---
>>> 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.
>
>>> 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.
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;
>>>
>>> /**
>>>
>
> 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 <[email protected]>
> >>> ---
> >>> 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;
> >>>
> >>> /**
> >>>
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 <[email protected]>
>>>>> ---
>>>>> 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;
>>>>>
>>>>> /**
>>>>>