Re: [PATCH v2 5/7] media: uvcvideo: Increment intervals pointer at end of parsing

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

Hi Ricardo,

On Mon, May 15, 2023 at 03:22:13PM +0200, Ricardo Ribalda wrote:
> On Sat, 6 May 2023 at 09:14, Laurent Pinchart wrote:
> >
> > The intervals pointer is incremented for each interval when parsing the
> > format descriptor. This doesn't cause any issue as such, but gets in the
> > way of constifying some pointers. Modify the parsing code to index the
> > intervals pointer as an array and increment it in one go at end of
> > parsing.
> >
> > Careful readers will notice that the maxIntervalIndex variable is set to
> > 1 instead of n - 2 when bFrameIntervalType has a zero value. This is
> > functionally equivalent, as n is equal to 3 in that case.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
>
> Reviewed-by: Ricardo Ribalda <ribalda@chromium.org>
>
> > ---
> >  drivers/media/usb/uvc/uvc_driver.c | 10 +++++++---
> >  1 file changed, 7 insertions(+), 3 deletions(-)
> >
> > diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
> > index 446bd8ff128c..11e4fa007f3f 100644
> > --- a/drivers/media/usb/uvc/uvc_driver.c
> > +++ b/drivers/media/usb/uvc/uvc_driver.c
> > @@ -370,6 +370,8 @@ static int uvc_parse_format(struct uvc_device *dev,
> >          */
> >         while (buflen > 2 && buffer[1] == USB_DT_CS_INTERFACE &&
> >                buffer[2] == ftype) {
> > +               unsigned int maxIntervalIndex;
> > +
> >                 frame = &format->frames[format->nframes];
> >                 if (ftype != UVC_VS_FRAME_FRAME_BASED)
> >                         n = buflen > 25 ? buffer[25] : 0;
> > @@ -418,7 +420,7 @@ static int uvc_parse_format(struct uvc_device *dev,
> >
> >                 for (i = 0; i < n; ++i) {
> >                         interval = get_unaligned_le32(&buffer[26+4*i]);
> > -                       *(*intervals)++ = interval ? interval : 1;
> > +                       (*intervals)[i] = interval ? interval : 1;
> >                 }
> >
> >                 /*
> > @@ -440,11 +442,11 @@ static int uvc_parse_format(struct uvc_device *dev,
> >                                 * frame->wWidth * frame->wHeight / 8;
> >
> >                 /* Clamp the default frame interval to the boundaries. */
> > -               n -= frame->bFrameIntervalType ? 1 : 2;
> > +               maxIntervalIndex = frame->bFrameIntervalType ? n - 1 : 1;
> 
> Maybe it is worth mentioning that bFrameIntervalType == 0 is a
> continuous interval. idex 0 is min and 1 is max.

I'll update the comment to

                /*
                 * Clamp the default frame interval to the boundaries. A zero
                 * bFrameIntervalType value indicates a continuous frame
                 * interval range, with dwFrameInterval[0] storing the minimum
                 * value and dwFrameInterval[1] storing the maximum value.
                 */

> >                 frame->dwDefaultFrameInterval =
> >                         clamp(frame->dwDefaultFrameInterval,
> >                               frame->dwFrameInterval[0],
> > -                             frame->dwFrameInterval[n]);
> > +                             frame->dwFrameInterval[maxIntervalIndex]);
> >
> >                 /*
> >                  * Some devices report frame intervals that are not functional.
> > @@ -463,6 +465,8 @@ static int uvc_parse_format(struct uvc_device *dev,
> >                         (100000000 / frame->dwDefaultFrameInterval) % 10);
> >
> >                 format->nframes++;
> > +               *intervals += n;
> > +
> >                 buflen -= buffer[0];
> >                 buffer += buffer[0];
> >         }

-- 
Regards,

Laurent Pinchart