[PATCH v2 0/6] scsi: libsas: Use request tag in more drivers

[PATCH v2 0/6] scsi: libsas: Use request tag in more drivers

[John Garry]

Currently hisi_sas is the only libsas driver which uses the request tag
for per-HW IO tag.

All other libsas drivers manage the tags internally. Tag management in
pm8001 and mvsas is currently using a simple bitmap, so use the request
tag when available there. With this change we still need to manage tags
for libsas "internal" commands, like SMP commands, and any other
private commands so reserve some tags for this:
- For pm8001 I went with pre-existing and unused PM8001_RESERVE_SLOT size.
  The value is 8, which should be enough. It is greater than mvsas, below,
  but this driver sends a lot of other private commands to HW.
- For mvsas I went with 4, which still should be enough.

isci and aic9xx have elaborate tag alloc schemes, so I'm not going to
bother changing them, especially since I have no HW to test with.

Helper sas_task_find_rq() is added to get the request and associated tag
per sas_task when it is available.

This series looks not to conflict with
https://lore.kernel.org/linux-scsi/20220928070130.3657183-1-yanaijie@huawei.com/T/#mefdcb7b63b4e6ebc8b77a689b3923571ab3d33ab

Based on https://lore.kernel.org/linux-scsi/1664262298-239952-1-git-send-email-john.garry@huawei.com/T/#t

Differences to v1:
- Rework sas_task_find_rq()
- Rename tags->rsvd and remove tag size struct arg for both mvsas and
  pm8001
- Decrement can_queue for mvsas
- Use sas_task_find_rq() in pm80xx_chip_get_q_index()
- Add Damien's tags (thanks)

Igor Pylypiv (1):
  scsi: pm8001: Remove pm8001_tag_init()

John Garry (5):
  scsi: libsas: Add sas_task_find_rq()
  scsi: hisi_sas: Use sas_task_find_rq()
  scsi: pm8001: Use sas_task_find_rq() for tagging
  scsi: mvsas: Delete mvs_tag_init()
  scsi: mvsas: Use sas_task_find_rq() for tagging

 drivers/scsi/hisi_sas/hisi_sas_main.c | 26 +++++-----------
 drivers/scsi/mvsas/mv_defs.h          |  1 +
 drivers/scsi/mvsas/mv_init.c          | 11 +++----
 drivers/scsi/mvsas/mv_sas.c           | 45 +++++++++++++++------------
 drivers/scsi/mvsas/mv_sas.h           |  8 +----
 drivers/scsi/pm8001/pm8001_init.c     | 14 +++------
 drivers/scsi/pm8001/pm8001_sas.c      | 23 +++++++-------
 drivers/scsi/pm8001/pm8001_sas.h      | 12 ++++---
 drivers/scsi/pm8001/pm80xx_hwi.c      | 17 ++--------
 include/scsi/libsas.h                 | 18 +++++++++++
 10 files changed, 85 insertions(+), 90 deletions(-)

-- 
2.35.3

[PATCH v2 1/6] scsi: libsas: Add sas_task_find_rq()

[John Garry]

blk-mq already provides a unique tag per request. Some libsas LLDDs - like
hisi_sas - already use this tag as the unique per-IO HW tag.

Add a common function to provide the request associated with a sas_task
for all libsas LLDDs.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 include/scsi/libsas.h | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
index f86b56bf7833..f498217961db 100644
--- a/include/scsi/libsas.h
+++ b/include/scsi/libsas.h
@@ -644,6 +644,24 @@ static inline bool sas_is_internal_abort(struct sas_task *task)
 	return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
 }
 
+static inline struct request *sas_task_find_rq(struct sas_task *task)
+{
+	struct scsi_cmnd *scmd;
+
+	if (task->task_proto & SAS_PROTOCOL_STP_ALL) {
+		struct ata_queued_cmd *qc = task->uldd_task;
+
+		scmd = qc ? qc->scsicmd : NULL;
+	} else {
+		scmd = task->uldd_task;
+	}
+
+	if (!scmd)
+		return NULL;
+
+	return scsi_cmd_to_rq(scmd);
+}
+
 struct sas_domain_function_template {
 	/* The class calls these to notify the LLDD of an event. */
 	void (*lldd_port_formed)(struct asd_sas_phy *);
-- 
2.35.3

[PATCH v2 2/6] scsi: hisi_sas: Use sas_task_find_rq()

[John Garry]

Use sas_task_find_rq() to lookup the request per task for its driver tag.

Signed-off-by: John Garry <john.garry@huawei.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 26 ++++++++------------------
 1 file changed, 8 insertions(+), 18 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 4c37ae9eb6b6..1011dffed51f 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -177,13 +177,13 @@ static void hisi_sas_slot_index_set(struct hisi_hba *hisi_hba, int slot_idx)
 }
 
 static int hisi_sas_slot_index_alloc(struct hisi_hba *hisi_hba,
-				     struct scsi_cmnd *scsi_cmnd)
+				     struct request *rq)
 {
 	int index;
 	void *bitmap = hisi_hba->slot_index_tags;
 
-	if (scsi_cmnd)
-		return scsi_cmd_to_rq(scsi_cmnd)->tag;
+	if (rq)
+		return rq->tag;
 
 	spin_lock(&hisi_hba->lock);
 	index = find_next_zero_bit(bitmap, hisi_hba->slot_index_count,
@@ -461,11 +461,11 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
 	struct asd_sas_port *sas_port = device->port;
 	struct hisi_sas_device *sas_dev = device->lldd_dev;
 	bool internal_abort = sas_is_internal_abort(task);
-	struct scsi_cmnd *scmd = NULL;
 	struct hisi_sas_dq *dq = NULL;
 	struct hisi_sas_port *port;
 	struct hisi_hba *hisi_hba;
 	struct hisi_sas_slot *slot;
+	struct request *rq = NULL;
 	struct device *dev;
 	int rc;
 
@@ -520,22 +520,12 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
 				return -ECOMM;
 		}
 
-		if (task->uldd_task) {
-			struct ata_queued_cmd *qc;
-
-			if (dev_is_sata(device)) {
-				qc = task->uldd_task;
-				scmd = qc->scsicmd;
-			} else {
-				scmd = task->uldd_task;
-			}
-		}
-
-		if (scmd) {
+		rq = sas_task_find_rq(task);
+		if (rq) {
 			unsigned int dq_index;
 			u32 blk_tag;
 
-			blk_tag = blk_mq_unique_tag(scsi_cmd_to_rq(scmd));
+			blk_tag = blk_mq_unique_tag(rq);
 			dq_index = blk_mq_unique_tag_to_hwq(blk_tag);
 			dq = &hisi_hba->dq[dq_index];
 		} else {
@@ -580,7 +570,7 @@ static int hisi_sas_queue_command(struct sas_task *task, gfp_t gfp_flags)
 	if (!internal_abort && hisi_hba->hw->slot_index_alloc)
 		rc = hisi_hba->hw->slot_index_alloc(hisi_hba, device);
 	else
-		rc = hisi_sas_slot_index_alloc(hisi_hba, scmd);
+		rc = hisi_sas_slot_index_alloc(hisi_hba, rq);
 
 	if (rc < 0)
 		goto err_out_dif_dma_unmap;
-- 
2.35.3

[PATCH v2 3/6] scsi: pm8001: Remove pm8001_tag_init()

[John Garry]

From: Igor Pylypiv <ipylypiv@google.com>

In commit 5a141315ed7c ("scsi: pm80xx: Increase the number of outstanding
I/O supported to 1024") the pm8001_ha->tags allocation was moved into
pm8001_init_ccb_tag(). This changed the execution order of allocation.
pm8001_tag_init() used to be called after the pm8001_ha->tags allocation
and now it is called before the allocation.

Before:

pm8001_pci_probe()
`--> pm8001_pci_alloc()
     `--> pm8001_alloc()
          `--> pm8001_ha->tags = kzalloc(...)
          `--> pm8001_tag_init(pm8001_ha); // OK: tags are allocated

After:

pm8001_pci_probe()
`--> pm8001_pci_alloc()
|    `--> pm8001_alloc()
|         `--> pm8001_tag_init(pm8001_ha); // NOK: tags are not allocated
|
`--> pm8001_init_ccb_tag()
     `-->  pm8001_ha->tags = kzalloc(...) // today it is bitmap_zalloc()

Since pm8001_ha->tags_num is zero when pm8001_tag_init() is called it does
nothing. Tags memory is allocated with bitmap_zalloc() so there is no need
to manually clear each bit with pm8001_tag_free().

Reviewed-by: Changyuan Lyu <changyuanl@google.com>
Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
Signed-off-by: John Garry <john.garry@huawei.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/pm8001/pm8001_init.c | 2 --
 drivers/scsi/pm8001/pm8001_sas.c  | 7 -------
 drivers/scsi/pm8001/pm8001_sas.h  | 1 -
 3 files changed, 10 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index a0028e130a7e..0edc9857a8bd 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -436,8 +436,6 @@ static int pm8001_alloc(struct pm8001_hba_info *pm8001_ha,
 		atomic_set(&pm8001_ha->devices[i].running_req, 0);
 	}
 	pm8001_ha->flags = PM8001F_INIT_TIME;
-	/* Initialize tags */
-	pm8001_tag_init(pm8001_ha);
 	return 0;
 
 err_out_nodev:
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index d5ec29f69be3..066dfa9f4683 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -96,13 +96,6 @@ int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out)
 	return 0;
 }
 
-void pm8001_tag_init(struct pm8001_hba_info *pm8001_ha)
-{
-	int i;
-	for (i = 0; i < pm8001_ha->tags_num; ++i)
-		pm8001_tag_free(pm8001_ha, i);
-}
-
 /**
  * pm8001_mem_alloc - allocate memory for pm8001.
  * @pdev: pci device.
diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index 8ab0654327f9..9acaadf02150 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -632,7 +632,6 @@ extern struct workqueue_struct *pm8001_wq;
 
 /******************** function prototype *********************/
 int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out);
-void pm8001_tag_init(struct pm8001_hba_info *pm8001_ha);
 u32 pm8001_get_ncq_tag(struct sas_task *task, u32 *tag);
 void pm8001_ccb_task_free(struct pm8001_hba_info *pm8001_ha,
 			  struct pm8001_ccb_info *ccb);
-- 
2.35.3

[PATCH v2 4/6] scsi: pm8001: Use sas_task_find_rq() for tagging

[John Garry]

The request associated with a scsi command coming from the block layer
has a unique tag, so use that when possible for getting a CCB.

Unfortunately we don't support reserved commands in the SCSI midlayer yet,
so in the interim continue to manage those tags internally (along with
tags for private commands).

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/pm8001/pm8001_init.c | 12 ++++--------
 drivers/scsi/pm8001/pm8001_sas.c  | 16 ++++++++++++----
 drivers/scsi/pm8001/pm8001_sas.h  | 11 ++++++++---
 drivers/scsi/pm8001/pm80xx_hwi.c  | 17 +++--------------
 4 files changed, 27 insertions(+), 29 deletions(-)

diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
index 0edc9857a8bd..abb884ddcaf9 100644
--- a/drivers/scsi/pm8001/pm8001_init.c
+++ b/drivers/scsi/pm8001/pm8001_init.c
@@ -196,7 +196,7 @@ static void pm8001_free(struct pm8001_hba_info *pm8001_ha)
 	}
 	PM8001_CHIP_DISP->chip_iounmap(pm8001_ha);
 	flush_workqueue(pm8001_wq);
-	bitmap_free(pm8001_ha->tags);
+	bitmap_free(pm8001_ha->rsvd_tags);
 	kfree(pm8001_ha);
 }
 
@@ -1208,18 +1208,15 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
 	struct Scsi_Host *shost = pm8001_ha->shost;
 	struct device *dev = pm8001_ha->dev;
 	u32 max_out_io, ccb_count;
-	u32 can_queue;
 	int i;
 
 	max_out_io = pm8001_ha->main_cfg_tbl.pm80xx_tbl.max_out_io;
 	ccb_count = min_t(int, PM8001_MAX_CCB, max_out_io);
 
-	/* Update to the scsi host*/
-	can_queue = ccb_count - PM8001_RESERVE_SLOT;
-	shost->can_queue = can_queue;
+	shost->can_queue = ccb_count - PM8001_RESERVE_SLOT;
 
-	pm8001_ha->tags = bitmap_zalloc(ccb_count, GFP_KERNEL);
-	if (!pm8001_ha->tags)
+	pm8001_ha->rsvd_tags = bitmap_zalloc(PM8001_RESERVE_SLOT, GFP_KERNEL);
+	if (!pm8001_ha->rsvd_tags)
 		goto err_out;
 
 	/* Memory region for ccb_info*/
@@ -1244,7 +1241,6 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
 		pm8001_ha->ccb_info[i].task = NULL;
 		pm8001_ha->ccb_info[i].ccb_tag = PM8001_INVALID_TAG;
 		pm8001_ha->ccb_info[i].device = NULL;
-		++pm8001_ha->tags_num;
 	}
 
 	return 0;
diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
index 066dfa9f4683..d60bc311a4e9 100644
--- a/drivers/scsi/pm8001/pm8001_sas.c
+++ b/drivers/scsi/pm8001/pm8001_sas.c
@@ -65,9 +65,14 @@ static int pm8001_find_tag(struct sas_task *task, u32 *tag)
   */
 void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag)
 {
-	void *bitmap = pm8001_ha->tags;
+	void *bitmap = pm8001_ha->rsvd_tags;
 	unsigned long flags;
 
+	if (tag < pm8001_ha->shost->can_queue)
+		return;
+
+	tag -= pm8001_ha->shost->can_queue;
+
 	spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
 	__clear_bit(tag, bitmap);
 	spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
@@ -80,18 +85,21 @@ void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag)
   */
 int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out)
 {
-	void *bitmap = pm8001_ha->tags;
+	void *bitmap = pm8001_ha->rsvd_tags;
 	unsigned long flags;
 	unsigned int tag;
 
 	spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
-	tag = find_first_zero_bit(bitmap, pm8001_ha->tags_num);
-	if (tag >= pm8001_ha->tags_num) {
+	tag = find_first_zero_bit(bitmap, PM8001_RESERVE_SLOT);
+	if (tag >= PM8001_RESERVE_SLOT) {
 		spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
 		return -SAS_QUEUE_FULL;
 	}
 	__set_bit(tag, bitmap);
 	spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
+
+	/* reserved tags are in the upper region of the tagset */
+	tag += pm8001_ha->shost->can_queue;
 	*tag_out = tag;
 	return 0;
 }
diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
index 9acaadf02150..ba32b009f412 100644
--- a/drivers/scsi/pm8001/pm8001_sas.h
+++ b/drivers/scsi/pm8001/pm8001_sas.h
@@ -510,8 +510,7 @@ struct pm8001_hba_info {
 	u32			chip_id;
 	const struct pm8001_chip_info	*chip;
 	struct completion	*nvmd_completion;
-	int			tags_num;
-	unsigned long		*tags;
+	unsigned long		*rsvd_tags;
 	struct pm8001_phy	phy[PM8001_MAX_PHYS];
 	struct pm8001_port	port[PM8001_MAX_PHYS];
 	u32			id;
@@ -737,9 +736,15 @@ pm8001_ccb_alloc(struct pm8001_hba_info *pm8001_ha,
 		 struct pm8001_device *dev, struct sas_task *task)
 {
 	struct pm8001_ccb_info *ccb;
+	struct request *rq = NULL;
 	u32 tag;
 
-	if (pm8001_tag_alloc(pm8001_ha, &tag)) {
+	if (task)
+		rq = sas_task_find_rq(task);
+
+	if (rq) {
+		tag = rq->tag;
+	} else if (pm8001_tag_alloc(pm8001_ha, &tag)) {
 		pm8001_dbg(pm8001_ha, FAIL, "Failed to allocate a tag\n");
 		return NULL;
 	}
diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
index 4484c498bcb6..ed2d65d3749a 100644
--- a/drivers/scsi/pm8001/pm80xx_hwi.c
+++ b/drivers/scsi/pm8001/pm80xx_hwi.c
@@ -4247,24 +4247,13 @@ static int check_enc_sat_cmd(struct sas_task *task)
 
 static u32 pm80xx_chip_get_q_index(struct sas_task *task)
 {
-	struct scsi_cmnd *scmd = NULL;
+	struct request *rq = sas_task_find_rq(task);
 	u32 blk_tag;
 
-	if (task->uldd_task) {
-		struct ata_queued_cmd *qc;
-
-		if (dev_is_sata(task->dev)) {
-			qc = task->uldd_task;
-			scmd = qc->scsicmd;
-		} else {
-			scmd = task->uldd_task;
-		}
-	}
-
-	if (!scmd)
+	if (!rq)
 		return 0;
 
-	blk_tag = blk_mq_unique_tag(scsi_cmd_to_rq(scmd));
+	blk_tag = blk_mq_unique_tag(rq);
 	return blk_mq_unique_tag_to_hwq(blk_tag);
 }
 
-- 
2.35.3

[PATCH v2 5/6] scsi: mvsas: Delete mvs_tag_init()

[John Garry]

All mvs_tag_init() does is zero the tag bitmap, but this is already done
with the kzalloc() call to alloc the tags, so delete this unneeded
function.

Signed-off-by: John Garry <john.garry@huawei.com>
Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
---
 drivers/scsi/mvsas/mv_init.c | 2 --
 drivers/scsi/mvsas/mv_sas.c  | 7 -------
 drivers/scsi/mvsas/mv_sas.h  | 1 -
 3 files changed, 10 deletions(-)

diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index 2fde496fff5f..c85fb812ad43 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -286,8 +286,6 @@ static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost)
 	}
 	mvi->tags_num = slot_nr;
 
-	/* Initialize tags */
-	mvs_tag_init(mvi);
 	return 0;
 err_out:
 	return 1;
diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index a6867dae0e7c..0810e6c930e1 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -51,13 +51,6 @@ inline int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out)
 	return 0;
 }
 
-void mvs_tag_init(struct mvs_info *mvi)
-{
-	int i;
-	for (i = 0; i < mvi->tags_num; ++i)
-		mvs_tag_clear(mvi, i);
-}
-
 static struct mvs_info *mvs_find_dev_mvi(struct domain_device *dev)
 {
 	unsigned long i = 0, j = 0, hi = 0;
diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h
index 509d8f32a04f..fe57665bdb50 100644
--- a/drivers/scsi/mvsas/mv_sas.h
+++ b/drivers/scsi/mvsas/mv_sas.h
@@ -428,7 +428,6 @@ void mvs_tag_clear(struct mvs_info *mvi, u32 tag);
 void mvs_tag_free(struct mvs_info *mvi, u32 tag);
 void mvs_tag_set(struct mvs_info *mvi, unsigned int tag);
 int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out);
-void mvs_tag_init(struct mvs_info *mvi);
 void mvs_iounmap(void __iomem *regs);
 int mvs_ioremap(struct mvs_info *mvi, int bar, int bar_ex);
 void mvs_phys_reset(struct mvs_info *mvi, u32 phy_mask, int hard);
-- 
2.35.3

[PATCH v2 6/6] scsi: mvsas: Use sas_task_find_rq() for tagging

[John Garry]

The request associated with a scsi command coming from the block layer
has a unique tag, so use that when possible for getting a slot.

Unfortunately we don't support reserved commands in the SCSI midlayer yet.
As such, SMP tasks - as an example - will not have a request associated, so
in the interim continue to manage those tags for that type of sas_task
internally.

We reserve an arbitrary 4 tags for these internal tags. Indeed, we already
decrement MVS_RSVD_SLOTS by 2 for the shost can_queue when flag
MVF_FLAG_SOC is set. This change was made in commit 20b09c2992fef
("[PATCH] [SCSI] mvsas: add support for 94xx; layout change; bug fixes"),
but what those 2 slots are used for is not obvious.

Also make the tag management functions static, where possible.

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/mvsas/mv_defs.h |  1 +
 drivers/scsi/mvsas/mv_init.c |  9 +++++----
 drivers/scsi/mvsas/mv_sas.c  | 38 ++++++++++++++++++++++++------------
 drivers/scsi/mvsas/mv_sas.h  |  7 +------
 4 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/drivers/scsi/mvsas/mv_defs.h b/drivers/scsi/mvsas/mv_defs.h
index 7123a2efbf58..8ef174cd4d37 100644
--- a/drivers/scsi/mvsas/mv_defs.h
+++ b/drivers/scsi/mvsas/mv_defs.h
@@ -40,6 +40,7 @@ enum driver_configuration {
 	MVS_ATA_CMD_SZ		= 96,	/* SATA command table buffer size */
 	MVS_OAF_SZ		= 64,	/* Open address frame buffer size */
 	MVS_QUEUE_SIZE		= 64,	/* Support Queue depth */
+	MVS_RSVD_SLOTS		= 4,
 	MVS_SOC_CAN_QUEUE	= MVS_SOC_SLOTS - 2,
 };
 
diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
index c85fb812ad43..cfe84473a515 100644
--- a/drivers/scsi/mvsas/mv_init.c
+++ b/drivers/scsi/mvsas/mv_init.c
@@ -142,7 +142,7 @@ static void mvs_free(struct mvs_info *mvi)
 		scsi_host_put(mvi->shost);
 	list_for_each_entry(mwq, &mvi->wq_list, entry)
 		cancel_delayed_work(&mwq->work_q);
-	kfree(mvi->tags);
+	kfree(mvi->rsvd_tags);
 	kfree(mvi);
 }
 
@@ -284,7 +284,6 @@ static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost)
 			printk(KERN_DEBUG "failed to create dma pool %s.\n", pool_name);
 			goto err_out;
 	}
-	mvi->tags_num = slot_nr;
 
 	return 0;
 err_out:
@@ -367,8 +366,8 @@ static struct mvs_info *mvs_pci_alloc(struct pci_dev *pdev,
 	mvi->sas = sha;
 	mvi->shost = shost;
 
-	mvi->tags = kzalloc(MVS_CHIP_SLOT_SZ>>3, GFP_KERNEL);
-	if (!mvi->tags)
+	mvi->rsvd_tags = bitmap_zalloc(MVS_RSVD_SLOTS, GFP_KERNEL);
+	if (!mvi->rsvd_tags)
 		goto err_out;
 
 	if (MVS_CHIP_DISP->chip_ioremap(mvi))
@@ -469,6 +468,8 @@ static void  mvs_post_sas_ha_init(struct Scsi_Host *shost,
 	else
 		can_queue = MVS_CHIP_SLOT_SZ;
 
+	can_queue -= MVS_RSVD_SLOTS;
+
 	shost->sg_tablesize = min_t(u16, SG_ALL, MVS_MAX_SG);
 	shost->can_queue = can_queue;
 	mvi->shost->cmd_per_lun = MVS_QUEUE_SIZE;
diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
index 0810e6c930e1..00b3a2781d21 100644
--- a/drivers/scsi/mvsas/mv_sas.c
+++ b/drivers/scsi/mvsas/mv_sas.c
@@ -20,33 +20,39 @@ static int mvs_find_tag(struct mvs_info *mvi, struct sas_task *task, u32 *tag)
 	return 0;
 }
 
-void mvs_tag_clear(struct mvs_info *mvi, u32 tag)
+static void mvs_tag_clear(struct mvs_info *mvi, u32 tag)
 {
-	void *bitmap = mvi->tags;
+	void *bitmap = mvi->rsvd_tags;
 	clear_bit(tag, bitmap);
 }
 
-void mvs_tag_free(struct mvs_info *mvi, u32 tag)
+static void mvs_tag_free(struct mvs_info *mvi, u32 tag)
 {
+	if (tag < mvi->shost->can_queue)
+		return;
+
+	tag -= mvi->shost->can_queue;
+
 	mvs_tag_clear(mvi, tag);
 }
 
-void mvs_tag_set(struct mvs_info *mvi, unsigned int tag)
+static void mvs_tag_set(struct mvs_info *mvi, unsigned int tag)
 {
-	void *bitmap = mvi->tags;
+	void *bitmap = mvi->rsvd_tags;
 	set_bit(tag, bitmap);
 }
 
-inline int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out)
+static int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out)
 {
 	unsigned int index, tag;
-	void *bitmap = mvi->tags;
+	void *bitmap = mvi->rsvd_tags;
 
-	index = find_first_zero_bit(bitmap, mvi->tags_num);
+	index = find_first_zero_bit(bitmap, MVS_RSVD_SLOTS);
 	tag = index;
-	if (tag >= mvi->tags_num)
+	if (tag >= MVS_RSVD_SLOTS)
 		return -SAS_QUEUE_FULL;
 	mvs_tag_set(mvi, tag);
+	tag += mvi->shost->can_queue;
 	*tag_out = tag;
 	return 0;
 }
@@ -696,6 +702,7 @@ static int mvs_task_prep(struct sas_task *task, struct mvs_info *mvi, int is_tmf
 	struct mvs_task_exec_info tei;
 	struct mvs_slot_info *slot;
 	u32 tag = 0xdeadbeef, n_elem = 0;
+	struct request *rq;
 	int rc = 0;
 
 	if (!dev->port) {
@@ -760,9 +767,14 @@ static int mvs_task_prep(struct sas_task *task, struct mvs_info *mvi, int is_tmf
 		n_elem = task->num_scatter;
 	}
 
-	rc = mvs_tag_alloc(mvi, &tag);
-	if (rc)
-		goto err_out;
+	rq = sas_task_find_rq(task);
+	if (rq) {
+		tag = rq->tag;
+	} else {
+		rc = mvs_tag_alloc(mvi, &tag);
+		if (rc)
+			goto err_out;
+	}
 
 	slot = &mvi->slot_info[tag];
 
@@ -857,7 +869,7 @@ int mvs_queue_command(struct sas_task *task, gfp_t gfp_flags)
 static void mvs_slot_free(struct mvs_info *mvi, u32 rx_desc)
 {
 	u32 slot_idx = rx_desc & RXQ_SLOT_MASK;
-	mvs_tag_clear(mvi, slot_idx);
+	mvs_tag_free(mvi, slot_idx);
 }
 
 static void mvs_slot_task_free(struct mvs_info *mvi, struct sas_task *task,
diff --git a/drivers/scsi/mvsas/mv_sas.h b/drivers/scsi/mvsas/mv_sas.h
index fe57665bdb50..68df771e2975 100644
--- a/drivers/scsi/mvsas/mv_sas.h
+++ b/drivers/scsi/mvsas/mv_sas.h
@@ -370,8 +370,7 @@ struct mvs_info {
 	u32 chip_id;
 	const struct mvs_chip_info *chip;
 
-	int tags_num;
-	unsigned long *tags;
+	unsigned long *rsvd_tags;
 	/* further per-slot information */
 	struct mvs_phy phy[MVS_MAX_PHYS];
 	struct mvs_port port[MVS_MAX_PHYS];
@@ -424,10 +423,6 @@ struct mvs_task_exec_info {
 
 /******************** function prototype *********************/
 void mvs_get_sas_addr(void *buf, u32 buflen);
-void mvs_tag_clear(struct mvs_info *mvi, u32 tag);
-void mvs_tag_free(struct mvs_info *mvi, u32 tag);
-void mvs_tag_set(struct mvs_info *mvi, unsigned int tag);
-int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out);
 void mvs_iounmap(void __iomem *regs);
 int mvs_ioremap(struct mvs_info *mvi, int bar, int bar_ex);
 void mvs_phys_reset(struct mvs_info *mvi, u32 phy_mask, int hard);
-- 
2.35.3

Re: [PATCH v2 1/6] scsi: libsas: Add sas_task_find_rq()

[Jinpu Wang]

On Fri, Sep 30, 2022 at 11:03 AM John Garry <john.garry@huawei.com> wrote:
>
> blk-mq already provides a unique tag per request. Some libsas LLDDs - like
> hisi_sas - already use this tag as the unique per-IO HW tag.
>
> Add a common function to provide the request associated with a sas_task
> for all libsas LLDDs.
>
> Signed-off-by: John Garry <john.garry@huawei.com>
lgtm
Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  include/scsi/libsas.h | 18 ++++++++++++++++++
>  1 file changed, 18 insertions(+)
>
> diff --git a/include/scsi/libsas.h b/include/scsi/libsas.h
> index f86b56bf7833..f498217961db 100644
> --- a/include/scsi/libsas.h
> +++ b/include/scsi/libsas.h
> @@ -644,6 +644,24 @@ static inline bool sas_is_internal_abort(struct sas_task *task)
>         return task->task_proto == SAS_PROTOCOL_INTERNAL_ABORT;
>  }
>
> +static inline struct request *sas_task_find_rq(struct sas_task *task)
> +{
> +       struct scsi_cmnd *scmd;
> +
> +       if (task->task_proto & SAS_PROTOCOL_STP_ALL) {
> +               struct ata_queued_cmd *qc = task->uldd_task;
> +
> +               scmd = qc ? qc->scsicmd : NULL;
> +       } else {
> +               scmd = task->uldd_task;
> +       }
> +
> +       if (!scmd)
> +               return NULL;
> +
> +       return scsi_cmd_to_rq(scmd);
> +}
> +
>  struct sas_domain_function_template {
>         /* The class calls these to notify the LLDD of an event. */
>         void (*lldd_port_formed)(struct asd_sas_phy *);
> --
> 2.35.3
>

Re: [PATCH v2 3/6] scsi: pm8001: Remove pm8001_tag_init()

[Jinpu Wang]

On Fri, Sep 30, 2022 at 11:03 AM John Garry <john.garry@huawei.com> wrote:
>
> From: Igor Pylypiv <ipylypiv@google.com>
>
> In commit 5a141315ed7c ("scsi: pm80xx: Increase the number of outstanding
> I/O supported to 1024") the pm8001_ha->tags allocation was moved into
> pm8001_init_ccb_tag(). This changed the execution order of allocation.
> pm8001_tag_init() used to be called after the pm8001_ha->tags allocation
> and now it is called before the allocation.
>
> Before:
>
> pm8001_pci_probe()
> `--> pm8001_pci_alloc()
>      `--> pm8001_alloc()
>           `--> pm8001_ha->tags = kzalloc(...)
>           `--> pm8001_tag_init(pm8001_ha); // OK: tags are allocated
>
> After:
>
> pm8001_pci_probe()
> `--> pm8001_pci_alloc()
> |    `--> pm8001_alloc()
> |         `--> pm8001_tag_init(pm8001_ha); // NOK: tags are not allocated
> |
> `--> pm8001_init_ccb_tag()
>      `-->  pm8001_ha->tags = kzalloc(...) // today it is bitmap_zalloc()
>
> Since pm8001_ha->tags_num is zero when pm8001_tag_init() is called it does
> nothing. Tags memory is allocated with bitmap_zalloc() so there is no need
> to manually clear each bit with pm8001_tag_free().
>
> Reviewed-by: Changyuan Lyu <changyuanl@google.com>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> Signed-off-by: John Garry <john.garry@huawei.com>
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
> ---
>  drivers/scsi/pm8001/pm8001_init.c | 2 --
>  drivers/scsi/pm8001/pm8001_sas.c  | 7 -------
>  drivers/scsi/pm8001/pm8001_sas.h  | 1 -
>  3 files changed, 10 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index a0028e130a7e..0edc9857a8bd 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -436,8 +436,6 @@ static int pm8001_alloc(struct pm8001_hba_info *pm8001_ha,
>                 atomic_set(&pm8001_ha->devices[i].running_req, 0);
>         }
>         pm8001_ha->flags = PM8001F_INIT_TIME;
> -       /* Initialize tags */
> -       pm8001_tag_init(pm8001_ha);
>         return 0;
>
>  err_out_nodev:
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> index d5ec29f69be3..066dfa9f4683 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -96,13 +96,6 @@ int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out)
>         return 0;
>  }
>
> -void pm8001_tag_init(struct pm8001_hba_info *pm8001_ha)
> -{
> -       int i;
> -       for (i = 0; i < pm8001_ha->tags_num; ++i)
> -               pm8001_tag_free(pm8001_ha, i);
> -}
> -
>  /**
>   * pm8001_mem_alloc - allocate memory for pm8001.
>   * @pdev: pci device.
> diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
> index 8ab0654327f9..9acaadf02150 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.h
> +++ b/drivers/scsi/pm8001/pm8001_sas.h
> @@ -632,7 +632,6 @@ extern struct workqueue_struct *pm8001_wq;
>
>  /******************** function prototype *********************/
>  int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out);
> -void pm8001_tag_init(struct pm8001_hba_info *pm8001_ha);
>  u32 pm8001_get_ncq_tag(struct sas_task *task, u32 *tag);
>  void pm8001_ccb_task_free(struct pm8001_hba_info *pm8001_ha,
>                           struct pm8001_ccb_info *ccb);
> --
> 2.35.3
>

Re: [PATCH v2 4/6] scsi: pm8001: Use sas_task_find_rq() for tagging

[Jinpu Wang]

On Fri, Sep 30, 2022 at 11:03 AM John Garry <john.garry@huawei.com> wrote:
>
> The request associated with a scsi command coming from the block layer
> has a unique tag, so use that when possible for getting a CCB.
>
> Unfortunately we don't support reserved commands in the SCSI midlayer yet,
> so in the interim continue to manage those tags internally (along with
> tags for private commands).
>
> Signed-off-by: John Garry <john.garry@huawei.com>
Reviewed-by: Jack Wang <jinpu.wang@ionos.com>
nice, I would expect this can improve performance, do you have numbers?
> ---
>  drivers/scsi/pm8001/pm8001_init.c | 12 ++++--------
>  drivers/scsi/pm8001/pm8001_sas.c  | 16 ++++++++++++----
>  drivers/scsi/pm8001/pm8001_sas.h  | 11 ++++++++---
>  drivers/scsi/pm8001/pm80xx_hwi.c  | 17 +++--------------
>  4 files changed, 27 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index 0edc9857a8bd..abb884ddcaf9 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -196,7 +196,7 @@ static void pm8001_free(struct pm8001_hba_info *pm8001_ha)
>         }
>         PM8001_CHIP_DISP->chip_iounmap(pm8001_ha);
>         flush_workqueue(pm8001_wq);
> -       bitmap_free(pm8001_ha->tags);
> +       bitmap_free(pm8001_ha->rsvd_tags);
>         kfree(pm8001_ha);
>  }
>
> @@ -1208,18 +1208,15 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
>         struct Scsi_Host *shost = pm8001_ha->shost;
>         struct device *dev = pm8001_ha->dev;
>         u32 max_out_io, ccb_count;
> -       u32 can_queue;
>         int i;
>
>         max_out_io = pm8001_ha->main_cfg_tbl.pm80xx_tbl.max_out_io;
>         ccb_count = min_t(int, PM8001_MAX_CCB, max_out_io);
>
> -       /* Update to the scsi host*/
> -       can_queue = ccb_count - PM8001_RESERVE_SLOT;
> -       shost->can_queue = can_queue;
> +       shost->can_queue = ccb_count - PM8001_RESERVE_SLOT;
>
> -       pm8001_ha->tags = bitmap_zalloc(ccb_count, GFP_KERNEL);
> -       if (!pm8001_ha->tags)
> +       pm8001_ha->rsvd_tags = bitmap_zalloc(PM8001_RESERVE_SLOT, GFP_KERNEL);
> +       if (!pm8001_ha->rsvd_tags)
>                 goto err_out;
>
>         /* Memory region for ccb_info*/
> @@ -1244,7 +1241,6 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
>                 pm8001_ha->ccb_info[i].task = NULL;
>                 pm8001_ha->ccb_info[i].ccb_tag = PM8001_INVALID_TAG;
>                 pm8001_ha->ccb_info[i].device = NULL;
> -               ++pm8001_ha->tags_num;
>         }
>
>         return 0;
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> index 066dfa9f4683..d60bc311a4e9 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -65,9 +65,14 @@ static int pm8001_find_tag(struct sas_task *task, u32 *tag)
>    */
>  void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag)
>  {
> -       void *bitmap = pm8001_ha->tags;
> +       void *bitmap = pm8001_ha->rsvd_tags;
>         unsigned long flags;
>
> +       if (tag < pm8001_ha->shost->can_queue)
> +               return;
> +
> +       tag -= pm8001_ha->shost->can_queue;
> +
>         spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
>         __clear_bit(tag, bitmap);
>         spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
> @@ -80,18 +85,21 @@ void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag)
>    */
>  int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out)
>  {
> -       void *bitmap = pm8001_ha->tags;
> +       void *bitmap = pm8001_ha->rsvd_tags;
>         unsigned long flags;
>         unsigned int tag;
>
>         spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
> -       tag = find_first_zero_bit(bitmap, pm8001_ha->tags_num);
> -       if (tag >= pm8001_ha->tags_num) {
> +       tag = find_first_zero_bit(bitmap, PM8001_RESERVE_SLOT);
> +       if (tag >= PM8001_RESERVE_SLOT) {
>                 spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
>                 return -SAS_QUEUE_FULL;
>         }
>         __set_bit(tag, bitmap);
>         spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
> +
> +       /* reserved tags are in the upper region of the tagset */
> +       tag += pm8001_ha->shost->can_queue;
>         *tag_out = tag;
>         return 0;
>  }
> diff --git a/drivers/scsi/pm8001/pm8001_sas.h b/drivers/scsi/pm8001/pm8001_sas.h
> index 9acaadf02150..ba32b009f412 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.h
> +++ b/drivers/scsi/pm8001/pm8001_sas.h
> @@ -510,8 +510,7 @@ struct pm8001_hba_info {
>         u32                     chip_id;
>         const struct pm8001_chip_info   *chip;
>         struct completion       *nvmd_completion;
> -       int                     tags_num;
> -       unsigned long           *tags;
> +       unsigned long           *rsvd_tags;
>         struct pm8001_phy       phy[PM8001_MAX_PHYS];
>         struct pm8001_port      port[PM8001_MAX_PHYS];
>         u32                     id;
> @@ -737,9 +736,15 @@ pm8001_ccb_alloc(struct pm8001_hba_info *pm8001_ha,
>                  struct pm8001_device *dev, struct sas_task *task)
>  {
>         struct pm8001_ccb_info *ccb;
> +       struct request *rq = NULL;
>         u32 tag;
>
> -       if (pm8001_tag_alloc(pm8001_ha, &tag)) {
> +       if (task)
> +               rq = sas_task_find_rq(task);
> +
> +       if (rq) {
> +               tag = rq->tag;
> +       } else if (pm8001_tag_alloc(pm8001_ha, &tag)) {
>                 pm8001_dbg(pm8001_ha, FAIL, "Failed to allocate a tag\n");
>                 return NULL;
>         }
> diff --git a/drivers/scsi/pm8001/pm80xx_hwi.c b/drivers/scsi/pm8001/pm80xx_hwi.c
> index 4484c498bcb6..ed2d65d3749a 100644
> --- a/drivers/scsi/pm8001/pm80xx_hwi.c
> +++ b/drivers/scsi/pm8001/pm80xx_hwi.c
> @@ -4247,24 +4247,13 @@ static int check_enc_sat_cmd(struct sas_task *task)
>
>  static u32 pm80xx_chip_get_q_index(struct sas_task *task)
>  {
> -       struct scsi_cmnd *scmd = NULL;
> +       struct request *rq = sas_task_find_rq(task);
>         u32 blk_tag;
>
> -       if (task->uldd_task) {
> -               struct ata_queued_cmd *qc;
> -
> -               if (dev_is_sata(task->dev)) {
> -                       qc = task->uldd_task;
> -                       scmd = qc->scsicmd;
> -               } else {
> -                       scmd = task->uldd_task;
> -               }
> -       }
> -
> -       if (!scmd)
> +       if (!rq)
>                 return 0;
>
> -       blk_tag = blk_mq_unique_tag(scsi_cmd_to_rq(scmd));
> +       blk_tag = blk_mq_unique_tag(rq);
>         return blk_mq_unique_tag_to_hwq(blk_tag);
>  }
>
> --
> 2.35.3
>

Re: [PATCH v2 1/6] scsi: libsas: Add sas_task_find_rq()

[Jason Yan]

On 2022/9/30 16:56, John Garry wrote:
> blk-mq already provides a unique tag per request. Some libsas LLDDs - like
> hisi_sas - already use this tag as the unique per-IO HW tag.
> 
> Add a common function to provide the request associated with a sas_task
> for all libsas LLDDs.
> 
> Signed-off-by: John Garry<john.garry@huawei.com>
> ---
>   include/scsi/libsas.h | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)

Reviewed-by: Jason Yan <yanaijie@huawei.com>

Re: [PATCH v2 4/6] scsi: pm8001: Use sas_task_find_rq() for tagging

[John Garry]

On 30/09/2022 10:17, Jinpu Wang wrote:
> On Fri, Sep 30, 2022 at 11:03 AM John Garry<john.garry@huawei.com>  wrote:
>> The request associated with a scsi command coming from the block layer
>> has a unique tag, so use that when possible for getting a CCB.
>>
>> Unfortunately we don't support reserved commands in the SCSI midlayer yet,
>> so in the interim continue to manage those tags internally (along with
>> tags for private commands).
>>
>> Signed-off-by: John Garry<john.garry@huawei.com>
> Reviewed-by: Jack Wang<jinpu.wang@ionos.com>
> nice, I would expect this can improve performance, do you have numbers?

Unfortunately my system hangs after I run for an appreciable period of 
time. I normally get around it by turning on much heavy debug options, 
but that would not be much use for performance testing.

But we did get considerable performance improvement for hisi_sas when we 
made the equivalent change.

Damien, if you are interested then sharing any results would be great.

BTW, I do notice that we still have this global lock in delivery path 
which should be removed at some stage:

int mvs_queue_command(struct sas_task *task, gfp_t gfp_flags)
{
	...

	spin_lock_irqsave(&mvi->lock, flags);
	rc = mvs_task_prep(task, mvi, is_tmf, tmf, &pass);
	...
	spin_unlock_irqrestore(&mvi->lock, flags);
}

That really will affect performance...

Thanks,
John

Re: [PATCH v2 4/6] scsi: pm8001: Use sas_task_find_rq() for tagging

[John Garry]

On 30/09/2022 11:20, John Garry wrote:
> BTW, I do notice that we still have this global lock in delivery path 
> which should be removed at some stage:
> > int mvs_queue_command(struct sas_task *task, gfp_t gfp_flags)
> {
>      ...
> 
>      spin_lock_irqsave(&mvi->lock, flags);
>      rc = mvs_task_prep(task, mvi, is_tmf, tmf, &pass);
>      ...
>      spin_unlock_irqrestore(&mvi->lock, flags);
> }
> 

oops... that's mvsas. But pm8001 does still use a big lock (which we 
should get rid off):

int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags)
{
	...
	pm8001_dbg(pm8001_ha, IO, "pm8001_task_exec device\n");

	spin_lock_irqsave(&pm8001_ha->lock, flags);


Thanks,
John

> That really will affect performance...

Re: [PATCH v2 4/6] scsi: pm8001: Use sas_task_find_rq() for tagging

[Jinpu Wang]

On Fri, Sep 30, 2022 at 1:05 PM John Garry <john.garry@huawei.com> wrote:
>
> On 30/09/2022 11:20, John Garry wrote:
> > BTW, I do notice that we still have this global lock in delivery path
> > which should be removed at some stage:
> > > int mvs_queue_command(struct sas_task *task, gfp_t gfp_flags)
> > {
> >      ...
> >
> >      spin_lock_irqsave(&mvi->lock, flags);
> >      rc = mvs_task_prep(task, mvi, is_tmf, tmf, &pass);
> >      ...
> >      spin_unlock_irqrestore(&mvi->lock, flags);
> > }
> >
>
> oops... that's mvsas. But pm8001 does still use a big lock (which we
> should get rid off):
Yes, would be great to get rid of the per ha lock.
>
> int pm8001_queue_command(struct sas_task *task, gfp_t gfp_flags)
> {
>         ...
>         pm8001_dbg(pm8001_ha, IO, "pm8001_task_exec device\n");
>
>         spin_lock_irqsave(&pm8001_ha->lock, flags);
>
>
> Thanks,
> John
>
> > That really will affect performance...
>

Re: [PATCH v2 1/6] scsi: libsas: Add sas_task_find_rq()

[Hannes Reinecke]

On 9/30/22 10:56, John Garry wrote:
> blk-mq already provides a unique tag per request. Some libsas LLDDs - like
> hisi_sas - already use this tag as the unique per-IO HW tag.
> 
> Add a common function to provide the request associated with a sas_task
> for all libsas LLDDs.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>   include/scsi/libsas.h | 18 ++++++++++++++++++
>   1 file changed, 18 insertions(+)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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 v2 2/6] scsi: hisi_sas: Use sas_task_find_rq()

[Hannes Reinecke]

On 9/30/22 10:56, John Garry wrote:
> Use sas_task_find_rq() to lookup the request per task for its driver tag.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>   drivers/scsi/hisi_sas/hisi_sas_main.c | 26 ++++++++------------------
>   1 file changed, 8 insertions(+), 18 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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 v2 3/6] scsi: pm8001: Remove pm8001_tag_init()

[Hannes Reinecke]

On 9/30/22 10:56, John Garry wrote:
> From: Igor Pylypiv <ipylypiv@google.com>
> 
> In commit 5a141315ed7c ("scsi: pm80xx: Increase the number of outstanding
> I/O supported to 1024") the pm8001_ha->tags allocation was moved into
> pm8001_init_ccb_tag(). This changed the execution order of allocation.
> pm8001_tag_init() used to be called after the pm8001_ha->tags allocation
> and now it is called before the allocation.
> 
> Before:
> 
> pm8001_pci_probe()
> `--> pm8001_pci_alloc()
>       `--> pm8001_alloc()
>            `--> pm8001_ha->tags = kzalloc(...)
>            `--> pm8001_tag_init(pm8001_ha); // OK: tags are allocated
> 
> After:
> 
> pm8001_pci_probe()
> `--> pm8001_pci_alloc()
> |    `--> pm8001_alloc()
> |         `--> pm8001_tag_init(pm8001_ha); // NOK: tags are not allocated
> |
> `--> pm8001_init_ccb_tag()
>       `-->  pm8001_ha->tags = kzalloc(...) // today it is bitmap_zalloc()
> 
> Since pm8001_ha->tags_num is zero when pm8001_tag_init() is called it does
> nothing. Tags memory is allocated with bitmap_zalloc() so there is no need
> to manually clear each bit with pm8001_tag_free().
> 
> Reviewed-by: Changyuan Lyu <changyuanl@google.com>
> Signed-off-by: Igor Pylypiv <ipylypiv@google.com>
> Signed-off-by: John Garry <john.garry@huawei.com>
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>   drivers/scsi/pm8001/pm8001_init.c | 2 --
>   drivers/scsi/pm8001/pm8001_sas.c  | 7 -------
>   drivers/scsi/pm8001/pm8001_sas.h  | 1 -
>   3 files changed, 10 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

Cheerx,

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 v2 4/6] scsi: pm8001: Use sas_task_find_rq() for tagging

[Hannes Reinecke]

On 9/30/22 10:56, John Garry wrote:
> The request associated with a scsi command coming from the block layer
> has a unique tag, so use that when possible for getting a CCB.
> 
> Unfortunately we don't support reserved commands in the SCSI midlayer yet,
> so in the interim continue to manage those tags internally (along with
> tags for private commands).
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/scsi/pm8001/pm8001_init.c | 12 ++++--------
>   drivers/scsi/pm8001/pm8001_sas.c  | 16 ++++++++++++----
>   drivers/scsi/pm8001/pm8001_sas.h  | 11 ++++++++---
>   drivers/scsi/pm8001/pm80xx_hwi.c  | 17 +++--------------
>   4 files changed, 27 insertions(+), 29 deletions(-)
> 
> diff --git a/drivers/scsi/pm8001/pm8001_init.c b/drivers/scsi/pm8001/pm8001_init.c
> index 0edc9857a8bd..abb884ddcaf9 100644
> --- a/drivers/scsi/pm8001/pm8001_init.c
> +++ b/drivers/scsi/pm8001/pm8001_init.c
> @@ -196,7 +196,7 @@ static void pm8001_free(struct pm8001_hba_info *pm8001_ha)
>   	}
>   	PM8001_CHIP_DISP->chip_iounmap(pm8001_ha);
>   	flush_workqueue(pm8001_wq);
> -	bitmap_free(pm8001_ha->tags);
> +	bitmap_free(pm8001_ha->rsvd_tags);
>   	kfree(pm8001_ha);
>   }
>   
> @@ -1208,18 +1208,15 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
>   	struct Scsi_Host *shost = pm8001_ha->shost;
>   	struct device *dev = pm8001_ha->dev;
>   	u32 max_out_io, ccb_count;
> -	u32 can_queue;
>   	int i;
>   
>   	max_out_io = pm8001_ha->main_cfg_tbl.pm80xx_tbl.max_out_io;
>   	ccb_count = min_t(int, PM8001_MAX_CCB, max_out_io);
>   
> -	/* Update to the scsi host*/
> -	can_queue = ccb_count - PM8001_RESERVE_SLOT;
> -	shost->can_queue = can_queue;
> +	shost->can_queue = ccb_count - PM8001_RESERVE_SLOT;
>   
> -	pm8001_ha->tags = bitmap_zalloc(ccb_count, GFP_KERNEL);
> -	if (!pm8001_ha->tags)
> +	pm8001_ha->rsvd_tags = bitmap_zalloc(PM8001_RESERVE_SLOT, GFP_KERNEL);
> +	if (!pm8001_ha->rsvd_tags)
>   		goto err_out;
>   
>   	/* Memory region for ccb_info*/
> @@ -1244,7 +1241,6 @@ static int pm8001_init_ccb_tag(struct pm8001_hba_info *pm8001_ha)
>   		pm8001_ha->ccb_info[i].task = NULL;
>   		pm8001_ha->ccb_info[i].ccb_tag = PM8001_INVALID_TAG;
>   		pm8001_ha->ccb_info[i].device = NULL;
> -		++pm8001_ha->tags_num;
>   	}
>   
>   	return 0;
> diff --git a/drivers/scsi/pm8001/pm8001_sas.c b/drivers/scsi/pm8001/pm8001_sas.c
> index 066dfa9f4683..d60bc311a4e9 100644
> --- a/drivers/scsi/pm8001/pm8001_sas.c
> +++ b/drivers/scsi/pm8001/pm8001_sas.c
> @@ -65,9 +65,14 @@ static int pm8001_find_tag(struct sas_task *task, u32 *tag)
>     */
>   void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag)
>   {
> -	void *bitmap = pm8001_ha->tags;
> +	void *bitmap = pm8001_ha->rsvd_tags;
>   	unsigned long flags;
>   
> +	if (tag < pm8001_ha->shost->can_queue)
> +		return;
> +
> +	tag -= pm8001_ha->shost->can_queue;
> +
>   	spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
>   	__clear_bit(tag, bitmap);
>   	spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
> @@ -80,18 +85,21 @@ void pm8001_tag_free(struct pm8001_hba_info *pm8001_ha, u32 tag)
>     */
>   int pm8001_tag_alloc(struct pm8001_hba_info *pm8001_ha, u32 *tag_out)
>   {
> -	void *bitmap = pm8001_ha->tags;
> +	void *bitmap = pm8001_ha->rsvd_tags;
>   	unsigned long flags;
>   	unsigned int tag;
>   
>   	spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
> -	tag = find_first_zero_bit(bitmap, pm8001_ha->tags_num);
> -	if (tag >= pm8001_ha->tags_num) {
> +	tag = find_first_zero_bit(bitmap, PM8001_RESERVE_SLOT);
> +	if (tag >= PM8001_RESERVE_SLOT) {
>   		spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
>   		return -SAS_QUEUE_FULL;
>   	}
>   	__set_bit(tag, bitmap);
>   	spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
> +
> +	/* reserved tags are in the upper region of the tagset */
> +	tag += pm8001_ha->shost->can_queue;
>   	*tag_out = tag;
>   	return 0;
>   }
Can you move the reserved tags to the _lower_ region of the tagset?
That way the tag allocation scheme matches with what the block-layer 
does, and the eventual conversion to reserved tags becomes easier.

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 v2 5/6] scsi: mvsas: Delete mvs_tag_init()

[Hannes Reinecke]

On 9/30/22 10:56, John Garry wrote:
> All mvs_tag_init() does is zero the tag bitmap, but this is already done
> with the kzalloc() call to alloc the tags, so delete this unneeded
> function.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> Reviewed-by: Damien Le Moal <damien.lemoal@opensource.wdc.com>
> ---
>   drivers/scsi/mvsas/mv_init.c | 2 --
>   drivers/scsi/mvsas/mv_sas.c  | 7 -------
>   drivers/scsi/mvsas/mv_sas.h  | 1 -
>   3 files changed, 10 deletions(-)
> 
Reviewed-by: Hannes Reinecke <hare@suse.de>

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 v2 6/6] scsi: mvsas: Use sas_task_find_rq() for tagging

[Hannes Reinecke]

On 9/30/22 10:56, John Garry wrote:
> The request associated with a scsi command coming from the block layer
> has a unique tag, so use that when possible for getting a slot.
> 
> Unfortunately we don't support reserved commands in the SCSI midlayer yet.
> As such, SMP tasks - as an example - will not have a request associated, so
> in the interim continue to manage those tags for that type of sas_task
> internally.
> 
> We reserve an arbitrary 4 tags for these internal tags. Indeed, we already
> decrement MVS_RSVD_SLOTS by 2 for the shost can_queue when flag
> MVF_FLAG_SOC is set. This change was made in commit 20b09c2992fef
> ("[PATCH] [SCSI] mvsas: add support for 94xx; layout change; bug fixes"),
> but what those 2 slots are used for is not obvious.
> 
> Also make the tag management functions static, where possible.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>   drivers/scsi/mvsas/mv_defs.h |  1 +
>   drivers/scsi/mvsas/mv_init.c |  9 +++++----
>   drivers/scsi/mvsas/mv_sas.c  | 38 ++++++++++++++++++++++++------------
>   drivers/scsi/mvsas/mv_sas.h  |  7 +------
>   4 files changed, 32 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/scsi/mvsas/mv_defs.h b/drivers/scsi/mvsas/mv_defs.h
> index 7123a2efbf58..8ef174cd4d37 100644
> --- a/drivers/scsi/mvsas/mv_defs.h
> +++ b/drivers/scsi/mvsas/mv_defs.h
> @@ -40,6 +40,7 @@ enum driver_configuration {
>   	MVS_ATA_CMD_SZ		= 96,	/* SATA command table buffer size */
>   	MVS_OAF_SZ		= 64,	/* Open address frame buffer size */
>   	MVS_QUEUE_SIZE		= 64,	/* Support Queue depth */
> +	MVS_RSVD_SLOTS		= 4,
>   	MVS_SOC_CAN_QUEUE	= MVS_SOC_SLOTS - 2,
>   };
>   
> diff --git a/drivers/scsi/mvsas/mv_init.c b/drivers/scsi/mvsas/mv_init.c
> index c85fb812ad43..cfe84473a515 100644
> --- a/drivers/scsi/mvsas/mv_init.c
> +++ b/drivers/scsi/mvsas/mv_init.c
> @@ -142,7 +142,7 @@ static void mvs_free(struct mvs_info *mvi)
>   		scsi_host_put(mvi->shost);
>   	list_for_each_entry(mwq, &mvi->wq_list, entry)
>   		cancel_delayed_work(&mwq->work_q);
> -	kfree(mvi->tags);
> +	kfree(mvi->rsvd_tags);
>   	kfree(mvi);
>   }
>   
> @@ -284,7 +284,6 @@ static int mvs_alloc(struct mvs_info *mvi, struct Scsi_Host *shost)
>   			printk(KERN_DEBUG "failed to create dma pool %s.\n", pool_name);
>   			goto err_out;
>   	}
> -	mvi->tags_num = slot_nr;
>   
>   	return 0;
>   err_out:
> @@ -367,8 +366,8 @@ static struct mvs_info *mvs_pci_alloc(struct pci_dev *pdev,
>   	mvi->sas = sha;
>   	mvi->shost = shost;
>   
> -	mvi->tags = kzalloc(MVS_CHIP_SLOT_SZ>>3, GFP_KERNEL);
> -	if (!mvi->tags)
> +	mvi->rsvd_tags = bitmap_zalloc(MVS_RSVD_SLOTS, GFP_KERNEL);
> +	if (!mvi->rsvd_tags)
>   		goto err_out;
>   
>   	if (MVS_CHIP_DISP->chip_ioremap(mvi))
> @@ -469,6 +468,8 @@ static void  mvs_post_sas_ha_init(struct Scsi_Host *shost,
>   	else
>   		can_queue = MVS_CHIP_SLOT_SZ;
>   
> +	can_queue -= MVS_RSVD_SLOTS;
> +
>   	shost->sg_tablesize = min_t(u16, SG_ALL, MVS_MAX_SG);
>   	shost->can_queue = can_queue;
>   	mvi->shost->cmd_per_lun = MVS_QUEUE_SIZE;
> diff --git a/drivers/scsi/mvsas/mv_sas.c b/drivers/scsi/mvsas/mv_sas.c
> index 0810e6c930e1..00b3a2781d21 100644
> --- a/drivers/scsi/mvsas/mv_sas.c
> +++ b/drivers/scsi/mvsas/mv_sas.c
> @@ -20,33 +20,39 @@ static int mvs_find_tag(struct mvs_info *mvi, struct sas_task *task, u32 *tag)
>   	return 0;
>   }
>   
> -void mvs_tag_clear(struct mvs_info *mvi, u32 tag)
> +static void mvs_tag_clear(struct mvs_info *mvi, u32 tag)
>   {
> -	void *bitmap = mvi->tags;
> +	void *bitmap = mvi->rsvd_tags;
>   	clear_bit(tag, bitmap);
>   }
>   
> -void mvs_tag_free(struct mvs_info *mvi, u32 tag)
> +static void mvs_tag_free(struct mvs_info *mvi, u32 tag)
>   {
> +	if (tag < mvi->shost->can_queue)
> +		return;
> +
> +	tag -= mvi->shost->can_queue;
> +
>   	mvs_tag_clear(mvi, tag);
>   }
>   
> -void mvs_tag_set(struct mvs_info *mvi, unsigned int tag)
> +static void mvs_tag_set(struct mvs_info *mvi, unsigned int tag)
>   {
> -	void *bitmap = mvi->tags;
> +	void *bitmap = mvi->rsvd_tags;
>   	set_bit(tag, bitmap);
>   }
>   
> -inline int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out)
> +static int mvs_tag_alloc(struct mvs_info *mvi, u32 *tag_out)
>   {
>   	unsigned int index, tag;
> -	void *bitmap = mvi->tags;
> +	void *bitmap = mvi->rsvd_tags;
>   
> -	index = find_first_zero_bit(bitmap, mvi->tags_num);
> +	index = find_first_zero_bit(bitmap, MVS_RSVD_SLOTS);
>   	tag = index;
> -	if (tag >= mvi->tags_num)
> +	if (tag >= MVS_RSVD_SLOTS)
>   		return -SAS_QUEUE_FULL;
>   	mvs_tag_set(mvi, tag);
> +	tag += mvi->shost->can_queue;
>   	*tag_out = tag;
>   	return 0;
>   }

Also here: please move the reserved tags to the front, such that the tag 
allocation scheme matches with the blk-mq tag allocation scheme.

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 v2 4/6] scsi: pm8001: Use sas_task_find_rq() for tagging

[John Garry]

On 04/10/2022 06:53, Hannes Reinecke wrote:
>> -    void *bitmap = pm8001_ha->tags;
>> +    void *bitmap = pm8001_ha->rsvd_tags;
>>       unsigned long flags;
>>       unsigned int tag;
>>       spin_lock_irqsave(&pm8001_ha->bitmap_lock, flags);
>> -    tag = find_first_zero_bit(bitmap, pm8001_ha->tags_num);
>> -    if (tag >= pm8001_ha->tags_num) {
>> +    tag = find_first_zero_bit(bitmap, PM8001_RESERVE_SLOT);
>> +    if (tag >= PM8001_RESERVE_SLOT) {
>>           spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
>>           return -SAS_QUEUE_FULL;
>>       }
>>       __set_bit(tag, bitmap);
>>       spin_unlock_irqrestore(&pm8001_ha->bitmap_lock, flags);
>> +
>> +    /* reserved tags are in the upper region of the tagset */
>> +    tag += pm8001_ha->shost->can_queue;
>>       *tag_out = tag;
>>       return 0;
>>   }
> Can you move the reserved tags to the _lower_ region of the tagset?
> That way the tag allocation scheme matches with what the block-layer 
> does, and the eventual conversion to reserved tags becomes easier.

Yeah, I agree that having a scheme which matches the block layer would 
be good for consistency and I will make that change, but I am not sure 
how it helps conversion to reserved tags.

Thanks,
John