[PATCH 0/4] scsi: remove legacy cmd_list implementation

[PATCH 0/4] 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.

Hannes Reinecke (4):
  dpt_i2o: use midlayer tcq implementation
  dpt_i2o: make adpt_i2o_to_scsi() a void function
  aacraid: use blk_mq_rq_busy_iter() for traversing outstanding commands
  scsi: Remove cmd_list functionality

 drivers/scsi/aacraid/aachba.c   | 127 ++++++++++++++++++++++------------------
 drivers/scsi/aacraid/comminit.c |  30 ++++------
 drivers/scsi/aacraid/commsup.c  |  38 +++++-------
 drivers/scsi/aacraid/linit.c    |  87 ++++++++++++++-------------
 drivers/scsi/dpt_i2o.c          |  27 ++++-----
 drivers/scsi/dpti.h             |   2 +-
 drivers/scsi/scsi.c             |  14 -----
 drivers/scsi/scsi_error.c       |   1 -
 drivers/scsi/scsi_lib.c         |  32 ----------
 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 -
 14 files changed, 160 insertions(+), 205 deletions(-)

-- 
2.16.4

[PATCH 1/4] dpt_i2o: use midlayer tcq implementation

[Hannes Reinecke]

Switch to use the SCSI midlayer TCQ implementation and drop the
use of the scsi command list.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/dpt_i2o.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index abc74fd474dc..cf0851563f57 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -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;
 }
@@ -2647,20 +2646,19 @@ static s32 adpt_i2o_reparse_lct(adpt_hba* pHba)
 	return 0;
 }
 
-static void adpt_fail_posted_scbs(adpt_hba* pHba)
+bool fail_posted_scbs_iter(struct request *rq, void *data, bool reserved)
 {
-	struct scsi_cmnd* 	cmd = NULL;
-	struct scsi_device* 	d = NULL;
+	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
 
-	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);
-	}
+	cmd->result = (DID_OK << 16) | (QUEUE_FULL <<1);
+	cmd->scsi_done(cmd);
+
+	return true;
+}
+
+static void adpt_fail_posted_scbs(adpt_hba* pHba)
+{
+	blk_mq_tagset_busy_iter(&pHba->host->tag_set, fail_posted_scbs_iter, NULL);
 }
 
 
-- 
2.16.4

[PATCH 2/4] dpt_i2o: make adpt_i2o_to_scsi() a void function

[Hannes Reinecke]

The return value is never used anywhere, and returning it
risks a use-after-free crash.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/dpt_i2o.c | 3 +--
 drivers/scsi/dpti.h    | 2 +-
 2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
index cf0851563f57..9fc3bb53880f 100644
--- a/drivers/scsi/dpt_i2o.c
+++ b/drivers/scsi/dpt_i2o.c
@@ -2340,7 +2340,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_to_scsi(void __iomem *reply, struct scsi_cmnd* cmd)
 {
 	adpt_hba* pHba;
 	u32 hba_status;
@@ -2458,7 +2458,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..1f344a2ac0f5 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_to_scsi(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 3/4] aacraid: use blk_mq_rq_busy_iter() for traversing outstanding commands

[Hannes Reinecke]

Use blk_mq_rq_busy_iter() for traversing outstanding commands and
drop the cmd_list usage.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/aacraid/aachba.c   | 127 ++++++++++++++++++++++------------------
 drivers/scsi/aacraid/comminit.c |  30 ++++------
 drivers/scsi/aacraid/commsup.c  |  38 +++++-------
 drivers/scsi/aacraid/linit.c    |  87 ++++++++++++++-------------
 4 files changed, 147 insertions(+), 135 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index e36608ce937a..df590240e2c9 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -2639,81 +2639,96 @@ static void synchronize_callback(void *context, struct fib *fibptr)
 	cmd->scsi_done(cmd);
 }
 
+struct synchronize_busy_data {
+	struct scsi_device *sdev;
+	u64 lba;
+	u32 count;
+	int active;
+};
+
+static bool synchronize_busy_iter(struct request *req, void *data, bool reserved)
+{
+	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
+	struct synchronize_busy_data *busy_data = data;
+
+	if (busy_data->sdev == cmd->device &&
+	    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
+			return true;
+		if (((cmnd_lba + cmnd_count) < busy_data->lba) ||
+		    (busy_data->count && ((busy_data->lba + busy_data->count) < cmnd_lba)))
+			return true;;
+		++busy_data->active;
+	}
+	return true;
+}
+
 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;
+	struct synchronize_busy_data busy_data = {
+		.sdev = sdev,
+		.lba = lba,
+		.count = count,
+		.active = 0,
+	};
 
 	/*
 	 * 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);
+	blk_mq_tagset_busy_iter(&sdev->host->tag_set, synchronize_busy_iter, &busy_data);
 
 	/*
 	 *	Yield the processor (requeue for later)
 	 */
-	if (active)
+	if (busy_data.active)
 		return SCSI_MLQUEUE_DEVICE_BUSY;
 
 	aac = (struct aac_dev *)sdev->host->hostdata;
diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c
index f75878d773cf..72cdf47437fe 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 request *rq, void *data, bool reserved)
+{
+	struct scsi_cmnd *command = blk_mq_rq_to_pdu(rq);
+	int *active = data;
+
+	if (command->SCp.phase == AAC_OWNER_FIRMWARE)
+		*active = 1;
+	return true;
+}
+
 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;
-
-		}
+		blk_mq_tagset_busy_iter(&aac->scsi_host_ptr->tag_set,
+					wait_for_io_iter, &active);
 		/*
 		 * We can exit If all the commands are complete
 		 */
diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index 5a8a999606ea..218b0954b91c 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -1472,14 +1472,26 @@ static void aac_schedule_bus_scan(struct aac_dev *aac)
 		aac_schedule_src_reinit_aif_worker(aac);
 }
 
+static bool reset_adapter_complete_iter(struct request *rq, void *data, bool reserved)
+{
+	struct scsi_cmnd *command = blk_mq_rq_to_pdu(rq);
+
+	if (command->SCp.phase == AAC_OWNER_FIRMWARE) {
+		command->result = DID_OK << 16
+		  | COMMAND_COMPLETE << 8
+		  | SAM_STAT_TASK_SET_FULL;
+		command->SCp.phase = AAC_OWNER_ERROR_HANDLER;
+		command->scsi_done(command);
+	}
+	return true;
+}
+
 static int _aac_reset_adapter(struct aac_dev *aac, int forced, u8 reset_type)
 {
 	int index, quirks;
 	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 +1619,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);
-	}
+	blk_mq_tagset_busy_iter(&host->tag_set, reset_adapter_complete_iter, NULL);
+
 	/*
 	 * Any Device that was already marked offline needs to be marked
 	 * running
diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index ee6bc2f9b80a..30af9eef6774 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -622,54 +622,62 @@ 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 request *rq, void *data, bool reserved)
 {
+	struct scsi_cmnd *scmnd = blk_mq_rq_to_pdu(rq);
+	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;
+}
+
+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 fib_count = {
+		.mlcnt  = 0,
+		.llcnt  = 0,
+		.ehcnt  = 0,
+		.fwcnt  = 0,
+		.krlcnt = 0,
+	};
 
-	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);
-	}
+	blk_mq_tagset_busy_iter(&shost->tag_set, fib_count_iter, &fib_count);
 
 	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", fib_count.mlcnt);
+	dev_info(ctrl_dev, "outstanding cmd: lowlevel-%d\n", fib_count.llcnt);
+	dev_info(ctrl_dev, "outstanding cmd: error handler-%d\n", fib_count.ehcnt);
+	dev_info(ctrl_dev, "outstanding cmd: firmware-%d\n", fib_count.fwcnt);
+	dev_info(ctrl_dev, "outstanding cmd: kernel-%d\n", fib_count.krlcnt);
 
-	return mlcnt + llcnt + ehcnt + fwcnt;
+	return fib_count.mlcnt + fib_count.llcnt + fib_count.ehcnt + fib_count.fwcnt;
 }
 
 static int aac_eh_abort(struct scsi_cmnd* cmd)
@@ -1675,7 +1683,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

Re: [PATCH 3/4] aacraid: use blk_mq_rq_busy_iter() for traversing outstanding commands

[Bart Van Assche]

On 11/1/19 4:18 AM, Hannes Reinecke wrote:
> +static bool synchronize_busy_iter(struct request *req, void *data, bool reserved)
> +{
> +	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
> +	struct synchronize_busy_data *busy_data = data;
> +
> +	if (busy_data->sdev == cmd->device &&
> +	    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
> +			return true;

The above code looks very similar to the code in scsi_trace.c. Although 
SCSI LLDs shouldn't parse CDBs, there are a few SCSI LLDs that do this. 
Would it be worth it to introduce a function in the SCSI core that 
extracts the most important fields from a CDB (LBA, data buffer size, ...)?

Bart.

Re: [PATCH 3/4] aacraid: use blk_mq_rq_busy_iter() for traversing outstanding commands

[Hannes Reinecke]

On 11/1/19 4:34 PM, Bart Van Assche wrote:
> On 11/1/19 4:18 AM, Hannes Reinecke wrote:
>> +static bool synchronize_busy_iter(struct request *req, void *data, 
>> bool reserved)
>> +{
>> +    struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
>> +    struct synchronize_busy_data *busy_data = data;
>> +
>> +    if (busy_data->sdev == cmd->device &&
>> +        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
>> +            return true;
> 
> The above code looks very similar to the code in scsi_trace.c. Although 
> SCSI LLDs shouldn't parse CDBs, there are a few SCSI LLDs that do this. 
> Would it be worth it to introduce a function in the SCSI core that 
> extracts the most important fields from a CDB (LBA, data buffer size, ...)?
> 
Possibly.
I'll see what I can come up with.

But in either case I'd rather do this as a separate patchset.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke            Teamlead Storage & Networking
hare@suse.de                              +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [PATCH 3/4] aacraid: use blk_mq_rq_busy_iter() for traversing outstanding commands

[kbuild test robot]

Hi Hannes,

I love your patch! Perhaps something to improve:

[auto build test WARNING on mkp-scsi/for-next]
[also build test WARNING on next-20191031]
[cannot apply to v5.4-rc5]
[if your patch is applied to the wrong git tree, please drop us a note to help
improve the system. BTW, we also suggest to use '--base' option to specify the
base tree in git format-patch, please see https://stackoverflow.com/a/37406982]

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/scsi-remove-legacy-cmd_list-implementation/20191102-214752
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next

If you fix the issue, kindly add following tag
Reported-by: kbuild test robot <lkp@intel.com>


coccinelle warnings: (new ones prefixed by >>)

>> drivers/scsi/aacraid/aachba.c:2699:15-16: Unneeded semicolon

Please review and possibly fold the followup patch.

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[PATCH] aacraid: fix semicolon.cocci warnings

[kbuild test robot]

From: kbuild test robot <lkp@intel.com>

drivers/scsi/aacraid/aachba.c:2699:15-16: Unneeded semicolon


 Remove unneeded semicolon.

Generated by: scripts/coccinelle/misc/semicolon.cocci

Fixes: 73021c7fb117 ("aacraid: use blk_mq_rq_busy_iter() for traversing outstanding commands")
CC: Hannes Reinecke <hare@suse.de>
Signed-off-by: kbuild test robot <lkp@intel.com>
---

url:    https://github.com/0day-ci/linux/commits/Hannes-Reinecke/scsi-remove-legacy-cmd_list-implementation/20191102-214752
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mkp/scsi.git for-next

 aachba.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -2696,7 +2696,7 @@ static bool synchronize_busy_iter(struct
 			return true;
 		if (((cmnd_lba + cmnd_count) < busy_data->lba) ||
 		    (busy_data->count && ((busy_data->lba + busy_data->count) < cmnd_lba)))
-			return true;;
+			return true;
 		++busy_data->active;
 	}
 	return true;

[PATCH 4/4] 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>
---
 drivers/scsi/scsi.c        | 14 --------------
 drivers/scsi/scsi_error.c  |  1 -
 drivers/scsi/scsi_lib.c    | 32 --------------------------------
 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, 54 deletions(-)

diff --git a/drivers/scsi/scsi.c b/drivers/scsi/scsi.c
index 4f76841a7038..ae7a1adfa551 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 dc210b9d4896..6b957531aaf0 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -565,7 +565,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 */
@@ -1101,35 +1100,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)
 {
@@ -1158,8 +1128,6 @@ void scsi_init_command(struct scsi_device *dev, struct scsi_cmnd *cmd)
 	INIT_DELAYED_WORK(&cmd->abort_work, scmd_eh_abort_handler);
 	cmd->jiffies_at_alloc = jiffies_at_alloc;
 	cmd->retries = retries;
-
-	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 cc2859d76d81..88b6ef2cc94d 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);
 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 91bd749a02f7..134686e27005 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -158,7 +158,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 2c3f0c58869b..6459ca7969bb 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -641,8 +641,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 4/4] scsi: Remove cmd_list functionality

[Bart Van Assche]

On 11/1/19 4:18 AM, Hannes Reinecke wrote:
> Remove cmd_list functionality; no users left.
> With that the scsi_put_command() becomes empty,
> so remove that one, too.

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

[PATCHv2 0/4] 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

Hannes Reinecke (4):
  dpt_i2o: use midlayer tcq implementation
  dpt_i2o: make adpt_i2o_to_scsi() a void function
  aacraid: use blk_mq_rq_busy_iter() for traversing outstanding commands
  scsi: Remove cmd_list functionality

 drivers/scsi/aacraid/aachba.c   | 127 ++++++++++++++++++++++------------------
 drivers/scsi/aacraid/comminit.c |  30 ++++------
 drivers/scsi/aacraid/commsup.c  |  38 +++++-------
 drivers/scsi/aacraid/linit.c    |  87 ++++++++++++++-------------
 drivers/scsi/dpt_i2o.c          |  28 ++++-----
 drivers/scsi/dpti.h             |   2 +-
 drivers/scsi/scsi.c             |  14 -----
 drivers/scsi/scsi_error.c       |   1 -
 drivers/scsi/scsi_lib.c         |  32 ----------
 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 -
 14 files changed, 161 insertions(+), 205 deletions(-)

-- 
2.16.4

[PATCH 3/4] aacraid: use blk_mq_rq_busy_iter() for traversing outstanding commands

[Hannes Reinecke]

Use blk_mq_rq_busy_iter() for traversing outstanding commands and
drop the cmd_list usage.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/aacraid/aachba.c   | 127 ++++++++++++++++++++++------------------
 drivers/scsi/aacraid/comminit.c |  30 ++++------
 drivers/scsi/aacraid/commsup.c  |  38 +++++-------
 drivers/scsi/aacraid/linit.c    |  87 ++++++++++++++-------------
 4 files changed, 147 insertions(+), 135 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index e36608ce937a..c7bb8bc5ea5e 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -2639,81 +2639,96 @@ static void synchronize_callback(void *context, struct fib *fibptr)
 	cmd->scsi_done(cmd);
 }
 
+struct synchronize_busy_data {
+	struct scsi_device *sdev;
+	u64 lba;
+	u32 count;
+	int active;
+};
+
+static bool synchronize_busy_iter(struct request *req, void *data, bool reserved)
+{
+	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
+	struct synchronize_busy_data *busy_data = data;
+
+	if (busy_data->sdev == cmd->device &&
+	    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
+			return true;
+		if (((cmnd_lba + cmnd_count) < busy_data->lba) ||
+		    (busy_data->count && ((busy_data->lba + busy_data->count) < cmnd_lba)))
+			return true;
+		++busy_data->active;
+	}
+	return true;
+}
+
 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;
+	struct synchronize_busy_data busy_data = {
+		.sdev = sdev,
+		.lba = lba,
+		.count = count,
+		.active = 0,
+	};
 
 	/*
 	 * 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);
+	blk_mq_tagset_busy_iter(&sdev->host->tag_set, synchronize_busy_iter, &busy_data);
 
 	/*
 	 *	Yield the processor (requeue for later)
 	 */
-	if (active)
+	if (busy_data.active)
 		return SCSI_MLQUEUE_DEVICE_BUSY;
 
 	aac = (struct aac_dev *)sdev->host->hostdata;
diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c
index f75878d773cf..72cdf47437fe 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 request *rq, void *data, bool reserved)
+{
+	struct scsi_cmnd *command = blk_mq_rq_to_pdu(rq);
+	int *active = data;
+
+	if (command->SCp.phase == AAC_OWNER_FIRMWARE)
+		*active = 1;
+	return true;
+}
+
 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;
-
-		}
+		blk_mq_tagset_busy_iter(&aac->scsi_host_ptr->tag_set,
+					wait_for_io_iter, &active);
 		/*
 		 * We can exit If all the commands are complete
 		 */
diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index 5a8a999606ea..096b1a107d98 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -1472,14 +1472,26 @@ static void aac_schedule_bus_scan(struct aac_dev *aac)
 		aac_schedule_src_reinit_aif_worker(aac);
 }
 
+static bool reset_adapter_iter(struct request *rq, void *data, bool reserved)
+{
+	struct scsi_cmnd *command = blk_mq_rq_to_pdu(rq);
+
+	if (command->SCp.phase == AAC_OWNER_FIRMWARE) {
+		command->result = DID_OK << 16
+		  | COMMAND_COMPLETE << 8
+		  | SAM_STAT_TASK_SET_FULL;
+		command->SCp.phase = AAC_OWNER_ERROR_HANDLER;
+		command->scsi_done(command);
+	}
+	return true;
+}
+
 static int _aac_reset_adapter(struct aac_dev *aac, int forced, u8 reset_type)
 {
 	int index, quirks;
 	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 +1619,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);
-	}
+	blk_mq_tagset_busy_iter(&host->tag_set, reset_adapter_iter, NULL);
+
 	/*
 	 * Any Device that was already marked offline needs to be marked
 	 * running
diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index ee6bc2f9b80a..30af9eef6774 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -622,54 +622,62 @@ 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 request *rq, void *data, bool reserved)
 {
+	struct scsi_cmnd *scmnd = blk_mq_rq_to_pdu(rq);
+	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;
+}
+
+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 fib_count = {
+		.mlcnt  = 0,
+		.llcnt  = 0,
+		.ehcnt  = 0,
+		.fwcnt  = 0,
+		.krlcnt = 0,
+	};
 
-	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);
-	}
+	blk_mq_tagset_busy_iter(&shost->tag_set, fib_count_iter, &fib_count);
 
 	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", fib_count.mlcnt);
+	dev_info(ctrl_dev, "outstanding cmd: lowlevel-%d\n", fib_count.llcnt);
+	dev_info(ctrl_dev, "outstanding cmd: error handler-%d\n", fib_count.ehcnt);
+	dev_info(ctrl_dev, "outstanding cmd: firmware-%d\n", fib_count.fwcnt);
+	dev_info(ctrl_dev, "outstanding cmd: kernel-%d\n", fib_count.krlcnt);
 
-	return mlcnt + llcnt + ehcnt + fwcnt;
+	return fib_count.mlcnt + fib_count.llcnt + fib_count.ehcnt + fib_count.fwcnt;
 }
 
 static int aac_eh_abort(struct scsi_cmnd* cmd)
@@ -1675,7 +1683,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

Re: [PATCH 3/4] aacraid: use blk_mq_rq_busy_iter() for traversing outstanding commands

[Christoph Hellwig]

Dave and Sagar have been maintaining aacraid for a while, you should
Cc them.

This patch seems to touch fout entirely different areas in aacraid, it
would probably help to split it up into one patch per area explaining how
the replacement for the cmd list for choosen.

> +struct synchronize_busy_data {
> +	struct scsi_device *sdev;
> +	u64 lba;
> +	u32 count;
> +	int active;
> +};
> +
> +static bool synchronize_busy_iter(struct request *req, void *data, bool reserved)

This adds an overly long line. 

> +{
> +	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(req);
> +	struct synchronize_busy_data *busy_data = data;
> +
> +	if (busy_data->sdev == cmd->device &&

Given that you itere over just a single scsi device using
blk_mq_queue_tag_busy_iter here would seem like the better API.

> +	    cmd->SCp.phase == AAC_OWNER_FIRMWARE) {

And the function would become more readable if it just exists early
for not firmware owned commands, as that saves one level of indentation
for all the heavy lifting.

> +		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) {

Instead of reverse engineering the lba and commands, why not check
the request for REQ_OP_WRITE and then look at bi_iter.bi_sector
(also for the caller to avoid the Linux sector to LBA conversion).

> +		if (((cmnd_lba + cmnd_count) < busy_data->lba) ||
> +		    (busy_data->count && ((busy_data->lba + busy_data->count) < cmnd_lba)))

Lots of braces not required here, and overy longs lines.

> +			return true;
> +		++busy_data->active;

Normally we do a post-fix increment if no one cares about the
return value.

> +	blk_mq_tagset_busy_iter(&sdev->host->tag_set, synchronize_busy_iter, &busy_data);

Another overly long line.

> +static bool wait_for_io_iter(struct request *rq, void *data, bool reserved)
> +{
> +	struct scsi_cmnd *command = blk_mq_rq_to_pdu(rq);
> +	int *active = data;
> +
> +	if (command->SCp.phase == AAC_OWNER_FIRMWARE)
> +		*active = 1;
> +	return true;
> +}

Without bloc ayer quiescing this use is a bit of a hack.  Can you
add a comment toward that?

> +static bool reset_adapter_iter(struct request *rq, void *data, bool reserved)
> +{
> +	struct scsi_cmnd *command = blk_mq_rq_to_pdu(rq);
> +
> +	if (command->SCp.phase == AAC_OWNER_FIRMWARE) {
> +		command->result = DID_OK << 16
> +		  | COMMAND_COMPLETE << 8
> +		  | SAM_STAT_TASK_SET_FULL;

The | goes onto the previous line.

> +static int get_num_of_incomplete_fibs(struct aac_dev *aac)
> +{
>  	struct Scsi_Host *shost = aac->scsi_host_ptr;
>  	struct device *ctrl_dev;
> +	struct fib_count_data fib_count = {
> +		.mlcnt  = 0,
> +		.llcnt  = 0,
> +		.ehcnt  = 0,
> +		.fwcnt  = 0,
> +		.krlcnt = 0,
> +	};

You can do a:

	struct fib_count_data fib_count = { };

to zero all values.


> +	dev_info(ctrl_dev, "outstanding cmd: error handler-%d\n", fib_count.ehcnt);

This adds an overly long line.

> +	return fib_count.mlcnt + fib_count.llcnt + fib_count.ehcnt + fib_count.fwcnt;

Another one.