Hi Sergei!
On 3/2/21 11:26 PM, Sergei Trofimovich wrote:
> Gave v5.12-rc1 a try today and got a similar boot failure around
> hpsa queue initialization, but my failure is later:
> https://dev.gentoo.org/~slyfox/configs/guppy-dmesg-5.12-rc1
> Maybe I get different error because I flipped on most debugging
> kernel options :)
>
> Looks like 'ERROR: Invalid distance value range' while being
> very scary are harmless. It's just a new spammy way for kernel
> to report lack of NUMA config on the machine (no SRAT and SLIT
> ACPI tables).
>
> At least I get hpsa detected on PCI bus. But I guess it's discovered
> configuration is very wrong as I get unaligned accesses:
> [ 19.811570] kernel unaligned access to 0xe000000105dd8295, ip=0xa000000100b874d1
>
> Bisecting now.
Sounds good. I guess we should get Jens' fix for the signal regression
merged as well as your two fixes for strace.
Adrian
--
.''`. John Paul Adrian Glaubitz
: :' : Debian Developer - [email protected]
`. `' Freie Universitaet Berlin - [email protected]
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
On Tue, 2 Mar 2021 23:31:32 +0100
John Paul Adrian Glaubitz <[email protected]> wrote:
> Hi Sergei!
>
> On 3/2/21 11:26 PM, Sergei Trofimovich wrote:
> > Gave v5.12-rc1 a try today and got a similar boot failure around
> > hpsa queue initialization, but my failure is later:
> > https://dev.gentoo.org/~slyfox/configs/guppy-dmesg-5.12-rc1
> > Maybe I get different error because I flipped on most debugging
> > kernel options :)
> >
> > Looks like 'ERROR: Invalid distance value range' while being
> > very scary are harmless. It's just a new spammy way for kernel
> > to report lack of NUMA config on the machine (no SRAT and SLIT
> > ACPI tables).
> >
> > At least I get hpsa detected on PCI bus. But I guess it's discovered
> > configuration is very wrong as I get unaligned accesses:
> > [ 19.811570] kernel unaligned access to 0xe000000105dd8295, ip=0xa000000100b874d1
> >
> > Bisecting now.
>
> Sounds good. I guess we should get Jens' fix for the signal regression
> merged as well as your two fixes for strace.
"bisected" (cheated halfway through) and verified that reverting
f749d8b7a9896bc6e5ffe104cc64345037e0b152 makes rx3600 boot again.
CCing authors who might be able to help us here.
commit f749d8b7a9896bc6e5ffe104cc64345037e0b152
Author: Don Brace <[email protected]>
Date: Mon Feb 15 16:26:57 2021 -0600
scsi: hpsa: Correct dev cmds outstanding for retried cmds
Prevent incrementing device->commands_outstanding for ioaccel command
retries that are driver initiated. If the command goes through the retry
path, the device->commands_outstanding counter has already accounted for
the number of commands outstanding to the device. Only commands going
through function hpsa_cmd_resolve_events decrement this counter.
- ioaccel commands go to either HBA disks or to logical volumes comprised
of SSDs.
The extra increment is causing device resets to hang.
- Resets wait for all device outstanding commands to complete before
returning.
Replace unused field abort_pending with retry_pending. This is a
maintenance driver so these changes have the least impact/risk.
Link: https://lore.kernel.org/r/161342801747.29388.13045495968308188518.stgit@brunhilda
Tested-by: Joe Szczypek <[email protected]>
Reviewed-by: Scott Benesh <[email protected]>
Reviewed-by: Scott Teel <[email protected]>
Reviewed-by: Tomas Henzl <[email protected]>
Signed-off-by: Don Brace <[email protected]>
Signed-off-by: Martin K. Petersen <[email protected]>
Don, do you happen to know why this patch caused some controller init failure
for device
14:01.0 RAID bus controller: Hewlett-Packard Company Smart Array P600
?
Boot failure: https://dev.gentoo.org/~slyfox/configs/guppy-dmesg-5.12-rc1
Boot success: https://dev.gentoo.org/~slyfox/configs/guppy-dmesg-5.12-rc1-good
The difference between the two boots is
f749d8b7a9896bc6e5ffe104cc64345037e0b152 reverted on top of 5.12-rc1
in -good case.
Looks like hpsa controller fails to initialize in bad case (could be a race?).
--
Sergei
Hi Don,
On Fri, Mar 5, 2021 at 12:26 AM <[email protected]> wrote:
> -----Original Message-----
> From: Sergei Trofimovich [mailto:[email protected]]
> Sent: Wednesday, March 3, 2021 2:56 AM
> To: John Paul Adrian Glaubitz <[email protected]>; Don Brace - C33706 <[email protected]>; storagedev <[email protected]>; [email protected]
> Cc: [email protected]; [email protected]; Joe Szczypek <[email protected]>; Scott Benesh - C33703 <[email protected]>; Scott Teel - C33730 <[email protected]>; Tomas Henzl <[email protected]>; Martin K. Petersen <[email protected]>
> Subject: Re: [bisected] 5.12-rc1 hpsa regression: "scsi: hpsa: Correct dev cmds outstanding for retried cmds" breaks hpsa P600
>
> EXTERNAL EMAIL: Do not click links or open attachments unless you know the content is safe
>
> On Wed, 3 Mar 2021 00:22:36 +0000
> Sergei Trofimovich <[email protected]> wrote:
>
> > On Tue, 2 Mar 2021 23:31:32 +0100
> > John Paul Adrian Glaubitz <[email protected]> wrote:
> >
> > > Hi Sergei!
> > >
> > > On 3/2/21 11:26 PM, Sergei Trofimovich wrote:
> > > > Gave v5.12-rc1 a try today and got a similar boot failure around
> > > > hpsa queue initialization, but my failure is later:
> > > > https://dev.gentoo.org/~slyfox/configs/guppy-dmesg-5.12-rc1
> > > > Maybe I get different error because I flipped on most debugging
> > > > kernel options :)
> > > >
> > > > Looks like 'ERROR: Invalid distance value range' while being very
> > > > scary are harmless. It's just a new spammy way for kernel to
> > > > report lack of NUMA config on the machine (no SRAT and SLIT ACPI
> > > > tables).
> > > >
> > > > At least I get hpsa detected on PCI bus. But I guess it's
> > > > discovered configuration is very wrong as I get unaligned accesses:
> > > > [ 19.811570] kernel unaligned access to 0xe000000105dd8295, ip=0xa000000100b874d1
>
> Running pahole before the patch:
>
> struct CommandList {
> struct CommandListHeader Header; /* 0 20 */
> struct RequestBlock Request; /* 20 20 */
> struct ErrDescriptor ErrDesc; /* 40 12 */
> struct SGDescriptor SG[32]; /* 52 512 */
> /* --- cacheline 8 boundary (512 bytes) was 52 bytes ago --- */
> u32 busaddr; /* 564 4 */
> struct ErrorInfo * err_info; /* 568 8 */
> /* --- cacheline 9 boundary (576 bytes) --- */
> struct ctlr_info * h; /* 576 8 */
> int cmd_type; /* 584 4 */
> long int cmdindex; /* 588 8 */
> struct completion * waiting; /* 596 8 */
> struct scsi_cmnd * scsi_cmd; /* 604 8 */
> struct work_struct work; /* 612 32 */
> /* --- cacheline 10 boundary (640 bytes) was 4 bytes ago --- */
> struct hpsa_scsi_dev_t * phys_disk; /* 644 8 */
> int abort_pending; /* 652 4 */
> struct hpsa_scsi_dev_t * device; /* 656 8 */
> atomic_t refcount; /* 664 4 */
>
> /* size: 768, cachelines: 12, members: 16 */
> /* padding: 100 */
> } __attribute__((__aligned__(128)));
>
> Pahole after the patch:
>
> struct CommandList {
> struct CommandListHeader Header; /* 0 20 */
> struct RequestBlock Request; /* 20 20 */
> struct ErrDescriptor ErrDesc; /* 40 12 */
> struct SGDescriptor SG[32]; /* 52 512 */
> /* --- cacheline 8 boundary (512 bytes) was 52 bytes ago --- */
> u32 busaddr; /* 564 4 */
> struct ErrorInfo * err_info; /* 568 8 */
> /* --- cacheline 9 boundary (576 bytes) --- */
> struct ctlr_info * h; /* 576 8 */
> int cmd_type; /* 584 4 */
> long int cmdindex; /* 588 8 */
> struct completion * waiting; /* 596 8 */
> struct scsi_cmnd * scsi_cmd; /* 604 8 */
> struct work_struct work; /* 612 32 */
> /* --- cacheline 10 boundary (640 bytes) was 4 bytes ago --- */
> struct hpsa_scsi_dev_t * phys_disk; /* 644 8 */
> struct hpsa_scsi_dev_t * device; /* 652 8 */
> bool retry_pending; /* 660 1 */
> atomic_t refcount; /* 661 4 */
How come this atomic_t is no longer aligned to its natural alignment?
>
> /* size: 768, cachelines: 12, members: 16 */
> /* padding: 103 */
> } __attribute__((__aligned__(128)));
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
On 3/4/21 6:00 PM, [email protected] wrote:
> -----Original Message-----
> From: Sergei Trofimovich [mailto:[email protected]]
> Sent: Wednesday, March 3, 2021 4:04 PM
> To: Don Brace - C33706 <[email protected]>
> Cc: [email protected]; storagedev <[email protected]>; [email protected]; [email protected]; [email protected]; [email protected]; Scott Benesh - C33703 <[email protected]>; Scott Teel - C33730 <[email protected]>; [email protected]; [email protected]
> Subject: Re: [bisected] 5.12-rc1 hpsa regression: "scsi: hpsa: Correct dev cmds outstanding for retried cmds" breaks hpsa P600
>
> Glad to hear the patch works.
> The P600 is an unsupported controller that was removed some time ago (by us) and re-added in this patch:
> commit 135ae6edeb51979d0998daf1357f149a7d6ebb08 scsi: hpsa: add support for legacy boards
> Author: Hannes Reinecke <[email protected]>
> Date: Tue Aug 15 08:58:04 2017 +0200
>
> So, when testing the original patch, I no longer have a P600 to test with. It used to be supported by our obsoleted cciss driver.
Do you know why this is specific to P600, since the struct is used only
internally in the driver and not used to communicate with the controller
or am I wrong?
>
> Since patch f749d8b7a9896bc6e5ffe104cc64345037e0b152 scsi: hpsa: Correct dev cmds outstanding for retried cmds has
> already been applied to Martin Petersons 5.13/scsi-queue, perhaps it would be better to send up another patch to correct your alignment issue on these legacy boards.
>
> Wondering what others think about this?
I agree with you I'd prefer a new patch.
Thanks,
Tomas
>
> Thanks,
> Don
>
On Fri, Mar 5, 2021 at 10:24 AM Geert Uytterhoeven <[email protected]> wrote:
> On Fri, Mar 5, 2021 at 12:26 AM <[email protected]> wrote:
> > > > On 3/2/21 11:26 PM, Sergei Trofimovich wrote:
> > struct CommandList {
> > struct CommandListHeader Header; /* 0 20 */
> > struct RequestBlock Request; /* 20 20 */
> > struct ErrDescriptor ErrDesc; /* 40 12 */
> > struct SGDescriptor SG[32]; /* 52 512 */
> > /* --- cacheline 8 boundary (512 bytes) was 52 bytes ago --- */
> > u32 busaddr; /* 564 4 */
> > struct ErrorInfo * err_info; /* 568 8 */
> > /* --- cacheline 9 boundary (576 bytes) --- */
> > struct ctlr_info * h; /* 576 8 */
> > int cmd_type; /* 584 4 */
> > long int cmdindex; /* 588 8 */
> > struct completion * waiting; /* 596 8 */
> > struct scsi_cmnd * scsi_cmd; /* 604 8 */
> > struct work_struct work; /* 612 32 */
> > /* --- cacheline 10 boundary (640 bytes) was 4 bytes ago --- */
> > struct hpsa_scsi_dev_t * phys_disk; /* 644 8 */
> > struct hpsa_scsi_dev_t * device; /* 652 8 */
> > bool retry_pending; /* 660 1 */
> > atomic_t refcount; /* 661 4 */
>
> How come this atomic_t is no longer aligned to its natural alignment?
There is a
#pragma pack(1)
in linux 203 of this file!
It looks like some of the members in struct raid_map_data
and struct CommandListHeader need to be annotated as packed,
but the file accidentally packs everything until the '#pragma pack()'
in line 875, including the kernel-side CommandList data structure
that clearly must not be packed.
Arnd
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: [email protected]
CC: [email protected]
CC: [email protected]
CC: Joe Szczypek <[email protected]>
CC: Scott Benesh <[email protected]>
CC: Scott Teel <[email protected]>
CC: Tomas Henzl <[email protected]>
CC: "Martin K. Petersen" <[email protected]>
CC: Don Brace <[email protected]>
Reported-by: John Paul Adrian Glaubitz <[email protected]>
Suggested-by: Don Brace <[email protected]>
Fixes: f749d8b7a "scsi: hpsa: Correct dev cmds outstanding for retried cmds"
Signed-off-by: Sergei Trofimovich <[email protected]>
---
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
-----Original Message-----
From: Sergei Trofimovich [mailto:[email protected]]
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: [email protected]
CC: [email protected]
CC: [email protected]
CC: Joe Szczypek <[email protected]>
CC: Scott Benesh <[email protected]>
CC: Scott Teel <[email protected]>
CC: Tomas Henzl <[email protected]>
CC: "Martin K. Petersen" <[email protected]>
CC: Don Brace <[email protected]>
Reported-by: John Paul Adrian Glaubitz <[email protected]>
Suggested-by: Don Brace <[email protected]>
Fixes: f749d8b7a "scsi: hpsa: Correct dev cmds outstanding for retried cmds"
Signed-off-by: Sergei Trofimovich <[email protected]>
---
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 <[email protected]> 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 - [email protected]
`. `' Freie Universitaet Berlin - [email protected]
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
-----Original Message-----
From: David Laight [mailto:[email protected]]
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: [email protected]
> CC: [email protected]
> CC: [email protected]
> CC: Joe Szczypek <[email protected]>
> CC: Scott Benesh <[email protected]>
> CC: Scott Teel <[email protected]>
> CC: Tomas Henzl <[email protected]>
> CC: "Martin K. Petersen" <[email protected]>
> CC: Don Brace <[email protected]>
> Reported-by: John Paul Adrian Glaubitz <[email protected]>
> Suggested-by: Don Brace <[email protected]>
> Fixes: f749d8b7a "scsi: hpsa: Correct dev cmds outstanding for retried cmds"
> Signed-off-by: Sergei Trofimovich <[email protected]>
> ---
> 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 - [email protected]
`. `' Freie Universitaet Berlin - [email protected]
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
-----Original Message-----
From: Sergei Trofimovich [mailto:[email protected]]
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: [email protected]
CC: [email protected]
CC: [email protected]
CC: Joe Szczypek <[email protected]>
CC: Scott Benesh <[email protected]>
CC: Scott Teel <[email protected]>
CC: Tomas Henzl <[email protected]>
CC: "Martin K. Petersen" <[email protected]>
CC: Don Brace <[email protected]>
Reported-by: John Paul Adrian Glaubitz <[email protected]>
Suggested-by: Don Brace <[email protected]>
Fixes: f749d8b7a "scsi: hpsa: Correct dev cmds outstanding for retried cmds"
Signed-off-by: Sergei Trofimovich <[email protected]>
---
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 <[email protected]>
Thanks for your patch and extra effort.
On Wed, 17 Mar 2021 18:28:31 +0100
John Paul Adrian Glaubitz <[email protected]> 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, [email protected] wrote:
> -----Original Message-----
> From: Sergei Trofimovich [mailto:[email protected]]
> 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: [email protected]
> CC: [email protected]
> CC: [email protected]
> CC: Joe Szczypek <[email protected]>
> CC: Scott Benesh <[email protected]>
> CC: Scott Teel <[email protected]>
> CC: Tomas Henzl <[email protected]>
> CC: "Martin K. Petersen" <[email protected]>
> CC: Don Brace <[email protected]>
> Reported-by: John Paul Adrian Glaubitz <[email protected]>
> Suggested-by: Don Brace <[email protected]>
> Fixes: f749d8b7a "scsi: hpsa: Correct dev cmds outstanding for retried cmds"
> Signed-off-by: Sergei Trofimovich <[email protected]>
> ---
> 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 <[email protected]>
>
> 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 - [email protected]
`. `' Freie Universitaet Berlin - [email protected]
`- GPG: 62FF 8A75 84E0 2956 9546 0006 7426 3B37 F5B5 F913
On Mon, Mar 29, 2021 at 1:28 PM John Paul Adrian Glaubitz
<[email protected]> wrote:
> On 3/24/21 7:37 PM, [email protected] wrote:
> >
> > Acked-by: Don Brace <[email protected]>
> >
> > 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
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)
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: Joe Szczypek <[email protected]>
CC: Scott Benesh <[email protected]>
CC: Scott Teel <[email protected]>
CC: Tomas Henzl <[email protected]>
CC: "Martin K. Petersen" <[email protected]>
CC: Don Brace <[email protected]>
Reported-by: John Paul Adrian Glaubitz <[email protected]>
Suggested-by: Don Brace <[email protected]>
Fixes: f749d8b7a "scsi: hpsa: Correct dev cmds outstanding for retried cmds"
Signed-off-by: Sergei Trofimovich <[email protected]>
---
drivers/scsi/hpsa_cmd.h | 68 ++++++++++++++++++++---------------------
1 file changed, 34 insertions(+), 34 deletions(-)
diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h
index d126bb877250..280e933d27e7 100644
--- a/drivers/scsi/hpsa_cmd.h
+++ b/drivers/scsi/hpsa_cmd.h
@@ -20,6 +20,8 @@
#ifndef HPSA_CMD_H
#define HPSA_CMD_H
+#include <linux/compiler.h>
+
/* general boundary defintions */
#define SENSEINFOBYTES 32 /* may vary between hbas */
#define SG_ENTRIES_IN_CMD 32 /* Max SG entries excluding chain blocks */
@@ -200,12 +202,10 @@ union u64bit {
MAX_EXT_TARGETS + 1) /* + 1 is for the controller itself */
/* SCSI-3 Commands */
-#pragma pack(1)
-
#define HPSA_INQUIRY 0x12
struct InquiryData {
u8 data_byte[36];
-};
+} __packed;
#define HPSA_REPORT_LOG 0xc2 /* Report Logical LUNs */
#define HPSA_REPORT_PHYS 0xc3 /* Report Physical LUNs */
@@ -221,7 +221,7 @@ struct raid_map_disk_data {
u8 xor_mult[2]; /**< XOR multipliers for this position,
* valid for data disks only */
u8 reserved[2];
-};
+} __packed;
struct raid_map_data {
__le32 structure_size; /* Size of entire structure in bytes */
@@ -247,14 +247,14 @@ struct raid_map_data {
__le16 dekindex; /* Data encryption key index. */
u8 reserved[16];
struct raid_map_disk_data data[RAID_MAP_MAX_ENTRIES];
-};
+} __packed;
struct ReportLUNdata {
u8 LUNListLength[4];
u8 extended_response_flag;
u8 reserved[3];
u8 LUN[HPSA_MAX_LUN][8];
-};
+} __packed;
struct ext_report_lun_entry {
u8 lunid[8];
@@ -269,20 +269,20 @@ struct ext_report_lun_entry {
u8 lun_count; /* multi-lun device, how many luns */
u8 redundant_paths;
u32 ioaccel_handle; /* ioaccel1 only uses lower 16 bits */
-};
+} __packed;
struct ReportExtendedLUNdata {
u8 LUNListLength[4];
u8 extended_response_flag;
u8 reserved[3];
struct ext_report_lun_entry LUN[HPSA_MAX_PHYS_LUN];
-};
+} __packed;
struct SenseSubsystem_info {
u8 reserved[36];
u8 portname[8];
u8 reserved1[1108];
-};
+} __packed;
/* BMIC commands */
#define BMIC_READ 0x26
@@ -317,7 +317,7 @@ union SCSI3Addr {
u8 Targ:6;
u8 Mode:2; /* b10 */
} LogUnit;
-};
+} __packed;
struct PhysDevAddr {
u32 TargetId:24;
@@ -325,20 +325,20 @@ struct PhysDevAddr {
u32 Mode:2;
/* 2 level target device addr */
union SCSI3Addr Target[2];
-};
+} __packed;
struct LogDevAddr {
u32 VolId:30;
u32 Mode:2;
u8 reserved[4];
-};
+} __packed;
union LUNAddr {
u8 LunAddrBytes[8];
union SCSI3Addr SCSI3Lun[4];
struct PhysDevAddr PhysDev;
struct LogDevAddr LogDev;
-};
+} __packed;
struct CommandListHeader {
u8 ReplyQueue;
@@ -346,7 +346,7 @@ struct CommandListHeader {
__le16 SGTotal;
__le64 tag;
union LUNAddr LUN;
-};
+} __packed;
struct RequestBlock {
u8 CDBLen;
@@ -365,18 +365,18 @@ struct RequestBlock {
#define GET_DIR(tad) (((tad) >> 6) & 0x03)
u16 Timeout;
u8 CDB[16];
-};
+} __packed;
struct ErrDescriptor {
__le64 Addr;
__le32 Len;
-};
+} __packed;
struct SGDescriptor {
__le64 Addr;
__le32 Len;
__le32 Ext;
-};
+} __packed;
union MoreErrInfo {
struct {
@@ -390,7 +390,8 @@ union MoreErrInfo {
u8 offense_num; /* byte # of offense 0-base */
u32 offense_value;
} Invalid_Cmd;
-};
+} __packed;
+
struct ErrorInfo {
u8 ScsiStatus;
u8 SenseLen;
@@ -398,7 +399,7 @@ struct ErrorInfo {
u32 ResidualCnt;
union MoreErrInfo MoreErrInfo;
u8 SenseInfo[SENSEINFOBYTES];
-};
+} __packed;
/* Command types */
#define CMD_IOCTL_PEND 0x01
#define CMD_SCSI 0x03
@@ -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);
/* Max S/G elements in I/O accelerator command */
#define IOACCEL1_MAXSGENTRIES 24
@@ -489,7 +490,7 @@ struct io_accel1_cmd {
__le64 host_addr; /* 0x70 - 0x77 */
u8 CISS_LUN[8]; /* 0x78 - 0x7F */
struct SGDescriptor SG[IOACCEL1_MAXSGENTRIES];
-} __aligned(IOACCEL1_COMMANDLIST_ALIGNMENT);
+} __packed __aligned(IOACCEL1_COMMANDLIST_ALIGNMENT);
#define IOACCEL1_FUNCTION_SCSIIO 0x00
#define IOACCEL1_SGLOFFSET 32
@@ -519,7 +520,7 @@ struct ioaccel2_sg_element {
u8 chain_indicator;
#define IOACCEL2_CHAIN 0x80
#define IOACCEL2_LAST_SG 0x40
-};
+} __packed;
/*
* SCSI Response Format structure for IO Accelerator Mode 2
@@ -559,7 +560,7 @@ struct io_accel2_scsi_response {
u8 sense_data_len; /* sense/response data length */
u8 resid_cnt[4]; /* residual count */
u8 sense_data_buff[32]; /* sense/response data buffer */
-};
+} __packed;
/*
* Structure for I/O accelerator (mode 2 or m2) commands.
@@ -592,7 +593,7 @@ struct io_accel2_cmd {
__le32 tweak_upper; /* Encryption tweak, upper 4 bytes */
struct ioaccel2_sg_element sg[IOACCEL2_MAXSGENTRIES];
struct io_accel2_scsi_response error_data;
-} __aligned(IOACCEL2_COMMANDLIST_ALIGNMENT);
+} __packed __aligned(IOACCEL2_COMMANDLIST_ALIGNMENT);
/*
* defines for Mode 2 command struct
@@ -618,7 +619,7 @@ struct hpsa_tmf_struct {
__le64 abort_tag; /* cciss tag of SCSI cmd or TMF to abort */
__le64 error_ptr; /* Error Pointer */
__le32 error_len; /* Error Length */
-} __aligned(IOACCEL2_COMMANDLIST_ALIGNMENT);
+} __packed __aligned(IOACCEL2_COMMANDLIST_ALIGNMENT);
/* Configuration Table Structure */
struct HostWrite {
@@ -626,7 +627,7 @@ struct HostWrite {
__le32 command_pool_addr_hi;
__le32 CoalIntDelay;
__le32 CoalIntCount;
-};
+} __packed;
#define SIMPLE_MODE 0x02
#define PERFORMANT_MODE 0x04
@@ -675,7 +676,7 @@ struct CfgTable {
#define HPSA_EVENT_NOTIFY_ACCEL_IO_PATH_STATE_CHANGE (1 << 30)
#define HPSA_EVENT_NOTIFY_ACCEL_IO_PATH_CONFIG_CHANGE (1 << 31)
__le32 clear_event_notify;
-};
+} __packed;
#define NUM_BLOCKFETCH_ENTRIES 8
struct TransTable_struct {
@@ -686,14 +687,14 @@ struct TransTable_struct {
__le32 RepQCtrAddrHigh32;
#define MAX_REPLY_QUEUES 64
struct vals32 RepQAddr[MAX_REPLY_QUEUES];
-};
+} __packed;
struct hpsa_pci_info {
unsigned char bus;
unsigned char dev_fn;
unsigned short domain;
u32 board_id;
-};
+} __packed;
struct bmic_identify_controller {
u8 configured_logical_drive_count; /* offset 0 */
@@ -702,7 +703,7 @@ struct bmic_identify_controller {
u8 pad2[136];
u8 controller_mode; /* offset 292 */
u8 pad3[32];
-};
+} __packed;
struct bmic_identify_physical_device {
@@ -845,7 +846,7 @@ struct bmic_identify_physical_device {
u8 max_link_rate[256];
u8 neg_phys_link_rate[256];
u8 box_conn_name[8];
-} __attribute((aligned(512)));
+} __packed __attribute((aligned(512)));
struct bmic_sense_subsystem_info {
u8 primary_slot_number;
@@ -858,7 +859,7 @@ struct bmic_sense_subsystem_info {
u8 secondary_array_serial_number[32];
u8 secondary_cache_serial_number[32];
u8 pad[332];
-};
+} __packed;
struct bmic_sense_storage_box_params {
u8 reserved[36];
@@ -870,7 +871,6 @@ struct bmic_sense_storage_box_params {
u8 reserver_3[84];
u8 phys_connector[2];
u8 reserved_4[296];
-};
+} __packed;
-#pragma pack()
#endif /* HPSA_CMD_H */
--
2.31.1
CC: [email protected]
CC: [email protected]
CC: [email protected]
CC: Joe Szczypek <[email protected]>
CC: Scott Benesh <[email protected]>
CC: Scott Teel <[email protected]>
CC: Tomas Henzl <[email protected]>
CC: "Martin K. Petersen" <[email protected]>
CC: Don Brace <[email protected]>
Reported-by: John Paul Adrian Glaubitz <[email protected]>
Suggested-by: Don Brace <[email protected]>
Fixes: f749d8b7a "scsi: hpsa: Correct dev cmds outstanding for retried cmds"
Signed-off-by: Sergei Trofimovich <[email protected]>
---
drivers/scsi/hpsa_cmd.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h
index 885b1f1fb20a..ba6a3aa8d954 100644
--- a/drivers/scsi/hpsa_cmd.h
+++ b/drivers/scsi/hpsa_cmd.h
@@ -22,6 +22,9 @@
#include <linux/compiler.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 */
@@ -454,6 +457,15 @@ struct CommandList {
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.
+ *
+ * The assert guards against reintroductin against unwanted __packed to
+ * the struct CommandList.
+ */
+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.31.1
On Tue, Mar 30, 2021 at 9:22 AM Sergei Trofimovich <[email protected]> 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 <[email protected]> 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 <[email protected]> wrote:
> On Tue, Mar 30, 2021 at 9:22 AM Sergei Trofimovich <[email protected]> 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 <[email protected]>
On Tue, 30 Mar 2021 08:19:56 +0100, Sergei Trofimovich 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)
Applied to 5.12/scsi-fixes, thanks!
[1/3] hpsa: use __packed on individual structs, not header-wide
https://git.kernel.org/mkp/scsi/c/5482a9a1a8fd
[2/3] hpsa: fix boot on ia64 (atomic_t alignment)
https://git.kernel.org/mkp/scsi/c/02ec144292bc
[3/3] hpsa: add an assert to prevent from __packed reintroduction
https://git.kernel.org/mkp/scsi/c/e01a00ff62ad
--
Martin K. Petersen Oracle Linux Engineering
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)" <[email protected]> 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
-----Original Message-----
From: Martin K. Petersen Subject: Re: [PATCH v2 1/3] hpsa: use __packed on individual structs, not header-wide
> 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)
Applied to 5.12/scsi-fixes, thanks!
Don: Thank-you for all of your efforts. I appreciate to work that you have done on these patches.
Thanks,
Don Brace
[1/3] hpsa: use __packed on individual structs, not header-wide
https://git.kernel.org/mkp/scsi/c/5482a9a1a8fd
[2/3] hpsa: fix boot on ia64 (atomic_t alignment)
https://git.kernel.org/mkp/scsi/c/02ec144292bc
[3/3] hpsa: add an assert to prevent from __packed reintroduction
https://git.kernel.org/mkp/scsi/c/e01a00ff62ad
--
Martin K. Petersen Oracle Linux Engineering