[PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling

[PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling

[Anton Leontiev]

Set error bit for incomplete buffers when end of buffer is detected by
FID toggling (for example when last transaction with EOF is lost).
This prevents passing incomplete buffers to the userspace.

Signed-off-by: Anton Leontiev <bunder@t-25.ru>
---
 drivers/media/usb/uvc/uvc_video.c | 21 +++++++++++++++------
 1 file changed, 15 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
index 8d52baf..57c9a4b 100644
--- a/drivers/media/usb/uvc/uvc_video.c
+++ b/drivers/media/usb/uvc/uvc_video.c
@@ -1133,6 +1133,17 @@ static int uvc_video_encode_data(struct uvc_streaming *stream,
  */
 
 /*
+ * Set error flag for incomplete buffer.
+ */
+static void uvc_buffer_check_bytesused(const struct uvc_streaming *const stream,
+	struct uvc_buffer *const buf)
+{
+	if (buf->length != buf->bytesused &&
+			!(stream->cur_format->flags & UVC_FMT_FLAG_COMPRESSED))
+		buf->error = 1;
+}
+
+/*
  * Completion handler for video URBs.
  */
 static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming *stream,
@@ -1156,9 +1167,11 @@ static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming *stream,
 		do {
 			ret = uvc_video_decode_start(stream, buf, mem,
 				urb->iso_frame_desc[i].actual_length);
-			if (ret == -EAGAIN)
+			if (ret == -EAGAIN) {
+				uvc_buffer_check_bytesused(stream, buf);
 				buf = uvc_queue_next_buffer(&stream->queue,
 							    buf);
+			}
 		} while (ret == -EAGAIN);
 
 		if (ret < 0)
@@ -1173,11 +1186,7 @@ static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming *stream,
 			urb->iso_frame_desc[i].actual_length);
 
 		if (buf->state == UVC_BUF_STATE_READY) {
-			if (buf->length != buf->bytesused &&
-			    !(stream->cur_format->flags &
-			      UVC_FMT_FLAG_COMPRESSED))
-				buf->error = 1;
-
+			uvc_buffer_check_bytesused(stream, buf);
 			buf = uvc_queue_next_buffer(&stream->queue, buf);
 		}
 	}
-- 
1.9.1

Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling

[Laurent Pinchart]

Hi Anton,

Thank you for the patch.

On Tuesday 25 March 2014 08:40:57 Anton Leontiev wrote:
> Set error bit for incomplete buffers when end of buffer is detected by
> FID toggling (for example when last transaction with EOF is lost).
> This prevents passing incomplete buffers to the userspace.

But this would also breaks buggy webcams that toggle the FID bit but don't set 
the EOF bit. Support for this was added before the driver got merged in the 
mainline kernel, and the SVN log is a bit terse I'm afraid:

V 104
- Check both EOF and FID bits to detect end of frames.
- Updated disclaimer and general status comment.

I don't remember which webcam(s) exhibit this behaviour.

Your patch would also mark valid buffers as erroneous when the list EOF bit is 
in a packet of its own with no data.

As the uvcvideo driver already marks buffers smaller than the expected image 
size as erroneous, missing EOF packets that contain data should already result 
in buffers with the error bit set. Are you concerned about compressed formats 
only ? While this patch would correctly detect the missing EOF packet in that 
case, any other missing packet would still result in a corrupt image, so I'm 
not sure if this would be worth it.

> Signed-off-by: Anton Leontiev <bunder@t-25.ru>
> ---
>  drivers/media/usb/uvc/uvc_video.c | 21 +++++++++++++++------
>  1 file changed, 15 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/media/usb/uvc/uvc_video.c
> b/drivers/media/usb/uvc/uvc_video.c index 8d52baf..57c9a4b 100644
> --- a/drivers/media/usb/uvc/uvc_video.c
> +++ b/drivers/media/usb/uvc/uvc_video.c
> @@ -1133,6 +1133,17 @@ static int uvc_video_encode_data(struct uvc_streaming
> *stream, */
> 
>  /*
> + * Set error flag for incomplete buffer.
> + */
> +static void uvc_buffer_check_bytesused(const struct uvc_streaming *const
> stream,
> +	struct uvc_buffer *const buf)
> +{
> +	if (buf->length != buf->bytesused &&
> +			!(stream->cur_format->flags & UVC_FMT_FLAG_COMPRESSED))
> +		buf->error = 1;
> +}
> +
> +/*
>   * Completion handler for video URBs.
>   */
>  static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
> *stream, @@ -1156,9 +1167,11 @@ static void uvc_video_decode_isoc(struct
> urb *urb, struct uvc_streaming *stream, do {
>  			ret = uvc_video_decode_start(stream, buf, mem,
>  				urb->iso_frame_desc[i].actual_length);
> -			if (ret == -EAGAIN)
> +			if (ret == -EAGAIN) {
> +				uvc_buffer_check_bytesused(stream, buf);
>  				buf = uvc_queue_next_buffer(&stream->queue,
>  							    buf);
> +			}
>  		} while (ret == -EAGAIN);
> 
>  		if (ret < 0)
> @@ -1173,11 +1186,7 @@ static void uvc_video_decode_isoc(struct urb *urb,
> struct uvc_streaming *stream, urb->iso_frame_desc[i].actual_length);
> 
>  		if (buf->state == UVC_BUF_STATE_READY) {
> -			if (buf->length != buf->bytesused &&
> -			    !(stream->cur_format->flags &
> -			      UVC_FMT_FLAG_COMPRESSED))
> -				buf->error = 1;
> -
> +			uvc_buffer_check_bytesused(stream, buf);
>  			buf = uvc_queue_next_buffer(&stream->queue, buf);
>  		}
>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling

[Anton Leontiev]

26.03.2014 21:41, Laurent Pinchart wrote:
> Hi Anton,
> 
> Thank you for the patch.
> 
> On Tuesday 25 March 2014 08:40:57 Anton Leontiev wrote:
>> Set error bit for incomplete buffers when end of buffer is detected by
>> FID toggling (for example when last transaction with EOF is lost).
>> This prevents passing incomplete buffers to the userspace.
> 
> But this would also breaks buggy webcams that toggle the FID bit but don't set 
> the EOF bit. Support for this was added before the driver got merged in the 
> mainline kernel, and the SVN log is a bit terse I'm afraid:
> 
> V 104
> - Check both EOF and FID bits to detect end of frames.
> - Updated disclaimer and general status comment.
> 
> I don't remember which webcam(s) exhibit this behaviour.
> 
> Your patch would also mark valid buffers as erroneous when the list EOF bit is 
> in a packet of its own with no data.

If camera streams compressed video the patch doesn't affect it. It
affects only uncompressed streams.

If we get EOF bit in a packet of its own with no data we take second
check under 'if (buf->state == UVC_BUF_STATE_READY)' that was before my
patch. In this case if all data were received buffer is processed
normally, but if some data is missing buffer is marked erroneous.

> As the uvcvideo driver already marks buffers smaller than the expected image 
> size as erroneous, missing EOF packets that contain data should already result 
> in buffers with the error bit set.

No, because there was no check for that in case then new frame is
detected by FID toggling, it was there only for the case then new frame
is detected by EOF bit.

> Are you concerned about compressed formats only ?

No, I'm concerning about uncompressed frames only.

> While this patch would correctly detect the missing EOF packet in that
> case, any other missing packet would still result in a corrupt image, so I'm 
> not sure if this would be worth it.
> 
>> Signed-off-by: Anton Leontiev <bunder@t-25.ru>
>> ---
>>  drivers/media/usb/uvc/uvc_video.c | 21 +++++++++++++++------
>>  1 file changed, 15 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_video.c
>> b/drivers/media/usb/uvc/uvc_video.c index 8d52baf..57c9a4b 100644
>> --- a/drivers/media/usb/uvc/uvc_video.c
>> +++ b/drivers/media/usb/uvc/uvc_video.c
>> @@ -1133,6 +1133,17 @@ static int uvc_video_encode_data(struct uvc_streaming
>> *stream, */
>>
>>  /*
>> + * Set error flag for incomplete buffer.
>> + */
>> +static void uvc_buffer_check_bytesused(const struct uvc_streaming *const
>> stream,
>> +	struct uvc_buffer *const buf)
>> +{
>> +	if (buf->length != buf->bytesused &&
>> +			!(stream->cur_format->flags & UVC_FMT_FLAG_COMPRESSED))
>> +		buf->error = 1;
>> +}
>> +
>> +/*
>>   * Completion handler for video URBs.
>>   */
>>  static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
>> *stream, @@ -1156,9 +1167,11 @@ static void uvc_video_decode_isoc(struct
>> urb *urb, struct uvc_streaming *stream, do {
>>  			ret = uvc_video_decode_start(stream, buf, mem,
>>  				urb->iso_frame_desc[i].actual_length);
>> -			if (ret == -EAGAIN)
>> +			if (ret == -EAGAIN) {
>> +				uvc_buffer_check_bytesused(stream, buf);
>>  				buf = uvc_queue_next_buffer(&stream->queue,
>>  							    buf);
>> +			}
>>  		} while (ret == -EAGAIN);
>>
>>  		if (ret < 0)
>> @@ -1173,11 +1186,7 @@ static void uvc_video_decode_isoc(struct urb *urb,
>> struct uvc_streaming *stream, urb->iso_frame_desc[i].actual_length);
>>
>>  		if (buf->state == UVC_BUF_STATE_READY) {
>> -			if (buf->length != buf->bytesused &&
>> -			    !(stream->cur_format->flags &
>> -			      UVC_FMT_FLAG_COMPRESSED))
>> -				buf->error = 1;
>> -
>> +			uvc_buffer_check_bytesused(stream, buf);
>>  			buf = uvc_queue_next_buffer(&stream->queue, buf);
>>  		}
>>  	}
> 

-- 
Anton Leontiev

Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling

[Laurent Pinchart]

Hi Anton,

On Friday 28 March 2014 07:58:00 Anton Leontiev wrote:
> 26.03.2014 21:41, Laurent Pinchart wrote:
> > On Tuesday 25 March 2014 08:40:57 Anton Leontiev wrote:
> >> Set error bit for incomplete buffers when end of buffer is detected by
> >> FID toggling (for example when last transaction with EOF is lost).
> >> This prevents passing incomplete buffers to the userspace.
> > 
> > But this would also breaks buggy webcams that toggle the FID bit but don't
> > set the EOF bit. Support for this was added before the driver got merged
> > in the mainline kernel, and the SVN log is a bit terse I'm afraid:
> > 
> > V 104
> > - Check both EOF and FID bits to detect end of frames.
> > - Updated disclaimer and general status comment.
> > 
> > I don't remember which webcam(s) exhibit this behaviour.
> > 
> > Your patch would also mark valid buffers as erroneous when the list EOF
> > bit is in a packet of its own with no data.
> 
> If camera streams compressed video the patch doesn't affect it. It affects
> only uncompressed streams.
> 
> If we get EOF bit in a packet of its own with no data we take second
> check under 'if (buf->state == UVC_BUF_STATE_READY)' that was before my
> patch. In this case if all data were received buffer is processed
> normally, but if some data is missing buffer is marked erroneous.
> 
> > As the uvcvideo driver already marks buffers smaller than the expected
> > image size as erroneous, missing EOF packets that contain data should
> > already result in buffers with the error bit set.
> 
> No, because there was no check for that in case then new frame is detected
> by FID toggling, it was there only for the case then new frame is detected
> by EOF bit.

Right, my bad, I had misread your patch. Please see below for a couple of 
small comments.

> > Are you concerned about compressed formats only ?
> 
> No, I'm concerning about uncompressed frames only.
> 
> > While this patch would correctly detect the missing EOF packet in that
> > case, any other missing packet would still result in a corrupt image, so
> > I'm not sure if this would be worth it.
> > 
> >> Signed-off-by: Anton Leontiev <bunder@t-25.ru>
> >> ---
> >> 
> >>  drivers/media/usb/uvc/uvc_video.c | 21 +++++++++++++++------
> >>  1 file changed, 15 insertions(+), 6 deletions(-)
> >> 
> >> diff --git a/drivers/media/usb/uvc/uvc_video.c
> >> b/drivers/media/usb/uvc/uvc_video.c index 8d52baf..57c9a4b 100644
> >> --- a/drivers/media/usb/uvc/uvc_video.c
> >> +++ b/drivers/media/usb/uvc/uvc_video.c
> >> @@ -1133,6 +1133,17 @@ static int uvc_video_encode_data(struct
> >> uvc_streaming *stream, */
> >> 
> >>  /*
> >> 
> >> + * Set error flag for incomplete buffer.
> >> + */
> >> +static void uvc_buffer_check_bytesused(const struct uvc_streaming *const
> >> stream,

No need for the second const keyword here.

I would have used "uvc_video_" as a prefix, to be in sync with the surrounding 
functions. What would you think of uvc_video_validate_buffer() ?

> >> +	struct uvc_buffer *const buf)

And no need for const at all here.

> >> +{
> >> +	if (buf->length != buf->bytesused &&
> >> +			!(stream->cur_format->flags & UVC_FMT_FLAG_COMPRESSED))

The indentation is wrong here, the ! on the second line should be aligned to 
the first 'buf' of the first line.

If you agree with these changes I can perform them while applying, there's no 
need to resubmit the patch.

> >> +		buf->error = 1;
> >> +}
> >> +
> >> +/*
> >>   * Completion handler for video URBs.
> >>   */
> >>  static void uvc_video_decode_isoc(struct urb *urb, struct uvc_streaming
> >> *stream,
> >> @@ -1156,9 +1167,11 @@ static void uvc_video_decode_isoc(struct
> >> urb *urb, struct uvc_streaming *stream,
> >>  		do {
> >>  			ret = uvc_video_decode_start(stream, buf, mem,
> >>  				urb->iso_frame_desc[i].actual_length);
> >> -			if (ret == -EAGAIN)
> >> +			if (ret == -EAGAIN) {
> >> +				uvc_buffer_check_bytesused(stream, buf);
> >>  				buf = uvc_queue_next_buffer(&stream->queue,
> >>  							    buf);
> >> +			}
> >>  		} while (ret == -EAGAIN);
> >>  		
> >>  		if (ret < 0)
> >> @@ -1173,11 +1186,7 @@ static void uvc_video_decode_isoc(struct urb *urb,
> >> struct uvc_streaming *stream, urb->iso_frame_desc[i].actual_length);
> >>  		if (buf->state == UVC_BUF_STATE_READY) {
> >> -			if (buf->length != buf->bytesused &&
> >> -			    !(stream->cur_format->flags &
> >> -			      UVC_FMT_FLAG_COMPRESSED))
> >> -				buf->error = 1;
> >> -
> >> +			uvc_buffer_check_bytesused(stream, buf);
> >>  			buf = uvc_queue_next_buffer(&stream->queue, buf);
> >>  		}
> >>  	}

-- 
Regards,

Laurent Pinchart

Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling

[Anton Leontiev]

28.03.2014 20:12, Laurent Pinchart пишет:
>>>> + * Set error flag for incomplete buffer.
>>>> + */
>>>> +static void uvc_buffer_check_bytesused(const struct uvc_streaming *const
>>>> stream,
> 
> No need for the second const keyword here.
> 
> I would have used "uvc_video_" as a prefix, to be in sync with the surrounding 
> functions. What would you think of uvc_video_validate_buffer() ?
> 
>>>> +	struct uvc_buffer *const buf)
> 
> And no need for const at all here.
> 
>>>> +{
>>>> +	if (buf->length != buf->bytesused &&
>>>> +			!(stream->cur_format->flags & UVC_FMT_FLAG_COMPRESSED))
> 
> The indentation is wrong here, the ! on the second line should be aligned to 
> the first 'buf' of the first line.
> 
> If you agree with these changes I can perform them while applying, there's no 
> need to resubmit the patch.
> 

Thank you for reviewing my first patch to Linux kernel. I completely
agree with your changes.

Just want to ask why there is no need for the second 'const' after
pointer character '*'? I thought it marks pointer itself as constant for
type-checking opposite to first 'const', which marks memory it points to
as constant for type-checking. I understand that the function is simple
enough to verify it by hand but it's better to add more information for
automatic checking.

Is there any guidelines on 'const' keyword usage in Linux kernel code?

Regards,

-- 
Anton Leontiev

Re: [PATCH] [media] uvcvideo: Fix marking buffer erroneous in case of FID toggling

[Laurent Pinchart]

Hi Anton,

On Saturday 29 March 2014 09:28:02 Anton Leontiev wrote:
> 28.03.2014 20:12, Laurent Pinchart пишет:
> >>>> + * Set error flag for incomplete buffer.
> >>>> + */
> >>>> +static void uvc_buffer_check_bytesused(const struct uvc_streaming
> >>>> *const stream,
> > 
> > No need for the second const keyword here.
> > 
> > I would have used "uvc_video_" as a prefix, to be in sync with the
> > surrounding functions. What would you think of
> > uvc_video_validate_buffer() ?
> > 
> >>>> +	struct uvc_buffer *const buf)
> > 
> > And no need for const at all here.
> > 
> >>>> +{
> >>>> +	if (buf->length != buf->bytesused &&
> >>>> +			!(stream->cur_format->flags & UVC_FMT_FLAG_COMPRESSED))
> > 
> > The indentation is wrong here, the ! on the second line should be aligned
> > to the first 'buf' of the first line.
> > 
> > If you agree with these changes I can perform them while applying, there's
> > no need to resubmit the patch.
> 
> Thank you for reviewing my first patch to Linux kernel.

Thank you for submitting it :-)

> I completely agree with your changes.
> 
> Just want to ask why there is no need for the second 'const' after pointer
> character '*'? I thought it marks pointer itself as constant for type-
> checking opposite to first 'const', which marks memory it points to as
> constant for type-checking.

Your understanding is correct (as far as I know).

> I understand that the function is simple enough to verify it by hand but
> it's better to add more information for automatic checking.
>
> Is there any guidelines on 'const' keyword usage in Linux kernel code?

Marking the memory pointed to by the pointer as const ensures that the 
function won't modify memory that the caller thought wouldn't be modified. It 
thus serves as a contract between the caller and the callee, enforced by the 
compiler.

Marking the pointer as const, on the other hand, only affects the function 
implementation, without influencing its call contract. Its only use in this 
case would be to prevent the function from modifying a pointer that the 
function itself thought it shouldn't modify. While not useless in all cases, 
this compile-time checked if usually not performed in the kernel, especially 
for simple functions. I'm not aware of any explicit rule regarding const usage 
though.

-- 
Regards,

Laurent Pinchart