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

[Dan Carpenter <dan.carpenter@oracle.com>]

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