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 <[email protected]>
---
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/[email protected]
---
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;
+ }
+
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
---
base-commit: 34d7bf1c8e59f5fbf438ee32c96389ebe41ca2e8
change-id: 20240410-smatch-8f235d50753d
Best regards,
--
Ricardo Ribalda <[email protected]>
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 <[email protected]>
> ---
> 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/[email protected]
> ---
> 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,
Hi Hans
On Mon, 15 Apr 2024 at 14:24, Hans Verkuil <[email protected]> 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 <[email protected]>
> > ---
> > 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/[email protected]
> > ---
> > 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