[PATCHv4 0/9] scsi: remove legacy cmd_list implementation

[PATCHv4 0/9] scsi: remove legacy cmd_list implementation

[Hannes Reinecke]

Hi all,

with the switch to blk-mq we have an efficient way of looking up
outstanding commands via blk_mq_rq_busy_iter().
In this patchset the dpt_i2o and aacraid drivers are switched over
to using that function, and the now obsolete cmd_list implemantation
in the SCSI midlayer is removed.

As usual, comments and reviews are welcome.

Changes to v1:
- Fixup kbuild warning

Changes to v2:
- Add scsi_host_busy_iter()
- Include reviews from Christoph

Changes to v3:
- Include reviews from Christoph
- Add midlayer helper to terminate outstanding commands
- Split off aacraid modifcations into several patches

Hannes Reinecke (9):
  dpt_i2o: rename adpt_i2o_to_scsi() to adpt_i2o_scsi_complete()
  scsi: add scsi_host_flush_commands() helper
  dpt_i2o: use scsi_host_flush_commands() to abort outstanding commands
  aacraid: Do not wait for outstanding write commands on
    synchronize_cache
  aacraid: use midlayer helper to terminate outstanding commands
  scsi: add scsi_host_busy_iter()
  aacraid: use scsi_host_busy_iter() in aac_wait_for_io_completion()
  aacraid: use scsi_host_busy_iter() in get_num_of_incomplete_fibs()
  scsi: Remove cmd_list functionality

 drivers/scsi/aacraid/aachba.c   | 76 +-------------------------------------
 drivers/scsi/aacraid/comminit.c | 30 +++++++--------
 drivers/scsi/aacraid/commsup.c  | 24 +-----------
 drivers/scsi/aacraid/linit.c    | 81 +++++++++++++++++++++--------------------
 drivers/scsi/dpt_i2o.c          | 25 ++-----------
 drivers/scsi/dpti.h             |  3 +-
 drivers/scsi/hosts.c            | 57 +++++++++++++++++++++++++++++
 drivers/scsi/scsi.c             | 14 -------
 drivers/scsi/scsi_error.c       |  1 -
 drivers/scsi/scsi_lib.c         | 31 ----------------
 drivers/scsi/scsi_priv.h        |  2 -
 drivers/scsi/scsi_scan.c        |  1 -
 include/scsi/scsi_cmnd.h        |  1 -
 include/scsi/scsi_device.h      |  1 -
 include/scsi/scsi_host.h        |  8 +++-
 15 files changed, 125 insertions(+), 230 deletions(-)

-- 
2.16.4

[PATCH 1/9] dpt_i2o: rename adpt_i2o_to_scsi() to adpt_i2o_scsi_complete()

[Hannes Reinecke]

Rename the badly named function into adpt_i2o_scsi_complete(),
and make it a void function as the return value is never used.
This also fixes a potential use-after free as the return value
might be evaluated from the command result after the command
has been freed.

Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/dpt_i2o.c | 5 ++---
 drivers/scsi/dpti.h    | 2 +-
 2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index abc74fd474dc..c30ace9f251e 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -2173,7 +2173,7 @@ static irqreturn_t adpt_isr(int irq, void *dev_id)
 						 readl(reply + 12) - 1);
 			if(cmd != NULL){
 				scsi_dma_unmap(cmd);
-				adpt_i2o_to_scsi(reply, cmd);
+				adpt_i2o_scsi_complete(reply, cmd);
 			}
 		}
 		writel(m, pHba->reply_port);
@@ -2341,7 +2341,7 @@ static s32 adpt_scsi_host_alloc(adpt_hba* pHba, struct scsi_host_template *sht)
 }
 
 
-static s32 adpt_i2o_to_scsi(void __iomem *reply, struct scsi_cmnd* cmd)
+static void adpt_i2o_scsi_complete(void __iomem *reply, struct scsi_cmnd *cmd)
 {
 	adpt_hba* pHba;
 	u32 hba_status;
@@ -2459,7 +2459,6 @@ static s32 adpt_i2o_to_scsi(void __iomem *reply, struct scsi_cmnd* cmd)
 	if(cmd->scsi_done != NULL){
 		cmd->scsi_done(cmd);
 	} 
-	return cmd->result;
 }
 
 
diff --git a/drivers/scsi/dpti.h b/drivers/scsi/dpti.h
index 42b1e28b5884..3ec391134bb0 100644
--- a/drivers/scsi/dpti.h
+++ b/drivers/scsi/dpti.h
@@ -286,7 +286,7 @@ static s32 adpt_i2o_status_get(adpt_hba* pHba);
 static s32 adpt_i2o_init_outbound_q(adpt_hba* pHba);
 static s32 adpt_i2o_hrt_get(adpt_hba* pHba);
 static s32 adpt_scsi_to_i2o(adpt_hba* pHba, struct scsi_cmnd* cmd, struct adpt_device* dptdevice);
-static s32 adpt_i2o_to_scsi(void __iomem *reply, struct scsi_cmnd* cmd);
+static void adpt_i2o_scsi_complete(void __iomem *reply, struct scsi_cmnd *cmd);
 static s32 adpt_scsi_host_alloc(adpt_hba* pHba,struct scsi_host_template * sht);
 static s32 adpt_hba_reset(adpt_hba* pHba);
 static s32 adpt_i2o_reset_hba(adpt_hba* pHba);
-- 
2.16.4

[PATCH 2/9] scsi: add scsi_host_flush_commands() helper

[Hannes Reinecke]

Add a helper scsi_host_flush_commands() to terminate all outstanding
commands on a scsi host.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/hosts.c     | 24 ++++++++++++++++++++++++
 include/scsi/scsi_host.h |  1 +
 2 files changed, 25 insertions(+)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 1d669e47b692..da1df48ef27c 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -650,3 +650,27 @@ void scsi_flush_work(struct Scsi_Host *shost)
 	flush_workqueue(shost->work_q);
 }
 EXPORT_SYMBOL_GPL(scsi_flush_work);
+
+
+/**
+ * scsi_host_flush_commands -- Terminate all running commands
+ * @shost:	Scsi Host on which commands should be terminated
+ * @status:	Status to be set for the terminated commands
+ */
+static bool flush_cmds_iter(struct request *rq, void *data, bool reserved)
+{
+	struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(rq);
+	int status = *(int *)data;
+
+	if (reserved)
+		return true;
+	scmd->result = status << 16;
+	scmd->scsi_done(scmd);
+	return true;
+}
+
+void scsi_host_flush_commands(struct Scsi_Host *shost, int status)
+{
+	blk_mq_tagset_busy_iter(&shost->tag_set, flush_cmds_iter, &status);
+}
+EXPORT_SYMBOL_GPL(scsi_host_flush_commands);
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index f577647bf5f2..cb9a6fe9ad5b 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -735,6 +735,7 @@ extern int scsi_host_busy(struct Scsi_Host *shost);
 extern void scsi_host_put(struct Scsi_Host *t);
 extern struct Scsi_Host *scsi_host_lookup(unsigned short);
 extern const char *scsi_host_state_name(enum scsi_host_state);
+extern void scsi_host_flush_commands(struct Scsi_Host *shost, int status);
 
 static inline int __must_check scsi_add_host(struct Scsi_Host *host,
 					     struct device *dev)
-- 
2.16.4

[PATCH 3/9] dpt_i2o: use scsi_host_flush_commands() to abort outstanding commands

[Hannes Reinecke]

Rather than traversing all outstanding commands manually use the
scsi_host_flush_commands() helper to terminate all commands during
reset.
With that we can drop the cmd_list usage from the midlayer.

Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/dpt_i2o.c | 20 +-------------------
 drivers/scsi/dpti.h    |  1 -
 2 files changed, 1 insertion(+), 20 deletions(-)

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index c30ace9f251e..5fc09caa7ded 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -817,7 +817,7 @@ static int adpt_hba_reset(adpt_hba* pHba)
 	}
 	pHba->state &= ~DPTI_STATE_RESET;
 
-	adpt_fail_posted_scbs(pHba);
+	scsi_host_flush_commands(pHba->host, DID_RESET);
 	return 0;	/* return success */
 }
 
@@ -2335,7 +2335,6 @@ static s32 adpt_scsi_host_alloc(adpt_hba* pHba, struct scsi_host_template *sht)
 	host->unique_id = (u32)sys_tbl_pa + pHba->unit;
 	host->sg_tablesize = pHba->sg_tablesize;
 	host->can_queue = pHba->post_fifo_size;
-	host->use_cmd_list = 1;
 
 	return 0;
 }
@@ -2646,23 +2645,6 @@ static s32 adpt_i2o_reparse_lct(adpt_hba* pHba)
 	return 0;
 }
 
-static void adpt_fail_posted_scbs(adpt_hba* pHba)
-{
-	struct scsi_cmnd* 	cmd = NULL;
-	struct scsi_device* 	d = NULL;
-
-	shost_for_each_device(d, pHba->host) {
-		unsigned long flags;
-		spin_lock_irqsave(&d->list_lock, flags);
-		list_for_each_entry(cmd, &d->cmd_list, list) {
-			cmd->result = (DID_OK << 16) | (QUEUE_FULL <<1);
-			cmd->scsi_done(cmd);
-		}
-		spin_unlock_irqrestore(&d->list_lock, flags);
-	}
-}
-
-
 /*============================================================================
  *  Routines from i2o subsystem
  *============================================================================
diff --git a/drivers/scsi/dpti.h b/drivers/scsi/dpti.h
index 3ec391134bb0..72293b8450b6 100644
--- a/drivers/scsi/dpti.h
+++ b/drivers/scsi/dpti.h
@@ -295,7 +295,6 @@ static s32 adpt_i2o_reparse_lct(adpt_hba* pHba);
 static s32 adpt_send_nop(adpt_hba*pHba,u32 m);
 static void adpt_i2o_delete_hba(adpt_hba* pHba);
 static void adpt_inquiry(adpt_hba* pHba);
-static void adpt_fail_posted_scbs(adpt_hba* pHba);
 static struct adpt_device* adpt_find_device(adpt_hba* pHba, u32 chan, u32 id, u64 lun);
 static int adpt_install_hba(struct scsi_host_template* sht, struct pci_dev* pDev) ;
 static int adpt_i2o_online_hba(adpt_hba* pHba);
-- 
2.16.4

[PATCH 4/9] aacraid: Do not wait for outstanding write commands on synchronize_cache

[Hannes Reinecke]

There is no need to wait for outstanding write commands on synchronize
cache; the block layer is responsible for I/O scheduling, so we shouldn't
try to out-guess it on the driver layer.

Cc: Balsundar P <balsundar.p@microsemi.com>
Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/aacraid/aachba.c | 76 ++-----------------------------------------
 1 file changed, 2 insertions(+), 74 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index e36608ce937a..cfa14e15d5f0 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -2601,9 +2601,7 @@ static int aac_write(struct scsi_cmnd * scsicmd)
 static void synchronize_callback(void *context, struct fib *fibptr)
 {
 	struct aac_synchronize_reply *synchronizereply;
-	struct scsi_cmnd *cmd;
-
-	cmd = context;
+	struct scsi_cmnd *cmd = context;
 
 	if (!aac_valid_context(cmd, fibptr))
 		return;
@@ -2644,77 +2642,8 @@ static int aac_synchronize(struct scsi_cmnd *scsicmd)
 	int status;
 	struct fib *cmd_fibcontext;
 	struct aac_synchronize *synchronizecmd;
-	struct scsi_cmnd *cmd;
 	struct scsi_device *sdev = scsicmd->device;
-	int active = 0;
 	struct aac_dev *aac;
-	u64 lba = ((u64)scsicmd->cmnd[2] << 24) | (scsicmd->cmnd[3] << 16) |
-		(scsicmd->cmnd[4] << 8) | scsicmd->cmnd[5];
-	u32 count = (scsicmd->cmnd[7] << 8) | scsicmd->cmnd[8];
-	unsigned long flags;
-
-	/*
-	 * Wait for all outstanding queued commands to complete to this
-	 * specific target (block).
-	 */
-	spin_lock_irqsave(&sdev->list_lock, flags);
-	list_for_each_entry(cmd, &sdev->cmd_list, list)
-		if (cmd->SCp.phase == AAC_OWNER_FIRMWARE) {
-			u64 cmnd_lba;
-			u32 cmnd_count;
-
-			if (cmd->cmnd[0] == WRITE_6) {
-				cmnd_lba = ((cmd->cmnd[1] & 0x1F) << 16) |
-					(cmd->cmnd[2] << 8) |
-					cmd->cmnd[3];
-				cmnd_count = cmd->cmnd[4];
-				if (cmnd_count == 0)
-					cmnd_count = 256;
-			} else if (cmd->cmnd[0] == WRITE_16) {
-				cmnd_lba = ((u64)cmd->cmnd[2] << 56) |
-					((u64)cmd->cmnd[3] << 48) |
-					((u64)cmd->cmnd[4] << 40) |
-					((u64)cmd->cmnd[5] << 32) |
-					((u64)cmd->cmnd[6] << 24) |
-					(cmd->cmnd[7] << 16) |
-					(cmd->cmnd[8] << 8) |
-					cmd->cmnd[9];
-				cmnd_count = (cmd->cmnd[10] << 24) |
-					(cmd->cmnd[11] << 16) |
-					(cmd->cmnd[12] << 8) |
-					cmd->cmnd[13];
-			} else if (cmd->cmnd[0] == WRITE_12) {
-				cmnd_lba = ((u64)cmd->cmnd[2] << 24) |
-					(cmd->cmnd[3] << 16) |
-					(cmd->cmnd[4] << 8) |
-					cmd->cmnd[5];
-				cmnd_count = (cmd->cmnd[6] << 24) |
-					(cmd->cmnd[7] << 16) |
-					(cmd->cmnd[8] << 8) |
-					cmd->cmnd[9];
-			} else if (cmd->cmnd[0] == WRITE_10) {
-				cmnd_lba = ((u64)cmd->cmnd[2] << 24) |
-					(cmd->cmnd[3] << 16) |
-					(cmd->cmnd[4] << 8) |
-					cmd->cmnd[5];
-				cmnd_count = (cmd->cmnd[7] << 8) |
-					cmd->cmnd[8];
-			} else
-				continue;
-			if (((cmnd_lba + cmnd_count) < lba) ||
-			  (count && ((lba + count) < cmnd_lba)))
-				continue;
-			++active;
-			break;
-		}
-
-	spin_unlock_irqrestore(&sdev->list_lock, flags);
-
-	/*
-	 *	Yield the processor (requeue for later)
-	 */
-	if (active)
-		return SCSI_MLQUEUE_DEVICE_BUSY;
 
 	aac = (struct aac_dev *)sdev->host->hostdata;
 	if (aac->in_reset)
@@ -2723,8 +2652,7 @@ static int aac_synchronize(struct scsi_cmnd *scsicmd)
 	/*
 	 *	Allocate and initialize a Fib
 	 */
-	if (!(cmd_fibcontext = aac_fib_alloc(aac)))
-		return SCSI_MLQUEUE_HOST_BUSY;
+	cmd_fibcontext = aac_fib_alloc_tag(aac, scsicmd);
 
 	aac_fib_init(cmd_fibcontext);
 
-- 
2.16.4

[PATCH 5/9] aacraid: use midlayer helper to terminate outstanding commands

[Hannes Reinecke]

Use scsi_host_flush_commands() to terminate all outstanding commands
instead, and change the command result for terminated commands to
the more common 'DID_RESET' instead of 'QUEUE_FULL'.

Cc: Balsundar P <balsundar.p@microsemi.com>
Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/aacraid/commsup.c | 24 ++----------------------
 1 file changed, 2 insertions(+), 22 deletions(-)

diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index 5a8a999606ea..e413b784a8d0 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -1478,8 +1478,6 @@ static int _aac_reset_adapter(struct aac_dev *aac, int forced, u8 reset_type)
 	int retval;
 	struct Scsi_Host *host;
 	struct scsi_device *dev;
-	struct scsi_cmnd *command;
-	struct scsi_cmnd *command_list;
 	int jafo = 0;
 	int bled;
 	u64 dmamask;
@@ -1607,26 +1605,8 @@ static int _aac_reset_adapter(struct aac_dev *aac, int forced, u8 reset_type)
 	 * This is where the assumption that the Adapter is quiesced
 	 * is important.
 	 */
-	command_list = NULL;
-	__shost_for_each_device(dev, host) {
-		unsigned long flags;
-		spin_lock_irqsave(&dev->list_lock, flags);
-		list_for_each_entry(command, &dev->cmd_list, list)
-			if (command->SCp.phase == AAC_OWNER_FIRMWARE) {
-				command->SCp.buffer = (struct scatterlist *)command_list;
-				command_list = command;
-			}
-		spin_unlock_irqrestore(&dev->list_lock, flags);
-	}
-	while ((command = command_list)) {
-		command_list = (struct scsi_cmnd *)command->SCp.buffer;
-		command->SCp.buffer = NULL;
-		command->result = DID_OK << 16
-		  | COMMAND_COMPLETE << 8
-		  | SAM_STAT_TASK_SET_FULL;
-		command->SCp.phase = AAC_OWNER_ERROR_HANDLER;
-		command->scsi_done(command);
-	}
+	scsi_host_flush_commands(host, DID_RESET);
+
 	/*
 	 * Any Device that was already marked offline needs to be marked
 	 * running
-- 
2.16.4

[PATCH 6/9] scsi: add scsi_host_busy_iter()

[Hannes Reinecke]

Add an iterator scsi_host_busy_iter() to traverse all busy commands.
If locking against concurrent command completions is required it
has to be provided by the caller.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/hosts.c     | 33 +++++++++++++++++++++++++++++++++
 include/scsi/scsi_host.h |  5 +++++
 2 files changed, 38 insertions(+)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index da1df48ef27c..a0a660b50929 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -674,3 +674,36 @@ void scsi_host_flush_commands(struct Scsi_Host *shost, int status)
 	blk_mq_tagset_busy_iter(&shost->tag_set, flush_cmds_iter, &status);
 }
 EXPORT_SYMBOL_GPL(scsi_host_flush_commands);
+
+struct scsi_host_busy_iter_data {
+	scsi_host_busy_iter_fn *fn;
+	void *priv;
+};
+
+static bool __scsi_host_busy_iter_fn(struct request *req, void *priv,
+				   bool reserved)
+{
+	struct scsi_host_busy_iter_data *iter_data = priv;
+	struct scsi_cmnd *sc = blk_mq_rq_to_pdu(req);
+
+	return iter_data->fn(sc, iter_data->priv, reserved);
+}
+
+/**
+ * scsi_host_busy_iter - Iterate over all busy commands
+ * @shost:	Pointer to Scsi_Host.
+ * @fn:		Function to call on each busy command
+ * @priv:	Data pointer passed to @fn
+ **/
+void scsi_host_busy_iter(struct Scsi_Host *shost,
+			 scsi_host_busy_iter_fn *fn, void *priv)
+{
+	struct scsi_host_busy_iter_data iter_data = {
+		.fn = fn,
+		.priv = priv,
+	};
+
+	blk_mq_tagset_busy_iter(&shost->tag_set, __scsi_host_busy_iter_fn,
+				&iter_data);
+}
+EXPORT_SYMBOL_GPL(scsi_host_busy_iter);
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index cb9a6fe9ad5b..1293d9686115 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -761,6 +761,11 @@ static inline int scsi_host_scan_allowed(struct Scsi_Host *shost)
 extern void scsi_unblock_requests(struct Scsi_Host *);
 extern void scsi_block_requests(struct Scsi_Host *);
 
+typedef bool (scsi_host_busy_iter_fn)(struct scsi_cmnd *, void *, bool);
+
+void scsi_host_busy_iter(struct Scsi_Host *,
+			 scsi_host_busy_iter_fn *fn, void *priv);
+
 struct class_container;
 
 /*
-- 
2.16.4

[PATCH 7/9] aacraid: use scsi_host_busy_iter() in aac_wait_for_io_completion()

[Hannes Reinecke]

Use the midlayer helper function to traverse outstanding commands.

Cc: Balsundar P <balsundar.p@microsemi.com>
Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/aacraid/comminit.c | 30 +++++++++++++-----------------
 1 file changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c
index f75878d773cf..89c0ca339ef5 100644
--- a/drivers/scsi/aacraid/comminit.c
+++ b/drivers/scsi/aacraid/comminit.c
@@ -272,29 +272,25 @@ static void aac_queue_init(struct aac_dev * dev, struct aac_queue * q, u32 *mem,
 	q->entries = qsize;
 }
 
+static bool wait_for_io_iter(struct scsi_cmnd *cmd, void *data, bool reserved)
+{
+	int *active = data;
+
+	if (cmd->SCp.phase == AAC_OWNER_FIRMWARE)
+		*active = 1;
+	return true;
+}
+
+/* scsi_block_requests() has been called, so no new request can be issued */
 static void aac_wait_for_io_completion(struct aac_dev *aac)
 {
-	unsigned long flagv = 0;
-	int i = 0;
+	int i;
 
 	for (i = 60; i; --i) {
-		struct scsi_device *dev;
-		struct scsi_cmnd *command;
 		int active = 0;
 
-		__shost_for_each_device(dev, aac->scsi_host_ptr) {
-			spin_lock_irqsave(&dev->list_lock, flagv);
-			list_for_each_entry(command, &dev->cmd_list, list) {
-				if (command->SCp.phase == AAC_OWNER_FIRMWARE) {
-					active++;
-					break;
-				}
-			}
-			spin_unlock_irqrestore(&dev->list_lock, flagv);
-			if (active)
-				break;
-
-		}
+		scsi_host_busy_iter(aac->scsi_host_ptr,
+				    wait_for_io_iter, &active);
 		/*
 		 * We can exit If all the commands are complete
 		 */
-- 
2.16.4

[PATCH 8/9] aacraid: use scsi_host_busy_iter() in get_num_of_incomplete_fibs()

[Hannes Reinecke]

Use the scsi midlayer helper to traverse the number of outstanding
commands. This also eliminates the last usage for the cmd_list
functionality so we can drop it.

Cc: Balsundar P <balsundar.p@microsemi.com>
Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com>
Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/aacraid/linit.c | 81 ++++++++++++++++++++++----------------------
 1 file changed, 41 insertions(+), 40 deletions(-)

diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index ee6bc2f9b80a..db96482c4fdc 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -622,54 +622,56 @@ static int aac_ioctl(struct scsi_device *sdev, unsigned int cmd,
 	return aac_do_ioctl(dev, cmd, arg);
 }
 
-static int get_num_of_incomplete_fibs(struct aac_dev *aac)
+struct fib_count_data {
+	int mlcnt;
+	int llcnt;
+	int ehcnt;
+	int fwcnt;
+	int krlcnt;
+};
+
+static bool fib_count_iter(struct scsi_cmnd *scmnd, void *data, bool reserved)
 {
+	struct fib_count_data *fib_count = data;
 
-	unsigned long flags;
-	struct scsi_device *sdev = NULL;
+	switch (scmnd->SCp.phase) {
+	case AAC_OWNER_FIRMWARE:
+		fib_count->fwcnt++;
+		break;
+	case AAC_OWNER_ERROR_HANDLER:
+		fib_count->ehcnt++;
+		break;
+	case AAC_OWNER_LOWLEVEL:
+		fib_count->llcnt++;
+		break;
+	case AAC_OWNER_MIDLEVEL:
+		fib_count->mlcnt++;
+		break;
+	default:
+		fib_count->krlcnt++;
+		break;
+	}
+	return true;
+}
+
+/* Called during SCSI EH, so we don't need to block requests */
+static int get_num_of_incomplete_fibs(struct aac_dev *aac)
+{
 	struct Scsi_Host *shost = aac->scsi_host_ptr;
-	struct scsi_cmnd *scmnd = NULL;
 	struct device *ctrl_dev;
+	struct fib_count_data fcnt = { };
 
-	int mlcnt  = 0;
-	int llcnt  = 0;
-	int ehcnt  = 0;
-	int fwcnt  = 0;
-	int krlcnt = 0;
-
-	__shost_for_each_device(sdev, shost) {
-		spin_lock_irqsave(&sdev->list_lock, flags);
-		list_for_each_entry(scmnd, &sdev->cmd_list, list) {
-			switch (scmnd->SCp.phase) {
-			case AAC_OWNER_FIRMWARE:
-				fwcnt++;
-				break;
-			case AAC_OWNER_ERROR_HANDLER:
-				ehcnt++;
-				break;
-			case AAC_OWNER_LOWLEVEL:
-				llcnt++;
-				break;
-			case AAC_OWNER_MIDLEVEL:
-				mlcnt++;
-				break;
-			default:
-				krlcnt++;
-				break;
-			}
-		}
-		spin_unlock_irqrestore(&sdev->list_lock, flags);
-	}
+	scsi_host_busy_iter(shost, fib_count_iter, &fcnt);
 
 	ctrl_dev = &aac->pdev->dev;
 
-	dev_info(ctrl_dev, "outstanding cmd: midlevel-%d\n", mlcnt);
-	dev_info(ctrl_dev, "outstanding cmd: lowlevel-%d\n", llcnt);
-	dev_info(ctrl_dev, "outstanding cmd: error handler-%d\n", ehcnt);
-	dev_info(ctrl_dev, "outstanding cmd: firmware-%d\n", fwcnt);
-	dev_info(ctrl_dev, "outstanding cmd: kernel-%d\n", krlcnt);
+	dev_info(ctrl_dev, "outstanding cmd: midlevel-%d\n", fcnt.mlcnt);
+	dev_info(ctrl_dev, "outstanding cmd: lowlevel-%d\n", fcnt.llcnt);
+	dev_info(ctrl_dev, "outstanding cmd: error handler-%d\n", fcnt.ehcnt);
+	dev_info(ctrl_dev, "outstanding cmd: firmware-%d\n", fcnt.fwcnt);
+	dev_info(ctrl_dev, "outstanding cmd: kernel-%d\n", fcnt.krlcnt);
 
-	return mlcnt + llcnt + ehcnt + fwcnt;
+	return fcnt.mlcnt + fcnt.llcnt + fcnt.ehcnt + fcnt.fwcnt;
 }
 
 static int aac_eh_abort(struct scsi_cmnd* cmd)
@@ -1675,7 +1677,6 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	shost->irq = pdev->irq;
 	shost->unique_id = unique_id;
 	shost->max_cmd_len = 16;
-	shost->use_cmd_list = 1;
 
 	if (aac_cfg_major == AAC_CHARDEV_NEEDS_REINIT)
 		aac_init_char();
-- 
2.16.4

[PATCH 9/9] scsi: Remove cmd_list functionality

[Hannes Reinecke]

Remove cmd_list functionality; no users left.
With that the scsi_put_command() becomes empty,
so remove that one, too.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Reviewed-by: Bart van Assche <bvanassche@acm.org>
---
 drivers/scsi/scsi.c        | 14 --------------
 drivers/scsi/scsi_error.c  |  1 -
 drivers/scsi/scsi_lib.c    | 31 -------------------------------
 drivers/scsi/scsi_priv.h   |  2 --
 drivers/scsi/scsi_scan.c   |  1 -
 include/scsi/scsi_cmnd.h   |  1 -
 include/scsi/scsi_device.h |  1 -
 include/scsi/scsi_host.h   |  2 --
 8 files changed, 53 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index adfe8b3693d5..ed533b6e1fd0 100644
--- a/drivers/scsi/scsi.c
+++ b/drivers/scsi/scsi.c
@@ -94,20 +94,6 @@ EXPORT_SYMBOL(scsi_logging_level);
 ASYNC_DOMAIN_EXCLUSIVE(scsi_sd_pm_domain);
 EXPORT_SYMBOL(scsi_sd_pm_domain);
 
-/**
- * scsi_put_command - Free a scsi command block
- * @cmd: command block to free
- *
- * Returns:	Nothing.
- *
- * Notes:	The command must not belong to any lists.
- */
-void scsi_put_command(struct scsi_cmnd *cmd)
-{
-	scsi_del_cmd_from_list(cmd);
-	BUG_ON(delayed_work_pending(&cmd->abort_work));
-}
-
 #ifdef CONFIG_SCSI_LOGGING
 void scsi_log_send(struct scsi_cmnd *cmd)
 {
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index ae2fa170f6ad..978be1602f71 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -2412,7 +2412,6 @@ scsi_ioctl_reset(struct scsi_device *dev, int __user *arg)
 	wake_up(&shost->host_wait);
 	scsi_run_host_queues(shost);
 
-	scsi_put_command(scmd);
 	kfree(rq);
 
 out_put_autopm_host:
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 2563b061f56b..893804e87e25 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -562,7 +562,6 @@ static void scsi_mq_uninit_cmd(struct scsi_cmnd *cmd)
 {
 	scsi_mq_free_sgtables(cmd);
 	scsi_uninit_cmd(cmd);
-	scsi_del_cmd_from_list(cmd);
 }
 
 /* Returns false when no more bytes to process, true if there are more */
@@ -1098,35 +1097,6 @@ static void scsi_cleanup_rq(struct request *rq)
 	}
 }
 
-/* Add a command to the list used by the aacraid and dpt_i2o drivers */
-void scsi_add_cmd_to_list(struct scsi_cmnd *cmd)
-{
-	struct scsi_device *sdev = cmd->device;
-	struct Scsi_Host *shost = sdev->host;
-	unsigned long flags;
-
-	if (shost->use_cmd_list) {
-		spin_lock_irqsave(&sdev->list_lock, flags);
-		list_add_tail(&cmd->list, &sdev->cmd_list);
-		spin_unlock_irqrestore(&sdev->list_lock, flags);
-	}
-}
-
-/* Remove a command from the list used by the aacraid and dpt_i2o drivers */
-void scsi_del_cmd_from_list(struct scsi_cmnd *cmd)
-{
-	struct scsi_device *sdev = cmd->device;
-	struct Scsi_Host *shost = sdev->host;
-	unsigned long flags;
-
-	if (shost->use_cmd_list) {
-		spin_lock_irqsave(&sdev->list_lock, flags);
-		BUG_ON(list_empty(&cmd->list));
-		list_del_init(&cmd->list);
-		spin_unlock_irqrestore(&sdev->list_lock, flags);
-	}
-}
-
 /* Called after a request has been started. */
 void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
 {
@@ -1160,7 +1130,6 @@ void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
 	if (in_flight)
 		__set_bit(SCMD_STATE_INFLIGHT, &cmd->state);
 
-	scsi_add_cmd_to_list(cmd);
 }
 
 static blk_status_t scsi_setup_scsi_cmnd(struct scsi_device *sdev,
diff --git a/drivers/scsi/scsi_priv.h b/drivers/scsi/scsi_priv.h
index 3bff9f7aa684..5eab5da4efe7 100644
--- a/drivers/scsi/scsi_priv.h
+++ b/drivers/scsi/scsi_priv.h
@@ -84,8 +84,6 @@ int scsi_eh_get_sense(struct list_head *work_q,
 int scsi_noretry_cmd(struct scsi_cmnd *scmd);
 
 /* scsi_lib.c */
-extern void scsi_add_cmd_to_list(struct scsi_cmnd *cmd);
-extern void scsi_del_cmd_from_list(struct scsi_cmnd *cmd);
 extern int scsi_maybe_unblock_host(struct scsi_device *sdev);
 extern void scsi_device_unbusy(struct scsi_device *sdev, struct scsi_cmnd *cmd);
 extern void scsi_queue_insert(struct scsi_cmnd *cmd, int reason);
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 058079f915f1..f2437a7570ce 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -236,7 +236,6 @@ static struct scsi_device *scsi_alloc_sdev(struct scsi_target *starget,
 	sdev->sdev_state = SDEV_CREATED;
 	INIT_LIST_HEAD(&sdev->siblings);
 	INIT_LIST_HEAD(&sdev->same_target_siblings);
-	INIT_LIST_HEAD(&sdev->cmd_list);
 	INIT_LIST_HEAD(&sdev->starved_entry);
 	INIT_LIST_HEAD(&sdev->event_list);
 	spin_lock_init(&sdev->list_lock);
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index a2849bb9cd19..80ac89e47b47 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -159,7 +159,6 @@ static inline struct scsi_driver *scsi_cmd_to_driver(struct scsi_cmnd *cmd)
 	return *(struct scsi_driver **)cmd->request->rq_disk->private_data;
 }
 
-extern void scsi_put_command(struct scsi_cmnd *);
 extern void scsi_finish_command(struct scsi_cmnd *cmd);
 
 extern void *scsi_kmap_atomic_sg(struct scatterlist *sg, int sg_count,
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 3ed836db5306..fd2aee1f59fc 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -110,7 +110,6 @@ struct scsi_device {
 	atomic_t device_blocked;	/* Device returned QUEUE_FULL. */
 
 	spinlock_t list_lock;
-	struct list_head cmd_list;	/* queue of in use SCSI Command structures */
 	struct list_head starved_entry;
 	unsigned short queue_depth;	/* How deep of a queue we want */
 	unsigned short max_queue_depth;	/* max queue depth */
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 1293d9686115..ee43db94a479 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -627,8 +627,6 @@ struct Scsi_Host {
 	/* The controller does not support WRITE SAME */
 	unsigned no_write_same:1;
 
-	unsigned use_cmd_list:1;
-
 	/* Host responded with short (<36 bytes) INQUIRY result */
 	unsigned short_inquiry:1;
 
-- 
2.16.4

Re: [PATCH 1/9] dpt_i2o: rename adpt_i2o_to_scsi() to adpt_i2o_scsi_complete()

[Bart Van Assche]

On 11/18/19 1:22 AM, Hannes Reinecke wrote:
> Rename the badly named function into adpt_i2o_scsi_complete(),
> and make it a void function as the return value is never used.
> This also fixes a potential use-after free as the return value
> might be evaluated from the command result after the command
> has been freed.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 2/9] scsi: add scsi_host_flush_commands() helper

[Bart Van Assche]

On 11/18/19 1:22 AM, Hannes Reinecke wrote:
> +/**
> + * scsi_host_flush_commands -- Terminate all running commands
> + * @shost:	Scsi Host on which commands should be terminated
> + * @status:	Status to be set for the terminated commands
> + */
> +static bool flush_cmds_iter(struct request *rq, void *data, bool reserved)
> +{
> +	struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(rq);
> +	int status = *(int *)data;
> +
> +	if (reserved)
> +		return true;
> +	scmd->result = status << 16;
> +	scmd->scsi_done(scmd);
> +	return true;
> +}
> +
> +void scsi_host_flush_commands(struct Scsi_Host *shost, int status)
> +{
> +	blk_mq_tagset_busy_iter(&shost->tag_set, flush_cmds_iter, &status);
> +}

Should the kernel-doc comment be moved from the static function to the 
exported function? Please also add a comment that it is only safe to 
call scsi_host_flush_commands() from inside a LLD reset handler.

Thanks,

Bart.

Re: [PATCH 3/9] dpt_i2o: use scsi_host_flush_commands() to abort outstanding commands

[Bart Van Assche]

On 11/18/19 1:22 AM, Hannes Reinecke wrote:
> Rather than traversing all outstanding commands manually use the
> scsi_host_flush_commands() helper to terminate all commands during
> reset.
> With that we can drop the cmd_list usage from the midlayer.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 5/9] aacraid: use midlayer helper to terminate outstanding commands

[Bart Van Assche]

On 11/18/19 1:22 AM, Hannes Reinecke wrote:
> Use scsi_host_flush_commands() to terminate all outstanding commands
> instead, and change the command result for terminated commands to
> the more common 'DID_RESET' instead of 'QUEUE_FULL'.

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 7/9] aacraid: use scsi_host_busy_iter() in aac_wait_for_io_completion()

[Bart Van Assche]

On 11/18/19 1:22 AM, Hannes Reinecke wrote:
> Use the midlayer helper function to traverse outstanding commands.
> 
> Cc: Balsundar P <balsundar.p@microsemi.com>
> Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/scsi/aacraid/comminit.c | 30 +++++++++++++-----------------
>   1 file changed, 13 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c
> index f75878d773cf..89c0ca339ef5 100644
> --- a/drivers/scsi/aacraid/comminit.c
> +++ b/drivers/scsi/aacraid/comminit.c
> @@ -272,29 +272,25 @@ static void aac_queue_init(struct aac_dev * dev, struct aac_queue * q, u32 *mem,
>   	q->entries = qsize;
>   }
>   
> +static bool wait_for_io_iter(struct scsi_cmnd *cmd, void *data, bool reserved)
> +{
> +	int *active = data;
> +
> +	if (cmd->SCp.phase == AAC_OWNER_FIRMWARE)
> +		*active = 1;
> +	return true;
> +}
> +
> +/* scsi_block_requests() has been called, so no new request can be issued */
>   static void aac_wait_for_io_completion(struct aac_dev *aac)
>   {
> -	unsigned long flagv = 0;
> -	int i = 0;
> +	int i;
>   
>   	for (i = 60; i; --i) {
> -		struct scsi_device *dev;
> -		struct scsi_cmnd *command;
>   		int active = 0;
>   
> -		__shost_for_each_device(dev, aac->scsi_host_ptr) {
> -			spin_lock_irqsave(&dev->list_lock, flagv);
> -			list_for_each_entry(command, &dev->cmd_list, list) {
> -				if (command->SCp.phase == AAC_OWNER_FIRMWARE) {
> -					active++;
> -					break;
> -				}
> -			}
> -			spin_unlock_irqrestore(&dev->list_lock, flagv);
> -			if (active)
> -				break;
> -
> -		}
> +		scsi_host_busy_iter(aac->scsi_host_ptr,
> +				    wait_for_io_iter, &active);
>   		/*
>   		 * We can exit If all the commands are complete
>   		 */

Would using scsi_device_quiesce() and scsi_device_resume() allow to 
eliminate the busy-waiting loop from the aacraid driver?

Thanks,

Bart.

Re: [PATCH 8/9] aacraid: use scsi_host_busy_iter() in get_num_of_incomplete_fibs()

[Bart Van Assche]

On 11/18/19 1:22 AM, Hannes Reinecke wrote:
> Use the scsi midlayer helper to traverse the number of outstanding
> commands. This also eliminates the last usage for the cmd_list
> functionality so we can drop it.
> 
> Cc: Balsundar P <balsundar.p@microsemi.com>
> Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/scsi/aacraid/linit.c | 81 ++++++++++++++++++++++----------------------
>   1 file changed, 41 insertions(+), 40 deletions(-)
> 
> diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
> index ee6bc2f9b80a..db96482c4fdc 100644
> --- a/drivers/scsi/aacraid/linit.c
> +++ b/drivers/scsi/aacraid/linit.c
> @@ -622,54 +622,56 @@ static int aac_ioctl(struct scsi_device *sdev, unsigned int cmd,
>   	return aac_do_ioctl(dev, cmd, arg);
>   }
>   
> -static int get_num_of_incomplete_fibs(struct aac_dev *aac)
> +struct fib_count_data {
> +	int mlcnt;
> +	int llcnt;
> +	int ehcnt;
> +	int fwcnt;
> +	int krlcnt;
> +};
> +
> +static bool fib_count_iter(struct scsi_cmnd *scmnd, void *data, bool reserved)
>   {
> +	struct fib_count_data *fib_count = data;
>   
> -	unsigned long flags;
> -	struct scsi_device *sdev = NULL;
> +	switch (scmnd->SCp.phase) {
> +	case AAC_OWNER_FIRMWARE:
> +		fib_count->fwcnt++;
> +		break;
> +	case AAC_OWNER_ERROR_HANDLER:
> +		fib_count->ehcnt++;
> +		break;
> +	case AAC_OWNER_LOWLEVEL:
> +		fib_count->llcnt++;
> +		break;
> +	case AAC_OWNER_MIDLEVEL:
> +		fib_count->mlcnt++;
> +		break;
> +	default:
> +		fib_count->krlcnt++;
> +		break;
> +	}
> +	return true;
> +}
> +
> +/* Called during SCSI EH, so we don't need to block requests */
> +static int get_num_of_incomplete_fibs(struct aac_dev *aac)
> +{
>   	struct Scsi_Host *shost = aac->scsi_host_ptr;
> -	struct scsi_cmnd *scmnd = NULL;
>   	struct device *ctrl_dev;
> +	struct fib_count_data fcnt = { };
>   
> -	int mlcnt  = 0;
> -	int llcnt  = 0;
> -	int ehcnt  = 0;
> -	int fwcnt  = 0;
> -	int krlcnt = 0;
> -
> -	__shost_for_each_device(sdev, shost) {
> -		spin_lock_irqsave(&sdev->list_lock, flags);
> -		list_for_each_entry(scmnd, &sdev->cmd_list, list) {
> -			switch (scmnd->SCp.phase) {
> -			case AAC_OWNER_FIRMWARE:
> -				fwcnt++;
> -				break;
> -			case AAC_OWNER_ERROR_HANDLER:
> -				ehcnt++;
> -				break;
> -			case AAC_OWNER_LOWLEVEL:
> -				llcnt++;
> -				break;
> -			case AAC_OWNER_MIDLEVEL:
> -				mlcnt++;
> -				break;
> -			default:
> -				krlcnt++;
> -				break;
> -			}
> -		}
> -		spin_unlock_irqrestore(&sdev->list_lock, flags);
> -	}
> +	scsi_host_busy_iter(shost, fib_count_iter, &fcnt);
>   
>   	ctrl_dev = &aac->pdev->dev;
>   
> -	dev_info(ctrl_dev, "outstanding cmd: midlevel-%d\n", mlcnt);
> -	dev_info(ctrl_dev, "outstanding cmd: lowlevel-%d\n", llcnt);
> -	dev_info(ctrl_dev, "outstanding cmd: error handler-%d\n", ehcnt);
> -	dev_info(ctrl_dev, "outstanding cmd: firmware-%d\n", fwcnt);
> -	dev_info(ctrl_dev, "outstanding cmd: kernel-%d\n", krlcnt);
> +	dev_info(ctrl_dev, "outstanding cmd: midlevel-%d\n", fcnt.mlcnt);
> +	dev_info(ctrl_dev, "outstanding cmd: lowlevel-%d\n", fcnt.llcnt);
> +	dev_info(ctrl_dev, "outstanding cmd: error handler-%d\n", fcnt.ehcnt);
> +	dev_info(ctrl_dev, "outstanding cmd: firmware-%d\n", fcnt.fwcnt);
> +	dev_info(ctrl_dev, "outstanding cmd: kernel-%d\n", fcnt.krlcnt);
>   
> -	return mlcnt + llcnt + ehcnt + fwcnt;
> +	return fcnt.mlcnt + fcnt.llcnt + fcnt.ehcnt + fcnt.fwcnt;
>   }
>   
>   static int aac_eh_abort(struct scsi_cmnd* cmd)
> @@ -1675,7 +1677,6 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
>   	shost->irq = pdev->irq;
>   	shost->unique_id = unique_id;
>   	shost->max_cmd_len = 16;
> -	shost->use_cmd_list = 1;
>   
>   	if (aac_cfg_major == AAC_CHARDEV_NEEDS_REINIT)
>   		aac_init_char();

Same comment here: could scsi_device_quiesce() and scsi_device_resume() 
have been used instead?

Thanks,

Bart.

Re: [PATCH 2/9] scsi: add scsi_host_flush_commands() helper

[Hannes Reinecke]

On 11/18/19 11:57 PM, Bart Van Assche wrote:
> On 11/18/19 1:22 AM, Hannes Reinecke wrote:
>> +/**
>> + * scsi_host_flush_commands -- Terminate all running commands
>> + * @shost:    Scsi Host on which commands should be terminated
>> + * @status:    Status to be set for the terminated commands
>> + */
>> +static bool flush_cmds_iter(struct request *rq, void *data, bool
>> reserved)
>> +{
>> +    struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(rq);
>> +    int status = *(int *)data;
>> +
>> +    if (reserved)
>> +        return true;
>> +    scmd->result = status << 16;
>> +    scmd->scsi_done(scmd);
>> +    return true;
>> +}
>> +
>> +void scsi_host_flush_commands(struct Scsi_Host *shost, int status)
>> +{
>> +    blk_mq_tagset_busy_iter(&shost->tag_set, flush_cmds_iter, &status);
>> +}
> 
> Should the kernel-doc comment be moved from the static function to the
> exported function? Please also add a comment that it is only safe to
> call scsi_host_flush_commands() from inside a LLD reset handler.
> 
I'd rather say that concurrent I/O submission must be prevented from the
caller. But yes, will be updating the comments.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

Re: [PATCH 7/9] aacraid: use scsi_host_busy_iter() in aac_wait_for_io_completion()

[Hannes Reinecke]

On 11/19/19 12:05 AM, Bart Van Assche wrote:
> On 11/18/19 1:22 AM, Hannes Reinecke wrote:
>> Use the midlayer helper function to traverse outstanding commands.
>>
>> Cc: Balsundar P <balsundar.p@microsemi.com>
>> Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/scsi/aacraid/comminit.c | 30 +++++++++++++-----------------
>>   1 file changed, 13 insertions(+), 17 deletions(-)
>>
>> diff --git a/drivers/scsi/aacraid/comminit.c
>> b/drivers/scsi/aacraid/comminit.c
>> index f75878d773cf..89c0ca339ef5 100644
>> --- a/drivers/scsi/aacraid/comminit.c
>> +++ b/drivers/scsi/aacraid/comminit.c
>> @@ -272,29 +272,25 @@ static void aac_queue_init(struct aac_dev * dev,
>> struct aac_queue * q, u32 *mem,
>>       q->entries = qsize;
>>   }
>>   +static bool wait_for_io_iter(struct scsi_cmnd *cmd, void *data,
>> bool reserved)
>> +{
>> +    int *active = data;
>> +
>> +    if (cmd->SCp.phase == AAC_OWNER_FIRMWARE)
>> +        *active = 1;
>> +    return true;
>> +}
>> +
>> +/* scsi_block_requests() has been called, so no new request can be
>> issued */
>>   static void aac_wait_for_io_completion(struct aac_dev *aac)
>>   {
>> -    unsigned long flagv = 0;
>> -    int i = 0;
>> +    int i;
>>         for (i = 60; i; --i) {
>> -        struct scsi_device *dev;
>> -        struct scsi_cmnd *command;
>>           int active = 0;
>>   -        __shost_for_each_device(dev, aac->scsi_host_ptr) {
>> -            spin_lock_irqsave(&dev->list_lock, flagv);
>> -            list_for_each_entry(command, &dev->cmd_list, list) {
>> -                if (command->SCp.phase == AAC_OWNER_FIRMWARE) {
>> -                    active++;
>> -                    break;
>> -                }
>> -            }
>> -            spin_unlock_irqrestore(&dev->list_lock, flagv);
>> -            if (active)
>> -                break;
>> -
>> -        }
>> +        scsi_host_busy_iter(aac->scsi_host_ptr,
>> +                    wait_for_io_iter, &active);
>>           /*
>>            * We can exit If all the commands are complete
>>            */
> 
> Would using scsi_device_quiesce() and scsi_device_resume() allow to
> eliminate the busy-waiting loop from the aacraid driver?
> 
I've actually looked into it, but then figured that it'll be more
cumbersome as I'd have to iterate across all devices here, calling
scsi_device_quiesce() on each one.
But looking closer into it, we could have a helper scsi_host_quiesce()
and scsi_host_resume() ... yes, indeed.
Will be updating the patchset.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer

Re: [PATCH 8/9] aacraid: use scsi_host_busy_iter() in get_num_of_incomplete_fibs()

[Hannes Reinecke]

On 11/19/19 12:06 AM, Bart Van Assche wrote:
> On 11/18/19 1:22 AM, Hannes Reinecke wrote:
>> Use the scsi midlayer helper to traverse the number of outstanding
>> commands. This also eliminates the last usage for the cmd_list
>> functionality so we can drop it.
>>
>> Cc: Balsundar P <balsundar.p@microsemi.com>
>> Cc: Adaptec OEM Raid Solutions <aacraid@microsemi.com>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/scsi/aacraid/linit.c | 81
>> ++++++++++++++++++++++----------------------
>>   1 file changed, 41 insertions(+), 40 deletions(-)
>>
>> diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
>> index ee6bc2f9b80a..db96482c4fdc 100644
>> --- a/drivers/scsi/aacraid/linit.c
>> +++ b/drivers/scsi/aacraid/linit.c
>> @@ -622,54 +622,56 @@ static int aac_ioctl(struct scsi_device *sdev,
>> unsigned int cmd,
>>       return aac_do_ioctl(dev, cmd, arg);
>>   }
>>   -static int get_num_of_incomplete_fibs(struct aac_dev *aac)
>> +struct fib_count_data {
>> +    int mlcnt;
>> +    int llcnt;
>> +    int ehcnt;
>> +    int fwcnt;
>> +    int krlcnt;
>> +};
>> +
>> +static bool fib_count_iter(struct scsi_cmnd *scmnd, void *data, bool
>> reserved)
>>   {
>> +    struct fib_count_data *fib_count = data;
>>   -    unsigned long flags;
>> -    struct scsi_device *sdev = NULL;
>> +    switch (scmnd->SCp.phase) {
>> +    case AAC_OWNER_FIRMWARE:
>> +        fib_count->fwcnt++;
>> +        break;
>> +    case AAC_OWNER_ERROR_HANDLER:
>> +        fib_count->ehcnt++;
>> +        break;
>> +    case AAC_OWNER_LOWLEVEL:
>> +        fib_count->llcnt++;
>> +        break;
>> +    case AAC_OWNER_MIDLEVEL:
>> +        fib_count->mlcnt++;
>> +        break;
>> +    default:
>> +        fib_count->krlcnt++;
>> +        break;
>> +    }
>> +    return true;
>> +}
>> +
>> +/* Called during SCSI EH, so we don't need to block requests */
>> +static int get_num_of_incomplete_fibs(struct aac_dev *aac)
>> +{
>>       struct Scsi_Host *shost = aac->scsi_host_ptr;
>> -    struct scsi_cmnd *scmnd = NULL;
>>       struct device *ctrl_dev;
>> +    struct fib_count_data fcnt = { };
>>   -    int mlcnt  = 0;
>> -    int llcnt  = 0;
>> -    int ehcnt  = 0;
>> -    int fwcnt  = 0;
>> -    int krlcnt = 0;
>> -
>> -    __shost_for_each_device(sdev, shost) {
>> -        spin_lock_irqsave(&sdev->list_lock, flags);
>> -        list_for_each_entry(scmnd, &sdev->cmd_list, list) {
>> -            switch (scmnd->SCp.phase) {
>> -            case AAC_OWNER_FIRMWARE:
>> -                fwcnt++;
>> -                break;
>> -            case AAC_OWNER_ERROR_HANDLER:
>> -                ehcnt++;
>> -                break;
>> -            case AAC_OWNER_LOWLEVEL:
>> -                llcnt++;
>> -                break;
>> -            case AAC_OWNER_MIDLEVEL:
>> -                mlcnt++;
>> -                break;
>> -            default:
>> -                krlcnt++;
>> -                break;
>> -            }
>> -        }
>> -        spin_unlock_irqrestore(&sdev->list_lock, flags);
>> -    }
>> +    scsi_host_busy_iter(shost, fib_count_iter, &fcnt);
>>         ctrl_dev = &aac->pdev->dev;
>>   -    dev_info(ctrl_dev, "outstanding cmd: midlevel-%d\n", mlcnt);
>> -    dev_info(ctrl_dev, "outstanding cmd: lowlevel-%d\n", llcnt);
>> -    dev_info(ctrl_dev, "outstanding cmd: error handler-%d\n", ehcnt);
>> -    dev_info(ctrl_dev, "outstanding cmd: firmware-%d\n", fwcnt);
>> -    dev_info(ctrl_dev, "outstanding cmd: kernel-%d\n", krlcnt);
>> +    dev_info(ctrl_dev, "outstanding cmd: midlevel-%d\n", fcnt.mlcnt);
>> +    dev_info(ctrl_dev, "outstanding cmd: lowlevel-%d\n", fcnt.llcnt);
>> +    dev_info(ctrl_dev, "outstanding cmd: error handler-%d\n",
>> fcnt.ehcnt);
>> +    dev_info(ctrl_dev, "outstanding cmd: firmware-%d\n", fcnt.fwcnt);
>> +    dev_info(ctrl_dev, "outstanding cmd: kernel-%d\n", fcnt.krlcnt);
>>   -    return mlcnt + llcnt + ehcnt + fwcnt;
>> +    return fcnt.mlcnt + fcnt.llcnt + fcnt.ehcnt + fcnt.fwcnt;
>>   }
>>     static int aac_eh_abort(struct scsi_cmnd* cmd)
>> @@ -1675,7 +1677,6 @@ static int aac_probe_one(struct pci_dev *pdev,
>> const struct pci_device_id *id)
>>       shost->irq = pdev->irq;
>>       shost->unique_id = unique_id;
>>       shost->max_cmd_len = 16;
>> -    shost->use_cmd_list = 1;
>>         if (aac_cfg_major == AAC_CHARDEV_NEEDS_REINIT)
>>           aac_init_char();
> 
> Same comment here: could scsi_device_quiesce() and scsi_device_resume()
> have been used instead?
> 
We don't need to.
This function is called during SCSI EH when the host is quiesced anyway.
That's what I tried to imply with the added comment on top.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		      Teamlead Storage & Networking
hare@suse.de			                  +49 911 74053 688
SUSE Software Solutions Germany GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 247165 (AG München), GF: Felix Imendörffer