[PATCHv9 00/15] scsi: enabled reserved commands for LLDDs

[PATCHv9 00/15] scsi: enabled reserved commands for LLDDs

[Hannes Reinecke]

Hi all,

quite some drivers use internal commands for various purposes, most
commonly sending TMFs or querying the HBA status.
While these commands use the same submission mechanism than normal
I/O commands, they will not be counted as outstanding commands,
requiring those drivers to implement their own mechanism to figure
out outstanding commands.
The block layer already has the concept of 'reserved' tags for
precisely this purpose, namely non-I/O tags which live off a separate
tag pool. That guarantees that these commands can always be sent,
and won't be influenced by tag starvation from the I/O tag pool.
This patchset enables the use of reserved tags for the SCSI midlayer
by allocating a virtual LUN for the HBA itself which just serves
as a resource to allocate valid tags from.

Command allocation currently ignores the hardware queues, as none
of the modified drivers is mq-capable.

This patchset is being sent out as a base for the current discussion
for enabling reserved commands for the UFS driver; idea is to
base those patches on top of this one if we are agree that this
is the way forward.

The entire patchset can be found at

git://git.kernel.org/pub/scm/linux/kernel/git/hare/scsi-devel.git
reserved-tags.v9

As usual, comments and reviews are welcome.

Changes to v8:
- Drop changes to fnic and snic
- Rebase after scsi_get_host_dev() removal
- Rework aacraid to store callback pointer in host_scribble

Changes to v7:
- Drop changes to hisi_sas, pm8001, and mv_sas
- Drop patch to introduce REQ_INTERNAL flag
- Include reviews from John Garry

Changes to v6:
- Remove patch to drop gdth
- Rework libsas to use a tag per slow task
- Update hisi_sas, pm8001, and mv_sas

Changes to v5:
- Remove patch for csiostor
- Warn on normal commands in scsi_put_reserved_cmd()
- Fixup aacraid to not only scsi_put_internal_cmd() for
  reserved commands
- Add 'nr_reserved_cmds' field to host template
- Reshuffle patches

Changes to v4:
- Fixup kbuild warning
- Include reviews from Bart

Changes to v3:
- Kill gdth
- Only convert fnic, snic, hpsa, and aacraid
- Drop command emulation for pseudo host device
- make 'can_queue' exclude the number or reserved tags
- Drop persistent commands proposal
- Sanitize host device handling

Changes to v2:
- Update patches from John Garry
- Use virtual LUN as suggested by Christoph
- Improve SCSI Host device to present a real SCSI device
- Implement 'persistent' commands for AENs
- Convert Megaraid SAS

Changes to v1:
- Make scsi_{get, put}_reserved_cmd() for Scsi host
- Previously we separate scsi_{get, put}_reserved_cmd() for sdev
  and scsi_host_get_reserved_cmd() for the host
- Fix how Scsi_Host.can_queue is set in the virtio-scsi change
- Drop Scsi_Host.use_reserved_cmd_q
- Drop scsi_is_reserved_cmd()
- Add support in libsas and associated HBA drivers
- Allocate reserved command in slow task
- Switch hisi_sas to use reserved Scsi command
- Reorder the series a little
- Some tidying

Hannes Reinecke (15):
  scsi: allocate host device
  scsi: add scsi_{get,put}_internal_cmd() helper
  scsi: implement reserved command handling
  hpsa: move hpsa_hba_inquiry after scsi_add_host()
  hpsa: use reserved commands
  hpsa: use scsi_host_busy_iter() to traverse outstanding commands
  hpsa: drop refcount field from CommandList
  aacraid: return valid status from aac_scsi_cmd()
  aacraid: don't bother with setting SCp.Status
  aacraid: move scsi_add_host()
  aacraid: move container ID into struct fib
  aacraid: fsa_dev pointer is always valid
  aacraid: store callback in scsi_cmnd.host_scribble
  aacraid: use scsi_get_internal_cmd()
  aacraid: use scsi_host_busy_iter() to traverse outstanding commands

 drivers/scsi/aacraid/aachba.c   | 208 +++++++++---------
 drivers/scsi/aacraid/aacraid.h  |  15 +-
 drivers/scsi/aacraid/commctrl.c |  25 ++-
 drivers/scsi/aacraid/comminit.c |   2 +-
 drivers/scsi/aacraid/commsup.c  | 115 +++++-----
 drivers/scsi/aacraid/dpcsup.c   |   2 +-
 drivers/scsi/aacraid/linit.c    | 170 +++++++--------
 drivers/scsi/hosts.c            |  11 +
 drivers/scsi/hpsa.c             | 364 ++++++++++++++------------------
 drivers/scsi/hpsa.h             |   1 -
 drivers/scsi/hpsa_cmd.h         |  10 -
 drivers/scsi/scsi_lib.c         |  53 ++++-
 drivers/scsi/scsi_scan.c        |  67 +++++-
 drivers/scsi/scsi_sysfs.c       |   3 +-
 include/scsi/scsi_device.h      |   5 +-
 include/scsi/scsi_host.h        |  43 +++-
 16 files changed, 595 insertions(+), 499 deletions(-)

-- 
2.29.2

[PATCH 01/15] scsi: allocate host device

[Hannes Reinecke]

Add a flag 'alloc_host_dev' to the SCSI host template and allocate
a virtual scsi device if the flag is set.
This device has the SCSI id <max_id + 1>:0, so won't clash with any
devices the HBA might allocate. It's also excluded from scanning and
will not show up in sysfs.
Intention is to use this device to send internal commands to the HBA.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/hosts.c       |  8 +++++
 drivers/scsi/scsi_scan.c   | 67 +++++++++++++++++++++++++++++++++++++-
 drivers/scsi/scsi_sysfs.c  |  3 +-
 include/scsi/scsi_device.h |  2 +-
 include/scsi/scsi_host.h   | 21 ++++++++++++
 5 files changed, 98 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index f69b77cbf538..a539fa2fb221 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -290,6 +290,14 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 	if (error)
 		goto out_del_dev;
 
+	if (sht->alloc_host_sdev) {
+		shost->shost_sdev = scsi_get_host_dev(shost);
+		if (!shost->shost_sdev) {
+			error = -ENOMEM;
+			goto out_del_dev;
+		}
+	}
+
 	scsi_proc_host_add(shost);
 	scsi_autopm_put_host(shost);
 	return error;
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 328c0e79dfe7..e2910aa02a65 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1139,6 +1139,12 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
 	if (!sdev)
 		goto out;
 
+	if (scsi_device_is_host_dev(sdev)) {
+		if (bflagsp)
+			*bflagsp = BLIST_NOLUN;
+		return SCSI_SCAN_LUN_PRESENT;
+	}
+
 	result = kmalloc(result_len, GFP_KERNEL);
 	if (!result)
 		goto out_free_sdev;
@@ -1755,6 +1761,9 @@ static void scsi_sysfs_add_devices(struct Scsi_Host *shost)
 		/* If device is already visible, skip adding it to sysfs */
 		if (sdev->is_visible)
 			continue;
+		/* Host devices should never be visible in sysfs */
+		if (scsi_device_is_host_dev(sdev))
+			continue;
 		if (!scsi_host_scan_allowed(shost) ||
 		    scsi_sysfs_add_sdev(sdev) != 0)
 			__scsi_remove_device(sdev);
@@ -1919,12 +1928,16 @@ EXPORT_SYMBOL(scsi_scan_host);
 
 void scsi_forget_host(struct Scsi_Host *shost)
 {
-	struct scsi_device *sdev;
+	struct scsi_device *sdev, *host_sdev = NULL;
 	unsigned long flags;
 
  restart:
 	spin_lock_irqsave(shost->host_lock, flags);
 	list_for_each_entry(sdev, &shost->__devices, siblings) {
+		if (scsi_device_is_host_dev(sdev)) {
+			host_sdev = sdev;
+			continue;
+		}
 		if (sdev->sdev_state == SDEV_DEL)
 			continue;
 		spin_unlock_irqrestore(shost->host_lock, flags);
@@ -1932,5 +1945,57 @@ void scsi_forget_host(struct Scsi_Host *shost)
 		goto restart;
 	}
 	spin_unlock_irqrestore(shost->host_lock, flags);
+	/* Remove host device last, might be needed to send commands */
+	if (host_sdev)
+		__scsi_remove_device(host_sdev);
 }
 
+/**
+ * scsi_get_host_dev - Create a virtual scsi_device to the host adapter
+ * @shost: Host that needs a scsi_device
+ *
+ * Lock status: None assumed.
+ *
+ * Returns:     The scsi_device or NULL
+ *
+ * Notes:
+ *	Attach a single scsi_device to the Scsi_Host. The primary aim
+ *	for this device is to serve as a container from which valid
+ *	scsi commands can be allocated from. Each scsi command will carry
+ *	an unused/free command tag, which then can be used by the LLDD to
+ *	send internal or passthrough commands without having to find a
+ *	valid command tag internally.
+ */
+struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost)
+{
+	struct scsi_device *sdev = NULL;
+	struct scsi_target *starget;
+
+	mutex_lock(&shost->scan_mutex);
+	if (!scsi_host_scan_allowed(shost))
+		goto out;
+	starget = scsi_alloc_target(&shost->shost_gendev, 0,
+				    shost->max_id);
+	if (!starget)
+		goto out;
+
+	sdev = scsi_alloc_sdev(starget, 0, NULL);
+	if (sdev)
+		sdev->borken = 0;
+	else
+		scsi_target_reap(starget);
+	put_device(&starget->dev);
+ out:
+	mutex_unlock(&shost->scan_mutex);
+	return sdev;
+}
+EXPORT_SYMBOL(scsi_get_host_dev);
+
+/*
+ * Test if a given device is a SCSI host device
+ */
+bool scsi_device_is_host_dev(struct scsi_device *sdev)
+{
+	return sdev == sdev->host->shost_sdev;
+}
+EXPORT_SYMBOL_GPL(scsi_device_is_host_dev);
diff --git a/drivers/scsi/scsi_sysfs.c b/drivers/scsi/scsi_sysfs.c
index 61839773cc72..a6e0a70ca6f5 100644
--- a/drivers/scsi/scsi_sysfs.c
+++ b/drivers/scsi/scsi_sysfs.c
@@ -500,7 +500,8 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
 		kfree_rcu(vpd_pg80, rcu);
 	if (vpd_pg89)
 		kfree_rcu(vpd_pg89, rcu);
-	kfree(sdev->inquiry);
+	if (!scsi_device_is_host_dev(sdev))
+		kfree(sdev->inquiry);
 	kfree(sdev);
 
 	if (parent)
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index d1c6fc83b1e3..5d7204186831 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -604,7 +604,7 @@ static inline int scsi_device_busy(struct scsi_device *sdev)
 	return sbitmap_weight(&sdev->budget_map);
 }
 
-#define MODULE_ALIAS_SCSI_DEVICE(type) \
+#define MODULE_ALIAS_SCSI_DEVICE(type)			\
 	MODULE_ALIAS("scsi:t-" __stringify(type) "*")
 #define SCSI_DEVICE_MODALIAS_FMT "scsi:t-0x%02x"
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 72e1a347baa6..6f49a8940dc4 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -459,6 +459,9 @@ struct scsi_host_template {
 	/* True if the host uses host-wide tagspace */
 	unsigned host_tagset:1;
 
+	/* True if a host sdev should be allocated */
+	unsigned alloc_host_sdev:1;
+
 	/*
 	 * Countdown for host blocking with no commands outstanding.
 	 */
@@ -704,6 +707,12 @@ struct Scsi_Host {
 	 */
 	struct device *dma_dev;
 
+	/*
+	 * Points to a virtual SCSI device used for sending
+	 * internal commands to the HBA.
+	 */
+	struct scsi_device *shost_sdev;
+
 	/*
 	 * We should ensure that this is aligned, both for better performance
 	 * and also because some compilers (m68k) don't automatically force
@@ -793,6 +802,18 @@ void scsi_host_busy_iter(struct Scsi_Host *,
 
 struct class_container;
 
+/*
+ * These functions are used to allocate and test a pseudo device
+ * which will refer to the host adapter itself rather than any
+ * physical device.  The device will be deallocated together with
+ * all other scsi devices, so there is no need to have a separate
+ * function to free it.
+ * This device will not show up in sysfs and won't be available
+ * from any high-level drivers.
+ */
+struct scsi_device *scsi_get_host_dev(struct Scsi_Host *);
+bool scsi_device_is_host_dev(struct scsi_device *sdev);
+
 /*
  * DIF defines the exchange of protection information between
  * initiator and SBC block device.
-- 
2.29.2

[PATCH 02/15] scsi: add scsi_{get,put}_internal_cmd() helper

[Hannes Reinecke]

Add helper functions to allow LLDDs to allocate and free
internal commands.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/scsi_lib.c    | 44 ++++++++++++++++++++++++++++++++++++++
 include/scsi/scsi_device.h |  3 +++
 2 files changed, 47 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 621d841d819a..6fbd36c9c416 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1957,6 +1957,50 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost)
 	blk_mq_free_tag_set(&shost->tag_set);
 }
 
+/**
+ * scsi_get_internal_cmd - allocate an internal SCSI command
+ * @sdev: SCSI device from which to allocate the command
+ * @data_direction: Data direction for the allocated command
+ * @nowait: do not wait for command allocation to succeed.
+ *
+ * Allocates a SCSI command for internal LLDD use.
+ */
+struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,
+	int data_direction, bool nowait)
+{
+	struct request *rq;
+	struct scsi_cmnd *scmd;
+	blk_mq_req_flags_t flags = 0;
+	int op;
+
+	if (nowait)
+		flags |= BLK_MQ_REQ_NOWAIT;
+	op = (data_direction == DMA_TO_DEVICE) ?
+		REQ_OP_DRV_OUT : REQ_OP_DRV_IN;
+	rq = blk_mq_alloc_request(sdev->request_queue, op, flags);
+	if (IS_ERR(rq))
+		return NULL;
+	scmd = blk_mq_rq_to_pdu(rq);
+	scmd->device = sdev;
+	return scmd;
+}
+EXPORT_SYMBOL_GPL(scsi_get_internal_cmd);
+
+/**
+ * scsi_put_internal_cmd - free an internal SCSI command
+ * @scmd: SCSI command to be freed
+ *
+ * Check if @scmd is an internal command, and call
+ * blk_mq_free_request() if true.
+ */
+void scsi_put_internal_cmd(struct scsi_cmnd *scmd)
+{
+	struct request *rq = blk_mq_rq_from_pdu(scmd);
+
+	blk_mq_free_request(rq);
+}
+EXPORT_SYMBOL_GPL(scsi_put_internal_cmd);
+
 /**
  * scsi_device_from_queue - return sdev associated with a request_queue
  * @q: The request queue to return the sdev from
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 5d7204186831..218b541515bf 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -470,6 +470,9 @@ static inline int scsi_execute_req(struct scsi_device *sdev,
 	return scsi_execute(sdev, cmd, data_direction, buffer,
 		bufflen, NULL, sshdr, timeout, retries,  0, 0, resid);
 }
+struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,
+	int data_direction, bool nowait);
+void scsi_put_internal_cmd(struct scsi_cmnd *scmd);
 extern void sdev_disable_disk_events(struct scsi_device *sdev);
 extern void sdev_enable_disk_events(struct scsi_device *sdev);
 extern int scsi_vpd_lun_id(struct scsi_device *, char *, size_t);
-- 
2.29.2

[PATCH 03/15] scsi: implement reserved command handling

[Hannes Reinecke]

Quite some drivers are using management commands internally, which
typically use the same hardware tag pool (ie they are being allocated
from the same hardware resources) as the 'normal' I/O commands.
These commands are set aside before allocating the block-mq tag bitmap,
so they'll never show up as busy in the tag map.
The block-layer, OTOH, already has 'reserved_tags' to handle precisely
this situation.
So this patch adds a new field 'nr_reserved_cmds' to the SCSI host
template to instruct the block layer to set aside a tag space for these
management commands by using reserved tags.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/hosts.c     |  3 +++
 drivers/scsi/scsi_lib.c  |  9 ++++++++-
 include/scsi/scsi_host.h | 22 +++++++++++++++++++++-
 3 files changed, 32 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index a539fa2fb221..8ee7a7279b6b 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -482,6 +482,9 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
 	if (sht->virt_boundary_mask)
 		shost->virt_boundary_mask = sht->virt_boundary_mask;
 
+	if (sht->nr_reserved_cmds)
+		shost->nr_reserved_cmds = sht->nr_reserved_cmds;
+
 	device_initialize(&shost->shost_gendev);
 	dev_set_name(&shost->shost_gendev, "host%d", shost->host_no);
 	shost->shost_gendev.bus = &scsi_bus_type;
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 6fbd36c9c416..e8f1025d0ed8 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1939,7 +1939,9 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
 		tag_set->ops = &scsi_mq_ops_no_commit;
 	tag_set->nr_hw_queues = shost->nr_hw_queues ? : 1;
 	tag_set->nr_maps = shost->nr_maps ? : 1;
-	tag_set->queue_depth = shost->can_queue;
+	tag_set->queue_depth =
+		shost->can_queue + shost->nr_reserved_cmds;
+	tag_set->reserved_tags = shost->nr_reserved_cmds;
 	tag_set->cmd_size = cmd_size;
 	tag_set->numa_node = NUMA_NO_NODE;
 	tag_set->flags = BLK_MQ_F_SHOULD_MERGE;
@@ -1964,6 +1966,9 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost)
  * @nowait: do not wait for command allocation to succeed.
  *
  * Allocates a SCSI command for internal LLDD use.
+ * If 'nr_reserved_commands' is spectified by the host the
+ * command will be allocated from the reserved tag pool;
+ * otherwise the normal tag pool will be used.
  */
 struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,
 	int data_direction, bool nowait)
@@ -1973,6 +1978,8 @@ struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,
 	blk_mq_req_flags_t flags = 0;
 	int op;
 
+	if (sdev->host->nr_reserved_cmds)
+		flags |= BLK_MQ_REQ_RESERVED;
 	if (nowait)
 		flags |= BLK_MQ_REQ_NOWAIT;
 	op = (data_direction == DMA_TO_DEVICE) ?
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 6f49a8940dc4..7512d97aceb4 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -367,10 +367,19 @@ struct scsi_host_template {
 	/*
 	 * This determines if we will use a non-interrupt driven
 	 * or an interrupt driven scheme.  It is set to the maximum number
-	 * of simultaneous commands a single hw queue in HBA will accept.
+	 * of simultaneous commands a single hw queue in HBA will accept
+	 * excluding internal commands.
 	 */
 	int can_queue;
 
+	/*
+	 * This determines how many commands the HBA will set aside
+	 * for internal commands. This number will be added to
+	 * @can_queue to calcumate the maximum number of simultaneous
+	 * commands sent to the host.
+	 */
+	int nr_reserved_cmds;
+
 	/*
 	 * In many instances, especially where disconnect / reconnect are
 	 * supported, our host also has an ID on the SCSI bus.  If this is
@@ -608,6 +617,11 @@ struct Scsi_Host {
 	unsigned short max_cmd_len;
 
 	int this_id;
+
+	/*
+	 * Number of commands this host can handle at the same time.
+	 * This excludes reserved commands as specified by nr_reserved_cmds.
+	 */
 	int can_queue;
 	short cmd_per_lun;
 	short unsigned int sg_tablesize;
@@ -626,6 +640,12 @@ struct Scsi_Host {
 	 */
 	unsigned nr_hw_queues;
 	unsigned nr_maps;
+
+	/*
+	 * Number of reserved commands to allocate, if any.
+	 */
+	unsigned nr_reserved_cmds;
+
 	unsigned active_mode:2;
 
 	/*
-- 
2.29.2

[PATCH 04/15] hpsa: move hpsa_hba_inquiry after scsi_add_host()

[Hannes Reinecke]

Move hpsa_hba_inquiry to after scsi_add_host() so that the host
is fully initialized.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Tested-by: Don Brace <don.brace@microchip.com>
---
 drivers/scsi/hpsa.c | 37 +++++++++++++++++++------------------
 1 file changed, 19 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index cdf3328cc065..d0a7d1086c74 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -5877,6 +5877,22 @@ static int hpsa_scsi_host_alloc(struct ctlr_info *h)
 	return 0;
 }
 
+static void hpsa_hba_inquiry(struct ctlr_info *h)
+{
+	int rc;
+
+#define HBA_INQUIRY_BYTE_COUNT 64
+	h->hba_inquiry_data = kmalloc(HBA_INQUIRY_BYTE_COUNT, GFP_KERNEL);
+	if (!h->hba_inquiry_data)
+		return;
+	rc = hpsa_scsi_do_inquiry(h, RAID_CTLR_LUNID, 0,
+		h->hba_inquiry_data, HBA_INQUIRY_BYTE_COUNT);
+	if (rc != 0) {
+		kfree(h->hba_inquiry_data);
+		h->hba_inquiry_data = NULL;
+	}
+}
+
 static int hpsa_scsi_add_host(struct ctlr_info *h)
 {
 	int rv;
@@ -5886,6 +5902,9 @@ static int hpsa_scsi_add_host(struct ctlr_info *h)
 		dev_err(&h->pdev->dev, "scsi_add_host failed\n");
 		return rv;
 	}
+
+	hpsa_hba_inquiry(h);
+
 	scsi_scan_host(h->scsi_host);
 	return 0;
 }
@@ -7952,22 +7971,6 @@ static int hpsa_pci_init(struct ctlr_info *h)
 	return err;
 }
 
-static void hpsa_hba_inquiry(struct ctlr_info *h)
-{
-	int rc;
-
-#define HBA_INQUIRY_BYTE_COUNT 64
-	h->hba_inquiry_data = kmalloc(HBA_INQUIRY_BYTE_COUNT, GFP_KERNEL);
-	if (!h->hba_inquiry_data)
-		return;
-	rc = hpsa_scsi_do_inquiry(h, RAID_CTLR_LUNID, 0,
-		h->hba_inquiry_data, HBA_INQUIRY_BYTE_COUNT);
-	if (rc != 0) {
-		kfree(h->hba_inquiry_data);
-		h->hba_inquiry_data = NULL;
-	}
-}
-
 static int hpsa_init_reset_devices(struct pci_dev *pdev, u32 board_id)
 {
 	int rc, i;
@@ -8872,8 +8875,6 @@ static int hpsa_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
 	/* Turn the interrupts on so we can service requests */
 	h->access.set_intr_mask(h, HPSA_INTR_ON);
 
-	hpsa_hba_inquiry(h);
-
 	h->lastlogicals = kzalloc(sizeof(*(h->lastlogicals)), GFP_KERNEL);
 	if (!h->lastlogicals)
 		dev_info(&h->pdev->dev,
-- 
2.29.2

[PATCH 05/15] hpsa: use reserved commands

[Hannes Reinecke]

Enable the use of reserved commands, and drop the hand-crafted
command allocation.

Signed-off-by: Hannes Reinecke <hare@suse.de>
Tested-by: Don Brace <don.brace@microchip.com>
---
 drivers/scsi/hpsa.c | 199 ++++++++++++++++----------------------------
 drivers/scsi/hpsa.h |   1 -
 2 files changed, 71 insertions(+), 129 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index d0a7d1086c74..c5f55b56fd2f 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -244,10 +244,6 @@ static struct hpsa_scsi_dev_t
 	*hpsa_find_device_by_sas_rphy(struct ctlr_info *h,
 		struct sas_rphy *rphy);
 
-#define SCSI_CMD_BUSY ((struct scsi_cmnd *)&hpsa_cmd_busy)
-static const struct scsi_cmnd hpsa_cmd_busy;
-#define SCSI_CMD_IDLE ((struct scsi_cmnd *)&hpsa_cmd_idle)
-static const struct scsi_cmnd hpsa_cmd_idle;
 static int number_of_controllers;
 
 static irqreturn_t do_hpsa_intr_intx(int irq, void *dev_id);
@@ -265,7 +261,7 @@ static int hpsa_compat_ioctl(struct scsi_device *dev, unsigned int cmd,
 #endif
 
 static void cmd_free(struct ctlr_info *h, struct CommandList *c);
-static struct CommandList *cmd_alloc(struct ctlr_info *h);
+static struct CommandList *cmd_alloc(struct ctlr_info *h, u8 direction);
 static void cmd_tagged_free(struct ctlr_info *h, struct CommandList *c);
 static struct CommandList *cmd_tagged_alloc(struct ctlr_info *h,
 					    struct scsi_cmnd *scmd);
@@ -346,7 +342,7 @@ static inline struct ctlr_info *shost_to_hba(struct Scsi_Host *sh)
 
 static inline bool hpsa_is_cmd_idle(struct CommandList *c)
 {
-	return c->scsi_cmd == SCSI_CMD_IDLE;
+	return c->scsi_cmd == NULL;
 }
 
 /* extract sense key, asc, and ascq from sense data.  -1 means invalid. */
@@ -988,6 +984,7 @@ static struct scsi_host_template hpsa_driver_template = {
 	.shost_groups = hpsa_shost_groups,
 	.max_sectors = 2048,
 	.no_write_same = 1,
+	.alloc_host_sdev = 1,
 };
 
 static inline u32 next_command(struct ctlr_info *h, u8 q)
@@ -2465,7 +2462,12 @@ static void hpsa_cmd_resolve_events(struct ctlr_info *h,
 	 * this command has completed.  Then, check to see if the handler is
 	 * waiting for this command, and, if so, wake it.
 	 */
-	c->scsi_cmd = SCSI_CMD_IDLE;
+	if (c->scsi_cmd && c->cmd_type == CMD_IOCTL_PEND) {
+		struct scsi_cmnd *scmd = c->scsi_cmd;
+
+		scsi_put_internal_cmd(scmd);
+	}
+	c->scsi_cmd = NULL;
 	mb();	/* Declare command idle before checking for pending events. */
 	if (dev) {
 		atomic_dec(&dev->commands_outstanding);
@@ -3007,7 +3009,7 @@ static int hpsa_do_receive_diagnostic(struct ctlr_info *h, u8 *scsi3addr,
 	struct CommandList *c;
 	struct ErrorInfo *ei;
 
-	c = cmd_alloc(h);
+	c = cmd_alloc(h, XFER_READ);
 	if (fill_cmd(c, RECEIVE_DIAGNOSTIC, h, buf, bufsize,
 			page, scsi3addr, TYPE_CMD)) {
 		rc = -1;
@@ -3059,7 +3061,7 @@ static int hpsa_scsi_do_inquiry(struct ctlr_info *h, unsigned char *scsi3addr,
 	struct CommandList *c;
 	struct ErrorInfo *ei;
 
-	c = cmd_alloc(h);
+	c = cmd_alloc(h, XFER_READ);
 
 	if (fill_cmd(c, HPSA_INQUIRY, h, buf, bufsize,
 			page, scsi3addr, TYPE_CMD)) {
@@ -3087,7 +3089,7 @@ static int hpsa_send_reset(struct ctlr_info *h, struct hpsa_scsi_dev_t *dev,
 	struct CommandList *c;
 	struct ErrorInfo *ei;
 
-	c = cmd_alloc(h);
+	c = cmd_alloc(h, XFER_NONE);
 	c->device = dev;
 
 	/* fill_cmd can't fail here, no data buffer to map. */
@@ -3313,7 +3315,7 @@ static int hpsa_get_raid_map(struct ctlr_info *h,
 	struct CommandList *c;
 	struct ErrorInfo *ei;
 
-	c = cmd_alloc(h);
+	c = cmd_alloc(h, XFER_READ);
 
 	if (fill_cmd(c, HPSA_GET_RAID_MAP, h, &this_device->raid_map,
 			sizeof(this_device->raid_map), 0,
@@ -3355,7 +3357,7 @@ static int hpsa_bmic_sense_subsystem_information(struct ctlr_info *h,
 	struct CommandList *c;
 	struct ErrorInfo *ei;
 
-	c = cmd_alloc(h);
+	c = cmd_alloc(h, XFER_READ);
 
 	rc = fill_cmd(c, BMIC_SENSE_SUBSYSTEM_INFORMATION, h, buf, bufsize,
 		0, RAID_CTLR_LUNID, TYPE_CMD);
@@ -3386,7 +3388,7 @@ static int hpsa_bmic_id_controller(struct ctlr_info *h,
 	struct CommandList *c;
 	struct ErrorInfo *ei;
 
-	c = cmd_alloc(h);
+	c = cmd_alloc(h, XFER_READ);
 
 	rc = fill_cmd(c, BMIC_IDENTIFY_CONTROLLER, h, buf, bufsize,
 		0, RAID_CTLR_LUNID, TYPE_CMD);
@@ -3415,7 +3417,7 @@ static int hpsa_bmic_id_physical_device(struct ctlr_info *h,
 	struct CommandList *c;
 	struct ErrorInfo *ei;
 
-	c = cmd_alloc(h);
+	c = cmd_alloc(h, XFER_READ);
 	rc = fill_cmd(c, BMIC_IDENTIFY_PHYSICAL_DEVICE, h, buf, bufsize,
 		0, RAID_CTLR_LUNID, TYPE_CMD);
 	if (rc)
@@ -3492,7 +3494,7 @@ static void hpsa_get_enclosure_info(struct ctlr_info *h,
 		goto out;
 	}
 
-	c = cmd_alloc(h);
+	c = cmd_alloc(h, XFER_READ);
 
 	rc = fill_cmd(c, BMIC_SENSE_STORAGE_BOX_PARAMS, h, bssbp,
 			sizeof(*bssbp), 0, RAID_CTLR_LUNID, TYPE_CMD);
@@ -3748,7 +3750,7 @@ static int hpsa_scsi_do_report_luns(struct ctlr_info *h, int logical,
 	unsigned char scsi3addr[8];
 	struct ErrorInfo *ei;
 
-	c = cmd_alloc(h);
+	c = cmd_alloc(h, XFER_READ);
 
 	/* address the controller */
 	memset(scsi3addr, 0, sizeof(scsi3addr));
@@ -3889,7 +3891,7 @@ static unsigned char hpsa_volume_offline(struct ctlr_info *h,
 #define ASCQ_LUN_NOT_READY_FORMAT_IN_PROGRESS 0x04
 #define ASCQ_LUN_NOT_READY_INITIALIZING_CMD_REQ 0x02
 
-	c = cmd_alloc(h);
+	c = cmd_alloc(h, XFER_NONE);
 
 	(void) fill_cmd(c, TEST_UNIT_READY, h, NULL, 0, 0, scsi3addr, TYPE_CMD);
 	rc = hpsa_scsi_do_simple_cmd(h, c, DEFAULT_REPLY_QUEUE,
@@ -5545,7 +5547,6 @@ static void hpsa_cmd_init(struct ctlr_info *h, int index,
 	c->ErrDesc.Addr = cpu_to_le64((u64) err_dma_handle);
 	c->ErrDesc.Len = cpu_to_le32((u32) sizeof(*c->err_info));
 	c->h = h;
-	c->scsi_cmd = SCSI_CMD_IDLE;
 }
 
 static void hpsa_preinitialize_commands(struct ctlr_info *h)
@@ -5860,12 +5861,12 @@ static int hpsa_scsi_host_alloc(struct ctlr_info *h)
 
 	sh->io_port = 0;
 	sh->n_io_port = 0;
-	sh->this_id = -1;
 	sh->max_channel = 3;
 	sh->max_cmd_len = MAX_COMMAND_SIZE;
 	sh->max_lun = HPSA_MAX_LUN;
 	sh->max_id = HPSA_MAX_LUN;
 	sh->can_queue = h->nr_cmds - HPSA_NRESERVED_CMDS;
+	sh->nr_reserved_cmds = HPSA_NRESERVED_CMDS;
 	sh->cmd_per_lun = sh->can_queue;
 	sh->sg_tablesize = h->maxsgentries;
 	sh->transportt = hpsa_sas_transport_template;
@@ -5909,23 +5910,6 @@ static int hpsa_scsi_add_host(struct ctlr_info *h)
 	return 0;
 }
 
-/*
- * The block layer has already gone to the trouble of picking out a unique,
- * small-integer tag for this request.  We use an offset from that value as
- * an index to select our command block.  (The offset allows us to reserve the
- * low-numbered entries for our own uses.)
- */
-static int hpsa_get_cmd_index(struct scsi_cmnd *scmd)
-{
-	int idx = scsi_cmd_to_rq(scmd)->tag;
-
-	if (idx < 0)
-		return idx;
-
-	/* Offset to leave space for internal cmds. */
-	return idx += HPSA_NRESERVED_CMDS;
-}
-
 /*
  * Send a TEST_UNIT_READY command to the specified LUN using the specified
  * reply queue; returns zero if the unit is ready, and non-zero otherwise.
@@ -6009,7 +5993,7 @@ static int wait_for_device_to_become_ready(struct ctlr_info *h,
 	int rc = 0;
 	struct CommandList *c;
 
-	c = cmd_alloc(h);
+	c = cmd_alloc(h, XFER_NONE);
 
 	/*
 	 * If no specific reply queue was requested, then send the TUR
@@ -6082,7 +6066,7 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
 	if (lockup_detected(h)) {
 		snprintf(msg, sizeof(msg),
 			 "cmd %d RESET FAILED, lockup detected",
-			 hpsa_get_cmd_index(scsicmd));
+			 scsi_cmd_to_rq(scsicmd)->tag);
 		hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 		rc = FAILED;
 		goto return_reset_status;
@@ -6092,7 +6076,7 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
 	if (detect_controller_lockup(h)) {
 		snprintf(msg, sizeof(msg),
 			 "cmd %d RESET FAILED, new lockup detected",
-			 hpsa_get_cmd_index(scsicmd));
+			 scsi_cmd_to_rq(scsicmd)->tag);
 		hpsa_show_dev_msg(KERN_WARNING, h, dev, msg);
 		rc = FAILED;
 		goto return_reset_status;
@@ -6155,12 +6139,12 @@ static int hpsa_eh_device_reset_handler(struct scsi_cmnd *scsicmd)
 static struct CommandList *cmd_tagged_alloc(struct ctlr_info *h,
 					    struct scsi_cmnd *scmd)
 {
-	int idx = hpsa_get_cmd_index(scmd);
+	int idx = scsi_cmd_to_rq(scmd)->tag;
 	struct CommandList *c = h->cmd_pool + idx;
 
-	if (idx < HPSA_NRESERVED_CMDS || idx >= h->nr_cmds) {
+	if (idx < 0 || idx >= h->nr_cmds) {
 		dev_err(&h->pdev->dev, "Bad block tag: %d not in [%d..%d]\n",
-			idx, HPSA_NRESERVED_CMDS, h->nr_cmds - 1);
+			idx, 0, h->nr_cmds - 1);
 		/* The index value comes from the block layer, so if it's out of
 		 * bounds, it's probably not our bug.
 		 */
@@ -6203,62 +6187,33 @@ static void cmd_tagged_free(struct ctlr_info *h, struct CommandList *c)
 	 * else to free it, because it is accessed by index.
 	 */
 	(void)atomic_dec(&c->refcount);
+	c->scsi_cmd = NULL;
 }
 
-/*
- * For operations that cannot sleep, a command block is allocated at init,
- * and managed by cmd_alloc() and cmd_free() using a simple bitmap to track
- * which ones are free or in use.  Lock must be held when calling this.
- * cmd_free() is the complement.
- * This function never gives up and returns NULL.  If it hangs,
- * another thread must call cmd_free() to free some tags.
- */
-
-static struct CommandList *cmd_alloc(struct ctlr_info *h)
+static struct CommandList *cmd_alloc(struct ctlr_info *h, u8 direction)
 {
+	struct scsi_cmnd *scmd;
 	struct CommandList *c;
-	int refcount, i;
-	int offset = 0;
-
-	/*
-	 * There is some *extremely* small but non-zero chance that that
-	 * multiple threads could get in here, and one thread could
-	 * be scanning through the list of bits looking for a free
-	 * one, but the free ones are always behind him, and other
-	 * threads sneak in behind him and eat them before he can
-	 * get to them, so that while there is always a free one, a
-	 * very unlucky thread might be starved anyway, never able to
-	 * beat the other threads.  In reality, this happens so
-	 * infrequently as to be indistinguishable from never.
-	 *
-	 * Note that we start allocating commands before the SCSI host structure
-	 * is initialized.  Since the search starts at bit zero, this
-	 * all works, since we have at least one command structure available;
-	 * however, it means that the structures with the low indexes have to be
-	 * reserved for driver-initiated requests, while requests from the block
-	 * layer will use the higher indexes.
-	 */
-
-	for (;;) {
-		i = find_next_zero_bit(h->cmd_pool_bits,
-					HPSA_NRESERVED_CMDS,
-					offset);
-		if (unlikely(i >= HPSA_NRESERVED_CMDS)) {
-			offset = 0;
-			continue;
-		}
-		c = h->cmd_pool + i;
-		refcount = atomic_inc_return(&c->refcount);
-		if (unlikely(refcount > 1)) {
-			cmd_free(h, c); /* already in use */
-			offset = (i + 1) % HPSA_NRESERVED_CMDS;
-			continue;
-		}
-		set_bit(i & (BITS_PER_LONG - 1),
-			h->cmd_pool_bits + (i / BITS_PER_LONG));
-		break; /* it's ours now. */
+	int idx;
+
+	scmd = scsi_get_internal_cmd(h->scsi_host->shost_sdev,
+				     (direction & XFER_WRITE) ?
+				     DMA_TO_DEVICE : DMA_FROM_DEVICE,
+				     true);
+	if (!scmd) {
+		dev_warn(&h->pdev->dev, "failed to allocate reserved cmd\n");
+		return NULL;
+	}
+	idx = scsi_cmd_to_rq(scmd)->tag;
+	c = cmd_tagged_alloc(h, scmd);
+	if (!c) {
+		dev_warn(&h->pdev->dev, "failed to allocate reserved cmd %u\n",
+			 idx);
+		scsi_put_internal_cmd(scmd);
+		return NULL;
 	}
-	hpsa_cmd_partial_init(h, i, c);
+	hpsa_cmd_partial_init(h, idx, c);
+	c->scsi_cmd = scmd;
 	c->device = NULL;
 
 	/*
@@ -6266,24 +6221,24 @@ static struct CommandList *cmd_alloc(struct ctlr_info *h)
 	 * retried.
 	 */
 	c->retry_pending = false;
-
+	c->cmd_type = CMD_IOCTL_PEND;
+	dev_dbg(&h->pdev->dev, "using reserved cmd %u\n", idx);
 	return c;
 }
 
-/*
- * This is the complementary operation to cmd_alloc().  Note, however, in some
- * corner cases it may also be used to free blocks allocated by
- * cmd_tagged_alloc() in which case the ref-count decrement does the trick and
- * the clear-bit is harmless.
- */
 static void cmd_free(struct ctlr_info *h, struct CommandList *c)
 {
-	if (atomic_dec_and_test(&c->refcount)) {
-		int i;
+	struct scsi_cmnd *scmd = c->scsi_cmd;
 
-		i = c - h->cmd_pool;
-		clear_bit(i & (BITS_PER_LONG - 1),
-			  h->cmd_pool_bits + (i / BITS_PER_LONG));
+	if (!scmd) {
+		dev_warn(&h->pdev->dev, "freeing idle cmd\n");
+		return;
+	}
+	cmd_tagged_free(h, c);
+	if (c->cmd_type == CMD_IOCTL_PEND) {
+		dev_dbg(&h->pdev->dev, "returning reserved cmd %u\n",
+			scsi_cmd_to_rq(scmd)->tag);
+		scsi_put_internal_cmd(scmd);
 	}
 }
 
@@ -6450,11 +6405,8 @@ static int hpsa_passthru_ioctl(struct ctlr_info *h,
 			memset(buff, 0, iocommand->buf_size);
 		}
 	}
-	c = cmd_alloc(h);
+	c = cmd_alloc(h, iocommand->Request.Type.Direction);
 
-	/* Fill in the command type */
-	c->cmd_type = CMD_IOCTL_PEND;
-	c->scsi_cmd = SCSI_CMD_BUSY;
 	/* Fill in Command Header */
 	c->Header.ReplyQueue = 0; /* unused in simple mode */
 	if (iocommand->buf_size > 0) {	/* buffer to fill */
@@ -6567,10 +6519,8 @@ static int hpsa_big_passthru_ioctl(struct ctlr_info *h,
 		data_ptr += sz;
 		sg_used++;
 	}
-	c = cmd_alloc(h);
+	c = cmd_alloc(h, ioc->Request.Type.Direction);
 
-	c->cmd_type = CMD_IOCTL_PEND;
-	c->scsi_cmd = SCSI_CMD_BUSY;
 	c->Header.ReplyQueue = 0;
 	c->Header.SGList = (u8) sg_used;
 	c->Header.SGTotal = cpu_to_le16(sg_used);
@@ -6701,7 +6651,7 @@ static void hpsa_send_host_reset(struct ctlr_info *h, u8 reset_type)
 {
 	struct CommandList *c;
 
-	c = cmd_alloc(h);
+	c = cmd_alloc(h, XFER_NONE);
 
 	/* fill_cmd can't fail here, no data buffer to map */
 	(void) fill_cmd(c, HPSA_DEVICE_RESET_MSG, h, NULL, 0, 0,
@@ -6722,8 +6672,6 @@ static int fill_cmd(struct CommandList *c, u8 cmd, struct ctlr_info *h,
 {
 	enum dma_data_direction dir = DMA_NONE;
 
-	c->cmd_type = CMD_IOCTL_PEND;
-	c->scsi_cmd = SCSI_CMD_BUSY;
 	c->Header.ReplyQueue = 0;
 	if (buff != NULL && size > 0) {
 		c->Header.SGList = 1;
@@ -8035,8 +7983,6 @@ static int hpsa_init_reset_devices(struct pci_dev *pdev, u32 board_id)
 
 static void hpsa_free_cmd_pool(struct ctlr_info *h)
 {
-	kfree(h->cmd_pool_bits);
-	h->cmd_pool_bits = NULL;
 	if (h->cmd_pool) {
 		dma_free_coherent(&h->pdev->dev,
 				h->nr_cmds * sizeof(struct CommandList),
@@ -8057,17 +8003,13 @@ static void hpsa_free_cmd_pool(struct ctlr_info *h)
 
 static int hpsa_alloc_cmd_pool(struct ctlr_info *h)
 {
-	h->cmd_pool_bits = kcalloc(DIV_ROUND_UP(h->nr_cmds, BITS_PER_LONG),
-				   sizeof(unsigned long),
-				   GFP_KERNEL);
 	h->cmd_pool = dma_alloc_coherent(&h->pdev->dev,
 		    h->nr_cmds * sizeof(*h->cmd_pool),
 		    &h->cmd_pool_dhandle, GFP_KERNEL);
 	h->errinfo_pool = dma_alloc_coherent(&h->pdev->dev,
 		    h->nr_cmds * sizeof(*h->errinfo_pool),
 		    &h->errinfo_pool_dhandle, GFP_KERNEL);
-	if ((h->cmd_pool_bits == NULL)
-	    || (h->cmd_pool == NULL)
+	if ((h->cmd_pool == NULL)
 	    || (h->errinfo_pool == NULL)) {
 		dev_err(&h->pdev->dev, "out of memory in %s", __func__);
 		goto clean_up;
@@ -8948,7 +8890,7 @@ static void hpsa_flush_cache(struct ctlr_info *h)
 	if (!flush_buf)
 		return;
 
-	c = cmd_alloc(h);
+	c = cmd_alloc(h, XFER_NONE);
 
 	if (fill_cmd(c, HPSA_CACHE_FLUSH, h, flush_buf, 4, 0,
 		RAID_CTLR_LUNID, TYPE_CMD)) {
@@ -8983,7 +8925,7 @@ static void hpsa_disable_rld_caching(struct ctlr_info *h)
 	if (!options)
 		return;
 
-	c = cmd_alloc(h);
+	c = cmd_alloc(h, XFER_READ);
 
 	/* first, get the current diag options settings */
 	if (fill_cmd(c, BMIC_SENSE_DIAG_OPTIONS, h, options, 4, 0,
@@ -9033,11 +8975,10 @@ static void __hpsa_shutdown(struct pci_dev *pdev)
 	struct ctlr_info *h;
 
 	h = pci_get_drvdata(pdev);
-	/* Turn board interrupts off  and send the flush cache command
-	 * sendcmd will turn off interrupt, and send the flush...
-	 * To write all data in the battery backed cache to disks
+	/*
+	 * Turn board interrupts off;
+	 * flush cache command has already been sent.
 	 */
-	hpsa_flush_cache(h);
 	h->access.set_intr_mask(h, HPSA_INTR_OFF);
 	hpsa_free_irqs(h);			/* init_one 4 */
 	hpsa_disable_interrupt_mode(h);		/* pci_init 2 */
@@ -9045,6 +8986,7 @@ static void __hpsa_shutdown(struct pci_dev *pdev)
 
 static void hpsa_shutdown(struct pci_dev *pdev)
 {
+	hpsa_flush_cache(pci_get_drvdata(pdev));
 	__hpsa_shutdown(pdev);
 	pci_disable_device(pdev);
 }
@@ -9089,6 +9031,7 @@ static void hpsa_remove_one(struct pci_dev *pdev)
 	 * when multipath is enabled. There can be SYNCHRONIZE CACHE
 	 * operations which cannot complete and will hang the system.
 	 */
+	hpsa_flush_cache(h);
 	if (h->scsi_host)
 		scsi_remove_host(h->scsi_host);		/* init_one 8 */
 	/* includes hpsa_free_irqs - init_one 4 */
diff --git a/drivers/scsi/hpsa.h b/drivers/scsi/hpsa.h
index 99b0750850b2..bdd9598cd914 100644
--- a/drivers/scsi/hpsa.h
+++ b/drivers/scsi/hpsa.h
@@ -206,7 +206,6 @@ struct ctlr_info {
 	dma_addr_t		ioaccel2_cmd_pool_dhandle;
 	struct ErrorInfo 	*errinfo_pool;
 	dma_addr_t		errinfo_pool_dhandle;
-	unsigned long  		*cmd_pool_bits;
 	int			scan_finished;
 	u8			scan_waiting : 1;
 	spinlock_t		scan_lock;
-- 
2.29.2

[PATCH 06/15] hpsa: use scsi_host_busy_iter() to traverse outstanding commands

[Hannes Reinecke]

Replace all hand-crafted command iterations with
scsi_host_busy_iter() calls.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/hpsa.c | 117 +++++++++++++++++++++++++-------------------
 1 file changed, 67 insertions(+), 50 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index c5f55b56fd2f..23babe177f5d 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -1824,30 +1824,26 @@ static int hpsa_add_device(struct ctlr_info *h, struct hpsa_scsi_dev_t *device)
 	return rc;
 }
 
-static int hpsa_find_outstanding_commands_for_dev(struct ctlr_info *h,
-						struct hpsa_scsi_dev_t *dev)
-{
-	int i;
-	int count = 0;
-
-	for (i = 0; i < h->nr_cmds; i++) {
-		struct CommandList *c = h->cmd_pool + i;
-		int refcount = atomic_inc_return(&c->refcount);
-
-		if (refcount > 1 && hpsa_cmd_dev_match(h, c, dev,
-				dev->scsi3addr)) {
-			unsigned long flags;
+struct hpsa_command_iter_data {
+	struct ctlr_info *h;
+	struct hpsa_scsi_dev_t *dev;
+	unsigned char *scsi3addr;
+	int count;
+};
 
-			spin_lock_irqsave(&h->lock, flags);	/* Implied MB */
-			if (!hpsa_is_cmd_idle(c))
-				++count;
-			spin_unlock_irqrestore(&h->lock, flags);
-		}
+static bool hpsa_find_outstanding_commands_iter(struct scsi_cmnd *sc,
+						void *data, bool reserved)
+{
+	struct hpsa_command_iter_data *iter_data = data;
+	struct ctlr_info *h = iter_data->h;
+	struct hpsa_scsi_dev_t *dev = iter_data->dev;
+	struct CommandList *c = h->cmd_pool + scsi_cmd_to_rq(sc)->tag;
 
-		cmd_free(h, c);
+	if (hpsa_cmd_dev_match(h, c, dev, dev->scsi3addr)) {
+		iter_data->count++;
+		return false;
 	}
-
-	return count;
+	return true;
 }
 
 #define NUM_WAIT 20
@@ -1857,13 +1853,20 @@ static void hpsa_wait_for_outstanding_commands_for_dev(struct ctlr_info *h,
 	int cmds = 0;
 	int waits = 0;
 	int num_wait = NUM_WAIT;
+	struct hpsa_command_iter_data iter_data = {
+		.h = h,
+		.dev = device,
+	};
 
 	if (device->external)
 		num_wait = HPSA_EH_PTRAID_TIMEOUT;
 
 	while (1) {
-		cmds = hpsa_find_outstanding_commands_for_dev(h, device);
-		if (cmds == 0)
+		iter_data.count = 0;
+		scsi_host_busy_iter(h->scsi_host,
+				    hpsa_find_outstanding_commands_iter,
+				    &iter_data);
+		if (iter_data.count == 0)
 			break;
 		if (++waits > num_wait)
 			break;
@@ -8180,27 +8183,34 @@ static void hpsa_undo_allocations_after_kdump_soft_reset(struct ctlr_info *h)
 	kfree(h);				/* init_one 1 */
 }
 
+static bool fail_all_outstanding_cmds_iter(struct scsi_cmnd *sc, void *data,
+					   bool reserved)
+{
+	struct hpsa_command_iter_data *iter_data = data;
+	struct ctlr_info *h = iter_data->h;
+	struct CommandList *c = h->cmd_pool + scsi_cmd_to_rq(sc)->tag;
+
+	c->err_info->CommandStatus = CMD_CTLR_LOCKUP;
+	finish_cmd(c);
+	atomic_dec(&h->commands_outstanding);
+	iter_data->count++;
+
+	return true;
+}
+
 /* Called when controller lockup detected. */
 static void fail_all_outstanding_cmds(struct ctlr_info *h)
 {
-	int i, refcount;
-	struct CommandList *c;
-	int failcount = 0;
+	struct hpsa_command_iter_data iter_data = {
+		.h = h,
+		.count = 0,
+	};
 
 	flush_workqueue(h->resubmit_wq); /* ensure all cmds are fully built */
-	for (i = 0; i < h->nr_cmds; i++) {
-		c = h->cmd_pool + i;
-		refcount = atomic_inc_return(&c->refcount);
-		if (refcount > 1) {
-			c->err_info->CommandStatus = CMD_CTLR_LOCKUP;
-			finish_cmd(c);
-			atomic_dec(&h->commands_outstanding);
-			failcount++;
-		}
-		cmd_free(h, c);
-	}
+	scsi_host_busy_iter(h->scsi_host,
+			    fail_all_outstanding_cmds_iter, &iter_data);
 	dev_warn(&h->pdev->dev,
-		"failed %d commands in fail_all\n", failcount);
+		"failed %d commands in fail_all\n", iter_data.count);
 }
 
 static void set_lockup_detected_for_all_cpus(struct ctlr_info *h, u32 value)
@@ -9499,22 +9509,29 @@ static int is_accelerated_cmd(struct CommandList *c)
 	return c->cmd_type == CMD_IOACCEL1 || c->cmd_type == CMD_IOACCEL2;
 }
 
+static bool hpsa_drain_accel_commands_iter(struct scsi_cmnd *sc, void *data,
+					   bool reserved)
+{
+	struct hpsa_command_iter_data *iter_data = data;
+	struct ctlr_info *h = iter_data->h;
+	struct CommandList *c = h->cmd_pool + scsi_cmd_to_rq(sc)->tag;
+
+	iter_data->count += is_accelerated_cmd(c);
+	return true;
+}
+
 static void hpsa_drain_accel_commands(struct ctlr_info *h)
 {
-	struct CommandList *c = NULL;
-	int i, accel_cmds_out;
-	int refcount;
+	struct hpsa_command_iter_data iter_data = {
+		.h = h,
+	};
 
 	do { /* wait for all outstanding ioaccel commands to drain out */
-		accel_cmds_out = 0;
-		for (i = 0; i < h->nr_cmds; i++) {
-			c = h->cmd_pool + i;
-			refcount = atomic_inc_return(&c->refcount);
-			if (refcount > 1) /* Command is allocated */
-				accel_cmds_out += is_accelerated_cmd(c);
-			cmd_free(h, c);
-		}
-		if (accel_cmds_out <= 0)
+		iter_data.count = 0;
+		scsi_host_busy_iter(h->scsi_host,
+				    hpsa_drain_accel_commands_iter,
+				    &iter_data);
+		if (iter_data.count <= 0)
 			break;
 		msleep(100);
 	} while (1);
-- 
2.29.2

[PATCH 07/15] hpsa: drop refcount field from CommandList

[Hannes Reinecke]

Field is now unused, so drop it.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/hpsa.c     | 11 ++---------
 drivers/scsi/hpsa_cmd.h | 10 ----------
 2 files changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/scsi/hpsa.c b/drivers/scsi/hpsa.c
index 23babe177f5d..08cf9f570c1e 100644
--- a/drivers/scsi/hpsa.c
+++ b/drivers/scsi/hpsa.c
@@ -5537,8 +5537,8 @@ static void hpsa_cmd_init(struct ctlr_info *h, int index,
 {
 	dma_addr_t cmd_dma_handle, err_dma_handle;
 
-	/* Zero out all of commandlist except the last field, refcount */
-	memset(c, 0, offsetof(struct CommandList, refcount));
+	/* Zero out all of commandlist */
+	memset(c, 0, sizeof(struct CommandList));
 	c->Header.tag = cpu_to_le64((u64) (index << DIRECT_LOOKUP_SHIFT));
 	cmd_dma_handle = h->cmd_pool_dhandle + index * sizeof(*c);
 	c->err_info = h->errinfo_pool + index;
@@ -5560,7 +5560,6 @@ static void hpsa_preinitialize_commands(struct ctlr_info *h)
 		struct CommandList *c = h->cmd_pool + i;
 
 		hpsa_cmd_init(h, i, c);
-		atomic_set(&c->refcount, 0);
 	}
 }
 
@@ -6171,7 +6170,6 @@ static struct CommandList *cmd_tagged_alloc(struct ctlr_info *h,
 		return NULL;
 	}
 
-	atomic_inc(&c->refcount);
 	hpsa_cmd_partial_init(h, idx, c);
 
 	/*
@@ -6185,11 +6183,6 @@ static struct CommandList *cmd_tagged_alloc(struct ctlr_info *h,
 
 static void cmd_tagged_free(struct ctlr_info *h, struct CommandList *c)
 {
-	/*
-	 * Release our reference to the block.  We don't need to do anything
-	 * else to free it, because it is accessed by index.
-	 */
-	(void)atomic_dec(&c->refcount);
 	c->scsi_cmd = NULL;
 }
 
diff --git a/drivers/scsi/hpsa_cmd.h b/drivers/scsi/hpsa_cmd.h
index ba6a3aa8d954..04c92c94cc6c 100644
--- a/drivers/scsi/hpsa_cmd.h
+++ b/drivers/scsi/hpsa_cmd.h
@@ -454,18 +454,8 @@ struct CommandList {
 
 	bool retry_pending;
 	struct hpsa_scsi_dev_t *device;
-	atomic_t refcount; /* Must be last to avoid memset in hpsa_cmd_init() */
 } __aligned(COMMANDLIST_ALIGNMENT);
 
-/*
- * Make sure our embedded atomic variable is aligned. Otherwise we break atomic
- * operations on architectures that don't support unaligned atomics like IA64.
- *
- * The assert guards against reintroductin against unwanted __packed to
- * the struct CommandList.
- */
-static_assert(offsetof(struct CommandList, refcount) % __alignof__(atomic_t) == 0);
-
 /* Max S/G elements in I/O accelerator command */
 #define IOACCEL1_MAXSGENTRIES           24
 #define IOACCEL2_MAXSGENTRIES		28
-- 
2.29.2

[PATCH 08/15] aacraid: return valid status from aac_scsi_cmd()

[Hannes Reinecke]

aac_scsi_cmd() is called directly from .queuecommand(), and should
to return valid SCSI_MLQUEUE_XXX status codes.

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

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 59f6b7b2a70a..5b309a8beab8 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -2775,8 +2775,11 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 	struct aac_dev *dev = (struct aac_dev *)host->hostdata;
 	struct fsa_dev_info *fsa_dev_ptr = dev->fsa_dev;
 
-	if (fsa_dev_ptr == NULL)
-		return -1;
+	if (fsa_dev_ptr == NULL) {
+		scsicmd->result = DID_NO_CONNECT << 16;
+		goto scsi_done_ret;
+	}
+
 	/*
 	 *	If the bus, id or lun is out of range, return fail
 	 *	Test does not apply to ID 16, the pseudo id for the controller
@@ -2809,7 +2812,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 				case READ_CAPACITY:
 				case TEST_UNIT_READY:
 					if (dev->in_reset)
-						return -1;
+						return SCSI_MLQUEUE_DEVICE_BUSY;
 					return _aac_probe_container(scsicmd,
 							aac_probe_container_callback2);
 				default:
@@ -2823,12 +2826,12 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 				dev->hba_map[bus][cid].devtype
 					== AAC_DEVTYPE_NATIVE_RAW) {
 				if (dev->in_reset)
-					return -1;
+					return SCSI_MLQUEUE_DEVICE_BUSY;
 				return aac_send_hba_fib(scsicmd);
 			} else if (dev->nondasd_support || expose_physicals ||
 				dev->jbod) {
 				if (dev->in_reset)
-					return -1;
+					return SCSI_MLQUEUE_DEVICE_BUSY;
 				return aac_send_srb_fib(scsicmd);
 			} else {
 				scsicmd->result = DID_NO_CONNECT << 16;
@@ -2859,7 +2862,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 	case READ_12:
 	case READ_16:
 		if (dev->in_reset)
-			return -1;
+			return SCSI_MLQUEUE_DEVICE_BUSY;
 		return aac_read(scsicmd);
 
 	case WRITE_6:
@@ -2867,7 +2870,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 	case WRITE_12:
 	case WRITE_16:
 		if (dev->in_reset)
-			return -1;
+			return SCSI_MLQUEUE_DEVICE_BUSY;
 		return aac_write(scsicmd);
 
 	case SYNCHRONIZE_CACHE:
@@ -2954,7 +2957,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 			break;
 		}
 		if (dev->in_reset)
-			return -1;
+			return SCSI_MLQUEUE_DEVICE_BUSY;
 		setinqstr(dev, (void *) (inq_data.inqd_vid), fsa_dev_ptr[cid].type);
 		inq_data.inqd_pdt = INQD_PDT_DA;	/* Direct/random access device */
 		scsi_sg_copy_from_buffer(scsicmd, &inq_data, sizeof(inq_data));
-- 
2.29.2

[PATCH 09/15] aacraid: don't bother with setting SCp.Status

[Hannes Reinecke]

SCp.Status is only ever evaluated to set the return code of
aac_probe_container(), and all callers only check for negative
values here. The callbacks set it to the value of aac_mount.count,
which is an unsigned int; as such it should never become negative
here, and so we only need to return an error from aac_probe_container()
if we can't send the initial fib. But once that's done we can return
'0' and don't need to set SCp.Status at all.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/aacraid/aachba.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 5b309a8beab8..289c4968a92e 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -641,7 +641,6 @@ static void _aac_probe_container2(void * context, struct fib * fibptr)
 	if (!aac_valid_context(scsicmd, fibptr))
 		return;
 
-	scsicmd->SCp.Status = 0;
 	fsa_dev_ptr = fibptr->dev->fsa_dev;
 	if (fsa_dev_ptr) {
 		struct aac_mount * dresp = (struct aac_mount *) fib_data(fibptr);
@@ -679,7 +678,6 @@ static void _aac_probe_container2(void * context, struct fib * fibptr)
 		}
 		if ((fsa_dev_ptr->valid & 1) == 0)
 			fsa_dev_ptr->valid = 0;
-		scsicmd->SCp.Status = le32_to_cpu(dresp->count);
 	}
 	aac_fib_complete(fibptr);
 	aac_fib_free(fibptr);
@@ -831,11 +829,11 @@ int aac_probe_container(struct aac_dev *dev, int cid)
 	scsidev->id = cid;
 	scsidev->host = dev->scsi_host_ptr;
 
-	if (_aac_probe_container(scsicmd, aac_probe_container_callback1) == 0)
+	status = _aac_probe_container(scsicmd, aac_probe_container_callback1);
+	if (status == 0)
 		while (scsicmd->device == scsidev)
 			schedule();
 	kfree(scsidev);
-	status = scsicmd->SCp.Status;
 	kfree(scsicmd);
 	return status;
 }
-- 
2.29.2

[PATCH 10/15] aacraid: move scsi_add_host()

[Hannes Reinecke]

Move the call to scsi_add_host() so that the Scsi_Host structure
is initialized before any I/O is sent.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/aacraid/linit.c | 31 +++++++++++++++----------------
 1 file changed, 15 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index a911252075a6..b92a6595358e 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -1643,6 +1643,9 @@ 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->max_id = MAXIMUM_NUM_CONTAINERS;
+	shost->max_lun = AAC_MAX_LUN;
+	shost->sg_tablesize = HBA_MAX_SG_SEPARATE;
 
 	if (aac_cfg_major == AAC_CHARDEV_NEEDS_REINIT)
 		aac_init_char();
@@ -1681,7 +1684,7 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	aac->base_size = AAC_MIN_FOOTPRINT_SIZE;
 	if ((*aac_drivers[index].init)(aac)) {
 		error = -ENODEV;
-		goto out_unmap;
+		goto out_free_fibs;
 	}
 
 	if (aac->sync_mode) {
@@ -1707,9 +1710,15 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 		printk(KERN_ERR "aacraid: Unable to create command thread.\n");
 		error = PTR_ERR(aac->thread);
 		aac->thread = NULL;
-		goto out_deinit;
+		goto out_unmap;
 	}
 
+	pci_set_drvdata(pdev, shost);
+
+	error = scsi_add_host(shost, &pdev->dev);
+	if (error)
+		goto out_deinit;
+
 	aac->maximum_num_channels = aac_drivers[index].channels;
 	error = aac_get_adapter_info(aac);
 	if (error < 0)
@@ -1768,18 +1777,6 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	if (!aac->sa_firmware && aac_drivers[index].quirks & AAC_QUIRK_SRC)
 		aac_intr_normal(aac, 0, 2, 0, NULL);
 
-	/*
-	 * dmb - we may need to move the setting of these parms somewhere else once
-	 * we get a fib that can report the actual numbers
-	 */
-	shost->max_lun = AAC_MAX_LUN;
-
-	pci_set_drvdata(pdev, shost);
-
-	error = scsi_add_host(shost, &pdev->dev);
-	if (error)
-		goto out_deinit;
-
 	aac_scan_host(aac);
 
 	pci_enable_pcie_error_reporting(pdev);
@@ -1796,10 +1793,12 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 				  aac->comm_addr, aac->comm_phys);
 	kfree(aac->queues);
 	aac_adapter_ioremap(aac, 0);
-	kfree(aac->fibs);
 	kfree(aac->fsa_dev);
+ out_free_fibs:
+	kfree(aac->fibs);
  out_free_host:
 	scsi_host_put(shost);
+	pci_set_drvdata(pdev, NULL);
  out_disable_pdev:
 	pci_disable_device(pdev);
  out:
@@ -1907,9 +1906,9 @@ static void aac_remove_one(struct pci_dev *pdev)
 	struct aac_dev *aac = (struct aac_dev *)shost->hostdata;
 
 	aac_cancel_rescan_worker(aac);
-	scsi_remove_host(shost);
 
 	__aac_shutdown(aac);
+	scsi_remove_host(shost);
 	aac_fib_map_free(aac);
 	dma_free_coherent(&aac->pdev->dev, aac->comm_size, aac->comm_addr,
 			  aac->comm_phys);
-- 
2.29.2

[PATCH 11/15] aacraid: move container ID into struct fib

[Hannes Reinecke]

Add a new field 'cid' to struct fib to hold the container ID this
I/O is destined for.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/aacraid/aachba.c  | 19 +++++++++++--------
 drivers/scsi/aacraid/aacraid.h |  6 +++++-
 2 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 289c4968a92e..2cc9f79c75ff 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -646,7 +646,7 @@ static void _aac_probe_container2(void * context, struct fib * fibptr)
 		struct aac_mount * dresp = (struct aac_mount *) fib_data(fibptr);
 		__le32 sup_options2;
 
-		fsa_dev_ptr += scmd_id(scsicmd);
+		fsa_dev_ptr += fibptr->cid;
 		sup_options2 =
 			fibptr->dev->supplement_adapter_info.supported_options2;
 
@@ -718,7 +718,7 @@ static void _aac_probe_container1(void * context, struct fib * fibptr)
 	else
 		dinfo->command = cpu_to_le32(VM_NameServe64);
 
-	dinfo->count = cpu_to_le32(scmd_id(scsicmd));
+	dinfo->count = cpu_to_le32(fibptr->cid);
 	dinfo->type = cpu_to_le32(FT_FILESYS);
 	scsicmd->SCp.phase = AAC_OWNER_FIRMWARE;
 
@@ -739,7 +739,8 @@ static void _aac_probe_container1(void * context, struct fib * fibptr)
 	}
 }
 
-static int _aac_probe_container(struct scsi_cmnd * scsicmd, int (*callback)(struct scsi_cmnd *))
+static int _aac_probe_container(struct scsi_cmnd * scsicmd, unsigned int cid,
+				int (*callback)(struct scsi_cmnd *))
 {
 	struct fib * fibptr;
 	int status = -ENOMEM;
@@ -748,6 +749,7 @@ static int _aac_probe_container(struct scsi_cmnd * scsicmd, int (*callback)(stru
 		struct aac_query_mount *dinfo;
 
 		aac_fib_init(fibptr);
+		fibptr->cid = cid;
 
 		dinfo = (struct aac_query_mount *)fib_data(fibptr);
 
@@ -757,7 +759,7 @@ static int _aac_probe_container(struct scsi_cmnd * scsicmd, int (*callback)(stru
 		else
 			dinfo->command = cpu_to_le32(VM_NameServe);
 
-		dinfo->count = cpu_to_le32(scmd_id(scsicmd));
+		dinfo->count = cpu_to_le32(fibptr->cid);
 		dinfo->type = cpu_to_le32(FT_FILESYS);
 		scsicmd->SCp.ptr = (char *)callback;
 		scsicmd->SCp.phase = AAC_OWNER_FIRMWARE;
@@ -784,7 +786,7 @@ static int _aac_probe_container(struct scsi_cmnd * scsicmd, int (*callback)(stru
 	if (status < 0) {
 		struct fsa_dev_info *fsa_dev_ptr = ((struct aac_dev *)(scsicmd->device->host->hostdata))->fsa_dev;
 		if (fsa_dev_ptr) {
-			fsa_dev_ptr += scmd_id(scsicmd);
+			fsa_dev_ptr += cid;
 			if ((fsa_dev_ptr->valid & 1) == 0) {
 				fsa_dev_ptr->valid = 0;
 				return (*callback)(scsicmd);
@@ -812,7 +814,7 @@ static void aac_probe_container_scsi_done(struct scsi_cmnd *scsi_cmnd)
 	aac_probe_container_callback1(scsi_cmnd);
 }
 
-int aac_probe_container(struct aac_dev *dev, int cid)
+int aac_probe_container(struct aac_dev *dev, unsigned int cid)
 {
 	struct scsi_cmnd *scsicmd = kzalloc(sizeof(*scsicmd), GFP_KERNEL);
 	struct scsi_device *scsidev = kzalloc(sizeof(*scsidev), GFP_KERNEL);
@@ -829,7 +831,8 @@ int aac_probe_container(struct aac_dev *dev, int cid)
 	scsidev->id = cid;
 	scsidev->host = dev->scsi_host_ptr;
 
-	status = _aac_probe_container(scsicmd, aac_probe_container_callback1);
+	status = _aac_probe_container(scsicmd, cid,
+				      aac_probe_container_callback1);
 	if (status == 0)
 		while (scsicmd->device == scsidev)
 			schedule();
@@ -2811,7 +2814,7 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 				case TEST_UNIT_READY:
 					if (dev->in_reset)
 						return SCSI_MLQUEUE_DEVICE_BUSY;
-					return _aac_probe_container(scsicmd,
+					return _aac_probe_container(scsicmd, cid,
 							aac_probe_container_callback2);
 				default:
 					break;
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 3733df77bc65..90705c4f8ec8 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -1298,6 +1298,10 @@ struct fib {
 	 *	The Adapter that this I/O is destined for.
 	 */
 	struct aac_dev		*dev;
+	/*
+	 *	The Container that this I/O is destined for.
+	 */
+	u32			cid;
 	/*
 	 *	This is the event the sendfib routine will wait on if the
 	 *	caller did not pass one and this is synch io.
@@ -2734,7 +2738,7 @@ int aac_fib_adapter_complete(struct fib * fibptr, unsigned short size);
 struct aac_driver_ident* aac_get_driver_ident(int devtype);
 int aac_get_adapter_info(struct aac_dev* dev);
 int aac_send_shutdown(struct aac_dev *dev);
-int aac_probe_container(struct aac_dev *dev, int cid);
+int aac_probe_container(struct aac_dev *dev, unsigned int cid);
 int _aac_rx_init(struct aac_dev *dev);
 int aac_rx_select_comm(struct aac_dev *dev, int comm);
 int aac_rx_deliver_producer(struct fib * fib);
-- 
2.29.2

[PATCH 12/15] aacraid: fsa_dev pointer is always valid

[Hannes Reinecke]

All call sites to aac_probe_container() already check for a valid
fsa_dev pointer, so we don't need to do that during probing.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/aacraid/aachba.c | 79 ++++++++++++++++-------------------
 1 file changed, 36 insertions(+), 43 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 2cc9f79c75ff..580d74b2ee14 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -636,49 +636,44 @@ static void _aac_probe_container2(void * context, struct fib * fibptr)
 	int (*callback)(struct scsi_cmnd *);
 	struct scsi_cmnd * scsicmd = (struct scsi_cmnd *)context;
 	int i;
+	struct aac_mount * dresp = (struct aac_mount *) fib_data(fibptr);
+	__le32 sup_options2;
 
 
 	if (!aac_valid_context(scsicmd, fibptr))
 		return;
 
-	fsa_dev_ptr = fibptr->dev->fsa_dev;
-	if (fsa_dev_ptr) {
-		struct aac_mount * dresp = (struct aac_mount *) fib_data(fibptr);
-		__le32 sup_options2;
+	fsa_dev_ptr = &fibptr->dev->fsa_dev[fibptr->cid];
+	sup_options2 = fibptr->dev->supplement_adapter_info.supported_options2;
 
-		fsa_dev_ptr += fibptr->cid;
-		sup_options2 =
-			fibptr->dev->supplement_adapter_info.supported_options2;
-
-		if ((le32_to_cpu(dresp->status) == ST_OK) &&
-		    (le32_to_cpu(dresp->mnt[0].vol) != CT_NONE) &&
-		    (le32_to_cpu(dresp->mnt[0].state) != FSCS_HIDDEN)) {
-			if (!(sup_options2 & AAC_OPTION_VARIABLE_BLOCK_SIZE)) {
-				dresp->mnt[0].fileinfo.bdevinfo.block_size = 0x200;
-				fsa_dev_ptr->block_size = 0x200;
-			} else {
-				fsa_dev_ptr->block_size =
-					le32_to_cpu(dresp->mnt[0].fileinfo.bdevinfo.block_size);
-			}
-			for (i = 0; i < 16; i++)
-				fsa_dev_ptr->identifier[i] =
-					dresp->mnt[0].fileinfo.bdevinfo
-								.identifier[i];
-			fsa_dev_ptr->valid = 1;
-			/* sense_key holds the current state of the spin-up */
-			if (dresp->mnt[0].state & cpu_to_le32(FSCS_NOT_READY))
-				fsa_dev_ptr->sense_data.sense_key = NOT_READY;
-			else if (fsa_dev_ptr->sense_data.sense_key == NOT_READY)
-				fsa_dev_ptr->sense_data.sense_key = NO_SENSE;
-			fsa_dev_ptr->type = le32_to_cpu(dresp->mnt[0].vol);
-			fsa_dev_ptr->size
-			  = ((u64)le32_to_cpu(dresp->mnt[0].capacity)) +
-			    (((u64)le32_to_cpu(dresp->mnt[0].capacityhigh)) << 32);
-			fsa_dev_ptr->ro = ((le32_to_cpu(dresp->mnt[0].state) & FSCS_READONLY) != 0);
+	if ((le32_to_cpu(dresp->status) == ST_OK) &&
+	    (le32_to_cpu(dresp->mnt[0].vol) != CT_NONE) &&
+	    (le32_to_cpu(dresp->mnt[0].state) != FSCS_HIDDEN)) {
+		if (!(sup_options2 & AAC_OPTION_VARIABLE_BLOCK_SIZE)) {
+			dresp->mnt[0].fileinfo.bdevinfo.block_size = 0x200;
+			fsa_dev_ptr->block_size = 0x200;
+		} else {
+			fsa_dev_ptr->block_size =
+				le32_to_cpu(dresp->mnt[0].fileinfo.bdevinfo.block_size);
 		}
-		if ((fsa_dev_ptr->valid & 1) == 0)
-			fsa_dev_ptr->valid = 0;
+		for (i = 0; i < 16; i++)
+			fsa_dev_ptr->identifier[i] =
+				dresp->mnt[0].fileinfo.bdevinfo.identifier[i];
+		fsa_dev_ptr->valid = 1;
+		/* sense_key holds the current state of the spin-up */
+		if (dresp->mnt[0].state & cpu_to_le32(FSCS_NOT_READY))
+			fsa_dev_ptr->sense_data.sense_key = NOT_READY;
+		else if (fsa_dev_ptr->sense_data.sense_key == NOT_READY)
+			fsa_dev_ptr->sense_data.sense_key = NO_SENSE;
+		fsa_dev_ptr->type = le32_to_cpu(dresp->mnt[0].vol);
+		fsa_dev_ptr->size
+			= ((u64)le32_to_cpu(dresp->mnt[0].capacity)) +
+			(((u64)le32_to_cpu(dresp->mnt[0].capacityhigh)) << 32);
+		fsa_dev_ptr->ro = ((le32_to_cpu(dresp->mnt[0].state) & FSCS_READONLY) != 0);
 	}
+	if ((fsa_dev_ptr->valid & 1) == 0)
+		fsa_dev_ptr->valid = 0;
+
 	aac_fib_complete(fibptr);
 	aac_fib_free(fibptr);
 	callback = (int (*)(struct scsi_cmnd *))(scsicmd->SCp.ptr);
@@ -742,10 +737,12 @@ static void _aac_probe_container1(void * context, struct fib * fibptr)
 static int _aac_probe_container(struct scsi_cmnd * scsicmd, unsigned int cid,
 				int (*callback)(struct scsi_cmnd *))
 {
+	struct aac_dev * dev =
+		(struct aac_dev *)scsicmd->device->host->hostdata;
 	struct fib * fibptr;
 	int status = -ENOMEM;
 
-	if ((fibptr = aac_fib_alloc((struct aac_dev *)scsicmd->device->host->hostdata))) {
+	if ((fibptr = aac_fib_alloc(dev))) {
 		struct aac_query_mount *dinfo;
 
 		aac_fib_init(fibptr);
@@ -784,13 +781,9 @@ static int _aac_probe_container(struct scsi_cmnd * scsicmd, unsigned int cid,
 		}
 	}
 	if (status < 0) {
-		struct fsa_dev_info *fsa_dev_ptr = ((struct aac_dev *)(scsicmd->device->host->hostdata))->fsa_dev;
-		if (fsa_dev_ptr) {
-			fsa_dev_ptr += cid;
-			if ((fsa_dev_ptr->valid & 1) == 0) {
-				fsa_dev_ptr->valid = 0;
-				return (*callback)(scsicmd);
-			}
+		if ((dev->fsa_dev[cid].valid & 1) == 0) {
+			dev->fsa_dev[cid].valid = 0;
+			return (*callback)(scsicmd);
 		}
 	}
 	return status;
-- 
2.29.2

[PATCH 13/15] aacraid: store callback in scsi_cmnd.host_scribble

[Hannes Reinecke]

Store the container callback in scsi_cmnd.host_scribble to avoid
having to rely on the fake scsi device during container probing.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/aacraid/aachba.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index 580d74b2ee14..b21ccf7af43f 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -676,8 +676,8 @@ static void _aac_probe_container2(void * context, struct fib * fibptr)
 
 	aac_fib_complete(fibptr);
 	aac_fib_free(fibptr);
-	callback = (int (*)(struct scsi_cmnd *))(scsicmd->SCp.ptr);
-	scsicmd->SCp.ptr = NULL;
+	callback = (int (*)(struct scsi_cmnd *))(scsicmd->host_scribble);
+	scsicmd->host_scribble = NULL;
 	(*callback)(scsicmd);
 	return;
 }
@@ -758,7 +758,7 @@ static int _aac_probe_container(struct scsi_cmnd * scsicmd, unsigned int cid,
 
 		dinfo->count = cpu_to_le32(fibptr->cid);
 		dinfo->type = cpu_to_le32(FT_FILESYS);
-		scsicmd->SCp.ptr = (char *)callback;
+		scsicmd->host_scribble = (char *)callback;
 		scsicmd->SCp.phase = AAC_OWNER_FIRMWARE;
 
 		status = aac_fib_send(ContainerCommand,
@@ -775,9 +775,9 @@ static int _aac_probe_container(struct scsi_cmnd * scsicmd, unsigned int cid,
 			return 0;
 
 		if (status < 0) {
-			scsicmd->SCp.ptr = NULL;
 			aac_fib_complete(fibptr);
 			aac_fib_free(fibptr);
+			scsicmd->host_scribble = NULL;
 		}
 	}
 	if (status < 0) {
@@ -798,7 +798,7 @@ static int _aac_probe_container(struct scsi_cmnd * scsicmd, unsigned int cid,
  */
 static int aac_probe_container_callback1(struct scsi_cmnd * scsicmd)
 {
-	scsicmd->device = NULL;
+	scsicmd->host_scribble = NULL;
 	return 0;
 }
 
@@ -827,7 +827,7 @@ int aac_probe_container(struct aac_dev *dev, unsigned int cid)
 	status = _aac_probe_container(scsicmd, cid,
 				      aac_probe_container_callback1);
 	if (status == 0)
-		while (scsicmd->device == scsidev)
+		while (scsicmd->host_scribble != NULL)
 			schedule();
 	kfree(scsidev);
 	kfree(scsicmd);
-- 
2.29.2

[PATCH 14/15] aacraid: use scsi_get_internal_cmd()

[Hannes Reinecke]

From: Hannes Reinecke <hare@suse.com>

Use scsi_get_internal_cmd() to allocate internal commands and drop
the fake scsi command and device allocation during container probing.

Signed-off-by: Hannes Reinecke <hare@suse.com>
---
 drivers/scsi/aacraid/aachba.c   | 103 +++++++++++++++-----------------
 drivers/scsi/aacraid/aacraid.h  |   9 ++-
 drivers/scsi/aacraid/commctrl.c |  25 ++++----
 drivers/scsi/aacraid/comminit.c |   2 +-
 drivers/scsi/aacraid/commsup.c  |  66 +++++++++-----------
 drivers/scsi/aacraid/dpcsup.c   |   2 +-
 drivers/scsi/aacraid/linit.c    |   8 ++-
 7 files changed, 104 insertions(+), 111 deletions(-)

diff --git a/drivers/scsi/aacraid/aachba.c b/drivers/scsi/aacraid/aachba.c
index b21ccf7af43f..ff4c5153b5d6 100644
--- a/drivers/scsi/aacraid/aachba.c
+++ b/drivers/scsi/aacraid/aachba.c
@@ -360,7 +360,7 @@ int aac_get_config_status(struct aac_dev *dev, int commit_flag)
 	int status = 0;
 	struct fib * fibptr;
 
-	if (!(fibptr = aac_fib_alloc(dev)))
+	if (!(fibptr = aac_fib_alloc(dev, DMA_FROM_DEVICE)))
 		return -ENOMEM;
 
 	aac_fib_init(fibptr);
@@ -457,7 +457,7 @@ int aac_get_containers(struct aac_dev *dev)
 	struct aac_get_container_count_resp *dresp;
 	int maximum_num_containers = MAXIMUM_NUM_CONTAINERS;
 
-	if (!(fibptr = aac_fib_alloc(dev)))
+	if (!(fibptr = aac_fib_alloc(dev, DMA_FROM_DEVICE)))
 		return -ENOMEM;
 
 	aac_fib_init(fibptr);
@@ -520,7 +520,7 @@ int aac_get_containers(struct aac_dev *dev)
 
 static void aac_scsi_done(struct scsi_cmnd *scmd)
 {
-	if (scmd->device->request_queue) {
+	if (scmd->device != scmd->device->host->shost_sdev) {
 		/* SCSI command has been submitted by the SCSI mid-layer. */
 		scsi_done(scmd);
 	} else {
@@ -620,9 +620,10 @@ static int aac_get_container_name(struct scsi_cmnd * scsicmd)
 
 static int aac_probe_container_callback2(struct scsi_cmnd * scsicmd)
 {
-	struct fsa_dev_info *fsa_dev_ptr = ((struct aac_dev *)(scsicmd->device->host->hostdata))->fsa_dev;
+	struct aac_dev * dev = (struct aac_dev *)scsicmd->device->host->hostdata;
+	unsigned int cid = scmd_id(scsicmd);
 
-	if ((fsa_dev_ptr[scmd_id(scsicmd)].valid & 1))
+	if (cid < dev->maximum_num_containers && (dev->fsa_dev[cid].valid & 1))
 		return aac_scsi_cmd(scsicmd);
 
 	scsicmd->result = DID_NO_CONNECT << 16;
@@ -675,7 +676,6 @@ static void _aac_probe_container2(void * context, struct fib * fibptr)
 		fsa_dev_ptr->valid = 0;
 
 	aac_fib_complete(fibptr);
-	aac_fib_free(fibptr);
 	callback = (int (*)(struct scsi_cmnd *))(scsicmd->host_scribble);
 	scsicmd->host_scribble = NULL;
 	(*callback)(scsicmd);
@@ -734,55 +734,46 @@ static void _aac_probe_container1(void * context, struct fib * fibptr)
 	}
 }
 
-static int _aac_probe_container(struct scsi_cmnd * scsicmd, unsigned int cid,
+static int _aac_probe_container(struct aac_dev *dev, struct fib *fibptr,
 				int (*callback)(struct scsi_cmnd *))
 {
-	struct aac_dev * dev =
-		(struct aac_dev *)scsicmd->device->host->hostdata;
-	struct fib * fibptr;
+	struct scsi_cmnd *scsicmd = fibptr->scmd;
 	int status = -ENOMEM;
+	struct aac_query_mount *dinfo;
 
-	if ((fibptr = aac_fib_alloc(dev))) {
-		struct aac_query_mount *dinfo;
-
-		aac_fib_init(fibptr);
-		fibptr->cid = cid;
+	aac_fib_init(fibptr);
 
-		dinfo = (struct aac_query_mount *)fib_data(fibptr);
+	dinfo = (struct aac_query_mount *)fib_data(fibptr);
 
-		if (fibptr->dev->supplement_adapter_info.supported_options2 &
-		    AAC_OPTION_VARIABLE_BLOCK_SIZE)
-			dinfo->command = cpu_to_le32(VM_NameServeAllBlk);
-		else
-			dinfo->command = cpu_to_le32(VM_NameServe);
+	if (fibptr->dev->supplement_adapter_info.supported_options2 &
+	    AAC_OPTION_VARIABLE_BLOCK_SIZE)
+		dinfo->command = cpu_to_le32(VM_NameServeAllBlk);
+	else
+		dinfo->command = cpu_to_le32(VM_NameServe);
 
-		dinfo->count = cpu_to_le32(fibptr->cid);
-		dinfo->type = cpu_to_le32(FT_FILESYS);
-		scsicmd->host_scribble = (char *)callback;
-		scsicmd->SCp.phase = AAC_OWNER_FIRMWARE;
+	dinfo->count = cpu_to_le32(fibptr->cid);
+	dinfo->type = cpu_to_le32(FT_FILESYS);
+	scsicmd->host_scribble = (unsigned char *)callback;
+	scsicmd->SCp.phase = AAC_OWNER_FIRMWARE;
 
-		status = aac_fib_send(ContainerCommand,
+	status = aac_fib_send(ContainerCommand,
 			  fibptr,
 			  sizeof(struct aac_query_mount),
 			  FsaNormal,
 			  0, 1,
 			  _aac_probe_container1,
 			  (void *) scsicmd);
-		/*
-		 *	Check that the command queued to the controller
-		 */
-		if (status == -EINPROGRESS)
-			return 0;
+	/*
+	 *	Check that the command queued to the controller
+	 */
+	if (status == -EINPROGRESS)
+		return 0;
 
-		if (status < 0) {
-			aac_fib_complete(fibptr);
-			aac_fib_free(fibptr);
-			scsicmd->host_scribble = NULL;
-		}
-	}
 	if (status < 0) {
-		if ((dev->fsa_dev[cid].valid & 1) == 0) {
-			dev->fsa_dev[cid].valid = 0;
+		aac_fib_complete(fibptr);
+		if ((dev->fsa_dev[fibptr->cid].valid & 1) == 0) {
+			dev->fsa_dev[fibptr->cid].valid = 0;
+			scsicmd->host_scribble = NULL;
 			return (*callback)(scsicmd);
 		}
 	}
@@ -809,28 +800,24 @@ static void aac_probe_container_scsi_done(struct scsi_cmnd *scsi_cmnd)
 
 int aac_probe_container(struct aac_dev *dev, unsigned int cid)
 {
-	struct scsi_cmnd *scsicmd = kzalloc(sizeof(*scsicmd), GFP_KERNEL);
-	struct scsi_device *scsidev = kzalloc(sizeof(*scsidev), GFP_KERNEL);
+	struct fib *fibptr;
+	struct scsi_cmnd *scsicmd;
 	int status;
 
-	if (!scsicmd || !scsidev) {
-		kfree(scsicmd);
-		kfree(scsidev);
+	fibptr = aac_fib_alloc(dev, DMA_FROM_DEVICE);
+	if (!fibptr)
 		return -ENOMEM;
-	}
 
-	scsicmd->device = scsidev;
-	scsidev->sdev_state = 0;
-	scsidev->id = cid;
-	scsidev->host = dev->scsi_host_ptr;
+	fibptr->cid = cid;
+	scsicmd = fibptr->scmd;
 
-	status = _aac_probe_container(scsicmd, cid,
+	status = _aac_probe_container(dev, fibptr,
 				      aac_probe_container_callback1);
 	if (status == 0)
 		while (scsicmd->host_scribble != NULL)
 			schedule();
-	kfree(scsidev);
-	kfree(scsicmd);
+
+	aac_fib_free(fibptr);
 	return status;
 }
 
@@ -1675,7 +1662,7 @@ static int aac_send_safw_bmic_cmd(struct aac_dev *dev,
 		return 0;
 
 	/* allocate FIB */
-	fibptr = aac_fib_alloc(dev);
+	fibptr = aac_fib_alloc(dev, DMA_BIDIRECTIONAL);
 	if (!fibptr)
 		return -ENOMEM;
 
@@ -2035,7 +2022,7 @@ int aac_get_adapter_info(struct aac_dev* dev)
 	struct aac_bus_info *command;
 	struct aac_bus_info_response *bus_info;
 
-	if (!(fibptr = aac_fib_alloc(dev)))
+	if (!(fibptr = aac_fib_alloc(dev, DMA_FROM_DEVICE)))
 		return -ENOMEM;
 
 	aac_fib_init(fibptr);
@@ -2082,7 +2069,7 @@ int aac_get_adapter_info(struct aac_dev* dev)
 		if (rcode >= 0)
 			memcpy(&dev->supplement_adapter_info, sinfo, sizeof(*sinfo));
 		if (rcode == -ERESTARTSYS) {
-			fibptr = aac_fib_alloc(dev);
+			fibptr = aac_fib_alloc(dev, DMA_FROM_DEVICE);
 			if (!fibptr)
 				return -ENOMEM;
 		}
@@ -2795,6 +2782,8 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 			if (((fsa_dev_ptr[cid].valid & 1) == 0) ||
 			  (fsa_dev_ptr[cid].sense_data.sense_key ==
 			   NOT_READY)) {
+				struct fib * fibptr;
+
 				switch (scsicmd->cmnd[0]) {
 				case SERVICE_ACTION_IN_16:
 					if (!(dev->raw_io_interface) ||
@@ -2807,7 +2796,9 @@ int aac_scsi_cmd(struct scsi_cmnd * scsicmd)
 				case TEST_UNIT_READY:
 					if (dev->in_reset)
 						return SCSI_MLQUEUE_DEVICE_BUSY;
-					return _aac_probe_container(scsicmd, cid,
+					fibptr = aac_fib_alloc_tag(dev, scsicmd);
+					fibptr->cid = cid;
+					return _aac_probe_container(dev, fibptr,
 							aac_probe_container_callback2);
 				default:
 					break;
diff --git a/drivers/scsi/aacraid/aacraid.h b/drivers/scsi/aacraid/aacraid.h
index 90705c4f8ec8..1892f49668aa 100644
--- a/drivers/scsi/aacraid/aacraid.h
+++ b/drivers/scsi/aacraid/aacraid.h
@@ -1291,7 +1291,6 @@ struct fsa_dev_info {
 };
 
 struct fib {
-	void			*next;	/* this is used by the allocator */
 	s16			type;
 	s16			size;
 	/*
@@ -1302,6 +1301,10 @@ struct fib {
 	 *	The Container that this I/O is destined for.
 	 */
 	u32			cid;
+	/*
+	 * The associated scsi command
+	 */
+	struct scsi_cmnd 	*scmd;
 	/*
 	 *	This is the event the sendfib routine will wait on if the
 	 *	caller did not pass one and this is synch io.
@@ -1556,7 +1559,6 @@ struct aac_dev
 	 */
 	struct fib              *fibs;
 
-	struct fib		*free_fib;
 	spinlock_t		fib_lock;
 
 	struct mutex		ioctl_mutex;
@@ -1733,6 +1735,7 @@ struct aac_dev
 #define FIB_CONTEXT_FLAG_NATIVE_HBA_TMF	(0x00000020)
 #define FIB_CONTEXT_FLAG_SCSI_CMD	(0x00000040)
 #define FIB_CONTEXT_FLAG_EH_RESET	(0x00000080)
+#define FIB_CONTEXT_FLAG_INTERNAL_CMD	(0x00000100)
 
 /*
  *	Define the command values
@@ -2690,7 +2693,7 @@ void aac_free_irq(struct aac_dev *dev);
 int aac_setup_safw_adapter(struct aac_dev *dev);
 const char *aac_driverinfo(struct Scsi_Host *);
 void aac_fib_vector_assign(struct aac_dev *dev);
-struct fib *aac_fib_alloc(struct aac_dev *dev);
+struct fib *aac_fib_alloc(struct aac_dev *dev, int direction);
 struct fib *aac_fib_alloc_tag(struct aac_dev *dev, struct scsi_cmnd *scmd);
 int aac_fib_setup(struct aac_dev *dev);
 void aac_fib_map_free(struct aac_dev *dev);
diff --git a/drivers/scsi/aacraid/commctrl.c b/drivers/scsi/aacraid/commctrl.c
index e7cc927ed952..bc5b81696476 100644
--- a/drivers/scsi/aacraid/commctrl.c
+++ b/drivers/scsi/aacraid/commctrl.c
@@ -55,7 +55,7 @@ static int ioctl_send_fib(struct aac_dev * dev, void __user *arg)
 	if (dev->in_reset) {
 		return -EBUSY;
 	}
-	fibptr = aac_fib_alloc(dev);
+	fibptr = aac_fib_alloc(dev, DMA_BIDIRECTIONAL);
 	if(fibptr == NULL) {
 		return -ENOMEM;
 	}
@@ -478,7 +478,7 @@ static int check_revision(struct aac_dev *dev, void __user *arg)
  */
 static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 {
-	struct fib* srbfib;
+	struct fib* srbfib = NULL;
 	int status;
 	struct aac_srb *srbcmd = NULL;
 	struct aac_hba_cmd_req *hbacmd = NULL;
@@ -509,12 +509,6 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 		dprintk((KERN_DEBUG"aacraid: No permission to send raw srb\n"));
 		return -EPERM;
 	}
-	/*
-	 *	Allocate and initialize a Fib then setup a SRB command
-	 */
-	if (!(srbfib = aac_fib_alloc(dev))) {
-		return -ENOMEM;
-	}
 
 	memset(sg_list, 0, sizeof(sg_list)); /* cleanup may take issue */
 	if(copy_from_user(&fibsize, &user_srb->count,sizeof(u32))){
@@ -561,6 +555,15 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 		rcode = -EINVAL;
 		goto cleanup;
 	}
+
+	/*
+	 *	Allocate and initialize a Fib
+	 */
+	if (!(srbfib = aac_fib_alloc(dev, data_dir))) {
+		rcode = -ENOMEM;
+		goto cleanup;
+	}
+
 	actual_fibsize = sizeof(struct aac_srb) - sizeof(struct sgentry) +
 		((user_srbcmd->sg.count & 0xff) * sizeof(struct sgentry));
 	actual_fibsize64 = actual_fibsize + (user_srbcmd->sg.count & 0xff) *
@@ -988,8 +991,10 @@ static int aac_send_raw_srb(struct aac_dev* dev, void __user * arg)
 	if (rcode != -ERESTARTSYS) {
 		for (i = 0; i <= sg_indx; i++)
 			kfree(sg_list[i]);
-		aac_fib_complete(srbfib);
-		aac_fib_free(srbfib);
+		if (srbfib) {
+			aac_fib_complete(srbfib);
+			aac_fib_free(srbfib);
+		}
 	}
 
 	return rcode;
diff --git a/drivers/scsi/aacraid/comminit.c b/drivers/scsi/aacraid/comminit.c
index 355b16f0b145..0d5be76891b0 100644
--- a/drivers/scsi/aacraid/comminit.c
+++ b/drivers/scsi/aacraid/comminit.c
@@ -327,7 +327,7 @@ int aac_send_shutdown(struct aac_dev * dev)
 
 	aac_wait_for_io_completion(dev);
 
-	fibctx = aac_fib_alloc(dev);
+	fibctx = aac_fib_alloc(dev, DMA_NONE);
 	if (!fibctx)
 		return -ENOMEM;
 	aac_fib_init(fibctx);
diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index deb32c9f4b3e..aaa9b5d6f55d 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -173,7 +173,6 @@ int aac_fib_setup(struct aac_dev * dev)
 		fibptr->dev = dev;
 		fibptr->hw_fib_va = hw_fib;
 		fibptr->data = (void *) fibptr->hw_fib_va->data;
-		fibptr->next = fibptr+1;	/* Forward chain the fibs */
 		init_completion(&fibptr->event_wait);
 		spin_lock_init(&fibptr->event_lock);
 		hw_fib->header.XferState = cpu_to_le32(0xffffffff);
@@ -200,14 +199,6 @@ int aac_fib_setup(struct aac_dev * dev)
 	 */
 	aac_fib_vector_assign(dev);
 
-	/*
-	 *	Add the fib chain to the free list
-	 */
-	dev->fibs[dev->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB - 1].next = NULL;
-	/*
-	*	Set 8 fibs aside for management tools
-	*/
-	dev->free_fib = &dev->fibs[dev->scsi_host_ptr->can_queue];
 	return 0;
 }
 
@@ -233,7 +224,7 @@ struct fib *aac_fib_alloc_tag(struct aac_dev *dev, struct scsi_cmnd *scmd)
 	fibptr->type = FSAFS_NTC_FIB_CONTEXT;
 	fibptr->callback_data = NULL;
 	fibptr->callback = NULL;
-	fibptr->flags = 0;
+	fibptr->scmd = scmd;
 
 	return fibptr;
 }
@@ -241,36 +232,30 @@ struct fib *aac_fib_alloc_tag(struct aac_dev *dev, struct scsi_cmnd *scmd)
 /**
  *	aac_fib_alloc	-	allocate a fib
  *	@dev: Adapter to allocate the fib for
+ *	@direction: DMA data direction
  *
  *	Allocate a fib from the adapter fib pool. If the pool is empty we
  *	return NULL.
  */
 
-struct fib *aac_fib_alloc(struct aac_dev *dev)
+struct fib *aac_fib_alloc(struct aac_dev *dev, int direction)
 {
-	struct fib * fibptr;
+	struct scsi_cmnd *scmd;
+	struct fib * fibptr = NULL;
 	unsigned long flags;
+
 	spin_lock_irqsave(&dev->fib_lock, flags);
-	fibptr = dev->free_fib;
-	if(!fibptr){
-		spin_unlock_irqrestore(&dev->fib_lock, flags);
-		return fibptr;
+	scmd = scsi_get_internal_cmd(dev->scsi_host_ptr->shost_sdev,
+				     direction, true);
+	if (scmd) {
+		fibptr = aac_fib_alloc_tag(dev, scmd);
+		fibptr->flags |= FIB_CONTEXT_FLAG_INTERNAL_CMD;
 	}
-	dev->free_fib = fibptr->next;
 	spin_unlock_irqrestore(&dev->fib_lock, flags);
-	/*
-	 *	Set the proper node type code and node byte size
-	 */
-	fibptr->type = FSAFS_NTC_FIB_CONTEXT;
+	if (!fibptr)
+		return NULL;
+
 	fibptr->size = sizeof(struct fib);
-	/*
-	 *	Null out fields that depend on being zero at the start of
-	 *	each I/O
-	 */
-	fibptr->hw_fib_va->header.XferState = 0;
-	fibptr->flags = 0;
-	fibptr->callback = NULL;
-	fibptr->callback_data = NULL;
 
 	return fibptr;
 }
@@ -298,8 +283,15 @@ void aac_fib_free(struct fib *fibptr)
 			 (void*)fibptr,
 			 le32_to_cpu(fibptr->hw_fib_va->header.XferState));
 	}
-	fibptr->next = fibptr->dev->free_fib;
-	fibptr->dev->free_fib = fibptr;
+	if (fibptr->scmd) {
+		struct scsi_cmnd *scmd = fibptr->scmd;
+
+		fibptr->scmd = NULL;
+		if (fibptr->flags & FIB_CONTEXT_FLAG_INTERNAL_CMD) {
+			scsi_put_internal_cmd(scmd);
+			fibptr->flags &= ~FIB_CONTEXT_FLAG_INTERNAL_CMD;
+		}
+	}
 	spin_unlock_irqrestore(&fibptr->dev->fib_lock, flags);
 }
 
@@ -507,7 +499,7 @@ int aac_fib_send(u16 command, struct fib *fibptr, unsigned long size,
 	 *	will have a debug mode where the adapter can notify the host
 	 *	it had a problem and the host can log that fact.
 	 */
-	fibptr->flags = 0;
+	fibptr->flags &= FIB_CONTEXT_FLAG_INTERNAL_CMD;
 	if (wait && !reply) {
 		return -EINVAL;
 	} else if (!wait && reply) {
@@ -562,7 +554,7 @@ int aac_fib_send(u16 command, struct fib *fibptr, unsigned long size,
 	if (!wait) {
 		fibptr->callback = callback;
 		fibptr->callback_data = callback_data;
-		fibptr->flags = FIB_CONTEXT_FLAG;
+		fibptr->flags |= FIB_CONTEXT_FLAG;
 	}
 
 	fibptr->done = 0;
@@ -714,7 +706,7 @@ int aac_hba_send(u8 command, struct fib *fibptr, fib_callback callback,
 	struct aac_hba_cmd_req *hbacmd = (struct aac_hba_cmd_req *)
 			fibptr->hw_fib_va;
 
-	fibptr->flags = (FIB_CONTEXT_FLAG | FIB_CONTEXT_FLAG_NATIVE_HBA);
+	fibptr->flags |= (FIB_CONTEXT_FLAG | FIB_CONTEXT_FLAG_NATIVE_HBA);
 	if (callback) {
 		wait = 0;
 		fibptr->callback = callback;
@@ -1663,7 +1655,7 @@ int aac_reset_adapter(struct aac_dev *aac, int forced, u8 reset_type)
 		retval = unblock_retval;
 	if ((forced < 2) && (retval == -ENODEV)) {
 		/* Unwind aac_send_shutdown() IOP_RESET unsupported/disabled */
-		struct fib * fibctx = aac_fib_alloc(aac);
+		struct fib * fibctx = aac_fib_alloc(aac, DMA_NONE);
 		if (fibctx) {
 			struct aac_pause *cmd;
 			int status;
@@ -2290,7 +2282,7 @@ static int aac_send_wellness_command(struct aac_dev *dev, char *wellness_str,
 	int ret = -ENOMEM;
 	u32 vbus, vid;
 
-	fibptr = aac_fib_alloc(dev);
+	fibptr = aac_fib_alloc(dev, DMA_TO_DEVICE);
 	if (!fibptr)
 		goto out;
 
@@ -2388,7 +2380,7 @@ static int aac_send_hosttime(struct aac_dev *dev, struct timespec64 *now)
 	struct fib *fibptr;
 	__le32 *info;
 
-	fibptr = aac_fib_alloc(dev);
+	fibptr = aac_fib_alloc(dev, DMA_TO_DEVICE);
 	if (!fibptr)
 		goto out;
 
diff --git a/drivers/scsi/aacraid/dpcsup.c b/drivers/scsi/aacraid/dpcsup.c
index fbe334c59f37..9424792432ec 100644
--- a/drivers/scsi/aacraid/dpcsup.c
+++ b/drivers/scsi/aacraid/dpcsup.c
@@ -315,7 +315,7 @@ unsigned int aac_intr_normal(struct aac_dev *dev, u32 index, int isAif,
 		struct fib *fibctx;
 		struct aac_aifcmd *cmd;
 
-		fibctx = aac_fib_alloc(dev);
+		fibctx = aac_fib_alloc(dev, DMA_FROM_DEVICE);
 		if (!fibctx)
 			return 1;
 		aac_fib_init(fibctx);
diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index b92a6595358e..7268118186d1 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -716,7 +716,7 @@ static int aac_eh_abort(struct scsi_cmnd* cmd)
 			return ret;
 
 		/* start a HBA_TMF_ABORT_TASK TMF request */
-		fib = aac_fib_alloc(aac);
+		fib = aac_fib_alloc(aac, DMA_NONE);
 		if (!fib)
 			return ret;
 
@@ -927,7 +927,7 @@ static int aac_eh_dev_reset(struct scsi_cmnd *cmd)
 	pr_err("%s: Host device reset request. SCSI hang ?\n",
 	       AAC_DRIVERNAME);
 
-	fib = aac_fib_alloc(aac);
+	fib = aac_fib_alloc(aac, DMA_NONE);
 	if (!fib)
 		return ret;
 
@@ -990,7 +990,7 @@ static int aac_eh_target_reset(struct scsi_cmnd *cmd)
 	pr_err("%s: Host target reset request. SCSI hang ?\n",
 	       AAC_DRIVERNAME);
 
-	fib = aac_fib_alloc(aac);
+	fib = aac_fib_alloc(aac, DMA_NONE);
 	if (!fib)
 		return ret;
 
@@ -1507,6 +1507,7 @@ static struct scsi_host_template aac_driver_template = {
 #endif
 	.emulated			= 1,
 	.no_write_same			= 1,
+	.alloc_host_sdev		= 1,
 };
 
 static void __aac_shutdown(struct aac_dev * aac)
@@ -1645,6 +1646,7 @@ static int aac_probe_one(struct pci_dev *pdev, const struct pci_device_id *id)
 	shost->max_cmd_len = 16;
 	shost->max_id = MAXIMUM_NUM_CONTAINERS;
 	shost->max_lun = AAC_MAX_LUN;
+	shost->nr_reserved_cmds = AAC_NUM_MGT_FIB;
 	shost->sg_tablesize = HBA_MAX_SG_SEPARATE;
 
 	if (aac_cfg_major == AAC_CHARDEV_NEEDS_REINIT)
-- 
2.29.2

[PATCH 15/15] aacraid: use scsi_host_busy_iter() to traverse outstanding commands

[Hannes Reinecke]

Instead of walking the array of potential commands and trying to figure out
which one might be pending the driver should be using
scsi_host_busy_iter() to traverse all outstanding commands.
And for command abort we can now lookup the fibs directly as we now have
a 1:1 mapping between request tags and fibs.

Signed-off-by: Hannes Reinecke <hare@suse.de>
---
 drivers/scsi/aacraid/commsup.c |  49 ++++++------
 drivers/scsi/aacraid/linit.c   | 131 ++++++++++++++-------------------
 2 files changed, 84 insertions(+), 96 deletions(-)

diff --git a/drivers/scsi/aacraid/commsup.c b/drivers/scsi/aacraid/commsup.c
index aaa9b5d6f55d..54ec5a3435cd 100644
--- a/drivers/scsi/aacraid/commsup.c
+++ b/drivers/scsi/aacraid/commsup.c
@@ -1465,6 +1465,32 @@ static void aac_schedule_bus_scan(struct aac_dev *aac)
 		aac_schedule_src_reinit_aif_worker(aac);
 }
 
+static bool aac_close_sync_fib_iter(struct scsi_cmnd *command, void *data,
+				    bool reserved)
+{
+	struct Scsi_Host *host = command->device->host;
+	struct aac_dev *aac = (struct aac_dev *)host->hostdata;
+	struct fib *fib = &aac->fibs[command->request->tag];
+	int *retval = data;
+	__le32 XferState = fib->hw_fib_va->header.XferState;
+	bool is_response_expected = false;
+
+	if (!(XferState & cpu_to_le32(NoResponseExpected | Async)) &&
+	    (XferState & cpu_to_le32(ResponseExpected)))
+		is_response_expected = true;
+
+	if (is_response_expected
+	    || fib->flags & FIB_CONTEXT_FLAG_WAIT) {
+		unsigned long flagv;
+		spin_lock_irqsave(&fib->event_lock, flagv);
+		complete(&fib->event_wait);
+		spin_unlock_irqrestore(&fib->event_lock, flagv);
+		schedule();
+		*retval = 0;
+	}
+	return true;
+}
+
 static int _aac_reset_adapter(struct aac_dev *aac, int forced, u8 reset_type)
 {
 	int index, quirks;
@@ -1473,7 +1499,6 @@ static int _aac_reset_adapter(struct aac_dev *aac, int forced, u8 reset_type)
 	int jafo = 0;
 	int bled;
 	u64 dmamask;
-	int num_of_fibs = 0;
 
 	/*
 	 * Assumptions:
@@ -1507,27 +1532,7 @@ static int _aac_reset_adapter(struct aac_dev *aac, int forced, u8 reset_type)
 	 *	Loop through the fibs, close the synchronous FIBS
 	 */
 	retval = 1;
-	num_of_fibs = aac->scsi_host_ptr->can_queue + AAC_NUM_MGT_FIB;
-	for (index = 0; index <  num_of_fibs; index++) {
-
-		struct fib *fib = &aac->fibs[index];
-		__le32 XferState = fib->hw_fib_va->header.XferState;
-		bool is_response_expected = false;
-
-		if (!(XferState & cpu_to_le32(NoResponseExpected | Async)) &&
-		   (XferState & cpu_to_le32(ResponseExpected)))
-			is_response_expected = true;
-
-		if (is_response_expected
-		  || fib->flags & FIB_CONTEXT_FLAG_WAIT) {
-			unsigned long flagv;
-			spin_lock_irqsave(&fib->event_lock, flagv);
-			complete(&fib->event_wait);
-			spin_unlock_irqrestore(&fib->event_lock, flagv);
-			schedule();
-			retval = 0;
-		}
-	}
+	scsi_host_busy_iter(host, aac_close_sync_fib_iter, &retval);
 	/* Give some extra time for ioctls to complete. */
 	if (retval == 0)
 		ssleep(2);
diff --git a/drivers/scsi/aacraid/linit.c b/drivers/scsi/aacraid/linit.c
index 7268118186d1..6db6e7377cb0 100644
--- a/drivers/scsi/aacraid/linit.c
+++ b/drivers/scsi/aacraid/linit.c
@@ -683,7 +683,8 @@ static int aac_eh_abort(struct scsi_cmnd* cmd)
 	struct scsi_device * dev = cmd->device;
 	struct Scsi_Host * host = dev->host;
 	struct aac_dev * aac = (struct aac_dev *)host->hostdata;
-	int count, found;
+	struct fib *fib;
+	int count;
 	u32 bus, cid;
 	int ret = FAILED;
 
@@ -693,26 +694,20 @@ static int aac_eh_abort(struct scsi_cmnd* cmd)
 	bus = aac_logical_to_phys(scmd_channel(cmd));
 	cid = scmd_id(cmd);
 	if (aac->hba_map[bus][cid].devtype == AAC_DEVTYPE_NATIVE_RAW) {
-		struct fib *fib;
 		struct aac_hba_tm_req *tmf;
 		int status;
 		u64 address;
 
 		pr_err("%s: Host adapter abort request (%d,%d,%d,%d)\n",
-		 AAC_DRIVERNAME,
-		 host->host_no, sdev_channel(dev), sdev_id(dev), (int)dev->lun);
-
-		found = 0;
-		for (count = 0; count < (host->can_queue + AAC_NUM_MGT_FIB); ++count) {
-			fib = &aac->fibs[count];
-			if (*(u8 *)fib->hw_fib_va != 0 &&
-				(fib->flags & FIB_CONTEXT_FLAG_NATIVE_HBA) &&
-				(fib->callback_data == cmd)) {
-				found = 1;
-				break;
-			}
-		}
-		if (!found)
+		       AAC_DRIVERNAME, host->host_no,
+		       sdev_channel(dev), sdev_id(dev), (int)dev->lun);
+
+		fib = &aac->fibs[cmd->request->tag];
+		if (*(u8 *)fib->hw_fib_va != 0 &&
+		    (fib->flags & FIB_CONTEXT_FLAG_NATIVE_HBA) &&
+		    (fib->callback_data == cmd))
+			ret = SUCCESS;
+		if (ret == FAILED)
 			return ret;
 
 		/* start a HBA_TMF_ABORT_TASK TMF request */
@@ -774,20 +769,13 @@ static int aac_eh_abort(struct scsi_cmnd* cmd)
 			 * Mark associated FIB to not complete,
 			 * eh handler does this
 			 */
-			for (count = 0;
-				count < (host->can_queue + AAC_NUM_MGT_FIB);
-				++count) {
-				struct fib *fib = &aac->fibs[count];
-
-				if (fib->hw_fib_va->header.XferState &&
-				(fib->flags & FIB_CONTEXT_FLAG) &&
-				(fib->callback_data == cmd)) {
-					fib->flags |=
-						FIB_CONTEXT_FLAG_TIMED_OUT;
-					cmd->SCp.phase =
-						AAC_OWNER_ERROR_HANDLER;
-					ret = SUCCESS;
-				}
+			fib = &aac->fibs[cmd->request->tag];
+			if (fib->hw_fib_va->header.XferState &&
+			    (fib->flags & FIB_CONTEXT_FLAG) &&
+			    (fib->callback_data == cmd)) {
+				fib->flags |= FIB_CONTEXT_FLAG_TIMED_OUT;
+				cmd->SCp.phase = AAC_OWNER_ERROR_HANDLER;
+				ret = SUCCESS;
 			}
 			break;
 		case TEST_UNIT_READY:
@@ -795,27 +783,14 @@ static int aac_eh_abort(struct scsi_cmnd* cmd)
 			 * Mark associated FIB to not complete,
 			 * eh handler does this
 			 */
-			for (count = 0;
-				count < (host->can_queue + AAC_NUM_MGT_FIB);
-				++count) {
-				struct scsi_cmnd *command;
-				struct fib *fib = &aac->fibs[count];
-
-				command = fib->callback_data;
-
-				if ((fib->hw_fib_va->header.XferState &
-					cpu_to_le32
-					(Async | NoResponseExpected)) &&
-					(fib->flags & FIB_CONTEXT_FLAG) &&
-					((command)) &&
-					(command->device == cmd->device)) {
-					fib->flags |=
-						FIB_CONTEXT_FLAG_TIMED_OUT;
-					command->SCp.phase =
-						AAC_OWNER_ERROR_HANDLER;
-					if (command == cmd)
-						ret = SUCCESS;
-				}
+			fib = &aac->fibs[cmd->request->tag];
+			if ((fib->hw_fib_va->header.XferState &
+			     cpu_to_le32(Async | NoResponseExpected)) &&
+			    (fib->flags & FIB_CONTEXT_FLAG) &&
+			    (fib->callback_data == cmd)) {
+				fib->flags |= FIB_CONTEXT_FLAG_TIMED_OUT;
+				cmd->SCp.phase = AAC_OWNER_ERROR_HANDLER;
+				ret = SUCCESS;
 			}
 			break;
 		}
@@ -1023,6 +998,36 @@ static int aac_eh_target_reset(struct scsi_cmnd *cmd)
 	return ret;
 }
 
+static bool aac_eh_bus_reset_iter(struct scsi_cmnd *cmd, void *data,
+				  bool reserved)
+{
+	struct Scsi_Host *host = cmd->device->host;
+	struct aac_dev *aac = (struct aac_dev *)host->hostdata;
+	struct fib *fib = &aac->fibs[cmd->request->tag];
+	int *cmd_bus = data;
+
+	if (fib->hw_fib_va->header.XferState &&
+	    (fib->flags & FIB_CONTEXT_FLAG) &&
+	    (fib->flags & FIB_CONTEXT_FLAG_SCSI_CMD)) {
+		struct aac_hba_map_info *info;
+		u32 bus, cid;
+
+		if (cmd != (struct scsi_cmnd *)fib->callback_data)
+			return true;
+		bus = aac_logical_to_phys(scmd_channel(cmd));
+		if (bus != *cmd_bus)
+			return true;
+		cid = scmd_id(cmd);
+		info = &aac->hba_map[bus][cid];
+		if (bus >= AAC_MAX_BUSES || cid >= AAC_MAX_TARGETS ||
+		    info->devtype != AAC_DEVTYPE_NATIVE_RAW) {
+			fib->flags |= FIB_CONTEXT_FLAG_EH_RESET;
+			cmd->SCp.phase = AAC_OWNER_ERROR_HANDLER;
+		}
+	}
+	return true;
+}
+
 /*
  *	aac_eh_bus_reset	- Bus reset command handling
  *	@scsi_cmd:	SCSI command block causing the reset
@@ -1040,29 +1045,7 @@ static int aac_eh_bus_reset(struct scsi_cmnd* cmd)
 
 	cmd_bus = aac_logical_to_phys(scmd_channel(cmd));
 	/* Mark the assoc. FIB to not complete, eh handler does this */
-	for (count = 0; count < (host->can_queue + AAC_NUM_MGT_FIB); ++count) {
-		struct fib *fib = &aac->fibs[count];
-
-		if (fib->hw_fib_va->header.XferState &&
-		    (fib->flags & FIB_CONTEXT_FLAG) &&
-		    (fib->flags & FIB_CONTEXT_FLAG_SCSI_CMD)) {
-			struct aac_hba_map_info *info;
-			u32 bus, cid;
-
-			cmd = (struct scsi_cmnd *)fib->callback_data;
-			bus = aac_logical_to_phys(scmd_channel(cmd));
-			if (bus != cmd_bus)
-				continue;
-			cid = scmd_id(cmd);
-			info = &aac->hba_map[bus][cid];
-			if (bus >= AAC_MAX_BUSES || cid >= AAC_MAX_TARGETS ||
-			    info->devtype != AAC_DEVTYPE_NATIVE_RAW) {
-				fib->flags |= FIB_CONTEXT_FLAG_EH_RESET;
-				cmd->SCp.phase = AAC_OWNER_ERROR_HANDLER;
-			}
-		}
-	}
-
+	scsi_host_busy_iter(host, aac_eh_bus_reset_iter, &cmd_bus);
 	pr_err("%s: Host bus reset request. SCSI hang ?\n", AAC_DRIVERNAME);
 
 	/*
-- 
2.29.2

Re: [PATCH 01/15] scsi: allocate host device

[Bart Van Assche]

On 11/25/21 7:10 AM, Hannes Reinecke wrote:
> +/**
> + * scsi_get_host_dev - Create a virtual scsi_device to the host adapter
                           ^^^^^^
                           Attach?

> @@ -500,7 +500,8 @@ static void scsi_device_dev_release_usercontext(struct work_struct *work)
>   		kfree_rcu(vpd_pg80, rcu);
>   	if (vpd_pg89)
>   		kfree_rcu(vpd_pg89, rcu);
> -	kfree(sdev->inquiry);
> +	if (!scsi_device_is_host_dev(sdev))
> +		kfree(sdev->inquiry);
>   	kfree(sdev);

kfree() accepts a NULL pointer so please leave out the new if-test.

> -#define MODULE_ALIAS_SCSI_DEVICE(type) \
> +#define MODULE_ALIAS_SCSI_DEVICE(type)			\
>   	MODULE_ALIAS("scsi:t-" __stringify(type) "*")

The above change seems not related to the rest of this patch? Can it be left out?

Otherwise this patch looks good to me.

Thanks,

Bart.

Re: [PATCH 01/15] scsi: allocate host device

[chenxiang (M)]

Hi Hannes,


在 2021/11/25 23:10, Hannes Reinecke 写道:
> Add a flag 'alloc_host_dev' to the SCSI host template and allocate
> a virtual scsi device if the flag is set.
> This device has the SCSI id <max_id + 1>:0, so won't clash with any
> devices the HBA might allocate. It's also excluded from scanning and
> will not show up in sysfs.
> Intention is to use this device to send internal commands to the HBA.
>
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
>   drivers/scsi/hosts.c       |  8 +++++
>   drivers/scsi/scsi_scan.c   | 67 +++++++++++++++++++++++++++++++++++++-
>   drivers/scsi/scsi_sysfs.c  |  3 +-
>   include/scsi/scsi_device.h |  2 +-
>   include/scsi/scsi_host.h   | 21 ++++++++++++
>   5 files changed, 98 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index f69b77cbf538..a539fa2fb221 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -290,6 +290,14 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>   	if (error)
>   		goto out_del_dev;
>   
> +	if (sht->alloc_host_sdev) {
> +		shost->shost_sdev = scsi_get_host_dev(shost);
> +		if (!shost->shost_sdev) {
> +			error = -ENOMEM;
> +			goto out_del_dev;
> +		}
> +	}
> +
>   	scsi_proc_host_add(shost);
>   	scsi_autopm_put_host(shost);
>   	return error;
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index 328c0e79dfe7..e2910aa02a65 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -1139,6 +1139,12 @@ static int scsi_probe_and_add_lun(struct scsi_target *starget,
>   	if (!sdev)
>   		goto out;
>   
> +	if (scsi_device_is_host_dev(sdev)) {
> +		if (bflagsp)
> +			*bflagsp = BLIST_NOLUN;
> +		return SCSI_SCAN_LUN_PRESENT;
> +	}
> +
>   	result = kmalloc(result_len, GFP_KERNEL);
>   	if (!result)
>   		goto out_free_sdev;
> @@ -1755,6 +1761,9 @@ static void scsi_sysfs_add_devices(struct Scsi_Host *shost)
>   		/* If device is already visible, skip adding it to sysfs */
>   		if (sdev->is_visible)
>   			continue;
> +		/* Host devices should never be visible in sysfs */
> +		if (scsi_device_is_host_dev(sdev))
> +			continue;
>   		if (!scsi_host_scan_allowed(shost) ||
>   		    scsi_sysfs_add_sdev(sdev) != 0)
>   			__scsi_remove_device(sdev);
> @@ -1919,12 +1928,16 @@ EXPORT_SYMBOL(scsi_scan_host);
>   
>   void scsi_forget_host(struct Scsi_Host *shost)
>   {
> -	struct scsi_device *sdev;
> +	struct scsi_device *sdev, *host_sdev = NULL;
>   	unsigned long flags;
>   
>    restart:
>   	spin_lock_irqsave(shost->host_lock, flags);
>   	list_for_each_entry(sdev, &shost->__devices, siblings) {
> +		if (scsi_device_is_host_dev(sdev)) {
> +			host_sdev = sdev;
> +			continue;
> +		}
>   		if (sdev->sdev_state == SDEV_DEL)
>   			continue;
>   		spin_unlock_irqrestore(shost->host_lock, flags);
> @@ -1932,5 +1945,57 @@ void scsi_forget_host(struct Scsi_Host *shost)
>   		goto restart;
>   	}
>   	spin_unlock_irqrestore(shost->host_lock, flags);
> +	/* Remove host device last, might be needed to send commands */
> +	if (host_sdev)
> +		__scsi_remove_device(host_sdev);
>   }
>   
> +/**
> + * scsi_get_host_dev - Create a virtual scsi_device to the host adapter
> + * @shost: Host that needs a scsi_device
> + *
> + * Lock status: None assumed.
> + *
> + * Returns:     The scsi_device or NULL
> + *
> + * Notes:
> + *	Attach a single scsi_device to the Scsi_Host. The primary aim
> + *	for this device is to serve as a container from which valid
> + *	scsi commands can be allocated from. Each scsi command will carry
> + *	an unused/free command tag, which then can be used by the LLDD to
> + *	send internal or passthrough commands without having to find a
> + *	valid command tag internally.
> + */
> +struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost)
> +{
> +	struct scsi_device *sdev = NULL;
> +	struct scsi_target *starget;
> +
> +	mutex_lock(&shost->scan_mutex);
> +	if (!scsi_host_scan_allowed(shost))
> +		goto out;
> +	starget = scsi_alloc_target(&shost->shost_gendev, 0,
> +				    shost->max_id);
> +	if (!starget)
> +		goto out;
> +
> +	sdev = scsi_alloc_sdev(starget, 0, NULL);
> +	if (sdev)
> +		sdev->borken = 0;
> +	else
> +		scsi_target_reap(starget);

Currently many scsi drivers fill some interfaces such as 
target_alloc()/slave_alloc() for real disks.
When allocating scsi target and scsi device for host dev, it will also 
call those interfaces, and not sure whether it breaks those drivers.
 From function sas_target_alloc() (common interface in libsas layer), it 
seems break it as there is no sas_rphy for host dev.

Re: [PATCH 06/15] hpsa: use scsi_host_busy_iter() to traverse outstanding commands

[John Garry]

On 25/11/2021 15:10, Hannes Reinecke wrote:
>   static void hpsa_drain_accel_commands(struct ctlr_info *h)
>   {
> -	struct CommandList *c = NULL;
> -	int i, accel_cmds_out;
> -	int refcount;
> +	struct hpsa_command_iter_data iter_data = {
> +		.h = h,
> +	};
>   
>   	do { /* wait for all outstanding ioaccel commands to drain out */
> -		accel_cmds_out = 0;
> -		for (i = 0; i < h->nr_cmds; i++) {
> -			c = h->cmd_pool + i;
> -			refcount = atomic_inc_return(&c->refcount);
> -			if (refcount > 1) /* Command is allocated */
> -				accel_cmds_out += is_accelerated_cmd(c);
> -			cmd_free(h, c);
> -		}
> -		if (accel_cmds_out <= 0)
> +		iter_data.count = 0;
> +		scsi_host_busy_iter(h->scsi_host,
> +				    hpsa_drain_accel_commands_iter,
> +				    &iter_data);

I haven't following this code exactly, but I assume that you want to 
iter the reserved requests as well (or in other places in others drivers 
in this series). For that to work we need to call 
blk_mq_start_request(), right? I could not see it called.

> +		if (iter_data.count <= 0)
>   			break;
>   		msleep(100);
>   	} while (1);
> -- 2.29.2

Re: [PATCH 02/15] scsi: add scsi_{get,put}_internal_cmd() helper

[John Garry]

On 25/11/2021 15:10, Hannes Reinecke wrote:
> +/**
> + * scsi_get_internal_cmd - allocate an internal SCSI command
> + * @sdev: SCSI device from which to allocate the command
> + * @data_direction: Data direction for the allocated command
> + * @nowait: do not wait for command allocation to succeed.
> + *
> + * Allocates a SCSI command for internal LLDD use.
> + */
> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,
> +	int data_direction, bool nowait)
> +{
> +	struct request *rq;
> +	struct scsi_cmnd *scmd;
> +	blk_mq_req_flags_t flags = 0;
> +	int op;
> +
> +	if (nowait)
> +		flags |= BLK_MQ_REQ_NOWAIT;
> +	op = (data_direction == DMA_TO_DEVICE) ?
> +		REQ_OP_DRV_OUT : REQ_OP_DRV_IN;
> +	rq = blk_mq_alloc_request(sdev->request_queue, op, flags);
> +	if (IS_ERR(rq))
> +		return NULL;
> +	scmd = blk_mq_rq_to_pdu(rq);
> +	scmd->device = sdev;
> +	return scmd;
> +}
> +EXPORT_SYMBOL_GPL(scsi_get_internal_cmd);

So there are a couple of generally-accepted grievances about this approach:
a. we're being allocated a scsi_cmnd, but not using what is being 
allocated as a scsi_cmnd, but rather just a holder as a reference to an 
allocated tag
b. we're being allocated a request, which is not being sent through the 
block layer*

It just seems to me that what the block layer is providing is not suitable.

How about these:
a. allow block driver to specify size of reserved request PDU separately 
to regular requests, so we can use something like this for rsvd commands:
struct scsi_rsvd_cmnd {
	struct scsi_device *sdev;
}
And fix up SCSI iter functions and LLDs to deal with it.
b. provide block layer API to provide just same as is returned from 
blk_mq_unique_tag(), but no request is provided. This just gives what we 
need but would be disruptive in scsi layer and LLDs.
c. as alternative to b., send all rsvd requests through the block layer, 
but can be very difficult+disruptive for users

*For polling rsvd commands on a poll queue (which we will need for 
hisi_sas driver and maybe others for poll mode support), we would need 
to send the request through the block layer, but block layer polling 
requires a request with a bio, which is a problem.

Thanks,
John

Re: [PATCH 02/15] scsi: add scsi_{get,put}_internal_cmd() helper

[Bart Van Assche]

On 11/25/21 07:10, Hannes Reinecke wrote:
> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,
> +	int data_direction, bool nowait)

Please use enum dma_data_direction instead of 'int' for the data direction.
> +{
> +	struct request *rq;
> +	struct scsi_cmnd *scmd;
> +	blk_mq_req_flags_t flags = 0;
> +	int op;

The name 'op' is confusing since that variable is a bitfield that 
includes the operation and operation flags. Consider one of the names 
'opf', 'op_and_flags' or 'cmd_and_flags'. Please also change the data 
type from 'int' into 'unsigned int' or 'u32'.

> +	op = (data_direction == DMA_TO_DEVICE) ?
> +		REQ_OP_DRV_OUT : REQ_OP_DRV_IN;

Please leave out the parentheses from the above assignment.

Thanks,

Bart.

Re: [PATCH 02/15] scsi: add scsi_{get,put}_internal_cmd() helper

[Bart Van Assche]

On 11/26/21 01:58, John Garry wrote:
> How about these:
> a. allow block driver to specify size of reserved request PDU separately 
> to regular requests, so we can use something like this for rsvd commands:
> struct scsi_rsvd_cmnd {
>      struct scsi_device *sdev;
> }
> And fix up SCSI iter functions and LLDs to deal with it.
> b. provide block layer API to provide just same as is returned from 
> blk_mq_unique_tag(), but no request is provided. This just gives what we 
> need but would be disruptive in scsi layer and LLDs.
> c. as alternative to b., send all rsvd requests through the block layer, 
> but can be very difficult+disruptive for users
> 
> *For polling rsvd commands on a poll queue (which we will need for 
> hisi_sas driver and maybe others for poll mode support), we would need 
> to send the request through the block layer, but block layer polling 
> requires a request with a bio, which is a problem.

How about postponing these changes until a patch is ready that converts 
at least one SCSI LLD such that it uses the above functionality?

Thanks,

Bart.

Re: [PATCH 03/15] scsi: implement reserved command handling

[Bart Van Assche]

On 11/25/21 07:10, Hannes Reinecke wrote:
> Quite some drivers are using management commands internally, which
> typically use the same hardware tag pool (ie they are being allocated
> from the same hardware resources) as the 'normal' I/O commands.
> These commands are set aside before allocating the block-mq tag bitmap,
> so they'll never show up as busy in the tag map.
> The block-layer, OTOH, already has 'reserved_tags' to handle precisely
> this situation.
> So this patch adds a new field 'nr_reserved_cmds' to the SCSI host
> template to instruct the block layer to set aside a tag space for these
> management commands by using reserved tags.

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

Re: [PATCH 01/15] scsi: allocate host device

[Hannes Reinecke]

On 11/26/21 12:16 AM, Bart Van Assche wrote:
> On 11/25/21 7:10 AM, Hannes Reinecke wrote:
>> +/**
>> + * scsi_get_host_dev - Create a virtual scsi_device to the host adapter
>                            ^^^^^^
>                            Attach?
> 
It's just words ... sure I can change it.

>> @@ -500,7 +500,8 @@ static void 
>> scsi_device_dev_release_usercontext(struct work_struct *work)
>>           kfree_rcu(vpd_pg80, rcu);
>>       if (vpd_pg89)
>>           kfree_rcu(vpd_pg89, rcu);
>> -    kfree(sdev->inquiry);
>> +    if (!scsi_device_is_host_dev(sdev))
>> +        kfree(sdev->inquiry);
>>       kfree(sdev);
> 
> kfree() accepts a NULL pointer so please leave out the new if-test.
> 
Actually a left-over from a different patch (to use 'real' inquiry data 
for dummy devices). Will be removing it.

>> -#define MODULE_ALIAS_SCSI_DEVICE(type) \
>> +#define MODULE_ALIAS_SCSI_DEVICE(type)            \
>>       MODULE_ALIAS("scsi:t-" __stringify(type) "*")
> 
> The above change seems not related to the rest of this patch? Can it be 
> left out?
> 
Sure.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 01/15] scsi: allocate host device

[Hannes Reinecke]

On 11/26/21 3:47 AM, chenxiang (M) wrote:
> Hi Hannes,
> 
> 
> 在 2021/11/25 23:10, Hannes Reinecke 写道:
>> Add a flag 'alloc_host_dev' to the SCSI host template and allocate
>> a virtual scsi device if the flag is set.
>> This device has the SCSI id <max_id + 1>:0, so won't clash with any
>> devices the HBA might allocate. It's also excluded from scanning and
>> will not show up in sysfs.
>> Intention is to use this device to send internal commands to the HBA.
>>
>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>> ---
>>   drivers/scsi/hosts.c       |  8 +++++
>>   drivers/scsi/scsi_scan.c   | 67 +++++++++++++++++++++++++++++++++++++-
>>   drivers/scsi/scsi_sysfs.c  |  3 +-
>>   include/scsi/scsi_device.h |  2 +-
>>   include/scsi/scsi_host.h   | 21 ++++++++++++
>>   5 files changed, 98 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>> index f69b77cbf538..a539fa2fb221 100644
>> --- a/drivers/scsi/hosts.c
>> +++ b/drivers/scsi/hosts.c
>> @@ -290,6 +290,14 @@ int scsi_add_host_with_dma(struct Scsi_Host 
>> *shost, struct device *dev,
>>       if (error)
>>           goto out_del_dev;
>> +    if (sht->alloc_host_sdev) {
>> +        shost->shost_sdev = scsi_get_host_dev(shost);
>> +        if (!shost->shost_sdev) {
>> +            error = -ENOMEM;
>> +            goto out_del_dev;
>> +        }
>> +    }
>> +
>>       scsi_proc_host_add(shost);
>>       scsi_autopm_put_host(shost);
>>       return error;
>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>> index 328c0e79dfe7..e2910aa02a65 100644
>> --- a/drivers/scsi/scsi_scan.c
>> +++ b/drivers/scsi/scsi_scan.c
>> @@ -1139,6 +1139,12 @@ static int scsi_probe_and_add_lun(struct 
>> scsi_target *starget,
>>       if (!sdev)
>>           goto out;
>> +    if (scsi_device_is_host_dev(sdev)) {
>> +        if (bflagsp)
>> +            *bflagsp = BLIST_NOLUN;
>> +        return SCSI_SCAN_LUN_PRESENT;
>> +    }
>> +
>>       result = kmalloc(result_len, GFP_KERNEL);
>>       if (!result)
>>           goto out_free_sdev;
>> @@ -1755,6 +1761,9 @@ static void scsi_sysfs_add_devices(struct 
>> Scsi_Host *shost)
>>           /* If device is already visible, skip adding it to sysfs */
>>           if (sdev->is_visible)
>>               continue;
>> +        /* Host devices should never be visible in sysfs */
>> +        if (scsi_device_is_host_dev(sdev))
>> +            continue;
>>           if (!scsi_host_scan_allowed(shost) ||
>>               scsi_sysfs_add_sdev(sdev) != 0)
>>               __scsi_remove_device(sdev);
>> @@ -1919,12 +1928,16 @@ EXPORT_SYMBOL(scsi_scan_host);
>>   void scsi_forget_host(struct Scsi_Host *shost)
>>   {
>> -    struct scsi_device *sdev;
>> +    struct scsi_device *sdev, *host_sdev = NULL;
>>       unsigned long flags;
>>    restart:
>>       spin_lock_irqsave(shost->host_lock, flags);
>>       list_for_each_entry(sdev, &shost->__devices, siblings) {
>> +        if (scsi_device_is_host_dev(sdev)) {
>> +            host_sdev = sdev;
>> +            continue;
>> +        }
>>           if (sdev->sdev_state == SDEV_DEL)
>>               continue;
>>           spin_unlock_irqrestore(shost->host_lock, flags);
>> @@ -1932,5 +1945,57 @@ void scsi_forget_host(struct Scsi_Host *shost)
>>           goto restart;
>>       }
>>       spin_unlock_irqrestore(shost->host_lock, flags);
>> +    /* Remove host device last, might be needed to send commands */
>> +    if (host_sdev)
>> +        __scsi_remove_device(host_sdev);
>>   }
>> +/**
>> + * scsi_get_host_dev - Create a virtual scsi_device to the host adapter
>> + * @shost: Host that needs a scsi_device
>> + *
>> + * Lock status: None assumed.
>> + *
>> + * Returns:     The scsi_device or NULL
>> + *
>> + * Notes:
>> + *    Attach a single scsi_device to the Scsi_Host. The primary aim
>> + *    for this device is to serve as a container from which valid
>> + *    scsi commands can be allocated from. Each scsi command will carry
>> + *    an unused/free command tag, which then can be used by the LLDD to
>> + *    send internal or passthrough commands without having to find a
>> + *    valid command tag internally.
>> + */
>> +struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost)
>> +{
>> +    struct scsi_device *sdev = NULL;
>> +    struct scsi_target *starget;
>> +
>> +    mutex_lock(&shost->scan_mutex);
>> +    if (!scsi_host_scan_allowed(shost))
>> +        goto out;
>> +    starget = scsi_alloc_target(&shost->shost_gendev, 0,
>> +                    shost->max_id);
>> +    if (!starget)
>> +        goto out;
>> +
>> +    sdev = scsi_alloc_sdev(starget, 0, NULL);
>> +    if (sdev)
>> +        sdev->borken = 0;
>> +    else
>> +        scsi_target_reap(starget);
> 
> Currently many scsi drivers fill some interfaces such as 
> target_alloc()/slave_alloc() for real disks.
> When allocating scsi target and scsi device for host dev, it will also 
> call those interfaces, and not sure whether it breaks those drivers.
>  From function sas_target_alloc() (common interface in libsas layer), it 
> seems break it as there is no sas_rphy for host dev.
> 
Ah. Didn't consider that.
Will be having a look and fixup the patch.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 06/15] hpsa: use scsi_host_busy_iter() to traverse outstanding commands

[Hannes Reinecke]

On 11/26/21 10:33 AM, John Garry wrote:
> On 25/11/2021 15:10, Hannes Reinecke wrote:
>>   static void hpsa_drain_accel_commands(struct ctlr_info *h)
>>   {
>> -    struct CommandList *c = NULL;
>> -    int i, accel_cmds_out;
>> -    int refcount;
>> +    struct hpsa_command_iter_data iter_data = {
>> +        .h = h,
>> +    };
>>       do { /* wait for all outstanding ioaccel commands to drain out */
>> -        accel_cmds_out = 0;
>> -        for (i = 0; i < h->nr_cmds; i++) {
>> -            c = h->cmd_pool + i;
>> -            refcount = atomic_inc_return(&c->refcount);
>> -            if (refcount > 1) /* Command is allocated */
>> -                accel_cmds_out += is_accelerated_cmd(c);
>> -            cmd_free(h, c);
>> -        }
>> -        if (accel_cmds_out <= 0)
>> +        iter_data.count = 0;
>> +        scsi_host_busy_iter(h->scsi_host,
>> +                    hpsa_drain_accel_commands_iter,
>> +                    &iter_data);
> 
> I haven't following this code exactly, but I assume that you want to 
> iter the reserved requests as well (or in other places in others drivers 
> in this series). For that to work we need to call 
> blk_mq_start_request(), right? I could not see it called.
> 
Actually, no; this is iterating over 'accel' commands, ie fastpath 
commands for RAID I/0. Which none of the reserved commands are.

But that doesn't mean that your comment about reserved commands not 
being started is invalid. Hmm.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 02/15] scsi: add scsi_{get,put}_internal_cmd() helper

[Bart Van Assche]

On 11/25/21 07:10, Hannes Reinecke wrote:
> Add helper functions to allow LLDDs to allocate and free
> internal commands.

Are the changes for the SCSI timeout handler perhaps missing from this 
patch? In the UFS driver we need the ability not to trigger the SCSI 
error handler if an internal command times out.

Thanks,

Bart.

Re: [PATCH 02/15] scsi: add scsi_{get,put}_internal_cmd() helper

[Hannes Reinecke]

On 11/26/21 10:58 AM, John Garry wrote:
> On 25/11/2021 15:10, Hannes Reinecke wrote:
>> +/**
>> + * scsi_get_internal_cmd - allocate an internal SCSI command
>> + * @sdev: SCSI device from which to allocate the command
>> + * @data_direction: Data direction for the allocated command
>> + * @nowait: do not wait for command allocation to succeed.
>> + *
>> + * Allocates a SCSI command for internal LLDD use.
>> + */
>> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,
>> +    int data_direction, bool nowait)
>> +{
>> +    struct request *rq;
>> +    struct scsi_cmnd *scmd;
>> +    blk_mq_req_flags_t flags = 0;
>> +    int op;
>> +
>> +    if (nowait)
>> +        flags |= BLK_MQ_REQ_NOWAIT;
>> +    op = (data_direction == DMA_TO_DEVICE) ?
>> +        REQ_OP_DRV_OUT : REQ_OP_DRV_IN;
>> +    rq = blk_mq_alloc_request(sdev->request_queue, op, flags);
>> +    if (IS_ERR(rq))
>> +        return NULL;
>> +    scmd = blk_mq_rq_to_pdu(rq);
>> +    scmd->device = sdev;
>> +    return scmd;
>> +}
>> +EXPORT_SYMBOL_GPL(scsi_get_internal_cmd);
> 
> So there are a couple of generally-accepted grievances about this approach:
> a. we're being allocated a scsi_cmnd, but not using what is being 
> allocated as a scsi_cmnd, but rather just a holder as a reference to an 
> allocated tag
> b. we're being allocated a request, which is not being sent through the 
> block layer*
> 
And while being true in general, it does make some assumptions:
- Reserved commands will never being sent via the block layer
- None of the drivers will need to use the additional 'scsi_cmnd' payload.

Here I'm not sure if this is true in general.
While it doesn't seem to be necessary to send reserved commands via the 
block layer (ie calling 'queue_rq' on them), we shouldn't exclude the 
possibility.
Didn't we speak about that in the context of converting libata?

And I have some driver conversions queued (fnic in particular), which 
encapsulate everything into a scsi command.

> It just seems to me that what the block layer is providing is not suitable.
> 
> How about these:
> a. allow block driver to specify size of reserved request PDU separately 
> to regular requests, so we can use something like this for rsvd commands:
> struct scsi_rsvd_cmnd {
>      struct scsi_device *sdev;
> }
> And fix up SCSI iter functions and LLDs to deal with it.

That's what Bart suggested a while back, but then we have to problem 
that the reserved tags are completed with the same interrupt routine, 
and _that_ currently assumes that everything is a scsi command.
Trying to fix up that assumption would mean to audit the midlayer 
(scmd->device is a particular common pattern), _and_ all drivers wanting 
to make use of reserved commands.
For me that's too high an risk to introduce errors; audits are always 
painful and error-prone.

> b. provide block layer API to provide just same as is returned from 
> blk_mq_unique_tag(), but no request is provided. This just gives what we 
> need but would be disruptive in scsi layer and LLDs.

Having looked at the block layer and how tags are allocated I found it 
too deeply interlinked with the request queue and requests in general.
Plus I've suggested that with a previous patchset, which got vetoed by 
Bart as he couldn't use such an API for UFS.

> c. as alternative to b., send all rsvd requests through the block layer, 
> but can be very difficult+disruptive for users
> 
And, indeed, not possible when we will need to send these requests 
during error handling, where the queue might be blocked/frozen/quiesced 
precisely because we are in error handling ...

> *For polling rsvd commands on a poll queue (which we will need for 
> hisi_sas driver and maybe others for poll mode support), we would need 
> to send the request through the block layer, but block layer polling 
> requires a request with a bio, which is a problem.
> 
Allocating a bio is a relatively trivial task. But as soon as we ever 
want to be able to implement polling support for reserved tags we 
essentially _have_ to use requests, and that means we'll have to use the 
provided interfaces from the block layer.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 02/15] scsi: add scsi_{get,put}_internal_cmd() helper

[Hannes Reinecke]

On 11/27/21 12:12 AM, Bart Van Assche wrote:
> On 11/25/21 07:10, Hannes Reinecke wrote:
>> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,
>> +    int data_direction, bool nowait)
> 
> Please use enum dma_data_direction instead of 'int' for the data direction.

I have oriented myself at __scsi_execute(), which also has 
'data_direction' as an integer.
Presumably to avoid header clutter.
Martin?

>> +{
>> +    struct request *rq;
>> +    struct scsi_cmnd *scmd;
>> +    blk_mq_req_flags_t flags = 0;
>> +    int op;
> 
> The name 'op' is confusing since that variable is a bitfield that 
> includes the operation and operation flags. Consider one of the names 
> 'opf', 'op_and_flags' or 'cmd_and_flags'. Please also change the data 
> type from 'int' into 'unsigned int' or 'u32'.
> 
'op' is the name the variable has in blk_mq_alloc_request(), so I prefer 
to stay with that name. Agree with the 'unsigned int', though.

>> +    op = (data_direction == DMA_TO_DEVICE) ?
>> +        REQ_OP_DRV_OUT : REQ_OP_DRV_IN;
> 
> Please leave out the parentheses from the above assignment.
> 
Okay.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 02/15] scsi: add scsi_{get,put}_internal_cmd() helper

[Hannes Reinecke]

On 11/28/21 4:33 AM, Bart Van Assche wrote:
> On 11/25/21 07:10, Hannes Reinecke wrote:
>> Add helper functions to allow LLDDs to allocate and free
>> internal commands.
> 
> Are the changes for the SCSI timeout handler perhaps missing from this 
> patch? In the UFS driver we need the ability not to trigger the SCSI 
> error handler if an internal command times out.
> 
It's based on 5.17/scsi-queue; so if the SCSI timeout handler changes 
are not present in there I won't have it.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 01/15] scsi: allocate host device

[Hannes Reinecke]

On 11/27/21 5:52 PM, Hannes Reinecke wrote:
> On 11/26/21 3:47 AM, chenxiang (M) wrote:
>> Hi Hannes,
>>
>>
>> 在 2021/11/25 23:10, Hannes Reinecke 写道:
>>> Add a flag 'alloc_host_dev' to the SCSI host template and allocate
>>> a virtual scsi device if the flag is set.
>>> This device has the SCSI id <max_id + 1>:0, so won't clash with any
>>> devices the HBA might allocate. It's also excluded from scanning and
>>> will not show up in sysfs.
>>> Intention is to use this device to send internal commands to the HBA.
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> ---
>>>   drivers/scsi/hosts.c       |  8 +++++
>>>   drivers/scsi/scsi_scan.c   | 67 +++++++++++++++++++++++++++++++++++++-
>>>   drivers/scsi/scsi_sysfs.c  |  3 +-
>>>   include/scsi/scsi_device.h |  2 +-
>>>   include/scsi/scsi_host.h   | 21 ++++++++++++
>>>   5 files changed, 98 insertions(+), 3 deletions(-)
>>>
>>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>>> index f69b77cbf538..a539fa2fb221 100644
>>> --- a/drivers/scsi/hosts.c
>>> +++ b/drivers/scsi/hosts.c
>>> @@ -290,6 +290,14 @@ int scsi_add_host_with_dma(struct Scsi_Host 
>>> *shost, struct device *dev,
>>>       if (error)
>>>           goto out_del_dev;
>>> +    if (sht->alloc_host_sdev) {
>>> +        shost->shost_sdev = scsi_get_host_dev(shost);
>>> +        if (!shost->shost_sdev) {
>>> +            error = -ENOMEM;
>>> +            goto out_del_dev;
>>> +        }
>>> +    }
>>> +
>>>       scsi_proc_host_add(shost);
>>>       scsi_autopm_put_host(shost);
>>>       return error;
>>> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
>>> index 328c0e79dfe7..e2910aa02a65 100644
>>> --- a/drivers/scsi/scsi_scan.c
>>> +++ b/drivers/scsi/scsi_scan.c
>>> @@ -1139,6 +1139,12 @@ static int scsi_probe_and_add_lun(struct 
>>> scsi_target *starget,
>>>       if (!sdev)
>>>           goto out;
>>> +    if (scsi_device_is_host_dev(sdev)) {
>>> +        if (bflagsp)
>>> +            *bflagsp = BLIST_NOLUN;
>>> +        return SCSI_SCAN_LUN_PRESENT;
>>> +    }
>>> +
>>>       result = kmalloc(result_len, GFP_KERNEL);
>>>       if (!result)
>>>           goto out_free_sdev;
>>> @@ -1755,6 +1761,9 @@ static void scsi_sysfs_add_devices(struct 
>>> Scsi_Host *shost)
>>>           /* If device is already visible, skip adding it to sysfs */
>>>           if (sdev->is_visible)
>>>               continue;
>>> +        /* Host devices should never be visible in sysfs */
>>> +        if (scsi_device_is_host_dev(sdev))
>>> +            continue;
>>>           if (!scsi_host_scan_allowed(shost) ||
>>>               scsi_sysfs_add_sdev(sdev) != 0)
>>>               __scsi_remove_device(sdev);
>>> @@ -1919,12 +1928,16 @@ EXPORT_SYMBOL(scsi_scan_host);
>>>   void scsi_forget_host(struct Scsi_Host *shost)
>>>   {
>>> -    struct scsi_device *sdev;
>>> +    struct scsi_device *sdev, *host_sdev = NULL;
>>>       unsigned long flags;
>>>    restart:
>>>       spin_lock_irqsave(shost->host_lock, flags);
>>>       list_for_each_entry(sdev, &shost->__devices, siblings) {
>>> +        if (scsi_device_is_host_dev(sdev)) {
>>> +            host_sdev = sdev;
>>> +            continue;
>>> +        }
>>>           if (sdev->sdev_state == SDEV_DEL)
>>>               continue;
>>>           spin_unlock_irqrestore(shost->host_lock, flags);
>>> @@ -1932,5 +1945,57 @@ void scsi_forget_host(struct Scsi_Host *shost)
>>>           goto restart;
>>>       }
>>>       spin_unlock_irqrestore(shost->host_lock, flags);
>>> +    /* Remove host device last, might be needed to send commands */
>>> +    if (host_sdev)
>>> +        __scsi_remove_device(host_sdev);
>>>   }
>>> +/**
>>> + * scsi_get_host_dev - Create a virtual scsi_device to the host adapter
>>> + * @shost: Host that needs a scsi_device
>>> + *
>>> + * Lock status: None assumed.
>>> + *
>>> + * Returns:     The scsi_device or NULL
>>> + *
>>> + * Notes:
>>> + *    Attach a single scsi_device to the Scsi_Host. The primary aim
>>> + *    for this device is to serve as a container from which valid
>>> + *    scsi commands can be allocated from. Each scsi command will carry
>>> + *    an unused/free command tag, which then can be used by the LLDD to
>>> + *    send internal or passthrough commands without having to find a
>>> + *    valid command tag internally.
>>> + */
>>> +struct scsi_device *scsi_get_host_dev(struct Scsi_Host *shost)
>>> +{
>>> +    struct scsi_device *sdev = NULL;
>>> +    struct scsi_target *starget;
>>> +
>>> +    mutex_lock(&shost->scan_mutex);
>>> +    if (!scsi_host_scan_allowed(shost))
>>> +        goto out;
>>> +    starget = scsi_alloc_target(&shost->shost_gendev, 0,
>>> +                    shost->max_id);
>>> +    if (!starget)
>>> +        goto out;
>>> +
>>> +    sdev = scsi_alloc_sdev(starget, 0, NULL);
>>> +    if (sdev)
>>> +        sdev->borken = 0;
>>> +    else
>>> +        scsi_target_reap(starget);
>>
>> Currently many scsi drivers fill some interfaces such as 
>> target_alloc()/slave_alloc() for real disks.
>> When allocating scsi target and scsi device for host dev, it will also 
>> call those interfaces, and not sure whether it breaks those drivers.
>>  From function sas_target_alloc() (common interface in libsas layer), 
>> it seems break it as there is no sas_rphy for host dev.
>>
> Ah. Didn't consider that.
> Will be having a look and fixup the patch.
> 
After giving it some more consideration, I don't think that this is the 
best way to go.

An update to make sas_target_alloc() work correctly would mean a change 
in the SAS topology, as we'd need to create an rphy for the host port, 
and have the host device using that as a parent.
But then this rphy doesn't really exist (as it's the SAS host itself), 
so we would either need to modify libsas to convert the SAS host into 
being able to serve as a port/phy, or add a 'fake' rphy for the SAS 
host. And in either case it would be a bigger surgery.

I'd rather prefer to add checks to libsas to figure out if a given SCSI 
device is a SAS device or a the SCSI host device; John Garry did some 
patches here to libsas.

But anyway, I would first want to concentrate on the API _how_ reserved 
tags should be allocated, and once we have that we can work on porting 
it to more complex things like libsas.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 03/15] scsi: implement reserved command handling

[Asutosh Das (asd)]

On 11/25/2021 7:10 AM, Hannes Reinecke wrote:
> Quite some drivers are using management commands internally, which
> typically use the same hardware tag pool (ie they are being allocated
> from the same hardware resources) as the 'normal' I/O commands.
> These commands are set aside before allocating the block-mq tag bitmap,
> so they'll never show up as busy in the tag map.
> The block-layer, OTOH, already has 'reserved_tags' to handle precisely
> this situation.
> So this patch adds a new field 'nr_reserved_cmds' to the SCSI host
> template to instruct the block layer to set aside a tag space for these
> management commands by using reserved tags.
> 
> Signed-off-by: Hannes Reinecke <hare@suse.de>
> ---
Reviewed-by: Asutosh Das <asutoshd@codeaurora.org>

>   drivers/scsi/hosts.c     |  3 +++
>   drivers/scsi/scsi_lib.c  |  9 ++++++++-
>   include/scsi/scsi_host.h | 22 +++++++++++++++++++++-
>   3 files changed, 32 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index a539fa2fb221..8ee7a7279b6b 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -482,6 +482,9 @@ struct Scsi_Host *scsi_host_alloc(struct scsi_host_template *sht, int privsize)
>   	if (sht->virt_boundary_mask)
>   		shost->virt_boundary_mask = sht->virt_boundary_mask;
>   
> +	if (sht->nr_reserved_cmds)
> +		shost->nr_reserved_cmds = sht->nr_reserved_cmds;
> +
>   	device_initialize(&shost->shost_gendev);
>   	dev_set_name(&shost->shost_gendev, "host%d", shost->host_no);
>   	shost->shost_gendev.bus = &scsi_bus_type;
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index 6fbd36c9c416..e8f1025d0ed8 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1939,7 +1939,9 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
>   		tag_set->ops = &scsi_mq_ops_no_commit;
>   	tag_set->nr_hw_queues = shost->nr_hw_queues ? : 1;
>   	tag_set->nr_maps = shost->nr_maps ? : 1;
> -	tag_set->queue_depth = shost->can_queue;
> +	tag_set->queue_depth =
> +		shost->can_queue + shost->nr_reserved_cmds;
> +	tag_set->reserved_tags = shost->nr_reserved_cmds;
>   	tag_set->cmd_size = cmd_size;
>   	tag_set->numa_node = NUMA_NO_NODE;
>   	tag_set->flags = BLK_MQ_F_SHOULD_MERGE;
> @@ -1964,6 +1966,9 @@ void scsi_mq_destroy_tags(struct Scsi_Host *shost)
>    * @nowait: do not wait for command allocation to succeed.
>    *
>    * Allocates a SCSI command for internal LLDD use.
> + * If 'nr_reserved_commands' is spectified by the host the
> + * command will be allocated from the reserved tag pool;
> + * otherwise the normal tag pool will be used.
>    */
>   struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,
>   	int data_direction, bool nowait)
> @@ -1973,6 +1978,8 @@ struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,
>   	blk_mq_req_flags_t flags = 0;
>   	int op;
>   
> +	if (sdev->host->nr_reserved_cmds)
> +		flags |= BLK_MQ_REQ_RESERVED;
>   	if (nowait)
>   		flags |= BLK_MQ_REQ_NOWAIT;
>   	op = (data_direction == DMA_TO_DEVICE) ?
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 6f49a8940dc4..7512d97aceb4 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -367,10 +367,19 @@ struct scsi_host_template {
>   	/*
>   	 * This determines if we will use a non-interrupt driven
>   	 * or an interrupt driven scheme.  It is set to the maximum number
> -	 * of simultaneous commands a single hw queue in HBA will accept.
> +	 * of simultaneous commands a single hw queue in HBA will accept
> +	 * excluding internal commands.
>   	 */
>   	int can_queue;
>   
> +	/*
> +	 * This determines how many commands the HBA will set aside
> +	 * for internal commands. This number will be added to
> +	 * @can_queue to calcumate the maximum number of simultaneous
> +	 * commands sent to the host.
> +	 */
> +	int nr_reserved_cmds;
> +
>   	/*
>   	 * In many instances, especially where disconnect / reconnect are
>   	 * supported, our host also has an ID on the SCSI bus.  If this is
> @@ -608,6 +617,11 @@ struct Scsi_Host {
>   	unsigned short max_cmd_len;
>   
>   	int this_id;
> +
> +	/*
> +	 * Number of commands this host can handle at the same time.
> +	 * This excludes reserved commands as specified by nr_reserved_cmds.
> +	 */
>   	int can_queue;
>   	short cmd_per_lun;
>   	short unsigned int sg_tablesize;
> @@ -626,6 +640,12 @@ struct Scsi_Host {
>   	 */
>   	unsigned nr_hw_queues;
>   	unsigned nr_maps;
> +
> +	/*
> +	 * Number of reserved commands to allocate, if any.
> +	 */
> +	unsigned nr_reserved_cmds;
> +
>   	unsigned active_mode:2;
>   
>   	/*
> 


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
Linux Foundation Collaborative Project

Re: [PATCH 02/15] scsi: add scsi_{get,put}_internal_cmd() helper

[Martin K. Petersen]

Hannes,

> I have oriented myself at __scsi_execute(), which also has
> 'data_direction' as an integer.  Presumably to avoid header clutter.
> Martin?

Just a vestige from ancient times. I really hate scsi_execute() and its
10,000 randomly ordered arguments. The more sanity checking we have in
that department, the better.

At some point I proposed having scsi_execute() take a single struct as
argument to get better input validation. I've lost count how many things
have been broken because of misordered arguments to this function.
Backporting patches almost inevitably causes regressions because of this
interface.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 02/15] scsi: add scsi_{get,put}_internal_cmd() helper

[Hannes Reinecke]

On 11/30/21 5:17 AM, Martin K. Petersen wrote:
> 
> Hannes,
> 
>> I have oriented myself at __scsi_execute(), which also has
>> 'data_direction' as an integer.  Presumably to avoid header clutter.
>> Martin?
> 
> Just a vestige from ancient times. I really hate scsi_execute() and its
> 10,000 randomly ordered arguments. The more sanity checking we have in
> that department, the better.
> 
> At some point I proposed having scsi_execute() take a single struct as
> argument to get better input validation. I've lost count how many things
> have been broken because of misordered arguments to this function.
> Backporting patches almost inevitably causes regressions because of this
> interface.
> 
Right. As it so happens, I've already created a patch to include 
<linux/dma-direction.h> here.
But yeah, the arguments to __scsi_execute are patently horrible.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 02/15] scsi: add scsi_{get,put}_internal_cmd() helper

[John Garry]

On 28/11/2021 10:36, Hannes Reinecke wrote:
>>>   * Allocates a SCSI command for internal LLDD use.
>>> + */
>>> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,
>>> +    int data_direction, bool nowait)
>>> +{
>>> +    struct request *rq;
>>> +    struct scsi_cmnd *scmd;
>>> +    blk_mq_req_flags_t flags = 0;
>>> +    int op;
>>> +
>>> +    if (nowait)
>>> +        flags |= BLK_MQ_REQ_NOWAIT;
>>> +    op = (data_direction == DMA_TO_DEVICE) ?
>>> +        REQ_OP_DRV_OUT : REQ_OP_DRV_IN;
>>> +    rq = blk_mq_alloc_request(sdev->request_queue, op, flags);
>>> +    if (IS_ERR(rq))
>>> +        return NULL;
>>> +    scmd = blk_mq_rq_to_pdu(rq);
>>> +    scmd->device = sdev;
>>> +    return scmd;
>>> +}
>>> +EXPORT_SYMBOL_GPL(scsi_get_internal_cmd);
>> So there are a couple of generally-accepted grievances about this approach:
>> a. we're being allocated a scsi_cmnd, but not using what is being
>> allocated as a scsi_cmnd, but rather just a holder as a reference to an
>> allocated tag
>> b. we're being allocated a request, which is not being sent through the
>> block layer*
>>
> And while being true in general, it does make some assumptions:
> - Reserved commands will never being sent via the block layer
> - None of the drivers will need to use the additional 'scsi_cmnd' payload.
> 
> Here I'm not sure if this is true in general.
> While it doesn't seem to be necessary to send reserved commands via the
> block layer (ie calling 'queue_rq' on them), we shouldn't exclude the
> possibility.

Agreed

> Didn't we speak about that in the context of converting libata?
> 

We did discuss libata before, but I'm not sure on the context you mean.

One thing that I know is that libata-core has "internal" commands in 
ata_exec_internal(). I could not see how that function could be 
converted to use queue_rq. The problem is that it calls ->qc_issue() 
with IRQs disabled, which is not permitted for calling blk_execute_rq() 
instead.

> And I have some driver conversions queued (fnic in particular), which
> encapsulate everything into a scsi command.
> 
>> It just seems to me that what the block layer is providing is not suitable.
>>
>> How about these:
>> a. allow block driver to specify size of reserved request PDU separately
>> to regular requests, so we can use something like this for rsvd commands:
>> struct scsi_rsvd_cmnd {
>>       struct scsi_device *sdev;
>> }
>> And fix up SCSI iter functions and LLDs to deal with it.
> That's what Bart suggested a while back, 

I don't recall that one.

> but then we have to problem
> that the reserved tags are completed with the same interrupt routine,
> and_that_  currently assumes that everything is a scsi command.

I think that any driver which uses reserved commands needs to be thought 
that not all commands are "real" scsi commands, i.e. we don't call 
scsi_done() in the LLD ISR always. As such, they should be able to deal 
with something like struct scsi_rsvd_cmnd.

BTW, for this current series, please ensure that we can't call 
scsi_host_complete_all_commands() which could iter reserved tags, as we 
call stuff like scsi_done() there. I don't think it's an issue here, but 
just thought that it was worth mentioning.

> Trying to fix up that assumption would mean to audit the midlayer
> (scmd->device is a particular common pattern),_and_  all drivers wanting
> to make use of reserved commands.
> For me that's too high an risk to introduce errors; audits are always
> painful and error-prone.
> 
>> b. provide block layer API to provide just same as is returned from
>> blk_mq_unique_tag(), but no request is provided. This just gives what we
>> need but would be disruptive in scsi layer and LLDs.
> Having looked at the block layer and how tags are allocated I found it
> too deeply interlinked with the request queue and requests in general.

They are indeed interlinked in the block layer, but we don't need expose 
requests or anything else.

Such an interface could just be a wrapper for 
blk_mq_alloc_request()+_start_request().

> Plus I've suggested that with a previous patchset, which got vetoed by
> Bart as he couldn't use such an API for UFS.
>  >> c. as alternative to b., send all rsvd requests through the block layer,
>> but can be very difficult+disruptive for users
>>
> And, indeed, not possible when we will need to send these requests
> during error handling, where the queue might be blocked/frozen/quiesced
> precisely because we are in error handling ...

If we send for the host request queue, would it ever be 
blocked/frozen/quiesced?

> 
>> *For polling rsvd commands on a poll queue (which we will need for
>> hisi_sas driver and maybe others for poll mode support), we would need
>> to send the request through the block layer, but block layer polling
>> requires a request with a bio, which is a problem.
>>
> Allocating a bio is a relatively trivial task.

So do you suggest a dummy bio for that request? I hacked something 
locally to get it to work as a PoC, but no idea on a real solution yet.

> But as soon as we ever
> want to be able to implement polling support for reserved tags we
> essentially_have_  to use requests, 

Agreed

> and that means we'll have to use the
> provided interfaces from the block layer.

Thanks,
John

Re: [PATCH 02/15] scsi: add scsi_{get,put}_internal_cmd() helper

[Hannes Reinecke]

On 12/6/21 6:15 PM, John Garry wrote:
> On 28/11/2021 10:36, Hannes Reinecke wrote:
>>>>   * Allocates a SCSI command for internal LLDD use.
>>>> + */
>>>> +struct scsi_cmnd *scsi_get_internal_cmd(struct scsi_device *sdev,
>>>> +    int data_direction, bool nowait)
>>>> +{
>>>> +    struct request *rq;
>>>> +    struct scsi_cmnd *scmd;
>>>> +    blk_mq_req_flags_t flags = 0;
>>>> +    int op;
>>>> +
>>>> +    if (nowait)
>>>> +        flags |= BLK_MQ_REQ_NOWAIT;
>>>> +    op = (data_direction == DMA_TO_DEVICE) ?
>>>> +        REQ_OP_DRV_OUT : REQ_OP_DRV_IN;
>>>> +    rq = blk_mq_alloc_request(sdev->request_queue, op, flags);
>>>> +    if (IS_ERR(rq))
>>>> +        return NULL;
>>>> +    scmd = blk_mq_rq_to_pdu(rq);
>>>> +    scmd->device = sdev;
>>>> +    return scmd;
>>>> +}
>>>> +EXPORT_SYMBOL_GPL(scsi_get_internal_cmd);
>>> So there are a couple of generally-accepted grievances about this 
>>> approach:
>>> a. we're being allocated a scsi_cmnd, but not using what is being
>>> allocated as a scsi_cmnd, but rather just a holder as a reference to an
>>> allocated tag
>>> b. we're being allocated a request, which is not being sent through the
>>> block layer*
>>>
>> And while being true in general, it does make some assumptions:
>> - Reserved commands will never being sent via the block layer
>> - None of the drivers will need to use the additional 'scsi_cmnd' 
>> payload.
>>
>> Here I'm not sure if this is true in general.
>> While it doesn't seem to be necessary to send reserved commands via the
>> block layer (ie calling 'queue_rq' on them), we shouldn't exclude the
>> possibility.
> 
> Agreed
> 
>> Didn't we speak about that in the context of converting libata?
>>
> 
> We did discuss libata before, but I'm not sure on the context you mean.
> 
> One thing that I know is that libata-core has "internal" commands in 
> ata_exec_internal(). I could not see how that function could be 
> converted to use queue_rq. The problem is that it calls ->qc_issue() 
> with IRQs disabled, which is not permitted for calling blk_execute_rq() 
> instead.
> 

We're conflating two issues here.

The one issue is to use 'reserved' tags (as defined by the block layer) 
to allocate commands which are internal within some drivers (like hpsa 
etc). That is what my patchset does.
As these commands are internal within specific drivers these commands 
will never show up in the upper layers, precisely because they are internal.

The other issue is to allow these 'reserved' tags (and the attached 
requests/commands) to be routed via the normal block-layer execution 
path. This is _not_ part of the above patchset, as that just deals with 
driver internal commands and will never execute ->queue_rq.
For that we would need an additional patchset, on top of the first one.

>> And I have some driver conversions queued (fnic in particular), which
>> encapsulate everything into a scsi command.
>>
>>> It just seems to me that what the block layer is providing is not 
>>> suitable.
>>>
>>> How about these:
>>> a. allow block driver to specify size of reserved request PDU separately
>>> to regular requests, so we can use something like this for rsvd 
>>> commands:
>>> struct scsi_rsvd_cmnd {
>>>       struct scsi_device *sdev;
>>> }
>>> And fix up SCSI iter functions and LLDs to deal with it.
>> That's what Bart suggested a while back, 
> 
> I don't recall that one.
> 
>> but then we have to problem
>> that the reserved tags are completed with the same interrupt routine,
>> and_that_  currently assumes that everything is a scsi command.
> 
> I think that any driver which uses reserved commands needs to be thought 
> that not all commands are "real" scsi commands, i.e. we don't call 
> scsi_done() in the LLD ISR always. As such, they should be able to deal 
> with something like struct scsi_rsvd_cmnd.
> 
See above. My patchset is strictly for driver internal commands, which 
will never be send nor completed like 'real' commands.
And the main point (well, the 'other' main point aside from not having 
to allocate a tag yourself) is that the driver can be _simplified_, as 
now every tag references to the _same_ structure.
If we start using different structure we'll lose that ability 
completely, and really haven't gained that much.

> BTW, for this current series, please ensure that we can't call 
> scsi_host_complete_all_commands() which could iter reserved tags, as we 
> call stuff like scsi_done() there. I don't think it's an issue here, but 
> just thought that it was worth mentioning.
> 
See above. These are driver internal commands, for which scsi_done() is 
never called.

I deliberately did _not_ add checks to scsi_done() or queue_rq(), as 
there presumably will be an additional patch which allows precisely 
this, eg when converting libsas.

>> Trying to fix up that assumption would mean to audit the midlayer
>> (scmd->device is a particular common pattern),_and_  all drivers wanting
>> to make use of reserved commands.
>> For me that's too high an risk to introduce errors; audits are always
>> painful and error-prone.
>>
>>> b. provide block layer API to provide just same as is returned from
>>> blk_mq_unique_tag(), but no request is provided. This just gives what we
>>> need but would be disruptive in scsi layer and LLDs.
>> Having looked at the block layer and how tags are allocated I found it
>> too deeply interlinked with the request queue and requests in general.
> 
> They are indeed interlinked in the block layer, but we don't need expose 
> requests or anything else.
> 
Correct. And this is one of the drawbacks of this approach, in that 
we're always allocating a 'struct request' and a 'struct scsi_cmnd' 
payload even if we don't actually use them.
But then again, we _currently_ don't use them.
If we ever want to sent these 'reserved' commands via queue_rqs() and be 
able to call 'scsi_done()' on them (again, the libsas case) then we need 
these payloads.

> Such an interface could just be a wrapper for 
> blk_mq_alloc_request()+_start_request().
> 
I do agree that currently I don't start requests, and consequently they 
won't show up in any iterators.
But then (currently) it doesn't matter, as none of the iterators in any 
of the drivers I've been converting needed to look at those requests.

>> Plus I've suggested that with a previous patchset, which got vetoed by
>> Bart as he couldn't use such an API for UFS.
>>  >> c. as alternative to b., send all rsvd requests through the block 
>> layer,
>>> but can be very difficult+disruptive for users
>>>
>> And, indeed, not possible when we will need to send these requests
>> during error handling, where the queue might be blocked/frozen/quiesced
>> precisely because we are in error handling ...
> 
> If we send for the host request queue, would it ever be 
> blocked/frozen/quiesced?
> 
Possibly not. But be aware that 'reserved' tags is actually independent 
on the host request queue; it's perfectly possible to use 'reserved' 
tags without a host request queue. Again, fnic is such an example.

>>
>>> *For polling rsvd commands on a poll queue (which we will need for
>>> hisi_sas driver and maybe others for poll mode support), we would need
>>> to send the request through the block layer, but block layer polling
>>> requires a request with a bio, which is a problem.
>>>
>> Allocating a bio is a relatively trivial task.
> 
> So do you suggest a dummy bio for that request? I hacked something 
> locally to get it to work as a PoC, but no idea on a real solution yet.
> 
We're allocating bios for all kind of purposes, even 'dummy' bios in the 
case of bio_clone() or bio_split(). So that's nothing new, and should be 
relatively easy.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke                Kernel Storage Architect
hare@suse.de                              +49 911 74053 688
SUSE Software Solutions GmbH, Maxfeldstr. 5, 90409 Nürnberg
HRB 36809 (AG Nürnberg), Geschäftsführer: Felix Imendörffer

Re: [PATCH 02/15] scsi: add scsi_{get,put}_internal_cmd() helper

[John Garry]

On 06/12/2021 17:46, Hannes Reinecke wrote:
>> We did discuss libata before, but I'm not sure on the context you mean.
>>
>> One thing that I know is that libata-core has "internal" commands in 
>> ata_exec_internal(). I could not see how that function could be 
>> converted to use queue_rq. The problem is that it calls ->qc_issue() 
>> with IRQs disabled, which is not permitted for calling 
>> blk_execute_rq() instead.
>>
> 
> We're conflating two issues here.

I'm just trying to see the final picture, and not just this first step,
please

> 
> The one issue is to use 'reserved' tags (as defined by the block layer) 
> to allocate commands which are internal within some drivers (like hpsa 
> etc). That is what my patchset does.
> As these commands are internal within specific drivers these commands 
> will never show up in the upper layers, precisely because they are 
> internal.
> 
> The other issue is to allow these 'reserved' tags (and the attached 
> requests/commands) to be routed via the normal block-layer execution 
> path. This is _not_ part of the above patchset, as that just deals with 
> driver internal commands and will never execute ->queue_rq.
> For that we would need an additional patchset, on top of the first one.
> 
>>> And I have some driver conversions queued (fnic in particular), which
>>> encapsulate everything into a scsi command.
>>>
>>>> It just seems to me that what the block layer is providing is not 
>>>> suitable.
>>>>
>>>> How about these:
>>>> a. allow block driver to specify size of reserved request PDU 
>>>> separately
>>>> to regular requests, so we can use something like this for rsvd 
>>>> commands:
>>>> struct scsi_rsvd_cmnd {
>>>>       struct scsi_device *sdev;
>>>> }
>>>> And fix up SCSI iter functions and LLDs to deal with it.
>>> That's what Bart suggested a while back, 
>>
>> I don't recall that one.
>>
>>> but then we have to problem
>>> that the reserved tags are completed with the same interrupt routine,
>>> and_that_  currently assumes that everything is a scsi command.
>>
>> I think that any driver which uses reserved commands needs to be 
>> thought that not all commands are "real" scsi commands, i.e. we don't 
>> call scsi_done() in the LLD ISR always. As such, they should be able 
>> to deal with something like struct scsi_rsvd_cmnd.
>>
> See above. My patchset is strictly for driver internal commands, which 
> will never be send nor completed like 'real' commands.
> And the main point (well, the 'other' main point aside from not having 
> to allocate a tag yourself) is that the driver can be _simplified_, as 
> now every tag references to the _same_ structure.

Sure, but I like the distinction of a separate struct scsi_rsvd_cmnd,
even if it is at the cost of some simplification.

> If we start using different structure we'll lose that ability 
> completely, and really haven't gained that much.
> 
>> BTW, for this current series, please ensure that we can't call 
>> scsi_host_complete_all_commands() which could iter reserved tags, as 
>> we call stuff like scsi_done() there. I don't think it's an issue 
>> here, but just thought that it was worth mentioning.
>>
> See above. These are driver internal commands, for which scsi_done() is 
> never called.

Right, but I am just saying that we need to be careful that it will not be
called possibly in the future. Ignore it for now.

> 
> I deliberately did _not_ add checks to scsi_done() or queue_rq(), as 
> there presumably will be an additional patch which allows precisely 
> this, eg when converting libsas.
> 
>>> Trying to fix up that assumption would mean to audit the midlayer
>>> (scmd->device is a particular common pattern),_and_  all drivers wanting
>>> to make use of reserved commands.
>>> For me that's too high an risk to introduce errors; audits are always
>>> painful and error-prone.
>>>
>>>> b. provide block layer API to provide just same as is returned from
>>>> blk_mq_unique_tag(), but no request is provided. This just gives 
>>>> what we
>>>> need but would be disruptive in scsi layer and LLDs.
>>> Having looked at the block layer and how tags are allocated I found it
>>> too deeply interlinked with the request queue and requests in general.
>>
>> They are indeed interlinked in the block layer, but we don't need 
>> expose requests or anything else.
>>
> Correct. And this is one of the drawbacks of this approach, in that 
> we're always allocating a 'struct request' and a 'struct scsi_cmnd' 
> payload even if we don't actually use them.
> But then again, we _currently_ don't use them.
> If we ever want to sent these 'reserved' commands via queue_rqs() and be 
> able to call 'scsi_done()' on them (again, the libsas case) then we need 
> these payloads.
> 
>> Such an interface could just be a wrapper for 
>> blk_mq_alloc_request()+_start_request().
>>
> I do agree that currently I don't start requests, and consequently they 
> won't show up in any iterators.
> But then (currently) it doesn't matter, as none of the iterators in any 
> of the drivers I've been converting needed to look at those requests.

ok, fine

> 
>>> Plus I've suggested that with a previous patchset, which got vetoed by
>>> Bart as he couldn't use such an API for UFS.
>>>  >> c. as alternative to b., send all rsvd requests through the block 
>>> layer,
>>>> but can be very difficult+disruptive for users
>>>>
>>> And, indeed, not possible when we will need to send these requests
>>> during error handling, where the queue might be blocked/frozen/quiesced
>>> precisely because we are in error handling ...
>>
>> If we send for the host request queue, would it ever be 
>> blocked/frozen/quiesced?
>>
> Possibly not. But be aware that 'reserved' tags is actually independent 
> on the host request queue; it's perfectly possible to use 'reserved' 
> tags without a host request queue. Again, fnic is such an example.

So considering a scsi_device request queue may be blocked/frozen/quiesced,
how should we decide which request queue to use when allocating a
reserved command for some error handling IO? I assume that request
allocation/queuing is failed or blocked in this mentioned scenario for the
sdev request queue.

> 
>>>
>>>> *For polling rsvd commands on a poll queue (which we will need for
>>>> hisi_sas driver and maybe others for poll mode support), we would need
>>>> to send the request through the block layer, but block layer polling
>>>> requires a request with a bio, which is a problem.
>>>>
>>> Allocating a bio is a relatively trivial task.
>>
>> So do you suggest a dummy bio for that request? I hacked something 
>> locally to get it to work as a PoC, but no idea on a real solution yet.
>>
> We're allocating bios for all kind of purposes, even 'dummy' bios in the 
> case of bio_clone() or bio_split(). So that's nothing new, and should be 
> relatively easy.

But these APIs need some bio to begin with, right? Where does that come
from?

Thanks,
John