| Message ID | 20220110224831.266749-1-sakari.ailus@linux.intel.com |
|---|---|
| State | Accepted |
| Commit | edd4fbff5378a8103470304809195dc8f4b1d42a |
| Headers | show |
| Series | [1/1] ov5648: Don't pack controls struct | expand |
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.
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
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.
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;
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(-)