[PATCH 0/7] hisi_sas: Misc bugfixes and an optimisation patch

[PATCH 0/7] hisi_sas: Misc bugfixes and an optimisation patch

[John Garry]

This patchset introduces mostly more minor/obscure bugfixes for the
driver.

Also included is an optimisation to use the block layer tag for the IPTT
indexing. This quite a nice optimisation as it means we don't have to
evaluate this in the driver - it was a bit of a bottle-neck.

However it does block us in future from enabling SCSI MQ in the driver.
This is because the IPTT index must be a unique value per HBA. However,
if we switched to SCSI MQ, the block layer tag becomes unique per queue,
and not per HBA.

Having said this, testing has shown that performance is better by using
this block layer tag instead of enabling SCSI MQ in the driver.

Luo Jiaxing (2):
  scsi: hisi_sas: Feed back linkrate(max/min) when re-attached
  scsi: hisi_sas: Move evaluation of hisi_hba in hisi_sas_task_prep()

Xiang Chen (5):
  scsi: hisi_sas: Fix the race between IO completion and timeout for
    SMP/internal IO
  scsi: hisi_sas: Free slot later in slot_complete_vx_hw()
  scsi: hisi_sas: unmask interrupts ent72 and ent74
  scsi: hisi_sas: Use block layer tag instead for IPTT
  scsi: hisi_sas: Update v3 hw AIP_LIMIT and CFG_AGING_TIME register
    values

 drivers/scsi/hisi_sas/hisi_sas.h       |   3 +-
 drivers/scsi/hisi_sas/hisi_sas_main.c  | 154 ++++++++++++++++++++++++---------
 drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |   1 -
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |  11 +--
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |  15 ++--
 5 files changed, 130 insertions(+), 54 deletions(-)

-- 
1.9.1

[PATCH 1/7] scsi: hisi_sas: Feed back linkrate(max/min) when re-attached

[John Garry]

From: Luo Jiaxing <luojiaxing@huawei.com>

At directly attached situation, if the user modifies the sysfs interface
of maximum_linkrate and minimum_linkrate to renegotiate the linkrate
between SAS controller and target, the value of both files mentioned above
should have change to user setting after renegotiate is over, but it remain
unchanged.

To fix this bug, maximum_linkrate and minimum_linkrate will be directly
fed back to relevant sas_phy structure.

Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index a4e2e6a..ba6fb535 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -904,6 +904,9 @@ static void hisi_sas_phy_set_linkrate(struct hisi_hba *hisi_hba, int phy_no,
 	_r.maximum_linkrate = max;
 	_r.minimum_linkrate = min;
 
+	sas_phy->phy->maximum_linkrate = max;
+	sas_phy->phy->minimum_linkrate = min;
+
 	hisi_hba->hw->phy_disable(hisi_hba, phy_no);
 	msleep(100);
 	hisi_hba->hw->phy_set_linkrate(hisi_hba, phy_no, &_r);
-- 
1.9.1

[PATCH 2/7] scsi: hisi_sas: Move evaluation of hisi_hba in hisi_sas_task_prep()

[John Garry]

From: Luo Jiaxing <luojiaxing@huawei.com>

In evaluating hisi_hba, the sas_port may be NULL, so for safety relocate
the the check to value possible NULL deference.

Signed-off-by: Luo Jiaxing <luojiaxing@huawei.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index ba6fb535..3b95a7a 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -287,13 +287,13 @@ static int hisi_sas_task_prep(struct sas_task *task,
 			      int *pass)
 {
 	struct domain_device *device = task->dev;
-	struct hisi_hba *hisi_hba = dev_to_hisi_hba(device);
+	struct hisi_hba *hisi_hba;
 	struct hisi_sas_device *sas_dev = device->lldd_dev;
 	struct hisi_sas_port *port;
 	struct hisi_sas_slot *slot;
 	struct hisi_sas_cmd_hdr	*cmd_hdr_base;
 	struct asd_sas_port *sas_port = device->port;
-	struct device *dev = hisi_hba->dev;
+	struct device *dev;
 	int dlvry_queue_slot, dlvry_queue, rc, slot_idx;
 	int n_elem = 0, n_elem_req = 0, n_elem_resp = 0;
 	struct hisi_sas_dq *dq;
@@ -314,6 +314,9 @@ static int hisi_sas_task_prep(struct sas_task *task,
 		return -ECOMM;
 	}
 
+	hisi_hba = dev_to_hisi_hba(device);
+	dev = hisi_hba->dev;
+
 	if (DEV_IS_GONE(sas_dev)) {
 		if (sas_dev)
 			dev_info(dev, "task prep: device %d not ready\n",
-- 
1.9.1

[PATCH 3/7] scsi: hisi_sas: Fix the race between IO completion and timeout for SMP/internal IO

[John Garry]

From: Xiang Chen <chenxiang66@hisilicon.com>

If SMP/internal IO times out, we will possibly free the task immediately.

However if the IO actually completes at the same time, the IO completion
may refer to task which have been freed.

So to solve the issue, flush the tasklet to finish IO completion before
free'ing slot/task.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_main.c | 55 +++++++++++++++++++++++++++++------
 1 file changed, 46 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 3b95a7a..416f2c0 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -956,8 +956,7 @@ static int hisi_sas_control_phy(struct asd_sas_phy *sas_phy, enum phy_func func,
 
 static void hisi_sas_task_done(struct sas_task *task)
 {
-	if (!del_timer(&task->slow_task->timer))
-		return;
+	del_timer(&task->slow_task->timer);
 	complete(&task->slow_task->completion);
 }
 
@@ -966,13 +965,17 @@ static void hisi_sas_tmf_timedout(struct timer_list *t)
 	struct sas_task_slow *slow = from_timer(slow, t, timer);
 	struct sas_task *task = slow->task;
 	unsigned long flags;
+	bool is_completed = true;
 
 	spin_lock_irqsave(&task->task_state_lock, flags);
-	if (!(task->task_state_flags & SAS_TASK_STATE_DONE))
+	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);
 
-	complete(&task->slow_task->completion);
+	if (!is_completed)
+		complete(&task->slow_task->completion);
 }
 
 #define TASK_TIMEOUT 20
@@ -1023,10 +1026,18 @@ static int hisi_sas_exec_internal_tmf_task(struct domain_device *device,
 		if ((task->task_state_flags & SAS_TASK_STATE_ABORTED)) {
 			if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
 				struct hisi_sas_slot *slot = task->lldd_task;
+				struct hisi_sas_cq *cq =
+					&hisi_hba->cq[slot->dlvry_queue];
 
 				dev_err(dev, "abort tmf: TMF task timeout and not done\n");
-				if (slot)
+				if (slot) {
+					/*
+					 * flush tasklet to avoid free'ing task
+					 * before using task in IO completion
+					 */
+					tasklet_kill(&cq->tasklet);
 					slot->task = NULL;
+				}
 
 				goto ex_err;
 			} else
@@ -1402,6 +1413,17 @@ static int hisi_sas_abort_task(struct sas_task *task)
 
 	spin_lock_irqsave(&task->task_state_lock, flags);
 	if (task->task_state_flags & SAS_TASK_STATE_DONE) {
+		struct hisi_sas_slot *slot = task->lldd_task;
+		struct hisi_sas_cq *cq;
+
+		if (slot) {
+			/*
+			 * flush tasklet to avoid free'ing task
+			 * before using task in IO completion
+			 */
+			cq = &hisi_hba->cq[slot->dlvry_queue];
+			tasklet_kill(&cq->tasklet);
+		}
 		spin_unlock_irqrestore(&task->task_state_lock, flags);
 		rc = TMF_RESP_FUNC_COMPLETE;
 		goto out;
@@ -1457,12 +1479,19 @@ static int hisi_sas_abort_task(struct sas_task *task)
 		/* SMP */
 		struct hisi_sas_slot *slot = task->lldd_task;
 		u32 tag = slot->idx;
+		struct hisi_sas_cq *cq = &hisi_hba->cq[slot->dlvry_queue];
 
 		rc = hisi_sas_internal_task_abort(hisi_hba, device,
 			     HISI_SAS_INT_ABT_CMD, tag);
 		if (((rc < 0) || (rc == TMF_RESP_FUNC_FAILED)) &&
-					task->lldd_task)
-			hisi_sas_do_release_task(hisi_hba, task, slot);
+					task->lldd_task) {
+			/*
+			 * flush tasklet to avoid free'ing task
+			 * before using task in IO completion
+			 */
+			tasklet_kill(&cq->tasklet);
+			slot->task = NULL;
+		}
 	}
 
 out:
@@ -1828,9 +1857,17 @@ static int hisi_sas_query_task(struct sas_task *task)
 	if ((task->task_state_flags & SAS_TASK_STATE_ABORTED)) {
 		if (!(task->task_state_flags & SAS_TASK_STATE_DONE)) {
 			struct hisi_sas_slot *slot = task->lldd_task;
-
-			if (slot)
+			struct hisi_sas_cq *cq =
+				&hisi_hba->cq[slot->dlvry_queue];
+
+			if (slot) {
+				/*
+				 * flush tasklet to avoid free'ing task
+				 * before using task in IO completion
+				 */
+				tasklet_kill(&cq->tasklet);
 				slot->task = NULL;
+			}
 			dev_err(dev, "internal task abort: timeout and not done.\n");
 			res = -EIO;
 			goto exit;
-- 
1.9.1

[PATCH 4/7] scsi: hisi_sas: Free slot later in slot_complete_vx_hw()

[John Garry]

From: Xiang Chen <chenxiang66@hisilicon.com>

If an SSP/SMP IO times out, it may be actually in reality be
simultaneously processing completion of the slot in slot_complete_vx_hw().

Then if the slot is freed in slot_complete_vx_hw() (this IPTT is freed and
it may be re-used by other slot), and we may abort the wrong slot in
hisi_sas_abort_task().

So to solve the issue, free the slot after the check of
SAS_TASK_STATE_ABORTED in slot_complete_vx_hw().

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 2 +-
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index 9c5c5a6..67134b4 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -2483,7 +2483,6 @@ static void slot_err_v2_hw(struct hisi_hba *hisi_hba,
 	}
 
 out:
-	hisi_sas_slot_task_free(hisi_hba, task, slot);
 	sts = ts->stat;
 	spin_lock_irqsave(&task->task_state_lock, flags);
 	if (task->task_state_flags & SAS_TASK_STATE_ABORTED) {
@@ -2493,6 +2492,7 @@ static void slot_err_v2_hw(struct hisi_hba *hisi_hba,
 	}
 	task->task_state_flags |= SAS_TASK_STATE_DONE;
 	spin_unlock_irqrestore(&task->task_state_lock, flags);
+	hisi_sas_slot_task_free(hisi_hba, task, slot);
 
 	if (!is_internal && (task->task_proto != SAS_PROTOCOL_SMP)) {
 		spin_lock_irqsave(&device->done_lock, flags);
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 08b503e2..3995ff6 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -1751,7 +1751,6 @@ static irqreturn_t fatal_axi_int_v3_hw(int irq_no, void *p)
 	}
 
 out:
-	hisi_sas_slot_task_free(hisi_hba, task, slot);
 	sts = ts->stat;
 	spin_lock_irqsave(&task->task_state_lock, flags);
 	if (task->task_state_flags & SAS_TASK_STATE_ABORTED) {
@@ -1761,6 +1760,7 @@ static irqreturn_t fatal_axi_int_v3_hw(int irq_no, void *p)
 	}
 	task->task_state_flags |= SAS_TASK_STATE_DONE;
 	spin_unlock_irqrestore(&task->task_state_lock, flags);
+	hisi_sas_slot_task_free(hisi_hba, task, slot);
 
 	if (!is_internal && (task->task_proto != SAS_PROTOCOL_SMP)) {
 		spin_lock_irqsave(&device->done_lock, flags);
-- 
1.9.1

[PATCH 5/7] scsi: hisi_sas: unmask interrupts ent72 and ent74

[John Garry]

From: Xiang Chen <chenxiang66@hisilicon.com>

The interrupts of ent72 and ent74 are not processed by PCIe AER handling,
so we need to unmask the interrupts and process them first in the driver.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 3995ff6..34c8f30 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -441,7 +441,7 @@ static void init_reg_v3_hw(struct hisi_hba *hisi_hba)
 	hisi_sas_write32(hisi_hba, ENT_INT_SRC_MSK1, 0xfefefefe);
 	hisi_sas_write32(hisi_hba, ENT_INT_SRC_MSK2, 0xfefefefe);
 	if (pdev->revision >= 0x21)
-		hisi_sas_write32(hisi_hba, ENT_INT_SRC_MSK3, 0xffff7fff);
+		hisi_sas_write32(hisi_hba, ENT_INT_SRC_MSK3, 0xffff7aff);
 	else
 		hisi_sas_write32(hisi_hba, ENT_INT_SRC_MSK3, 0xfffe20ff);
 	hisi_sas_write32(hisi_hba, CHNL_PHYUPDOWN_INT_MSK, 0x0);
-- 
1.9.1

[PATCH 6/7] scsi: hisi_sas: Use block layer tag instead for IPTT

[John Garry]

From: Xiang Chen <chenxiang66@hisilicon.com>

Currently we use the IPTT defined in LLDD to identify IOs. Actually for
IOs which are from the block layer, they have tags to identify them. So
for those IOs, use tag of the block layer directly, and for IOs which is
not from the block layer (such as internal IOs from libsas/LLDD), reserve
96 IPTTs for them.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas.h       |  3 +-
 drivers/scsi/hisi_sas/hisi_sas_main.c  | 89 ++++++++++++++++++++++------------
 drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |  1 -
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |  9 ++--
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |  8 +--
 5 files changed, 70 insertions(+), 40 deletions(-)

diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h
index 6c7d2e2..0ddb53c 100644
--- a/drivers/scsi/hisi_sas/hisi_sas.h
+++ b/drivers/scsi/hisi_sas/hisi_sas.h
@@ -34,6 +34,7 @@
 #define HISI_SAS_MAX_DEVICES HISI_SAS_MAX_ITCT_ENTRIES
 #define HISI_SAS_RESET_BIT	0
 #define HISI_SAS_REJECT_CMD_BIT	1
+#define HISI_SAS_RESERVED_IPTT_CNT  96
 
 #define HISI_SAS_STATUS_BUF_SZ (sizeof(struct hisi_sas_status_buffer))
 #define HISI_SAS_COMMAND_TABLE_SZ (sizeof(union hisi_sas_command_table))
@@ -217,7 +218,7 @@ struct hisi_sas_hw {
 	int (*hw_init)(struct hisi_hba *hisi_hba);
 	void (*setup_itct)(struct hisi_hba *hisi_hba,
 			   struct hisi_sas_device *device);
-	int (*slot_index_alloc)(struct hisi_hba *hisi_hba, int *slot_idx,
+	int (*slot_index_alloc)(struct hisi_hba *hisi_hba,
 				struct domain_device *device);
 	struct hisi_sas_device *(*alloc_dev)(struct domain_device *device);
 	void (*sl_notify)(struct hisi_hba *hisi_hba, int phy_no);
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 416f2c0..8fcd1ab 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -183,7 +183,14 @@ static void hisi_sas_slot_index_clear(struct hisi_hba *hisi_hba, int slot_idx)
 
 static void hisi_sas_slot_index_free(struct hisi_hba *hisi_hba, int slot_idx)
 {
-	hisi_sas_slot_index_clear(hisi_hba, slot_idx);
+	unsigned long flags;
+
+	if (hisi_hba->hw->slot_index_alloc || (slot_idx >=
+	    hisi_hba->hw->max_command_entries - HISI_SAS_RESERVED_IPTT_CNT)) {
+		spin_lock_irqsave(&hisi_hba->lock, flags);
+		hisi_sas_slot_index_clear(hisi_hba, slot_idx);
+		spin_unlock_irqrestore(&hisi_hba->lock, flags);
+	}
 }
 
 static void hisi_sas_slot_index_set(struct hisi_hba *hisi_hba, int slot_idx)
@@ -193,24 +200,34 @@ static void hisi_sas_slot_index_set(struct hisi_hba *hisi_hba, int slot_idx)
 	set_bit(slot_idx, bitmap);
 }
 
-static int hisi_sas_slot_index_alloc(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)
 {
-	unsigned int index;
+	int index;
 	void *bitmap = hisi_hba->slot_index_tags;
+	unsigned long flags;
 
+	if (scsi_cmnd)
+		return scsi_cmnd->request->tag;
+
+	spin_lock_irqsave(&hisi_hba->lock, flags);
 	index = find_next_zero_bit(bitmap, hisi_hba->slot_index_count,
-			hisi_hba->last_slot_index + 1);
+				   hisi_hba->last_slot_index + 1);
 	if (index >= hisi_hba->slot_index_count) {
-		index = find_next_zero_bit(bitmap, hisi_hba->slot_index_count,
-					   0);
-		if (index >= hisi_hba->slot_index_count)
+		index = find_next_zero_bit(bitmap,
+				hisi_hba->slot_index_count,
+				hisi_hba->hw->max_command_entries -
+				HISI_SAS_RESERVED_IPTT_CNT);
+		if (index >= hisi_hba->slot_index_count) {
+			spin_unlock_irqrestore(&hisi_hba->lock, flags);
 			return -SAS_QUEUE_FULL;
+		}
 	}
 	hisi_sas_slot_index_set(hisi_hba, index);
-	*slot_idx = index;
 	hisi_hba->last_slot_index = index;
+	spin_unlock_irqrestore(&hisi_hba->lock, flags);
 
-	return 0;
+	return index;
 }
 
 static void hisi_sas_slot_index_init(struct hisi_hba *hisi_hba)
@@ -249,9 +266,7 @@ void hisi_sas_slot_task_free(struct hisi_hba *hisi_hba, struct sas_task *task,
 
 	memset(slot, 0, offsetof(struct hisi_sas_slot, buf));
 
-	spin_lock_irqsave(&hisi_hba->lock, flags);
 	hisi_sas_slot_index_free(hisi_hba, slot->idx);
-	spin_unlock_irqrestore(&hisi_hba->lock, flags);
 }
 EXPORT_SYMBOL_GPL(hisi_sas_slot_task_free);
 
@@ -384,16 +399,27 @@ static int hisi_sas_task_prep(struct sas_task *task,
 		goto err_out_dma_unmap;
 	}
 
-	spin_lock_irqsave(&hisi_hba->lock, flags);
 	if (hisi_hba->hw->slot_index_alloc)
-		rc = hisi_hba->hw->slot_index_alloc(hisi_hba, &slot_idx,
-						    device);
-	else
-		rc = hisi_sas_slot_index_alloc(hisi_hba, &slot_idx);
-	spin_unlock_irqrestore(&hisi_hba->lock, flags);
-	if (rc)
+		rc = hisi_hba->hw->slot_index_alloc(hisi_hba, device);
+	else {
+		struct scsi_cmnd *scsi_cmnd = NULL;
+
+		if (task->uldd_task) {
+			struct ata_queued_cmd *qc;
+
+			if (dev_is_sata(device)) {
+				qc = task->uldd_task;
+				scsi_cmnd = qc->scsicmd;
+			} else {
+				scsi_cmnd = task->uldd_task;
+			}
+		}
+		rc  = hisi_sas_slot_index_alloc(hisi_hba, scsi_cmnd);
+	}
+	if (rc < 0)
 		goto err_out_dma_unmap;
 
+	slot_idx = rc;
 	slot = &hisi_hba->slot_info[slot_idx];
 
 	spin_lock_irqsave(&dq->lock, flags);
@@ -454,9 +480,7 @@ static int hisi_sas_task_prep(struct sas_task *task,
 	return 0;
 
 err_out_tag:
-	spin_lock_irqsave(&hisi_hba->lock, flags);
 	hisi_sas_slot_index_free(hisi_hba, slot_idx);
-	spin_unlock_irqrestore(&hisi_hba->lock, flags);
 err_out_dma_unmap:
 	if (!sas_protocol_ata(task->task_proto)) {
 		if (task->num_scatter) {
@@ -1740,14 +1764,11 @@ static int hisi_sas_query_task(struct sas_task *task)
 	port = to_hisi_sas_port(sas_port);
 
 	/* simply get a slot and send abort command */
-	spin_lock_irqsave(&hisi_hba->lock, flags);
-	rc = hisi_sas_slot_index_alloc(hisi_hba, &slot_idx);
-	if (rc) {
-		spin_unlock_irqrestore(&hisi_hba->lock, flags);
+	rc = hisi_sas_slot_index_alloc(hisi_hba, NULL);
+	if (rc < 0)
 		goto err_out;
-	}
-	spin_unlock_irqrestore(&hisi_hba->lock, flags);
 
+	slot_idx = rc;
 	slot = &hisi_hba->slot_info[slot_idx];
 
 	spin_lock_irqsave(&dq->lock, flags_dq);
@@ -1783,7 +1804,6 @@ static int hisi_sas_query_task(struct sas_task *task)
 	spin_lock_irqsave(&task->task_state_lock, flags);
 	task->task_state_flags |= SAS_TASK_AT_INITIATOR;
 	spin_unlock_irqrestore(&task->task_state_lock, flags);
-
 	WRITE_ONCE(slot->ready, 1);
 	/* send abort command to the chip */
 	spin_lock_irqsave(&dq->lock, flags);
@@ -1794,9 +1814,7 @@ static int hisi_sas_query_task(struct sas_task *task)
 	return 0;
 
 err_out_tag:
-	spin_lock_irqsave(&hisi_hba->lock, flags);
 	hisi_sas_slot_index_free(hisi_hba, slot_idx);
-	spin_unlock_irqrestore(&hisi_hba->lock, flags);
 err_out:
 	dev_err(dev, "internal abort task prep: failed[%d]!\n", rc);
 
@@ -2163,6 +2181,8 @@ int hisi_sas_alloc(struct hisi_hba *hisi_hba, struct Scsi_Host *shost)
 	hisi_sas_init_mem(hisi_hba);
 
 	hisi_sas_slot_index_init(hisi_hba);
+	hisi_hba->last_slot_index = hisi_hba->hw->max_command_entries -
+		HISI_SAS_RESERVED_IPTT_CNT;
 
 	hisi_hba->wq = create_singlethread_workqueue(dev_name(dev));
 	if (!hisi_hba->wq) {
@@ -2366,8 +2386,15 @@ int hisi_sas_probe(struct platform_device *pdev,
 	shost->max_channel = 1;
 	shost->max_cmd_len = 16;
 	shost->sg_tablesize = min_t(u16, SG_ALL, HISI_SAS_SGE_PAGE_CNT);
-	shost->can_queue = hisi_hba->hw->max_command_entries;
-	shost->cmd_per_lun = hisi_hba->hw->max_command_entries;
+	if (hisi_hba->hw->slot_index_alloc) {
+		shost->can_queue = hisi_hba->hw->max_command_entries;
+		shost->cmd_per_lun = hisi_hba->hw->max_command_entries;
+	} else {
+		shost->can_queue = hisi_hba->hw->max_command_entries -
+			HISI_SAS_RESERVED_IPTT_CNT;
+		shost->cmd_per_lun = hisi_hba->hw->max_command_entries -
+			HISI_SAS_RESERVED_IPTT_CNT;
+	}
 
 	sha->sas_ha_name = DRV_NAME;
 	sha->dev = hisi_hba->dev;
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
index 8f60f0e..f0e457e 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
@@ -1809,7 +1809,6 @@ static int hisi_sas_v1_init(struct hisi_hba *hisi_hba)
 	.scan_start		= hisi_sas_scan_start,
 	.change_queue_depth	= sas_change_queue_depth,
 	.bios_param		= sas_bios_param,
-	.can_queue		= 1,
 	.this_id		= -1,
 	.sg_tablesize		= SG_ALL,
 	.max_sectors		= SCSI_DEFAULT_MAX_SECTORS,
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
index 67134b4..70d6b28 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v2_hw.c
@@ -770,7 +770,7 @@ static u32 hisi_sas_phy_read32(struct hisi_hba *hisi_hba,
 
 /* This function needs to be protected from pre-emption. */
 static int
-slot_index_alloc_quirk_v2_hw(struct hisi_hba *hisi_hba, int *slot_idx,
+slot_index_alloc_quirk_v2_hw(struct hisi_hba *hisi_hba,
 			     struct domain_device *device)
 {
 	int sata_dev = dev_is_sata(device);
@@ -778,6 +778,7 @@ static u32 hisi_sas_phy_read32(struct hisi_hba *hisi_hba,
 	struct hisi_sas_device *sas_dev = device->lldd_dev;
 	int sata_idx = sas_dev->sata_idx;
 	int start, end;
+	unsigned long flags;
 
 	if (!sata_dev) {
 		/*
@@ -801,6 +802,7 @@ static u32 hisi_sas_phy_read32(struct hisi_hba *hisi_hba,
 		end = 64 * (sata_idx + 2);
 	}
 
+	spin_lock_irqsave(&hisi_hba->lock, flags);
 	while (1) {
 		start = find_next_zero_bit(bitmap,
 					hisi_hba->slot_index_count, start);
@@ -815,8 +817,8 @@ static u32 hisi_sas_phy_read32(struct hisi_hba *hisi_hba,
 	}
 
 	set_bit(start, bitmap);
-	*slot_idx = start;
-	return 0;
+	spin_unlock_irqrestore(&hisi_hba->lock, flags);
+	return start;
 }
 
 static bool sata_index_alloc_v2_hw(struct hisi_hba *hisi_hba, int *idx)
@@ -3560,7 +3562,6 @@ static void wait_cmds_complete_timeout_v2_hw(struct hisi_hba *hisi_hba,
 	.scan_start		= hisi_sas_scan_start,
 	.change_queue_depth	= sas_change_queue_depth,
 	.bios_param		= sas_bios_param,
-	.can_queue		= 1,
 	.this_id		= -1,
 	.sg_tablesize		= SG_ALL,
 	.max_sectors		= SCSI_DEFAULT_MAX_SECTORS,
diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index 34c8f30..f30c4e4 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -2098,7 +2098,6 @@ static void wait_cmds_complete_timeout_v3_hw(struct hisi_hba *hisi_hba,
 	.scan_start		= hisi_sas_scan_start,
 	.change_queue_depth	= sas_change_queue_depth,
 	.bios_param		= sas_bios_param,
-	.can_queue		= 1,
 	.this_id		= -1,
 	.sg_tablesize		= SG_ALL,
 	.max_sectors		= SCSI_DEFAULT_MAX_SECTORS,
@@ -2108,6 +2107,7 @@ static void wait_cmds_complete_timeout_v3_hw(struct hisi_hba *hisi_hba,
 	.target_destroy		= sas_target_destroy,
 	.ioctl			= sas_ioctl,
 	.shost_attrs		= host_attrs,
+	.tag_alloc_policy	= BLK_TAG_ALLOC_RR,
 };
 
 static const struct hisi_sas_hw hisi_sas_v3_hw = {
@@ -2245,8 +2245,10 @@ static void wait_cmds_complete_timeout_v3_hw(struct hisi_hba *hisi_hba,
 	shost->max_channel = 1;
 	shost->max_cmd_len = 16;
 	shost->sg_tablesize = min_t(u16, SG_ALL, HISI_SAS_SGE_PAGE_CNT);
-	shost->can_queue = hisi_hba->hw->max_command_entries;
-	shost->cmd_per_lun = hisi_hba->hw->max_command_entries;
+	shost->can_queue = hisi_hba->hw->max_command_entries -
+		HISI_SAS_RESERVED_IPTT_CNT;
+	shost->cmd_per_lun = hisi_hba->hw->max_command_entries -
+		HISI_SAS_RESERVED_IPTT_CNT;
 
 	sha->sas_ha_name = DRV_NAME;
 	sha->dev = dev;
-- 
1.9.1

[PATCH 7/7] scsi: hisi_sas: Update v3 hw AIP_LIMIT and CFG_AGING_TIME register values

[John Garry]

From: Xiang Chen <chenxiang66@hisilicon.com>

Update registers as follows:
- Default value of AIP timer is 1ms, and it is easy for some expanders to
  cause IO error. Change the value to max value 65ms to avoid IO error for
  those expanders.

- A CQ completion will be reported by HW when 4 CQs have occurred or the
  aging timer expires, whichever happens first. Sor serial IO scenario, it
  will still wait 8us for every IO before it is reported. So in the
  situation, the performance is poor. So to improve it, change the limit
  time to the least value.
  For other scenario, it does little affect to the performance.

Signed-off-by: Xiang Chen <chenxiang66@hisilicon.com>
Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas_v3_hw.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
index f30c4e4..bd4ce38 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v3_hw.c
@@ -127,6 +127,7 @@
 #define PHY_CTRL_RESET_OFF		0
 #define PHY_CTRL_RESET_MSK		(0x1 << PHY_CTRL_RESET_OFF)
 #define SL_CFG				(PORT_BASE + 0x84)
+#define AIP_LIMIT			(PORT_BASE + 0x90)
 #define SL_CONTROL			(PORT_BASE + 0x94)
 #define SL_CONTROL_NOTIFY_EN_OFF	0
 #define SL_CONTROL_NOTIFY_EN_MSK	(0x1 << SL_CONTROL_NOTIFY_EN_OFF)
@@ -431,6 +432,7 @@ static void init_reg_v3_hw(struct hisi_hba *hisi_hba)
 			 (u32)((1ULL << hisi_hba->queue_count) - 1));
 	hisi_sas_write32(hisi_hba, CFG_MAX_TAG, 0xfff0400);
 	hisi_sas_write32(hisi_hba, HGC_SAS_TXFAIL_RETRY_CTRL, 0x108);
+	hisi_sas_write32(hisi_hba, CFG_AGING_TIME, 0x1);
 	hisi_sas_write32(hisi_hba, INT_COAL_EN, 0x1);
 	hisi_sas_write32(hisi_hba, OQ_INT_COAL_TIME, 0x1);
 	hisi_sas_write32(hisi_hba, OQ_INT_COAL_CNT, 0x1);
@@ -495,6 +497,7 @@ static void init_reg_v3_hw(struct hisi_hba *hisi_hba)
 
 		/* used for 12G negotiate */
 		hisi_sas_phy_write32(hisi_hba, i, COARSETUNE_TIME, 0x1e);
+		hisi_sas_phy_write32(hisi_hba, i, AIP_LIMIT, 0x2ffff);
 	}
 
 	for (i = 0; i < hisi_hba->queue_count; i++) {
-- 
1.9.1

Re: [PATCH 0/7] hisi_sas: Misc bugfixes and an optimisation patch

[John Garry]

On 24/09/2018 16:06, John Garry wrote:
> This patchset introduces mostly more minor/obscure bugfixes for the
> driver.

Hi Martin,

Can you please consider merging this patchset?

Thanks,
John

>
> Also included is an optimisation to use the block layer tag for the IPTT
> indexing. This quite a nice optimisation as it means we don't have to
> evaluate this in the driver - it was a bit of a bottle-neck.
>
> However it does block us in future from enabling SCSI MQ in the driver.
> This is because the IPTT index must be a unique value per HBA. However,
> if we switched to SCSI MQ, the block layer tag becomes unique per queue,
> and not per HBA.
>
> Having said this, testing has shown that performance is better by using
> this block layer tag instead of enabling SCSI MQ in the driver.
>
> Luo Jiaxing (2):
>   scsi: hisi_sas: Feed back linkrate(max/min) when re-attached
>   scsi: hisi_sas: Move evaluation of hisi_hba in hisi_sas_task_prep()
>
> Xiang Chen (5):
>   scsi: hisi_sas: Fix the race between IO completion and timeout for
>     SMP/internal IO
>   scsi: hisi_sas: Free slot later in slot_complete_vx_hw()
>   scsi: hisi_sas: unmask interrupts ent72 and ent74
>   scsi: hisi_sas: Use block layer tag instead for IPTT
>   scsi: hisi_sas: Update v3 hw AIP_LIMIT and CFG_AGING_TIME register
>     values
>
>  drivers/scsi/hisi_sas/hisi_sas.h       |   3 +-
>  drivers/scsi/hisi_sas/hisi_sas_main.c  | 154 ++++++++++++++++++++++++---------
>  drivers/scsi/hisi_sas/hisi_sas_v1_hw.c |   1 -
>  drivers/scsi/hisi_sas/hisi_sas_v2_hw.c |  11 +--
>  drivers/scsi/hisi_sas/hisi_sas_v3_hw.c |  15 ++--
>  5 files changed, 130 insertions(+), 54 deletions(-)
>

Re: [PATCH 0/7] hisi_sas: Misc bugfixes and an optimisation patch

[Martin K. Petersen]

John,

> However it does block us in future from enabling SCSI MQ in the driver.

We're going to remove the legacy I/O path so I'm not particularly keen
on merging something that's going in the opposite direction.

> This is because the IPTT index must be a unique value per HBA. However,
> if we switched to SCSI MQ, the block layer tag becomes unique per queue,
> and not per HBA.

That doesn't sound right.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/7] hisi_sas: Misc bugfixes and an optimisation patch

[Christoph Hellwig]

On Wed, Oct 10, 2018 at 09:58:25PM -0400, Martin K. Petersen wrote:
> > This is because the IPTT index must be a unique value per HBA. However,
> > if we switched to SCSI MQ, the block layer tag becomes unique per queue,
> > and not per HBA.
> 
> That doesn't sound right.

blk-mq tags are always per-host (which has actually caused problems for
ATA, which is now using its own per-device tags).

Re: [PATCH 0/7] hisi_sas: Misc bugfixes and an optimisation patch

[John Garry]

On 11/10/2018 07:36, Christoph Hellwig wrote:
> On Wed, Oct 10, 2018 at 09:58:25PM -0400, Martin K. Petersen wrote:
>>> This is because the IPTT index must be a unique value per HBA. However,
>>> if we switched to SCSI MQ, the block layer tag becomes unique per queue,
>>> and not per HBA.
>>
>> That doesn't sound right.
>

Hmmm, well this is what I was told ...

> blk-mq tags are always per-host (which has actually caused problems for
> ATA, which is now using its own per-device tags).
>

So, for example, if Scsi_host.can_queue = 2048 and 
Scsi_host.nr_hw_queues = 16, then rq tags are still in range [0, 2048) 
for that HBA, i.e. invariant on queue count?

> .
>

Thanks,
John

Re: [PATCH 0/7] hisi_sas: Misc bugfixes and an optimisation patch

[Christoph Hellwig]

On Thu, Oct 11, 2018 at 10:59:11AM +0100, John Garry wrote:
> 
> > blk-mq tags are always per-host (which has actually caused problems for
> > ATA, which is now using its own per-device tags).
> > 
> 
> So, for example, if Scsi_host.can_queue = 2048 and Scsi_host.nr_hw_queues =
> 16, then rq tags are still in range [0, 2048) for that HBA, i.e. invariant
> on queue count?

Yes, if can_queue is 2048 you will gets tags from 0..2047.

IFF you device needs different tags for different queues it can use
the blk_mq_unique_tag heper to generate unique global tag.

But unless you actuall have multiple hardware queues that latter part
is rather irrelevant to start with.

Re: [PATCH 0/7] hisi_sas: Misc bugfixes and an optimisation patch

[John Garry]

On 11/10/2018 11:15, Christoph Hellwig wrote:
> On Thu, Oct 11, 2018 at 10:59:11AM +0100, John Garry wrote:
>>
>>> blk-mq tags are always per-host (which has actually caused problems for
>>> ATA, which is now using its own per-device tags).
>>>
>>
>> So, for example, if Scsi_host.can_queue = 2048 and Scsi_host.nr_hw_queues =
>> 16, then rq tags are still in range [0, 2048) for that HBA, i.e. invariant
>> on queue count?
>
> Yes, if can_queue is 2048 you will gets tags from 0..2047.
>

I should be clear about some things before discussing this further. Our 
device has 16 hw queues. And each command we send to any queue in the 
device must have a unique tag across all hw queues for that device, and 
should be in the range [0, 2048) - it's called an IPTT. So 
Scsi_host.can_queue = 2048.

However today we only expose a single queue to upper layer (for 
unrelated LLDD error handling restriction). We hope to expose all 16 
queues in future, which is what I meant by "enabling SCSI MQ in the 
driver". However, with 6/7, this creates a problem, below.

> IFF you device needs different tags for different queues it can use
> the blk_mq_unique_tag heper to generate unique global tag.

So this helper can't help, as fundamentially the issue is "the tag field 
in struct request is unique per hardware queue but not all all hw 
queues". Indeed blk_mq_unique_tag() does give a unique global tag, but 
cannot be used for the IPTT.

OTOH, We could expose 16 queues to upper layer, and drop 6/7, but we 
found it performs worse.

>
> But unless you actuall have multiple hardware queues that latter part
> is rather irrelevant to start with.
>
> .
>

Thanks,
John

Re: [PATCH 0/7] hisi_sas: Misc bugfixes and an optimisation patch

[Ming Lei]

On Thu, Oct 11, 2018 at 02:12:11PM +0100, John Garry wrote:
> On 11/10/2018 11:15, Christoph Hellwig wrote:
> > On Thu, Oct 11, 2018 at 10:59:11AM +0100, John Garry wrote:
> > > 
> > > > blk-mq tags are always per-host (which has actually caused problems for
> > > > ATA, which is now using its own per-device tags).
> > > > 
> > > 
> > > So, for example, if Scsi_host.can_queue = 2048 and Scsi_host.nr_hw_queues =
> > > 16, then rq tags are still in range [0, 2048) for that HBA, i.e. invariant
> > > on queue count?
> > 
> > Yes, if can_queue is 2048 you will gets tags from 0..2047.
> > 
> 
> I should be clear about some things before discussing this further. Our
> device has 16 hw queues. And each command we send to any queue in the device
> must have a unique tag across all hw queues for that device, and should be
> in the range [0, 2048) - it's called an IPTT. So Scsi_host.can_queue = 2048.

Could you describe a bit about IPTT?

Looks like the 16 hw queues are like reply queues in other drivers,
such as megara_sas, but given all the 16 reply queues share one tagset,
so the hw queue number has to be 1 from blk-mq's view.

> 
> However today we only expose a single queue to upper layer (for unrelated
> LLDD error handling restriction). We hope to expose all 16 queues in future,
> which is what I meant by "enabling SCSI MQ in the driver". However, with
> 6/7, this creates a problem, below.

If the tag of each request from all hw queues has to be unique, you
can't expose all 16 queues.

> 
> > IFF you device needs different tags for different queues it can use
> > the blk_mq_unique_tag heper to generate unique global tag.
> 
> So this helper can't help, as fundamentially the issue is "the tag field in
> struct request is unique per hardware queue but not all all hw queues".
> Indeed blk_mq_unique_tag() does give a unique global tag, but cannot be used
> for the IPTT.
> 
> OTOH, We could expose 16 queues to upper layer, and drop 6/7, but we found
> it performs worse.

We discussed this issue before, but not found a good solution yet for
exposing multiple hw queues to blk-mq.

However, we still get good performance in case of none scheduler by the
following patches:

8824f62246be blk-mq: fail the request in case issue failure
6ce3dd6eec11 blk-mq: issue directly if hw queue isn't busy in case of 'none'


Thanks,
Ming

Re: [PATCH 0/7] hisi_sas: Misc bugfixes and an optimisation patch

[John Garry]

On 11/10/2018 14:32, Ming Lei wrote:
> On Thu, Oct 11, 2018 at 02:12:11PM +0100, John Garry wrote:
>> On 11/10/2018 11:15, Christoph Hellwig wrote:
>>> On Thu, Oct 11, 2018 at 10:59:11AM +0100, John Garry wrote:
>>>>
>>>>> blk-mq tags are always per-host (which has actually caused problems for
>>>>> ATA, which is now using its own per-device tags).
>>>>>
>>>>
>>>> So, for example, if Scsi_host.can_queue = 2048 and Scsi_host.nr_hw_queues =
>>>> 16, then rq tags are still in range [0, 2048) for that HBA, i.e. invariant
>>>> on queue count?
>>>
>>> Yes, if can_queue is 2048 you will gets tags from 0..2047.
>>>
>>
>> I should be clear about some things before discussing this further. Our
>> device has 16 hw queues. And each command we send to any queue in the device
>> must have a unique tag across all hw queues for that device, and should be
>> in the range [0, 2048) - it's called an IPTT. So Scsi_host.can_queue = 2048.
>
> Could you describe a bit about IPTT?
>

IPTT is an "Initiator Tag". It is a tag to map to the context of a hw 
queue command. It is related to SAS protocol Initiator Port tag. I think 
that most SAS HBAs have a similar concept.

IPTTs are limited, and must be recycled when an IO completes. Our hw 
supports upto 2048. So we have a limit of 2048 commands issued at any 
point in time.

Previously we had been managing IPTT in LLDD, but found rq tag can be 
used as IPTT (as in 6/7), to gave a good performance boost.

> Looks like the 16 hw queues are like reply queues in other drivers,
> such as megara_sas, but given all the 16 reply queues share one tagset,
> so the hw queue number has to be 1 from blk-mq's view.
>
>>
>> However today we only expose a single queue to upper layer (for unrelated
>> LLDD error handling restriction). We hope to expose all 16 queues in future,
>> which is what I meant by "enabling SCSI MQ in the driver". However, with
>> 6/7, this creates a problem, below.
>
> If the tag of each request from all hw queues has to be unique, you
> can't expose all 16 queues.

Well we can if we generate and manage the IPTT in the LLDD, as we had 
been doing. If we want to use the rq tag - which 6/7 is for - then we can't.

>
>>
>>> IFF you device needs different tags for different queues it can use
>>> the blk_mq_unique_tag heper to generate unique global tag.
>>
>> So this helper can't help, as fundamentially the issue is "the tag field in
>> struct request is unique per hardware queue but not all all hw queues".
>> Indeed blk_mq_unique_tag() does give a unique global tag, but cannot be used
>> for the IPTT.
>>
>> OTOH, We could expose 16 queues to upper layer, and drop 6/7, but we found
>> it performs worse.
>
> We discussed this issue before, but not found a good solution yet for
> exposing multiple hw queues to blk-mq.

I just think that it's unfortunate that enabling blk-mq means that the 
LLDD loses this unique tag across all queues in range [0, 
Scsi_host.can_queue), so much so that we found performance better by not 
exposing multiple queues and continuing to use single rq tag...

>
> However, we still get good performance in case of none scheduler by the
> following patches:
>
> 8824f62246be blk-mq: fail the request in case issue failure
> 6ce3dd6eec11 blk-mq: issue directly if hw queue isn't busy in case of 'none'
>

I think that these patches would have been included in our testing. I 
need to check.

>
> Thanks,
> Ming


Thanks,
John

>
> .
>

Re: [PATCH 0/7] hisi_sas: Misc bugfixes and an optimisation patch

[Ming Lei]

On Thu, Oct 11, 2018 at 03:07:33PM +0100, John Garry wrote:
> On 11/10/2018 14:32, Ming Lei wrote:
> > On Thu, Oct 11, 2018 at 02:12:11PM +0100, John Garry wrote:
> > > On 11/10/2018 11:15, Christoph Hellwig wrote:
> > > > On Thu, Oct 11, 2018 at 10:59:11AM +0100, John Garry wrote:
> > > > > 
> > > > > > blk-mq tags are always per-host (which has actually caused problems for
> > > > > > ATA, which is now using its own per-device tags).
> > > > > > 
> > > > > 
> > > > > So, for example, if Scsi_host.can_queue = 2048 and Scsi_host.nr_hw_queues =
> > > > > 16, then rq tags are still in range [0, 2048) for that HBA, i.e. invariant
> > > > > on queue count?
> > > > 
> > > > Yes, if can_queue is 2048 you will gets tags from 0..2047.
> > > > 
> > > 
> > > I should be clear about some things before discussing this further. Our
> > > device has 16 hw queues. And each command we send to any queue in the device
> > > must have a unique tag across all hw queues for that device, and should be
> > > in the range [0, 2048) - it's called an IPTT. So Scsi_host.can_queue = 2048.
> > 
> > Could you describe a bit about IPTT?
> > 
> 
> IPTT is an "Initiator Tag". It is a tag to map to the context of a hw queue
> command. It is related to SAS protocol Initiator Port tag. I think that most
> SAS HBAs have a similar concept.
> 
> IPTTs are limited, and must be recycled when an IO completes. Our hw
> supports upto 2048. So we have a limit of 2048 commands issued at any point
> in time.
> 
> Previously we had been managing IPTT in LLDD, but found rq tag can be used
> as IPTT (as in 6/7), to gave a good performance boost.
> 
> > Looks like the 16 hw queues are like reply queues in other drivers,
> > such as megara_sas, but given all the 16 reply queues share one tagset,
> > so the hw queue number has to be 1 from blk-mq's view.
> > 
> > > 
> > > However today we only expose a single queue to upper layer (for unrelated
> > > LLDD error handling restriction). We hope to expose all 16 queues in future,
> > > which is what I meant by "enabling SCSI MQ in the driver". However, with
> > > 6/7, this creates a problem, below.
> > 
> > If the tag of each request from all hw queues has to be unique, you
> > can't expose all 16 queues.
> 
> Well we can if we generate and manage the IPTT in the LLDD, as we had been
> doing. If we want to use the rq tag - which 6/7 is for - then we can't.

In theory, you still may generate and manage the IPTT in the LLDD by
simply ignoring rq->tag, meantime enabling SCSI_MQ with 16 hw queues.

However, not sure how much this way may improve performance, and it may
degrade IO perf. If 16 hw queues are exposed to blk-mq, 16*.can_queue
requests may be queued to the driver, and allocation & free on the single
IPTT pool will become a bottleneck.

Per my experiment on host tagset, it might be a good tradeoff to allocate
one hw queue for each node to avoid the remote access on dispatch
data/requests structure for this case, but your IPTT pool is still
shared all CPUs, maybe you can try the smart sbitmap.

https://www.spinics.net/lists/linux-scsi/msg117920.html


> 
> > 
> > > 
> > > > IFF you device needs different tags for different queues it can use
> > > > the blk_mq_unique_tag heper to generate unique global tag.
> > > 
> > > So this helper can't help, as fundamentially the issue is "the tag field in
> > > struct request is unique per hardware queue but not all all hw queues".
> > > Indeed blk_mq_unique_tag() does give a unique global tag, but cannot be used
> > > for the IPTT.
> > > 
> > > OTOH, We could expose 16 queues to upper layer, and drop 6/7, but we found
> > > it performs worse.
> > 
> > We discussed this issue before, but not found a good solution yet for
> > exposing multiple hw queues to blk-mq.
> 
> I just think that it's unfortunate that enabling blk-mq means that the LLDD
> loses this unique tag across all queues in range [0, Scsi_host.can_queue),
> so much so that we found performance better by not exposing multiple queues
> and continuing to use single rq tag...

It isn't a new problem, we discussed it a lot on megaraid_sas which has
same situation with yours, you may find it in block list.

Kashyap Desai did lots of test on this case.

> 
> > 
> > However, we still get good performance in case of none scheduler by the
> > following patches:
> > 
> > 8824f62246be blk-mq: fail the request in case issue failure
> > 6ce3dd6eec11 blk-mq: issue directly if hw queue isn't busy in case of 'none'
> > 
> 
> I think that these patches would have been included in our testing. I need
> to check.

Please switch to none io sched in your test, and it is observed that IO
perf becomes good on megaraid_sas.

Thanks,
Ming

Re: [PATCH 0/7] hisi_sas: Misc bugfixes and an optimisation patch

[John Garry]

Hi Ming,

> In theory, you still may generate and manage the IPTT in the LLDD by
> simply ignoring rq->tag, meantime enabling SCSI_MQ with 16 hw queues.
>

Well at the moment we can't expose all 16 hw queues to upper layer 
anyway, due to ordering restiction imposed by HW on LLDD. We have a plan 
to solve it.

Regardless, we still found performance better by using rq tag instead of 
exposing all 16 queues.

> However, not sure how much this way may improve performance, and it may
> degrade IO perf. If 16 hw queues are exposed to blk-mq, 16*.can_queue
> requests may be queued to the driver, and allocation & free on the single
> IPTT pool will become a bottleneck.

Right, this IPTT management doesn't scale (especially for our host with 
2 sockets @ 48/64 cores each). So if upper layer is already generating a 
tag which we can use, good to use it.

>
> Per my experiment on host tagset, it might be a good tradeoff to allocate
> one hw queue for each node to avoid the remote access on dispatch
> data/requests structure for this case, but your IPTT pool is still
> shared all CPUs, maybe you can try the smart sbitmap.
>
> https://www.spinics.net/lists/linux-scsi/msg117920.html

I don't understand this about allocating a hw queue per node. Surely 
having and using all available queues in an intelligent way means less 
queue contention, right?

Looking at this change:
@@ -5761,6 +5762,11 @@ static int hpsa_scsi_host_alloc(struct ctlr_info *h)
  static int hpsa_scsi_add_host(struct ctlr_info *h)
  {
  	int rv;
+	/* 256 tags should be high enough to saturate device */
+	int max_queues = DIV_ROUND_UP(h->scsi_host->can_queue, 256);
+
+	/* per NUMA node hw queue */
+	h->scsi_host->nr_hw_queues = min_t(int, nr_node_ids, max_queues);

I assume h->scsi_host->nr_hw_queues was zero-init previously, so we're 
now using > 1 queue, but why limit?

>
>
>>
>>>
>>>>
>>>>> IFF you device needs different tags for different queues it can use
>>>>> the blk_mq_unique_tag heper to generate unique global tag.
>>>>
>>>> So this helper can't help, as fundamentially the issue is "the tag field in
>>>> struct request is unique per hardware queue but not all all hw queues".
>>>> Indeed blk_mq_unique_tag() does give a unique global tag, but cannot be used
>>>> for the IPTT.
>>>>
>>>> OTOH, We could expose 16 queues to upper layer, and drop 6/7, but we found
>>>> it performs worse.
>>>
>>> We discussed this issue before, but not found a good solution yet for
>>> exposing multiple hw queues to blk-mq.
>>
>> I just think that it's unfortunate that enabling blk-mq means that the LLDD
>> loses this unique tag across all queues in range [0, Scsi_host.can_queue),
>> so much so that we found performance better by not exposing multiple queues
>> and continuing to use single rq tag...
>
> It isn't a new problem, we discussed it a lot on megaraid_sas which has
> same situation with yours, you may find it in block list.

I'll do some digging.

>
> Kashyap Desai did lots of test on this case.
>
>>
>>>
>>> However, we still get good performance in case of none scheduler by the
>>> following patches:
>>>
>>> 8824f62246be blk-mq: fail the request in case issue failure
>>> 6ce3dd6eec11 blk-mq: issue directly if hw queue isn't busy in case of 'none'
>>>
>>
>> I think that these patches would have been included in our testing. I need
>> to check.
>
> Please switch to none io sched in your test, and it is observed that IO
> perf becomes good on megaraid_sas.

Hmmm... I thought that deadline was preferred.

>
> Thanks,
> Ming
>
> .
>

Much appreciated,
John

Re: [PATCH 0/7] hisi_sas: Misc bugfixes and an optimisation patch

[John Garry]

On 11/10/2018 02:58, Martin K. Petersen wrote:
>
> John,
>

Hi Martin,

>> However it does block us in future from enabling SCSI MQ in the driver.
>
> We're going to remove the legacy I/O path so I'm not particularly keen
> on merging something that's going in the opposite direction.

What I really meant was that we cannot expose multiple queues to upper 
layer, and nothing related to legacy IO path.

>
>> This is because the IPTT index must be a unique value per HBA. However,
>> if we switched to SCSI MQ, the block layer tag becomes unique per queue,
>> and not per HBA.
>
> That doesn't sound right.
>

Again, my wording my probably was not accurate. I meant that rq tag is 
not unqiue across all hw queues.

As I mentioned in the thread that spawned from this, we actually can't 
expose multiple hw queues at the moment. And, if we did, we find a 
performance drop due to having to go back to manage this IPTT internally.

So how to handle? We're going to continue to work towards exposing 
multiple queues to upper layer, but for the moment I think patch 6/7 is 
a good change.

Please let me know.

Thanks,
John

Re: [PATCH 0/7] hisi_sas: Misc bugfixes and an optimisation patch

[Ming Lei]

On Fri, Oct 12, 2018 at 10:02:57AM +0100, John Garry wrote:
> Hi Ming,
> 
> > In theory, you still may generate and manage the IPTT in the LLDD by
> > simply ignoring rq->tag, meantime enabling SCSI_MQ with 16 hw queues.
> > 
> 
> Well at the moment we can't expose all 16 hw queues to upper layer anyway,
> due to ordering restiction imposed by HW on LLDD. We have a plan to solve
> it.
> 
> Regardless, we still found performance better by using rq tag instead of
> exposing all 16 queues.
> 
> > However, not sure how much this way may improve performance, and it may
> > degrade IO perf. If 16 hw queues are exposed to blk-mq, 16*.can_queue
> > requests may be queued to the driver, and allocation & free on the single
> > IPTT pool will become a bottleneck.
> 
> Right, this IPTT management doesn't scale (especially for our host with 2
> sockets @ 48/64 cores each). So if upper layer is already generating a tag
> which we can use, good to use it.
> 
> > 
> > Per my experiment on host tagset, it might be a good tradeoff to allocate
> > one hw queue for each node to avoid the remote access on dispatch
> > data/requests structure for this case, but your IPTT pool is still
> > shared all CPUs, maybe you can try the smart sbitmap.
> > 
> > https://www.spinics.net/lists/linux-scsi/msg117920.html
> 
> I don't understand this about allocating a hw queue per node. Surely having
> and using all available queues in an intelligent way means less queue
> contention, right?

Yes.

> 
> Looking at this change:
> @@ -5761,6 +5762,11 @@ static int hpsa_scsi_host_alloc(struct ctlr_info *h)
>  static int hpsa_scsi_add_host(struct ctlr_info *h)
>  {
>  	int rv;
> +	/* 256 tags should be high enough to saturate device */
> +	int max_queues = DIV_ROUND_UP(h->scsi_host->can_queue, 256);
> +
> +	/* per NUMA node hw queue */
> +	h->scsi_host->nr_hw_queues = min_t(int, nr_node_ids, max_queues);
> 
> I assume h->scsi_host->nr_hw_queues was zero-init previously, so we're now
> using > 1 queue, but why limit?

From my test on null_blk/scsi_debug, per-node hw queue improves iops
much more obviously.

Also you may manage IPTT in LLD, and contention on the single IPTT pool
shouldn't be very serious, given there are less NUMA nodes usually.



Thanks,
Ming

Re: [PATCH 0/7] hisi_sas: Misc bugfixes and an optimisation patch

[Martin K. Petersen]

John,

> As I mentioned in the thread that spawned from this, we actually can't
> expose multiple hw queues at the moment. And, if we did, we find a
> performance drop due to having to go back to manage this IPTT
> internally.
>
> So how to handle? We're going to continue to work towards exposing
> multiple queues to upper layer, but for the moment I think patch 6/7 is
> a good change.

Thanks for the discussion. That's exactly what I was looking for.

Applied to 4.20/scsi-queue.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH 0/7] hisi_sas: Misc bugfixes and an optimisation patch

[John Garry]

On 16/10/2018 05:28, Martin K. Petersen wrote:
>
> John,
>
>> As I mentioned in the thread that spawned from this, we actually can't
>> expose multiple hw queues at the moment. And, if we did, we find a
>> performance drop due to having to go back to manage this IPTT
>> internally.
>>
>> So how to handle? We're going to continue to work towards exposing
>> multiple queues to upper layer, but for the moment I think patch 6/7 is
>> a good change.
>
> Thanks for the discussion. That's exactly what I was looking for.
>
> Applied to 4.20/scsi-queue.
>

Great, thanks.