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