All of lore.kernel.org
 help / color / mirror / Atom feed
From: Don Brace <don.brace@microchip.com>
To: <Kevin.Barnett@microchip.com>, <scott.teel@microchip.com>,
	<Justin.Lindley@microchip.com>, <scott.benesh@microchip.com>,
	<gerry.morong@microchip.com>, <mahesh.rajashekhara@microchip.com>,
	<hch@infradead.org>, <jejb@linux.vnet.ibm.com>,
	<joseph.szczypek@hpe.com>, <POSWALD@suse.com>
Cc: <linux-scsi@vger.kernel.org>
Subject: [PATCH] hpsa: fix regression issue for old controllers
Date: Wed, 10 Mar 2021 13:06:12 -0600	[thread overview]
Message-ID: <161540317205.18786.5821926127237311408.stgit@brunhilda> (raw)

Fix CommandList alignment issues for old and
unsupported controllers.

This patch fixes the alignment issue reported for patch
'commit f749d8b7a989
     ("scsi: hpsa: Correct dev cmds outstanding for retried cmds")'
reported in https://lkml.org/lkml/2021/3/3/243

Patch 'commit f749d8b7a989
     ("scsi: hpsa: Correct dev cmds outstanding for retried cmds")'
removed an 'int' member field "abort_pending" and replaced it with
a 'bool' field "retry_pending". This was tested and verified both
by me and several others outside of Microchip.
  * It should be noted that the patch changed the alignment of an
    atomic_t shown in my pahole analysis below.

However, when the cciss block driver was removed from the
kernel, some old legacy cciss controllers were added into the
hpsa driver in patch 'commit 135ae6edeb51
     ("scsi: hpsa: add support for legacy boards")'

Changing the field from 'int' to 'bool' caused a command alignment
issue on these legacy controllers (reported on IA64 architectures)
but went unnoticed because we no longer have any functional test
systems supporting these controllers and also these controller
are not included in our out-of-box driver.

This patch corrects the CommandList alignment issue on
the legacy controllers by changing the 'bool' back to
an 'int'. I ran pahole against the original driver, current driver,
and on the driver built using the updated patch to verify
the alignment.

I changed setting retry_pending in the driver accordingly.

Before patch 'f749d8b7a989
     ("scsi: hpsa: Correct dev cmds outstanding for retried cmds")
   /* --- 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)));

Current patch 'f749d8b7a989
     ("scsi: hpsa: Correct dev cmds outstanding for retried cmds")
   /* --- cacheline 10 boundary (640 bytes) was 4 bytes ago --- */
   struct hpsa_scsi_dev_t *   phys_disk;            /*   644     8 */
   bool                       retry_pending;        /*   652     1 */
   struct hpsa_scsi_dev_t *   device;               /*   653     8 */
   atomic_t                   refcount;             /*   661     4 */

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

After proposed patch applied:
   /* --- 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)));

Reported-by: Sergei Trofimovich <slyich@gmail.com>
Tested-by: Sergei Trofimovich <slyich@gmail.com>
Reviewed-by: Scott Benesh <scott.benesh@microchip.com>
Reviewed-by: Mike McGowen <mike.mcgowen@microchip.com>
Signed-off-by: Don Brace <don.brace@microchip.com>
---
 drivers/scsi/hpsa.c     |   10 +++++-----
 drivers/scsi/hpsa_cmd.h |    2 +-
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 38369766511c..96f26e250dae 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -5593,7 +5593,7 @@ static int hpsa_ioaccel_submit(struct ctlr_info *h,
 		c->scsi_cmd = cmd;
 		c->device = dev;
 		if (retry) /* Resubmit but do not increment device->commands_outstanding. */
-			c->retry_pending = true;
+			c->retry_pending = 1;
 		rc = hpsa_scsi_ioaccel_raid_map(h, c);
 		if (rc < 0)     /* scsi_dma_map failed. */
 			rc = SCSI_MLQUEUE_HOST_BUSY;
@@ -5603,7 +5603,7 @@ static int hpsa_ioaccel_submit(struct ctlr_info *h,
 		c->scsi_cmd = cmd;
 		c->device = dev;
 		if (retry) /* Resubmit but do not increment device->commands_outstanding. */
-			c->retry_pending = true;
+			c->retry_pending = 1;
 		rc = hpsa_scsi_ioaccel_direct_map(h, c);
 		if (rc < 0)     /* scsi_dma_map failed. */
 			rc = SCSI_MLQUEUE_HOST_BUSY;
@@ -5661,7 +5661,7 @@ static void hpsa_command_resubmit_worker(struct work_struct *work)
 	 * Note: hpsa_ciss_submit does not zero out the command fields like
 	 *       ioaccel submit does.
 	 */
-	c->retry_pending = true;
+	c->retry_pending = 1;
 	if (hpsa_ciss_submit(c->h, c, cmd, dev)) {
 		/*
 		 * If we get here, it means dma mapping failed. Try
@@ -6169,7 +6169,7 @@ static struct CommandList *cmd_tagged_alloc(struct ctlr_info *h,
 	 * This is a new command obtained from queue_command so
 	 * there have not been any driver initiated retry attempts.
 	 */
-	c->retry_pending = false;
+	c->retry_pending = 0;
 
 	return c;
 }
@@ -6243,7 +6243,7 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h)
 	 * cmd_alloc is for "internal" commands and they are never
 	 * retried.
 	 */
-	c->retry_pending = false;
+	c->retry_pending = 0;
 
 	return c;
 }
diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h
index d126bb877250..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;
 
-	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);


                 reply	other threads:[~2021-03-10 19:06 UTC|newest]

Thread overview: [no followups] expand[flat|nested]  mbox.gz  Atom feed

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=161540317205.18786.5821926127237311408.stgit@brunhilda \
    --to=don.brace@microchip.com \
    --cc=Justin.Lindley@microchip.com \
    --cc=Kevin.Barnett@microchip.com \
    --cc=POSWALD@suse.com \
    --cc=gerry.morong@microchip.com \
    --cc=hch@infradead.org \
    --cc=jejb@linux.vnet.ibm.com \
    --cc=joseph.szczypek@hpe.com \
    --cc=linux-scsi@vger.kernel.org \
    --cc=mahesh.rajashekhara@microchip.com \
    --cc=scott.benesh@microchip.com \
    --cc=scott.teel@microchip.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.