Re: [PATCH 1/1] ov5648: Don't pack controls struct

[Paul Kocialkowski <paul.kocialkowski@bootlin.com>]

[-- Attachment #1: Type: text/plain, Size: 1836 bytes --]

Hi Sakari,

On Tue 11 Jan 22, 13:38, Sakari Ailus wrote:
> Hi Paul,
> 
> On Tue, Jan 11, 2022 at 09:28:12AM +0100, Paul Kocialkowski wrote:
> > Hi Sakari,
> > 
> > On Tue 11 Jan 22, 00:48, Sakari Ailus wrote:
> > > Don't pack the driver specific struct containing control pointers. This
> > > lead to potential alignment issues when working with the pointers.
> > 
> > Thanks for looking into the report and making this fix.
> > 
> > Honestly I was a bit puzzled because I explicitly added the __packed
> > to avoid possible holes in the structures that could be problematic
> > when using v4l2_ctrl_auto_cluster and I think the problem still stands.
> > 
> > I feel like solving both issues at once would require having the controls
> > that belong in the same cluster declared as an array and not individual
> > members of the struct.
> > 
> > What do you think?
> 
> No architecture used in Linux requires adding padding between two pointers
> to my knowledge --- generally the alignment is at most the size of the
> data: otherwise arrays would not work either. Therefore packing isn't
> required.

I was under the impression that padding may happen in structures generally
speaking. Are you saying that because it's pointers, there will most likely
be no padding required?

Also there's a struct v4l2_ctrl_handler at the end of the struct
(not a pointer), maybe that can somehow play a role too and introduce padding?

My feeling was that there's no strong guarantee here, so packing the struct
would be the safe thing to do. I also don't see how unaligned access can occur
in the packed struct in that case (pointers should always offset to something
properly aligned, shouldn't they?).

Paul

-- 
Paul Kocialkowski, Bootlin
Embedded Linux and kernel engineering
https://bootlin.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]