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

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

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

Hi Sakari,

On Wed 12 Jan 22, 13:21, Sakari Ailus wrote:
> Hi Paul,
> 
> On Wed, Jan 12, 2022 at 12:09:46PM +0100, Paul Kocialkowski wrote:
> > 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?
> 
> Not really just pointers; the same goes for any data type.
> 
> > 
> > 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?
> 
> There could be padding added at the end of the struct. (But that depends on
> what comes after the struct.)
> 
> > 
> > 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?).
> 
> My understanding is this is a false positive warning from clang. Gcc does
> not complain but I'm not sure it's capable of doing that either.
> 
> Of course it would be the best to fix clang but until that happens or we
> change the code, we'll be permanent targets of these e-mails.
> 
> Still __packed isn't needed here.

Okay understood, thanks!

So all in all I was under the impression that padding can be added by the
compiler "as it likes" but it seems that it will only happen for alignment
reasons. However here no padding should be required between pointers to maintain
alignment, so we're good. Is that correct?

If that's right then feel free to add:
Reviewed-by: Paul Kocialkowski <paul.kocialkowski@bootlin.com>

Thanks,

Paul


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

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