| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZfCju80gnglLNB6N@pengutronix.de>
Date: Tue, 12 Mar 2024 19:49:31 +0100
From: Michael Grzeschik <mgr@...gutronix.de>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Laurent Pinchart <laurent.pinchart@...asonboard.com>,
Mauro Carvalho Chehab <mchehab@...nel.org>,
linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
kernel@...gutronix.de
Subject: Re: [PATCH v2] uvc_video: check for fid change early in decode_start
and avoid wrong error counting
On Thu, Feb 29, 2024 at 07:20:53PM +0100, Ricardo Ribalda wrote:
>Hi Michael
>
>So if my understanding is correct, what you want to achieve is to
>avoid double stats_decode when the function returns -EAGAIN.
>
>Wouldn't it be simpler to simply move uvc_video_clock_decode() and
>uvc_video_stats_decode() before
>
>stream->last_fid = fid;
>
>just at the end of the function? Or am I missing something?
>
>Besides being a small and documented function,
>uvc_video_decode_start() is difficult to follow :), So I might be
>saying something stupid
No, there is nothing stupid. I tested the changed patch. And it works as
expected. So thanks for the review and the extra mile. I will send v3 now.
Regards,
Michael
>On Sat, 24 Feb 2024 at 23:52, Michael Grzeschik
><m.grzeschik@...gutronix.de> wrote:
>>
>> When the uvc request will get parsed by uvc_video_decode_start it will
>> leave the function with -EAGAIN to be restarted on the next frame. While
>> the first wrong parse the statistics will already be updated with
>> uvc_video_stats_decode.
>>
>> One value e.g. is the error_count, which therefor will be incremented
>> twice in case the fid has changed on the way. This patch fixes the
>> unnecessary extra parsing by returning early from the function when the
>> fid has changed.
>>
>> Signed-off-by: Michael Grzeschik <m.grzeschik@...gutronix.de>
>> ---
>> Changes in v2:
>> - Moved the EAGAIN bailout after the sequence handling as mentioned by Ricardo Ribalda
>> - Link to v1: https://lore.kernel.org/r/20240221-uvc-host-video-decode-start-v1-1-228995925c70@pengutronix.de
>> ---
>> drivers/media/usb/uvc/uvc_video.c | 64 +++++++++++++++++++--------------------
>> 1 file changed, 32 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
>> index 7cbf4692bd875..af368c45c4297 100644
>> --- a/drivers/media/usb/uvc/uvc_video.c
>> +++ b/drivers/media/usb/uvc/uvc_video.c
>> @@ -1068,6 +1068,15 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
>>
>> fid = data[1] & UVC_STREAM_FID;
>>
>> + /*
>> + * Store the payload FID bit and return immediately when the buffer is
>> + * NULL.
>> + */
>> + if (buf == NULL) {
>> + stream->last_fid = fid;
>> + return -ENODATA;
>> + }
>> +
>> /*
>> * Increase the sequence number regardless of any buffer states, so
>> * that discontinuous sequence numbers always indicate lost frames.
>> @@ -1076,20 +1085,34 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
>> stream->sequence++;
>> if (stream->sequence)
>> uvc_video_stats_update(stream);
>> +
>> + /*
>> + * Mark the buffer as done if we're at the beginning of a new frame.
>> + * End of frame detection is better implemented by checking the EOF
>> + * bit (FID bit toggling is delayed by one frame compared to the EOF
>> + * bit), but some devices don't set the bit at end of frame (and the
>> + * last payload can be lost anyway). We thus must check if the FID has
>> + * been toggled.
>> + *
>> + * stream->last_fid is initialized to -1, so the first isochronous
>> + * frame will never trigger an end of frame detection.
>> + *
>> + * Empty buffers (bytesused == 0) don't trigger end of frame detection
>> + * as it doesn't make sense to return an empty buffer. This also
>> + * avoids detecting end of frame conditions at FID toggling if the
>> + * previous payload had the EOF bit set.
>> + */
>> + if (buf->bytesused) {
>> + uvc_dbg(stream->dev, FRAME,
>> + "Frame complete (FID bit toggled)\n");
>> + buf->state = UVC_BUF_STATE_READY;
>> + return -EAGAIN;
>> + }
>> }
>>
>> uvc_video_clock_decode(stream, buf, data, len);
>> uvc_video_stats_decode(stream, data, len);
>>
>> - /*
>> - * Store the payload FID bit and return immediately when the buffer is
>> - * NULL.
>> - */
>> - if (buf == NULL) {
>> - stream->last_fid = fid;
>> - return -ENODATA;
>> - }
>> -
>> /* Mark the buffer as bad if the error bit is set. */
>> if (data[1] & UVC_STREAM_ERR) {
>> uvc_dbg(stream->dev, FRAME,
>> @@ -1124,29 +1147,6 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
>> buf->state = UVC_BUF_STATE_ACTIVE;
>> }
>>
>> - /*
>> - * Mark the buffer as done if we're at the beginning of a new frame.
>> - * End of frame detection is better implemented by checking the EOF
>> - * bit (FID bit toggling is delayed by one frame compared to the EOF
>> - * bit), but some devices don't set the bit at end of frame (and the
>> - * last payload can be lost anyway). We thus must check if the FID has
>> - * been toggled.
>> - *
>> - * stream->last_fid is initialized to -1, so the first isochronous
>> - * frame will never trigger an end of frame detection.
>> - *
>> - * Empty buffers (bytesused == 0) don't trigger end of frame detection
>> - * as it doesn't make sense to return an empty buffer. This also
>> - * avoids detecting end of frame conditions at FID toggling if the
>> - * previous payload had the EOF bit set.
>> - */
>> - if (fid != stream->last_fid && buf->bytesused != 0) {
>> - uvc_dbg(stream->dev, FRAME,
>> - "Frame complete (FID bit toggled)\n");
>> - buf->state = UVC_BUF_STATE_READY;
>> - return -EAGAIN;
>> - }
>> -
>> stream->last_fid = fid;
>>
>> return data[0];
>>
>> ---
>> base-commit: e89fbb5bc21a10a0de2bb878d4df09f538dc523b
>> change-id: 20240221-uvc-host-video-decode-start-af53df5924cd
>>
>> Best regards,
>> --
>> Michael Grzeschik <m.grzeschik@...gutronix.de>
>>
>>
>
>
>--
>Ricardo Ribalda
>
--
Pengutronix e.K. | |
Steuerwalder Str. 21 | http://www.pengutronix.de/ |
31137 Hildesheim, Germany | Phone: +49-5121-206917-0 |
Amtsgericht Hildesheim, HRA 2686 | Fax: +49-5121-206917-5555 |
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists