Re: [PATCH v6 06/10] media: uvcvideo: Abstract streaming object lifetime

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Kieran,

Thank you for the patch.

On Friday, 9 November 2018 19:05:29 EET Kieran Bingham wrote:
> The streaming object is a key part of handling the UVC device. Although
> not critical, we are currently missing a call to destroy the mutex on
> clean up paths, and we are due to extend the objects complexity in the
> near future.
> 
> Facilitate easy management of a stream object by creating a pair of
> functions to handle creating and destroying the allocation. The new
> uvc_stream_delete() function also performs the missing mutex_destroy()
> operation.
> 
> Previously a failed streaming object allocation would cause
> uvc_parse_streaming() to return -EINVAL, which is inappropriate. If the
> constructor failes, we will instead return -ENOMEM.
> 
> While we're here, fix the trivial spelling error in the function banner
> of uvc_delete().
> 
> Signed-off-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  drivers/media/usb/uvc/uvc_driver.c | 54 +++++++++++++++++++++----------
>  1 file changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_driver.c
> b/drivers/media/usb/uvc/uvc_driver.c index 67bd58c6f397..afb44d1c9d04
> 100644
> --- a/drivers/media/usb/uvc/uvc_driver.c
> +++ b/drivers/media/usb/uvc/uvc_driver.c
> @@ -396,6 +396,39 @@ static struct uvc_streaming *uvc_stream_by_id(struct
> uvc_device *dev, int id) }
> 
>  /* ------------------------------------------------------------------------
> + * Streaming Object Management
> + */
> +
> +static void uvc_stream_delete(struct uvc_streaming *stream)
> +{
> +	mutex_destroy(&stream->mutex);
> +
> +	usb_put_intf(stream->intf);
> +
> +	kfree(stream->format);
> +	kfree(stream->header.bmaControls);
> +	kfree(stream);
> +}
> +
> +static struct uvc_streaming *uvc_stream_new(struct uvc_device *dev,
> +					    struct usb_interface *intf)
> +{
> +	struct uvc_streaming *stream;
> +
> +	stream = kzalloc(sizeof(*stream), GFP_KERNEL);
> +	if (stream == NULL)
> +		return NULL;
> +
> +	mutex_init(&stream->mutex);
> +
> +	stream->dev = dev;
> +	stream->intf = usb_get_intf(intf);
> +	stream->intfnum = intf->cur_altsetting->desc.bInterfaceNumber;
> +
> +	return stream;
> +}
> +
> +/* ------------------------------------------------------------------------
> * Descriptors parsing
>   */
> 
> @@ -687,17 +720,12 @@ static int uvc_parse_streaming(struct uvc_device *dev,
> return -EINVAL;
>  	}
> 
> -	streaming = kzalloc(sizeof(*streaming), GFP_KERNEL);
> +	streaming = uvc_stream_new(dev, intf);
>  	if (streaming == NULL) {
>  		usb_driver_release_interface(&uvc_driver.driver, intf);
> -		return -EINVAL;
> +		return -ENOMEM;
>  	}
> 
> -	mutex_init(&streaming->mutex);
> -	streaming->dev = dev;
> -	streaming->intf = usb_get_intf(intf);
> -	streaming->intfnum = intf->cur_altsetting->desc.bInterfaceNumber;
> -
>  	/* The Pico iMage webcam has its class-specific interface descriptors
>  	 * after the endpoint descriptors.
>  	 */
> @@ -904,10 +932,7 @@ static int uvc_parse_streaming(struct uvc_device *dev,
> 
>  error:
>  	usb_driver_release_interface(&uvc_driver.driver, intf);
> -	usb_put_intf(intf);
> -	kfree(streaming->format);
> -	kfree(streaming->header.bmaControls);
> -	kfree(streaming);
> +	uvc_stream_delete(streaming);
>  	return ret;
>  }
> 
> @@ -1815,7 +1840,7 @@ static int uvc_scan_device(struct uvc_device *dev)
>   * is released.
>   *
>   * As this function is called after or during disconnect(), all URBs have
> - * already been canceled by the USB core. There is no need to kill the
> + * already been cancelled by the USB core. There is no need to kill the
>   * interrupt URB manually.
>   */
>  static void uvc_delete(struct kref *kref)
> @@ -1853,10 +1878,7 @@ static void uvc_delete(struct kref *kref)
>  		streaming = list_entry(p, struct uvc_streaming, list);
>  		usb_driver_release_interface(&uvc_driver.driver,
>  			streaming->intf);
> -		usb_put_intf(streaming->intf);
> -		kfree(streaming->format);
> -		kfree(streaming->header.bmaControls);
> -		kfree(streaming);
> +		uvc_stream_delete(streaming);
>  	}
> 
>  	kfree(dev);

-- 
Regards,

Laurent Pinchart