Re: [bisected] 5.12-rc1 hpsa regression: "scsi: hpsa: Correct dev cmds outstanding for retried cmds" breaks hpsa P600

Re: [bisected] 5.12-rc1 hpsa regression: "scsi: hpsa: Correct dev cmds outstanding for retried cmds" breaks hpsa P600

[Sergei Trofimovich]

On Wed, 3 Mar 2021 00:22:36 +0000
Sergei Trofimovich <slyich@gmail.com> wrote:

> On Tue, 2 Mar 2021 23:31:32 +0100
> John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> 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 <don.brace@microchip.com>
> 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 <jszczype@redhat.com>
>     Reviewed-by: Scott Benesh <scott.benesh@microchip.com>
>     Reviewed-by: Scott Teel <scott.teel@microchip.com>
>     Reviewed-by: Tomas Henzl <thenzl@redhat.com>
>     Signed-off-by: Don Brace <don.brace@microchip.com>
>     Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
> 
> 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?).

Also CCing hpsa maintainer mailing lists.

Looking more into the suspect commit
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f749d8b7a9896bc6e5ffe104cc64345037e0b152
it roughly does the:

@@ -448,7 +448,7 @@ struct CommandList {
 	 */
 	struct hpsa_scsi_dev_t *phys_disk;
 
-	int abort_pending;
+	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);
...
@@ -1151,7 +1151,10 @@ static void __enqueue_cmd_and_start_io(struct ctlr_info *h,
 {
        dial_down_lockup_detection_during_fw_flash(h, c);
        atomic_inc(&h->commands_outstanding);
-       if (c->device)
+       /*
+        * Check to see if the command is being retried.
+        */
+       if (c->device && !c->retry_pending)
                atomic_inc(&c->device->commands_outstanding);

But I don't immediately see anything wrong with it.

-- 

  Sergei

RE: [bisected] 5.12-rc1 hpsa regression: "scsi: hpsa: Correct dev cmds outstanding for retried cmds" breaks hpsa P600

[Don.Brace]

[-- Attachment #1: Type: text/plain, Size: 10316 bytes --]

-----Original Message-----
From: Sergei Trofimovich [mailto:slyich@gmail.com] 
Sent: Wednesday, March 3, 2021 2:56 AM
To: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>; Don Brace - C33706 <Don.Brace@microchip.com>; storagedev <storagedev@microchip.com>; linux-scsi@vger.kernel.org
Cc: linux-ia64@vger.kernel.org; linux-kernel@vger.kernel.org; Joe Szczypek <jszczype@redhat.com>; Scott Benesh - C33703 <Scott.Benesh@microchip.com>; Scott Teel - C33730 <Scott.Teel@microchip.com>; Tomas Henzl <thenzl@redhat.com>; Martin K. Petersen <martin.petersen@oracle.com>
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 <slyich@gmail.com> wrote:

> On Tue, 2 Mar 2021 23:31:32 +0100
> John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> 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 */

        /* size: 768, cachelines: 12, members: 16 */
        /* padding: 103 */
} __attribute__((__aligned__(128)));

So, I did replace abort_pending field (an int) with retry_pending (bool)
Can I send you a patch to just rename abort_pending to retry_pending using the same type and position?
It will mean some minor code changes in the driver...

With the above changes, pahole is the same
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                        retry_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)));


> > >
> > > 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 <don.brace@microchip.com>
> 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 <jszczype@redhat.com>
>     Reviewed-by: Scott Benesh <scott.benesh@microchip.com>
>     Reviewed-by: Scott Teel <scott.teel@microchip.com>
>     Reviewed-by: Tomas Henzl <thenzl@redhat.com>
>     Signed-off-by: Don Brace <don.brace@microchip.com>
>     Signed-off-by: Martin K. Petersen <martin.petersen@oracle.com>
>
> 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?).

Also CCing hpsa maintainer mailing lists.

Looking more into the suspect commit
    https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=f749d8b7a9896bc6e5ffe104cc64345037e0b152
it roughly does the:

@@ -448,7 +448,7 @@ struct CommandList {
         */
        struct hpsa_scsi_dev_t *phys_disk;

-       int abort_pending;
+       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); ...
@@ -1151,7 +1151,10 @@ static void __enqueue_cmd_and_start_io(struct ctlr_info *h,  {
        dial_down_lockup_detection_during_fw_flash(h, c);
        atomic_inc(&h->commands_outstanding);
-       if (c->device)
+       /*
+        * Check to see if the command is being retried.
+        */
+       if (c->device && !c->retry_pending)
                atomic_inc(&c->device->commands_outstanding);

But I don't immediately see anything wrong with it.

--

  Sergei

[-- Attachment #2: hpsa-correct-dev-cmd-outstanding-for-retried-cmds-alignment-update --]
[-- Type: application/octet-stream, Size: 3733 bytes --]

commit ea1723092d356dc4d1266942aa132340677e7460
Author: Don Brace <dbrace@redhat.com>
Date:   Wed Mar 3 11:21:59 2021 -0600

    hpsa: correct dev cmd outstanding for retried cmds
    
    Prevent incrementing device->commands_outstanding for
    retried commands.

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index f4d3747cfa0b..43c8f7e25eb5 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -1151,7 +1151,10 @@ static void __enqueue_cmd_and_start_io(struct ctlr_info *h,
 {
 	dial_down_lockup_detection_during_fw_flash(h, c);
 	atomic_inc(&h->commands_outstanding);
-	if (c->device)
+	/*
+	 * Check to see if the command is being retried.
+	 */
+	if (c->device && !c->retry_pending)
 		atomic_inc(&c->device->commands_outstanding);
 
 	reply_queue = h->reply_map[raw_smp_processor_id()];
@@ -5623,6 +5626,16 @@ static void hpsa_command_resubmit_worker(struct work_struct *work)
 		return hpsa_cmd_free_and_done(c->h, c, cmd);
 	}
 
+	/*
+	 * SML retries come in through queue_command and therefore
+	 * go through cmd_tagged_alloc. So a new command.
+	 *
+	 * Set a retry_pending flag for a driver initiated retry attempt
+	 * on an existing command to indicate to not increment the
+	 * device->commands_outstanding when submitted.
+	 */
+	c->retry_pending = 1;
+
 	if (c->cmd_type == CMD_IOACCEL2) {
 		struct ctlr_info *h = c->h;
 		struct io_accel2_cmd *c2 = &h->ioaccel2_cmd_pool[c->cmdindex];
@@ -5646,6 +5659,7 @@ static void hpsa_command_resubmit_worker(struct work_struct *work)
 		}
 	}
 	hpsa_cmd_partial_init(c->h, c->cmdindex, c);
+
 	if (hpsa_ciss_submit(c->h, c, cmd, dev)) {
 		/*
 		 * If we get here, it means dma mapping failed. Try
@@ -5708,6 +5722,10 @@ static int hpsa_scsi_queue_command(struct Scsi_Host *sh, struct scsi_cmnd *cmd)
 	/*
 	 * Call alternate submit routine for I/O accelerated commands.
 	 * Retries always go down the normal I/O path.
+	 * Note: If cmd->retries is non-zero, then this is a SML
+	 *       initiated retry and not a driver initiated retry.
+	 *       This command has been obtained from cmd_tagged_alloc
+	 *       and is therefore a brand-new command.
 	 */
 	if (likely(cmd->retries == 0 &&
 			!blk_rq_is_passthrough(cmd->request) &&
@@ -6107,6 +6125,7 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
  * at init, and managed by cmd_tagged_alloc() and cmd_tagged_free() using the
  * block request tag as an index into a table of entries.  cmd_tagged_free() is
  * the complement, although cmd_free() may be called instead.
+ * This function is only called for new requests from queue_command.
  */
 static struct CommandList *cmd_tagged_alloc(struct ctlr_info *h,
 					    struct scsi_cmnd *scmd)
@@ -6141,6 +6160,11 @@ static struct CommandList *cmd_tagged_alloc(struct ctlr_info *h,
 	}
 
 	atomic_inc(&c->refcount);
+	/*
+	 * This is a new command obtained from queue_command so
+	 * there have not been any driver initiated retry attempts.
+	 */
+	c->retry_pending = 0;
 
 	hpsa_cmd_partial_init(h, idx, c);
 	return c;
@@ -6210,6 +6234,11 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h)
 	}
 	hpsa_cmd_partial_init(h, i, c);
 	c->device = NULL;
+	/*
+	 * cmd_alloc is for "internal" commands and they are never
+	 * retried.
+	 */
+	c->retry_pending = 0;
 	return c;
 }
 
diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h
index 46df2e3ff89b..e86af4e9eef0 100644
--- a/drivers/scsi/hpsa_cmd.h
+++ b/drivers/scsi/hpsa_cmd.h
@@ -448,7 +448,7 @@ struct CommandList {
 	 */
 	struct hpsa_scsi_dev_t *phys_disk;
 
-	int abort_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);

RE: [bisected] 5.12-rc1 hpsa regression: "scsi: hpsa: Correct dev cmds outstanding for retried cmds" breaks hpsa P600

[Don.Brace]

-----Original Message-----
From: Sergei Trofimovich [mailto:slyich@gmail.com] 
Sent: Wednesday, March 3, 2021 4:04 PM
To: Don Brace - C33706 <Don.Brace@microchip.com>
Cc: glaubitz@physik.fu-berlin.de; storagedev <storagedev@microchip.com>; linux-scsi@vger.kernel.org; linux-ia64@vger.kernel.org; linux-kernel@vger.kernel.org; jszczype@redhat.com; Scott Benesh - C33703 <Scott.Benesh@microchip.com>; Scott Teel - C33730 <Scott.Teel@microchip.com>; thenzl@redhat.com; martin.petersen@oracle.com
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 <hare@suse.de>
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.

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?

Thanks,
Don

Re: [bisected] 5.12-rc1 hpsa regression: "scsi: hpsa: Correct dev cmds outstanding for retried cmds" breaks hpsa P600

[Geert Uytterhoeven]

Hi Don,

On Fri, Mar 5, 2021 at 12:26 AM <Don.Brace@microchip.com> wrote:
> -----Original Message-----
> From: Sergei Trofimovich [mailto:slyich@gmail.com]
> Sent: Wednesday, March 3, 2021 2:56 AM
> To: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>; Don Brace - C33706 <Don.Brace@microchip.com>; storagedev <storagedev@microchip.com>; linux-scsi@vger.kernel.org
> Cc: linux-ia64@vger.kernel.org; linux-kernel@vger.kernel.org; Joe Szczypek <jszczype@redhat.com>; Scott Benesh - C33703 <Scott.Benesh@microchip.com>; Scott Teel - C33730 <Scott.Teel@microchip.com>; Tomas Henzl <thenzl@redhat.com>; Martin K. Petersen <martin.petersen@oracle.com>
> 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 <slyich@gmail.com> wrote:
>
> > On Tue, 2 Mar 2021 23:31:32 +0100
> > John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de> 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 -- geert@linux-m68k.org

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

Re: [bisected] 5.12-rc1 hpsa regression: "scsi: hpsa: Correct dev cmds outstanding for retried cmds" breaks hpsa P600

[Tomas Henzl]

On 3/4/21 6:00 PM, Don.Brace@microchip.com wrote:
> -----Original Message-----
> From: Sergei Trofimovich [mailto:slyich@gmail.com] 
> Sent: Wednesday, March 3, 2021 4:04 PM
> To: Don Brace - C33706 <Don.Brace@microchip.com>
> Cc: glaubitz@physik.fu-berlin.de; storagedev <storagedev@microchip.com>; linux-scsi@vger.kernel.org; linux-ia64@vger.kernel.org; linux-kernel@vger.kernel.org; jszczype@redhat.com; Scott Benesh - C33703 <Scott.Benesh@microchip.com>; Scott Teel - C33730 <Scott.Teel@microchip.com>; thenzl@redhat.com; martin.petersen@oracle.com
> 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 <hare@suse.de>
> 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
> 

Re: [bisected] 5.12-rc1 hpsa regression: "scsi: hpsa: Correct dev cmds outstanding for retried cmds" breaks hpsa P600

[Arnd Bergmann]

On Fri, Mar 5, 2021 at 10:24 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, Mar 5, 2021 at 12:26 AM <Don.Brace@microchip.com> 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

RE: [bisected] 5.12-rc1 hpsa regression: "scsi: hpsa: Correct dev cmds outstanding for retried cmds" breaks hpsa P600

[Don.Brace]

-----Original Message-----
From: Arnd Bergmann [mailto:arnd@kernel.org] 
Sent: Friday, March 5, 2021 7:32 AM
Subject: Re: [bisected] 5.12-rc1 hpsa regression: "scsi: hpsa: Correct dev cmds outstanding for retried cmds" breaks hpsa P600

On Fri, Mar 5, 2021 at 10:24 AM Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Fri, Mar 5, 2021 at 12:26 AM <Don.Brace@microchip.com> 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
---
Don:
Thanks for your input. It helps a lot.

The pragma setting predates my taking over the driver.

It's true that there is a section of each command entry that is DMAed from the controller (from start of the CommandList up to busaddr) and the rest is driver housekeeping information. The unsupported controllers seem to be unable to handle the changed alignment. 

I have a patch I'll send up soon to change the alignment back...
        int                        retry_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)));

Since this is a maintenance driver, I would rather not do too much surgery and invoke regression tests (and we no longer support these controllers). I'd rather just send up a patch to correct the issue on these legacy controllers. I have one ready to send up.

Thanks for your observation and your attention.
I'll send up the patch soon.

Don

[PATCH] hpsa: fix boot on ia64 (atomic_t alignment)

[Sergei Trofimovich]

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

RE: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment)

[Don.Brace]

-----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/

Re: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment)

[Arnd Bergmann]

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

Re: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment)

[Martin K. Petersen]

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

RE: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment)

[David Laight]

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)

Re: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment)

[John Paul Adrian Glaubitz]

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

RE: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment)

[Don.Brace]

-----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...

Re: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment)

[John Paul Adrian Glaubitz]

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

RE: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment)

[Don.Brace]

-----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.

Re: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment)

[Sergei Trofimovich]

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

Re: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment)

[John Paul Adrian Glaubitz]

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

Re: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment)

[Arnd Bergmann]

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

Re: [PATCH] hpsa: fix boot on ia64 (atomic_t alignment)

[Martin K. Petersen]

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

[PATCH v2 1/3] hpsa: use __packed on individual structs, not header-wide

[Sergei Trofimovich]

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: 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 | 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

[PATCH v2 2/3] hpsa: fix boot on ia64 (atomic_t alignment)

[Sergei Trofimovich]

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 removes packing annotation from struct not intended to be
sent to controller as is. This restores natural `atomic_t` alignment.

The change is tested on the same rx3600 machine.

CC: linux-ia64@vger.kernel.org
CC: linux-kernel@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 | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h
index 280e933d27e7..885b1f1fb20a 100644
--- a/drivers/scsi/hpsa_cmd.h
+++ b/drivers/scsi/hpsa_cmd.h
@@ -452,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() */
-} __packed __aligned(COMMANDLIST_ALIGNMENT);
+} __aligned(COMMANDLIST_ALIGNMENT);
 
 /* Max S/G elements in I/O accelerator command */
 #define IOACCEL1_MAXSGENTRIES           24
-- 
2.31.1

[PATCH v2 3/3] hpsa: add an assert to prevent from __packed reintroduction

[Sergei Trofimovich]

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 | 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

Re: [PATCH v2 1/3] hpsa: use __packed on individual structs, not header-wide

[Arnd Bergmann]

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

Re: [PATCH v2 3/3] hpsa: add an assert to prevent from __packed reintroduction

[Arnd Bergmann]

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

Re: [PATCH v2 1/3] hpsa: use __packed on individual structs, not header-wide

[Arnd Bergmann]

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>

Re: [PATCH v2 1/3] hpsa: use __packed on individual structs, not header-wide

[Martin K. Petersen]

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

RE: [PATCH v2 3/3] hpsa: add an assert to prevent from __packed reintroduction

[Elliott, Robert (Servers)]

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?

Re: [PATCH v2 3/3] hpsa: add an assert to prevent from __packed reintroduction

[Sergei Trofimovich]

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

RE: [PATCH v2 1/3] hpsa: use __packed on individual structs, not header-wide

[Don.Brace]

-----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