[PATCH 0/2] scsi: scsi_debug: Support hostwide tags and a fix

[PATCH 0/2] scsi: scsi_debug: Support hostwide tags and a fix

[John Garry]

This series includes hostwide tags support, so we can mimic some SCSI HBAs,
like megaraid sas or hisi_sas.

There is also a fix for an out-of-range module param.

John Garry (2):
  scsi: scsi_debug: Add check for sdebug_max_queue during module init
  scsi: scsi_debug: Support hostwide tags

 drivers/scsi/scsi_debug.c | 79 +++++++++++++++++++++++++++++++++------
 1 file changed, 67 insertions(+), 12 deletions(-)

-- 
2.26.2

[PATCH 1/2] scsi: scsi_debug: Add check for sdebug_max_queue during module init

[John Garry]

sdebug_max_queue should not exceed SDEBUG_CANQUEUE, otherwise crashes like
this can be triggered by passing an out-of-range value:

Hardware name: Huawei D06 /D06, BIOS Hisilicon D06 UEFI RC0 - V1.16.01 03/15/2019 
 pstate: 20400009 (nzCv daif +PAN -UAO BTYPE=--) 
 pc : schedule_resp+0x2a4/0xa70 [scsi_debug] 
 lr : schedule_resp+0x52c/0xa70 [scsi_debug] 
 sp : ffff800022ab36f0 
 x29: ffff800022ab36f0 x28: ffff0023a935a610 
 x27: ffff800008e0a648 x26: 0000000000000003 
 x25: ffff0023e84f3200 x24: 00000000003d0900 
 x23: 0000000000000000 x22: 0000000000000000 
 x21: ffff0023be60a320 x20: ffff0023be60b538 
 x19: ffff800008e13000 x18: 0000000000000000 
 x17: 0000000000000000 x16: 0000000000000000 
 x15: 0000000000000000 x14: 0000000000000000 
 x13: 0000000000000000 x12: 0000000000000000 
 x11: 0000000000000000 x10: 0000000000000000 
 x9 : 0000000000000001 x8 : 0000000000000000 
 x7 : 0000000000000000 x6 : 00000000000000c1 
 x5 : 0000020000200000 x4 : dead0000000000ff 
 x3 : 0000000000000200 x2 : 0000000000000200 
 x1 : ffff800008e13d88 x0 : 0000000000000000 
 Call trace: 
schedule_resp+0x2a4/0xa70 [scsi_debug] 
scsi_debug_queuecommand+0x2c4/0x9e0 [scsi_debug] 
scsi_queue_rq+0x698/0x840
__blk_mq_try_issue_directly+0x108/0x228
blk_mq_request_issue_directly+0x58/0x98
blk_mq_try_issue_list_directly+0x5c/0xf0 
blk_mq_sched_insert_requests+0x18c/0x200 
blk_mq_flush_plug_list+0x11c/0x190 
blk_flush_plug_list+0xdc/0x110 
blk_finish_plug+0x38/0x210 
blkdev_direct_IO+0x450/0x4d8 
generic_file_read_iter+0x84/0x180
blkdev_read_iter+0x3c/0x50 
aio_read+0xc0/0x170
io_submit_one+0x5c8/0xc98
__arm64_sys_io_submit+0x1b0/0x258
el0_svc_common.constprop.3+0x68/0x170
do_el0_svc+0x24/0x90 
el0_sync_handler+0x13c/0x1a8 
el0_sync+0x158/0x180 
 Code: 528847e0 72a001e0 6b00003f 540018cd (3941c340)

In addition, it should not be less than 1.

So add checks for these, and fail the module init for those cases.

Fixes: c483739430f10 ("scsi_debug: add multiple queue support")
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/scsi_debug.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 4692f5b6ad13..68534a23866e 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -6613,6 +6613,12 @@ static int __init scsi_debug_init(void)
 		pr_err("submit_queues must be 1 or more\n");
 		return -EINVAL;
 	}
+
+	if ((sdebug_max_queue > SDEBUG_CANQUEUE) || (sdebug_max_queue <= 0)) {
+		pr_err("max_queue must be in range [1, %d]\n", SDEBUG_CANQUEUE);
+		return -EINVAL;
+	}
+
 	sdebug_q_arr = kcalloc(submit_queues, sizeof(struct sdebug_queue),
 			       GFP_KERNEL);
 	if (sdebug_q_arr == NULL)
-- 
2.26.2

[PATCH 2/2] scsi: scsi_debug: Support hostwide tags

[John Garry]

Many SCSI HBAs support a hostwide tagset, whereby each command submitted
to the HW from all submission queues must have a unique tag identifier.

Normally this unique tag will be in the range [0, max queue), where "max
queue" is the depth of each of the submission queues.

Add support for this hostwide tag feature, via module parameter
"host_max_queue". A non-zero value means that the feature is enabled. In
this case, the submission queues are not exposed to upper layer, i.e. from
blk-mq perspective, the device has a single hw queue. There are 2 reasons
for this:
a. it is assumed that the host can support nr_hw_queues * can_queue
   commands, but this is not true for hostwide tags
b. for nr_hw_queues != 0, the request tag is not unique over all HW queues,
   and some HBA drivers want to use this tag for the hostwide tag

However, like many SCSI HBA drivers today - megaraid sas being an example -
the full set of HW submission queues are still used in the LLDD driver. So
instead of using a complicated "reply_map" to create a per-CPU submission
queue mapping like megaraid sas (as it depends on a PCI device + MSIs) -
use a simple algorithm:

hwq = cpu % queue count

If the host max queue param is less than the max queue depth param, then
the max queue depth param is clipped.

If and when hostwide shared tags are supported in blk-mq/scsi mid-layer,
then the policy to set nr_hw_queues = 0 for hostwide tags can be revised.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/scsi_debug.c | 73 ++++++++++++++++++++++++++++++++-------
 1 file changed, 61 insertions(+), 12 deletions(-)

diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
index 68534a23866e..a56d5ee9f4d7 100644
--- a/drivers/scsi/scsi_debug.c
+++ b/drivers/scsi/scsi_debug.c
@@ -344,6 +344,7 @@ struct sdebug_defer {
 	struct execute_work ew;
 	int sqa_idx;	/* index of sdebug_queue array */
 	int qc_idx;	/* index of sdebug_queued_cmd array within sqa_idx */
+	int hc_idx;	/* hostwide tag index */
 	int issuing_cpu;
 	bool init_hrt;
 	bool init_wq;
@@ -762,6 +763,7 @@ static unsigned int sdebug_guard = DEF_GUARD;
 static int sdebug_lowest_aligned = DEF_LOWEST_ALIGNED;
 static int sdebug_max_luns = DEF_MAX_LUNS;
 static int sdebug_max_queue = SDEBUG_CANQUEUE;	/* per submit queue */
+static int sdebug_host_max_queue;	/* per host */
 static unsigned int sdebug_medium_error_start = OPT_MEDIUM_ERR_ADDR;
 static int sdebug_medium_error_count = OPT_MEDIUM_ERR_NUM;
 static atomic_t retired_max_queue;	/* if > 0 then was prior max_queue */
@@ -4707,15 +4709,28 @@ static int resp_rwp_zone(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
 
 static struct sdebug_queue *get_queue(struct scsi_cmnd *cmnd)
 {
-	u32 tag = blk_mq_unique_tag(cmnd->request);
-	u16 hwq = blk_mq_unique_tag_to_hwq(tag);
+	u16 hwq;
 
-	pr_debug("tag=%#x, hwq=%d\n", tag, hwq);
-	if (WARN_ON_ONCE(hwq >= submit_queues))
-		hwq = 0;
+	if (sdebug_host_max_queue) {
+		/* Provide a simple method to choose the hwq */
+		hwq = smp_processor_id() % submit_queues;
+	} else {
+		u32 tag = blk_mq_unique_tag(cmnd->request);
+
+		hwq = blk_mq_unique_tag_to_hwq(tag);
+
+		pr_debug("tag=%#x, hwq=%d\n", tag, hwq);
+		if (WARN_ON_ONCE(hwq >= submit_queues))
+			hwq = 0;
+	}
 	return sdebug_q_arr + hwq;
 }
 
+static u32 get_tag(struct scsi_cmnd *cmnd)
+{
+	return blk_mq_unique_tag(cmnd->request);
+}
+
 /* Queued (deferred) command completions converge here. */
 static void sdebug_q_cmd_complete(struct sdebug_defer *sd_dp)
 {
@@ -4747,8 +4762,8 @@ static void sdebug_q_cmd_complete(struct sdebug_defer *sd_dp)
 	scp = sqcp->a_cmnd;
 	if (unlikely(scp == NULL)) {
 		spin_unlock_irqrestore(&sqp->qc_lock, iflags);
-		pr_err("scp is NULL, sqa_idx=%d, qc_idx=%d\n",
-		       sd_dp->sqa_idx, qc_idx);
+		pr_err("scp is NULL, sqa_idx=%d, qc_idx=%d, hc_idx=%d\n",
+		       sd_dp->sqa_idx, qc_idx, sd_dp->hc_idx);
 		return;
 	}
 	devip = (struct sdebug_dev_info *)scp->device->hostdata;
@@ -5451,6 +5466,10 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
 		new_sd_dp = false;
 	}
 
+	/* Set the hostwide tag */
+	if (sdebug_host_max_queue)
+		sd_dp->hc_idx = get_tag(cmnd);
+
 	if (ndelay > 0 && ndelay < INCLUSIVE_TIMING_MAX_NS)
 		ns_from_boot = ktime_get_boottime_ns();
 
@@ -5585,6 +5604,8 @@ module_param_named(lbpws10, sdebug_lbpws10, int, S_IRUGO);
 module_param_named(lowest_aligned, sdebug_lowest_aligned, int, S_IRUGO);
 module_param_named(max_luns, sdebug_max_luns, int, S_IRUGO | S_IWUSR);
 module_param_named(max_queue, sdebug_max_queue, int, S_IRUGO | S_IWUSR);
+module_param_named(host_max_queue, sdebug_host_max_queue, int,
+		   S_IRUGO | S_IWUSR);
 module_param_named(medium_error_count, sdebug_medium_error_count, int,
 		   S_IRUGO | S_IWUSR);
 module_param_named(medium_error_start, sdebug_medium_error_start, int,
@@ -5654,6 +5675,7 @@ MODULE_PARM_DESC(lbpws10, "enable LBP, support WRITE SAME(10) with UNMAP bit (de
 MODULE_PARM_DESC(lowest_aligned, "lowest aligned lba (def=0)");
 MODULE_PARM_DESC(max_luns, "number of LUNs per target to simulate(def=1)");
 MODULE_PARM_DESC(max_queue, "max number of queued commands (1 to max(def))");
+MODULE_PARM_DESC(host_max_queue, "max number of queued commands per host (0 to max(def))");
 MODULE_PARM_DESC(medium_error_count, "count of sectors to return follow on MEDIUM error");
 MODULE_PARM_DESC(medium_error_start, "starting sector number to return MEDIUM error");
 MODULE_PARM_DESC(ndelay, "response delay in nanoseconds (def=0 -> ignore)");
@@ -6141,7 +6163,8 @@ static ssize_t max_queue_store(struct device_driver *ddp, const char *buf,
 	struct sdebug_queue *sqp;
 
 	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n > 0) &&
-	    (n <= SDEBUG_CANQUEUE)) {
+	    (n <= SDEBUG_CANQUEUE) &&
+	    (sdebug_host_max_queue ? sdebug_host_max_queue >= n : 1)) {
 		block_unblock_all_queues(true);
 		k = 0;
 		for (j = 0, sqp = sdebug_q_arr; j < submit_queues;
@@ -6164,6 +6187,17 @@ static ssize_t max_queue_store(struct device_driver *ddp, const char *buf,
 }
 static DRIVER_ATTR_RW(max_queue);
 
+static ssize_t host_max_queue_show(struct device_driver *ddp, char *buf)
+{
+	return scnprintf(buf, PAGE_SIZE, "%d\n", sdebug_host_max_queue);
+}
+
+/*
+ * Since this is used for .can_queue, and we get the hc_idx tag from the bitmap
+ * in range [0, sdebug_host_max_queue), we can't change it.
+ */
+static DRIVER_ATTR_RO(host_max_queue);
+
 static ssize_t no_uld_show(struct device_driver *ddp, char *buf)
 {
 	return scnprintf(buf, PAGE_SIZE, "%d\n", sdebug_no_uld);
@@ -6510,6 +6544,7 @@ static struct attribute *sdebug_drv_attrs[] = {
 	&driver_attr_every_nth.attr,
 	&driver_attr_max_luns.attr,
 	&driver_attr_max_queue.attr,
+	&driver_attr_host_max_queue.attr,
 	&driver_attr_no_uld.attr,
 	&driver_attr_scsi_level.attr,
 	&driver_attr_virtual_gb.attr,
@@ -6619,6 +6654,13 @@ static int __init scsi_debug_init(void)
 		return -EINVAL;
 	}
 
+	if (sdebug_host_max_queue &&
+	    sdebug_max_queue > sdebug_host_max_queue) {
+		sdebug_max_queue = sdebug_host_max_queue;
+		pr_warn("reducing max submit queue depth to host max queue depth, %d\n",
+			sdebug_max_queue);
+	}
+
 	sdebug_q_arr = kcalloc(submit_queues, sizeof(struct sdebug_queue),
 			       GFP_KERNEL);
 	if (sdebug_q_arr == NULL)
@@ -7257,7 +7299,10 @@ static int sdebug_driver_probe(struct device *dev)
 
 	sdbg_host = to_sdebug_host(dev);
 
-	sdebug_driver_template.can_queue = sdebug_max_queue;
+	if (sdebug_host_max_queue)
+		sdebug_driver_template.can_queue = sdebug_host_max_queue;
+	else
+		sdebug_driver_template.can_queue = sdebug_max_queue;
 	if (!sdebug_clustering)
 		sdebug_driver_template.dma_boundary = PAGE_SIZE - 1;
 
@@ -7272,9 +7317,13 @@ static int sdebug_driver_probe(struct device *dev)
 			my_name, submit_queues, nr_cpu_ids);
 		submit_queues = nr_cpu_ids;
 	}
-	/* Decide whether to tell scsi subsystem that we want mq */
-	/* Following should give the same answer for each host */
-	hpnt->nr_hw_queues = submit_queues;
+	/*
+	 * Decide whether to tell scsi subsystem that we want mq. The
+	 * following should give the same answer for each host. If the host
+	 * has a limit of hostwide max commands, then do not set.
+	 */
+	if (!sdebug_host_max_queue)
+		hpnt->nr_hw_queues = submit_queues;
 
 	sdbg_host->shost = hpnt;
 	*((struct sdebug_host_info **)hpnt->hostdata) = sdbg_host;
-- 
2.26.2

Re: [PATCH 2/2] scsi: scsi_debug: Support hostwide tags

[Douglas Gilbert]

On 2020-07-07 9:32 a.m., John Garry wrote:
> Many SCSI HBAs support a hostwide tagset, whereby each command submitted
> to the HW from all submission queues must have a unique tag identifier.
> 
> Normally this unique tag will be in the range [0, max queue), where "max
> queue" is the depth of each of the submission queues.
> 
> Add support for this hostwide tag feature, via module parameter
> "host_max_queue". A non-zero value means that the feature is enabled. In
> this case, the submission queues are not exposed to upper layer, i.e. from
> blk-mq perspective, the device has a single hw queue. There are 2 reasons
> for this:
> a. it is assumed that the host can support nr_hw_queues * can_queue
>     commands, but this is not true for hostwide tags
> b. for nr_hw_queues != 0, the request tag is not unique over all HW queues,
>     and some HBA drivers want to use this tag for the hostwide tag
> 
> However, like many SCSI HBA drivers today - megaraid sas being an example -
> the full set of HW submission queues are still used in the LLDD driver. So
> instead of using a complicated "reply_map" to create a per-CPU submission
> queue mapping like megaraid sas (as it depends on a PCI device + MSIs) -
> use a simple algorithm:
> 
> hwq = cpu % queue count
> 
> If the host max queue param is less than the max queue depth param, then
> the max queue depth param is clipped.
> 
> If and when hostwide shared tags are supported in blk-mq/scsi mid-layer,
> then the policy to set nr_hw_queues = 0 for hostwide tags can be revised.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/scsi/scsi_debug.c | 73 ++++++++++++++++++++++++++++++++-------
>   1 file changed, 61 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 68534a23866e..a56d5ee9f4d7 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -344,6 +344,7 @@ struct sdebug_defer {
>   	struct execute_work ew;
>   	int sqa_idx;	/* index of sdebug_queue array */
>   	int qc_idx;	/* index of sdebug_queued_cmd array within sqa_idx */
> +	int hc_idx;	/* hostwide tag index */
>   	int issuing_cpu;
>   	bool init_hrt;
>   	bool init_wq;
> @@ -762,6 +763,7 @@ static unsigned int sdebug_guard = DEF_GUARD;
>   static int sdebug_lowest_aligned = DEF_LOWEST_ALIGNED;
>   static int sdebug_max_luns = DEF_MAX_LUNS;
>   static int sdebug_max_queue = SDEBUG_CANQUEUE;	/* per submit queue */
> +static int sdebug_host_max_queue;	/* per host */
>   static unsigned int sdebug_medium_error_start = OPT_MEDIUM_ERR_ADDR;
>   static int sdebug_medium_error_count = OPT_MEDIUM_ERR_NUM;
>   static atomic_t retired_max_queue;	/* if > 0 then was prior max_queue */
> @@ -4707,15 +4709,28 @@ static int resp_rwp_zone(struct scsi_cmnd *scp, struct sdebug_dev_info *devip)
>   
>   static struct sdebug_queue *get_queue(struct scsi_cmnd *cmnd)
>   {
> -	u32 tag = blk_mq_unique_tag(cmnd->request);
> -	u16 hwq = blk_mq_unique_tag_to_hwq(tag);
> +	u16 hwq;
>   
> -	pr_debug("tag=%#x, hwq=%d\n", tag, hwq);
> -	if (WARN_ON_ONCE(hwq >= submit_queues))
> -		hwq = 0;
> +	if (sdebug_host_max_queue) {
> +		/* Provide a simple method to choose the hwq */
> +		hwq = smp_processor_id() % submit_queues;
> +	} else {
> +		u32 tag = blk_mq_unique_tag(cmnd->request);
> +
> +		hwq = blk_mq_unique_tag_to_hwq(tag);
> +
> +		pr_debug("tag=%#x, hwq=%d\n", tag, hwq);
> +		if (WARN_ON_ONCE(hwq >= submit_queues))
> +			hwq = 0;
> +	}
>   	return sdebug_q_arr + hwq;
>   }
>   
> +static u32 get_tag(struct scsi_cmnd *cmnd)
> +{
> +	return blk_mq_unique_tag(cmnd->request);
> +}
> +
>   /* Queued (deferred) command completions converge here. */
>   static void sdebug_q_cmd_complete(struct sdebug_defer *sd_dp)
>   {
> @@ -4747,8 +4762,8 @@ static void sdebug_q_cmd_complete(struct sdebug_defer *sd_dp)
>   	scp = sqcp->a_cmnd;
>   	if (unlikely(scp == NULL)) {
>   		spin_unlock_irqrestore(&sqp->qc_lock, iflags);
> -		pr_err("scp is NULL, sqa_idx=%d, qc_idx=%d\n",
> -		       sd_dp->sqa_idx, qc_idx);
> +		pr_err("scp is NULL, sqa_idx=%d, qc_idx=%d, hc_idx=%d\n",
> +		       sd_dp->sqa_idx, qc_idx, sd_dp->hc_idx);
>   		return;
>   	}
>   	devip = (struct sdebug_dev_info *)scp->device->hostdata;
> @@ -5451,6 +5466,10 @@ static int schedule_resp(struct scsi_cmnd *cmnd, struct sdebug_dev_info *devip,
>   		new_sd_dp = false;
>   	}
>   
> +	/* Set the hostwide tag */
> +	if (sdebug_host_max_queue)
> +		sd_dp->hc_idx = get_tag(cmnd);
> +
>   	if (ndelay > 0 && ndelay < INCLUSIVE_TIMING_MAX_NS)
>   		ns_from_boot = ktime_get_boottime_ns();
>   
> @@ -5585,6 +5604,8 @@ module_param_named(lbpws10, sdebug_lbpws10, int, S_IRUGO);
>   module_param_named(lowest_aligned, sdebug_lowest_aligned, int, S_IRUGO);
>   module_param_named(max_luns, sdebug_max_luns, int, S_IRUGO | S_IWUSR);
>   module_param_named(max_queue, sdebug_max_queue, int, S_IRUGO | S_IWUSR);
> +module_param_named(host_max_queue, sdebug_host_max_queue, int,
> +		   S_IRUGO | S_IWUSR);

I can't see a "_store" method to match that S_IWUSR. If you don't want that
parameter to be run-time changeable, then please drop the S_IWUSR.

>   module_param_named(medium_error_count, sdebug_medium_error_count, int,
>   		   S_IRUGO | S_IWUSR);
>   module_param_named(medium_error_start, sdebug_medium_error_start, int,
> @@ -5654,6 +5675,7 @@ MODULE_PARM_DESC(lbpws10, "enable LBP, support WRITE SAME(10) with UNMAP bit (de
>   MODULE_PARM_DESC(lowest_aligned, "lowest aligned lba (def=0)");
>   MODULE_PARM_DESC(max_luns, "number of LUNs per target to simulate(def=1)");
>   MODULE_PARM_DESC(max_queue, "max number of queued commands (1 to max(def))");
> +MODULE_PARM_DESC(host_max_queue, "max number of queued commands per host (0 to max(def))");
>   MODULE_PARM_DESC(medium_error_count, "count of sectors to return follow on MEDIUM error");
>   MODULE_PARM_DESC(medium_error_start, "starting sector number to return MEDIUM error");
>   MODULE_PARM_DESC(ndelay, "response delay in nanoseconds (def=0 -> ignore)");
> @@ -6141,7 +6163,8 @@ static ssize_t max_queue_store(struct device_driver *ddp, const char *buf,
>   	struct sdebug_queue *sqp;
>   
>   	if ((count > 0) && (1 == sscanf(buf, "%d", &n)) && (n > 0) &&
> -	    (n <= SDEBUG_CANQUEUE)) {
> +	    (n <= SDEBUG_CANQUEUE) &&
> +	    (sdebug_host_max_queue ? sdebug_host_max_queue >= n : 1)) {
>   		block_unblock_all_queues(true);
>   		k = 0;
>   		for (j = 0, sqp = sdebug_q_arr; j < submit_queues;
> @@ -6164,6 +6187,17 @@ static ssize_t max_queue_store(struct device_driver *ddp, const char *buf,
>   }
>   static DRIVER_ATTR_RW(max_queue);
>   
> +static ssize_t host_max_queue_show(struct device_driver *ddp, char *buf)
> +{
> +	return scnprintf(buf, PAGE_SIZE, "%d\n", sdebug_host_max_queue);
> +}
> +
> +/*
> + * Since this is used for .can_queue, and we get the hc_idx tag from the bitmap
> + * in range [0, sdebug_host_max_queue), we can't change it.
> + */
> +static DRIVER_ATTR_RO(host_max_queue);
> +
>   static ssize_t no_uld_show(struct device_driver *ddp, char *buf)
>   {
>   	return scnprintf(buf, PAGE_SIZE, "%d\n", sdebug_no_uld);
> @@ -6510,6 +6544,7 @@ static struct attribute *sdebug_drv_attrs[] = {
>   	&driver_attr_every_nth.attr,
>   	&driver_attr_max_luns.attr,
>   	&driver_attr_max_queue.attr,
> +	&driver_attr_host_max_queue.attr,
>   	&driver_attr_no_uld.attr,
>   	&driver_attr_scsi_level.attr,
>   	&driver_attr_virtual_gb.attr,
> @@ -6619,6 +6654,13 @@ static int __init scsi_debug_init(void)
>   		return -EINVAL;
>   	}
>   
> +	if (sdebug_host_max_queue &&
> +	    sdebug_max_queue > sdebug_host_max_queue) {
> +		sdebug_max_queue = sdebug_host_max_queue;
> +		pr_warn("reducing max submit queue depth to host max queue depth, %d\n",
> +			sdebug_max_queue);
> +	}
> +
>   	sdebug_q_arr = kcalloc(submit_queues, sizeof(struct sdebug_queue),
>   			       GFP_KERNEL);
>   	if (sdebug_q_arr == NULL)
> @@ -7257,7 +7299,10 @@ static int sdebug_driver_probe(struct device *dev)
>   
>   	sdbg_host = to_sdebug_host(dev);
>   
> -	sdebug_driver_template.can_queue = sdebug_max_queue;
> +	if (sdebug_host_max_queue)
> +		sdebug_driver_template.can_queue = sdebug_host_max_queue;
> +	else
> +		sdebug_driver_template.can_queue = sdebug_max_queue;
>   	if (!sdebug_clustering)
>   		sdebug_driver_template.dma_boundary = PAGE_SIZE - 1;
>   
> @@ -7272,9 +7317,13 @@ static int sdebug_driver_probe(struct device *dev)
>   			my_name, submit_queues, nr_cpu_ids);
>   		submit_queues = nr_cpu_ids;
>   	}
> -	/* Decide whether to tell scsi subsystem that we want mq */
> -	/* Following should give the same answer for each host */
> -	hpnt->nr_hw_queues = submit_queues;
> +	/*
> +	 * Decide whether to tell scsi subsystem that we want mq. The
> +	 * following should give the same answer for each host. If the host
> +	 * has a limit of hostwide max commands, then do not set.
> +	 */
> +	if (!sdebug_host_max_queue)
> +		hpnt->nr_hw_queues = submit_queues;
>   
>   	sdbg_host->shost = hpnt;
>   	*((struct sdebug_host_info **)hpnt->hostdata) = sdbg_host;
> 

John,
Looks good apart from the issue above. Another minor point: in a year's
time (when this patch isn't (near) top of mind) then the output from
'modinfo scsi_debug' can be bewildering: so many parameter options, how
to find the one(s) I need. That is why I try to put them in alphabetical
order (namely the module_param_named() and MODULE_PARM_DESC() entries).
Maybe the "DESC" could be expanded to hint at the relationship with
max_queue.

Doug Gilbert

Re: [PATCH 2/2] scsi: scsi_debug: Support hostwide tags

[John Garry]

On 07/07/2020 17:28, Douglas Gilbert wrote:


>> +	if (sdebug_host_max_queue)
>> +		sd_dp->hc_idx = get_tag(cmnd);
>> +
>>    	if (ndelay > 0 && ndelay < INCLUSIVE_TIMING_MAX_NS)
>>    		ns_from_boot = ktime_get_boottime_ns();
>>    
>> @@ -5585,6 +5604,8 @@ module_param_named(lbpws10, sdebug_lbpws10, int, S_IRUGO);
>>    module_param_named(lowest_aligned, sdebug_lowest_aligned, int, S_IRUGO);
>>    module_param_named(max_luns, sdebug_max_luns, int, S_IRUGO | S_IWUSR);
>>    module_param_named(max_queue, sdebug_max_queue, int, S_IRUGO | S_IWUSR);
>> +module_param_named(host_max_queue, sdebug_host_max_queue, int,
>> +		   S_IRUGO | S_IWUSR);
> I can't see a "_store" method to match that S_IWUSR. If you don't want that
> parameter to be run-time changeable, then please drop the S_IWUSR.

Right, my intention was for RO, so I can drop S_IWUSR

> 
>>    module_param_named(medium_error_count, sdebug_medium_error_count, int,
>>    		   S_IRUGO | S_IWUSR);
>>    module_param_named(medium_error_start, sdebug_medium_error_start, int,
>> @@ -5654,6 +5675,7 @@ MODULE_PARM_DESC(lbpws10, "enable LBP, support WRITE SAME(10) with UNMAP bit (de
>>    MODULE_PARM_DESC(lowest_aligned, "lowest aligned lba (def=0)");
>>    MODULE_PARM_DESC(max_luns, "number of LUNs per target to simulate(def=1)");
>>    MODULE_PARM_DESC(max_queue, "max number of queued commands (1 to max(def))");
>> +MODULE_PARM_DESC(host_max_queue, "max number of queued commands per host (0 to max(def))");
>>    MODULE_PARM_DESC(medium_error_count, "count of sectors to return follow on MEDIUM error");
>>    MODULE_PARM_DESC(medium_error_start, "starting sector number to return MEDIUM error");
>>    MODULE_PARM_DESC(ndelay, "response delay in nanoseconds (def=0 -> ignore)");
>> @@ -6141,7 +6163,8 @@ static ssize_t max_queue_store(struct device_driver *ddp, const char *buf,
>>    	struct sdebug_queue *sqp;
>>    

[...]

>>    
>> @@ -7272,9 +7317,13 @@ static int sdebug_driver_probe(struct device *dev)
>>    			my_name, submit_queues, nr_cpu_ids);
>>    		submit_queues = nr_cpu_ids;
>>    	}
>> -	/* Decide whether to tell scsi subsystem that we want mq */
>> -	/* Following should give the same answer for each host */
>> -	hpnt->nr_hw_queues = submit_queues;
>> +	/*
>> +	 * Decide whether to tell scsi subsystem that we want mq. The
>> +	 * following should give the same answer for each host. If the host
>> +	 * has a limit of hostwide max commands, then do not set.
>> +	 */
>> +	if (!sdebug_host_max_queue)
>> +		hpnt->nr_hw_queues = submit_queues;
>>    
>>    	sdbg_host->shost = hpnt;
>>    	*((struct sdebug_host_info **)hpnt->hostdata) = sdbg_host;
>>
> John,
> Looks good apart from the issue above. Another minor point: in a year's
> time (when this patch isn't (near) top of mind) then the output from
> 'modinfo scsi_debug' can be bewildering: so many parameter options, how
> to find the one(s) I need. That is why I try to put them in alphabetical
> order (namely the module_param_named() and MODULE_PARM_DESC() entries).
> Maybe the "DESC" could be expanded to hint at the relationship with
> max_queue.

ok, I'll fix that up as requested.

Thanks for checking,
John

Re: [PATCH 1/2] scsi: scsi_debug: Add check for sdebug_max_queue during module init

[Douglas Gilbert]

On 2020-07-07 9:32 a.m., John Garry wrote:
> sdebug_max_queue should not exceed SDEBUG_CANQUEUE, otherwise crashes like
> this can be triggered by passing an out-of-range value:
> 
> Hardware name: Huawei D06 /D06, BIOS Hisilicon D06 UEFI RC0 - V1.16.01 03/15/2019
>   pstate: 20400009 (nzCv daif +PAN -UAO BTYPE=--)
>   pc : schedule_resp+0x2a4/0xa70 [scsi_debug]
>   lr : schedule_resp+0x52c/0xa70 [scsi_debug]
>   sp : ffff800022ab36f0
>   x29: ffff800022ab36f0 x28: ffff0023a935a610
>   x27: ffff800008e0a648 x26: 0000000000000003
>   x25: ffff0023e84f3200 x24: 00000000003d0900
>   x23: 0000000000000000 x22: 0000000000000000
>   x21: ffff0023be60a320 x20: ffff0023be60b538
>   x19: ffff800008e13000 x18: 0000000000000000
>   x17: 0000000000000000 x16: 0000000000000000
>   x15: 0000000000000000 x14: 0000000000000000
>   x13: 0000000000000000 x12: 0000000000000000
>   x11: 0000000000000000 x10: 0000000000000000
>   x9 : 0000000000000001 x8 : 0000000000000000
>   x7 : 0000000000000000 x6 : 00000000000000c1
>   x5 : 0000020000200000 x4 : dead0000000000ff
>   x3 : 0000000000000200 x2 : 0000000000000200
>   x1 : ffff800008e13d88 x0 : 0000000000000000
>   Call trace:
> schedule_resp+0x2a4/0xa70 [scsi_debug]
> scsi_debug_queuecommand+0x2c4/0x9e0 [scsi_debug]
> scsi_queue_rq+0x698/0x840
> __blk_mq_try_issue_directly+0x108/0x228
> blk_mq_request_issue_directly+0x58/0x98
> blk_mq_try_issue_list_directly+0x5c/0xf0
> blk_mq_sched_insert_requests+0x18c/0x200
> blk_mq_flush_plug_list+0x11c/0x190
> blk_flush_plug_list+0xdc/0x110
> blk_finish_plug+0x38/0x210
> blkdev_direct_IO+0x450/0x4d8
> generic_file_read_iter+0x84/0x180
> blkdev_read_iter+0x3c/0x50
> aio_read+0xc0/0x170
> io_submit_one+0x5c8/0xc98
> __arm64_sys_io_submit+0x1b0/0x258
> el0_svc_common.constprop.3+0x68/0x170
> do_el0_svc+0x24/0x90
> el0_sync_handler+0x13c/0x1a8
> el0_sync+0x158/0x180
>   Code: 528847e0 72a001e0 6b00003f 540018cd (3941c340)
> 
> In addition, it should not be less than 1.
> 
> So add checks for these, and fail the module init for those cases.
> 
> Fixes: c483739430f10 ("scsi_debug: add multiple queue support")
> Signed-off-by: John Garry <john.garry@huawei.com>

Acked-by: Douglas Gilbert <dgilbert@interlog.com>

> ---
>   drivers/scsi/scsi_debug.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/scsi/scsi_debug.c b/drivers/scsi/scsi_debug.c
> index 4692f5b6ad13..68534a23866e 100644
> --- a/drivers/scsi/scsi_debug.c
> +++ b/drivers/scsi/scsi_debug.c
> @@ -6613,6 +6613,12 @@ static int __init scsi_debug_init(void)
>   		pr_err("submit_queues must be 1 or more\n");
>   		return -EINVAL;
>   	}
> +
> +	if ((sdebug_max_queue > SDEBUG_CANQUEUE) || (sdebug_max_queue <= 0)) {
> +		pr_err("max_queue must be in range [1, %d]\n", SDEBUG_CANQUEUE);
> +		return -EINVAL;
> +	}
> +
>   	sdebug_q_arr = kcalloc(submit_queues, sizeof(struct sdebug_queue),
>   			       GFP_KERNEL);
>   	if (sdebug_q_arr == NULL)
>