Hi Ricardo,
Thank you for the patch.
On Tue, Dec 06, 2022 at 03:06:55PM +0100, Ricardo Ribalda wrote:
> Add a new variable to handle the streaming state and handle the
> streamoff errors, that were not handled before.
>
> Suggested-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
> Reviewed-by: Max Staudt <mstaudt@xxxxxxxxxxxx>
> Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> ---
> drivers/media/usb/uvc/uvc_v4l2.c | 19 ++++++++++++++++---
> drivers/media/usb/uvc/uvcvideo.h | 1 +
> 2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/media/usb/uvc/uvc_v4l2.c b/drivers/media/usb/uvc/uvc_v4l2.c
> index f4d4c33b6dfb..1389a87b8ae1 100644
> --- a/drivers/media/usb/uvc/uvc_v4l2.c
> +++ b/drivers/media/usb/uvc/uvc_v4l2.c
> @@ -840,13 +840,19 @@ static int uvc_ioctl_streamon(struct file *file, void *fh,
> {
> struct uvc_fh *handle = fh;
> struct uvc_streaming *stream = handle->stream;
> - int ret;
> + int ret = -EBUSY;
>
> if (!uvc_has_privileges(handle))
> return -EBUSY;
>
> mutex_lock(&stream->mutex);
> +
> + if (handle->is_streaming)
> + goto unlock;
This isn't needed, uvc_queue_streamon() calls vb2_streamon(), which
returns an error if the queue is already streaming.
> ret = uvc_queue_streamon(&stream->queue, type);
> + handle->is_streaming = !ret;
You can just turn this into
if (!ret)
handle->is_streaming = true;
and drop all other changes to this function.
> +
> +unlock:
> mutex_unlock(&stream->mutex);
>
> return ret;
> @@ -857,15 +863,22 @@ static int uvc_ioctl_streamoff(struct file *file, void *fh,
> {
> struct uvc_fh *handle = fh;
> struct uvc_streaming *stream = handle->stream;
> + int ret = 0;
>
> if (!uvc_has_privileges(handle))
> return -EBUSY;
>
> mutex_lock(&stream->mutex);
> - uvc_queue_streamoff(&stream->queue, type);
> +
> + if (!handle->is_streaming)
> + goto unlock;
More than unneeded, this is wrong. Calling VIDIOC_STREAMOFF on a queue
that is not streaming is a valid use case, it's the only way to release
buffers that have been queued to the device. vb2_core_streamoff()
handles this correctly. You should drop this check.
> + ret = uvc_queue_streamoff(&stream->queue, type);
> + handle->is_streaming = !!ret;
And turn this into
if (!ret)
handle->is_streaming = false;
and drop all other changes to this function.
> +
> +unlock:
> mutex_unlock(&stream->mutex);
>
> - return 0;
> + return ret;
> }
>
> static int uvc_ioctl_enum_input(struct file *file, void *fh,
> diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> index df93db259312..0d9f335053b8 100644
> --- a/drivers/media/usb/uvc/uvcvideo.h
> +++ b/drivers/media/usb/uvc/uvcvideo.h
> @@ -584,6 +584,7 @@ struct uvc_fh {
> struct uvc_video_chain *chain;
> struct uvc_streaming *stream;
> enum uvc_handle_state state;
> + bool is_streaming;
> };
>
> struct uvc_driver {
>
--
Regards,
Laurent Pinchart
