| Message ID | 20210323131530.2619900-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Queued, archived |
| Headers | show |
| Series | soc/fsl: qbman: fix conflicting alignment attributes | expand |
On Tue, Mar 23, 2021 at 8:17 AM Arnd Bergmann <arnd@kernel.org> wrote: > > From: Arnd Bergmann <arnd@arndb.de> > > When building with W=1, gcc points out that the __packed attribute > on struct qm_eqcr_entry conflicts with the 8-byte alignment > attribute on struct qm_fd inside it: > > drivers/soc/fsl/qbman/qman.c:189:1: error: alignment 1 of 'struct qm_eqcr_entry' is less than 8 [-Werror=packed-not-aligned] > > I assume that the alignment attribute is the correct one, and > that qm_eqcr_entry cannot actually be unaligned in memory, > so add the same alignment on the outer struct. > > Fixes: c535e923bb97 ("soc/fsl: Introduce DPAA 1.x QMan device driver") > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > --- > drivers/soc/fsl/qbman/qman.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c > index a1b9be1d105a..fde4edd83c14 100644 > --- a/drivers/soc/fsl/qbman/qman.c > +++ b/drivers/soc/fsl/qbman/qman.c > @@ -186,7 +186,7 @@ struct qm_eqcr_entry { > __be32 tag; > struct qm_fd fd; > u8 __reserved3[32]; > -} __packed; > +} __packed __aligned(8); The EQCR structure is actually aligned on 64-byte from the manual. But probably 8 is enough to let the compiler not complain. > #define QM_EQCR_VERB_VBIT 0x80 > #define QM_EQCR_VERB_CMD_MASK 0x61 /* but only one value; */ > #define QM_EQCR_VERB_CMD_ENQUEUE 0x01 > -- > 2.29.2 >
On Fri, Mar 26, 2021 at 3:17 AM Li Yang <leoyang.li@nxp.com> wrote: > On Tue, Mar 23, 2021 at 8:17 AM Arnd Bergmann <arnd@kernel.org> wrote: > > > > From: Arnd Bergmann <arnd@arndb.de> > > > > When building with W=1, gcc points out that the __packed attribute > > on struct qm_eqcr_entry conflicts with the 8-byte alignment > > attribute on struct qm_fd inside it: > > > > drivers/soc/fsl/qbman/qman.c:189:1: error: alignment 1 of 'struct qm_eqcr_entry' is less than 8 [-Werror=packed-not-aligned] > > > > I assume that the alignment attribute is the correct one, and > > that qm_eqcr_entry cannot actually be unaligned in memory, > > so add the same alignment on the outer struct. > > > > Fixes: c535e923bb97 ("soc/fsl: Introduce DPAA 1.x QMan device driver") > > Signed-off-by: Arnd Bergmann <arnd@arndb.de> > > --- > > drivers/soc/fsl/qbman/qman.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c > > index a1b9be1d105a..fde4edd83c14 100644 > > --- a/drivers/soc/fsl/qbman/qman.c > > +++ b/drivers/soc/fsl/qbman/qman.c > > @@ -186,7 +186,7 @@ struct qm_eqcr_entry { > > __be32 tag; > > struct qm_fd fd; > > u8 __reserved3[32]; > > -} __packed; > > +} __packed __aligned(8); > > The EQCR structure is actually aligned on 64-byte from the manual. > But probably 8 is enough to let the compiler not complain. The important bit is that all members inside are at most 8-byte long and naturally aligned, so the compiler can now optimize the accesses because it can assume that each member can be updated atomically. Marking a structure as unaligned with the __packed attribute can lead to rather unoptimized code, which is particularly important when the structure is mapped as uncached -- accessing it one byte at a time means you have eight times the latency for a simple read. I think on arm64, the compiler doesn't actually have to do that, but at least if you run a 32-bit kernel, the packing would prevent the use of ldrd or ldm instructions that need an aligned word. Arnd
From: Arnd Bergmann <arnd@arndb.de> On Tue, 23 Mar 2021 14:15:23 +0100, Arnd Bergmann wrote: > When building with W=1, gcc points out that the __packed attribute > on struct qm_eqcr_entry conflicts with the 8-byte alignment > attribute on struct qm_fd inside it: > > drivers/soc/fsl/qbman/qman.c:189:1: error: alignment 1 of 'struct qm_eqcr_entry' is less than 8 [-Werror=packed-not-aligned] > > I assume that the alignment attribute is the correct one, and > that qm_eqcr_entry cannot actually be unaligned in memory, > so add the same alignment on the outer struct. Applied to arm/fixes [1/1] soc/fsl: qbman: fix conflicting alignment attributes commit: b9ab2619047edc1c6c07dc68963d84a5979f0abe Arnd
diff --git a/drivers/soc/fsl/qbman/qman.c b/drivers/soc/fsl/qbman/qman.c index a1b9be1d105a..fde4edd83c14 100644 --- a/drivers/soc/fsl/qbman/qman.c +++ b/drivers/soc/fsl/qbman/qman.c @@ -186,7 +186,7 @@ struct qm_eqcr_entry { __be32 tag; struct qm_fd fd; u8 __reserved3[32]; -} __packed; +} __packed __aligned(8); #define QM_EQCR_VERB_VBIT 0x80 #define QM_EQCR_VERB_CMD_MASK 0x61 /* but only one value; */ #define QM_EQCR_VERB_CMD_ENQUEUE 0x01