All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
@ 2020-07-30 12:52 ` Herbert Xu
  0 siblings, 0 replies; 15+ messages in thread
From: Herbert Xu @ 2020-07-30 12:52 UTC (permalink / raw)
  To: Li Yang, linuxppc-dev, linux-arm-kernel

There are two __packed attributes in qman.h that are both unnecessary
and causing compiler warnings because they're conflicting with
explicit alignment requirements set on members within the structure.

This patch removes them both.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/include/soc/fsl/qman.h b/include/soc/fsl/qman.h
index cfe00e08e85b..d81ff185dc0b 100644
--- a/include/soc/fsl/qman.h
+++ b/include/soc/fsl/qman.h
@@ -256,7 +256,7 @@ struct qm_dqrr_entry {
 	__be32 context_b;
 	struct qm_fd fd;
 	u8 __reserved4[32];
-} __packed;
+};
 #define QM_DQRR_VERB_VBIT		0x80
 #define QM_DQRR_VERB_MASK		0x7f	/* where the verb contains; */
 #define QM_DQRR_VERB_FRAME_DEQUEUE	0x60	/* "this format" */
@@ -289,7 +289,7 @@ union qm_mr_entry {
 		__be32 tag;
 		struct qm_fd fd;
 		u8 __reserved1[32];
-	} __packed ern;
+	} ern;
 	struct {
 		u8 verb;
 		u8 fqs;		/* Frame Queue Status */
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
@ 2020-07-30 12:52 ` Herbert Xu
  0 siblings, 0 replies; 15+ messages in thread
From: Herbert Xu @ 2020-07-30 12:52 UTC (permalink / raw)
  To: Li Yang, linuxppc-dev, linux-arm-kernel

There are two __packed attributes in qman.h that are both unnecessary
and causing compiler warnings because they're conflicting with
explicit alignment requirements set on members within the structure.

This patch removes them both.

Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/include/soc/fsl/qman.h b/include/soc/fsl/qman.h
index cfe00e08e85b..d81ff185dc0b 100644
--- a/include/soc/fsl/qman.h
+++ b/include/soc/fsl/qman.h
@@ -256,7 +256,7 @@ struct qm_dqrr_entry {
 	__be32 context_b;
 	struct qm_fd fd;
 	u8 __reserved4[32];
-} __packed;
+};
 #define QM_DQRR_VERB_VBIT		0x80
 #define QM_DQRR_VERB_MASK		0x7f	/* where the verb contains; */
 #define QM_DQRR_VERB_FRAME_DEQUEUE	0x60	/* "this format" */
@@ -289,7 +289,7 @@ union qm_mr_entry {
 		__be32 tag;
 		struct qm_fd fd;
 		u8 __reserved1[32];
-	} __packed ern;
+	} ern;
 	struct {
 		u8 verb;
 		u8 fqs;		/* Frame Queue Status */
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
  2020-07-30 12:52 ` Herbert Xu
@ 2020-09-01  0:33   ` Herbert Xu
  -1 siblings, 0 replies; 15+ messages in thread
From: Herbert Xu @ 2020-09-01  0:33 UTC (permalink / raw)
  To: Li Yang, linuxppc-dev, linux-arm-kernel

On Thu, Jul 30, 2020 at 10:52:59PM +1000, Herbert Xu wrote:
> There are two __packed attributes in qman.h that are both unnecessary
> and causing compiler warnings because they're conflicting with
> explicit alignment requirements set on members within the structure.
> 
> This patch removes them both.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Ping.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
@ 2020-09-01  0:33   ` Herbert Xu
  0 siblings, 0 replies; 15+ messages in thread
From: Herbert Xu @ 2020-09-01  0:33 UTC (permalink / raw)
  To: Li Yang, linuxppc-dev, linux-arm-kernel

On Thu, Jul 30, 2020 at 10:52:59PM +1000, Herbert Xu wrote:
> There are two __packed attributes in qman.h that are both unnecessary
> and causing compiler warnings because they're conflicting with
> explicit alignment requirements set on members within the structure.
> 
> This patch removes them both.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

Ping.
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
  2020-07-30 12:52 ` Herbert Xu
@ 2020-09-01  1:50   ` Leo Li
  -1 siblings, 0 replies; 15+ messages in thread
From: Leo Li @ 2020-09-01  1:50 UTC (permalink / raw)
  To: Herbert Xu, linuxppc-dev, linux-arm-kernel



> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: Thursday, July 30, 2020 7:53 AM
> To: Leo Li <leoyang.li@nxp.com>; linuxppc-dev@lists.ozlabs.org; linux-arm-
> kernel@lists.infradead.org
> Subject: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
> 
> There are two __packed attributes in qman.h that are both unnecessary
> and causing compiler warnings because they're conflicting with
> explicit alignment requirements set on members within the structure.

Sorry for the late response.  I missed this email previously.

These structures are descriptors used by hardware, we cannot have _ANY_ padding from the compiler.  The compiled result might be the same with or without the __packed attribute for now, but I think keep it there probably is safer for dealing with unexpected alignment requirements from the compiler in the future.

Having conflicting alignment requirements warning might means something is wrong with the structure in certain scenario.  I just tried a ARM64 build but didn't see the warnings.  Could you share the warning you got and the build setup?  Thanks.

Regards,
Leo 
> 
> This patch removes them both.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/include/soc/fsl/qman.h b/include/soc/fsl/qman.h
> index cfe00e08e85b..d81ff185dc0b 100644
> --- a/include/soc/fsl/qman.h
> +++ b/include/soc/fsl/qman.h
> @@ -256,7 +256,7 @@ struct qm_dqrr_entry {
>  	__be32 context_b;
>  	struct qm_fd fd;
>  	u8 __reserved4[32];
> -} __packed;
> +};
>  #define QM_DQRR_VERB_VBIT		0x80
>  #define QM_DQRR_VERB_MASK		0x7f	/* where the verb
> contains; */
>  #define QM_DQRR_VERB_FRAME_DEQUEUE	0x60	/* "this format" */
> @@ -289,7 +289,7 @@ union qm_mr_entry {
>  		__be32 tag;
>  		struct qm_fd fd;
>  		u8 __reserved1[32];
> -	} __packed ern;
> +	} ern;
>  	struct {
>  		u8 verb;
>  		u8 fqs;		/* Frame Queue Status */
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page:
> https://eur01.safelinks.protection.outlook.com/?url=http:%2F%2Fgondor.ap
> ana.org.au%2F~herbert%2F&amp;data=02%7C01%7Cleoyang.li%40nxp.com
> %7Cb69aca8dc53a4030b14b08d8348783a9%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C637317103931120730&amp;sdata=g3%2FJfa%2FNcuhLD5
> SYhbmhno65O1bxisVt2xltu2IMPjQ%3D&amp;reserved=0
> PGP Key:
> https://eur01.safelinks.protection.outlook.com/?url=http:%2F%2Fgondor.ap
> ana.org.au%2F~herbert%2Fpubkey.txt&amp;data=02%7C01%7Cleoyang.li%4
> 0nxp.com%7Cb69aca8dc53a4030b14b08d8348783a9%7C686ea1d3bc2b4c6fa9
> 2cd99c5c301635%7C0%7C0%7C637317103931120730&amp;sdata=uSS2C4cuAL
> XcCgIhpIORK4EZ1BHHj%2BqAW2Pu%2FLrFKPM%3D&amp;reserved=0

^ permalink raw reply	[flat|nested] 15+ messages in thread

* RE: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
@ 2020-09-01  1:50   ` Leo Li
  0 siblings, 0 replies; 15+ messages in thread
From: Leo Li @ 2020-09-01  1:50 UTC (permalink / raw)
  To: Herbert Xu, linuxppc-dev, linux-arm-kernel



> -----Original Message-----
> From: Herbert Xu <herbert@gondor.apana.org.au>
> Sent: Thursday, July 30, 2020 7:53 AM
> To: Leo Li <leoyang.li@nxp.com>; linuxppc-dev@lists.ozlabs.org; linux-arm-
> kernel@lists.infradead.org
> Subject: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
> 
> There are two __packed attributes in qman.h that are both unnecessary
> and causing compiler warnings because they're conflicting with
> explicit alignment requirements set on members within the structure.

Sorry for the late response.  I missed this email previously.

These structures are descriptors used by hardware, we cannot have _ANY_ padding from the compiler.  The compiled result might be the same with or without the __packed attribute for now, but I think keep it there probably is safer for dealing with unexpected alignment requirements from the compiler in the future.

Having conflicting alignment requirements warning might means something is wrong with the structure in certain scenario.  I just tried a ARM64 build but didn't see the warnings.  Could you share the warning you got and the build setup?  Thanks.

Regards,
Leo 
> 
> This patch removes them both.
> 
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/include/soc/fsl/qman.h b/include/soc/fsl/qman.h
> index cfe00e08e85b..d81ff185dc0b 100644
> --- a/include/soc/fsl/qman.h
> +++ b/include/soc/fsl/qman.h
> @@ -256,7 +256,7 @@ struct qm_dqrr_entry {
>  	__be32 context_b;
>  	struct qm_fd fd;
>  	u8 __reserved4[32];
> -} __packed;
> +};
>  #define QM_DQRR_VERB_VBIT		0x80
>  #define QM_DQRR_VERB_MASK		0x7f	/* where the verb
> contains; */
>  #define QM_DQRR_VERB_FRAME_DEQUEUE	0x60	/* "this format" */
> @@ -289,7 +289,7 @@ union qm_mr_entry {
>  		__be32 tag;
>  		struct qm_fd fd;
>  		u8 __reserved1[32];
> -	} __packed ern;
> +	} ern;
>  	struct {
>  		u8 verb;
>  		u8 fqs;		/* Frame Queue Status */
> --
> Email: Herbert Xu <herbert@gondor.apana.org.au>
> Home Page:
> https://eur01.safelinks.protection.outlook.com/?url=http:%2F%2Fgondor.ap
> ana.org.au%2F~herbert%2F&amp;data=02%7C01%7Cleoyang.li%40nxp.com
> %7Cb69aca8dc53a4030b14b08d8348783a9%7C686ea1d3bc2b4c6fa92cd99c5c3
> 01635%7C0%7C0%7C637317103931120730&amp;sdata=g3%2FJfa%2FNcuhLD5
> SYhbmhno65O1bxisVt2xltu2IMPjQ%3D&amp;reserved=0
> PGP Key:
> https://eur01.safelinks.protection.outlook.com/?url=http:%2F%2Fgondor.ap
> ana.org.au%2F~herbert%2Fpubkey.txt&amp;data=02%7C01%7Cleoyang.li%4
> 0nxp.com%7Cb69aca8dc53a4030b14b08d8348783a9%7C686ea1d3bc2b4c6fa9
> 2cd99c5c301635%7C0%7C0%7C637317103931120730&amp;sdata=uSS2C4cuAL
> XcCgIhpIORK4EZ1BHHj%2BqAW2Pu%2FLrFKPM%3D&amp;reserved=0

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
  2020-09-01  1:50   ` Leo Li
  (?)
@ 2020-09-01  1:56     ` Herbert Xu
  -1 siblings, 0 replies; 15+ messages in thread
From: Herbert Xu @ 2020-09-01  1:56 UTC (permalink / raw)
  To: Leo Li; +Cc: linuxppc-dev, linux-arm-kernel, Linux Kernel Mailing List

On Tue, Sep 01, 2020 at 01:50:38AM +0000, Leo Li wrote:
>
> Sorry for the late response.  I missed this email previously.
> 
> These structures are descriptors used by hardware, we cannot have _ANY_ padding from the compiler.  The compiled result might be the same with or without the __packed attribute for now, but I think keep it there probably is safer for dealing with unexpected alignment requirements from the compiler in the future.
> 
> Having conflicting alignment requirements warning might means something is wrong with the structure in certain scenario.  I just tried a ARM64 build but didn't see the warnings.  Could you share the warning you got and the build setup?  Thanks.

Just do a COMPILE_TEST build on x86-64:

In file included from ../drivers/crypto/caam/qi.c:12:
../include/soc/fsl/qman.h:259:1: warning: alignment 1 of ‘struct qm_dqrr_entry’ is less than 8 [-Wpacked-not-aligned]
 } __packed;
 ^
../include/soc/fsl/qman.h:292:2: warning: alignment 1 of ‘struct <anonymous>’ is less than 8 [-Wpacked-not-aligned]
  } __packed ern;
  ^

In any case, those packed markers are completely unnecessary because
those structs contain no holes.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
@ 2020-09-01  1:56     ` Herbert Xu
  0 siblings, 0 replies; 15+ messages in thread
From: Herbert Xu @ 2020-09-01  1:56 UTC (permalink / raw)
  To: Leo Li; +Cc: linuxppc-dev, Linux Kernel Mailing List, linux-arm-kernel

On Tue, Sep 01, 2020 at 01:50:38AM +0000, Leo Li wrote:
>
> Sorry for the late response.  I missed this email previously.
> 
> These structures are descriptors used by hardware, we cannot have _ANY_ padding from the compiler.  The compiled result might be the same with or without the __packed attribute for now, but I think keep it there probably is safer for dealing with unexpected alignment requirements from the compiler in the future.
> 
> Having conflicting alignment requirements warning might means something is wrong with the structure in certain scenario.  I just tried a ARM64 build but didn't see the warnings.  Could you share the warning you got and the build setup?  Thanks.

Just do a COMPILE_TEST build on x86-64:

In file included from ../drivers/crypto/caam/qi.c:12:
../include/soc/fsl/qman.h:259:1: warning: alignment 1 of ‘struct qm_dqrr_entry’ is less than 8 [-Wpacked-not-aligned]
 } __packed;
 ^
../include/soc/fsl/qman.h:292:2: warning: alignment 1 of ‘struct <anonymous>’ is less than 8 [-Wpacked-not-aligned]
  } __packed ern;
  ^

In any case, those packed markers are completely unnecessary because
those structs contain no holes.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
@ 2020-09-01  1:56     ` Herbert Xu
  0 siblings, 0 replies; 15+ messages in thread
From: Herbert Xu @ 2020-09-01  1:56 UTC (permalink / raw)
  To: Leo Li; +Cc: linuxppc-dev, Linux Kernel Mailing List, linux-arm-kernel

On Tue, Sep 01, 2020 at 01:50:38AM +0000, Leo Li wrote:
>
> Sorry for the late response.  I missed this email previously.
> 
> These structures are descriptors used by hardware, we cannot have _ANY_ padding from the compiler.  The compiled result might be the same with or without the __packed attribute for now, but I think keep it there probably is safer for dealing with unexpected alignment requirements from the compiler in the future.
> 
> Having conflicting alignment requirements warning might means something is wrong with the structure in certain scenario.  I just tried a ARM64 build but didn't see the warnings.  Could you share the warning you got and the build setup?  Thanks.

Just do a COMPILE_TEST build on x86-64:

In file included from ../drivers/crypto/caam/qi.c:12:
../include/soc/fsl/qman.h:259:1: warning: alignment 1 of ‘struct qm_dqrr_entry’ is less than 8 [-Wpacked-not-aligned]
 } __packed;
 ^
../include/soc/fsl/qman.h:292:2: warning: alignment 1 of ‘struct <anonymous>’ is less than 8 [-Wpacked-not-aligned]
  } __packed ern;
  ^

In any case, those packed markers are completely unnecessary because
those structs contain no holes.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
  2020-09-01  1:56     ` Herbert Xu
  (?)
@ 2020-09-01 21:40       ` Li Yang
  -1 siblings, 0 replies; 15+ messages in thread
From: Li Yang @ 2020-09-01 21:40 UTC (permalink / raw)
  To: Herbert Xu; +Cc: linuxppc-dev, linux-arm-kernel, Linux Kernel Mailing List

On Mon, Aug 31, 2020 at 8:57 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Tue, Sep 01, 2020 at 01:50:38AM +0000, Leo Li wrote:
> >
> > Sorry for the late response.  I missed this email previously.
> >
> > These structures are descriptors used by hardware, we cannot have _ANY_ padding from the compiler.  The compiled result might be the same with or without the __packed attribute for now, but I think keep it there probably is safer for dealing with unexpected alignment requirements from the compiler in the future.
> >
> > Having conflicting alignment requirements warning might means something is wrong with the structure in certain scenario.  I just tried a ARM64 build but didn't see the warnings.  Could you share the warning you got and the build setup?  Thanks.
>
> Just do a COMPILE_TEST build on x86-64:
>
> In file included from ../drivers/crypto/caam/qi.c:12:

Looks like the CAAM driver and dependent QBMAN driver doesn't support
COMPILE_TEST yet.  Are you trying to add the support for it?

I changed the Kconfig to enable the COMPILE_TEST anyway and updated my
toolchain to gcc-10 trying to duplicate the issue.  The issues can
only be reproduced with "W=1".

> ../include/soc/fsl/qman.h:259:1: warning: alignment 1 of ‘struct qm_dqrr_entry’ is less than 8 [-Wpacked-not-aligned]
>  } __packed;
>  ^
> ../include/soc/fsl/qman.h:292:2: warning: alignment 1 of ‘struct <anonymous>’ is less than 8 [-Wpacked-not-aligned]
>   } __packed ern;
>   ^

I think this is a valid concern that if the parent structure doesn't
meet certain alignment requirements, the alignment for the
sub-structure cannot be guaranteed.  If we just remove the __packed
attribute from the parent structure, the compiler could try to add
padding in the parent structure to fulfill the alignment requirements
of the sub structure which is not good.  I think the following changes
are a better fix for the warnings:

diff --git a/include/soc/fsl/qman.h b/include/soc/fsl/qman.h
index cfe00e08e85b..9f484113cfda 100644
--- a/include/soc/fsl/qman.h
+++ b/include/soc/fsl/qman.h
@@ -256,7 +256,7 @@ struct qm_dqrr_entry {
        __be32 context_b;
        struct qm_fd fd;
        u8 __reserved4[32];
-} __packed;
+} __packed __aligned(64);
 #define QM_DQRR_VERB_VBIT              0x80
 #define QM_DQRR_VERB_MASK              0x7f    /* where the verb contains; */
 #define QM_DQRR_VERB_FRAME_DEQUEUE     0x60    /* "this format" */
@@ -289,7 +289,7 @@ union qm_mr_entry {
                __be32 tag;
                struct qm_fd fd;
                u8 __reserved1[32];
-       } __packed ern;
+       } __packed __aligned(64) ern;
        struct {
                u8 verb;
                u8 fqs;         /* Frame Queue Status */


Regards,
Leo

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
@ 2020-09-01 21:40       ` Li Yang
  0 siblings, 0 replies; 15+ messages in thread
From: Li Yang @ 2020-09-01 21:40 UTC (permalink / raw)
  To: Herbert Xu; +Cc: linuxppc-dev, Linux Kernel Mailing List, linux-arm-kernel

On Mon, Aug 31, 2020 at 8:57 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Tue, Sep 01, 2020 at 01:50:38AM +0000, Leo Li wrote:
> >
> > Sorry for the late response.  I missed this email previously.
> >
> > These structures are descriptors used by hardware, we cannot have _ANY_ padding from the compiler.  The compiled result might be the same with or without the __packed attribute for now, but I think keep it there probably is safer for dealing with unexpected alignment requirements from the compiler in the future.
> >
> > Having conflicting alignment requirements warning might means something is wrong with the structure in certain scenario.  I just tried a ARM64 build but didn't see the warnings.  Could you share the warning you got and the build setup?  Thanks.
>
> Just do a COMPILE_TEST build on x86-64:
>
> In file included from ../drivers/crypto/caam/qi.c:12:

Looks like the CAAM driver and dependent QBMAN driver doesn't support
COMPILE_TEST yet.  Are you trying to add the support for it?

I changed the Kconfig to enable the COMPILE_TEST anyway and updated my
toolchain to gcc-10 trying to duplicate the issue.  The issues can
only be reproduced with "W=1".

> ../include/soc/fsl/qman.h:259:1: warning: alignment 1 of ‘struct qm_dqrr_entry’ is less than 8 [-Wpacked-not-aligned]
>  } __packed;
>  ^
> ../include/soc/fsl/qman.h:292:2: warning: alignment 1 of ‘struct <anonymous>’ is less than 8 [-Wpacked-not-aligned]
>   } __packed ern;
>   ^

I think this is a valid concern that if the parent structure doesn't
meet certain alignment requirements, the alignment for the
sub-structure cannot be guaranteed.  If we just remove the __packed
attribute from the parent structure, the compiler could try to add
padding in the parent structure to fulfill the alignment requirements
of the sub structure which is not good.  I think the following changes
are a better fix for the warnings:

diff --git a/include/soc/fsl/qman.h b/include/soc/fsl/qman.h
index cfe00e08e85b..9f484113cfda 100644
--- a/include/soc/fsl/qman.h
+++ b/include/soc/fsl/qman.h
@@ -256,7 +256,7 @@ struct qm_dqrr_entry {
        __be32 context_b;
        struct qm_fd fd;
        u8 __reserved4[32];
-} __packed;
+} __packed __aligned(64);
 #define QM_DQRR_VERB_VBIT              0x80
 #define QM_DQRR_VERB_MASK              0x7f    /* where the verb contains; */
 #define QM_DQRR_VERB_FRAME_DEQUEUE     0x60    /* "this format" */
@@ -289,7 +289,7 @@ union qm_mr_entry {
                __be32 tag;
                struct qm_fd fd;
                u8 __reserved1[32];
-       } __packed ern;
+       } __packed __aligned(64) ern;
        struct {
                u8 verb;
                u8 fqs;         /* Frame Queue Status */


Regards,
Leo

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
@ 2020-09-01 21:40       ` Li Yang
  0 siblings, 0 replies; 15+ messages in thread
From: Li Yang @ 2020-09-01 21:40 UTC (permalink / raw)
  To: Herbert Xu; +Cc: linuxppc-dev, Linux Kernel Mailing List, linux-arm-kernel

On Mon, Aug 31, 2020 at 8:57 PM Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Tue, Sep 01, 2020 at 01:50:38AM +0000, Leo Li wrote:
> >
> > Sorry for the late response.  I missed this email previously.
> >
> > These structures are descriptors used by hardware, we cannot have _ANY_ padding from the compiler.  The compiled result might be the same with or without the __packed attribute for now, but I think keep it there probably is safer for dealing with unexpected alignment requirements from the compiler in the future.
> >
> > Having conflicting alignment requirements warning might means something is wrong with the structure in certain scenario.  I just tried a ARM64 build but didn't see the warnings.  Could you share the warning you got and the build setup?  Thanks.
>
> Just do a COMPILE_TEST build on x86-64:
>
> In file included from ../drivers/crypto/caam/qi.c:12:

Looks like the CAAM driver and dependent QBMAN driver doesn't support
COMPILE_TEST yet.  Are you trying to add the support for it?

I changed the Kconfig to enable the COMPILE_TEST anyway and updated my
toolchain to gcc-10 trying to duplicate the issue.  The issues can
only be reproduced with "W=1".

> ../include/soc/fsl/qman.h:259:1: warning: alignment 1 of ‘struct qm_dqrr_entry’ is less than 8 [-Wpacked-not-aligned]
>  } __packed;
>  ^
> ../include/soc/fsl/qman.h:292:2: warning: alignment 1 of ‘struct <anonymous>’ is less than 8 [-Wpacked-not-aligned]
>   } __packed ern;
>   ^

I think this is a valid concern that if the parent structure doesn't
meet certain alignment requirements, the alignment for the
sub-structure cannot be guaranteed.  If we just remove the __packed
attribute from the parent structure, the compiler could try to add
padding in the parent structure to fulfill the alignment requirements
of the sub structure which is not good.  I think the following changes
are a better fix for the warnings:

diff --git a/include/soc/fsl/qman.h b/include/soc/fsl/qman.h
index cfe00e08e85b..9f484113cfda 100644
--- a/include/soc/fsl/qman.h
+++ b/include/soc/fsl/qman.h
@@ -256,7 +256,7 @@ struct qm_dqrr_entry {
        __be32 context_b;
        struct qm_fd fd;
        u8 __reserved4[32];
-} __packed;
+} __packed __aligned(64);
 #define QM_DQRR_VERB_VBIT              0x80
 #define QM_DQRR_VERB_MASK              0x7f    /* where the verb contains; */
 #define QM_DQRR_VERB_FRAME_DEQUEUE     0x60    /* "this format" */
@@ -289,7 +289,7 @@ union qm_mr_entry {
                __be32 tag;
                struct qm_fd fd;
                u8 __reserved1[32];
-       } __packed ern;
+       } __packed __aligned(64) ern;
        struct {
                u8 verb;
                u8 fqs;         /* Frame Queue Status */


Regards,
Leo

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply related	[flat|nested] 15+ messages in thread

* Re: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
  2020-09-01 21:40       ` Li Yang
  (?)
@ 2020-09-02  1:34         ` Herbert Xu
  -1 siblings, 0 replies; 15+ messages in thread
From: Herbert Xu @ 2020-09-02  1:34 UTC (permalink / raw)
  To: Li Yang; +Cc: linuxppc-dev, linux-arm-kernel, Linux Kernel Mailing List

On Tue, Sep 01, 2020 at 04:40:16PM -0500, Li Yang wrote:
>
> Looks like the CAAM driver and dependent QBMAN driver doesn't support
> COMPILE_TEST yet.  Are you trying to add the support for it?

Yes.

> I think this is a valid concern that if the parent structure doesn't
> meet certain alignment requirements, the alignment for the
> sub-structure cannot be guaranteed.  If we just remove the __packed
> attribute from the parent structure, the compiler could try to add
> padding in the parent structure to fulfill the alignment requirements
> of the sub structure which is not good.  I think the following changes
> are a better fix for the warnings:

This works for me.  Thanks!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
@ 2020-09-02  1:34         ` Herbert Xu
  0 siblings, 0 replies; 15+ messages in thread
From: Herbert Xu @ 2020-09-02  1:34 UTC (permalink / raw)
  To: Li Yang; +Cc: linuxppc-dev, Linux Kernel Mailing List, linux-arm-kernel

On Tue, Sep 01, 2020 at 04:40:16PM -0500, Li Yang wrote:
>
> Looks like the CAAM driver and dependent QBMAN driver doesn't support
> COMPILE_TEST yet.  Are you trying to add the support for it?

Yes.

> I think this is a valid concern that if the parent structure doesn't
> meet certain alignment requirements, the alignment for the
> sub-structure cannot be guaranteed.  If we just remove the __packed
> attribute from the parent structure, the compiler could try to add
> padding in the parent structure to fulfill the alignment requirements
> of the sub structure which is not good.  I think the following changes
> are a better fix for the warnings:

This works for me.  Thanks!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

^ permalink raw reply	[flat|nested] 15+ messages in thread

* Re: [PATCH] soc: fsl: Remove bogus packed attributes from qman.h
@ 2020-09-02  1:34         ` Herbert Xu
  0 siblings, 0 replies; 15+ messages in thread
From: Herbert Xu @ 2020-09-02  1:34 UTC (permalink / raw)
  To: Li Yang; +Cc: linuxppc-dev, Linux Kernel Mailing List, linux-arm-kernel

On Tue, Sep 01, 2020 at 04:40:16PM -0500, Li Yang wrote:
>
> Looks like the CAAM driver and dependent QBMAN driver doesn't support
> COMPILE_TEST yet.  Are you trying to add the support for it?

Yes.

> I think this is a valid concern that if the parent structure doesn't
> meet certain alignment requirements, the alignment for the
> sub-structure cannot be guaranteed.  If we just remove the __packed
> attribute from the parent structure, the compiler could try to add
> padding in the parent structure to fulfill the alignment requirements
> of the sub structure which is not good.  I think the following changes
> are a better fix for the warnings:

This works for me.  Thanks!
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

^ permalink raw reply	[flat|nested] 15+ messages in thread

end of thread, other threads:[~2020-09-02  1:36 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-07-30 12:52 [PATCH] soc: fsl: Remove bogus packed attributes from qman.h Herbert Xu
2020-07-30 12:52 ` Herbert Xu
2020-09-01  0:33 ` Herbert Xu
2020-09-01  0:33   ` Herbert Xu
2020-09-01  1:50 ` Leo Li
2020-09-01  1:50   ` Leo Li
2020-09-01  1:56   ` Herbert Xu
2020-09-01  1:56     ` Herbert Xu
2020-09-01  1:56     ` Herbert Xu
2020-09-01 21:40     ` Li Yang
2020-09-01 21:40       ` Li Yang
2020-09-01 21:40       ` Li Yang
2020-09-02  1:34       ` Herbert Xu
2020-09-02  1:34         ` Herbert Xu
2020-09-02  1:34         ` Herbert Xu

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.