linux-kernel - Re: [PATCH v3] uvc_video: move clock_decode and stats_decode to the end of decode

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

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20240616234712.GL4782@pendragon.ideasonboard.com>
Date: Mon, 17 Jun 2024 02:47:12 +0300
From: Laurent Pinchart <laurent.pinchart@...asonboard.com>
To: Ricardo Ribalda <ribalda@...omium.org>
Cc: Michael Grzeschik <m.grzeschik@...gutronix.de>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	linux-media@...r.kernel.org, linux-kernel@...r.kernel.org,
	kernel@...gutronix.de
Subject: Re: [PATCH v3] uvc_video: move clock_decode and stats_decode to the
 end of decode_start

On Mon, Mar 18, 2024 at 10:43:16AM +0100, Ricardo Ribalda wrote:
> On Mon, 18 Mar 2024 at 10:28, Michael Grzeschik 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 moving the decode functions to the
> > end of decode_start, when it is save to really parse the data.
>
> nit: safe
>
> > Signed-off-by: Michael Grzeschik <m.grzeschik@...gutronix.de>
>
> Reviewed-by: Ricardo Ribalda <ribalda@...omium.org>
>
> > ---
> > Changes in v3:
> > - Moved the clock_decode and stat_decode to end of function as suggested by Ricardo Ribalda
> > - Link to v2: https://lore.kernel.org/r/20240221-uvc-host-video-decode-start-v2-1-88c6e17e487a@pengutronix.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 | 6 +++---
> >  1 file changed, 3 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > index 7cbf4692bd875..7471bff0ca894 100644
> > --- a/drivers/media/usb/uvc/uvc_video.c
> > +++ b/drivers/media/usb/uvc/uvc_video.c
> > @@ -1078,9 +1078,6 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
> >                         uvc_video_stats_update(stream);
> >         }
> >
> > -       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.
> > @@ -1147,6 +1144,9 @@ static int uvc_video_decode_start(struct uvc_streaming *stream,
> >                 return -EAGAIN;
> >         }
> >
> > +       uvc_video_clock_decode(stream, buf, data, len);
> > +       uvc_video_stats_decode(stream, data, len);

uvc_video_stats_decode() needs to be called before
uvc_video_stats_update(), so I don't think this is right.

> > +
> >         stream->last_fid = fid;
> >
> >         return data[0];
> >
> > ---
> > base-commit: d99e42ce6b8341d3f09e22c6706461ec900fe172
> > change-id: 20240221-uvc-host-video-decode-start-af53df5924cd

-- 
Regards,

Laurent Pinchart