Hi Hans On Mon, 15 Apr 2024 at 14:24, Hans Verkuil <hverkuil-cisco@xxxxxxxxx> wrote: > > On 15/04/2024 14:12, Ricardo Ribalda wrote: > > USB urbs must be allocated with usb_alloc_urb. Quoting the manual > > > > Only use this function (usb_init_urb) if you _really_ understand what you > > are doing. > > > > Fix the following smatch error: > > > > drivers/media/usb/siano/smsusb.c:53:38: warning: array of flexible structures > > > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx> > > --- > > Changes in v2: Thanks Hans > > - Only leave 1/6, the other ones are in another PR > > - Fix the return tag and NULLify the urbs on return > > - Link to v1: https://lore.kernel.org/r/20240410-smatch-v1-0-785d009a852b@xxxxxxxxxxxx > > --- > > drivers/media/usb/siano/smsusb.c | 36 ++++++++++++++++++++++++++---------- > > 1 file changed, 26 insertions(+), 10 deletions(-) > > > > diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c > > index 723510520d09..2e25b970946a 100644 > > --- a/drivers/media/usb/siano/smsusb.c > > +++ b/drivers/media/usb/siano/smsusb.c > > @@ -40,7 +40,7 @@ struct smsusb_urb_t { > > struct smscore_buffer_t *cb; > > struct smsusb_device_t *dev; > > > > - struct urb urb; > > + struct urb *urb; > > > > /* For the bottom half */ > > struct work_struct wq; > > @@ -160,7 +160,7 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev, > > } > > > > usb_fill_bulk_urb( > > - &surb->urb, > > + surb->urb, > > dev->udev, > > usb_rcvbulkpipe(dev->udev, dev->in_ep), > > surb->cb->p, > > @@ -168,9 +168,9 @@ static int smsusb_submit_urb(struct smsusb_device_t *dev, > > smsusb_onresponse, > > surb > > ); > > - surb->urb.transfer_flags |= URB_FREE_BUFFER; > > + surb->urb->transfer_flags |= URB_FREE_BUFFER; > > > > - return usb_submit_urb(&surb->urb, GFP_ATOMIC); > > + return usb_submit_urb(surb->urb, GFP_ATOMIC); > > } > > > > static void smsusb_stop_streaming(struct smsusb_device_t *dev) > > @@ -178,7 +178,7 @@ static void smsusb_stop_streaming(struct smsusb_device_t *dev) > > int i; > > > > for (i = 0; i < MAX_URBS; i++) { > > - usb_kill_urb(&dev->surbs[i].urb); > > + usb_kill_urb(dev->surbs[i].urb); > > if (dev->surbs[i].wq.func) > > cancel_work_sync(&dev->surbs[i].wq); > > > > @@ -338,6 +338,8 @@ static void smsusb_term_device(struct usb_interface *intf) > > struct smsusb_device_t *dev = usb_get_intfdata(intf); > > > > if (dev) { > > + int i; > > + > > dev->state = SMSUSB_DISCONNECTED; > > > > smsusb_stop_streaming(dev); > > @@ -346,6 +348,11 @@ static void smsusb_term_device(struct usb_interface *intf) > > if (dev->coredev) > > smscore_unregister_device(dev->coredev); > > > > + for (i = 0; i < MAX_URBS; i++) { > > + usb_free_urb(dev->surbs[i].urb); > > + dev->surbs[i].urb = NULL; > > You don't need to assign to NULL here... > > > + } > > + > > pr_debug("device 0x%p destroyed\n", dev); > > kfree(dev); > > ...since here the whole dev struct is freed. > > > } > > @@ -390,6 +397,7 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) > > void *mdev; > > int i, rc; > > int align = 0; > > + int n_urb = 0; > > > > /* create device object */ > > dev = kzalloc(sizeof(struct smsusb_device_t), GFP_KERNEL); > > @@ -461,16 +469,18 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) > > dev->coredev->is_usb_device = true; > > > > /* initialize urbs */ > > - for (i = 0; i < MAX_URBS; i++) { > > - dev->surbs[i].dev = dev; > > - usb_init_urb(&dev->surbs[i].urb); > > + for (n_urb = 0; n_urb < MAX_URBS; n_urb++) { > > + dev->surbs[n_urb].dev = dev; > > + dev->surbs[n_urb].urb = usb_alloc_urb(0, GFP_KERNEL); > > + if (!dev->surbs[n_urb].urb) > > + goto free_urbs; > > } > > > > pr_debug("smsusb_start_streaming(...).\n"); > > rc = smsusb_start_streaming(dev); > > if (rc < 0) { > > pr_err("smsusb_start_streaming(...) failed\n"); > > - goto err_unregister_device; > > + goto free_urbs; > > } > > > > dev->state = SMSUSB_ACTIVE; > > @@ -478,13 +488,19 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id) > > rc = smscore_start_device(dev->coredev); > > if (rc < 0) { > > pr_err("smscore_start_device(...) failed\n"); > > - goto err_unregister_device; > > + goto free_urbs; > > } > > > > pr_debug("device 0x%p created\n", dev); > > > > return rc; > > > > +free_urbs: > > + for (i = 0; i < n_urb; i++) { > > + usb_free_urb(dev->surbs[n_urb].urb); > > + dev->surbs[n_urb].urb = NULL; > > This should use index 'i', right? Not 'n_urb'. > > I'll wait for v3 :-) > ups :) Good catch :) > Regards, > > Hans > > > + } > > + > > err_unregister_device: > > smsusb_term_device(intf); > > #ifdef CONFIG_MEDIA_CONTROLLER_DVB > > > > --- > > base-commit: 34d7bf1c8e59f5fbf438ee32c96389ebe41ca2e8 > > change-id: 20240410-smatch-8f235d50753d > > > > Best regards, > -- Ricardo Ribalda
