Re: [PATCH] greybus: Avoid fake flexible array for response data

[Kees Cook <kees@kernel.org>]

On Mon, May 26, 2025 at 02:37:13PM +0300, Dan Carpenter wrote:
> On Mon, May 26, 2025 at 07:06:54PM +0800, clingfei wrote:
> > As https://lore.kernel.org/all/20240304211940.it.083-kees@kernel.org/
> 
> I don't want to have to read a link to understand the commit message.
> 
> Does this change really affect anything in terms of "enforce the 0-sized
> destinations" rule?  I don't think this is a destination.  I think Kees
> enabled the checking he wanted.  You should probably CC him since we're
> refering to his email.

FWIW, the above patch became commit 7ba59ac7da2a ("greybus: Avoid fake
flexible array for response data").

> > pointed out, to enforce the 0-sized destinations, the remaining 0-sized
> > destinations need to be handled. Thus the struct
> > gb_control_get_manifest_response and struct gb_i2c_transfer_response
> > are removed.
> 
> Here is a better commit message;
> 
> "We want to get rid of zero size arrays and use flexible arrays instead.
> However, in this case the struct is just one flexible array of u8 which
> adds no value.  Just use a char pointer instead."
> 
> I would have ignored it, probably but actually the
> gb_control_get_manifest_response struct is not used so put that in a
> separate commit.  That's a simpler commit to review.
> 
> "The gb_control_get_manifest_response struct is not used.  Delete it."

The patch content itself looks mechanically correct. I think Dan's style
suggestions would be good to see. Can you send a v2?

-Kees

> 
> > 
> > Signed-off-by: clingfei <clf700383@gmail.com>
> > ---
> >  drivers/staging/greybus/i2c.c             | 9 ++++-----
> >  include/linux/greybus/greybus_protocols.h | 9 ---------
> >  2 files changed, 4 insertions(+), 14 deletions(-)
> > 
> > diff --git a/drivers/staging/greybus/i2c.c b/drivers/staging/greybus/i2c.c
> > index 14f1ff6d448c..2857c2834206 100644
> > --- a/drivers/staging/greybus/i2c.c
> > +++ b/drivers/staging/greybus/i2c.c
> > @@ -144,15 +144,14 @@ gb_i2c_operation_create(struct gb_connection *connection,
> >  }
> >  
> >  static void gb_i2c_decode_response(struct i2c_msg *msgs, u32 msg_count,
> > -				   struct gb_i2c_transfer_response *response)
> > +				   u8 *data)
> >  {
> >  	struct i2c_msg *msg = msgs;
> > -	u8 *data;
> >  	u32 i;
> >  
> > -	if (!response)
> > +	if (!data)
> >  		return;
> > -	data = response->data;
> > +
> >  	for (i = 0; i < msg_count; i++) {
> >  		if (msg->flags & I2C_M_RD) {
> >  			memcpy(msg->buf, data, msg->len);
> > @@ -188,7 +187,7 @@ static int gb_i2c_transfer_operation(struct gb_i2c_device *gb_i2c_dev,
> >  
> >  	ret = gb_operation_request_send_sync(operation);
> >  	if (!ret) {
> > -		struct gb_i2c_transfer_response *response;
> > +		u8 *response;
> >  
> >  		response = operation->response->payload;
> >  		gb_i2c_decode_response(msgs, msg_count, response);
> 
> I like when parameters are called the same thing on both sides.  The
> name "response" adds no value.  Instead get rid of that variable and
> pass operation->response->payload directly.
> 
> 		gb_i2c_decode_response(msgs, msg_count,
> 				       operation->response->payload);
> 
> regards,
> dan carpenter
> 

-- 
Kees Cook