Re: [PATCH] media: uvcvideo: Explicit alignment of uvc_frame and uvc_format

[Laurent Pinchart]

Hi Ricardo,

On Fri, Mar 22, 2024 at 03:26:39PM +0100, Ricardo Ribalda wrote:
> On Fri, 22 Mar 2024 at 12:56, Laurent Pinchart wrote:
> > On Mon, May 01, 2023 at 04:49:31PM +0200, Ricardo Ribalda wrote:
> > > Struct uvc_frame and uvc_format are packaged together on
> > > streaming->formats on a sigle allocation.
> >
> > s/sigle/single/
> >
> > > This is working fine because both structures have a field with a
> > > pointer, but it will stop working when the sizeof() of any of those
> > > structs is not a muliple of the sizeof(void*).
> > >
> > > Make that aligment contract explicit.
> > >
> > > Signed-off-by: Ricardo Ribalda <ribalda@xxxxxxxxxxxx>
> > > ---
> > > This is better than 3 allocations, and do not have any performance
> > > penalty.
> > > ---
> > >  drivers/media/usb/uvc/uvcvideo.h | 4 ++--
> > >  1 file changed, 2 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/drivers/media/usb/uvc/uvcvideo.h b/drivers/media/usb/uvc/uvcvideo.h
> > > index 9a596c8d894a..03e8a543c8e6 100644
> > > --- a/drivers/media/usb/uvc/uvcvideo.h
> > > +++ b/drivers/media/usb/uvc/uvcvideo.h
> > > @@ -252,7 +252,7 @@ struct uvc_frame {
> > >       u8  bFrameIntervalType;
> > >       u32 dwDefaultFrameInterval;
> > >       u32 *dwFrameInterval;
> > > -};
> > > +} __aligned(sizeof(void *)); /* uvc_frame is packed on streaming->formats. */
> >
> > Don't we need u32 alignment here, not void * alignment, given that
> > uvc_frame is followed by an array of u32 ?
> 
> Let me make sure that I explain myself :)
> 
> I made a small program in compiler explorer:
> https://godbolt.org/z/7s9z8WTsx that shows the error that I want to
> avoid
> 
> When we have a structure like this:
> 
> struct n_foo_bar {
>    int n;
>    struct foo *foo;
>    struct bar *bar;
> };
> 
> We expect that *foo and *bar point to memory addresses with the right
> cpu alignment for each struct. Otherwise accessing foo and bar could
> be slow or simply not work.

So far, so good.

> In the driver we are doing something like this to allocate the structure:
> 
> int size
> struct n_foo_bar *out;
> 
> size = n*sizeof(struct foo)+n*sizeof(struct bar) +sizeof(struct n_foo_bar);
> out = malloc(size);
> if (!out)
>   return out;
> 
> out->foo=(void *)(out)+sizeof(struct n_foo_bar);
> out->bar=(void *)(out->foo)+n*sizeof(struct foo);
> 
> But that only works if sizeof(struct foo) is a multiple of the
> alignment required by struct bar.

The real requirement is a bit more complex, it's sizeof(struct n_foo_bar) +
sizeof(struct foo) that needs to be a multiple of the alignment required
by struct bar (and even that is simplified, as it assumes that malloc()
returns a pointer aligned to the requirements of struct bar, which in
practice should always be the case).

> We are "lucky" now because we have a
> pointer in each struct and that gives us a void* padding. ... but if
> we ever remove that pointer from the structure we will be in a bad
> position.

We have three levels in uvcvideo. The top-level structure (your
equivalent of n_foo_bar), struct uvc_format, has a pointer to an array
of struct uvc_frame. The second level, struct uvc_frame, has a pointer
to an array of u32. All three are then allocated in one go,
contiguously.

The largest field in uvc_frame is a pointer, so the alignment
requirement will be fulfilled if struct uvc_format is aligned to
sizeof(void *). When it comes to struct uvc_frame, however, its size
needs to be a multiple of sizeof(u32), not of sizeof(void *).

Given that the alignment constraints are not intrinsic to these
structures, I think it would be better to handle them when allocating
the memory. Something along the line of

diff --git a/drivers/media/usb/uvc/uvc_driver.c b/drivers/media/usb/uvc/uvc_driver.c
index f33a01dbb329..cbc40d663e4f 100644
--- a/drivers/media/usb/uvc/uvc_driver.c
+++ b/drivers/media/usb/uvc/uvc_driver.c
@@ -687,8 +687,11 @@ static int uvc_parse_streaming(struct uvc_device *dev,
 		goto error;
 	}

-	size = nformats * sizeof(*format) + nframes * sizeof(*frame)
+	size = nformats * sizeof(*format);
+	size = ALIGN(size, __alignof__(*frame)) + nframes * sizeof(*frame);
+	size = ALIGN(size, __alignof__(*interval))
 	     + nintervals * sizeof(*interval);
+
 	format = kzalloc(size, GFP_KERNEL);
 	if (format == NULL) {
 		ret = -ENOMEM;

plus a corresponding change when calculating the pointers to the frames
and intervals just after.

> With the  __aligned(sizeof(void *)); I want to explicitly say:
> 
> "Ey, this struct is embedded in another struct and they are allocated
> contiguously"
> 
> Does it make more sense now?
>
> > >
> > >  struct uvc_format {
> > >       u8 type;
> > > @@ -266,7 +266,7 @@ struct uvc_format {
> > >
> > >       unsigned int nframes;
> > >       struct uvc_frame *frame;
> > > -};
> > > +} __aligned(sizeof(void *)); /* uvc_format is packed on streaming->formats. */
> >
> > Same here, technically we need to ensure that the following uvc_frame
> > will be aligned. void * alignment will give us that now, but that's not
> > the actual constraint.
> >
> > Wouldn't it be better to handle the alignment constraints explicitly
> > when allocating the memory ? It's not that uvc_frame and uvc_format have
> > intrinsic alignment constraints, the constraints are only needed because
> > of the way memory is allocated.
> >
> > >
> > >  struct uvc_streaming_header {
> > >       u8 bNumFormats;
> > >
> > > ---
> > > base-commit: 58390c8ce1bddb6c623f62e7ed36383e7fa5c02f
> > > change-id: 20230501-uvc-align-6ff202b68dab

-- 
Regards,

Laurent Pinchart