| Message ID | 20210312222718.4117508-1-slyfox@gentoo.org |
|---|---|
| State | New |
| Headers | show |
| Series | hpsa: fix boot on ia64 (atomic_t alignment) | expand |
-----Original Message----- From: Sergei Trofimovich [mailto:slyfox@gentoo.org] Subject: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment) The failure initially observed as boot failure on rx3600 ia64 machine with RAID bus controller: Hewlett-Packard Company Smart Array P600: kernel unaligned access to 0xe000000105dd8b95, ip=0xa000000100b87551 kernel unaligned access to 0xe000000105dd8e95, ip=0xa000000100b87551 hpsa 0000:14:01.0: Controller reports max supported commands of 0 Using 16 instead. Ensure that firmware is up to date. swapper/0[1]: error during unaligned kernel access Here unaligned access comes from 'struct CommandList' that happens to be packed. The change f749d8b7a ("scsi: hpsa: Correct dev cmds outstanding for retried cmds") introduced unexpected padding and un-aligned atomic_t from natural alignment to something else. This change does not remove packing annotation from struct but only restores alignment of atomic variable. The change is tested on the same rx3600 machine. CC: linux-ia64@vger.kernel.org CC: storagedev@microchip.com CC: linux-scsi@vger.kernel.org CC: Joe Szczypek <jszczype@redhat.com> CC: Scott Benesh <scott.benesh@microchip.com> CC: Scott Teel <scott.teel@microchip.com> CC: Tomas Henzl <thenzl@redhat.com> CC: "Martin K. Petersen" <martin.petersen@oracle.com> CC: Don Brace <don.brace@microchip.com> Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> Suggested-by: Don Brace <don.brace@microchip.com> Fixes: f749d8b7a "scsi: hpsa: Correct dev cmds outstanding for retried cmds" Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> --- drivers/scsi/hpsa_cmd.h | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h index d126bb877250..617bdae9a7de 100644 --- a/drivers/scsi/hpsa_cmd.h +++ b/drivers/scsi/hpsa_cmd.h @@ -20,6 +20,9 @@ #ifndef HPSA_CMD_H #define HPSA_CMD_H +#include <linux/build_bug.h> /* static_assert */ #include +<linux/stddef.h> /* offsetof */ + /* general boundary defintions */ #define SENSEINFOBYTES 32 /* may vary between hbas */ #define SG_ENTRIES_IN_CMD 32 /* Max SG entries excluding chain blocks */ @@ -448,11 +451,20 @@ struct CommandList { */ struct hpsa_scsi_dev_t *phys_disk; - bool retry_pending; + int retry_pending; struct hpsa_scsi_dev_t *device; atomic_t refcount; /* Must be last to avoid memset in hpsa_cmd_init() */ } __aligned(COMMANDLIST_ALIGNMENT); +/* + * Make sure our embedded atomic variable is aligned. Otherwise we +break atomic + * operations on architectures that don't support unaligned atomics like IA64. + * + * Ideally this header should be cleaned up to only mark individual +structs as + * packed. + */ +static_assert(offsetof(struct CommandList, refcount) % +__alignof__(atomic_t) == 0); + /* Max S/G elements in I/O accelerator command */ #define IOACCEL1_MAXSGENTRIES 24 #define IOACCEL2_MAXSGENTRIES 28 -- 2.30.2 Thank-you for your testing. I would rather you add the atomic_t alignment check only. The current patch under review has other changes... https://patchwork.kernel.org/project/linux-scsi/patch/161540317205.18786.5821926127237311408.stgit@brunhilda/
On Tue, Mar 16, 2021 at 6:12 PM <Don.Brace@microchip.com> wrote: > drivers/scsi/hpsa_cmd.h | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h index d126bb877250..617bdae9a7de 100644 > --- a/drivers/scsi/hpsa_cmd.h > +++ b/drivers/scsi/hpsa_cmd.h > @@ -20,6 +20,9 @@ > #ifndef HPSA_CMD_H > #define HPSA_CMD_H > > +#include <linux/build_bug.h> /* static_assert */ #include > +<linux/stddef.h> /* offsetof */ > + > /* general boundary defintions */ > #define SENSEINFOBYTES 32 /* may vary between hbas */ > #define SG_ENTRIES_IN_CMD 32 /* Max SG entries excluding chain blocks */ > @@ -448,11 +451,20 @@ struct CommandList { > */ > struct hpsa_scsi_dev_t *phys_disk; > > - bool retry_pending; > + int retry_pending; > struct hpsa_scsi_dev_t *device; > atomic_t refcount; /* Must be last to avoid memset in hpsa_cmd_init() */ } __aligned(COMMANDLIST_ALIGNMENT); > > +/* > + * Make sure our embedded atomic variable is aligned. Otherwise we > +break atomic > + * operations on architectures that don't support unaligned atomics like IA64. > + * > + * Ideally this header should be cleaned up to only mark individual > +structs as > + * packed. > + */ > +static_assert(offsetof(struct CommandList, refcount) % > +__alignof__(atomic_t) == 0); > + Actually that still feels wrong: the annotation of the struct is to pack every member, which causes the access to be done in byte units on architectures that do not have hardware unaligned load/store instructions, at least for things like atomic_read() that does not go through a cmpxchg() or ll/sc cycle. This change may fix itanium, but it's still not correct. Other architectures would have already been broken before the recent change, but that's not a reason against fixing them now. I'd recommend marking the entire structure as having default alignment, by adding the appropriate pragmas before and after it. Arnd
Arnd, > Actually that still feels wrong: the annotation of the struct is to > pack every member, which causes the access to be done in byte units on > architectures that do not have hardware unaligned load/store > instructions, at least for things like atomic_read() that does not go > through a cmpxchg() or ll/sc cycle. > This change may fix itanium, but it's still not correct. Other > architectures would have already been broken before the recent change, > but that's not a reason against fixing them now. I agree. I understand why there are restrictions on fields consumed by the hardware. But for fields internal to the driver the packing doesn't make sense to me. -- Martin K. Petersen Oracle Linux Engineering
From: Martin K. Petersen > Sent: 17 March 2021 02:26 > > Arnd, > > > Actually that still feels wrong: the annotation of the struct is to > > pack every member, which causes the access to be done in byte units on > > architectures that do not have hardware unaligned load/store > > instructions, at least for things like atomic_read() that does not go > > through a cmpxchg() or ll/sc cycle. > > > This change may fix itanium, but it's still not correct. Other > > architectures would have already been broken before the recent change, > > but that's not a reason against fixing them now. > > I agree. I understand why there are restrictions on fields consumed by > the hardware. But for fields internal to the driver the packing doesn't > make sense to me. Jeepers -- that global #pragma pack(1) is bollocks. I think there are a couple of __u64 that are 32bit aligned. Just marking those field __packed __aligned(4) should have the desired effect. Or use a typedef for '__u64 with 32bit alignment'. (There probably ought to be one in types.h) Then add compile-time asserts that any non-trivial structures the hardware accesses are the right size. David - Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
Hi Sergei! On 3/12/21 11:27 PM, Sergei Trofimovich wrote: > The failure initially observed as boot failure on rx3600 ia64 machine > with RAID bus controller: Hewlett-Packard Company Smart Array P600: > > kernel unaligned access to 0xe000000105dd8b95, ip=0xa000000100b87551 > kernel unaligned access to 0xe000000105dd8e95, ip=0xa000000100b87551 > hpsa 0000:14:01.0: Controller reports max supported commands of 0 Using 16 instead. Ensure that firmware is up to date. > swapper/0[1]: error during unaligned kernel access > > Here unaligned access comes from 'struct CommandList' that happens > to be packed. The change f749d8b7a ("scsi: hpsa: Correct dev cmds > outstanding for retried cmds") introduced unexpected padding and > un-aligned atomic_t from natural alignment to something else. > > This change does not remove packing annotation from struct but only > restores alignment of atomic variable. > > The change is tested on the same rx3600 machine. I just gave it a try on my RX2660 and for me, the hpsa driver won't load even with your patch. Can you share your kernel configuration so I can give it a try? Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer - glaubitz@debian.org `. `' Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de `- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
-----Original Message----- From: David Laight [mailto:David.Laight@ACULAB.COM] Subject: RE: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment) From: Martin K. Petersen > Sent: 17 March 2021 02:26 > > Arnd, > > > Actually that still feels wrong: the annotation of the struct is to > > pack every member, which causes the access to be done in byte units > > on architectures that do not have hardware unaligned load/store > > instructions, at least for things like atomic_read() that does not > > go through a cmpxchg() or ll/sc cycle. > > > This change may fix itanium, but it's still not correct. Other > > architectures would have already been broken before the recent > > change, but that's not a reason against fixing them now. > > I agree. I understand why there are restrictions on fields consumed by > the hardware. But for fields internal to the driver the packing > doesn't make sense to me. Jeepers -- that global #pragma pack(1) is bollocks. I think there are a couple of __u64 that are 32bit aligned. Just marking those field __packed __aligned(4) should have the desired effect. Or use a typedef for '__u64 with 32bit alignment'. (There probably ought to be one in types.h) Then add compile-time asserts that any non-trivial structures the hardware accesses are the right size. David Don: My dilemma is that hpsa is now a maintenance driver and making more packing/alignment changes would trigger a lot of regression testing. My last patch aligns the structure with what has been in place for a long time now. All I did was rename an unused variable. So making any more changes is not high on my todo list...
Hello! On 3/12/21 11:27 PM, Sergei Trofimovich wrote: > The failure initially observed as boot failure on rx3600 ia64 machine > with RAID bus controller: Hewlett-Packard Company Smart Array P600: > > kernel unaligned access to 0xe000000105dd8b95, ip=0xa000000100b87551 > kernel unaligned access to 0xe000000105dd8e95, ip=0xa000000100b87551 > hpsa 0000:14:01.0: Controller reports max supported commands of 0 Using 16 instead. Ensure that firmware is up to date. > swapper/0[1]: error during unaligned kernel access > > Here unaligned access comes from 'struct CommandList' that happens > to be packed. The change f749d8b7a ("scsi: hpsa: Correct dev cmds > outstanding for retried cmds") introduced unexpected padding and > un-aligned atomic_t from natural alignment to something else. > > This change does not remove packing annotation from struct but only > restores alignment of atomic variable. > > The change is tested on the same rx3600 machine. > > CC: linux-ia64@vger.kernel.org > CC: storagedev@microchip.com > CC: linux-scsi@vger.kernel.org > CC: Joe Szczypek <jszczype@redhat.com> > CC: Scott Benesh <scott.benesh@microchip.com> > CC: Scott Teel <scott.teel@microchip.com> > CC: Tomas Henzl <thenzl@redhat.com> > CC: "Martin K. Petersen" <martin.petersen@oracle.com> > CC: Don Brace <don.brace@microchip.com> > Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> > Suggested-by: Don Brace <don.brace@microchip.com> > Fixes: f749d8b7a "scsi: hpsa: Correct dev cmds outstanding for retried cmds" > Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> > --- > drivers/scsi/hpsa_cmd.h | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h > index d126bb877250..617bdae9a7de 100644 > --- a/drivers/scsi/hpsa_cmd.h > +++ b/drivers/scsi/hpsa_cmd.h > @@ -20,6 +20,9 @@ > #ifndef HPSA_CMD_H > #define HPSA_CMD_H > > +#include <linux/build_bug.h> /* static_assert */ > +#include <linux/stddef.h> /* offsetof */ > + > /* general boundary defintions */ > #define SENSEINFOBYTES 32 /* may vary between hbas */ > #define SG_ENTRIES_IN_CMD 32 /* Max SG entries excluding chain blocks */ > @@ -448,11 +451,20 @@ struct CommandList { > */ > struct hpsa_scsi_dev_t *phys_disk; > > - bool retry_pending; > + int retry_pending; > struct hpsa_scsi_dev_t *device; > atomic_t refcount; /* Must be last to avoid memset in hpsa_cmd_init() */ > } __aligned(COMMANDLIST_ALIGNMENT); > > +/* > + * Make sure our embedded atomic variable is aligned. Otherwise we break atomic > + * operations on architectures that don't support unaligned atomics like IA64. > + * > + * Ideally this header should be cleaned up to only mark individual structs as > + * packed. > + */ > +static_assert(offsetof(struct CommandList, refcount) % __alignof__(atomic_t) == 0); > + > /* Max S/G elements in I/O accelerator command */ > #define IOACCEL1_MAXSGENTRIES 24 > #define IOACCEL2_MAXSGENTRIES 28 I'm seeing this issue as well and without the patch, the kernel won't boot on multiple ia64 servers. Is there anything that speaks against fixing this? Thanks, Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer - glaubitz@debian.org `. `' Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de `- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
-----Original Message----- From: Sergei Trofimovich [mailto:slyfox@gentoo.org] Subject: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment) The failure initially observed as boot failure on rx3600 ia64 machine with RAID bus controller: Hewlett-Packard Company Smart Array P600: kernel unaligned access to 0xe000000105dd8b95, ip=0xa000000100b87551 kernel unaligned access to 0xe000000105dd8e95, ip=0xa000000100b87551 hpsa 0000:14:01.0: Controller reports max supported commands of 0 Using 16 instead. Ensure that firmware is up to date. swapper/0[1]: error during unaligned kernel access Here unaligned access comes from 'struct CommandList' that happens to be packed. The change f749d8b7a ("scsi: hpsa: Correct dev cmds outstanding for retried cmds") introduced unexpected padding and un-aligned atomic_t from natural alignment to something else. This change does not remove packing annotation from struct but only restores alignment of atomic variable. The change is tested on the same rx3600 machine. CC: linux-ia64@vger.kernel.org CC: storagedev@microchip.com CC: linux-scsi@vger.kernel.org CC: Joe Szczypek <jszczype@redhat.com> CC: Scott Benesh <scott.benesh@microchip.com> CC: Scott Teel <scott.teel@microchip.com> CC: Tomas Henzl <thenzl@redhat.com> CC: "Martin K. Petersen" <martin.petersen@oracle.com> CC: Don Brace <don.brace@microchip.com> Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> Suggested-by: Don Brace <don.brace@microchip.com> Fixes: f749d8b7a "scsi: hpsa: Correct dev cmds outstanding for retried cmds" Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> --- drivers/scsi/hpsa_cmd.h | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h index d126bb877250..617bdae9a7de 100644 --- a/drivers/scsi/hpsa_cmd.h +++ b/drivers/scsi/hpsa_cmd.h @@ -20,6 +20,9 @@ #ifndef HPSA_CMD_H #define HPSA_CMD_H +#include <linux/build_bug.h> /* static_assert */ #include +<linux/stddef.h> /* offsetof */ + /* general boundary defintions */ #define SENSEINFOBYTES 32 /* may vary between hbas */ #define SG_ENTRIES_IN_CMD 32 /* Max SG entries excluding chain blocks */ @@ -448,11 +451,20 @@ struct CommandList { */ struct hpsa_scsi_dev_t *phys_disk; - bool retry_pending; + int retry_pending; struct hpsa_scsi_dev_t *device; atomic_t refcount; /* Must be last to avoid memset in hpsa_cmd_init() */ } __aligned(COMMANDLIST_ALIGNMENT); +/* + * Make sure our embedded atomic variable is aligned. Otherwise we +break atomic + * operations on architectures that don't support unaligned atomics like IA64. + * + * Ideally this header should be cleaned up to only mark individual +structs as + * packed. + */ +static_assert(offsetof(struct CommandList, refcount) % +__alignof__(atomic_t) == 0); + /* Max S/G elements in I/O accelerator command */ #define IOACCEL1_MAXSGENTRIES 24 #define IOACCEL2_MAXSGENTRIES 28 -- 2.30.2 Acked-by: Don Brace <don.brace@microchip.com> Thanks for your patch and extra effort.
On Wed, 17 Mar 2021 18:28:31 +0100 John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> wrote: > Hi Sergei! > > On 3/12/21 11:27 PM, Sergei Trofimovich wrote: > > The failure initially observed as boot failure on rx3600 ia64 machine > > with RAID bus controller: Hewlett-Packard Company Smart Array P600: > > > > kernel unaligned access to 0xe000000105dd8b95, ip=0xa000000100b87551 > > kernel unaligned access to 0xe000000105dd8e95, ip=0xa000000100b87551 > > hpsa 0000:14:01.0: Controller reports max supported commands of 0 Using 16 instead. Ensure that firmware is up to date. > > swapper/0[1]: error during unaligned kernel access > > > > Here unaligned access comes from 'struct CommandList' that happens > > to be packed. The change f749d8b7a ("scsi: hpsa: Correct dev cmds > > outstanding for retried cmds") introduced unexpected padding and > > un-aligned atomic_t from natural alignment to something else. > > > > This change does not remove packing annotation from struct but only > > restores alignment of atomic variable. > > > > The change is tested on the same rx3600 machine. > > I just gave it a try on my RX2660 and for me, the hpsa driver won't load even > with your patch. > > Can you share your kernel configuration so I can give it a try? Sure! Here is a config from a few days ago: https://dev.gentoo.org/~slyfox/configs/guppy-config-5.12.0-rc4-00016-g427684abc9fd-dirty -- Sergei
Hi! On 3/24/21 7:37 PM, Don.Brace@microchip.com wrote: > -----Original Message----- > From: Sergei Trofimovich [mailto:slyfox@gentoo.org] > Subject: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment) > > The failure initially observed as boot failure on rx3600 ia64 machine with RAID bus controller: Hewlett-Packard Company Smart Array P600: > > kernel unaligned access to 0xe000000105dd8b95, ip=0xa000000100b87551 > kernel unaligned access to 0xe000000105dd8e95, ip=0xa000000100b87551 > hpsa 0000:14:01.0: Controller reports max supported commands of 0 Using 16 instead. Ensure that firmware is up to date. > swapper/0[1]: error during unaligned kernel access > > Here unaligned access comes from 'struct CommandList' that happens to be packed. The change f749d8b7a ("scsi: hpsa: Correct dev cmds outstanding for retried cmds") introduced unexpected padding and un-aligned atomic_t from natural alignment to something else. > > This change does not remove packing annotation from struct but only restores alignment of atomic variable. > > The change is tested on the same rx3600 machine. > > CC: linux-ia64@vger.kernel.org > CC: storagedev@microchip.com > CC: linux-scsi@vger.kernel.org > CC: Joe Szczypek <jszczype@redhat.com> > CC: Scott Benesh <scott.benesh@microchip.com> > CC: Scott Teel <scott.teel@microchip.com> > CC: Tomas Henzl <thenzl@redhat.com> > CC: "Martin K. Petersen" <martin.petersen@oracle.com> > CC: Don Brace <don.brace@microchip.com> > Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> > Suggested-by: Don Brace <don.brace@microchip.com> > Fixes: f749d8b7a "scsi: hpsa: Correct dev cmds outstanding for retried cmds" > Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> > --- > drivers/scsi/hpsa_cmd.h | 14 +++++++++++++- > 1 file changed, 13 insertions(+), 1 deletion(-) > > diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h index d126bb877250..617bdae9a7de 100644 > --- a/drivers/scsi/hpsa_cmd.h > +++ b/drivers/scsi/hpsa_cmd.h > @@ -20,6 +20,9 @@ > #ifndef HPSA_CMD_H > #define HPSA_CMD_H > > +#include <linux/build_bug.h> /* static_assert */ #include > +<linux/stddef.h> /* offsetof */ > + > /* general boundary defintions */ > #define SENSEINFOBYTES 32 /* may vary between hbas */ > #define SG_ENTRIES_IN_CMD 32 /* Max SG entries excluding chain blocks */ > @@ -448,11 +451,20 @@ struct CommandList { > */ > struct hpsa_scsi_dev_t *phys_disk; > > - bool retry_pending; > + int retry_pending; > struct hpsa_scsi_dev_t *device; > atomic_t refcount; /* Must be last to avoid memset in hpsa_cmd_init() */ } __aligned(COMMANDLIST_ALIGNMENT); > > +/* > + * Make sure our embedded atomic variable is aligned. Otherwise we > +break atomic > + * operations on architectures that don't support unaligned atomics like IA64. > + * > + * Ideally this header should be cleaned up to only mark individual > +structs as > + * packed. > + */ > +static_assert(offsetof(struct CommandList, refcount) % > +__alignof__(atomic_t) == 0); > + > /* Max S/G elements in I/O accelerator command */ > #define IOACCEL1_MAXSGENTRIES 24 > #define IOACCEL2_MAXSGENTRIES 28 > -- > 2.30.2 > > > Acked-by: Don Brace <don.brace@microchip.com> > > Thanks for your patch and extra effort. Apologies for being so persistent, but has this patch been queued anywhere? This should be included for 5.12 if possible as it unbreaks the kernel on alot of Itanium servers (and potentially other machines with the HPSA controller). If no one wants to pick the patch up, it could go through Andrew Morton's tree (-mm). Thanks, Adrian -- .''`. John Paul Adrian Glaubitz : :' : Debian Developer - glaubitz@debian.org `. `' Freie Universitaet Berlin - glaubitz@physik.fu-berlin.de `- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
On Mon, Mar 29, 2021 at 1:28 PM John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> wrote: > On 3/24/21 7:37 PM, Don.Brace@microchip.com wrote: > > > > Acked-by: Don Brace <don.brace@microchip.com> > > > > Thanks for your patch and extra effort. > > Apologies for being so persistent, but has this patch been queued anywhere? > > This should be included for 5.12 if possible as it unbreaks the kernel on alot > of Itanium servers (and potentially other machines with the HPSA controller). > > If no one wants to pick the patch up, it could go through Andrew Morton's tree (-mm). > I think Martin is still waiting for a fixed version of the patch, as the proposed patch from March 12 only solves the immediate symptom, but not the underlying problem of the CommandList structure being marked as unaligned. If it gets fixed, the new version should work on all architectures. Arnd
Arnd, > I think Martin is still waiting for a fixed version of the patch, as > the proposed patch from March 12 only solves the immediate symptom, > but not the underlying problem of the CommandList structure being > marked as unaligned. If it gets fixed, the new version should work on > all architectures. Yep. I unfortunately don't have any hpsa adapters to test with. Was hoping somebody with hardware would attempt to fix up the struct properly. Given -rc5 we're running out of time so for 5.12 it's probably best if we queue up the workaround. I would prefer an amalgamation of Don's and Sergei's patches, though. I do like the assert so we can catch problems early. But really, somebody should fix this. While hpsa may be out of commercial support, Linux will support the driver it until there are no more users. And the current structure packing is just wrong. -- Martin K. Petersen Oracle Linux Engineering
On Tue, Mar 30, 2021 at 9:22 AM Sergei Trofimovich <slyfox@gentoo.org> wrote: > > Some of the structs contain `atomic_t` values and are not intended to be > sent to IO controller as is. > > The change adds __packed to every struct and union in the file. > Follow-up commits will fix `atomic_t` problems. > > The commit is a no-op at least on ia64: > $ diff -u <(objdump -d -r old.o) <(objdump -d -r new.o) This looks better to me, but I think it still has the same potential bug in the CommandList definition. Moving from #pragma to annotating the misaligned structures as __packed is more of a cleanup that could be done separately from the bugfix, but it does make it a little more robust. > #define HPSA_INQUIRY 0x12 > struct InquiryData { > u8 data_byte[36]; > -}; > +} __packed; Marking this one and a few others as __packed is a bit silly, but also obviously harmless, and closer to the original version, so that's ok. > @@ -451,7 +452,7 @@ struct CommandList { > bool retry_pending; > struct hpsa_scsi_dev_t *device; > atomic_t refcount; /* Must be last to avoid memset in hpsa_cmd_init() */ > -} __aligned(COMMANDLIST_ALIGNMENT); > +} __packed __aligned(COMMANDLIST_ALIGNMENT); You are still marking CommandList as __packed here, which is what caused the original problem. Please don't mark this one as __packed at all. If there are individual members that you want to be misaligned inside of the structure, you could mark those explicitly. Arnd
On Tue, Mar 30, 2021 at 9:23 AM Sergei Trofimovich <slyfox@gentoo.org> wrote: > +/* > + * Make sure our embedded atomic variable is aligned. Otherwise we break atomic > + * operations on architectures that don't support unaligned atomics like IA64. > + * > + * The assert guards against reintroductin against unwanted __packed to > + * the struct CommandList. > + */ > +static_assert(offsetof(struct CommandList, refcount) % __alignof__(atomic_t) == 0); > + There are a few other members that need to be aligned: the work_struct has another atomic_t inside it, and there are a few pointers that might rely on being written to atomically. While you could add a static_assert for each member, the easier solution is to just not ask for the members to be misaligned in the first place. Arnd
On Tue, Mar 30, 2021 at 9:30 AM Arnd Bergmann <arnd@kernel.org> wrote: > On Tue, Mar 30, 2021 at 9:22 AM Sergei Trofimovich <slyfox@gentoo.org> wrote: > > > @@ -451,7 +452,7 @@ struct CommandList { > > bool retry_pending; > > struct hpsa_scsi_dev_t *device; > > atomic_t refcount; /* Must be last to avoid memset in hpsa_cmd_init() */ > > -} __aligned(COMMANDLIST_ALIGNMENT); > > +} __packed __aligned(COMMANDLIST_ALIGNMENT); > > You are still marking CommandList as __packed here, which is > what caused the original problem. Please don't mark this one > as __packed at all. If there are individual members that you want > to be misaligned inside of the structure, you could mark those > explicitly. Nevermind, I just got patch 2/3, splitting up the patches like this seems fine to me. Whole series Reviewed-by: Arnd Bergmann <arnd@arndb.de>
It looks like ia64 implements atomic_t as a 64-bit value and expects atomic_t
to be 64-bit aligned, but does nothing to ensure that.
For x86, atomic_t is a 32-bit value and atomic64_t is a 64-bit value, and
the definition of atomic64_t is overridden in a way that ensures
64-bit (8 byte) alignment:
Generic definitions are in include/linux/types.h:
typedef struct {
int counter;
} atomic_t;
#define ATOMIC_INIT(i) { (i) }
#ifdef CONFIG_64BIT
typedef struct {
s64 counter;
} atomic64_t;
#endif
Override in arch/x86/include/asm/atomic64_32.h:
typedef struct {
s64 __aligned(8) counter;
} atomic64_t;
Perhaps ia64 needs to take over the definition of both atomic_t and atomic64_t
and do the same?
On Fri, 2 Apr 2021 14:40:39 +0000 "Elliott, Robert (Servers)" <elliott@hpe.com> wrote: > It looks like ia64 implements atomic_t as a 64-bit value and expects atomic_t > to be 64-bit aligned, but does nothing to ensure that. > > For x86, atomic_t is a 32-bit value and atomic64_t is a 64-bit value, and > the definition of atomic64_t is overridden in a way that ensures > 64-bit (8 byte) alignment: > > Generic definitions are in include/linux/types.h: > typedef struct { > int counter; > } atomic_t; > > #define ATOMIC_INIT(i) { (i) } > > #ifdef CONFIG_64BIT > typedef struct { > s64 counter; > } atomic64_t; > #endif > > Override in arch/x86/include/asm/atomic64_32.h: > typedef struct { > s64 __aligned(8) counter; > } atomic64_t; > > Perhaps ia64 needs to take over the definition of both atomic_t and atomic64_t > and do the same? I don't think it's needed. ia64 is a 64-bit arch with expected natural alignment for s64: alignof(s64)=8. Also if my understanding is correct adding __aligned(8) would not fix use case of embedding locks into packed structs even on x86_64 (or i386): $ cat a.c #include <stdio.h> #include <stddef.h> typedef struct { unsigned long long __attribute__((aligned(8))) l; } lock_t; struct s { char c; lock_t lock; } __attribute__((packed)); int main() { printf ("offsetof(struct s, lock) = %lu\nsizeof(struct s) = %lu\n", offsetof(struct s, lock), sizeof(struct s)); } $ x86_64-pc-linux-gnu-gcc a.c -o a && ./a offsetof(struct s, lock) = 1 sizeof(struct s) = 9 $ x86_64-pc-linux-gnu-gcc a.c -o a -m32 && ./a offsetof(struct s, lock) = 1 sizeof(struct s) = 9 Note how alignment of 'lock' stays 1 byte in both cases. 8-byte alignment added for i386 in https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=bbf2a330d92c5afccfd17592ba9ccd50f41cf748 is only as a performance optimization (not a correctness fix). Larger alignment on i386 is preferred because alignof(s64)=4 on that target which might make atomic op span cache lines that leads to performance degradation. -- Sergei
diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h index d126bb877250..617bdae9a7de 100644 --- a/drivers/scsi/hpsa_cmd.h +++ b/drivers/scsi/hpsa_cmd.h @@ -20,6 +20,9 @@ #ifndef HPSA_CMD_H #define HPSA_CMD_H +#include <linux/build_bug.h> /* static_assert */ +#include <linux/stddef.h> /* offsetof */ + /* general boundary defintions */ #define SENSEINFOBYTES 32 /* may vary between hbas */ #define SG_ENTRIES_IN_CMD 32 /* Max SG entries excluding chain blocks */ @@ -448,11 +451,20 @@ struct CommandList { */ struct hpsa_scsi_dev_t *phys_disk; - bool retry_pending; + int retry_pending; struct hpsa_scsi_dev_t *device; atomic_t refcount; /* Must be last to avoid memset in hpsa_cmd_init() */ } __aligned(COMMANDLIST_ALIGNMENT); +/* + * Make sure our embedded atomic variable is aligned. Otherwise we break atomic + * operations on architectures that don't support unaligned atomics like IA64. + * + * Ideally this header should be cleaned up to only mark individual structs as + * packed. + */ +static_assert(offsetof(struct CommandList, refcount) % __alignof__(atomic_t) == 0); + /* Max S/G elements in I/O accelerator command */ #define IOACCEL1_MAXSGENTRIES 24 #define IOACCEL2_MAXSGENTRIES 28
The failure initially observed as boot failure on rx3600 ia64 machine with RAID bus controller: Hewlett-Packard Company Smart Array P600: kernel unaligned access to 0xe000000105dd8b95, ip=0xa000000100b87551 kernel unaligned access to 0xe000000105dd8e95, ip=0xa000000100b87551 hpsa 0000:14:01.0: Controller reports max supported commands of 0 Using 16 instead. Ensure that firmware is up to date. swapper/0[1]: error during unaligned kernel access Here unaligned access comes from 'struct CommandList' that happens to be packed. The change f749d8b7a ("scsi: hpsa: Correct dev cmds outstanding for retried cmds") introduced unexpected padding and un-aligned atomic_t from natural alignment to something else. This change does not remove packing annotation from struct but only restores alignment of atomic variable. The change is tested on the same rx3600 machine. CC: linux-ia64@vger.kernel.org CC: storagedev@microchip.com CC: linux-scsi@vger.kernel.org CC: Joe Szczypek <jszczype@redhat.com> CC: Scott Benesh <scott.benesh@microchip.com> CC: Scott Teel <scott.teel@microchip.com> CC: Tomas Henzl <thenzl@redhat.com> CC: "Martin K. Petersen" <martin.petersen@oracle.com> CC: Don Brace <don.brace@microchip.com> Reported-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> Suggested-by: Don Brace <don.brace@microchip.com> Fixes: f749d8b7a "scsi: hpsa: Correct dev cmds outstanding for retried cmds" Signed-off-by: Sergei Trofimovich <slyfox@gentoo.org> --- drivers/scsi/hpsa_cmd.h | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-)