On 15/04/2024 14:29, 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 v3: Thanks Hans > - Remove unneeded NULL assignment > - Use the proper variable :) > - Link to v2: https://lore.kernel.org/r/20240415-smatch-v2-1-65215936d398@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 | 34 ++++++++++++++++++++++++---------- > 1 file changed, 24 insertions(+), 10 deletions(-) > > diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c > index 723510520d09..ba2c02e1070e 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,9 @@ 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); > + > pr_debug("device 0x%p destroyed\n", dev); > kfree(dev); > } > @@ -390,6 +395,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 +467,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 +486,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[i].urb); > + dev->surbs[i].urb = NULL; > + } > + > err_unregister_device: > smsusb_term_device(intf); After re-reading the code, I see that smsusb_term_device() actually frees all URBs. So the 'free_urbs' goto label is not needed, it can go straight to err_unregister_device. A comment just above the 'smsusb_term_device(intf);' line mentioning that it will free any allocated urbs would be helpful, though. Regards, Hans > #ifdef CONFIG_MEDIA_CONTROLLER_DVB > > --- > base-commit: 34d7bf1c8e59f5fbf438ee32c96389ebe41ca2e8 > change-id: 20240410-smatch-8f235d50753d > > Best regards,
