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

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

[Sakari Ailus]

Don't pack the driver specific struct containing control pointers. This
lead to potential alignment issues when working with the pointers.

Reported-by: kernel test robot <lkp@intel.com>
Fixes: e43ccb0a045f ("media: i2c: Add support for the OV5648 image sensor")
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/ov5648.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/i2c/ov5648.c b/drivers/media/i2c/ov5648.c
index 87f9b724cd7f..3478650ee732 100644
--- a/drivers/media/i2c/ov5648.c
+++ b/drivers/media/i2c/ov5648.c
@@ -639,7 +639,7 @@ struct ov5648_ctrls {
 	struct v4l2_ctrl *pixel_rate;
 
 	struct v4l2_ctrl_handler handler;
-} __packed;
+};
 
 struct ov5648_sensor {
 	struct device *dev;
-- 
2.30.2

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

[Paul Kocialkowski]

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

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?

Cheers,

Paul

> Reported-by: kernel test robot <lkp@intel.com>
> Fixes: e43ccb0a045f ("media: i2c: Add support for the OV5648 image sensor")
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/ov5648.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/media/i2c/ov5648.c b/drivers/media/i2c/ov5648.c
> index 87f9b724cd7f..3478650ee732 100644
> --- a/drivers/media/i2c/ov5648.c
> +++ b/drivers/media/i2c/ov5648.c
> @@ -639,7 +639,7 @@ struct ov5648_ctrls {
>  	struct v4l2_ctrl *pixel_rate;
>  
>  	struct v4l2_ctrl_handler handler;
> -} __packed;
> +};
>  
>  struct ov5648_sensor {
>  	struct device *dev;
> -- 
> 2.30.2
> 

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

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

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

[Sakari Ailus]

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.

-- 
Kind regards,

Sakari Ailus

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

[Paul Kocialkowski]

[-- 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 --]

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

[Sakari Ailus]

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.

-- 
Kind regards,

Sakari Ailus

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

[Paul Kocialkowski]

[-- 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 --]