diff mbox series

[v2] media: usb: siano: Fix allocation of urbs

Message ID 20240415-smatch-v2-1-65215936d398@chromium.org (mailing list archive)
State Superseded
Headers show
Series [v2] media: usb: siano: Fix allocation of urbs | expand

Commit Message

Ricardo Ribalda April 15, 2024, 12:12 p.m. UTC 
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@chromium.org>
---
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@chromium.org
---
 drivers/media/usb/siano/smsusb.c | 36 ++++++++++++++++++++++++++----------
 1 file changed, 26 insertions(+), 10 deletions(-)


---
base-commit: 34d7bf1c8e59f5fbf438ee32c96389ebe41ca2e8
change-id: 20240410-smatch-8f235d50753d

Best regards,

Comments

Hans Verkuil April 15, 2024, 12:23 p.m. UTC | #1 
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@chromium.org>
> ---
> 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@chromium.org
> ---
>  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 :-)

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 April 15, 2024, 12:30 p.m. UTC | #2 
Hi Hans

On Mon, 15 Apr 2024 at 14:24, Hans Verkuil <hverkuil-cisco@xs4all.nl> 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@chromium.org>
> > ---
> > 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@chromium.org
> > ---
> >  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,
>
diff mbox series

Patch

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;
+		}
+
 		pr_debug("device 0x%p destroyed\n", dev);
 		kfree(dev);
 	}
@@ -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;
+	}
+
 err_unregister_device:
 	smsusb_term_device(intf);
 #ifdef CONFIG_MEDIA_CONTROLLER_DVB