Re: [PATCH v9 1/6] media: uvcvideo: Fix negative modulus calculation

[Laurent Pinchart]

Hi Ricardo,

On Mon, Feb 19, 2024 at 04:07:12PM +0100, Ricardo Ribalda wrote:
> On Mon, 19 Feb 2024 at 11:40, Laurent Pinchart wrote:
> > On Mon, Feb 19, 2024 at 11:28:03AM +0100, Ricardo Ribalda wrote:
> > > On Mon, 12 Feb 2024 at 23:59, Laurent Pinchart wrote:
> > > > On Wed, Mar 15, 2023 at 02:30:12PM +0100, Ricardo Ribalda wrote:
> > > > > If head is 0, last will be addressing the index 0 instead of clock->size
> > > > > -1. Luckily clock->head is unsiged, otherwise it would be addressing
> > > > > 0xffffffff.
> > > >
> > > > I'm not following you. In the expression
> > > >
> > > >         (clock->head - 1) % clock->size
> > > >
> > > > clock->head is an unsigned int, and 1 as a signed int, so the result of
> > > > the subtraction is promoted to an unsigned int. When clock->head is 0, the expression evaluates to
> > > >
> > > >         0xffffffff % clock->size
> > > >
> > > > clock->size is a power of two (hardcoded to 32 at the moment), so the
> > > > expression evaluates to 31, as intended.
> > > >
> > > > Am I missing something ?
> > >
> > > Take a look to: https://godbolt.org/z/xYeqTx6ba
> > >
> > > The expression only works because the size is a power of two. In this
> > > set I am allowing sizes that are not powers of two.
> >
> > Could you then update the commit message to explain that ?
> >
> > I'll review the rest of the series this week.
> Thanks
> 
> Will update with the following text after the review:
> 
> The tail of the list lives at the position before the head. This is
> mathematically noted as
> ```
> (head-1)  mod size.
> ```
> 
> Unfortunately C, does not have a modulus operator, but a remainder
> operator (%).
> The reminder operation has a different result than the modulus if
> (head -1) is a negative number and size is not a power of two.
> 
> Adding size to (head-1) allows the code to run with any value of size.

Could you please add

This does not change the current behaviour of the driver, as the size is
always a power of two, but prepares for reworks that will change the
size to a non power of two.

or something similar ?

> > > > > Nontheless, this is not the intented behaviour and should be fixed.
> > > > >
> > > > > Fixes: 66847ef013cc ("[media] uvcvideo: Add UVC timestamps support")

I think this should be dropped, the patch doesn't fix an issue, but
prepares for further changes that add new features. I'd also like to
update the commit message to avoid stating "Fix", to avoid this being
picked for stable kernels automatically.

> > > > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > > > > ---
> > > > >  drivers/media/usb/uvc/uvc_video.c | 2 +-
> > > > >  1 file changed, 1 insertion(+), 1 deletion(-)
> > > > >
> > > > > diff --git a/drivers/media/usb/uvc/uvc_video.c b/drivers/media/usb/uvc/uvc_video.c
> > > > > index d4b023d4de7c..4ff4ab4471fe 100644
> > > > > --- a/drivers/media/usb/uvc/uvc_video.c
> > > > > +++ b/drivers/media/usb/uvc/uvc_video.c
> > > > > @@ -732,7 +732,7 @@ void uvc_video_clock_update(struct uvc_streaming *stream,
> > > > >               goto done;
> > > > >
> > > > >       first = &clock->samples[clock->head];
> > > > > -     last = &clock->samples[(clock->head - 1) % clock->size];
> > > > > +     last = &clock->samples[(clock->head - 1 + clock->size) % clock->size];
> > > > >
> > > > >       /* First step, PTS to SOF conversion. */
> > > > >       delta_stc = buf->pts - (1UL << 31);

-- 
Regards,

Laurent Pinchart