diff mbox series

scsi: lpfc: fix lpfc_name struct packing

Message ID 20230616090705.2623408-1-arnd@kernel.org (mailing list archive)
State Accepted
Headers show
Series scsi: lpfc: fix lpfc_name struct packing | expand

Commit Message

Arnd Bergmann June 16, 2023, 9:06 a.m. UTC 
From: Arnd Bergmann <arnd@arndb.de>

clang points out that the lpfc_name structure has an 8-byte alignement requirement
on most architectures, but is embedded in a number of other structures that are
forced to be only 1-byte aligned:

drivers/scsi/lpfc/lpfc_hw.h:1516:30: error: field pe within 'struct lpfc_fdmi_reg_port_list' is less aligned than 'struct lpfc_fdmi_port_entry' and is usually due to 'struct lpfc_fdmi_reg_port_list' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]
        struct lpfc_fdmi_port_entry pe;
drivers/scsi/lpfc/lpfc_hw.h:850:19: error: field portName within 'struct _ADISC' is less aligned than 'struct lpfc_name' and is usually due to 'struct _ADISC' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]
drivers/scsi/lpfc/lpfc_hw.h:851:19: error: field nodeName within 'struct _ADISC' is less aligned than 'struct lpfc_name' and is usually due to 'struct _ADISC' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]
drivers/scsi/lpfc/lpfc_hw.h:922:19: error: field portName within 'struct _RNID' is less aligned than 'struct lpfc_name' and is usually due to 'struct _RNID' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]
drivers/scsi/lpfc/lpfc_hw.h:923:19: error: field nodeName within 'struct _RNID' is less aligned than 'struct lpfc_name' and is usually due to 'struct _RNID' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]

From the git history, I can see that all the __packed annotations were done
specifically to avoid introducing implicit padding around the lpfc_name
instances, though this was probably the wrong approach.

To improve this, only annotate the one uint64_t field inside of lpfc_name
as packed, with an explicit 4-byte alignment, as is the default already on
the 32-bit x86 ABI but not on most others. With this, the other __packed
annotations can be removed again, as this avoids the incorrect padding.

Two other structures change their layout as a result of this change:

- struct _LOGO never gained a __packed annotation even though it has the
  same alignment problem as the others but is not used anywhere in the
  driver today.

- struct serv_param similarly has this issue, and it is used, my guess
  is that this is only an internal structure rather than part of a binary
  interface, so the padding has no negative effect here.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/scsi/lpfc/lpfc_hw.h | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

Comments

Justin Tee June 16, 2023, 6:35 p.m. UTC | #1 
Hi Arnd,

Thanks looks good.

Reviewed-by: Justin Tee <justin.tee@broadcom.com>


On Fri, Jun 16, 2023 at 2:07 AM Arnd Bergmann <arnd@kernel.org> wrote:
>
> From: Arnd Bergmann <arnd@arndb.de>
>
> clang points out that the lpfc_name structure has an 8-byte alignement requirement
> on most architectures, but is embedded in a number of other structures that are
> forced to be only 1-byte aligned:
>
> drivers/scsi/lpfc/lpfc_hw.h:1516:30: error: field pe within 'struct lpfc_fdmi_reg_port_list' is less aligned than 'struct lpfc_fdmi_port_entry' and is usually due to 'struct lpfc_fdmi_reg_port_list' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]
>         struct lpfc_fdmi_port_entry pe;
> drivers/scsi/lpfc/lpfc_hw.h:850:19: error: field portName within 'struct _ADISC' is less aligned than 'struct lpfc_name' and is usually due to 'struct _ADISC' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]
> drivers/scsi/lpfc/lpfc_hw.h:851:19: error: field nodeName within 'struct _ADISC' is less aligned than 'struct lpfc_name' and is usually due to 'struct _ADISC' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]
> drivers/scsi/lpfc/lpfc_hw.h:922:19: error: field portName within 'struct _RNID' is less aligned than 'struct lpfc_name' and is usually due to 'struct _RNID' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]
> drivers/scsi/lpfc/lpfc_hw.h:923:19: error: field nodeName within 'struct _RNID' is less aligned than 'struct lpfc_name' and is usually due to 'struct _RNID' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]
>
> From the git history, I can see that all the __packed annotations were done
> specifically to avoid introducing implicit padding around the lpfc_name
> instances, though this was probably the wrong approach.
>
> To improve this, only annotate the one uint64_t field inside of lpfc_name
> as packed, with an explicit 4-byte alignment, as is the default already on
> the 32-bit x86 ABI but not on most others. With this, the other __packed
> annotations can be removed again, as this avoids the incorrect padding.
>
> Two other structures change their layout as a result of this change:
>
> - struct _LOGO never gained a __packed annotation even though it has the
>   same alignment problem as the others but is not used anywhere in the
>   driver today.
>
> - struct serv_param similarly has this issue, and it is used, my guess
>   is that this is only an internal structure rather than part of a binary
>   interface, so the padding has no negative effect here.
>
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
>  drivers/scsi/lpfc/lpfc_hw.h | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/drivers/scsi/lpfc/lpfc_hw.h b/drivers/scsi/lpfc/lpfc_hw.h
> index 663755842e4a4..aaea3e31944d0 100644
> --- a/drivers/scsi/lpfc/lpfc_hw.h
> +++ b/drivers/scsi/lpfc/lpfc_hw.h
> @@ -365,7 +365,7 @@ struct lpfc_name {
>                         uint8_t IEEE[6];        /* FC IEEE address */
>                 } s;
>                 uint8_t wwn[8];
> -               uint64_t name;
> +               uint64_t name __packed __aligned(4);
>         } u;
>  };
>
> @@ -850,7 +850,7 @@ typedef struct _ADISC {             /* Structure is in Big Endian format */
>         struct lpfc_name portName;
>         struct lpfc_name nodeName;
>         uint32_t DID;
> -} __packed ADISC;
> +} ADISC;
>
>  typedef struct _FARP {         /* Structure is in Big Endian format */
>         uint32_t Mflags:8;
> @@ -880,7 +880,7 @@ typedef struct _FAN {               /* Structure is in Big Endian format */
>         uint32_t Fdid;
>         struct lpfc_name FportName;
>         struct lpfc_name FnodeName;
> -} __packed FAN;
> +} FAN;
>
>  typedef struct _SCR {          /* Structure is in Big Endian format */
>         uint8_t resvd1;
> @@ -924,7 +924,7 @@ typedef struct _RNID {              /* Structure is in Big Endian format */
>         union {
>                 RNID_TOP_DISC topologyDisc;     /* topology disc (0xdf) */
>         } un;
> -} __packed RNID;
> +} RNID;
>
>  struct RLS {                   /* Structure is in Big Endian format */
>         uint32_t rls;
> @@ -1514,7 +1514,7 @@ struct lpfc_fdmi_hba_ident {
>  struct lpfc_fdmi_reg_port_list {
>         __be32 EntryCnt;
>         struct lpfc_fdmi_port_entry pe;
> -} __packed;
> +};
>
>  /*
>   * Register HBA(RHBA)
> --
> 2.39.2
>
Martin K. Petersen June 22, 2023, 1:09 a.m. UTC | #2 
Arnd,

> clang points out that the lpfc_name structure has an 8-byte alignement
> requirement on most architectures, but is embedded in a number of
> other structures that are forced to be only 1-byte aligned:

Applied to 6.5/scsi-staging, thanks!
Martin K. Petersen June 29, 2023, 2:41 a.m. UTC | #3 
On Fri, 16 Jun 2023 11:06:56 +0200, Arnd Bergmann wrote:

> clang points out that the lpfc_name structure has an 8-byte alignement requirement
> on most architectures, but is embedded in a number of other structures that are
> forced to be only 1-byte aligned:
> 
> drivers/scsi/lpfc/lpfc_hw.h:1516:30: error: field pe within 'struct lpfc_fdmi_reg_port_list' is less aligned than 'struct lpfc_fdmi_port_entry' and is usually due to 'struct lpfc_fdmi_reg_port_list' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]
>         struct lpfc_fdmi_port_entry pe;
> drivers/scsi/lpfc/lpfc_hw.h:850:19: error: field portName within 'struct _ADISC' is less aligned than 'struct lpfc_name' and is usually due to 'struct _ADISC' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]
> drivers/scsi/lpfc/lpfc_hw.h:851:19: error: field nodeName within 'struct _ADISC' is less aligned than 'struct lpfc_name' and is usually due to 'struct _ADISC' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]
> drivers/scsi/lpfc/lpfc_hw.h:922:19: error: field portName within 'struct _RNID' is less aligned than 'struct lpfc_name' and is usually due to 'struct _RNID' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]
> drivers/scsi/lpfc/lpfc_hw.h:923:19: error: field nodeName within 'struct _RNID' is less aligned than 'struct lpfc_name' and is usually due to 'struct _RNID' being packed, which can lead to unaligned accesses [-Werror,-Wunaligned-access]
> 
> [...]

Applied to 6.5/scsi-queue, thanks!

[1/1] scsi: lpfc: fix lpfc_name struct packing
      https://git.kernel.org/mkp/scsi/c/00c2cae6b66a
diff mbox series

Patch

diff --git a/drivers/scsi/lpfc/lpfc_hw.h b/drivers/scsi/lpfc/lpfc_hw.h
index 663755842e4a4..aaea3e31944d0 100644
--- a/drivers/scsi/lpfc/lpfc_hw.h
+++ b/drivers/scsi/lpfc/lpfc_hw.h
@@ -365,7 +365,7 @@  struct lpfc_name {
 			uint8_t IEEE[6];	/* FC IEEE address */
 		} s;
 		uint8_t wwn[8];
-		uint64_t name;
+		uint64_t name __packed __aligned(4);
 	} u;
 };
 
@@ -850,7 +850,7 @@  typedef struct _ADISC {		/* Structure is in Big Endian format */
 	struct lpfc_name portName;
 	struct lpfc_name nodeName;
 	uint32_t DID;
-} __packed ADISC;
+} ADISC;
 
 typedef struct _FARP {		/* Structure is in Big Endian format */
 	uint32_t Mflags:8;
@@ -880,7 +880,7 @@  typedef struct _FAN {		/* Structure is in Big Endian format */
 	uint32_t Fdid;
 	struct lpfc_name FportName;
 	struct lpfc_name FnodeName;
-} __packed FAN;
+} FAN;
 
 typedef struct _SCR {		/* Structure is in Big Endian format */
 	uint8_t resvd1;
@@ -924,7 +924,7 @@  typedef struct _RNID {		/* Structure is in Big Endian format */
 	union {
 		RNID_TOP_DISC topologyDisc;	/* topology disc (0xdf) */
 	} un;
-} __packed RNID;
+} RNID;
 
 struct RLS {			/* Structure is in Big Endian format */
 	uint32_t rls;
@@ -1514,7 +1514,7 @@  struct lpfc_fdmi_hba_ident {
 struct lpfc_fdmi_reg_port_list {
 	__be32 EntryCnt;
 	struct lpfc_fdmi_port_entry pe;
-} __packed;
+};
 
 /*
  * Register HBA(RHBA)