Re: [PATCH RFC v3 00/22] blk-mq/libata/scsi: SCSI driver tagging improvements Part I

Re: [PATCH RFC v3 00/22] blk-mq/libata/scsi: SCSI driver tagging improvements Part I

[John Garry]

On 25/10/2022 11:17, John Garry wrote:

Hi all,

I meant to say that this is just an update for where I got to here. I am 
actually changing employer soon, but will continue in upstream linux 
storage domain. So I don't want people to think that I am just throwing 
some stuff over the wall for the community to deal with. I would still 
like people to check this.

Thanks,
John

> Currently SCSI low-level drivers are required to manage tags for commands
> which do not come via the block layer - libata internal commands would be
> an example of one of these. We want to make blk-mq manage these tags also.
> 
> There was some work to provide "reserved commands" support in such series
> as https://lore.kernel.org/linux-scsi/20211125151048.103910-1-hare@suse.de/
> 
> This was based on allocating a request for the lifetime of the "internal"
> command.
> 
> This series tries to solve that problem by not just allocating the request
> but also sending it as a request through the block layer. Reasons to do
> this:
> - Normal flow of a request and also commonality for regular scsi command
>    lifetime
> - We don't leave request and scsi_cmnd fields dangling as when we just
>    allocate and free the request for the lifetime of the "internal" command
> - For poll mode support we can only poll in block layer, so could not send
>    internal commands on poll mode queues if we did not do this, which is a
>    problem
> - Can get rid of duplicated code like libsas internal command timeout
>    handling
> 
> Series part I contains core SCSI midlayer, libata, and libsas changes to
> queue libsas "slow" tasks as requests.
> 
> Series part II of this series focused on changing libata to queue internal
> commands as requests.
> 
> Testing:
> QEMU with AHCI with disk and cdrom attached, hisi_sas, pm8001.
> 
> Branch containing all patches is at:
> https://github.com/hisilicon/kernel-dev/commits/private-topic-sas-6.1-block
> 
> v2 was here:
> https://lore.kernel.org/linux-scsi/1654770559-101375-1-git-send-email-john.garry@huawei.com/
> 
> Hannes Reinecke (1):
>    scsi: core: Implement reserved command handling
> 
> John Garry (21):
>    blk-mq: Don't get budget for reserved requests
>    scsi: core: Add scsi_get_dev()
>    scsi: core: Add support to send reserved commands
>    scsi: core: Add support for reserved command timeout handling
>    scsi: libsas: Improve sas_ex_discover_expander() error handling
>    scsi: libsas: Notify LLDD expander found before calling sas_rphy_add()
>    scsi: scsi_transport_sas: Alloc sdev for expander
>    scsi: libsas: Add sas_alloc_slow_task_rq()
>    scsi: libsas: Add sas_queuecommand_internal()
>    scsi: libsas: Add sas_internal_timeout()
>    scsi: core: Use SCSI_SCAN_RESCAN in  __scsi_add_device()
>    scsi: scsi_transport_sas: Allocate end device target id in the rphy
>      alloc
>    ata: libata-scsi: Add ata_scsi_setup_sdev()
>    scsi: libsas: Add sas_ata_setup_device()
>    ata: libata-scsi: Allocate sdev early in port probe
>    scsi: libsas drivers: Reserve tags
>    scsi: libsas: Queue SMP commands as requests
>    scsi: libsas: Queue TMF commands as requests
>    scsi: core: Add scsi_alloc_request_hwq()
>    scsi: libsas: Queue internal abort commands as requests
>    scsi: libsas: Delete sas_task_slow.timer
> 
>   block/blk-mq.c                         |   4 +-
>   drivers/ata/libata-eh.c                |   1 +
>   drivers/ata/libata-scsi.c              |  49 ++++++++----
>   drivers/ata/libata.h                   |   1 +
>   drivers/scsi/aic94xx/aic94xx_init.c    |   3 +
>   drivers/scsi/hisi_sas/hisi_sas_main.c  |  40 +++++-----
>   drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |   3 +
>   drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |   3 +
>   drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |   7 ++
>   drivers/scsi/hosts.c                   |  16 ++++
>   drivers/scsi/isci/init.c               |   3 +
>   drivers/scsi/libsas/sas_ata.c          |  20 +++++
>   drivers/scsi/libsas/sas_expander.c     | 101 ++++++++++++++-----------
>   drivers/scsi/libsas/sas_init.c         |  61 ++++++++++++++-
>   drivers/scsi/libsas/sas_internal.h     |   5 ++
>   drivers/scsi/libsas/sas_scsi_host.c    |  93 ++++++++++++-----------
>   drivers/scsi/mvsas/mv_init.c           |   7 ++
>   drivers/scsi/pm8001/pm8001_init.c      |   8 +-
>   drivers/scsi/scsi_error.c              |   3 +
>   drivers/scsi/scsi_lib.c                |  42 +++++++++-
>   drivers/scsi/scsi_scan.c               |  29 ++++++-
>   drivers/scsi/scsi_transport_sas.c      |  34 ++++++---
>   include/linux/libata.h                 |   2 +
>   include/scsi/libsas.h                  |   8 +-
>   include/scsi/scsi_cmnd.h               |   3 +
>   include/scsi/scsi_host.h               |  21 ++++-
>   26 files changed, 424 insertions(+), 143 deletions(-)
> 

[PATCH RFC v3 00/22] blk-mq/libata/scsi: SCSI driver tagging improvements Part I

[John Garry]

Currently SCSI low-level drivers are required to manage tags for commands
which do not come via the block layer - libata internal commands would be
an example of one of these. We want to make blk-mq manage these tags also.

There was some work to provide "reserved commands" support in such series
as https://lore.kernel.org/linux-scsi/20211125151048.103910-1-hare@suse.de/

This was based on allocating a request for the lifetime of the "internal"
command.

This series tries to solve that problem by not just allocating the request
but also sending it as a request through the block layer. Reasons to do
this:
- Normal flow of a request and also commonality for regular scsi command
  lifetime
- We don't leave request and scsi_cmnd fields dangling as when we just
  allocate and free the request for the lifetime of the "internal" command
- For poll mode support we can only poll in block layer, so could not send
  internal commands on poll mode queues if we did not do this, which is a
  problem
- Can get rid of duplicated code like libsas internal command timeout
  handling

Series part I contains core SCSI midlayer, libata, and libsas changes to
queue libsas "slow" tasks as requests.

Series part II of this series focused on changing libata to queue internal
commands as requests.

Testing:
QEMU with AHCI with disk and cdrom attached, hisi_sas, pm8001.

Branch containing all patches is at:
https://github.com/hisilicon/kernel-dev/commits/private-topic-sas-6.1-block

v2 was here:
https://lore.kernel.org/linux-scsi/1654770559-101375-1-git-send-email-john.garry@huawei.com/

Hannes Reinecke (1):
  scsi: core: Implement reserved command handling

John Garry (21):
  blk-mq: Don't get budget for reserved requests
  scsi: core: Add scsi_get_dev()
  scsi: core: Add support to send reserved commands
  scsi: core: Add support for reserved command timeout handling
  scsi: libsas: Improve sas_ex_discover_expander() error handling
  scsi: libsas: Notify LLDD expander found before calling sas_rphy_add()
  scsi: scsi_transport_sas: Alloc sdev for expander
  scsi: libsas: Add sas_alloc_slow_task_rq()
  scsi: libsas: Add sas_queuecommand_internal()
  scsi: libsas: Add sas_internal_timeout()
  scsi: core: Use SCSI_SCAN_RESCAN in  __scsi_add_device()
  scsi: scsi_transport_sas: Allocate end device target id in the rphy
    alloc
  ata: libata-scsi: Add ata_scsi_setup_sdev()
  scsi: libsas: Add sas_ata_setup_device()
  ata: libata-scsi: Allocate sdev early in port probe
  scsi: libsas drivers: Reserve tags
  scsi: libsas: Queue SMP commands as requests
  scsi: libsas: Queue TMF commands as requests
  scsi: core: Add scsi_alloc_request_hwq()
  scsi: libsas: Queue internal abort commands as requests
  scsi: libsas: Delete sas_task_slow.timer

 block/blk-mq.c                         |   4 +-
 drivers/ata/libata-eh.c                |   1 +
 drivers/ata/libata-scsi.c              |  49 ++++++++----
 drivers/ata/libata.h                   |   1 +
 drivers/scsi/aic94xx/aic94xx_init.c    |   3 +
 drivers/scsi/hisi_sas/hisi_sas_main.c  |  40 +++++-----
 drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |   3 +
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |   3 +
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |   7 ++
 drivers/scsi/hosts.c                   |  16 ++++
 drivers/scsi/isci/init.c               |   3 +
 drivers/scsi/libsas/sas_ata.c          |  20 +++++
 drivers/scsi/libsas/sas_expander.c     | 101 ++++++++++++++-----------
 drivers/scsi/libsas/sas_init.c         |  61 ++++++++++++++-
 drivers/scsi/libsas/sas_internal.h     |   5 ++
 drivers/scsi/libsas/sas_scsi_host.c    |  93 ++++++++++++-----------
 drivers/scsi/mvsas/mv_init.c           |   7 ++
 drivers/scsi/pm8001/pm8001_init.c      |   8 +-
 drivers/scsi/scsi_error.c              |   3 +
 drivers/scsi/scsi_lib.c                |  42 +++++++++-
 drivers/scsi/scsi_scan.c               |  29 ++++++-
 drivers/scsi/scsi_transport_sas.c      |  34 ++++++---
 include/linux/libata.h                 |   2 +
 include/scsi/libsas.h                  |   8 +-
 include/scsi/scsi_cmnd.h               |   3 +
 include/scsi/scsi_host.h               |  21 ++++-
 26 files changed, 424 insertions(+), 143 deletions(-)

-- 
2.35.3

[PATCH RFC v3 01/22] blk-mq: Don't get budget for reserved requests

[John Garry]

It should be possible to send reserved requests even when there is no
budget, so don't request a budget in that case.

This comes into play when we need to allocate a reserved request from the
target device request queue for error handling for that same device.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 block/blk-mq.c          | 4 +++-
 drivers/scsi/scsi_lib.c | 3 ++-
 2 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/block/blk-mq.c b/block/blk-mq.c
index 260adeb2e455..d8baabb32ea4 100644
--- a/block/blk-mq.c
+++ b/block/blk-mq.c
@@ -1955,11 +1955,13 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list,
 	errors = queued = 0;
 	do {
 		struct blk_mq_queue_data bd;
+		bool need_budget;
 
 		rq = list_first_entry(list, struct request, queuelist);
 
 		WARN_ON_ONCE(hctx != rq->mq_hctx);
-		prep = blk_mq_prep_dispatch_rq(rq, !nr_budgets);
+		need_budget = !nr_budgets && !blk_mq_is_reserved_rq(rq);
+		prep = blk_mq_prep_dispatch_rq(rq, need_budget);
 		if (prep != PREP_DISPATCH_OK)
 			break;
 
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index fa96d3cfdfa3..39d4fd124375 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -298,7 +298,8 @@ void scsi_device_unbusy(struct scsi_device *sdev, struct scsi_cmnd *cmd)
 	if (starget->can_queue > 0)
 		atomic_dec(&starget->target_busy);
 
-	sbitmap_put(&sdev->budget_map, cmd->budget_token);
+	if (!blk_mq_is_reserved_rq(scsi_cmd_to_rq(cmd)))
+		sbitmap_put(&sdev->budget_map, cmd->budget_token);
 	cmd->budget_token = -1;
 }
 
-- 
2.35.3

[PATCH RFC v3 02/22] scsi: core: Add scsi_get_dev()

[John Garry]

Add a function which allows us to alloc a sdev with configurable
device parent, and channel:id:lun

This is useful for separating adding a scsi device into separate alloc
and scan steps

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/scsi_scan.c | 25 +++++++++++++++++++++++++
 include/scsi/scsi_host.h |  4 ++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index 5d27f5196de6..ad0a5902d1a0 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1979,3 +1979,28 @@ void scsi_forget_host(struct Scsi_Host *shost)
 	spin_unlock_irqrestore(shost->host_lock, flags);
 }
 
+struct scsi_device *scsi_get_dev(struct device *parent, int channel, uint id, u64 lun)
+{
+	struct Scsi_Host *shost = dev_to_shost(parent);
+	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(parent, channel, id);
+	if (!starget)
+		goto out;
+
+	sdev = scsi_alloc_sdev(starget, lun, 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_dev);
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index e71436183c0d..750ccf126377 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -787,6 +787,10 @@ void scsi_host_busy_iter(struct Scsi_Host *,
 
 struct class_container;
 
+
+extern struct scsi_device *scsi_get_dev(struct device *parent, int channel,
+					uint id, u64 lun);
+
 /*
  * DIF defines the exchange of protection information between
  * initiator and SBC block device.
-- 
2.35.3

[PATCH RFC v3 03/22] scsi: core: Implement reserved command handling

[John Garry]

From: Hannes Reinecke <hare@suse.de>

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>
#jpg: Set tag_set->queue_depth = shost->can_queue, and not
= shost->can_queue + shost->nr_reserved_cmds;
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hosts.c     |  3 +++
 drivers/scsi/scsi_lib.c  |  2 ++
 include/scsi/scsi_host.h | 15 ++++++++++++++-
 3 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 12346e2297fd..db89afc37bc9 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -489,6 +489,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 39d4fd124375..a8c4e7c037ae 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1978,6 +1978,8 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
 	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->reserved_tags = shost->nr_reserved_cmds;
+
 	tag_set->cmd_size = cmd_size;
 	tag_set->numa_node = dev_to_node(shost->dma_dev);
 	tag_set->flags = BLK_MQ_F_SHOULD_MERGE;
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 750ccf126377..91678c77398e 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -360,10 +360,17 @@ 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
+	 * including reserved commands.
 	 */
 	int can_queue;
 
+	/*
+	 * This determines how many commands the HBA will set aside
+	 * for reserved commands.
+	 */
+	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
@@ -611,6 +618,12 @@ struct Scsi_Host {
 	 */
 	unsigned nr_hw_queues;
 	unsigned nr_maps;
+
+	/*
+	 * Number of reserved commands to allocate, if any.
+	 */
+	unsigned int nr_reserved_cmds;
+
 	unsigned active_mode:2;
 
 	/*
-- 
2.35.3

[PATCH RFC v3 04/22] scsi: core: Add support to send reserved commands

[John Garry]

Add a method to queue reserved commands.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hosts.c     |  6 ++++++
 drivers/scsi/scsi_lib.c  | 25 +++++++++++++++++++++++++
 include/scsi/scsi_host.h |  1 +
 3 files changed, 32 insertions(+)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index db89afc37bc9..78968553089f 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -230,6 +230,12 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 		goto fail;
 	}
 
+	if (shost->nr_reserved_cmds && !sht->reserved_queuecommand) {
+		shost_printk(KERN_ERR, shost,
+			"nr_reserved_cmds set but no method to queue\n");
+		goto fail;
+	}
+
 	/* Use min_t(int, ...) in case shost->can_queue exceeds SHRT_MAX */
 	shost->cmd_per_lun = min_t(int, shost->cmd_per_lun,
 				   shost->can_queue);
diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index a8c4e7c037ae..08015c42c326 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1428,6 +1428,16 @@ static void scsi_complete(struct request *rq)
 	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
 	enum scsi_disposition disposition;
 
+	if (blk_mq_is_reserved_rq(rq)) {
+		struct scsi_device *sdev = cmd->device;
+
+		scsi_mq_uninit_cmd(cmd);
+		scsi_device_unbusy(sdev, cmd);
+		__blk_mq_end_request(rq, 0);
+
+		return;
+	}
+
 	INIT_LIST_HEAD(&cmd->eh_entry);
 
 	atomic_inc(&cmd->device->iodone_cnt);
@@ -1718,6 +1728,21 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
 	blk_status_t ret;
 	int reason;
 
+	if (blk_mq_is_reserved_rq(req)) {
+		if (!(req->rq_flags & RQF_DONTPREP)) {
+			ret = scsi_prepare_cmd(req);
+			if (ret != BLK_STS_OK)
+				goto out_dec_host_busy;
+
+			req->rq_flags |= RQF_DONTPREP;
+		} else {
+			clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
+		}
+		blk_mq_start_request(req);
+
+		return shost->hostt->reserved_queuecommand(shost, cmd);
+	}
+
 	WARN_ON_ONCE(cmd->budget_token < 0);
 
 	/*
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index 91678c77398e..a39f36aa0b0d 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -73,6 +73,7 @@ struct scsi_host_template {
 	 * STATUS: REQUIRED
 	 */
 	int (* queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
+	int (*reserved_queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
 
 	/*
 	 * The commit_rqs function is used to trigger a hardware
-- 
2.35.3

[PATCH RFC v3 05/22] scsi: core: Add support for reserved command timeout handling

[John Garry]

Add support for reserved timeout handling. Any driver which specifies
that it supports reserved tags must supply a handler.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hosts.c      | 15 +++++++++++----
 drivers/scsi/scsi_error.c |  3 +++
 include/scsi/scsi_host.h  |  1 +
 3 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
index 78968553089f..52346085b3c1 100644
--- a/drivers/scsi/hosts.c
+++ b/drivers/scsi/hosts.c
@@ -230,10 +230,17 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
 		goto fail;
 	}
 
-	if (shost->nr_reserved_cmds && !sht->reserved_queuecommand) {
-		shost_printk(KERN_ERR, shost,
-			"nr_reserved_cmds set but no method to queue\n");
-		goto fail;
+	if (shost->nr_reserved_cmds) {
+		if (!sht->reserved_queuecommand) {
+			shost_printk(KERN_ERR, shost,
+				"nr_reserved_cmds set but no method to queue\n");
+			goto fail;
+		}
+		if (!sht->reserved_timedout) {
+			shost_printk(KERN_ERR, shost,
+				"nr_reserved_cmds set but no method to handle timeouts\n");
+			goto fail;
+		}
 	}
 
 	/* Use min_t(int, ...) in case shost->can_queue exceeds SHRT_MAX */
diff --git a/drivers/scsi/scsi_error.c b/drivers/scsi/scsi_error.c
index 6995c8979230..2f86a4284f29 100644
--- a/drivers/scsi/scsi_error.c
+++ b/drivers/scsi/scsi_error.c
@@ -338,6 +338,9 @@ enum blk_eh_timer_return scsi_timeout(struct request *req)
 	if (host->eh_deadline != -1 && !host->last_reset)
 		host->last_reset = jiffies;
 
+	if (blk_mq_is_reserved_rq(req))
+		return host->hostt->reserved_timedout(scmd);
+
 	if (host->hostt->eh_timed_out)
 		rtn = host->hostt->eh_timed_out(scmd);
 
diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
index a39f36aa0b0d..266c13e731de 100644
--- a/include/scsi/scsi_host.h
+++ b/include/scsi/scsi_host.h
@@ -74,6 +74,7 @@ struct scsi_host_template {
 	 */
 	int (* queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
 	int (*reserved_queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
+	enum blk_eh_timer_return (*reserved_timedout)(struct scsi_cmnd *);
 
 	/*
 	 * The commit_rqs function is used to trigger a hardware
-- 
2.35.3

[PATCH RFC v3 06/22] scsi: libsas: Improve sas_ex_discover_expander() error handling

[John Garry]

Errors in function sas_ex_discover_expander() are generally ignored, so add
handling to gracefully handles errors in all cases.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 50 ++++++++++++++++++++----------
 1 file changed, 34 insertions(+), 16 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 2907ca5d0ed4..9e33014ec412 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -919,7 +919,6 @@ static struct domain_device *sas_ex_discover_expander(
 	struct sas_rphy *rphy;
 	struct sas_expander_device *edev;
 	struct asd_sas_port *port;
-	int res;
 
 	if (phy->routing_attr == DIRECT_ROUTING) {
 		pr_warn("ex %016llx:%02d:D <--> ex %016llx:0x%x is not allowed\n",
@@ -933,9 +932,11 @@ static struct domain_device *sas_ex_discover_expander(
 		return NULL;
 
 	phy->port = sas_port_alloc(&parent->rphy->dev, phy_id);
-	/* FIXME: better error handling */
-	BUG_ON(sas_port_add(phy->port) != 0);
+	if (!phy->port)
+		goto err_put_child;
 
+	if (!sas_port_add(phy->port))
+		goto err_delete_port;
 
 	switch (phy->attached_dev_type) {
 	case SAS_EDGE_EXPANDER_DEVICE:
@@ -948,8 +949,15 @@ static struct domain_device *sas_ex_discover_expander(
 		break;
 	default:
 		rphy = NULL;	/* shut gcc up */
-		BUG();
+		pr_warn("ex unhandled dev type %d %016llx:%02d:D <--> ex %016llx:%02d\n",
+			phy->attached_dev_type,
+			SAS_ADDR(parent->sas_addr), phy_id,
+			SAS_ADDR(phy->attached_sas_addr),
+			phy->attached_phy_id);
 	}
+	if (!rphy)
+		goto err_free_port;
+
 	port = parent->port;
 	child->rphy = rphy;
 	get_device(&rphy->dev);
@@ -967,26 +975,36 @@ static struct domain_device *sas_ex_discover_expander(
 	parent->port->disc.max_level = max(parent->port->disc.max_level,
 					   edev->level);
 	sas_init_dev(child);
+
 	sas_fill_in_rphy(child, rphy);
-	sas_rphy_add(rphy);
+	if (!sas_rphy_add(rphy))
+		goto err_rphy_free;
 
 	spin_lock_irq(&parent->port->dev_list_lock);
 	list_add_tail(&child->dev_list_node, &parent->port->dev_list);
 	spin_unlock_irq(&parent->port->dev_list_lock);
 
-	res = sas_discover_expander(child);
-	if (res) {
-		sas_rphy_delete(rphy);
-		spin_lock_irq(&parent->port->dev_list_lock);
-		list_del(&child->dev_list_node);
-		spin_unlock_irq(&parent->port->dev_list_lock);
-		sas_put_device(child);
-		sas_port_delete(phy->port);
-		phy->port = NULL;
-		return NULL;
-	}
+	if (!sas_discover_expander(child))
+		goto err_rphy_remove;
+
 	list_add_tail(&child->siblings, &parent->ex_dev.children);
 	return child;
+
+err_rphy_remove:
+	spin_lock_irq(&parent->port->dev_list_lock);
+	list_del(&child->dev_list_node);
+	spin_unlock_irq(&parent->port->dev_list_lock);
+	sas_rphy_remove(rphy);
+err_rphy_free:
+	sas_rphy_free(rphy);
+err_free_port:
+	sas_port_free(phy->port);
+err_delete_port:
+	sas_port_delete(phy->port);
+	phy->port = NULL;
+err_put_child:
+	sas_put_device(child);
+	return NULL;
 }
 
 static int sas_ex_discover_dev(struct domain_device *dev, int phy_id)
-- 
2.35.3

[PATCH RFC v3 07/22] scsi: libsas: Notify LLDD expander found before calling sas_rphy_add()

[John Garry]

Ensure that we notify the LLDD of the expander being found before calling
sas_rphy_add() as this function will require the LLDD be notified when
we alloc a scsi device for an expander.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index 9e33014ec412..e7cb95683522 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -976,9 +976,12 @@ static struct domain_device *sas_ex_discover_expander(
 					   edev->level);
 	sas_init_dev(child);
 
+	if (!sas_notify_lldd_dev_found(child))
+		goto err_rphy_free;
+
 	sas_fill_in_rphy(child, rphy);
 	if (!sas_rphy_add(rphy))
-		goto err_rphy_free;
+		goto err_dev_gone;
 
 	spin_lock_irq(&parent->port->dev_list_lock);
 	list_add_tail(&child->dev_list_node, &parent->port->dev_list);
@@ -995,6 +998,8 @@ static struct domain_device *sas_ex_discover_expander(
 	list_del(&child->dev_list_node);
 	spin_unlock_irq(&parent->port->dev_list_lock);
 	sas_rphy_remove(rphy);
+err_dev_gone:
+	sas_notify_lldd_dev_gone(child);
 err_rphy_free:
 	sas_rphy_free(rphy);
 err_free_port:
@@ -1575,32 +1580,22 @@ static int sas_discover_expander(struct domain_device *dev)
 {
 	int res;
 
-	res = sas_notify_lldd_dev_found(dev);
-	if (res)
-		return res;
-
 	res = sas_ex_general(dev);
 	if (res)
-		goto out_err;
+		return res;
 	res = sas_ex_manuf_info(dev);
 	if (res)
-		goto out_err;
+		return res;
 
 	res = sas_expander_discover(dev);
 	if (res) {
 		pr_warn("expander %016llx discovery failed(0x%x)\n",
 			SAS_ADDR(dev->sas_addr), res);
-		goto out_err;
+		return res;
 	}
 
 	sas_check_ex_subtractive_boundary(dev);
-	res = sas_check_parent_topology(dev);
-	if (res)
-		goto out_err;
-	return 0;
-out_err:
-	sas_notify_lldd_dev_gone(dev);
-	return res;
+	return sas_check_parent_topology(dev);
 }
 
 static int sas_ex_level_discovery(struct asd_sas_port *port, const int level)
@@ -1643,6 +1638,10 @@ int sas_discover_root_expander(struct domain_device *dev)
 	int res;
 	struct sas_expander_device *ex = rphy_to_expander_device(dev->rphy);
 
+	res = sas_notify_lldd_dev_found(dev);
+	if (res)
+		return res;
+
 	res = sas_rphy_add(dev->rphy);
 	if (res)
 		goto out_err;
@@ -1659,6 +1658,7 @@ int sas_discover_root_expander(struct domain_device *dev)
 out_err2:
 	sas_rphy_remove(dev->rphy);
 out_err:
+	sas_notify_lldd_dev_gone(dev);
 	return res;
 }
 
-- 
2.35.3

[PATCH RFC v3 08/22] scsi: scsi_transport_sas: Alloc sdev for expander

[John Garry]

Allocate a scsi device for an expander for sending SMP commands on the
expander sdev request queue.

Use channel=1 to not conflict with scsi target devices.

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

diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index 2f88c61216ee..da075dfc7cd0 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -1548,6 +1548,11 @@ int sas_rphy_add(struct sas_rphy *rphy)
 
 		scsi_scan_target(&rphy->dev, 0, rphy->scsi_target_id, lun,
 				 SCSI_SCAN_INITIAL);
+	} else if (identify->device_type == SAS_EDGE_EXPANDER_DEVICE ||
+		   identify->device_type == SAS_FANOUT_EXPANDER_DEVICE) {
+
+		if (!scsi_get_dev(&rphy->dev, 1, rphy->scsi_target_id, 0))
+			return -ENODEV;
 	}
 
 	return 0;
@@ -1627,6 +1632,7 @@ sas_rphy_remove(struct sas_rphy *rphy)
 	case SAS_EDGE_EXPANDER_DEVICE:
 	case SAS_FANOUT_EXPANDER_DEVICE:
 		sas_remove_children(dev);
+		scsi_remove_target(dev);
 		break;
 	default:
 		break;
-- 
2.35.3

[PATCH RFC v3 09/22] scsi: libsas: Add sas_alloc_slow_task_rq()

[John Garry]

Add a function to add a slow task with a request associated.

The next step will be to send tasks same as we do for other requests -
through the block layer.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_init.c     | 51 ++++++++++++++++++++++++++++++
 drivers/scsi/libsas/sas_internal.h |  1 +
 include/scsi/libsas.h              |  2 +-
 3 files changed, 53 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index f2c05ebeb72f..90e63ff5e966 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -56,9 +56,60 @@ struct sas_task *sas_alloc_slow_task(gfp_t flags)
 	return task;
 }
 
+struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, gfp_t flags)
+{
+	struct sas_ha_struct *sas_ha = device->port->ha;
+	struct Scsi_Host *shost = sas_ha->core.shost;
+	struct scsi_device *sdev, *found_sdev = NULL;
+	struct scsi_cmnd *scmd;
+	struct sas_task *task;
+	struct request *rq;
+
+	shost_for_each_device(sdev, shost) {
+		struct scsi_target *starget = sdev->sdev_target;
+
+		if (starget->hostdata == device) {
+			found_sdev = sdev;
+			break;
+		}
+	}
+
+	if (!found_sdev)
+		return NULL;
+
+	scsi_device_put(found_sdev);
+
+	task = sas_alloc_slow_task(flags);
+	if (!task)
+		return NULL;
+
+	task->dev = device;
+
+	rq = scsi_alloc_request(sdev->request_queue, REQ_OP_DRV_IN,
+				BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT);
+	if (IS_ERR(rq)) {
+		sas_free_task(task);
+		return NULL;
+	}
+
+	scmd = blk_mq_rq_to_pdu(rq);
+
+	task->uldd_task = scmd;
+	rq->end_io_data = task;
+
+	return task;
+}
+
 void sas_free_task(struct sas_task *task)
 {
 	if (task) {
+		if (task->slow_task && task->uldd_task) {
+			struct scsi_cmnd *scmd = task->uldd_task;
+			struct request *rq = scsi_cmd_to_rq(scmd);
+
+			BUG_ON(!blk_mq_is_reserved_rq(rq));
+			blk_mq_free_request(rq);
+		}
 		kfree(task->slow_task);
 		kmem_cache_free(sas_task_cache, task);
 	}
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index 6cf190ade35e..f5ae4de382f7 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -54,6 +54,7 @@ void sas_free_event(struct asd_sas_event *event);
 
 struct sas_task *sas_alloc_task(gfp_t flags);
 struct sas_task *sas_alloc_slow_task(gfp_t flags);
+struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, gfp_t flags);
 void sas_free_task(struct sas_task *task);
 
 int  sas_register_ports(struct sas_ha_struct *sas_ha);
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 1aee3d0ebbb2..4c4d8c91b1c1 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -648,7 +648,7 @@ static inline struct request *sas_task_find_rq(struct sas_task *task)
 {
 	struct scsi_cmnd *scmd;
 
-	if (task->task_proto & SAS_PROTOCOL_STP_ALL) {
+	if (!task->slow_task && task->task_proto & SAS_PROTOCOL_STP_ALL) {
 		struct ata_queued_cmd *qc = task->uldd_task;
 
 		scmd = qc ? qc->scsicmd : NULL;
-- 
2.35.3

[PATCH RFC v3 10/22] scsi: libsas: Add sas_queuecommand_internal()

[John Garry]

Add a callback for scsi host template reserved_queuecommand callback, and
plug it into libsas LLDDs.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/aic94xx/aic94xx_init.c    |  1 +
 drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |  1 +
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |  1 +
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |  1 +
 drivers/scsi/isci/init.c               |  1 +
 drivers/scsi/libsas/sas_scsi_host.c    | 13 +++++++++++++
 drivers/scsi/mvsas/mv_init.c           |  1 +
 drivers/scsi/pm8001/pm8001_init.c      |  1 +
 include/scsi/libsas.h                  |  1 +
 9 files changed, 21 insertions(+)

diff --git a/drivers/scsi/aic94xx/aic94xx_init.c b/drivers/scsi/aic94xx/aic94xx_init.c
index 954d0c5ae2e2..e9d2ee5434c2 100644
--- a/drivers/scsi/aic94xx/aic94xx_init.c
+++ b/drivers/scsi/aic94xx/aic94xx_init.c
@@ -60,6 +60,7 @@ static struct scsi_host_template aic94xx_sht = {
 	.compat_ioctl		= sas_ioctl,
 #endif
 	.track_queue_depth	= 1,
+	.reserved_queuecommand = sas_queuecommand_internal,
 };
 
 static int asd_map_memio(struct asd_ha_struct *asd_ha)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
index d643c5a49aa9..6cf660b1212e 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
@@ -1760,6 +1760,7 @@ static struct scsi_host_template sht_v1_hw = {
 #endif
 	.shost_groups		= host_v1_hw_groups,
 	.host_reset             = hisi_sas_host_reset,
+	.reserved_queuecommand = sas_queuecommand_internal,
 };
 
 static const struct hisi_sas_hw hisi_sas_v1_hw = {
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index cded42f4ca44..d2bf23ad0833 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -3578,6 +3578,7 @@ static struct scsi_host_template sht_v2_hw = {
 	.host_reset		= hisi_sas_host_reset,
 	.map_queues		= map_queues_v2_hw,
 	.host_tagset		= 1,
+	.reserved_queuecommand = sas_queuecommand_internal,
 };
 
 static const struct hisi_sas_hw hisi_sas_v2_hw = {
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 0c3fcb807806..ff56072c7a33 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -3245,6 +3245,7 @@ static struct scsi_host_template sht_v3_hw = {
 	.tag_alloc_policy	= BLK_TAG_ALLOC_RR,
 	.host_reset             = hisi_sas_host_reset,
 	.host_tagset		= 1,
+	.reserved_queuecommand = sas_queuecommand_internal,
 };
 
 static const struct hisi_sas_hw hisi_sas_v3_hw = {
diff --git a/drivers/scsi/isci/init.c b/drivers/scsi/isci/init.c
index e294d5d961eb..e970f4b77ed3 100644
--- a/drivers/scsi/isci/init.c
+++ b/drivers/scsi/isci/init.c
@@ -177,6 +177,7 @@ static struct scsi_host_template isci_sht = {
 #endif
 	.shost_groups			= isci_host_groups,
 	.track_queue_depth		= 1,
+	.reserved_queuecommand = sas_queuecommand_internal,
 };
 
 static struct sas_domain_function_template isci_transport_ops  = {
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index a36fa1c128a8..04e8c0575021 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -158,6 +158,19 @@ static struct sas_task *sas_create_task(struct scsi_cmnd *cmd,
 	return task;
 }
 
+int sas_queuecommand_internal(struct Scsi_Host *shost, struct scsi_cmnd *cmnd)
+{
+	struct sas_ha_struct *ha = SHOST_TO_SAS_HA(shost);
+	struct sas_internal *i = to_sas_internal(ha->core.shost->transportt);
+	struct request *rq = blk_mq_rq_from_pdu(cmnd);
+	struct sas_task *task = rq->end_io_data;
+
+	ASSIGN_SAS_TASK(cmnd, task);
+
+	return i->dft->lldd_execute_task(task, GFP_KERNEL);
+}
+EXPORT_SYMBOL_GPL(sas_queuecommand_internal);
+
 int sas_queuecommand(struct Scsi_Host *host, struct scsi_cmnd *cmd)
 {
 	struct sas_internal *i = to_sas_internal(host->transportt);
diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index cfe84473a515..228ab00e180f 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -54,6 +54,7 @@ static struct scsi_host_template mvs_sht = {
 #endif
 	.shost_groups		= mvst_host_groups,
 	.track_queue_depth	= 1,
+	.reserved_queuecommand = sas_queuecommand_internal,
 };
 
 static struct sas_domain_function_template mvs_transport_ops = {
diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index a1df61205b20..1a12c3f97f53 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -123,6 +123,7 @@ static struct scsi_host_template pm8001_sht = {
 	.track_queue_depth	= 1,
 	.cmd_per_lun		= 32,
 	.map_queues		= pm8001_map_queues,
+	.reserved_queuecommand = sas_queuecommand_internal,
 };
 
 /*
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 4c4d8c91b1c1..64035f83c5bd 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -739,6 +739,7 @@ int  sas_discover_sata(struct domain_device *);
 int  sas_discover_end_dev(struct domain_device *);
 
 void sas_unregister_dev(struct asd_sas_port *port, struct domain_device *);
+int sas_queuecommand_internal(struct Scsi_Host *shost, struct scsi_cmnd *cmnd);
 
 void sas_init_dev(struct domain_device *);
 
-- 
2.35.3

[PATCH RFC v3 11/22] scsi: libsas: Add sas_internal_timeout()

[John Garry]

Add sas_internal_timeout(), which is the timeout handler for reserved
commands, and plug into the libsas LLDDs.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/aic94xx/aic94xx_init.c    |  1 +
 drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |  1 +
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |  1 +
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |  1 +
 drivers/scsi/isci/init.c               |  1 +
 drivers/scsi/libsas/sas_scsi_host.c    | 18 ++++++++++++++++++
 drivers/scsi/mvsas/mv_init.c           |  1 +
 drivers/scsi/pm8001/pm8001_init.c      |  1 +
 include/scsi/libsas.h                  |  1 +
 9 files changed, 26 insertions(+)

diff --git a/drivers/scsi/aic94xx/aic94xx_init.c b/drivers/scsi/aic94xx/aic94xx_init.c
index e9d2ee5434c2..06c86d7a777a 100644
--- a/drivers/scsi/aic94xx/aic94xx_init.c
+++ b/drivers/scsi/aic94xx/aic94xx_init.c
@@ -61,6 +61,7 @@ static struct scsi_host_template aic94xx_sht = {
 #endif
 	.track_queue_depth	= 1,
 	.reserved_queuecommand = sas_queuecommand_internal,
+	.reserved_timedout = sas_internal_timeout,
 };
 
 static int asd_map_memio(struct asd_ha_struct *asd_ha)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
index 6cf660b1212e..9f71cc72cd80 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
@@ -1761,6 +1761,7 @@ static struct scsi_host_template sht_v1_hw = {
 	.shost_groups		= host_v1_hw_groups,
 	.host_reset             = hisi_sas_host_reset,
 	.reserved_queuecommand = sas_queuecommand_internal,
+	.reserved_timedout = sas_internal_timeout,
 };
 
 static const struct hisi_sas_hw hisi_sas_v1_hw = {
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index d2bf23ad0833..483a03ed6253 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -3579,6 +3579,7 @@ static struct scsi_host_template sht_v2_hw = {
 	.map_queues		= map_queues_v2_hw,
 	.host_tagset		= 1,
 	.reserved_queuecommand = sas_queuecommand_internal,
+	.reserved_timedout = sas_internal_timeout,
 };
 
 static const struct hisi_sas_hw hisi_sas_v2_hw = {
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index ff56072c7a33..02d96fba510f 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -3246,6 +3246,7 @@ static struct scsi_host_template sht_v3_hw = {
 	.host_reset             = hisi_sas_host_reset,
 	.host_tagset		= 1,
 	.reserved_queuecommand = sas_queuecommand_internal,
+	.reserved_timedout = sas_internal_timeout,
 };
 
 static const struct hisi_sas_hw hisi_sas_v3_hw = {
diff --git a/drivers/scsi/isci/init.c b/drivers/scsi/isci/init.c
index e970f4b77ed3..9c7869bf6cde 100644
--- a/drivers/scsi/isci/init.c
+++ b/drivers/scsi/isci/init.c
@@ -178,6 +178,7 @@ static struct scsi_host_template isci_sht = {
 	.shost_groups			= isci_host_groups,
 	.track_queue_depth		= 1,
 	.reserved_queuecommand = sas_queuecommand_internal,
+	.reserved_timedout = sas_internal_timeout,
 };
 
 static struct sas_domain_function_template isci_transport_ops  = {
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 04e8c0575021..b7d1994a8f1b 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -930,6 +930,24 @@ void sas_task_internal_timedout(struct timer_list *t)
 	if (!is_completed)
 		complete(&task->slow_task->completion);
 }
+enum blk_eh_timer_return sas_internal_timeout(struct scsi_cmnd *scmd)
+{
+	struct sas_task *task = TO_SAS_TASK(scmd);
+	bool is_completed = true;
+	unsigned long flags;
+
+	spin_lock_irqsave(&task->task_state_lock, flags);
+	if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
+		task->task_state_flags |= SAS_TASK_STATE_ABORTED;
+		is_completed = false;
+	}
+	spin_unlock_irqrestore(&task->task_state_lock, flags);
+
+	if (!is_completed)
+		complete(&task->slow_task->completion);
+	return BLK_EH_DONE;
+}
+EXPORT_SYMBOL_GPL(sas_internal_timeout);
 
 #define TASK_TIMEOUT			(20 * HZ)
 #define TASK_RETRY			3
diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index 228ab00e180f..7fed0259e1f5 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -55,6 +55,7 @@ static struct scsi_host_template mvs_sht = {
 	.shost_groups		= mvst_host_groups,
 	.track_queue_depth	= 1,
 	.reserved_queuecommand = sas_queuecommand_internal,
+	.reserved_timedout = sas_internal_timeout,
 };
 
 static struct sas_domain_function_template mvs_transport_ops = {
diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index 1a12c3f97f53..ce9509792bc0 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -124,6 +124,7 @@ static struct scsi_host_template pm8001_sht = {
 	.cmd_per_lun		= 32,
 	.map_queues		= pm8001_map_queues,
 	.reserved_queuecommand = sas_queuecommand_internal,
+	.reserved_timedout = sas_internal_timeout,
 };
 
 /*
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 64035f83c5bd..f02156ccd376 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -711,6 +711,7 @@ int sas_set_phy_speed(struct sas_phy *phy, struct sas_phy_linkrates *rates);
 int sas_phy_reset(struct sas_phy *phy, int hard_reset);
 int sas_phy_enable(struct sas_phy *phy, int enable);
 extern int sas_queuecommand(struct Scsi_Host *, struct scsi_cmnd *);
+extern enum blk_eh_timer_return sas_internal_timeout(struct scsi_cmnd *scmd);
 extern int sas_target_alloc(struct scsi_target *);
 extern int sas_slave_configure(struct scsi_device *);
 extern int sas_change_queue_depth(struct scsi_device *, int new_depth);
-- 
2.35.3

[PATCH RFC v3 12/22] scsi: core: Use SCSI_SCAN_RESCAN in __scsi_add_device()

[John Garry]

Instead of using hardcoded '1' as the __scsi_add_device() ->
scsi_probe_and_add_lun() rescan arg, use proper macro SCSI_SCAN_RESCAN.

Signed-off-by: John Garry <john.garry@huawei.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/scsi_scan.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index ad0a5902d1a0..b795c138f2c1 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -1580,7 +1580,8 @@ struct scsi_device *__scsi_add_device(struct Scsi_Host *shost, uint channel,
 		scsi_complete_async_scans();
 
 	if (scsi_host_scan_allowed(shost) && scsi_autopm_get_host(shost) == 0) {
-		scsi_probe_and_add_lun(starget, lun, NULL, &sdev, 1, hostdata);
+		scsi_probe_and_add_lun(starget, lun, NULL, &sdev,
+				       SCSI_SCAN_RESCAN, hostdata);
 		scsi_autopm_put_host(shost);
 	}
 	mutex_unlock(&shost->scan_mutex);
-- 
2.35.3

[PATCH RFC v3 13/22] scsi: scsi_transport_sas: Allocate end device target id in the rphy alloc

[John Garry]

Currently the per-end device target id is allocated when adding the rphy,
which is when we execute the scan of the target.

However it will be useful to have the target id allocated earlier when
allocating the rphy for the end device. For libata we want to move to a
scheme of allocating the sdev early in the probe process and then later
executing the scan (for that target). As such, users of would libata would
require that the target id allocated earlier here (before the scan).

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/scsi_transport_sas.c | 28 ++++++++++++++++++----------
 1 file changed, 18 insertions(+), 10 deletions(-)

diff --git a/drivers/scsi/scsi_transport_sas.c b/drivers/scsi/scsi_transport_sas.c
index da075dfc7cd0..c03e88990264 100644
--- a/drivers/scsi/scsi_transport_sas.c
+++ b/drivers/scsi/scsi_transport_sas.c
@@ -1433,6 +1433,7 @@ static void sas_rphy_initialize(struct sas_rphy *rphy)
 struct sas_rphy *sas_end_device_alloc(struct sas_port *parent)
 {
 	struct Scsi_Host *shost = dev_to_shost(&parent->dev);
+	struct sas_host_attrs *sas_host = to_sas_host_attrs(shost);
 	struct sas_end_device *rdev;
 
 	rdev = kzalloc(sizeof(*rdev), GFP_KERNEL);
@@ -1455,6 +1456,10 @@ struct sas_rphy *sas_end_device_alloc(struct sas_port *parent)
 	sas_rphy_initialize(&rdev->rphy);
 	transport_setup_device(&rdev->rphy.dev);
 
+	mutex_lock(&sas_host->lock);
+	rdev->rphy.scsi_target_id = sas_host->next_target_id++;
+	mutex_unlock(&sas_host->lock);
+
 	return &rdev->rphy;
 }
 EXPORT_SYMBOL(sas_end_device_alloc);
@@ -1500,6 +1505,17 @@ struct sas_rphy *sas_expander_alloc(struct sas_port *parent,
 }
 EXPORT_SYMBOL(sas_expander_alloc);
 
+static bool sas_rphy_valid_tproto_end_device(struct sas_rphy *rphy)
+{
+	struct sas_identify *identify = &rphy->identify;
+
+	if (identify->device_type != SAS_END_DEVICE)
+		return false;
+
+	return (identify->target_port_protocols &
+	    (SAS_PROTOCOL_SSP | SAS_PROTOCOL_STP | SAS_PROTOCOL_SATA));
+}
+
 /**
  * sas_rphy_add  -  add a SAS remote PHY to the device hierarchy
  * @rphy:	The remote PHY to be added
@@ -1529,16 +1545,9 @@ int sas_rphy_add(struct sas_rphy *rphy)
 
 	mutex_lock(&sas_host->lock);
 	list_add_tail(&rphy->list, &sas_host->rphy_list);
-	if (identify->device_type == SAS_END_DEVICE &&
-	    (identify->target_port_protocols &
-	     (SAS_PROTOCOL_SSP | SAS_PROTOCOL_STP | SAS_PROTOCOL_SATA)))
-		rphy->scsi_target_id = sas_host->next_target_id++;
-	else if (identify->device_type == SAS_END_DEVICE)
-		rphy->scsi_target_id = -1;
 	mutex_unlock(&sas_host->lock);
 
-	if (identify->device_type == SAS_END_DEVICE &&
-	    rphy->scsi_target_id != -1) {
+	if (sas_rphy_valid_tproto_end_device(rphy)) {
 		int lun;
 
 		if (identify->target_port_protocols & SAS_PROTOCOL_SSP)
@@ -1672,8 +1681,7 @@ static int sas_user_scan(struct Scsi_Host *shost, uint channel,
 
 	mutex_lock(&sas_host->lock);
 	list_for_each_entry(rphy, &sas_host->rphy_list, list) {
-		if (rphy->identify.device_type != SAS_END_DEVICE ||
-		    rphy->scsi_target_id == -1)
+		if (!sas_rphy_valid_tproto_end_device(rphy))
 			continue;
 
 		if ((channel == SCAN_WILD_CARD || channel == 0) &&
-- 
2.35.3

[PATCH RFC v3 14/22] ata: libata-scsi: Add ata_scsi_setup_sdev()

[John Garry]

Add a function to setup the sdev associated with an ata_device.

Currently in libata to create the sdev we call __scsi_add_device() to
create the sdev and execute the scan. However if we want to move to a
2-step process where we allocate the sdev early in the port probe, then
we need a separate function just to allocate the sdev.

We add a ata_port_operations callback .setup_scsi_device for when the
driver needs a custom setup. This is essentially for libsas, which does
not use the same options for sdev parent and id.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/ata/libata-scsi.c | 26 ++++++++++++++++++++++++++
 drivers/ata/libata.h      |  1 +
 include/linux/libata.h    |  2 ++
 3 files changed, 29 insertions(+)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e2ebb0b065e2..efdba852e363 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -4253,6 +4253,32 @@ static void ata_scsi_assign_ofnode(struct ata_device *dev, struct ata_port *ap)
 }
 #endif
 
+int ata_scsi_setup_sdev(struct ata_device *dev)
+{
+	u64 lun = 0;
+	int channel = 0;
+	uint id = 0;
+	struct ata_link *link = dev->link;
+	struct ata_port *ap = link->ap;
+	struct Scsi_Host *shost = ap->scsi_host;
+	struct device *parent = &shost->shost_gendev;
+	struct scsi_device *sdev;
+
+	if (ap->ops->setup_scsi_device)
+		return ap->ops->setup_scsi_device(dev);
+
+	if (ata_is_host_link(link))
+		id = dev->devno;
+	else
+		channel = link->pmp;
+	sdev = scsi_get_dev(parent, channel, id, lun);
+	if (!sdev)
+		return -ENODEV;
+	dev->sdev = sdev;
+	ata_scsi_assign_ofnode(dev, ap);
+	return 0;
+}
+
 void ata_scsi_scan_host(struct ata_port *ap, int sync)
 {
 	int tries = 5;
diff --git a/drivers/ata/libata.h b/drivers/ata/libata.h
index 2c5c8273af01..0c2df1e60768 100644
--- a/drivers/ata/libata.h
+++ b/drivers/ata/libata.h
@@ -112,6 +112,7 @@ extern struct ata_device *ata_scsi_find_dev(struct ata_port *ap,
 					    const struct scsi_device *scsidev);
 extern int ata_scsi_add_hosts(struct ata_host *host,
 			      struct scsi_host_template *sht);
+extern int ata_scsi_setup_sdev(struct ata_device *dev);
 extern void ata_scsi_scan_host(struct ata_port *ap, int sync);
 extern int ata_scsi_offline_dev(struct ata_device *dev);
 extern void ata_scsi_set_sense(struct ata_device *dev,
diff --git a/include/linux/libata.h b/include/linux/libata.h
index fe990176e6ee..827d5838cd23 100644
--- a/include/linux/libata.h
+++ b/include/linux/libata.h
@@ -968,6 +968,8 @@ struct ata_port_operations {
 	void (*phy_reset)(struct ata_port *ap);
 	void (*eng_timeout)(struct ata_port *ap);
 
+	int (*setup_scsi_device)(struct ata_device *dev);
+
 	/*
 	 * ->inherits must be the last field and all the preceding
 	 * fields must be pointers.
-- 
2.35.3

[PATCH RFC v3 15/22] scsi: libsas: Add sas_ata_setup_device()

[John Garry]

Add a function which does a custom sdev alloc - essentially replicates
what we do in sas_rphy_add() in terms of sdev parent and id.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_ata.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
index 78e6046fb55a..a46a2d345422 100644
--- a/drivers/scsi/libsas/sas_ata.c
+++ b/drivers/scsi/libsas/sas_ata.c
@@ -523,6 +523,25 @@ static int sas_ata_prereset(struct ata_link *link, unsigned long deadline)
 	return res;
 }
 
+static int sas_ata_setup_device(struct ata_device *dev)
+{
+	u64 lun = 0;
+	int channel = 0;
+	struct ata_link *link = dev->link;
+	struct ata_port *ap = link->ap;
+	struct scsi_device *sdev;
+	struct domain_device *ddev = ap->private_data;
+	struct sas_rphy *rphy = ddev->rphy;
+	struct device *parent = &rphy->dev;
+	uint id = rphy->scsi_target_id;
+
+	sdev = scsi_get_dev(parent, channel, id, lun);
+	if (!sdev)
+		return -ENODEV;
+	dev->sdev = sdev;
+	return 0;
+}
+
 static struct ata_port_operations sas_sata_ops = {
 	.prereset		= sas_ata_prereset,
 	.hardreset		= sas_ata_hard_reset,
@@ -537,6 +556,7 @@ static struct ata_port_operations sas_sata_ops = {
 	.set_dmamode		= sas_ata_set_dmamode,
 	.sched_eh		= sas_ata_sched_eh,
 	.end_eh			= sas_ata_end_eh,
+	.setup_scsi_device	= sas_ata_setup_device,
 };
 
 static struct ata_port_info sata_port_info = {
-- 
2.35.3

[PATCH RFC v3 16/22] ata: libata-scsi: Allocate sdev early in port probe

[John Garry]

Currently the per-ata device sdev is allocated as part of the scsi target
scan, which is after the ata port probe.

However it is useful to have the sdev available in the port probe. As an
example of an advantage, if the request queue is available in the probe
(which it would be if the sdev is available), then it is possible to use
a SCSI cmnd for ATA internal commands. The benefit of this is then we can
put the ATA qc structure in the SCSI cmnd private data. It will also be
useful if we want to send ATA internal commands as requests.

Export scsi_target_reap() so that it can be used to put the extra
reference we get when allocating the sdev.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/ata/libata-eh.c   |  1 +
 drivers/ata/libata-scsi.c | 23 +++++++++--------------
 drivers/scsi/scsi_scan.c  |  1 +
 3 files changed, 11 insertions(+), 14 deletions(-)

diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
index 08e11bc312c2..1ed5b1b64792 100644
--- a/drivers/ata/libata-eh.c
+++ b/drivers/ata/libata-eh.c
@@ -3446,6 +3446,7 @@ static int ata_eh_schedule_probe(struct ata_device *dev)
 
 	ata_eh_detach_dev(dev);
 	ata_dev_init(dev);
+	ata_scsi_setup_sdev(dev);
 	ehc->did_probe_mask |= (1 << dev->devno);
 	ehc->i.action |= ATA_EH_RESET;
 	ehc->saved_xfer_mode[dev->devno] = 0;
diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index efdba852e363..476e0ef4bd29 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -1109,7 +1109,12 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)
 	if (dev->flags & ATA_DFLAG_TRUSTED)
 		sdev->security_supported = 1;
 
-	dev->sdev = sdev;
+	/*
+	 * Put extra reference which we get when allocating the starget
+	 * initially
+	 */
+	scsi_target_reap(scsi_target(sdev));
+
 	return 0;
 }
 
@@ -4289,26 +4294,16 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
  repeat:
 	ata_for_each_link(link, ap, EDGE) {
 		ata_for_each_dev(dev, link, ENABLED) {
-			struct scsi_device *sdev;
+			struct Scsi_Host *shost = ap->scsi_host;
 			int channel = 0, id = 0;
 
-			if (dev->sdev)
-				continue;
-
 			if (ata_is_host_link(link))
 				id = dev->devno;
 			else
 				channel = link->pmp;
 
-			sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
-						 NULL);
-			if (!IS_ERR(sdev)) {
-				dev->sdev = sdev;
-				ata_scsi_assign_ofnode(dev, ap);
-				scsi_device_put(sdev);
-			} else {
-				dev->sdev = NULL;
-			}
+			scsi_scan_target(&shost->shost_gendev, channel, id,
+					 0, SCSI_SCAN_INITIAL);
 		}
 	}
 
diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
index b795c138f2c1..da7bc14b030c 100644
--- a/drivers/scsi/scsi_scan.c
+++ b/drivers/scsi/scsi_scan.c
@@ -598,6 +598,7 @@ void scsi_target_reap(struct scsi_target *starget)
 	BUG_ON(starget->state == STARGET_DEL);
 	scsi_target_reap_ref_put(starget);
 }
+EXPORT_SYMBOL_GPL(scsi_target_reap);
 
 /**
  * scsi_sanitize_inquiry_string - remove non-graphical chars from an
-- 
2.35.3

[PATCH RFC v3 17/22] scsi: libsas drivers: Reserve tags

[John Garry]

Reserve 2x tags for libsas reserved tag handling, which should be
enough.

Continue to carve out a region of tags for driver internal management
until each sas_task always has a request.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/aic94xx/aic94xx_init.c    | 1 +
 drivers/scsi/hisi_sas/hisi_sas_main.c  | 4 ++++
 drivers/scsi/hisi_sas/hisi_sas_v1_hw.c | 1 +
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 1 +
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 5 +++++
 drivers/scsi/isci/init.c               | 1 +
 drivers/scsi/mvsas/mv_init.c           | 5 +++++
 drivers/scsi/pm8001/pm8001_init.c      | 6 +++++-
 8 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/aic94xx/aic94xx_init.c b/drivers/scsi/aic94xx/aic94xx_init.c
index 06c86d7a777a..70b735cedeb3 100644
--- a/drivers/scsi/aic94xx/aic94xx_init.c
+++ b/drivers/scsi/aic94xx/aic94xx_init.c
@@ -62,6 +62,7 @@ static struct scsi_host_template aic94xx_sht = {
 	.track_queue_depth	= 1,
 	.reserved_queuecommand = sas_queuecommand_internal,
 	.reserved_timedout = sas_internal_timeout,
+	.nr_reserved_cmds = 2,
 };
 
 static int asd_map_memio(struct asd_ha_struct *asd_ha)
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 54860d252466..fe2752d24fe8 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -2442,6 +2442,10 @@ int hisi_sas_probe(struct platform_device *pdev,
 		shost->can_queue = HISI_SAS_MAX_COMMANDS;
 		shost->cmd_per_lun = HISI_SAS_MAX_COMMANDS;
 	} else {
+		/*
+		 * Intentionally use HISI_SAS_UNRESERVED_IPTT for .can_queue until
+		 * every sas_task we're sent has a request associated.
+		 */
 		shost->can_queue = HISI_SAS_UNRESERVED_IPTT;
 		shost->cmd_per_lun = HISI_SAS_UNRESERVED_IPTT;
 	}
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
index 9f71cc72cd80..438e8a963782 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
@@ -1762,6 +1762,7 @@ static struct scsi_host_template sht_v1_hw = {
 	.host_reset             = hisi_sas_host_reset,
 	.reserved_queuecommand = sas_queuecommand_internal,
 	.reserved_timedout = sas_internal_timeout,
+	.nr_reserved_cmds = 2,
 };
 
 static const struct hisi_sas_hw hisi_sas_v1_hw = {
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index 483a03ed6253..b733eb18864c 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -3580,6 +3580,7 @@ static struct scsi_host_template sht_v2_hw = {
 	.host_tagset		= 1,
 	.reserved_queuecommand = sas_queuecommand_internal,
 	.reserved_timedout = sas_internal_timeout,
+	.nr_reserved_cmds = 2,
 };
 
 static const struct hisi_sas_hw hisi_sas_v2_hw = {
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 02d96fba510f..d703af7985b0 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -3247,6 +3247,7 @@ static struct scsi_host_template sht_v3_hw = {
 	.host_tagset		= 1,
 	.reserved_queuecommand = sas_queuecommand_internal,
 	.reserved_timedout = sas_internal_timeout,
+	.nr_reserved_cmds = 2,
 };
 
 static const struct hisi_sas_hw hisi_sas_v3_hw = {
@@ -4859,6 +4860,10 @@ hisi_sas_v3_probe(struct pci_dev *pdev, const struct pci_device_id *id)
 	shost->max_lun = ~0;
 	shost->max_channel = 1;
 	shost->max_cmd_len = 16;
+	/*
+	 * Intentionally use HISI_SAS_UNRESERVED_IPTT for .can_queue until
+	 * every sas_task we're sent has a request associated.
+	 */
 	shost->can_queue = HISI_SAS_UNRESERVED_IPTT;
 	shost->cmd_per_lun = HISI_SAS_UNRESERVED_IPTT;
 
diff --git a/drivers/scsi/isci/init.c b/drivers/scsi/isci/init.c
index 9c7869bf6cde..c07d89451bb6 100644
--- a/drivers/scsi/isci/init.c
+++ b/drivers/scsi/isci/init.c
@@ -179,6 +179,7 @@ static struct scsi_host_template isci_sht = {
 	.track_queue_depth		= 1,
 	.reserved_queuecommand = sas_queuecommand_internal,
 	.reserved_timedout = sas_internal_timeout,
+	.nr_reserved_cmds	= 2,
 };
 
 static struct sas_domain_function_template isci_transport_ops  = {
diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index 7fed0259e1f5..07e6c5d6c46d 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -56,6 +56,7 @@ static struct scsi_host_template mvs_sht = {
 	.track_queue_depth	= 1,
 	.reserved_queuecommand = sas_queuecommand_internal,
 	.reserved_timedout = sas_internal_timeout,
+	.nr_reserved_cmds = 2,
 };
 
 static struct sas_domain_function_template mvs_transport_ops = {
@@ -470,6 +471,10 @@ static void  mvs_post_sas_ha_init(struct Scsi_Host *shost,
 	else
 		can_queue = MVS_CHIP_SLOT_SZ;
 
+	/*
+	 * Carve out MVS_RSVD_SLOTS slots internally until every sas_task we're sent
+	 * has a request associated.
+	 */
 	can_queue -= MVS_RSVD_SLOTS;
 
 	shost->sg_tablesize = min_t(u16, SG_ALL, MVS_MAX_SG);
diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index ce9509792bc0..e37e8898afaa 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -125,6 +125,7 @@ static struct scsi_host_template pm8001_sht = {
 	.map_queues		= pm8001_map_queues,
 	.reserved_queuecommand = sas_queuecommand_internal,
 	.reserved_timedout = sas_internal_timeout,
+	.nr_reserved_cmds = 2,
 };
 
 /*
@@ -1214,7 +1215,10 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
 
 	max_out_io = pm8001_ha->main_cfg_tbl.pm80xx_tbl.max_out_io;
 	ccb_count = min_t(int, PM8001_MAX_CCB, max_out_io);
-
+	/*
+	 * Intentionally use ccb_count - PM8001_RESERVE_SLOT for .can_queue
+	 * until every sas_task we're sent has a request associated.
+	 */
 	shost->can_queue = ccb_count - PM8001_RESERVE_SLOT;
 
 	pm8001_ha->rsvd_tags = bitmap_zalloc(PM8001_RESERVE_SLOT, GFP_KERNEL);
-- 
2.35.3

[PATCH RFC v3 18/22] scsi: libsas: Queue SMP commands as requests

[John Garry]

Send SMP commands through the block layer so that each command gets a
unique tag associated.

Function sas_task_complete_internal() is what the LLDD calls to signal
that the CQ is complete and this calls into the SCSI midlayer. And then
sas_blk_end_sync_rq() is called when the request completes.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_expander.c  | 23 ++++++++---------------
 drivers/scsi/libsas/sas_init.c      |  3 +++
 drivers/scsi/libsas/sas_internal.h  |  3 +++
 drivers/scsi/libsas/sas_scsi_host.c | 16 ++++++++++++++++
 4 files changed, 30 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index e7cb95683522..cc41127ea5cc 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -43,34 +43,27 @@ static int smp_execute_task_sg(struct domain_device *dev,
 	pm_runtime_get_sync(ha->dev);
 	mutex_lock(&dev->ex_dev.cmd_mutex);
 	for (retry = 0; retry < 3; retry++) {
+		struct request *rq;
+
 		if (test_bit(SAS_DEV_GONE, &dev->state)) {
 			res = -ECOMM;
 			break;
 		}
 
-		task = sas_alloc_slow_task(GFP_KERNEL);
+		task = sas_alloc_slow_task_rq(dev, GFP_KERNEL);
 		if (!task) {
 			res = -ENOMEM;
 			break;
 		}
-		task->dev = dev;
+
+		rq = scsi_cmd_to_rq(task->uldd_task);
+		rq->timeout = SMP_TIMEOUT*HZ;
+
 		task->task_proto = dev->tproto;
 		task->smp_task.smp_req = *req;
 		task->smp_task.smp_resp = *resp;
 
-		task->task_done = sas_task_internal_done;
-
-		task->slow_task->timer.function = sas_task_internal_timedout;
-		task->slow_task->timer.expires = jiffies + SMP_TIMEOUT*HZ;
-		add_timer(&task->slow_task->timer);
-
-		res = i->dft->lldd_execute_task(task, GFP_KERNEL);
-
-		if (res) {
-			del_timer_sync(&task->slow_task->timer);
-			pr_notice("executing SMP task failed:%d\n", res);
-			break;
-		}
+		blk_execute_rq_nowait(rq, true);
 
 		wait_for_completion(&task->slow_task->completion);
 		res = -ECOMM;
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 90e63ff5e966..5f9e71a54799 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -84,6 +84,7 @@ struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, gfp_t flag
 		return NULL;
 
 	task->dev = device;
+	task->task_done = sas_task_complete_internal;
 
 	rq = scsi_alloc_request(sdev->request_queue, REQ_OP_DRV_IN,
 				BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT);
@@ -95,6 +96,8 @@ struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, gfp_t flag
 	scmd = blk_mq_rq_to_pdu(rq);
 
 	task->uldd_task = scmd;
+
+	rq->end_io = sas_blk_end_sync_rq;
 	rq->end_io_data = task;
 
 	return task;
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index f5ae4de382f7..9b58948c57c2 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -104,6 +104,9 @@ int sas_execute_tmf(struct domain_device *device, void *parameter,
 		    int para_len, int force_phy_id,
 		    struct sas_tmf_task *tmf);
 
+void sas_task_complete_internal(struct sas_task *task);
+enum rq_end_io_ret sas_blk_end_sync_rq(struct request *rq, blk_status_t error);
+
 #ifdef CONFIG_SCSI_SAS_HOST_SMP
 extern void sas_smp_host_handler(struct bsg_job *job, struct Scsi_Host *shost);
 #else
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index b7d1994a8f1b..2c734a87bb7c 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -913,6 +913,13 @@ void sas_task_internal_done(struct sas_task *task)
 	complete(&task->slow_task->completion);
 }
 
+void sas_task_complete_internal(struct sas_task *task)
+{
+	struct scsi_cmnd *scmd = task->uldd_task;
+
+	scsi_done(scmd);
+}
+
 void sas_task_internal_timedout(struct timer_list *t)
 {
 	struct sas_task_slow *slow = from_timer(slow, t, timer);
@@ -952,6 +959,15 @@ EXPORT_SYMBOL_GPL(sas_internal_timeout);
 #define TASK_TIMEOUT			(20 * HZ)
 #define TASK_RETRY			3
 
+enum rq_end_io_ret sas_blk_end_sync_rq(struct request *rq, blk_status_t error)
+{
+	struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(rq);
+	struct sas_task *task = TO_SAS_TASK(scmd);
+	complete(&task->slow_task->completion);
+
+	return RQ_END_IO_NONE;
+}
+
 static int sas_execute_internal_abort(struct domain_device *device,
 				      enum sas_internal_abort type, u16 tag,
 				      unsigned int qid, void *data)
-- 
2.35.3

[PATCH RFC v3 19/22] scsi: libsas: Queue TMF commands as requests

[John Garry]

Like what we did with SMP commands, send TMF commands through the block
layer.

In sas_task_abort() we must ensure to not confuse "TMF" for SATA with
regular SATA command which comes from libata.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_scsi_host.c | 23 ++++++++---------------
 1 file changed, 8 insertions(+), 15 deletions(-)

diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index 2c734a87bb7c..e6ee8dd56a45 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -1067,13 +1067,17 @@ int sas_execute_tmf(struct domain_device *device, void *parameter,
 	int res, retry;
 
 	for (retry = 0; retry < TASK_RETRY; retry++) {
-		task = sas_alloc_slow_task(GFP_KERNEL);
+		struct request *rq;
+
+		task = sas_alloc_slow_task_rq(device, GFP_KERNEL);
 		if (!task)
 			return -ENOMEM;
 
-		task->dev = device;
 		task->task_proto = device->tproto;
 
+		rq = scsi_cmd_to_rq(task->uldd_task);
+		rq->timeout = TASK_TIMEOUT;
+
 		if (dev_is_sata(device)) {
 			task->ata_task.device_control_reg_update = 1;
 			if (force_phy_id >= 0) {
@@ -1085,20 +1089,9 @@ int sas_execute_tmf(struct domain_device *device, void *parameter,
 			memcpy(&task->ssp_task, parameter, para_len);
 		}
 
-		task->task_done = sas_task_internal_done;
 		task->tmf = tmf;
 
-		task->slow_task->timer.function = sas_task_internal_timedout;
-		task->slow_task->timer.expires = jiffies + TASK_TIMEOUT;
-		add_timer(&task->slow_task->timer);
-
-		res = i->dft->lldd_execute_task(task, GFP_KERNEL);
-		if (res) {
-			del_timer_sync(&task->slow_task->timer);
-			pr_err("executing TMF task failed %016llx (%d)\n",
-			       SAS_ADDR(device->sas_addr), res);
-			break;
-		}
+		blk_execute_rq_nowait(rq, true);
 
 		wait_for_completion(&task->slow_task->completion);
 
@@ -1270,7 +1263,7 @@ void sas_task_abort(struct sas_task *task)
 		return;
 	}
 
-	if (dev_is_sata(task->dev))
+	if (dev_is_sata(task->dev) && !task->slow_task)
 		sas_ata_task_abort(task);
 	else
 		blk_abort_request(scsi_cmd_to_rq(sc));
-- 
2.35.3

[PATCH RFC v3 20/22] scsi: core: Add scsi_alloc_request_hwq()

[John Garry]

Add a variant of scsi_alloc_request() which allocates a request for a
specific hw queue.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/scsi_lib.c  | 12 ++++++++++++
 include/scsi/scsi_cmnd.h |  3 +++
 2 files changed, 15 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index 08015c42c326..a622be13bc48 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -1143,6 +1143,18 @@ struct request *scsi_alloc_request(struct request_queue *q, blk_opf_t opf,
 }
 EXPORT_SYMBOL_GPL(scsi_alloc_request);
 
+struct request *scsi_alloc_request_hwq(struct request_queue *q,
+		unsigned int op, blk_mq_req_flags_t flags, unsigned int hwq)
+{
+	struct request *rq;
+
+	rq = blk_mq_alloc_request_hctx(q, op, flags, hwq);
+	if (!IS_ERR(rq))
+		scsi_initialize_rq(rq);
+	return rq;
+}
+EXPORT_SYMBOL_GPL(scsi_alloc_request_hwq);
+
 /*
  * Only called when the request isn't completed by SCSI, and not freed by
  * SCSI
diff --git a/include/scsi/scsi_cmnd.h b/include/scsi/scsi_cmnd.h
index 7d3622db38ed..1fd98d1e9c73 100644
--- a/include/scsi/scsi_cmnd.h
+++ b/include/scsi/scsi_cmnd.h
@@ -389,4 +389,7 @@ extern void scsi_build_sense(struct scsi_cmnd *scmd, int desc,
 struct request *scsi_alloc_request(struct request_queue *q, blk_opf_t opf,
 				   blk_mq_req_flags_t flags);
 
+struct request *scsi_alloc_request_hwq(struct request_queue *q,
+		unsigned int op, blk_mq_req_flags_t flags, unsigned int hwq);
+
 #endif /* _SCSI_SCSI_CMND_H */
-- 
2.35.3

[PATCH RFC v3 21/22] scsi: libsas: Queue internal abort commands as requests

[John Garry]

Like what we did for SMP commands, send internal abort commands through
the block layer.

At this point we can delete special handling in sas_task_abort() for SAS
"internal" commands, as every slow task now has a request associated.

Function sas_task_internal_timedout() is no longer referenced, so delete
it.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 36 +++++++++----------
 drivers/scsi/libsas/sas_expander.c    |  2 +-
 drivers/scsi/libsas/sas_init.c        | 12 +++++--
 drivers/scsi/libsas/sas_internal.h    |  3 +-
 drivers/scsi/libsas/sas_scsi_host.c   | 52 ++++++---------------------
 include/scsi/libsas.h                 |  1 -
 6 files changed, 38 insertions(+), 68 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index fe2752d24fe8..65475775c844 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -465,7 +465,7 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
 	struct hisi_sas_port *port;
 	struct hisi_hba *hisi_hba;
 	struct hisi_sas_slot *slot;
-	struct request *rq = NULL;
+	struct request *rq;
 	struct device *dev;
 	int rc;
 
@@ -485,6 +485,21 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
 
 	hisi_hba = dev_to_hisi_hba(device);
 	dev = hisi_hba->dev;
+	rq = sas_task_find_rq(task);
+	if (rq) {
+		unsigned int dq_index;
+		u32 blk_tag;
+
+		blk_tag = blk_mq_unique_tag(rq);
+		dq_index = blk_mq_unique_tag_to_hwq(blk_tag);
+		dq = &hisi_hba->dq[dq_index];
+	} else {
+		struct Scsi_Host *shost = hisi_hba->shost;
+		struct blk_mq_queue_map *qmap = &shost->tag_set.map[HCTX_TYPE_DEFAULT];
+		int queue = qmap->mq_map[raw_smp_processor_id()];
+
+		dq = &hisi_hba->dq[queue];
+	}
 
 	switch (task->task_proto) {
 	case SAS_PROTOCOL_SSP:
@@ -519,22 +534,6 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
 
 				return -ECOMM;
 		}
-
-		rq = sas_task_find_rq(task);
-		if (rq) {
-			unsigned int dq_index;
-			u32 blk_tag;
-
-			blk_tag = blk_mq_unique_tag(rq);
-			dq_index = blk_mq_unique_tag_to_hwq(blk_tag);
-			dq = &hisi_hba->dq[dq_index];
-		} else {
-			struct Scsi_Host *shost = hisi_hba->shost;
-			struct blk_mq_queue_map *qmap = &shost->tag_set.map[HCTX_TYPE_DEFAULT];
-			int queue = qmap->mq_map[raw_smp_processor_id()];
-
-			dq = &hisi_hba->dq[queue];
-		}
 		break;
 	case SAS_PROTOCOL_INTERNAL_ABORT:
 		if (!hisi_hba->hw->prep_abort)
@@ -543,13 +542,10 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
 		if (test_bit(HISI_SAS_HW_FAULT_BIT, &hisi_hba->flags))
 			return -EIO;
 
-		hisi_hba = dev_to_hisi_hba(device);
-
 		if (unlikely(test_bit(HISI_SAS_REJECT_CMD_BIT, &hisi_hba->flags)))
 			return -EINVAL;
 
 		port = to_hisi_sas_port(sas_port);
-		dq = &hisi_hba->dq[task->abort_task.qid];
 		break;
 	default:
 		dev_err(hisi_hba->dev, "task prep: unknown/unsupported proto (0x%x)\n",
diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
index cc41127ea5cc..e852f1565fe7 100644
--- a/drivers/scsi/libsas/sas_expander.c
+++ b/drivers/scsi/libsas/sas_expander.c
@@ -50,7 +50,7 @@ static int smp_execute_task_sg(struct domain_device *dev,
 			break;
 		}
 
-		task = sas_alloc_slow_task_rq(dev, GFP_KERNEL);
+		task = sas_alloc_slow_task_rq(dev, GFP_KERNEL, -1U);
 		if (!task) {
 			res = -ENOMEM;
 			break;
diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index 5f9e71a54799..c3f602bd2c4c 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -56,7 +56,7 @@ struct sas_task *sas_alloc_slow_task(gfp_t flags)
 	return task;
 }
 
-struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, gfp_t flags)
+struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, gfp_t flags, unsigned int qid)
 {
 	struct sas_ha_struct *sas_ha = device->port->ha;
 	struct Scsi_Host *shost = sas_ha->core.shost;
@@ -86,8 +86,14 @@ struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, gfp_t flag
 	task->dev = device;
 	task->task_done = sas_task_complete_internal;
 
-	rq = scsi_alloc_request(sdev->request_queue, REQ_OP_DRV_IN,
-				BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT);
+	if (qid == -1U) {
+		rq = scsi_alloc_request(sdev->request_queue, REQ_OP_DRV_IN,
+					BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT);
+	} else {
+		rq = scsi_alloc_request_hwq(sdev->request_queue, REQ_OP_DRV_IN,
+					BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT,
+					qid);
+	}
 	if (IS_ERR(rq)) {
 		sas_free_task(task);
 		return NULL;
diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
index 9b58948c57c2..af4a4bf88830 100644
--- a/drivers/scsi/libsas/sas_internal.h
+++ b/drivers/scsi/libsas/sas_internal.h
@@ -54,7 +54,8 @@ void sas_free_event(struct asd_sas_event *event);
 
 struct sas_task *sas_alloc_task(gfp_t flags);
 struct sas_task *sas_alloc_slow_task(gfp_t flags);
-struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, gfp_t flags);
+struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, gfp_t flags,
+				      unsigned int qid);
 void sas_free_task(struct sas_task *task);
 
 int  sas_register_ports(struct sas_ha_struct *sas_ha);
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index e6ee8dd56a45..a93e019a7dbf 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -920,23 +920,6 @@ void sas_task_complete_internal(struct sas_task *task)
 	scsi_done(scmd);
 }
 
-void sas_task_internal_timedout(struct timer_list *t)
-{
-	struct sas_task_slow *slow = from_timer(slow, t, timer);
-	struct sas_task *task = slow->task;
-	bool is_completed = true;
-	unsigned long flags;
-
-	spin_lock_irqsave(&task->task_state_lock, flags);
-	if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
-		task->task_state_flags |= SAS_TASK_STATE_ABORTED;
-		is_completed = false;
-	}
-	spin_unlock_irqrestore(&task->task_state_lock, flags);
-
-	if (!is_completed)
-		complete(&task->slow_task->completion);
-}
 enum blk_eh_timer_return sas_internal_timeout(struct scsi_cmnd *scmd)
 {
 	struct sas_task *task = TO_SAS_TASK(scmd);
@@ -978,28 +961,23 @@ static int sas_execute_internal_abort(struct domain_device *device,
 	int res, retry;
 
 	for (retry = 0; retry < TASK_RETRY; retry++) {
-		task = sas_alloc_slow_task(GFP_KERNEL);
+		struct request *rq;
+
+		task = sas_alloc_slow_task_rq(device, GFP_KERNEL, qid);
 		if (!task)
 			return -ENOMEM;
 
 		task->dev = device;
 		task->task_proto = SAS_PROTOCOL_INTERNAL_ABORT;
-		task->task_done = sas_task_internal_done;
-		task->slow_task->timer.function = sas_task_internal_timedout;
-		task->slow_task->timer.expires = jiffies + TASK_TIMEOUT;
-		add_timer(&task->slow_task->timer);
+		task->task_done = sas_task_complete_internal;
 
 		task->abort_task.tag = tag;
 		task->abort_task.type = type;
-		task->abort_task.qid = qid;
 
-		res = i->dft->lldd_execute_task(task, GFP_KERNEL);
-		if (res) {
-			del_timer_sync(&task->slow_task->timer);
-			pr_err("Executing internal abort failed %016llx (%d)\n",
-			       SAS_ADDR(device->sas_addr), res);
-			break;
-		}
+		rq = scsi_cmd_to_rq(task->uldd_task);
+		rq->timeout = TASK_TIMEOUT;
+
+		blk_execute_rq_nowait(rq, true);
 
 		wait_for_completion(&task->slow_task->completion);
 		res = TMF_RESP_FUNC_FAILED;
@@ -1069,7 +1047,7 @@ int sas_execute_tmf(struct domain_device *device, void *parameter,
 	for (retry = 0; retry < TASK_RETRY; retry++) {
 		struct request *rq;
 
-		task = sas_alloc_slow_task_rq(device, GFP_KERNEL);
+		task = sas_alloc_slow_task_rq(device, GFP_KERNEL, -1U);
 		if (!task)
 			return -ENOMEM;
 
@@ -1251,17 +1229,7 @@ void sas_task_abort(struct sas_task *task)
 {
 	struct scsi_cmnd *sc = task->uldd_task;
 
-	/* Escape for libsas internal commands */
-	if (!sc) {
-		struct sas_task_slow *slow = task->slow_task;
-
-		if (!slow)
-			return;
-		if (!del_timer(&slow->timer))
-			return;
-		slow->timer.function(&slow->timer);
-		return;
-	}
+	WARN_ON_ONCE(!sc);
 
 	if (dev_is_sata(task->dev) && !task->slow_task)
 		sas_ata_task_abort(task);
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index f02156ccd376..60543d8b01d4 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -565,7 +565,6 @@ enum sas_internal_abort {
 
 struct sas_internal_abort_task {
 	enum sas_internal_abort type;
-	unsigned int qid;
 	u16 tag;
 };
 
-- 
2.35.3

[PATCH RFC v3 22/22] scsi: libsas: Delete sas_task_slow.timer

[John Garry]

We now send every slow task through the block layer and use the timeout
facility there, so delete sas_task_slow.timer .

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/libsas/sas_init.c      | 1 -
 drivers/scsi/libsas/sas_scsi_host.c | 1 -
 include/scsi/libsas.h               | 3 ---
 3 files changed, 5 deletions(-)

diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
index c3f602bd2c4c..8b0e72c447d0 100644
--- a/drivers/scsi/libsas/sas_init.c
+++ b/drivers/scsi/libsas/sas_init.c
@@ -50,7 +50,6 @@ struct sas_task *sas_alloc_slow_task(gfp_t flags)
 
 	task->slow_task = slow;
 	slow->task = task;
-	timer_setup(&slow->timer, NULL, 0);
 	init_completion(&slow->completion);
 
 	return task;
diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
index a93e019a7dbf..4fdd84868ac2 100644
--- a/drivers/scsi/libsas/sas_scsi_host.c
+++ b/drivers/scsi/libsas/sas_scsi_host.c
@@ -909,7 +909,6 @@ EXPORT_SYMBOL_GPL(sas_bios_param);
 
 void sas_task_internal_done(struct sas_task *task)
 {
-	del_timer(&task->slow_task->timer);
 	complete(&task->slow_task->completion);
 }
 
diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index 60543d8b01d4..f903be5895a9 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -9,8 +9,6 @@
 #ifndef _LIBSAS_H_
 #define _LIBSAS_H_
 
-
-#include <linux/timer.h>
 #include <linux/pci.h>
 #include <scsi/sas.h>
 #include <linux/libata.h>
@@ -628,7 +626,6 @@ struct sas_task_slow {
 	/* standard/extra infrastructure for slow path commands (SMP and
 	 * internal lldd commands
 	 */
-	struct timer_list     timer;
 	struct completion     completion;
 	struct sas_task       *task;
 };
-- 
2.35.3

Re: [PATCH RFC v3 01/22] blk-mq: Don't get budget for reserved requests

[Damien Le Moal]

On 10/25/22 19:17, John Garry wrote:
> It should be possible to send reserved requests even when there is no
> budget, so don't request a budget in that case.
> 
> This comes into play when we need to allocate a reserved request from the
> target device request queue for error handling for that same device.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  block/blk-mq.c          | 4 +++-
>  drivers/scsi/scsi_lib.c | 3 ++-
>  2 files changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/block/blk-mq.c b/block/blk-mq.c
> index 260adeb2e455..d8baabb32ea4 100644
> --- a/block/blk-mq.c
> +++ b/block/blk-mq.c
> @@ -1955,11 +1955,13 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list,
>  	errors = queued = 0;
>  	do {
>  		struct blk_mq_queue_data bd;
> +		bool need_budget;
>  
>  		rq = list_first_entry(list, struct request, queuelist);
>  
>  		WARN_ON_ONCE(hctx != rq->mq_hctx);
> -		prep = blk_mq_prep_dispatch_rq(rq, !nr_budgets);
> +		need_budget = !nr_budgets && !blk_mq_is_reserved_rq(rq);
> +		prep = blk_mq_prep_dispatch_rq(rq, need_budget);
>  		if (prep != PREP_DISPATCH_OK)
>  			break;

Below this code, there is:

		if (nr_budgets)
			nr_budgets--;

Don't you need to change that to:

		if (need_budget && nr_budgets)
			nr_budgets--;

? Otherwise, the accounting will be off.

>  
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index fa96d3cfdfa3..39d4fd124375 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -298,7 +298,8 @@ void scsi_device_unbusy(struct scsi_device *sdev, struct scsi_cmnd *cmd)
>  	if (starget->can_queue > 0)
>  		atomic_dec(&starget->target_busy);
>  
> -	sbitmap_put(&sdev->budget_map, cmd->budget_token);
> +	if (!blk_mq_is_reserved_rq(scsi_cmd_to_rq(cmd)))
> +		sbitmap_put(&sdev->budget_map, cmd->budget_token);
>  	cmd->budget_token = -1;
>  }
>  

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH RFC v3 03/22] scsi: core: Implement reserved command handling

[Damien Le Moal]

On 10/25/22 19:17, John Garry wrote:
> From: Hannes Reinecke <hare@suse.de>
> 
> 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>
> #jpg: Set tag_set->queue_depth = shost->can_queue, and not
> = shost->can_queue + shost->nr_reserved_cmds;
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/scsi/hosts.c     |  3 +++
>  drivers/scsi/scsi_lib.c  |  2 ++
>  include/scsi/scsi_host.h | 15 ++++++++++++++-
>  3 files changed, 19 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index 12346e2297fd..db89afc37bc9 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -489,6 +489,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;
> +

Nit: the if is not really necessary I think. But it does not hurt.

>  	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 39d4fd124375..a8c4e7c037ae 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1978,6 +1978,8 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
>  	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->reserved_tags = shost->nr_reserved_cmds;
> +

Why the blank line ?

>  	tag_set->cmd_size = cmd_size;
>  	tag_set->numa_node = dev_to_node(shost->dma_dev);
>  	tag_set->flags = BLK_MQ_F_SHOULD_MERGE;
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 750ccf126377..91678c77398e 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -360,10 +360,17 @@ 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
> +	 * including reserved commands.
>  	 */
>  	int can_queue;
>  
> +	/*
> +	 * This determines how many commands the HBA will set aside
> +	 * for reserved commands.
> +	 */
> +	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
> @@ -611,6 +618,12 @@ struct Scsi_Host {
>  	 */
>  	unsigned nr_hw_queues;
>  	unsigned nr_maps;
> +
> +	/*
> +	 * Number of reserved commands to allocate, if any.
> +	 */
> +	unsigned int nr_reserved_cmds;
> +
>  	unsigned active_mode:2;
>  
>  	/*

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH RFC v3 04/22] scsi: core: Add support to send reserved commands

[Damien Le Moal]

On 10/25/22 19:17, John Garry wrote:
> Add a method to queue reserved commands.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/scsi/hosts.c     |  6 ++++++
>  drivers/scsi/scsi_lib.c  | 25 +++++++++++++++++++++++++
>  include/scsi/scsi_host.h |  1 +
>  3 files changed, 32 insertions(+)
> 
> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
> index db89afc37bc9..78968553089f 100644
> --- a/drivers/scsi/hosts.c
> +++ b/drivers/scsi/hosts.c
> @@ -230,6 +230,12 @@ int scsi_add_host_with_dma(struct Scsi_Host *shost, struct device *dev,
>  		goto fail;
>  	}
>  
> +	if (shost->nr_reserved_cmds && !sht->reserved_queuecommand) {
> +		shost_printk(KERN_ERR, shost,
> +			"nr_reserved_cmds set but no method to queue\n");
> +		goto fail;
> +	}
> +
>  	/* Use min_t(int, ...) in case shost->can_queue exceeds SHRT_MAX */
>  	shost->cmd_per_lun = min_t(int, shost->cmd_per_lun,
>  				   shost->can_queue);
> diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
> index a8c4e7c037ae..08015c42c326 100644
> --- a/drivers/scsi/scsi_lib.c
> +++ b/drivers/scsi/scsi_lib.c
> @@ -1428,6 +1428,16 @@ static void scsi_complete(struct request *rq)
>  	struct scsi_cmnd *cmd = blk_mq_rq_to_pdu(rq);
>  	enum scsi_disposition disposition;
>  
> +	if (blk_mq_is_reserved_rq(rq)) {
> +		struct scsi_device *sdev = cmd->device;

This variable is not really needed. You can call:
		
		scsi_device_unbusy(cmd->device, cmd);

No ?

> +
> +		scsi_mq_uninit_cmd(cmd);
> +		scsi_device_unbusy(sdev, cmd);
> +		__blk_mq_end_request(rq, 0);
> +
> +		return;
> +	}
> +
>  	INIT_LIST_HEAD(&cmd->eh_entry);
>  
>  	atomic_inc(&cmd->device->iodone_cnt);
> @@ -1718,6 +1728,21 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>  	blk_status_t ret;
>  	int reason;
>  
> +	if (blk_mq_is_reserved_rq(req)) {
> +		if (!(req->rq_flags & RQF_DONTPREP)) {
> +			ret = scsi_prepare_cmd(req);
> +			if (ret != BLK_STS_OK)
> +				goto out_dec_host_busy;
> +
> +			req->rq_flags |= RQF_DONTPREP;
> +		} else {
> +			clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
> +		}
> +		blk_mq_start_request(req);
> +
> +		return shost->hostt->reserved_queuecommand(shost, cmd);
> +	}
> +
>  	WARN_ON_ONCE(cmd->budget_token < 0);
>  
>  	/*
> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
> index 91678c77398e..a39f36aa0b0d 100644
> --- a/include/scsi/scsi_host.h
> +++ b/include/scsi/scsi_host.h
> @@ -73,6 +73,7 @@ struct scsi_host_template {
>  	 * STATUS: REQUIRED
>  	 */
>  	int (* queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
> +	int (*reserved_queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);

Nit: This op name sound like something returning a bool... May be a
straight "queue_reserved_command" name would be clearer ?

>  
>  	/*
>  	 * The commit_rqs function is used to trigger a hardware

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH RFC v3 16/22] ata: libata-scsi: Allocate sdev early in port probe

[Damien Le Moal]

On 10/25/22 19:18, John Garry wrote:
> Currently the per-ata device sdev is allocated as part of the scsi target
> scan, which is after the ata port probe.
> 
> However it is useful to have the sdev available in the port probe. As an
> example of an advantage, if the request queue is available in the probe
> (which it would be if the sdev is available), then it is possible to use
> a SCSI cmnd for ATA internal commands. The benefit of this is then we can
> put the ATA qc structure in the SCSI cmnd private data. It will also be
> useful if we want to send ATA internal commands as requests.
> 
> Export scsi_target_reap() so that it can be used to put the extra
> reference we get when allocating the sdev.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/ata/libata-eh.c   |  1 +
>  drivers/ata/libata-scsi.c | 23 +++++++++--------------
>  drivers/scsi/scsi_scan.c  |  1 +
>  3 files changed, 11 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
> index 08e11bc312c2..1ed5b1b64792 100644
> --- a/drivers/ata/libata-eh.c
> +++ b/drivers/ata/libata-eh.c
> @@ -3446,6 +3446,7 @@ static int ata_eh_schedule_probe(struct ata_device *dev)
>  
>  	ata_eh_detach_dev(dev);
>  	ata_dev_init(dev);
> +	ata_scsi_setup_sdev(dev);
>  	ehc->did_probe_mask |= (1 << dev->devno);
>  	ehc->i.action |= ATA_EH_RESET;
>  	ehc->saved_xfer_mode[dev->devno] = 0;
> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
> index efdba852e363..476e0ef4bd29 100644
> --- a/drivers/ata/libata-scsi.c
> +++ b/drivers/ata/libata-scsi.c
> @@ -1109,7 +1109,12 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)
>  	if (dev->flags & ATA_DFLAG_TRUSTED)
>  		sdev->security_supported = 1;
>  
> -	dev->sdev = sdev;
> +	/*
> +	 * Put extra reference which we get when allocating the starget
> +	 * initially
> +	 */
> +	scsi_target_reap(scsi_target(sdev));
> +
>  	return 0;
>  }
>  
> @@ -4289,26 +4294,16 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
>   repeat:
>  	ata_for_each_link(link, ap, EDGE) {
>  		ata_for_each_dev(dev, link, ENABLED) {
> -			struct scsi_device *sdev;
> +			struct Scsi_Host *shost = ap->scsi_host;
>  			int channel = 0, id = 0;
>  
> -			if (dev->sdev)
> -				continue;
> -
>  			if (ata_is_host_link(link))
>  				id = dev->devno;
>  			else
>  				channel = link->pmp;
>  
> -			sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
> -						 NULL);
> -			if (!IS_ERR(sdev)) {
> -				dev->sdev = sdev;
> -				ata_scsi_assign_ofnode(dev, ap);

Is there something equivalent to what this function does inside
scsi_scan_target() ? I had a quick look but did not see anything...

> -				scsi_device_put(sdev);
> -			} else {
> -				dev->sdev = NULL;
> -			}
> +			scsi_scan_target(&shost->shost_gendev, channel, id,
> +					 0, SCSI_SCAN_INITIAL);
>  		}
>  	}
>  
> diff --git a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
> index b795c138f2c1..da7bc14b030c 100644
> --- a/drivers/scsi/scsi_scan.c
> +++ b/drivers/scsi/scsi_scan.c
> @@ -598,6 +598,7 @@ void scsi_target_reap(struct scsi_target *starget)
>  	BUG_ON(starget->state == STARGET_DEL);
>  	scsi_target_reap_ref_put(starget);
>  }
> +EXPORT_SYMBOL_GPL(scsi_target_reap);
>  
>  /**
>   * scsi_sanitize_inquiry_string - remove non-graphical chars from an

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH RFC v3 18/22] scsi: libsas: Queue SMP commands as requests

[Damien Le Moal]

On 10/25/22 19:18, John Garry wrote:
> Send SMP commands through the block layer so that each command gets a
> unique tag associated.
> 
> Function sas_task_complete_internal() is what the LLDD calls to signal
> that the CQ is complete and this calls into the SCSI midlayer. And then
> sas_blk_end_sync_rq() is called when the request completes.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/scsi/libsas/sas_expander.c  | 23 ++++++++---------------
>  drivers/scsi/libsas/sas_init.c      |  3 +++
>  drivers/scsi/libsas/sas_internal.h  |  3 +++
>  drivers/scsi/libsas/sas_scsi_host.c | 16 ++++++++++++++++
>  4 files changed, 30 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index e7cb95683522..cc41127ea5cc 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -43,34 +43,27 @@ static int smp_execute_task_sg(struct domain_device *dev,
>  	pm_runtime_get_sync(ha->dev);
>  	mutex_lock(&dev->ex_dev.cmd_mutex);
>  	for (retry = 0; retry < 3; retry++) {
> +		struct request *rq;
> +
>  		if (test_bit(SAS_DEV_GONE, &dev->state)) {
>  			res = -ECOMM;
>  			break;
>  		}
>  
> -		task = sas_alloc_slow_task(GFP_KERNEL);
> +		task = sas_alloc_slow_task_rq(dev, GFP_KERNEL);
>  		if (!task) {
>  			res = -ENOMEM;
>  			break;
>  		}
> -		task->dev = dev;
> +
> +		rq = scsi_cmd_to_rq(task->uldd_task);
> +		rq->timeout = SMP_TIMEOUT*HZ;

Missing spaces around "*"

> +
>  		task->task_proto = dev->tproto;
>  		task->smp_task.smp_req = *req;
>  		task->smp_task.smp_resp = *resp;
>  
> -		task->task_done = sas_task_internal_done;
> -
> -		task->slow_task->timer.function = sas_task_internal_timedout;
> -		task->slow_task->timer.expires = jiffies + SMP_TIMEOUT*HZ;
> -		add_timer(&task->slow_task->timer);
> -
> -		res = i->dft->lldd_execute_task(task, GFP_KERNEL);
> -
> -		if (res) {
> -			del_timer_sync(&task->slow_task->timer);
> -			pr_notice("executing SMP task failed:%d\n", res);
> -			break;
> -		}
> +		blk_execute_rq_nowait(rq, true);
>  
>  		wait_for_completion(&task->slow_task->completion);
>  		res = -ECOMM;
> diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
> index 90e63ff5e966..5f9e71a54799 100644
> --- a/drivers/scsi/libsas/sas_init.c
> +++ b/drivers/scsi/libsas/sas_init.c
> @@ -84,6 +84,7 @@ struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, gfp_t flag
>  		return NULL;
>  
>  	task->dev = device;
> +	task->task_done = sas_task_complete_internal;
>  
>  	rq = scsi_alloc_request(sdev->request_queue, REQ_OP_DRV_IN,
>  				BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT);
> @@ -95,6 +96,8 @@ struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, gfp_t flag
>  	scmd = blk_mq_rq_to_pdu(rq);
>  
>  	task->uldd_task = scmd;
> +
> +	rq->end_io = sas_blk_end_sync_rq;
>  	rq->end_io_data = task;
>  
>  	return task;
> diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
> index f5ae4de382f7..9b58948c57c2 100644
> --- a/drivers/scsi/libsas/sas_internal.h
> +++ b/drivers/scsi/libsas/sas_internal.h
> @@ -104,6 +104,9 @@ int sas_execute_tmf(struct domain_device *device, void *parameter,
>  		    int para_len, int force_phy_id,
>  		    struct sas_tmf_task *tmf);
>  
> +void sas_task_complete_internal(struct sas_task *task);
> +enum rq_end_io_ret sas_blk_end_sync_rq(struct request *rq, blk_status_t error);
> +
>  #ifdef CONFIG_SCSI_SAS_HOST_SMP
>  extern void sas_smp_host_handler(struct bsg_job *job, struct Scsi_Host *shost);
>  #else
> diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
> index b7d1994a8f1b..2c734a87bb7c 100644
> --- a/drivers/scsi/libsas/sas_scsi_host.c
> +++ b/drivers/scsi/libsas/sas_scsi_host.c
> @@ -913,6 +913,13 @@ void sas_task_internal_done(struct sas_task *task)
>  	complete(&task->slow_task->completion);
>  }
>  
> +void sas_task_complete_internal(struct sas_task *task)
> +{
> +	struct scsi_cmnd *scmd = task->uldd_task;
> +
> +	scsi_done(scmd);
> +}
> +
>  void sas_task_internal_timedout(struct timer_list *t)
>  {
>  	struct sas_task_slow *slow = from_timer(slow, t, timer);
> @@ -952,6 +959,15 @@ EXPORT_SYMBOL_GPL(sas_internal_timeout);
>  #define TASK_TIMEOUT			(20 * HZ)
>  #define TASK_RETRY			3
>  
> +enum rq_end_io_ret sas_blk_end_sync_rq(struct request *rq, blk_status_t error)
> +{
> +	struct scsi_cmnd *scmd = blk_mq_rq_to_pdu(rq);
> +	struct sas_task *task = TO_SAS_TASK(scmd);
> +	complete(&task->slow_task->completion);
> +
> +	return RQ_END_IO_NONE;
> +}
> +
>  static int sas_execute_internal_abort(struct domain_device *device,
>  				      enum sas_internal_abort type, u16 tag,
>  				      unsigned int qid, void *data)

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH RFC v3 03/22] scsi: core: Implement reserved command handling

[Hannes Reinecke]

On 10/27/22 03:18, Damien Le Moal wrote:
> On 10/25/22 19:17, John Garry wrote:
>> From: Hannes Reinecke <hare@suse.de>
>>
>> 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>
>> #jpg: Set tag_set->queue_depth = shost->can_queue, and not
>> = shost->can_queue + shost->nr_reserved_cmds;
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
>>   drivers/scsi/hosts.c     |  3 +++
>>   drivers/scsi/scsi_lib.c  |  2 ++
>>   include/scsi/scsi_host.h | 15 ++++++++++++++-
>>   3 files changed, 19 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>> index 12346e2297fd..db89afc37bc9 100644
>> --- a/drivers/scsi/hosts.c
>> +++ b/drivers/scsi/hosts.c
>> @@ -489,6 +489,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;
>> +
> 
> Nit: the if is not really necessary I think. But it does not hurt.
> 
Yes, we do.
Not all HBAs are able to figure out the number of reserved commands 
upfront; some modify that based on the PCI device used etc.
So I'd keep it for now.

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: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH RFC v3 16/22] ata: libata-scsi: Allocate sdev early in port probe

[Hannes Reinecke]

On 10/27/22 03:34, Damien Le Moal wrote:
> On 10/25/22 19:18, John Garry wrote:
>> Currently the per-ata device sdev is allocated as part of the scsi target
>> scan, which is after the ata port probe.
>>
>> However it is useful to have the sdev available in the port probe. As an
>> example of an advantage, if the request queue is available in the probe
>> (which it would be if the sdev is available), then it is possible to use
>> a SCSI cmnd for ATA internal commands. The benefit of this is then we can
>> put the ATA qc structure in the SCSI cmnd private data. It will also be
>> useful if we want to send ATA internal commands as requests.
>>
>> Export scsi_target_reap() so that it can be used to put the extra
>> reference we get when allocating the sdev.
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
>>   drivers/ata/libata-eh.c   |  1 +
>>   drivers/ata/libata-scsi.c | 23 +++++++++--------------
>>   drivers/scsi/scsi_scan.c  |  1 +
>>   3 files changed, 11 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>> index 08e11bc312c2..1ed5b1b64792 100644
>> --- a/drivers/ata/libata-eh.c
>> +++ b/drivers/ata/libata-eh.c
>> @@ -3446,6 +3446,7 @@ static int ata_eh_schedule_probe(struct ata_device *dev)
>>   
>>   	ata_eh_detach_dev(dev);
>>   	ata_dev_init(dev);
>> +	ata_scsi_setup_sdev(dev);
>>   	ehc->did_probe_mask |= (1 << dev->devno);
>>   	ehc->i.action |= ATA_EH_RESET;
>>   	ehc->saved_xfer_mode[dev->devno] = 0;
>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>> index efdba852e363..476e0ef4bd29 100644
>> --- a/drivers/ata/libata-scsi.c
>> +++ b/drivers/ata/libata-scsi.c
>> @@ -1109,7 +1109,12 @@ int ata_scsi_dev_config(struct scsi_device *sdev, struct ata_device *dev)
>>   	if (dev->flags & ATA_DFLAG_TRUSTED)
>>   		sdev->security_supported = 1;
>>   
>> -	dev->sdev = sdev;
>> +	/*
>> +	 * Put extra reference which we get when allocating the starget
>> +	 * initially
>> +	 */
>> +	scsi_target_reap(scsi_target(sdev));
>> +
>>   	return 0;
>>   }
>>   
>> @@ -4289,26 +4294,16 @@ void ata_scsi_scan_host(struct ata_port *ap, int sync)
>>    repeat:
>>   	ata_for_each_link(link, ap, EDGE) {
>>   		ata_for_each_dev(dev, link, ENABLED) {
>> -			struct scsi_device *sdev;
>> +			struct Scsi_Host *shost = ap->scsi_host;
>>   			int channel = 0, id = 0;
>>   
>> -			if (dev->sdev)
>> -				continue;
>> -
>>   			if (ata_is_host_link(link))
>>   				id = dev->devno;
>>   			else
>>   				channel = link->pmp;
>>   
>> -			sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
>> -						 NULL);
>> -			if (!IS_ERR(sdev)) {
>> -				dev->sdev = sdev;
>> -				ata_scsi_assign_ofnode(dev, ap);
> 
> Is there something equivalent to what this function does inside
> scsi_scan_target() ? I had a quick look but did not see anything...
> 
Typically, the SCSI layer has two ways of scanning.
One it the old-style serial scanning (originating in the old SCSI 
parallel model):
The scanning code will blindly scan _all_ devices up to max_luns, and 
attach every device for which the scanning code returns 'OK'.
The other one is to issue REPORT_LUNS and scan all LUNs returned there.

Mapped to libata we would need to figure out a stable SCSI enumeration, 
given that we have PMP and slave devices to content with.
To my knowledge we have ATA ports, each can have either a 'master' and 
'slave' device, _or_ it be a PMP port when it can support up to 16 
devices, right?
Point being, master/slave and PMP are exclusive, right?
So we can make the master as LUN 0, and the slave as LUN 1.
And for PMP we can use each PMP address as LUN <pmp> + 1, and keeping 
the actual device as LUN 0.

I think we can figure out whether it's a master/slave device setup 
upfront, so we should be able to set max_luns to '2' for these devices.
For PMP-capable (or devices which _might_ be PMP capable), we could 
emulate the REPORT LUNS command, mapping on the PMP mechanism to figure 
out which devices are connected.

Would that work?

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: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH RFC v3 03/22] scsi: core: Implement reserved command handling

[John Garry]

On 27/10/2022 08:51, Hannes Reinecke wrote:
>>>
>>> Signed-off-by: Hannes Reinecke <hare@suse.de>
>>> #jpg: Set tag_set->queue_depth = shost->can_queue, and not
>>> = shost->can_queue + shost->nr_reserved_cmds;
>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>> ---
>>>   drivers/scsi/hosts.c     |  3 +++
>>>   drivers/scsi/scsi_lib.c  |  2 ++
>>>   include/scsi/scsi_host.h | 15 ++++++++++++++-
>>>   3 files changed, 19 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/scsi/hosts.c b/drivers/scsi/hosts.c
>>> index 12346e2297fd..db89afc37bc9 100644
>>> --- a/drivers/scsi/hosts.c
>>> +++ b/drivers/scsi/hosts.c
>>> @@ -489,6 +489,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;
>>> +
>>
>> Nit: the if is not really necessary I think. But it does not hurt.
>>
> Yes, we do.
> Not all HBAs are able to figure out the number of reserved commands 
> upfront; some modify that based on the PCI device used etc.
> So I'd keep it for now.

I think logically Damien is right as in the shost alloc 
shost->nr_reserved_cmds is initially zero, so:

if (sht->nr_reserved_cmds)
        shost->nr_reserved_cmds = sht->nr_reserved_cmds;

is same as simply:

	shost->nr_reserved_cmds = sht->nr_reserved_cmds;

However I am just copying the coding style.

Thanks,
John

Re: [PATCH RFC v3 01/22] blk-mq: Don't get budget for reserved requests

[John Garry]

On 27/10/2022 02:16, Damien Le Moal wrote:
>> Signed-off-by: John Garry<john.garry@huawei.com>
>> ---
>>   block/blk-mq.c          | 4 +++-
>>   drivers/scsi/scsi_lib.c | 3 ++-
>>   2 files changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/block/blk-mq.c b/block/blk-mq.c
>> index 260adeb2e455..d8baabb32ea4 100644
>> --- a/block/blk-mq.c
>> +++ b/block/blk-mq.c
>> @@ -1955,11 +1955,13 @@ bool blk_mq_dispatch_rq_list(struct blk_mq_hw_ctx *hctx, struct list_head *list,
>>   	errors = queued = 0;
>>   	do {
>>   		struct blk_mq_queue_data bd;
>> +		bool need_budget;
>>   
>>   		rq = list_first_entry(list, struct request, queuelist);
>>   
>>   		WARN_ON_ONCE(hctx != rq->mq_hctx);
>> -		prep = blk_mq_prep_dispatch_rq(rq, !nr_budgets);
>> +		need_budget = !nr_budgets && !blk_mq_is_reserved_rq(rq);
>> +		prep = blk_mq_prep_dispatch_rq(rq, need_budget);
>>   		if (prep != PREP_DISPATCH_OK)
>>   			break;
> Below this code, there is:
> 
> 		if (nr_budgets)
> 			nr_budgets--;
> 
> Don't you need to change that to:
> 
> 		if (need_budget && nr_budgets)
> 			nr_budgets--;
> 
> ? Otherwise, the accounting will be off.
> 

Ah, yes, I think that you are right. I actually need to check nr_budgets 
usage further as nr_budgets initial value would be dependent on any 
reserved request requiring a budget (which we don't get).

Thanks,
John

Re: [PATCH RFC v3 03/22] scsi: core: Implement reserved command handling

[John Garry]

> 
>>   	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 39d4fd124375..a8c4e7c037ae 100644
>> --- a/drivers/scsi/scsi_lib.c
>> +++ b/drivers/scsi/scsi_lib.c
>> @@ -1978,6 +1978,8 @@ int scsi_mq_setup_tags(struct Scsi_Host *shost)
>>   	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->reserved_tags = shost->nr_reserved_cmds;
>> +
> Why the blank line ?
> 

I don't think that it is required, I can remedy.

>>   	tag_set->cmd_size = cmd_size;

Thanks,
John

Re: [PATCH RFC v3 04/22] scsi: core: Add support to send reserved commands

[John Garry]

On 27/10/2022 02:21, Damien Le Moal wrote:
>>   
>> +	if (blk_mq_is_reserved_rq(rq)) {
>> +		struct scsi_device *sdev = cmd->device;
> This variable is not really needed. You can call:
> 		
> 		scsi_device_unbusy(cmd->device, cmd);
> 
> No ?

ok, your suggestion is good

> 
>> +
>> +		scsi_mq_uninit_cmd(cmd);
>> +		scsi_device_unbusy(sdev, cmd);
>> +		__blk_mq_end_request(rq, 0);
>> +
>> +		return;
>> +	}
>> +
>>   	INIT_LIST_HEAD(&cmd->eh_entry);
>>   
>>   	atomic_inc(&cmd->device->iodone_cnt);
>> @@ -1718,6 +1728,21 @@ static blk_status_t scsi_queue_rq(struct blk_mq_hw_ctx *hctx,
>>   	blk_status_t ret;
>>   	int reason;
>>   
>> +	if (blk_mq_is_reserved_rq(req)) {
>> +		if (!(req->rq_flags & RQF_DONTPREP)) {
>> +			ret = scsi_prepare_cmd(req);
>> +			if (ret != BLK_STS_OK)
>> +				goto out_dec_host_busy;
>> +
>> +			req->rq_flags |= RQF_DONTPREP;
>> +		} else {
>> +			clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
>> +		}
>> +		blk_mq_start_request(req);
>> +
>> +		return shost->hostt->reserved_queuecommand(shost, cmd);
>> +	}
>> +
>>   	WARN_ON_ONCE(cmd->budget_token < 0);
>>   
>>   	/*
>> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
>> index 91678c77398e..a39f36aa0b0d 100644
>> --- a/include/scsi/scsi_host.h
>> +++ b/include/scsi/scsi_host.h
>> @@ -73,6 +73,7 @@ struct scsi_host_template {
>>   	 * STATUS: REQUIRED
>>   	 */
>>   	int (* queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
>> +	int (*reserved_queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
> Nit: This op name sound like something returning a bool... May be a
> straight "queue_reserved_command" name would be clearer ?

or queuecommand_reserved ? I'm just trying to have the name a variant of 
"queuecommand".

> 

thanks,
John

Re: [PATCH RFC v3 16/22] ata: libata-scsi: Allocate sdev early in port probe

[Damien Le Moal]

On 10/27/22 17:11, Hannes Reinecke wrote:
> On 10/27/22 03:34, Damien Le Moal wrote:
>> On 10/25/22 19:18, John Garry wrote:
>>> Currently the per-ata device sdev is allocated as part of the scsi
>>> target
>>> scan, which is after the ata port probe.
>>>
>>> However it is useful to have the sdev available in the port probe. As an
>>> example of an advantage, if the request queue is available in the probe
>>> (which it would be if the sdev is available), then it is possible to use
>>> a SCSI cmnd for ATA internal commands. The benefit of this is then we
>>> can
>>> put the ATA qc structure in the SCSI cmnd private data. It will also be
>>> useful if we want to send ATA internal commands as requests.
>>>
>>> Export scsi_target_reap() so that it can be used to put the extra
>>> reference we get when allocating the sdev.
>>>
>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>> ---
>>>   drivers/ata/libata-eh.c   |  1 +
>>>   drivers/ata/libata-scsi.c | 23 +++++++++--------------
>>>   drivers/scsi/scsi_scan.c  |  1 +
>>>   3 files changed, 11 insertions(+), 14 deletions(-)
>>>
>>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>>> index 08e11bc312c2..1ed5b1b64792 100644
>>> --- a/drivers/ata/libata-eh.c
>>> +++ b/drivers/ata/libata-eh.c
>>> @@ -3446,6 +3446,7 @@ static int ata_eh_schedule_probe(struct
>>> ata_device *dev)
>>>         ata_eh_detach_dev(dev);
>>>       ata_dev_init(dev);
>>> +    ata_scsi_setup_sdev(dev);
>>>       ehc->did_probe_mask |= (1 << dev->devno);
>>>       ehc->i.action |= ATA_EH_RESET;
>>>       ehc->saved_xfer_mode[dev->devno] = 0;
>>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>>> index efdba852e363..476e0ef4bd29 100644
>>> --- a/drivers/ata/libata-scsi.c
>>> +++ b/drivers/ata/libata-scsi.c
>>> @@ -1109,7 +1109,12 @@ int ata_scsi_dev_config(struct scsi_device
>>> *sdev, struct ata_device *dev)
>>>       if (dev->flags & ATA_DFLAG_TRUSTED)
>>>           sdev->security_supported = 1;
>>>   -    dev->sdev = sdev;
>>> +    /*
>>> +     * Put extra reference which we get when allocating the starget
>>> +     * initially
>>> +     */
>>> +    scsi_target_reap(scsi_target(sdev));
>>> +
>>>       return 0;
>>>   }
>>>   @@ -4289,26 +4294,16 @@ void ata_scsi_scan_host(struct ata_port
>>> *ap, int sync)
>>>    repeat:
>>>       ata_for_each_link(link, ap, EDGE) {
>>>           ata_for_each_dev(dev, link, ENABLED) {
>>> -            struct scsi_device *sdev;
>>> +            struct Scsi_Host *shost = ap->scsi_host;
>>>               int channel = 0, id = 0;
>>>   -            if (dev->sdev)
>>> -                continue;
>>> -
>>>               if (ata_is_host_link(link))
>>>                   id = dev->devno;
>>>               else
>>>                   channel = link->pmp;
>>>   -            sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
>>> -                         NULL);
>>> -            if (!IS_ERR(sdev)) {
>>> -                dev->sdev = sdev;
>>> -                ata_scsi_assign_ofnode(dev, ap);
>>
>> Is there something equivalent to what this function does inside
>> scsi_scan_target() ? I had a quick look but did not see anything...
>>
> Typically, the SCSI layer has two ways of scanning.
> One it the old-style serial scanning (originating in the old SCSI
> parallel model):
> The scanning code will blindly scan _all_ devices up to max_luns, and
> attach every device for which the scanning code returns 'OK'.
> The other one is to issue REPORT_LUNS and scan all LUNs returned there.
> 
> Mapped to libata we would need to figure out a stable SCSI enumeration,
> given that we have PMP and slave devices to content with.
> To my knowledge we have ATA ports, each can have either a 'master' and
> 'slave' device, _or_ it be a PMP port when it can support up to 16
> devices, right?

yes

> Point being, master/slave and PMP are exclusive, right?

Never heard of pmp with ide cable :)

> So we can make the master as LUN 0, and the slave as LUN 1.

Yes, but isn't that a little wrong ? That would assume that the ata port
is the device and the ata devices the luns of that device. But beside
the "link busy" stuff that needs to be dealt with, master and slave are
independent devices, unlike LUNs. No ?

> And for PMP we can use each PMP address as LUN <pmp> + 1, and keeping
> the actual device as LUN 0.
> 
> I think we can figure out whether it's a master/slave device setup
> upfront, so we should be able to set max_luns to '2' for these devices.
> For PMP-capable (or devices which _might_ be PMP capable), we could
> emulate the REPORT LUNS command, mapping on the PMP mechanism to figure
> out which devices are connected.
> 
> Would that work?
> 
> Cheers,
> 
> Hannes

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH RFC v3 04/22] scsi: core: Add support to send reserved commands

[Damien Le Moal]

On 10/27/22 18:13, John Garry wrote:
> On 27/10/2022 02:21, Damien Le Moal wrote:
>>>   +    if (blk_mq_is_reserved_rq(rq)) {
>>> +        struct scsi_device *sdev = cmd->device;
>> This variable is not really needed. You can call:
>>        
>>         scsi_device_unbusy(cmd->device, cmd);
>>
>> No ?
> 
> ok, your suggestion is good
> 
>>
>>> +
>>> +        scsi_mq_uninit_cmd(cmd);
>>> +        scsi_device_unbusy(sdev, cmd);
>>> +        __blk_mq_end_request(rq, 0);
>>> +
>>> +        return;
>>> +    }
>>> +
>>>       INIT_LIST_HEAD(&cmd->eh_entry);
>>>         atomic_inc(&cmd->device->iodone_cnt);
>>> @@ -1718,6 +1728,21 @@ static blk_status_t scsi_queue_rq(struct
>>> blk_mq_hw_ctx *hctx,
>>>       blk_status_t ret;
>>>       int reason;
>>>   +    if (blk_mq_is_reserved_rq(req)) {
>>> +        if (!(req->rq_flags & RQF_DONTPREP)) {
>>> +            ret = scsi_prepare_cmd(req);
>>> +            if (ret != BLK_STS_OK)
>>> +                goto out_dec_host_busy;
>>> +
>>> +            req->rq_flags |= RQF_DONTPREP;
>>> +        } else {
>>> +            clear_bit(SCMD_STATE_COMPLETE, &cmd->state);
>>> +        }
>>> +        blk_mq_start_request(req);
>>> +
>>> +        return shost->hostt->reserved_queuecommand(shost, cmd);
>>> +    }
>>> +
>>>       WARN_ON_ONCE(cmd->budget_token < 0);
>>>         /*
>>> diff --git a/include/scsi/scsi_host.h b/include/scsi/scsi_host.h
>>> index 91678c77398e..a39f36aa0b0d 100644
>>> --- a/include/scsi/scsi_host.h
>>> +++ b/include/scsi/scsi_host.h
>>> @@ -73,6 +73,7 @@ struct scsi_host_template {
>>>        * STATUS: REQUIRED
>>>        */
>>>       int (* queuecommand)(struct Scsi_Host *, struct scsi_cmnd *);
>>> +    int (*reserved_queuecommand)(struct Scsi_Host *, struct
>>> scsi_cmnd *);
>> Nit: This op name sound like something returning a bool... May be a
>> straight "queue_reserved_command" name would be clearer ?
> 
> or queuecommand_reserved ? I'm just trying to have the name a variant of
> "queuecommand".

I figured that :)
queuereservedcommand ? (hard to read...)
queuecommand_reserved is OK I guess.

> 
>>
> 
> thanks,
> John

-- 
Damien Le Moal
Western Digital Research

Re: [PATCH RFC v3 16/22] ata: libata-scsi: Allocate sdev early in port probe

[Hannes Reinecke]

On 10/27/22 11:16, Damien Le Moal wrote:
> On 10/27/22 17:11, Hannes Reinecke wrote:
>> On 10/27/22 03:34, Damien Le Moal wrote:
>>> On 10/25/22 19:18, John Garry wrote:
>>>> Currently the per-ata device sdev is allocated as part of the scsi
>>>> target
>>>> scan, which is after the ata port probe.
>>>>
>>>> However it is useful to have the sdev available in the port probe. As an
>>>> example of an advantage, if the request queue is available in the probe
>>>> (which it would be if the sdev is available), then it is possible to use
>>>> a SCSI cmnd for ATA internal commands. The benefit of this is then we
>>>> can
>>>> put the ATA qc structure in the SCSI cmnd private data. It will also be
>>>> useful if we want to send ATA internal commands as requests.
>>>>
>>>> Export scsi_target_reap() so that it can be used to put the extra
>>>> reference we get when allocating the sdev.
>>>>
>>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>>> ---
>>>>    drivers/ata/libata-eh.c   |  1 +
>>>>    drivers/ata/libata-scsi.c | 23 +++++++++--------------
>>>>    drivers/scsi/scsi_scan.c  |  1 +
>>>>    3 files changed, 11 insertions(+), 14 deletions(-)
>>>>
>>>> diff --git a/drivers/ata/libata-eh.c b/drivers/ata/libata-eh.c
>>>> index 08e11bc312c2..1ed5b1b64792 100644
>>>> --- a/drivers/ata/libata-eh.c
>>>> +++ b/drivers/ata/libata-eh.c
>>>> @@ -3446,6 +3446,7 @@ static int ata_eh_schedule_probe(struct
>>>> ata_device *dev)
>>>>          ata_eh_detach_dev(dev);
>>>>        ata_dev_init(dev);
>>>> +    ata_scsi_setup_sdev(dev);
>>>>        ehc->did_probe_mask |= (1 << dev->devno);
>>>>        ehc->i.action |= ATA_EH_RESET;
>>>>        ehc->saved_xfer_mode[dev->devno] = 0;
>>>> diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
>>>> index efdba852e363..476e0ef4bd29 100644
>>>> --- a/drivers/ata/libata-scsi.c
>>>> +++ b/drivers/ata/libata-scsi.c
>>>> @@ -1109,7 +1109,12 @@ int ata_scsi_dev_config(struct scsi_device
>>>> *sdev, struct ata_device *dev)
>>>>        if (dev->flags & ATA_DFLAG_TRUSTED)
>>>>            sdev->security_supported = 1;
>>>>    -    dev->sdev = sdev;
>>>> +    /*
>>>> +     * Put extra reference which we get when allocating the starget
>>>> +     * initially
>>>> +     */
>>>> +    scsi_target_reap(scsi_target(sdev));
>>>> +
>>>>        return 0;
>>>>    }
>>>>    @@ -4289,26 +4294,16 @@ void ata_scsi_scan_host(struct ata_port
>>>> *ap, int sync)
>>>>     repeat:
>>>>        ata_for_each_link(link, ap, EDGE) {
>>>>            ata_for_each_dev(dev, link, ENABLED) {
>>>> -            struct scsi_device *sdev;
>>>> +            struct Scsi_Host *shost = ap->scsi_host;
>>>>                int channel = 0, id = 0;
>>>>    -            if (dev->sdev)
>>>> -                continue;
>>>> -
>>>>                if (ata_is_host_link(link))
>>>>                    id = dev->devno;
>>>>                else
>>>>                    channel = link->pmp;
>>>>    -            sdev = __scsi_add_device(ap->scsi_host, channel, id, 0,
>>>> -                         NULL);
>>>> -            if (!IS_ERR(sdev)) {
>>>> -                dev->sdev = sdev;
>>>> -                ata_scsi_assign_ofnode(dev, ap);
>>>
>>> Is there something equivalent to what this function does inside
>>> scsi_scan_target() ? I had a quick look but did not see anything...
>>>
>> Typically, the SCSI layer has two ways of scanning.
>> One it the old-style serial scanning (originating in the old SCSI
>> parallel model):
>> The scanning code will blindly scan _all_ devices up to max_luns, and
>> attach every device for which the scanning code returns 'OK'.
>> The other one is to issue REPORT_LUNS and scan all LUNs returned there.
>>
>> Mapped to libata we would need to figure out a stable SCSI enumeration,
>> given that we have PMP and slave devices to content with.
>> To my knowledge we have ATA ports, each can have either a 'master' and
>> 'slave' device, _or_ it be a PMP port when it can support up to 16
>> devices, right?
> 
> yes
> 
>> Point being, master/slave and PMP are exclusive, right?
> 
> Never heard of pmp with ide cable :)
> 
See?

>> So we can make the master as LUN 0, and the slave as LUN 1.
> 
> Yes, but isn't that a little wrong ? That would assume that the ata port
> is the device and the ata devices the luns of that device. But beside
> the "link busy" stuff that needs to be dealt with, master and slave are
> independent devices, unlike LUNs. No ?
> Well; technically, yes.

But we already enumerate the ata ports (which is typically done by the 
hardware/PCI layer etc), and if we were try to model slave devices as 
independent ports we would either have to insert a numbering (awkward) 
or add a number at the en (even more awkward).

And the one key takeaway from the 'multiple actuators' discussion is 
that LUNs _are_ independent (cf all the hoops they had to jump through 
to define a command spanning several LUNs ...)(which, incidentally, we 
could leverage here, too ...), and the target port really only serves as 
an enumeration thingie to address the LUNs.

So we _could_ map the master device on LUN 0 and the slave device on LUN 
1 with no loss of functionality, _but_ enable a consistent SCSI enumeration.

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: Ivo Totev, Andrew
Myers, Andrew McDonald, Martje Boudien Moerman

Re: [PATCH RFC v3 18/22] scsi: libsas: Queue SMP commands as requests

[John Garry]

On 27/10/2022 02:36, Damien Le Moal wrote:
>> +		rq = scsi_cmd_to_rq(task->uldd_task);
>> +		rq->timeout = SMP_TIMEOUT*HZ;
> Missing spaces around "*"
> 

ok

Thanks,
John

Re: [PATCH RFC v3 21/22] scsi: libsas: Queue internal abort commands as requests

[chenxiang (M)]

Hi John,

For internal abort commands, it allocates and deliver requests through 
sdev->request_queue.

Is it possible that we still need to send internal abort commands even 
if sdev is freed?

I  notices that in sas_destruct_devices, it calls sas_rphy_delete() to 
remove target, and then call i->dft->lldd_dev_gone()

which will call internal abort commands.



在 2022/10/25 18:18, John Garry 写道:
> Like what we did for SMP commands, send internal abort commands through
> the block layer.
>
> At this point we can delete special handling in sas_task_abort() for SAS
> "internal" commands, as every slow task now has a request associated.
>
> Function sas_task_internal_timedout() is no longer referenced, so delete
> it.
>
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/scsi/hisi_sas/hisi_sas_main.c | 36 +++++++++----------
>   drivers/scsi/libsas/sas_expander.c    |  2 +-
>   drivers/scsi/libsas/sas_init.c        | 12 +++++--
>   drivers/scsi/libsas/sas_internal.h    |  3 +-
>   drivers/scsi/libsas/sas_scsi_host.c   | 52 ++++++---------------------
>   include/scsi/libsas.h                 |  1 -
>   6 files changed, 38 insertions(+), 68 deletions(-)
>
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index fe2752d24fe8..65475775c844 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -465,7 +465,7 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
>   	struct hisi_sas_port *port;
>   	struct hisi_hba *hisi_hba;
>   	struct hisi_sas_slot *slot;
> -	struct request *rq = NULL;
> +	struct request *rq;
>   	struct device *dev;
>   	int rc;
>   
> @@ -485,6 +485,21 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
>   
>   	hisi_hba = dev_to_hisi_hba(device);
>   	dev = hisi_hba->dev;
> +	rq = sas_task_find_rq(task);
> +	if (rq) {
> +		unsigned int dq_index;
> +		u32 blk_tag;
> +
> +		blk_tag = blk_mq_unique_tag(rq);
> +		dq_index = blk_mq_unique_tag_to_hwq(blk_tag);
> +		dq = &hisi_hba->dq[dq_index];
> +	} else {
> +		struct Scsi_Host *shost = hisi_hba->shost;
> +		struct blk_mq_queue_map *qmap = &shost->tag_set.map[HCTX_TYPE_DEFAULT];
> +		int queue = qmap->mq_map[raw_smp_processor_id()];
> +
> +		dq = &hisi_hba->dq[queue];
> +	}
>   
>   	switch (task->task_proto) {
>   	case SAS_PROTOCOL_SSP:
> @@ -519,22 +534,6 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
>   
>   				return -ECOMM;
>   		}
> -
> -		rq = sas_task_find_rq(task);
> -		if (rq) {
> -			unsigned int dq_index;
> -			u32 blk_tag;
> -
> -			blk_tag = blk_mq_unique_tag(rq);
> -			dq_index = blk_mq_unique_tag_to_hwq(blk_tag);
> -			dq = &hisi_hba->dq[dq_index];
> -		} else {
> -			struct Scsi_Host *shost = hisi_hba->shost;
> -			struct blk_mq_queue_map *qmap = &shost->tag_set.map[HCTX_TYPE_DEFAULT];
> -			int queue = qmap->mq_map[raw_smp_processor_id()];
> -
> -			dq = &hisi_hba->dq[queue];
> -		}
>   		break;
>   	case SAS_PROTOCOL_INTERNAL_ABORT:
>   		if (!hisi_hba->hw->prep_abort)
> @@ -543,13 +542,10 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
>   		if (test_bit(HISI_SAS_HW_FAULT_BIT, &hisi_hba->flags))
>   			return -EIO;
>   
> -		hisi_hba = dev_to_hisi_hba(device);
> -
>   		if (unlikely(test_bit(HISI_SAS_REJECT_CMD_BIT, &hisi_hba->flags)))
>   			return -EINVAL;
>   
>   		port = to_hisi_sas_port(sas_port);
> -		dq = &hisi_hba->dq[task->abort_task.qid];
>   		break;
>   	default:
>   		dev_err(hisi_hba->dev, "task prep: unknown/unsupported proto (0x%x)\n",
> diff --git a/drivers/scsi/libsas/sas_expander.c b/drivers/scsi/libsas/sas_expander.c
> index cc41127ea5cc..e852f1565fe7 100644
> --- a/drivers/scsi/libsas/sas_expander.c
> +++ b/drivers/scsi/libsas/sas_expander.c
> @@ -50,7 +50,7 @@ static int smp_execute_task_sg(struct domain_device *dev,
>   			break;
>   		}
>   
> -		task = sas_alloc_slow_task_rq(dev, GFP_KERNEL);
> +		task = sas_alloc_slow_task_rq(dev, GFP_KERNEL, -1U);
>   		if (!task) {
>   			res = -ENOMEM;
>   			break;
> diff --git a/drivers/scsi/libsas/sas_init.c b/drivers/scsi/libsas/sas_init.c
> index 5f9e71a54799..c3f602bd2c4c 100644
> --- a/drivers/scsi/libsas/sas_init.c
> +++ b/drivers/scsi/libsas/sas_init.c
> @@ -56,7 +56,7 @@ struct sas_task *sas_alloc_slow_task(gfp_t flags)
>   	return task;
>   }
>   
> -struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, gfp_t flags)
> +struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, gfp_t flags, unsigned int qid)
>   {
>   	struct sas_ha_struct *sas_ha = device->port->ha;
>   	struct Scsi_Host *shost = sas_ha->core.shost;
> @@ -86,8 +86,14 @@ struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, gfp_t flag
>   	task->dev = device;
>   	task->task_done = sas_task_complete_internal;
>   
> -	rq = scsi_alloc_request(sdev->request_queue, REQ_OP_DRV_IN,
> -				BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT);
> +	if (qid == -1U) {
> +		rq = scsi_alloc_request(sdev->request_queue, REQ_OP_DRV_IN,
> +					BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT);
> +	} else {
> +		rq = scsi_alloc_request_hwq(sdev->request_queue, REQ_OP_DRV_IN,
> +					BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT,
> +					qid);
> +	}
>   	if (IS_ERR(rq)) {
>   		sas_free_task(task);
>   		return NULL;
> diff --git a/drivers/scsi/libsas/sas_internal.h b/drivers/scsi/libsas/sas_internal.h
> index 9b58948c57c2..af4a4bf88830 100644
> --- a/drivers/scsi/libsas/sas_internal.h
> +++ b/drivers/scsi/libsas/sas_internal.h
> @@ -54,7 +54,8 @@ void sas_free_event(struct asd_sas_event *event);
>   
>   struct sas_task *sas_alloc_task(gfp_t flags);
>   struct sas_task *sas_alloc_slow_task(gfp_t flags);
> -struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, gfp_t flags);
> +struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, gfp_t flags,
> +				      unsigned int qid);
>   void sas_free_task(struct sas_task *task);
>   
>   int  sas_register_ports(struct sas_ha_struct *sas_ha);
> diff --git a/drivers/scsi/libsas/sas_scsi_host.c b/drivers/scsi/libsas/sas_scsi_host.c
> index e6ee8dd56a45..a93e019a7dbf 100644
> --- a/drivers/scsi/libsas/sas_scsi_host.c
> +++ b/drivers/scsi/libsas/sas_scsi_host.c
> @@ -920,23 +920,6 @@ void sas_task_complete_internal(struct sas_task *task)
>   	scsi_done(scmd);
>   }
>   
> -void sas_task_internal_timedout(struct timer_list *t)
> -{
> -	struct sas_task_slow *slow = from_timer(slow, t, timer);
> -	struct sas_task *task = slow->task;
> -	bool is_completed = true;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&task->task_state_lock, flags);
> -	if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
> -		task->task_state_flags |= SAS_TASK_STATE_ABORTED;
> -		is_completed = false;
> -	}
> -	spin_unlock_irqrestore(&task->task_state_lock, flags);
> -
> -	if (!is_completed)
> -		complete(&task->slow_task->completion);
> -}
>   enum blk_eh_timer_return sas_internal_timeout(struct scsi_cmnd *scmd)
>   {
>   	struct sas_task *task = TO_SAS_TASK(scmd);
> @@ -978,28 +961,23 @@ static int sas_execute_internal_abort(struct domain_device *device,
>   	int res, retry;
>   
>   	for (retry = 0; retry < TASK_RETRY; retry++) {
> -		task = sas_alloc_slow_task(GFP_KERNEL);
> +		struct request *rq;
> +
> +		task = sas_alloc_slow_task_rq(device, GFP_KERNEL, qid);
>   		if (!task)
>   			return -ENOMEM;
>   
>   		task->dev = device;
>   		task->task_proto = SAS_PROTOCOL_INTERNAL_ABORT;
> -		task->task_done = sas_task_internal_done;
> -		task->slow_task->timer.function = sas_task_internal_timedout;
> -		task->slow_task->timer.expires = jiffies + TASK_TIMEOUT;
> -		add_timer(&task->slow_task->timer);
> +		task->task_done = sas_task_complete_internal;
>   
>   		task->abort_task.tag = tag;
>   		task->abort_task.type = type;
> -		task->abort_task.qid = qid;
>   
> -		res = i->dft->lldd_execute_task(task, GFP_KERNEL);
> -		if (res) {
> -			del_timer_sync(&task->slow_task->timer);
> -			pr_err("Executing internal abort failed %016llx (%d)\n",
> -			       SAS_ADDR(device->sas_addr), res);
> -			break;
> -		}
> +		rq = scsi_cmd_to_rq(task->uldd_task);
> +		rq->timeout = TASK_TIMEOUT;
> +
> +		blk_execute_rq_nowait(rq, true);
>   
>   		wait_for_completion(&task->slow_task->completion);
>   		res = TMF_RESP_FUNC_FAILED;
> @@ -1069,7 +1047,7 @@ int sas_execute_tmf(struct domain_device *device, void *parameter,
>   	for (retry = 0; retry < TASK_RETRY; retry++) {
>   		struct request *rq;
>   
> -		task = sas_alloc_slow_task_rq(device, GFP_KERNEL);
> +		task = sas_alloc_slow_task_rq(device, GFP_KERNEL, -1U);
>   		if (!task)
>   			return -ENOMEM;
>   
> @@ -1251,17 +1229,7 @@ void sas_task_abort(struct sas_task *task)
>   {
>   	struct scsi_cmnd *sc = task->uldd_task;
>   
> -	/* Escape for libsas internal commands */
> -	if (!sc) {
> -		struct sas_task_slow *slow = task->slow_task;
> -
> -		if (!slow)
> -			return;
> -		if (!del_timer(&slow->timer))
> -			return;
> -		slow->timer.function(&slow->timer);
> -		return;
> -	}
> +	WARN_ON_ONCE(!sc);
>   
>   	if (dev_is_sata(task->dev) && !task->slow_task)
>   		sas_ata_task_abort(task);
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index f02156ccd376..60543d8b01d4 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -565,7 +565,6 @@ enum sas_internal_abort {
>   
>   struct sas_internal_abort_task {
>   	enum sas_internal_abort type;
> -	unsigned int qid;
>   	u16 tag;
>   };
>   

Re: [PATCH RFC v3 21/22] scsi: libsas: Queue internal abort commands as requests

[John Garry]

On 29/10/2022 02:15, chenxiang (M) wrote:
> Hi John,
> 
> For internal abort commands, it allocates and deliver requests through 
> sdev->request_queue.
> 
> Is it possible that we still need to send internal abort commands even 
> if sdev is freed?
> 
> I  notices that in sas_destruct_devices, it calls sas_rphy_delete() to 
> remove target, and then call i->dft->lldd_dev_gone()
> 
> which will call internal abort commands.

Good question. I did not properly check the removal path and, indeed, 
the sdev would be gone when we call lldd_dev_gone -> 
internal_task_abort_dev, so that should fail to execute.

However I do wonder why we really need to call internal_task_abort_dev 
in the lldd_dev_gone callback. At the point lldd_dev_gone is called all 
IO should be complete - indeed, it is now accounted for as requests 
associated with the sdev. If it is really required to be called (HW 
requirement?) then maybe it could be relocated to sdev/starget teardown 
callback.

Thanks,
John

> 
> 
> 
> 在 2022/10/25 18:18, John Garry 写道:
>> Like what we did for SMP commands, send internal abort commands through
>> the block layer.
>>
>> At this point we can delete special handling in sas_task_abort() for SAS
>> "internal" commands, as every slow task now has a request associated.
>>
>> Function sas_task_internal_timedout() is no longer referenced, so delete
>> it.
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
>>   drivers/scsi/hisi_sas/hisi_sas_main.c | 36 +++++++++----------
>>   drivers/scsi/libsas/sas_expander.c    |  2 +-
>>   drivers/scsi/libsas/sas_init.c        | 12 +++++--
>>   drivers/scsi/libsas/sas_internal.h    |  3 +-
>>   drivers/scsi/libsas/sas_scsi_host.c   | 52 ++++++---------------------
>>   include/scsi/libsas.h                 |  1 -
>>   6 files changed, 38 insertions(+), 68 deletions(-)
>>
>> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c 
>> b/drivers/scsi/hisi_sas/hisi_sas_main.c
>> index fe2752d24fe8..65475775c844 100644
>> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
>> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
>> @@ -465,7 +465,7 @@ static int hisi_sas_queue_command(struct sas_task 
>> *task, gfp_t gfp_flags)
>>       struct hisi_sas_port *port;
>>       struct hisi_hba *hisi_hba;
>>       struct hisi_sas_slot *slot;
>> -    struct request *rq = NULL;
>> +    struct request *rq;
>>       struct device *dev;
>>       int rc;
>> @@ -485,6 +485,21 @@ static int hisi_sas_queue_command(struct sas_task 
>> *task, gfp_t gfp_flags)
>>       hisi_hba = dev_to_hisi_hba(device);
>>       dev = hisi_hba->dev;
>> +    rq = sas_task_find_rq(task);
>> +    if (rq) {
>> +        unsigned int dq_index;
>> +        u32 blk_tag;
>> +
>> +        blk_tag = blk_mq_unique_tag(rq);
>> +        dq_index = blk_mq_unique_tag_to_hwq(blk_tag);
>> +        dq = &hisi_hba->dq[dq_index];
>> +    } else {
>> +        struct Scsi_Host *shost = hisi_hba->shost;
>> +        struct blk_mq_queue_map *qmap = 
>> &shost->tag_set.map[HCTX_TYPE_DEFAULT];
>> +        int queue = qmap->mq_map[raw_smp_processor_id()];
>> +
>> +        dq = &hisi_hba->dq[queue];
>> +    }
>>       switch (task->task_proto) {
>>       case SAS_PROTOCOL_SSP:
>> @@ -519,22 +534,6 @@ static int hisi_sas_queue_command(struct sas_task 
>> *task, gfp_t gfp_flags)
>>                   return -ECOMM;
>>           }
>> -
>> -        rq = sas_task_find_rq(task);
>> -        if (rq) {
>> -            unsigned int dq_index;
>> -            u32 blk_tag;
>> -
>> -            blk_tag = blk_mq_unique_tag(rq);
>> -            dq_index = blk_mq_unique_tag_to_hwq(blk_tag);
>> -            dq = &hisi_hba->dq[dq_index];
>> -        } else {
>> -            struct Scsi_Host *shost = hisi_hba->shost;
>> -            struct blk_mq_queue_map *qmap = 
>> &shost->tag_set.map[HCTX_TYPE_DEFAULT];
>> -            int queue = qmap->mq_map[raw_smp_processor_id()];
>> -
>> -            dq = &hisi_hba->dq[queue];
>> -        }
>>           break;
>>       case SAS_PROTOCOL_INTERNAL_ABORT:
>>           if (!hisi_hba->hw->prep_abort)
>> @@ -543,13 +542,10 @@ static int hisi_sas_queue_command(struct 
>> sas_task *task, gfp_t gfp_flags)
>>           if (test_bit(HISI_SAS_HW_FAULT_BIT, &hisi_hba->flags))
>>               return -EIO;
>> -        hisi_hba = dev_to_hisi_hba(device);
>> -
>>           if (unlikely(test_bit(HISI_SAS_REJECT_CMD_BIT, 
>> &hisi_hba->flags)))
>>               return -EINVAL;
>>           port = to_hisi_sas_port(sas_port);
>> -        dq = &hisi_hba->dq[task->abort_task.qid];
>>           break;
>>       default:
>>           dev_err(hisi_hba->dev, "task prep: unknown/unsupported proto 
>> (0x%x)\n",
>> diff --git a/drivers/scsi/libsas/sas_expander.c 
>> b/drivers/scsi/libsas/sas_expander.c
>> index cc41127ea5cc..e852f1565fe7 100644
>> --- a/drivers/scsi/libsas/sas_expander.c
>> +++ b/drivers/scsi/libsas/sas_expander.c
>> @@ -50,7 +50,7 @@ static int smp_execute_task_sg(struct domain_device 
>> *dev,
>>               break;
>>           }
>> -        task = sas_alloc_slow_task_rq(dev, GFP_KERNEL);
>> +        task = sas_alloc_slow_task_rq(dev, GFP_KERNEL, -1U);
>>           if (!task) {
>>               res = -ENOMEM;
>>               break;
>> diff --git a/drivers/scsi/libsas/sas_init.c 
>> b/drivers/scsi/libsas/sas_init.c
>> index 5f9e71a54799..c3f602bd2c4c 100644
>> --- a/drivers/scsi/libsas/sas_init.c
>> +++ b/drivers/scsi/libsas/sas_init.c
>> @@ -56,7 +56,7 @@ struct sas_task *sas_alloc_slow_task(gfp_t flags)
>>       return task;
>>   }
>> -struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, 
>> gfp_t flags)
>> +struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, 
>> gfp_t flags, unsigned int qid)
>>   {
>>       struct sas_ha_struct *sas_ha = device->port->ha;
>>       struct Scsi_Host *shost = sas_ha->core.shost;
>> @@ -86,8 +86,14 @@ struct sas_task *sas_alloc_slow_task_rq(struct 
>> domain_device *device, gfp_t flag
>>       task->dev = device;
>>       task->task_done = sas_task_complete_internal;
>> -    rq = scsi_alloc_request(sdev->request_queue, REQ_OP_DRV_IN,
>> -                BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT);
>> +    if (qid == -1U) {
>> +        rq = scsi_alloc_request(sdev->request_queue, REQ_OP_DRV_IN,
>> +                    BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT);
>> +    } else {
>> +        rq = scsi_alloc_request_hwq(sdev->request_queue, REQ_OP_DRV_IN,
>> +                    BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT,
>> +                    qid);
>> +    }
>>       if (IS_ERR(rq)) {
>>           sas_free_task(task);
>>           return NULL;
>> diff --git a/drivers/scsi/libsas/sas_internal.h 
>> b/drivers/scsi/libsas/sas_internal.h
>> index 9b58948c57c2..af4a4bf88830 100644
>> --- a/drivers/scsi/libsas/sas_internal.h
>> +++ b/drivers/scsi/libsas/sas_internal.h
>> @@ -54,7 +54,8 @@ void sas_free_event(struct asd_sas_event *event);
>>   struct sas_task *sas_alloc_task(gfp_t flags);
>>   struct sas_task *sas_alloc_slow_task(gfp_t flags);
>> -struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, 
>> gfp_t flags);
>> +struct sas_task *sas_alloc_slow_task_rq(struct domain_device *device, 
>> gfp_t flags,
>> +                      unsigned int qid);
>>   void sas_free_task(struct sas_task *task);
>>   int  sas_register_ports(struct sas_ha_struct *sas_ha);
>> diff --git a/drivers/scsi/libsas/sas_scsi_host.c 
>> b/drivers/scsi/libsas/sas_scsi_host.c
>> index e6ee8dd56a45..a93e019a7dbf 100644
>> --- a/drivers/scsi/libsas/sas_scsi_host.c
>> +++ b/drivers/scsi/libsas/sas_scsi_host.c
>> @@ -920,23 +920,6 @@ void sas_task_complete_internal(struct sas_task 
>> *task)
>>       scsi_done(scmd);
>>   }
>> -void sas_task_internal_timedout(struct timer_list *t)
>> -{
>> -    struct sas_task_slow *slow = from_timer(slow, t, timer);
>> -    struct sas_task *task = slow->task;
>> -    bool is_completed = true;
>> -    unsigned long flags;
>> -
>> -    spin_lock_irqsave(&task->task_state_lock, flags);
>> -    if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
>> -        task->task_state_flags |= SAS_TASK_STATE_ABORTED;
>> -        is_completed = false;
>> -    }
>> -    spin_unlock_irqrestore(&task->task_state_lock, flags);
>> -
>> -    if (!is_completed)
>> -        complete(&task->slow_task->completion);
>> -}
>>   enum blk_eh_timer_return sas_internal_timeout(struct scsi_cmnd *scmd)
>>   {
>>       struct sas_task *task = TO_SAS_TASK(scmd);
>> @@ -978,28 +961,23 @@ static int sas_execute_internal_abort(struct 
>> domain_device *device,
>>       int res, retry;
>>       for (retry = 0; retry < TASK_RETRY; retry++) {
>> -        task = sas_alloc_slow_task(GFP_KERNEL);
>> +        struct request *rq;
>> +
>> +        task = sas_alloc_slow_task_rq(device, GFP_KERNEL, qid);
>>           if (!task)
>>               return -ENOMEM;
>>           task->dev = device;
>>           task->task_proto = SAS_PROTOCOL_INTERNAL_ABORT;
>> -        task->task_done = sas_task_internal_done;
>> -        task->slow_task->timer.function = sas_task_internal_timedout;
>> -        task->slow_task->timer.expires = jiffies + TASK_TIMEOUT;
>> -        add_timer(&task->slow_task->timer);
>> +        task->task_done = sas_task_complete_internal;
>>           task->abort_task.tag = tag;
>>           task->abort_task.type = type;
>> -        task->abort_task.qid = qid;
>> -        res = i->dft->lldd_execute_task(task, GFP_KERNEL);
>> -        if (res) {
>> -            del_timer_sync(&task->slow_task->timer);
>> -            pr_err("Executing internal abort failed %016llx (%d)\n",
>> -                   SAS_ADDR(device->sas_addr), res);
>> -            break;
>> -        }
>> +        rq = scsi_cmd_to_rq(task->uldd_task);
>> +        rq->timeout = TASK_TIMEOUT;
>> +
>> +        blk_execute_rq_nowait(rq, true);
>>           wait_for_completion(&task->slow_task->completion);
>>           res = TMF_RESP_FUNC_FAILED;
>> @@ -1069,7 +1047,7 @@ int sas_execute_tmf(struct domain_device 
>> *device, void *parameter,
>>       for (retry = 0; retry < TASK_RETRY; retry++) {
>>           struct request *rq;
>> -        task = sas_alloc_slow_task_rq(device, GFP_KERNEL);
>> +        task = sas_alloc_slow_task_rq(device, GFP_KERNEL, -1U);
>>           if (!task)
>>               return -ENOMEM;
>> @@ -1251,17 +1229,7 @@ void sas_task_abort(struct sas_task *task)
>>   {
>>       struct scsi_cmnd *sc = task->uldd_task;
>> -    /* Escape for libsas internal commands */
>> -    if (!sc) {
>> -        struct sas_task_slow *slow = task->slow_task;
>> -
>> -        if (!slow)
>> -            return;
>> -        if (!del_timer(&slow->timer))
>> -            return;
>> -        slow->timer.function(&slow->timer);
>> -        return;
>> -    }
>> +    WARN_ON_ONCE(!sc);
>>       if (dev_is_sata(task->dev) && !task->slow_task)
>>           sas_ata_task_abort(task);
>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
>> index f02156ccd376..60543d8b01d4 100644
>> --- a/include/scsi/libsas.h
>> +++ b/include/scsi/libsas.h
>> @@ -565,7 +565,6 @@ enum sas_internal_abort {
>>   struct sas_internal_abort_task {
>>       enum sas_internal_abort type;
>> -    unsigned int qid;
>>       u16 tag;
>>   };
> 
> 
> 

Re: [PATCH RFC v3 21/22] scsi: libsas: Queue internal abort commands as requests

[chenxiang (M)]

在 2022/11/2 18:04, John Garry 写道:
> On 29/10/2022 02:15, chenxiang (M) wrote:
>> Hi John,
>>
>> For internal abort commands, it allocates and deliver requests 
>> through sdev->request_queue.
>>
>> Is it possible that we still need to send internal abort commands 
>> even if sdev is freed?
>>
>> I  notices that in sas_destruct_devices, it calls sas_rphy_delete() 
>> to remove target, and then call i->dft->lldd_dev_gone()
>>
>> which will call internal abort commands.
>
> Good question. I did not properly check the removal path and, indeed, 
> the sdev would be gone when we call lldd_dev_gone -> 
> internal_task_abort_dev, so that should fail to execute.
>
> However I do wonder why we really need to call internal_task_abort_dev 
> in the lldd_dev_gone callback. At the point lldd_dev_gone is called 
> all IO should be complete - indeed, it is now accounted for as 
> requests associated with the sdev. If it is really required to be 
> called (HW requirement?) then maybe it could be relocated to 
> sdev/starget teardown callback.

Call internal abort in lldd_dev_gone callback comes from the commit 
(40f2702b57eb16ef "scsi: hisi_sas: add internal abort in 
hisi_sas_dev_gone()"), and it seems not descript why adding it.
But those internal IOs are sent to SAS controller to do something in it, 
and it is not depend on scsi device. Like SMP IO, it allocates 
scsi_device for expander, why not allocate a scsi_device for controller?

>
> Thanks,
> John
>
>>
>>
>>
>> 在 2022/10/25 18:18, John Garry 写道:
>>> Like what we did for SMP commands, send internal abort commands through
>>> the block layer.
>>>
>>> At this point we can delete special handling in sas_task_abort() for 
>>> SAS
>>> "internal" commands, as every slow task now has a request associated.
>>>
>>> Function sas_task_internal_timedout() is no longer referenced, so 
>>> delete
>>> it.
>>>
>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>> ---
>>>   drivers/scsi/hisi_sas/hisi_sas_main.c | 36 +++++++++----------
>>>   drivers/scsi/libsas/sas_expander.c    |  2 +-
>>>   drivers/scsi/libsas/sas_init.c        | 12 +++++--
>>>   drivers/scsi/libsas/sas_internal.h    |  3 +-
>>>   drivers/scsi/libsas/sas_scsi_host.c   | 52 
>>> ++++++---------------------
>>>   include/scsi/libsas.h                 |  1 -
>>>   6 files changed, 38 insertions(+), 68 deletions(-)
>>>
>>> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c 
>>> b/drivers/scsi/hisi_sas/hisi_sas_main.c
>>> index fe2752d24fe8..65475775c844 100644
>>> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
>>> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
>>> @@ -465,7 +465,7 @@ static int hisi_sas_queue_command(struct 
>>> sas_task *task, gfp_t gfp_flags)
>>>       struct hisi_sas_port *port;
>>>       struct hisi_hba *hisi_hba;
>>>       struct hisi_sas_slot *slot;
>>> -    struct request *rq = NULL;
>>> +    struct request *rq;
>>>       struct device *dev;
>>>       int rc;
>>> @@ -485,6 +485,21 @@ static int hisi_sas_queue_command(struct 
>>> sas_task *task, gfp_t gfp_flags)
>>>       hisi_hba = dev_to_hisi_hba(device);
>>>       dev = hisi_hba->dev;
>>> +    rq = sas_task_find_rq(task);
>>> +    if (rq) {
>>> +        unsigned int dq_index;
>>> +        u32 blk_tag;
>>> +
>>> +        blk_tag = blk_mq_unique_tag(rq);
>>> +        dq_index = blk_mq_unique_tag_to_hwq(blk_tag);
>>> +        dq = &hisi_hba->dq[dq_index];
>>> +    } else {
>>> +        struct Scsi_Host *shost = hisi_hba->shost;
>>> +        struct blk_mq_queue_map *qmap = 
>>> &shost->tag_set.map[HCTX_TYPE_DEFAULT];
>>> +        int queue = qmap->mq_map[raw_smp_processor_id()];
>>> +
>>> +        dq = &hisi_hba->dq[queue];
>>> +    }
>>>       switch (task->task_proto) {
>>>       case SAS_PROTOCOL_SSP:
>>> @@ -519,22 +534,6 @@ static int hisi_sas_queue_command(struct 
>>> sas_task *task, gfp_t gfp_flags)
>>>                   return -ECOMM;
>>>           }
>>> -
>>> -        rq = sas_task_find_rq(task);
>>> -        if (rq) {
>>> -            unsigned int dq_index;
>>> -            u32 blk_tag;
>>> -
>>> -            blk_tag = blk_mq_unique_tag(rq);
>>> -            dq_index = blk_mq_unique_tag_to_hwq(blk_tag);
>>> -            dq = &hisi_hba->dq[dq_index];
>>> -        } else {
>>> -            struct Scsi_Host *shost = hisi_hba->shost;
>>> -            struct blk_mq_queue_map *qmap = 
>>> &shost->tag_set.map[HCTX_TYPE_DEFAULT];
>>> -            int queue = qmap->mq_map[raw_smp_processor_id()];
>>> -
>>> -            dq = &hisi_hba->dq[queue];
>>> -        }
>>>           break;
>>>       case SAS_PROTOCOL_INTERNAL_ABORT:
>>>           if (!hisi_hba->hw->prep_abort)
>>> @@ -543,13 +542,10 @@ static int hisi_sas_queue_command(struct 
>>> sas_task *task, gfp_t gfp_flags)
>>>           if (test_bit(HISI_SAS_HW_FAULT_BIT, &hisi_hba->flags))
>>>               return -EIO;
>>> -        hisi_hba = dev_to_hisi_hba(device);
>>> -
>>>           if (unlikely(test_bit(HISI_SAS_REJECT_CMD_BIT, 
>>> &hisi_hba->flags)))
>>>               return -EINVAL;
>>>           port = to_hisi_sas_port(sas_port);
>>> -        dq = &hisi_hba->dq[task->abort_task.qid];
>>>           break;
>>>       default:
>>>           dev_err(hisi_hba->dev, "task prep: unknown/unsupported 
>>> proto (0x%x)\n",
>>> diff --git a/drivers/scsi/libsas/sas_expander.c 
>>> b/drivers/scsi/libsas/sas_expander.c
>>> index cc41127ea5cc..e852f1565fe7 100644
>>> --- a/drivers/scsi/libsas/sas_expander.c
>>> +++ b/drivers/scsi/libsas/sas_expander.c
>>> @@ -50,7 +50,7 @@ static int smp_execute_task_sg(struct 
>>> domain_device *dev,
>>>               break;
>>>           }
>>> -        task = sas_alloc_slow_task_rq(dev, GFP_KERNEL);
>>> +        task = sas_alloc_slow_task_rq(dev, GFP_KERNEL, -1U);
>>>           if (!task) {
>>>               res = -ENOMEM;
>>>               break;
>>> diff --git a/drivers/scsi/libsas/sas_init.c 
>>> b/drivers/scsi/libsas/sas_init.c
>>> index 5f9e71a54799..c3f602bd2c4c 100644
>>> --- a/drivers/scsi/libsas/sas_init.c
>>> +++ b/drivers/scsi/libsas/sas_init.c
>>> @@ -56,7 +56,7 @@ struct sas_task *sas_alloc_slow_task(gfp_t flags)
>>>       return task;
>>>   }
>>> -struct sas_task *sas_alloc_slow_task_rq(struct domain_device 
>>> *device, gfp_t flags)
>>> +struct sas_task *sas_alloc_slow_task_rq(struct domain_device 
>>> *device, gfp_t flags, unsigned int qid)
>>>   {
>>>       struct sas_ha_struct *sas_ha = device->port->ha;
>>>       struct Scsi_Host *shost = sas_ha->core.shost;
>>> @@ -86,8 +86,14 @@ struct sas_task *sas_alloc_slow_task_rq(struct 
>>> domain_device *device, gfp_t flag
>>>       task->dev = device;
>>>       task->task_done = sas_task_complete_internal;
>>> -    rq = scsi_alloc_request(sdev->request_queue, REQ_OP_DRV_IN,
>>> -                BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT);
>>> +    if (qid == -1U) {
>>> +        rq = scsi_alloc_request(sdev->request_queue, REQ_OP_DRV_IN,
>>> +                    BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT);
>>> +    } else {
>>> +        rq = scsi_alloc_request_hwq(sdev->request_queue, 
>>> REQ_OP_DRV_IN,
>>> +                    BLK_MQ_REQ_RESERVED | BLK_MQ_REQ_NOWAIT,
>>> +                    qid);
>>> +    }
>>>       if (IS_ERR(rq)) {
>>>           sas_free_task(task);
>>>           return NULL;
>>> diff --git a/drivers/scsi/libsas/sas_internal.h 
>>> b/drivers/scsi/libsas/sas_internal.h
>>> index 9b58948c57c2..af4a4bf88830 100644
>>> --- a/drivers/scsi/libsas/sas_internal.h
>>> +++ b/drivers/scsi/libsas/sas_internal.h
>>> @@ -54,7 +54,8 @@ void sas_free_event(struct asd_sas_event *event);
>>>   struct sas_task *sas_alloc_task(gfp_t flags);
>>>   struct sas_task *sas_alloc_slow_task(gfp_t flags);
>>> -struct sas_task *sas_alloc_slow_task_rq(struct domain_device 
>>> *device, gfp_t flags);
>>> +struct sas_task *sas_alloc_slow_task_rq(struct domain_device 
>>> *device, gfp_t flags,
>>> +                      unsigned int qid);
>>>   void sas_free_task(struct sas_task *task);
>>>   int  sas_register_ports(struct sas_ha_struct *sas_ha);
>>> diff --git a/drivers/scsi/libsas/sas_scsi_host.c 
>>> b/drivers/scsi/libsas/sas_scsi_host.c
>>> index e6ee8dd56a45..a93e019a7dbf 100644
>>> --- a/drivers/scsi/libsas/sas_scsi_host.c
>>> +++ b/drivers/scsi/libsas/sas_scsi_host.c
>>> @@ -920,23 +920,6 @@ void sas_task_complete_internal(struct sas_task 
>>> *task)
>>>       scsi_done(scmd);
>>>   }
>>> -void sas_task_internal_timedout(struct timer_list *t)
>>> -{
>>> -    struct sas_task_slow *slow = from_timer(slow, t, timer);
>>> -    struct sas_task *task = slow->task;
>>> -    bool is_completed = true;
>>> -    unsigned long flags;
>>> -
>>> -    spin_lock_irqsave(&task->task_state_lock, flags);
>>> -    if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
>>> -        task->task_state_flags |= SAS_TASK_STATE_ABORTED;
>>> -        is_completed = false;
>>> -    }
>>> -    spin_unlock_irqrestore(&task->task_state_lock, flags);
>>> -
>>> -    if (!is_completed)
>>> -        complete(&task->slow_task->completion);
>>> -}
>>>   enum blk_eh_timer_return sas_internal_timeout(struct scsi_cmnd *scmd)
>>>   {
>>>       struct sas_task *task = TO_SAS_TASK(scmd);
>>> @@ -978,28 +961,23 @@ static int sas_execute_internal_abort(struct 
>>> domain_device *device,
>>>       int res, retry;
>>>       for (retry = 0; retry < TASK_RETRY; retry++) {
>>> -        task = sas_alloc_slow_task(GFP_KERNEL);
>>> +        struct request *rq;
>>> +
>>> +        task = sas_alloc_slow_task_rq(device, GFP_KERNEL, qid);
>>>           if (!task)
>>>               return -ENOMEM;
>>>           task->dev = device;
>>>           task->task_proto = SAS_PROTOCOL_INTERNAL_ABORT;
>>> -        task->task_done = sas_task_internal_done;
>>> -        task->slow_task->timer.function = sas_task_internal_timedout;
>>> -        task->slow_task->timer.expires = jiffies + TASK_TIMEOUT;
>>> -        add_timer(&task->slow_task->timer);
>>> +        task->task_done = sas_task_complete_internal;
>>>           task->abort_task.tag = tag;
>>>           task->abort_task.type = type;
>>> -        task->abort_task.qid = qid;
>>> -        res = i->dft->lldd_execute_task(task, GFP_KERNEL);
>>> -        if (res) {
>>> - del_timer_sync(&task->slow_task->timer);
>>> -            pr_err("Executing internal abort failed %016llx (%d)\n",
>>> -                   SAS_ADDR(device->sas_addr), res);
>>> -            break;
>>> -        }
>>> +        rq = scsi_cmd_to_rq(task->uldd_task);
>>> +        rq->timeout = TASK_TIMEOUT;
>>> +
>>> +        blk_execute_rq_nowait(rq, true);
>>> wait_for_completion(&task->slow_task->completion);
>>>           res = TMF_RESP_FUNC_FAILED;
>>> @@ -1069,7 +1047,7 @@ int sas_execute_tmf(struct domain_device 
>>> *device, void *parameter,
>>>       for (retry = 0; retry < TASK_RETRY; retry++) {
>>>           struct request *rq;
>>> -        task = sas_alloc_slow_task_rq(device, GFP_KERNEL);
>>> +        task = sas_alloc_slow_task_rq(device, GFP_KERNEL, -1U);
>>>           if (!task)
>>>               return -ENOMEM;
>>> @@ -1251,17 +1229,7 @@ void sas_task_abort(struct sas_task *task)
>>>   {
>>>       struct scsi_cmnd *sc = task->uldd_task;
>>> -    /* Escape for libsas internal commands */
>>> -    if (!sc) {
>>> -        struct sas_task_slow *slow = task->slow_task;
>>> -
>>> -        if (!slow)
>>> -            return;
>>> -        if (!del_timer(&slow->timer))
>>> -            return;
>>> -        slow->timer.function(&slow->timer);
>>> -        return;
>>> -    }
>>> +    WARN_ON_ONCE(!sc);
>>>       if (dev_is_sata(task->dev) && !task->slow_task)
>>>           sas_ata_task_abort(task);
>>> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
>>> index f02156ccd376..60543d8b01d4 100644
>>> --- a/include/scsi/libsas.h
>>> +++ b/include/scsi/libsas.h
>>> @@ -565,7 +565,6 @@ enum sas_internal_abort {
>>>   struct sas_internal_abort_task {
>>>       enum sas_internal_abort type;
>>> -    unsigned int qid;
>>>       u16 tag;
>>>   };
>>
>>
>>
>
> .
>

Re: [PATCH RFC v3 16/22] ata: libata-scsi: Allocate sdev early in port probe

[John Garry]

>>>>>    @@ -4289,26 +4294,16 @@ void ata_scsi_scan_host(struct ata_port
>>>>> *ap, int sync)
>>>>>     repeat:

I've been trying to follow this thread, below, but got a bit lost ....

>>>>>        ata_for_each_link(link, ap, EDGE) {
>>>>>            ata_for_each_dev(dev, link, ENABLED) {
>>>>> -            struct scsi_device *sdev;
>>>>> +            struct Scsi_Host *shost = ap->scsi_host;
>>>>>                int channel = 0, id = 0;
>>>>>    -            if (dev->sdev)
>>>>> -                continue;
>>>>> -
>>>>>                if (ata_is_host_link(link))
>>>>>                    id = dev->devno;
>>>>>                else
>>>>>                    channel = link->pmp;
>>>>>    -            sdev = __scsi_add_device(ap->scsi_host, channel, 
>>>>> id, 0,
>>>>> -                         NULL);
>>>>> -            if (!IS_ERR(sdev)) {
>>>>> -                dev->sdev = sdev;
>>>>> -                ata_scsi_assign_ofnode(dev, ap);
>>>>
>>>> Is there something equivalent to what this function does inside
>>>> scsi_scan_target() ? I had a quick look but did not see anything...
>>>>

So are we discussing below whether we can have fixed channel, id, lun 
per ATA sdev per shost? If so, I don't think it would work as libsas 
uses dynamic target ids per host.

>>> Typically, the SCSI layer has two ways of scanning.
>>> One it the old-style serial scanning (originating in the old SCSI
>>> parallel model):
>>> The scanning code will blindly scan _all_ devices up to max_luns, and
>>> attach every device for which the scanning code returns 'OK'.
>>> The other one is to issue REPORT_LUNS and scan all LUNs returned there.
>>>
>>> Mapped to libata we would need to figure out a stable SCSI enumeration,
>>> given that we have PMP and slave devices to content with.
>>> To my knowledge we have ATA ports, each can have either a 'master' and
>>> 'slave' device, _or_ it be a PMP port when it can support up to 16
>>> devices, right?
>>
>> yes
>>
>>> Point being, master/slave and PMP are exclusive, right?
>>
>> Never heard of pmp with ide cable :)
>>
> See?
> 
>>> So we can make the master as LUN 0, and the slave as LUN 1.
>>
>> Yes, but isn't that a little wrong ? That would assume that the ata port
>> is the device and the ata devices the luns of that device. But beside
>> the "link busy" stuff that needs to be dealt with, master and slave are
>> independent devices, unlike LUNs. No ?
>> Well; technically, yes.
> 
> But we already enumerate the ata ports (which is typically done by the 
> hardware/PCI layer etc), and if we were try to model slave devices as 
> independent ports we would either have to insert a numbering (awkward) 
> or add a number at the en (even more awkward).
> 
> And the one key takeaway from the 'multiple actuators' discussion is 
> that LUNs _are_ independent (cf all the hoops they had to jump through 
> to define a command spanning several LUNs ...)(which, incidentally, we 
> could leverage here, too ...), and the target port really only serves as 
> an enumeration thingie to address the LUNs.
> 
> So we _could_ map the master device on LUN 0 and the slave device on LUN 
> 1 with no loss of functionality, _but_ enable a consistent SCSI enumeration

Thanks,
John

Re: [PATCH RFC v3 16/22] ata: libata-scsi: Allocate sdev early in port probe

[Hannes Reinecke]

On 11/7/22 11:09, John Garry wrote:
> 
>>>>>>    @@ -4289,26 +4294,16 @@ void ata_scsi_scan_host(struct ata_port
>>>>>> *ap, int sync)
>>>>>>     repeat:
> 
> I've been trying to follow this thread, below, but got a bit lost ....
> 
>>>>>>        ata_for_each_link(link, ap, EDGE) {
>>>>>>            ata_for_each_dev(dev, link, ENABLED) {
>>>>>> -            struct scsi_device *sdev;
>>>>>> +            struct Scsi_Host *shost = ap->scsi_host;
>>>>>>                int channel = 0, id = 0;
>>>>>>    -            if (dev->sdev)
>>>>>> -                continue;
>>>>>> -
>>>>>>                if (ata_is_host_link(link))
>>>>>>                    id = dev->devno;
>>>>>>                else
>>>>>>                    channel = link->pmp;
>>>>>>    -            sdev = __scsi_add_device(ap->scsi_host, channel, 
>>>>>> id, 0,
>>>>>> -                         NULL);
>>>>>> -            if (!IS_ERR(sdev)) {
>>>>>> -                dev->sdev = sdev;
>>>>>> -                ata_scsi_assign_ofnode(dev, ap);
>>>>>
>>>>> Is there something equivalent to what this function does inside
>>>>> scsi_scan_target() ? I had a quick look but did not see anything...
>>>>>
> 
> So are we discussing below whether we can have fixed channel, id, lun 
> per ATA sdev per shost? If so, I don't think it would work as libsas 
> uses dynamic target ids per host.
> 
Not static. target port enumeration would still be left to the driver / 
transport, so it can do whatever it wants here.
Idea was to match the 'ata_port' structure to the scsi target port; 
well, not exactly 'match' but have a 1:1 relationship between them.
_And_ to have a defined way on how the devices are enumerated; PATA 
drives would always have LUN 0 and LUN 1(for the slave), and everything 
else would use REPORT LUNs. For which we need an emulation for libata, 
but that should be trivially to implement.

With that we would know exactly how many scsi target ports we'll have, 
and can create them based on hardware details before probing starts.
Whether or not a _device_ is attached to that port is being determined 
during scanning via the 'slave_alloc' or 'slave_configure' callbacks, so 
we still would be able to blank out any not implemented or not connected 
ports.
_But_ it would align better with the SCSI layer, such that we can work 
on integrating scanning and resetting, and make things like 
'sd_revalidate' actually work for libata.

And for libsas it might be worthwhile to check the 'scan_start' and 
'scan_finished' callbacks; that seems to be the ticket here.

Cheers,

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