Re: [PATCH] scsi: lpfc: fix lpfc_name struct packing

[Justin Tee]

Hi Arnd,

Thanks looks good.

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


On Fri, Jun 16, 2023 at 2:07 AM Arnd Bergmann <arnd@xxxxxxxxxx> wrote:
>
> From: Arnd Bergmann <arnd@xxxxxxxx>
>
> 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@xxxxxxxx>
> ---
>  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
>

-- 
This electronic communication and the information and any files transmitted 
with it, or attached to it, are confidential and are intended solely for 
the use of the individual or entity to whom it is addressed and may contain 
information that is confidential, legally privileged, protected by privacy 
laws, or otherwise restricted from disclosure to anyone else. If you are 
not the intended recipient or the person responsible for delivering the 
e-mail to the intended recipient, you are hereby notified that any use, 
copying, distributing, dissemination, forwarding, printing, or copying of 
this e-mail is strictly prohibited. If you received this e-mail in error, 
please return the e-mail to the sender, delete it from your computer, and 
destroy any printed copy of it.

Attachment: smime.p7s
Description: S/MIME Cryptographic Signature