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

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

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 [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

-----Original Message-----
From: Herbert Xu <herbert@gondor.apana.org.au>
Sent: Thursday, July 30, 2020 7:53 AM
To: Leo Li <redacted>; 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 

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 [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

On Mon, Aug 31, 2020 at 8:57 PM Herbert Xu [off-list ref] wrote:

On Tue, Sep 01, 2020 at 01:50:38AM +0000, Leo Li wrote:

quoted

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

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 [off-list ref]
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt