[PATCH 0/3] IPU3 ImgU driver parameter struct fixes

[PATCH 0/3] IPU3 ImgU driver parameter struct fixes

[Sakari Ailus]

Hi all,

This set addresses parameter struct memory layout changes introduced by
seemingly innocent patches, and adds a sanity check for the struct size to
avoid this from happening again. This only touches the uAPI, not the
ipu3-abi.h that deals with ABI not visible in the driver uAPI. That should
be addressed as well going forward.

The first patch does not probably entirely fix the issues, but with the
second patch added, pahole agrees the struct memory layout is unchanged
from the previous state.

Sakari Ailus (3):
  Revert "staging: imgu: Address a compiler warning on alignment"
  staging: ipu3-imgu: Move alignment attribute to field
  staging: ipu3-imgu: Add a sanity check for the parameter struct size

 drivers/staging/media/ipu3/include/intel-ipu3.h | 7 ++++---
 drivers/staging/media/ipu3/ipu3-css.c           | 7 +++++++
 2 files changed, 11 insertions(+), 3 deletions(-)

-- 
2.20.1

[PATCH 1/3] Revert "staging: imgu: Address a compiler warning on alignment"

[Sakari Ailus]

This reverts commit ef4104f3d81d8f0d897d7f0cd5d01b006c3caf80.

The patch being reverted changed the memory layout of struct
ipu3_uapi_acc_param. Revert it, and address the compiler warning issues in
further patches.

Reported-by: Tomasz Figa <tfiga@chromium.org>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/staging/media/ipu3/include/intel-ipu3.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
index 1c9c3ba4d518d..5f43f631cf62f 100644
--- a/drivers/staging/media/ipu3/include/intel-ipu3.h
+++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
@@ -2477,7 +2477,7 @@ struct ipu3_uapi_acc_param {
 	struct ipu3_uapi_yuvp1_yds_config yds2 __attribute__((aligned(32)));
 	struct ipu3_uapi_yuvp2_tcc_static_config tcc __attribute__((aligned(32)));
 	struct ipu3_uapi_anr_config anr;
-	struct ipu3_uapi_awb_fr_config_s awb_fr __attribute__((aligned(32)));
+	struct ipu3_uapi_awb_fr_config_s awb_fr;
 	struct ipu3_uapi_ae_config ae;
 	struct ipu3_uapi_af_config_s af;
 	struct ipu3_uapi_awb_config awb;
-- 
2.20.1

[PATCH v1.1 1/3] Revert "staging: imgu: Address a compiler warning on alignment"

[Sakari Ailus]

This reverts commit c9d52c114a9fcc61c30512c7f810247a9f2812af.

The patch being reverted changed the memory layout of struct
ipu3_uapi_acc_param. Revert it, and address the compiler warning issues in
further patches.

Reported-by: Tomasz Figa <tfiga@chromium.org>
Tested-by: Tested-by: Bingbu Cao <bingbu.cao@intel.com>
Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
since v1:

- Fixed upstream commit id.

 drivers/staging/media/ipu3/include/intel-ipu3.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
index 1c9c3ba4d518..5f43f631cf62 100644
--- a/drivers/staging/media/ipu3/include/intel-ipu3.h
+++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
@@ -2477,7 +2477,7 @@ struct ipu3_uapi_acc_param {
 	struct ipu3_uapi_yuvp1_yds_config yds2 __attribute__((aligned(32)));
 	struct ipu3_uapi_yuvp2_tcc_static_config tcc __attribute__((aligned(32)));
 	struct ipu3_uapi_anr_config anr;
-	struct ipu3_uapi_awb_fr_config_s awb_fr __attribute__((aligned(32)));
+	struct ipu3_uapi_awb_fr_config_s awb_fr;
 	struct ipu3_uapi_ae_config ae;
 	struct ipu3_uapi_af_config_s af;
 	struct ipu3_uapi_awb_config awb;
-- 
2.20.1

[PATCH 2/3] staging: ipu3-imgu: Move alignment attribute to field

[Sakari Ailus]

Move the alignment attribute of struct ipu3_uapi_awb_fr_config_s to the
field in struct ipu3_uapi_4a_config, the other location where the struct
is used.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/staging/media/ipu3/include/intel-ipu3.h | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/ipu3/include/intel-ipu3.h b/drivers/staging/media/ipu3/include/intel-ipu3.h
index 5f43f631cf62f..a607b0158c81d 100644
--- a/drivers/staging/media/ipu3/include/intel-ipu3.h
+++ b/drivers/staging/media/ipu3/include/intel-ipu3.h
@@ -450,7 +450,7 @@ struct ipu3_uapi_awb_fr_config_s {
 	__u32 bayer_sign;
 	__u8 bayer_nf;
 	__u8 reserved2[7];
-} __attribute__((aligned(32))) __packed;
+} __packed;
 
 /**
  * struct ipu3_uapi_4a_config - 4A config
@@ -466,7 +466,8 @@ struct ipu3_uapi_4a_config {
 	struct ipu3_uapi_ae_grid_config ae_grd_config;
 	__u8 padding[20];
 	struct ipu3_uapi_af_config_s af_config;
-	struct ipu3_uapi_awb_fr_config_s awb_fr_config;
+	struct ipu3_uapi_awb_fr_config_s awb_fr_config
+		__attribute__((aligned(32)));
 } __packed;
 
 /**
-- 
2.20.1

[PATCH 3/3] staging: ipu3-imgu: Add a sanity check for the parameter struct size

[Sakari Ailus]

There have been cases where seemingly innocuous patches have broken the
uAPI by changing the memory layout of the parameter struct. Generally such
changes also introduce a change in the size of the entire struct. This
patch adds a sanity check to avoid such cases happening in the future.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/staging/media/ipu3/ipu3-css.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/staging/media/ipu3/ipu3-css.c b/drivers/staging/media/ipu3/ipu3-css.c
index 4f04fe838b0c0..3c700ae9c94e9 100644
--- a/drivers/staging/media/ipu3/ipu3-css.c
+++ b/drivers/staging/media/ipu3/ipu3-css.c
@@ -1911,6 +1911,13 @@ int imgu_css_meta_fmt_set(struct v4l2_meta_format *fmt)
 	switch (fmt->dataformat) {
 	case V4L2_META_FMT_IPU3_PARAMS:
 		fmt->buffersize = sizeof(struct ipu3_uapi_params);
+
+		/*
+		 * Sanity check for the parameter struct size. This must
+		 * not change!
+		 */
+		BUILD_BUG_ON(sizeof(struct ipu3_uapi_params) != 39328);
+
 		break;
 	case V4L2_META_FMT_IPU3_STAT_3A:
 		fmt->buffersize = sizeof(struct ipu3_uapi_stats_3a);
-- 
2.20.1

Re: [PATCH 0/3] IPU3 ImgU driver parameter struct fixes

[Bingbu Cao]

Tested-by: Bingbu Cao <bingbu.cao@intel.com>

On 4/16/20 5:18 PM, Sakari Ailus wrote:
> Hi all,
> 
> This set addresses parameter struct memory layout changes introduced by
> seemingly innocent patches, and adds a sanity check for the struct size to
> avoid this from happening again. This only touches the uAPI, not the
> ipu3-abi.h that deals with ABI not visible in the driver uAPI. That should
> be addressed as well going forward.
> 
> The first patch does not probably entirely fix the issues, but with the
> second patch added, pahole agrees the struct memory layout is unchanged
> from the previous state.
> 
> Sakari Ailus (3):
>   Revert "staging: imgu: Address a compiler warning on alignment"
>   staging: ipu3-imgu: Move alignment attribute to field
>   staging: ipu3-imgu: Add a sanity check for the parameter struct size
> 
>  drivers/staging/media/ipu3/include/intel-ipu3.h | 7 ++++---
>  drivers/staging/media/ipu3/ipu3-css.c           | 7 +++++++
>  2 files changed, 11 insertions(+), 3 deletions(-)
> 

-- 
Best regards,
Bingbu Cao

Re: [PATCH 0/3] IPU3 ImgU driver parameter struct fixes

[Sakari Ailus]

On Sun, Apr 26, 2020 at 10:42:52AM +0800, Bingbu Cao wrote:
> Tested-by: Bingbu Cao <bingbu.cao@intel.com>

Thanks, Bingbu!

-- 
Sakari Ailus