All right, if you want to do that, fine. I did it this way, because it
took 1 less `goto`s... But your approach is equally valid.
But, to keep it consistent, if you change it one place, change it
everywhere else.
Regards,
Bence
Dan Carpenter <dan.carpenter@xxxxxxxxxx> ezt írta (időpont: 2021. máj.
12., Sze, 15:11):
>
> On Wed, May 12, 2021 at 12:54:47PM +0200, Bence Csókás wrote:
> > Dan Carpenter <dan.carpenter@xxxxxxxxxx> ezt írta (időpont: 2021. máj.
> > 12., Sze, 12:07):
> > >
> > > We need to add a check for if the kzalloc() fails.
> >
> > That is correct, I missed that :/
> >
> > >
> > > Fixes: 4a7695429ead ("i2c: cp2615: add i2c driver for Silicon Labs' CP2615 Digital Audio Bridge")
> > > Signed-off-by: Dan Carpenter <dan.carpenter@xxxxxxxxxx>
> > > ---
> > > drivers/i2c/busses/i2c-cp2615.c | 14 ++++++++++----
> > > 1 file changed, 10 insertions(+), 4 deletions(-)
> > >
> > > diff --git a/drivers/i2c/busses/i2c-cp2615.c b/drivers/i2c/busses/i2c-cp2615.c
> > > index 78cfecd1ea76..3ded28632e4c 100644
> > > --- a/drivers/i2c/busses/i2c-cp2615.c
> > > +++ b/drivers/i2c/busses/i2c-cp2615.c
> > > @@ -138,17 +138,23 @@ cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)
> > > static int
> > > cp2615_i2c_recv(struct usb_interface *usbif, unsigned char tag, void *buf)
> > > {
> > > - struct cp2615_iop_msg *msg = kzalloc(sizeof(*msg), GFP_KERNEL);
> > > - struct cp2615_i2c_transfer_result *i2c_r = (struct cp2615_i2c_transfer_result *)&msg->data;
> >
> > These two lines could stay as-is, since the invalid i2c_r will never be used.
> >
>
> Yeah, I know but people shouldn't put functions which can fail inside
> the declaration block. And I also know that doing:
>
> struct cp2615_i2c_transfer_result *i2c_r = (struct cp2615_i2c_transfer_result *)&msg->data;
>
> is fine even when "msg" is NULL. But don't do that, because it is ugly.
>
> > > struct usb_device *usbdev = interface_to_usbdev(usbif);
> > > - int res = usb_bulk_msg(usbdev, usb_rcvbulkpipe(usbdev, IOP_EP_IN),
> > > - msg, sizeof(struct cp2615_iop_msg), NULL, 0);
> > > + struct cp2615_iop_msg *msg;
> > > + struct cp2615_i2c_transfer_result *i2c_r;
> > > + int res;
> > > +
> > > + msg = kzalloc(sizeof(*msg), GFP_KERNEL);
> > > + if (!msg)
> > > + return -ENOMEM;
> >
> > You will want to also make `cp2615_init_iop_msg()` return -ENOMEM
> > instead of -EINVAL, for consistency's sake.
> >
>
> I view that as unrelated and I'm sorry but this error handling code is
> not really best practices... I don't have time to fix it up. :/
>
> drivers/i2c/busses/i2c-cp2615.c
> 124 static int
> 125 cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)
> 126 {
> 127 struct cp2615_iop_msg *msg = kzalloc(sizeof(*msg), GFP_KERNEL);
>
> Don't put functions which can fail inside the declaration block.
> Although, it is relatively uncommon to put allocations inside the
> declaration block these sorts of allocations are over represented when
> it comes to memory leaks and other static analysis bugs.
>
> 128 struct usb_device *usbdev = interface_to_usbdev(usbif);
> 129 int res = cp2615_init_i2c_msg(msg, i2c_w);
>
> Handle failures immediately. Try not to mix the error paths and the
> success paths. After a function call, then you're going to have to deal
> with both failure and success path, but the failure path is just
> "clean up and return an error code" and the success path is hopefully
> going to continue for days or months. So get the shorter failure path
> out of the way first, then continue with the success path.
>
> 130
> 131 if (!res)
>
> Always do error handling, don't do success handling.
>
> 132 res = usb_bulk_msg(usbdev, usb_sndbulkpipe(usbdev, IOP_EP_OUT),
> 133 msg, ntohs(msg->length), NULL, 0);
>
> The success path should be indented one tab and the error handling
> should be indented two tabs.
>
> 134 kfree(msg);
> 135 return res;
> 136 }
>
> The function should look like this.
>
> static int
> cp2615_i2c_send(struct usb_interface *usbif, struct cp2615_i2c_transfer *i2c_w)
> {
> struct usb_device *usbdev = interface_to_usbdev(usbif);
> struct cp2615_iop_msg *msg;
> int res;
>
> msg = kzalloc(sizeof(*msg), GFP_KERNEL);
> if (!msg)
> return -ENOMEM;
>
> res = cp2615_init_i2c_msg(msg, i2c_w);
> if (res)
> goto free;
>
> res = usb_bulk_msg(usbdev, usb_sndbulkpipe(usbdev, IOP_EP_OUT),
> msg, ntohs(msg->length), NULL, 0);
> free:
> kfree(msg);
> return res;
> }
>
> regards,
> dan carpenter
>
