On 10/04/2024 23:54, 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>
> ---
> drivers/media/usb/siano/smsusb.c | 28 ++++++++++++++++++++--------
> 1 file changed, 20 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
> index 723510520d092..d85308e0785db 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,9 +467,11 @@ 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;
> }
After allocating the URBs there are a few more error paths that do
'goto err_unregister_device;' instead of 'goto free_urbs;'. From what
I can see, those need to go through 'free_urbs' as well.
>
> pr_debug("smsusb_start_streaming(...).\n");
> @@ -485,6 +493,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
>
> return rc;
>
> +free_urbs:
> + for (i = 0; i < n_urb; i++)
> + usb_free_urb(dev->surbs[n_urb].urb);
Would it be better to also assign NULL to dev->surbs[n_urb].urb?
That way there are no invalid pointers that can mess up things.
Regards,
Hans
> +
> err_unregister_device:
> smsusb_term_device(intf);
> #ifdef CONFIG_MEDIA_CONTROLLER_DVB
>
