Hi Sebastian,
Thanks for the patch.
On Sunday 16 September 2012 21:58:42 Sebastian Andrzej Siewior wrote:
> The "video->minor = -1" assigment is done in V4L2 by
> video_register_device() so it is removed here.
> Now. uvc_function_bind() calls in error case uvc_function_unbind() for
> cleanup. The problem is that uvc_function_unbind() frees the uvc struct
> and uvc_bind_config() does as well in error case of usb_add_function().
> Removing kfree() in usb_add_function() would make the patch smaller but
> it would look odd because the new allocated memory is not cleaned up.
> However it is not guaranteed that if we call usb_add_function() we also
> get to the bind function.
> Therefore the patch extracts the conditional cleanup from
> uvc_function_unbind() applies to uvc_function_bind().
> uvc_function_unbind() now contains only the complete cleanup which is
> required once everything has been registrated.
>
> Cc: Laurent Pinchart <laurent.pinch...@ideasonboard.com>
> Cc: Bhupesh Sharma <bhupesh.sha...@st.com>
> Signed-off-by: Sebastian Andrzej Siewior <bige...@linutronix.de>
> ---
> drivers/usb/gadget/f_uvc.c | 39 +++++++++++++++++++++------------------
> 1 file changed, 21 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/usb/gadget/f_uvc.c b/drivers/usb/gadget/f_uvc.c
> index 2a8bf06..10f13c1 100644
> --- a/drivers/usb/gadget/f_uvc.c
> +++ b/drivers/usb/gadget/f_uvc.c
> @@ -417,7 +417,6 @@ uvc_register_video(struct uvc_device *uvc)
> return -ENOMEM;
>
> video->parent = &cdev->gadget->dev;
> - video->minor = -1;
> video->fops = &uvc_v4l2_fops;
> video->release = video_device_release;
> strncpy(video->name, cdev->gadget->name, sizeof(video->name));
> @@ -577,23 +576,12 @@ uvc_function_unbind(struct usb_configuration *c,
> struct usb_function *f)
>
> INFO(cdev, "uvc_function_unbind\n");
>
> - if (uvc->vdev) {
> - if (uvc->vdev->minor == -1)
> - video_device_release(uvc->vdev);
> - else
> - video_unregister_device(uvc->vdev);
> - uvc->vdev = NULL;
> - }
> -
> - if (uvc->control_ep)
> - uvc->control_ep->driver_data = NULL;
> - if (uvc->video.ep)
> - uvc->video.ep->driver_data = NULL;
> + video_unregister_device(uvc->vdev);
> + uvc->control_ep->driver_data = NULL;
> + uvc->video.ep->driver_data = NULL;
>
> - if (uvc->control_req) {
> - usb_ep_free_request(cdev->gadget->ep0, uvc->control_req);
> - kfree(uvc->control_buf);
> - }
What would you think about moving that part of the cleanup code to a new
function and calling it in both uvc_function_unbind() and in the error path of
uvc_function_bind() ?
> + usb_ep_free_request(cdev->gadget->ep0, uvc->control_req);
> + kfree(uvc->control_buf);
>
> kfree(f->descriptors);
> kfree(f->hs_descriptors);
> @@ -740,7 +728,22 @@ uvc_function_bind(struct usb_configuration *c, struct
> usb_function *f) return 0;
>
> error:
> - uvc_function_unbind(c, f);
> + if (uvc->vdev)
> + video_device_release(uvc->vdev);
> +
> + if (uvc->control_ep)
> + uvc->control_ep->driver_data = NULL;
> + if (uvc->video.ep)
> + uvc->video.ep->driver_data = NULL;
> +
> + if (uvc->control_req) {
> + usb_ep_free_request(cdev->gadget->ep0, uvc->control_req);
> + kfree(uvc->control_buf);
> + }
> +
> + kfree(f->descriptors);
> + kfree(f->hs_descriptors);
> + kfree(f->ss_descriptors);
> return ret;
> }
--
Regards,
Laurent Pinchart
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
