On 15/04/2024 12:08, Hans Verkuil wrote:
> 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.
Ricardo, just post a v2 of this patch. I posted a PR for the other
patches. Nice work, BTW!
Regards,
Hans
>
> Regards,
>
> Hans
>
>> +
>> err_unregister_device:
>> smsusb_term_device(intf);
>> #ifdef CONFIG_MEDIA_CONTROLLER_DVB
>>
>
>
