[PATCH] staging: greybus: replace zero-element array with flexible-array

[PATCH] staging: greybus: replace zero-element array with flexible-array

[Jaehee Park]

Zero-length and one-element arrays are deprecated. Flexible-array
members should be used instead. Flexible-array members are
recommended because this is the way the kernel expects dynamically
sized trailing elements to be declared.
Refer to Documentation/process/deprecated.rst.

Change the zero-length array, buf, in the struct 
gb_usb_hub_control_response to a flexible array. And add wLength as a 
member of the struct so that the struct is not a zero-sized struct.

Issue found by flexible_array coccinelle script.

Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
---

I have a question for the authors: 
I saw a fixme comment in the hub_control function in usb.c:
/ FIXME: handle unspecified lengths /

I was wondering why this comment was left there?

In this patch, I'm using this struct:

struct gb_usb_hub_control_response {
    __le16 wLength;
    u8 buf[];
};

And instead of using response_size, I'm doing this:

struct gb_usb_hub_control_response *response;
And using sizeof(*response) as the input to gb_operation_create.

Would the flexible array address the handling of unspecified lengths 
issue (in the fixme comment)?


 drivers/staging/greybus/usb.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/staging/greybus/usb.c b/drivers/staging/greybus/usb.c
index 8e9d9d59a357..d0b2422401df 100644
--- a/drivers/staging/greybus/usb.c
+++ b/drivers/staging/greybus/usb.c
@@ -27,7 +27,8 @@ struct gb_usb_hub_control_request {
 };
 
 struct gb_usb_hub_control_response {
-	u8 buf[0];
+	__le16 wLength;
+	u8 buf[];
 };
 
 struct gb_usb_device {
@@ -102,16 +103,14 @@ static int hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex,
 	struct gb_operation *operation;
 	struct gb_usb_hub_control_request *request;
 	struct gb_usb_hub_control_response *response;
-	size_t response_size;
 	int ret;
 
 	/* FIXME: handle unspecified lengths */
-	response_size = sizeof(*response) + wLength;
 
 	operation = gb_operation_create(dev->connection,
 					GB_USB_TYPE_HUB_CONTROL,
 					sizeof(*request),
-					response_size,
+					sizeof(*response),
 					GFP_KERNEL);
 	if (!operation)
 		return -ENOMEM;
-- 
2.25.1

Re: [PATCH] staging: greybus: replace zero-element array with flexible-array

[Greg Kroah-Hartman]

On Mon, Apr 11, 2022 at 05:14:11PM -0400, Jaehee Park wrote:
> Zero-length and one-element arrays are deprecated. Flexible-array
> members should be used instead. Flexible-array members are
> recommended because this is the way the kernel expects dynamically
> sized trailing elements to be declared.
> Refer to Documentation/process/deprecated.rst.
> 
> Change the zero-length array, buf, in the struct 
> gb_usb_hub_control_response to a flexible array. And add wLength as a 
> member of the struct so that the struct is not a zero-sized struct.
> 
> Issue found by flexible_array coccinelle script.
> 
> Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> ---
> 
> I have a question for the authors: 
> I saw a fixme comment in the hub_control function in usb.c:
> / FIXME: handle unspecified lengths /
> 
> I was wondering why this comment was left there?
> 
> In this patch, I'm using this struct:
> 
> struct gb_usb_hub_control_response {
>     __le16 wLength;
>     u8 buf[];
> };
> 
> And instead of using response_size, I'm doing this:
> 
> struct gb_usb_hub_control_response *response;
> And using sizeof(*response) as the input to gb_operation_create.
> 
> Would the flexible array address the handling of unspecified lengths 
> issue (in the fixme comment)?

No, you can not change the format of the data on the bus without also
changing the firmware in the device and usually the specification as
well.

>  drivers/staging/greybus/usb.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/staging/greybus/usb.c b/drivers/staging/greybus/usb.c
> index 8e9d9d59a357..d0b2422401df 100644
> --- a/drivers/staging/greybus/usb.c
> +++ b/drivers/staging/greybus/usb.c
> @@ -27,7 +27,8 @@ struct gb_usb_hub_control_request {
>  };
>  
>  struct gb_usb_hub_control_response {
> -	u8 buf[0];
> +	__le16 wLength;
> +	u8 buf[];

What is wrong with buf[0] here?

You can fix this in other ways if you really understand the difference
between [0] and [] in C.  Please look at many of the other conversions
if you wish to do this.

thanks,

greg k-h

Re: [PATCH] staging: greybus: replace zero-element array with flexible-array

[Greg Kroah-Hartman]

On Tue, Apr 12, 2022 at 06:56:12AM +0200, Greg Kroah-Hartman wrote:
> On Mon, Apr 11, 2022 at 05:14:11PM -0400, Jaehee Park wrote:
> > Zero-length and one-element arrays are deprecated. Flexible-array
> > members should be used instead. Flexible-array members are
> > recommended because this is the way the kernel expects dynamically
> > sized trailing elements to be declared.
> > Refer to Documentation/process/deprecated.rst.
> > 
> > Change the zero-length array, buf, in the struct 
> > gb_usb_hub_control_response to a flexible array. And add wLength as a 
> > member of the struct so that the struct is not a zero-sized struct.
> > 
> > Issue found by flexible_array coccinelle script.
> > 
> > Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> > ---
> > 
> > I have a question for the authors: 
> > I saw a fixme comment in the hub_control function in usb.c:
> > / FIXME: handle unspecified lengths /
> > 
> > I was wondering why this comment was left there?
> > 
> > In this patch, I'm using this struct:
> > 
> > struct gb_usb_hub_control_response {
> >     __le16 wLength;
> >     u8 buf[];
> > };
> > 
> > And instead of using response_size, I'm doing this:
> > 
> > struct gb_usb_hub_control_response *response;
> > And using sizeof(*response) as the input to gb_operation_create.
> > 
> > Would the flexible array address the handling of unspecified lengths 
> > issue (in the fixme comment)?
> 
> No, you can not change the format of the data on the bus without also
> changing the firmware in the device and usually the specification as
> well.
> 
> >  drivers/staging/greybus/usb.c | 7 +++----
> >  1 file changed, 3 insertions(+), 4 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/usb.c b/drivers/staging/greybus/usb.c
> > index 8e9d9d59a357..d0b2422401df 100644
> > --- a/drivers/staging/greybus/usb.c
> > +++ b/drivers/staging/greybus/usb.c
> > @@ -27,7 +27,8 @@ struct gb_usb_hub_control_request {
> >  };
> >  
> >  struct gb_usb_hub_control_response {
> > -	u8 buf[0];
> > +	__le16 wLength;
> > +	u8 buf[];
> 
> What is wrong with buf[0] here?
> 
> You can fix this in other ways if you really understand the difference
> between [0] and [] in C.  Please look at many of the other conversions
> if you wish to do this.

And I would not recommend this as an "outreachy introduction task"
unless you understand this.  There are much easier first patch tasks you
can accomplish instead.

good luck!

greg k-h

Re: [PATCH] staging: greybus: replace zero-element array with flexible-array

[Dan Carpenter]

No, this patch is not right.

On Mon, Apr 11, 2022 at 05:14:11PM -0400, Jaehee Park wrote:
> diff --git a/drivers/staging/greybus/usb.c b/drivers/staging/greybus/usb.c
> index 8e9d9d59a357..d0b2422401df 100644
> --- a/drivers/staging/greybus/usb.c
> +++ b/drivers/staging/greybus/usb.c
> @@ -27,7 +27,8 @@ struct gb_usb_hub_control_request {
>  };
>  
>  struct gb_usb_hub_control_response {
> -	u8 buf[0];
> +	__le16 wLength;
> +	u8 buf[];
>  };
>  
>  struct gb_usb_device {
> @@ -102,16 +103,14 @@ static int hub_control(struct usb_hcd *hcd, u16 typeReq, u16 wValue, u16 wIndex,
>  	struct gb_operation *operation;
>  	struct gb_usb_hub_control_request *request;
>  	struct gb_usb_hub_control_response *response;
> -	size_t response_size;
>  	int ret;
>  
>  	/* FIXME: handle unspecified lengths */
> -	response_size = sizeof(*response) + wLength;

You're mixing up the value of wLength with the size of wLength (2).

>  
>  	operation = gb_operation_create(dev->connection,
>  					GB_USB_TYPE_HUB_CONTROL,
>  					sizeof(*request),
> -					response_size,
> +					sizeof(*response),

In the original code response_size was equal to wLength.  But now you're
passing 2.

So, I mean the no brainer approach would be to just say:

-					response_size,
+					wLength,

And delete the gb_usb_hub_control_response completely along with the
reference to it.

But better to do a brainer approach and investigate how that response
buffer is used.  It's probably all fine.  So probably the no brainer
approach is the correct approach.  It makes the code look nicer, it
doesn't break anything and we will merge it.  But better to at least
look carefully at it first.

regards,
dan carpenter

Re: [PATCH] staging: greybus: replace zero-element array with flexible-array

[Jaehee Park]

On Tue, Apr 12, 2022 at 06:57:11AM +0200, Greg Kroah-Hartman wrote:
> On Tue, Apr 12, 2022 at 06:56:12AM +0200, Greg Kroah-Hartman wrote:
> > On Mon, Apr 11, 2022 at 05:14:11PM -0400, Jaehee Park wrote:
> > > Zero-length and one-element arrays are deprecated. Flexible-array
> > > members should be used instead. Flexible-array members are
> > > recommended because this is the way the kernel expects dynamically
> > > sized trailing elements to be declared.
> > > Refer to Documentation/process/deprecated.rst.
> > > 
> > > Change the zero-length array, buf, in the struct 
> > > gb_usb_hub_control_response to a flexible array. And add wLength as a 
> > > member of the struct so that the struct is not a zero-sized struct.
> > > 
> > > Issue found by flexible_array coccinelle script.
> > > 
> > > Signed-off-by: Jaehee Park <jhpark1013@gmail.com>
> > > ---
> > > 
> > > I have a question for the authors: 
> > > I saw a fixme comment in the hub_control function in usb.c:
> > > / FIXME: handle unspecified lengths /
> > > 
> > > I was wondering why this comment was left there?
> > > 
> > > In this patch, I'm using this struct:
> > > 
> > > struct gb_usb_hub_control_response {
> > >     __le16 wLength;
> > >     u8 buf[];
> > > };
> > > 
> > > And instead of using response_size, I'm doing this:
> > > 
> > > struct gb_usb_hub_control_response *response;
> > > And using sizeof(*response) as the input to gb_operation_create.
> > > 
> > > Would the flexible array address the handling of unspecified lengths 
> > > issue (in the fixme comment)?
> > 
> > No, you can not change the format of the data on the bus without also
> > changing the firmware in the device and usually the specification as
> > well.
> > 
> > >  drivers/staging/greybus/usb.c | 7 +++----
> > >  1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/staging/greybus/usb.c b/drivers/staging/greybus/usb.c
> > > index 8e9d9d59a357..d0b2422401df 100644
> > > --- a/drivers/staging/greybus/usb.c
> > > +++ b/drivers/staging/greybus/usb.c
> > > @@ -27,7 +27,8 @@ struct gb_usb_hub_control_request {
> > >  };
> > >  
> > >  struct gb_usb_hub_control_response {
> > > -	u8 buf[0];
> > > +	__le16 wLength;
> > > +	u8 buf[];
> > 
> > What is wrong with buf[0] here?
> > 
> > You can fix this in other ways if you really understand the difference
> > between [0] and [] in C.  Please look at many of the other conversions
> > if you wish to do this.
> 
> And I would not recommend this as an "outreachy introduction task"
> unless you understand this.  There are much easier first patch tasks you
> can accomplish instead.
> 

Hi Greg, 
I should've made this into a question for the maintainers instead of a
patch. Sorry about that. Dan's and your comments are well noted. 
Thank you,
Jaehee

> good luck!
> 
> greg k-h

Re: [PATCH] staging: greybus: replace zero-element array with flexible-array

[Dan Carpenter]

On Wed, Apr 13, 2022 at 12:50:31AM -0400, Jaehee Park wrote:
> I should've made this into a question for the maintainers instead of a
> patch. Sorry about that.

Not at all.  Always best to send patches instead of questions.  Patches
are easier to discuss as well.  Making mistakes is part of the process.

regards,
dan carpenter