Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1335305 > unrolled thread

[PATCH 0/6] hisi_sas: add abort and retry feature

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] hisi_sas: add abort and retry feature John Garry <john.garry@huawei.com> - 2016-02-16 13:10 +0100
    [PATCH 1/6] hisi_sas: add TMF_RESP_FUNC_SUCC check John Garry <john.garry@huawei.com> - 2016-02-16 13:10 +0100
      Re: [PATCH 1/6] hisi_sas: add TMF_RESP_FUNC_SUCC check Hannes Reinecke <hare@suse.de> - 2016-02-16 16:30 +0100
    [PATCH 6/6] hisi_sas: update driver version to 1.3 John Garry <john.garry@huawei.com> - 2016-02-16 13:10 +0100
    [PATCH 2/6] hisi_sas: add hisi_sas_slot_abort() John Garry <john.garry@huawei.com> - 2016-02-16 13:10 +0100
      Re: [PATCH 2/6] hisi_sas: add hisi_sas_slot_abort() Hannes Reinecke <hare@suse.de> - 2016-02-16 16:30 +0100
        Re: [PATCH 2/6] hisi_sas: add hisi_sas_slot_abort() John Garry <john.garry@huawei.com> - 2016-02-16 16:50 +0100
          Re: [PATCH 2/6] hisi_sas: add hisi_sas_slot_abort() John Garry <john.garry@huawei.com> - 2016-02-18 10:40 +0100
    [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure() John Garry <john.garry@huawei.com> - 2016-02-16 13:10 +0100
      Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure() Hannes Reinecke <hare@suse.de> - 2016-02-16 16:40 +0100
        Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure() John Garry <john.garry@huawei.com> - 2016-02-16 18:00 +0100
          Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure() Hannes Reinecke <hare@suse.de> - 2016-02-18 08:50 +0100
            Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure() John Garry <john.garry@huawei.com> - 2016-02-18 11:20 +0100
              Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure() Hannes Reinecke <hare@suse.de> - 2016-02-18 11:40 +0100
                Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure() John Garry <john.garry@huawei.com> - 2016-02-18 12:00 +0100
                  Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure() John Garry <john.garry@huawei.com> - 2016-02-19 11:50 +0100
                    Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure() Hannes Reinecke <hare@suse.de> - 2016-02-19 15:40 +0100
                      Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure() John Garry <john.garry@huawei.com> - 2016-02-22 11:10 +0100
    [PATCH 3/6] hisi_sas: use slot abort in v1 hw John Garry <john.garry@huawei.com> - 2016-02-16 13:10 +0100
      Re: [PATCH 3/6] hisi_sas: use slot abort in v1 hw Hannes Reinecke <hare@suse.de> - 2016-02-16 16:40 +0100
        Re: [PATCH 3/6] hisi_sas: use slot abort in v1 hw John Garry <john.garry@huawei.com> - 2016-02-16 17:20 +0100
          Re: [PATCH 3/6] hisi_sas: use slot abort in v1 hw Hannes Reinecke <hare@suse.de> - 2016-02-18 08:20 +0100
            Re: [PATCH 3/6] hisi_sas: use slot abort in v1 hw John Garry <john.garry@huawei.com> - 2016-02-18 11:00 +0100
    [PATCH 4/6] hisi_sas: use slot abort in v2 hw John Garry <john.garry@huawei.com> - 2016-02-16 13:10 +0100
      Re: [PATCH 4/6] hisi_sas: use slot abort in v2 hw Hannes Reinecke <hare@suse.de> - 2016-02-16 16:40 +0100
        Re: [PATCH 4/6] hisi_sas: use slot abort in v2 hw John Garry <john.garry@huawei.com> - 2016-02-16 18:00 +0100

Page 1 of 2  [1] 2  Next page →

#1335305 — [PATCH 0/6] hisi_sas: add abort and retry feature

This patchset introduces support to abort
certain commands which have failed and retry.
Certain errors require that the command be
retried, like TRANS_TX_CREDIT_TIMEOUT_ERR in
v1 hw.
However, when these errors occur the IO may
still be active, so the IO must first be
aborted, and then retried. The HiSilicon
SAS controller has no FW to do this work, so
it needs to be done manually.

John Garry (6):
  hisi_sas: add TMF_RESP_FUNC_SUCC check
  hisi_sas: add hisi_sas_slot_abort()
  hisi_sas: use slot abort in v1 hw
  hisi_sas: use slot abort in v2 hw
  hisi_sas: add hisi_sas_slave_configure()
  hisi_sas: update driver version to 1.3

 drivers/scsi/hisi_sas/hisi_sas.h       |  3 +-
 drivers/scsi/hisi_sas/hisi_sas_main.c  | 66 ++++++++++++++++++++++++++++++++--
 drivers/scsi/hisi_sas/hisi_sas_v1_hw.c | 22 +++++++++---
 drivers/scsi/hisi_sas/hisi_sas_v2_hw.c | 19 ++++++++--
 4 files changed, 101 insertions(+), 9 deletions(-)

-- 
1.9.1

[toc] | [next] | [standalone]

#1335306 — [PATCH 1/6] hisi_sas: add TMF_RESP_FUNC_SUCC check

When a tmf is issued, various response codes can be
returned from the target. For a query tmf the
response may be TMF_RESP_FUNC_COMPLETE or
TMF_RESP_FUNC_SUCC.
Add a condition for TMF_RESP_FUNC_SUCC.
Also, check for SAM_STAT_GOOD is replaced with
TMF_RESP_FUNC_COMPLETE, which is a genuine tmf
response code. SAM_STAT_GOOD and
TMF_RESP_FUNC_COMPLETE have the same value, so
this is why it worked before.

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

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 2194917..c600f5e 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -661,12 +661,18 @@ static int hisi_sas_exec_internal_tmf_task(struct domain_device *device,
 		}
 
 		if (task->task_status.resp == SAS_TASK_COMPLETE &&
-		    task->task_status.stat == SAM_STAT_GOOD) {
+		     task->task_status.stat == TMF_RESP_FUNC_COMPLETE) {
 			res = TMF_RESP_FUNC_COMPLETE;
 			break;
 		}
 
 		if (task->task_status.resp == SAS_TASK_COMPLETE &&
+		    task->task_status.stat == TMF_RESP_FUNC_SUCC) {
+			res = TMF_RESP_FUNC_SUCC;
+			break;
+		}
+
+		if (task->task_status.resp == SAS_TASK_COMPLETE &&
 		      task->task_status.stat == SAS_DATA_UNDERRUN) {
 			/* no error, but return the number of bytes of
 			 * underrun
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1335524 — Re: [PATCH 1/6] hisi_sas: add TMF_RESP_FUNC_SUCC check

On 02/16/2016 01:22 PM, John Garry wrote:
> When a tmf is issued, various response codes can be
> returned from the target. For a query tmf the
> response may be TMF_RESP_FUNC_COMPLETE or
> TMF_RESP_FUNC_SUCC.
> Add a condition for TMF_RESP_FUNC_SUCC.
> Also, check for SAM_STAT_GOOD is replaced with
> TMF_RESP_FUNC_COMPLETE, which is a genuine tmf
> response code. SAM_STAT_GOOD and
> TMF_RESP_FUNC_COMPLETE have the same value, so
> this is why it worked before.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/scsi/hisi_sas/hisi_sas_main.c | 8 +++++++-
>  1 file changed, 7 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index 2194917..c600f5e 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -661,12 +661,18 @@ static int hisi_sas_exec_internal_tmf_task(struct domain_device *device,
>  		}
>  
>  		if (task->task_status.resp == SAS_TASK_COMPLETE &&
> -		    task->task_status.stat == SAM_STAT_GOOD) {
> +		     task->task_status.stat == TMF_RESP_FUNC_COMPLETE) {
>  			res = TMF_RESP_FUNC_COMPLETE;
>  			break;
>  		}
>  
>  		if (task->task_status.resp == SAS_TASK_COMPLETE &&
> +		    task->task_status.stat == TMF_RESP_FUNC_SUCC) {
> +			res = TMF_RESP_FUNC_SUCC;
> +			break;
> +		}
> +
> +		if (task->task_status.resp == SAS_TASK_COMPLETE &&
>  		      task->task_status.stat == SAS_DATA_UNDERRUN) {
>  			/* no error, but return the number of bytes of
>  			 * underrun
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1335307 — [PATCH 6/6] hisi_sas: update driver version to 1.3

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

diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h
index a05ce71..beb7318 100644
--- a/drivers/scsi/hisi_sas/hisi_sas.h
+++ b/drivers/scsi/hisi_sas/hisi_sas.h
@@ -23,7 +23,7 @@
 #include <scsi/sas_ata.h>
 #include <scsi/libsas.h>
 
-#define DRV_VERSION "v1.2"
+#define DRV_VERSION "v1.3"
 
 #define HISI_SAS_MAX_PHYS	9
 #define HISI_SAS_MAX_QUEUES	32
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1335308 — [PATCH 2/6] hisi_sas: add hisi_sas_slot_abort()

Add a function to abort a slot (task) in the device
(if it is in the task set) and then cleanup and
complete the task.
The function is called from work queue context as
it cannot be called from the context where it is
triggered (interrupt).

Signed-off-by: John Garry <john.garry@huawei.com>
---
 drivers/scsi/hisi_sas/hisi_sas.h      |  1 +
 drivers/scsi/hisi_sas/hisi_sas_main.c | 43 +++++++++++++++++++++++++++++++++++
 2 files changed, 44 insertions(+)

diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h
index 02da7e4..a05ce71 100644
--- a/drivers/scsi/hisi_sas/hisi_sas.h
+++ b/drivers/scsi/hisi_sas/hisi_sas.h
@@ -120,6 +120,7 @@ struct hisi_sas_slot {
 	dma_addr_t command_table_dma;
 	struct hisi_sas_sge_page *sge_page;
 	dma_addr_t sge_page_dma;
+	struct work_struct abort_slot;
 };
 
 struct hisi_sas_tmf_task {
diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index c600f5e..65509eb 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -15,6 +15,9 @@
 #define DEV_IS_GONE(dev) \
 	((!dev) || (dev->dev_type == SAS_PHY_UNUSED))
 
+static int hisi_sas_debug_issue_ssp_tmf(struct domain_device *device,
+				u8 *lun, struct hisi_sas_tmf_task *tmf);
+
 static struct hisi_hba *dev_to_hisi_hba(struct domain_device *device)
 {
 	return device->port->ha->lldd_ha;
@@ -113,6 +116,45 @@ static int hisi_sas_task_prep_ata(struct hisi_hba *hisi_hba,
 	return hisi_hba->hw->prep_stp(hisi_hba, slot);
 }
 
+/*
+ * This function will issue an abort TMF if a task is still in
+ * the target. Then it will do the task complete cleanup and
+ * callbacks.
+ */
+static void hisi_sas_slot_abort(struct work_struct *work)
+{
+	struct hisi_sas_slot *abort_slot =
+		container_of(work, struct hisi_sas_slot, abort_slot);
+	struct sas_task *task = abort_slot->task;
+	struct hisi_hba *hisi_hba = dev_to_hisi_hba(task->dev);
+	struct scsi_cmnd *cmnd = task->uldd_task;
+	struct hisi_sas_tmf_task tmf_task;
+	struct domain_device *device = task->dev;
+	struct hisi_sas_device *sas_dev = device->lldd_dev;
+	struct scsi_lun lun;
+	int tag = abort_slot->idx, rc;
+
+	int_to_scsilun(cmnd->device->lun, &lun);
+	tmf_task.tmf = TMF_QUERY_TASK;
+	tmf_task.tag_of_task_to_be_managed = cpu_to_le16(tag);
+
+	rc = hisi_sas_debug_issue_ssp_tmf(task->dev, lun.scsi_lun, &tmf_task);
+
+	/* TMF_RESP_FUNC_SUCC means task is present in the task set */
+	if (rc != TMF_RESP_FUNC_SUCC)
+		goto out;
+	tmf_task.tmf = TMF_ABORT_TASK;
+	tmf_task.tag_of_task_to_be_managed = cpu_to_le16(tag);
+
+	rc = hisi_sas_debug_issue_ssp_tmf(task->dev, lun.scsi_lun, &tmf_task);
+out:
+	hisi_sas_slot_task_free(hisi_hba, task, abort_slot);
+	if (task->task_done)
+		task->task_done(task);
+	if (sas_dev && sas_dev->running_req)
+		sas_dev->running_req--;
+}
+
 static int hisi_sas_task_prep(struct sas_task *task, struct hisi_hba *hisi_hba,
 			      int is_tmf, struct hisi_sas_tmf_task *tmf,
 			      int *pass)
@@ -206,6 +248,7 @@ static int hisi_sas_task_prep(struct sas_task *task, struct hisi_hba *hisi_hba,
 	slot->task = task;
 	slot->port = port;
 	task->lldd_task = slot;
+	INIT_WORK(&slot->abort_slot, hisi_sas_slot_abort);
 
 	slot->status_buffer = dma_pool_alloc(hisi_hba->status_buffer_pool,
 					     GFP_ATOMIC,
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1335522 — Re: [PATCH 2/6] hisi_sas: add hisi_sas_slot_abort()

On 02/16/2016 01:22 PM, John Garry wrote:
> Add a function to abort a slot (task) in the device
> (if it is in the task set) and then cleanup and
> complete the task.
> The function is called from work queue context as
> it cannot be called from the context where it is
> triggered (interrupt).
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/scsi/hisi_sas/hisi_sas.h      |  1 +
>  drivers/scsi/hisi_sas/hisi_sas_main.c | 43 +++++++++++++++++++++++++++++++++++
>  2 files changed, 44 insertions(+)
> 
> diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h
> index 02da7e4..a05ce71 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas.h
> +++ b/drivers/scsi/hisi_sas/hisi_sas.h
> @@ -120,6 +120,7 @@ struct hisi_sas_slot {
>  	dma_addr_t command_table_dma;
>  	struct hisi_sas_sge_page *sge_page;
>  	dma_addr_t sge_page_dma;
> +	struct work_struct abort_slot;
>  };
>  
>  struct hisi_sas_tmf_task {
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
> index c600f5e..65509eb 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
> @@ -15,6 +15,9 @@
>  #define DEV_IS_GONE(dev) \
>  	((!dev) || (dev->dev_type == SAS_PHY_UNUSED))
>  
> +static int hisi_sas_debug_issue_ssp_tmf(struct domain_device *device,
> +				u8 *lun, struct hisi_sas_tmf_task *tmf);
> +
>  static struct hisi_hba *dev_to_hisi_hba(struct domain_device *device)
>  {
>  	return device->port->ha->lldd_ha;
> @@ -113,6 +116,45 @@ static int hisi_sas_task_prep_ata(struct hisi_hba *hisi_hba,
>  	return hisi_hba->hw->prep_stp(hisi_hba, slot);
>  }
>  
> +/*
> + * This function will issue an abort TMF if a task is still in
> + * the target. Then it will do the task complete cleanup and
> + * callbacks.
> + */
> +static void hisi_sas_slot_abort(struct work_struct *work)
> +{
> +	struct hisi_sas_slot *abort_slot =
> +		container_of(work, struct hisi_sas_slot, abort_slot);
> +	struct sas_task *task = abort_slot->task;
> +	struct hisi_hba *hisi_hba = dev_to_hisi_hba(task->dev);
> +	struct scsi_cmnd *cmnd = task->uldd_task;
> +	struct hisi_sas_tmf_task tmf_task;
> +	struct domain_device *device = task->dev;
> +	struct hisi_sas_device *sas_dev = device->lldd_dev;
> +	struct scsi_lun lun;
> +	int tag = abort_slot->idx, rc;
> +
> +	int_to_scsilun(cmnd->device->lun, &lun);
> +	tmf_task.tmf = TMF_QUERY_TASK;
> +	tmf_task.tag_of_task_to_be_managed = cpu_to_le16(tag);
> +
> +	rc = hisi_sas_debug_issue_ssp_tmf(task->dev, lun.scsi_lun, &tmf_task);
> +
> +	/* TMF_RESP_FUNC_SUCC means task is present in the task set */
> +	if (rc != TMF_RESP_FUNC_SUCC)
> +		goto out;
> +	tmf_task.tmf = TMF_ABORT_TASK;
> +	tmf_task.tag_of_task_to_be_managed = cpu_to_le16(tag);
> +
> +	rc = hisi_sas_debug_issue_ssp_tmf(task->dev, lun.scsi_lun, &tmf_task);
> +out:
> +	hisi_sas_slot_task_free(hisi_hba, task, abort_slot);
> +	if (task->task_done)
> +		task->task_done(task);
> +	if (sas_dev && sas_dev->running_req)
> +		sas_dev->running_req--;
> +}
> +
>  static int hisi_sas_task_prep(struct sas_task *task, struct hisi_hba *hisi_hba,
>  			      int is_tmf, struct hisi_sas_tmf_task *tmf,
>  			      int *pass)
Do you really need to query the task first?
As per SAM a successful return from an ABORT TASK TMF has this meaning:

A response of FUNCTION COMPLETE shall indicate that the task was
aborted or was not in the task set.

Ie it doesn't matter if the task was present or not.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1335556 — Re: [PATCH 2/6] hisi_sas: add hisi_sas_slot_abort()

On 16/02/2016 15:22, Hannes Reinecke wrote:
> On 02/16/2016 01:22 PM, John Garry wrote:
>> Add a function to abort a slot (task) in the device
>> (if it is in the task set) and then cleanup and
>> complete the task.
>> The function is called from work queue context as
>> it cannot be called from the context where it is
>> triggered (interrupt).
>>
>> Signed-off-by: John Garry <john.garry@huawei.com>
>> ---
>>   drivers/scsi/hisi_sas/hisi_sas.h      |  1 +
>>   drivers/scsi/hisi_sas/hisi_sas_main.c | 43 +++++++++++++++++++++++++++++++++++
>>   2 files changed, 44 insertions(+)
>>
>> diff --git a/drivers/scsi/hisi_sas/hisi_sas.h b/drivers/scsi/hisi_sas/hisi_sas.h
>> index 02da7e4..a05ce71 100644
>> --- a/drivers/scsi/hisi_sas/hisi_sas.h
>> +++ b/drivers/scsi/hisi_sas/hisi_sas.h
>> @@ -120,6 +120,7 @@ struct hisi_sas_slot {
>>   	dma_addr_t command_table_dma;
>>   	struct hisi_sas_sge_page *sge_page;
>>   	dma_addr_t sge_page_dma;
>> +	struct work_struct abort_slot;
>>   };
>>
>>   struct hisi_sas_tmf_task {
>> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
>> index c600f5e..65509eb 100644
>> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
>> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
>> @@ -15,6 +15,9 @@
>>   #define DEV_IS_GONE(dev) \
>>   	((!dev) || (dev->dev_type == SAS_PHY_UNUSED))
>>
>> +static int hisi_sas_debug_issue_ssp_tmf(struct domain_device *device,
>> +				u8 *lun, struct hisi_sas_tmf_task *tmf);
>> +
>>   static struct hisi_hba *dev_to_hisi_hba(struct domain_device *device)
>>   {
>>   	return device->port->ha->lldd_ha;
>> @@ -113,6 +116,45 @@ static int hisi_sas_task_prep_ata(struct hisi_hba *hisi_hba,
>>   	return hisi_hba->hw->prep_stp(hisi_hba, slot);
>>   }
>>
>> +/*
>> + * This function will issue an abort TMF if a task is still in
>> + * the target. Then it will do the task complete cleanup and
>> + * callbacks.
>> + */
>> +static void hisi_sas_slot_abort(struct work_struct *work)
>> +{
>> +	struct hisi_sas_slot *abort_slot =
>> +		container_of(work, struct hisi_sas_slot, abort_slot);
>> +	struct sas_task *task = abort_slot->task;
>> +	struct hisi_hba *hisi_hba = dev_to_hisi_hba(task->dev);
>> +	struct scsi_cmnd *cmnd = task->uldd_task;
>> +	struct hisi_sas_tmf_task tmf_task;
>> +	struct domain_device *device = task->dev;
>> +	struct hisi_sas_device *sas_dev = device->lldd_dev;
>> +	struct scsi_lun lun;
>> +	int tag = abort_slot->idx, rc;
>> +
>> +	int_to_scsilun(cmnd->device->lun, &lun);
>> +	tmf_task.tmf = TMF_QUERY_TASK;
>> +	tmf_task.tag_of_task_to_be_managed = cpu_to_le16(tag);
>> +
>> +	rc = hisi_sas_debug_issue_ssp_tmf(task->dev, lun.scsi_lun, &tmf_task);
>> +
>> +	/* TMF_RESP_FUNC_SUCC means task is present in the task set */
>> +	if (rc != TMF_RESP_FUNC_SUCC)
>> +		goto out;
>> +	tmf_task.tmf = TMF_ABORT_TASK;
>> +	tmf_task.tag_of_task_to_be_managed = cpu_to_le16(tag);
>> +
>> +	rc = hisi_sas_debug_issue_ssp_tmf(task->dev, lun.scsi_lun, &tmf_task);
>> +out:
>> +	hisi_sas_slot_task_free(hisi_hba, task, abort_slot);
>> +	if (task->task_done)
>> +		task->task_done(task);
>> +	if (sas_dev && sas_dev->running_req)
>> +		sas_dev->running_req--;
>> +}
>> +
>>   static int hisi_sas_task_prep(struct sas_task *task, struct hisi_hba *hisi_hba,
>>   			      int is_tmf, struct hisi_sas_tmf_task *tmf,
>>   			      int *pass)
> Do you really need to query the task first?
> As per SAM a successful return from an ABORT TASK TMF has this meaning:
>
> A response of FUNCTION COMPLETE shall indicate that the task was
> aborted or was not in the task set.
>
> Ie it doesn't matter if the task was present or not.
>
> Cheers,
>
> Hannes
>

I think that it should be ok to the abort without first querying. I'll 
just test this to double-check. (This would make the first patch in the 
series superflous)

Cheers,
John

[toc] | [prev] | [next] | [standalone]

#1337198 — Re: [PATCH 2/6] hisi_sas: add hisi_sas_slot_abort()

On 16/02/2016 15:41, John Garry wrote:
> On 16/02/2016 15:22, Hannes Reinecke wrote:
>> On 02/16/2016 01:22 PM, John Garry wrote:
>>> Add a function to abort a slot (task) in the device
>>> (if it is in the task set) and then cleanup and
>>> complete the task.
>>> The function is called from work queue context as
>>> it cannot be called from the context where it is
>>> triggered (interrupt).
>>>
>>> Signed-off-by: John Garry <john.garry@huawei.com>
>>> ---
>>>   drivers/scsi/hisi_sas/hisi_sas.h      |  1 +
>>>   drivers/scsi/hisi_sas/hisi_sas_main.c | 43
>>> +++++++++++++++++++++++++++++++++++
>>>   2 files changed, 44 insertions(+)
>>>
>>> diff --git a/drivers/scsi/hisi_sas/hisi_sas.h
>>> b/drivers/scsi/hisi_sas/hisi_sas.h
>>> index 02da7e4..a05ce71 100644
>>> --- a/drivers/scsi/hisi_sas/hisi_sas.h
>>> +++ b/drivers/scsi/hisi_sas/hisi_sas.h
>>> @@ -120,6 +120,7 @@ struct hisi_sas_slot {
>>>       dma_addr_t command_table_dma;
>>>       struct hisi_sas_sge_page *sge_page;
>>>       dma_addr_t sge_page_dma;
>>> +    struct work_struct abort_slot;
>>>   };
>>>
>>>   struct hisi_sas_tmf_task {
>>> diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c
>>> b/drivers/scsi/hisi_sas/hisi_sas_main.c
>>> index c600f5e..65509eb 100644
>>> --- a/drivers/scsi/hisi_sas/hisi_sas_main.c
>>> +++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
>>> @@ -15,6 +15,9 @@
>>>   #define DEV_IS_GONE(dev) \
>>>       ((!dev) || (dev->dev_type == SAS_PHY_UNUSED))
>>>
>>> +static int hisi_sas_debug_issue_ssp_tmf(struct domain_device *device,
>>> +                u8 *lun, struct hisi_sas_tmf_task *tmf);
>>> +
>>>   static struct hisi_hba *dev_to_hisi_hba(struct domain_device *device)
>>>   {
>>>       return device->port->ha->lldd_ha;
>>> @@ -113,6 +116,45 @@ static int hisi_sas_task_prep_ata(struct
>>> hisi_hba *hisi_hba,
>>>       return hisi_hba->hw->prep_stp(hisi_hba, slot);
>>>   }
>>>
>>> +/*
>>> + * This function will issue an abort TMF if a task is still in
>>> + * the target. Then it will do the task complete cleanup and
>>> + * callbacks.
>>> + */
>>> +static void hisi_sas_slot_abort(struct work_struct *work)
>>> +{
>>> +    struct hisi_sas_slot *abort_slot =
>>> +        container_of(work, struct hisi_sas_slot, abort_slot);
>>> +    struct sas_task *task = abort_slot->task;
>>> +    struct hisi_hba *hisi_hba = dev_to_hisi_hba(task->dev);
>>> +    struct scsi_cmnd *cmnd = task->uldd_task;
>>> +    struct hisi_sas_tmf_task tmf_task;
>>> +    struct domain_device *device = task->dev;
>>> +    struct hisi_sas_device *sas_dev = device->lldd_dev;
>>> +    struct scsi_lun lun;
>>> +    int tag = abort_slot->idx, rc;
>>> +
>>> +    int_to_scsilun(cmnd->device->lun, &lun);
>>> +    tmf_task.tmf = TMF_QUERY_TASK;
>>> +    tmf_task.tag_of_task_to_be_managed = cpu_to_le16(tag);
>>> +
>>> +    rc = hisi_sas_debug_issue_ssp_tmf(task->dev, lun.scsi_lun,
>>> &tmf_task);
>>> +
>>> +    /* TMF_RESP_FUNC_SUCC means task is present in the task set */
>>> +    if (rc != TMF_RESP_FUNC_SUCC)
>>> +        goto out;
>>> +    tmf_task.tmf = TMF_ABORT_TASK;
>>> +    tmf_task.tag_of_task_to_be_managed = cpu_to_le16(tag);
>>> +
>>> +    rc = hisi_sas_debug_issue_ssp_tmf(task->dev, lun.scsi_lun,
>>> &tmf_task);
>>> +out:
>>> +    hisi_sas_slot_task_free(hisi_hba, task, abort_slot);
>>> +    if (task->task_done)
>>> +        task->task_done(task);
>>> +    if (sas_dev && sas_dev->running_req)
>>> +        sas_dev->running_req--;
>>> +}
>>> +
>>>   static int hisi_sas_task_prep(struct sas_task *task, struct
>>> hisi_hba *hisi_hba,
>>>                     int is_tmf, struct hisi_sas_tmf_task *tmf,
>>>                     int *pass)
>> Do you really need to query the task first?
>> As per SAM a successful return from an ABORT TASK TMF has this meaning:
>>
>> A response of FUNCTION COMPLETE shall indicate that the task was
>> aborted or was not in the task set.
>>
>> Ie it doesn't matter if the task was present or not.
>>
>> Cheers,
>>
>> Hannes
>>
>
> I think that it should be ok to the abort without first querying. I'll
> just test this to double-check. (This would make the first patch in the
> series superflous)
>
> Cheers,
> John
>
>
> _______________________________________________
> linuxarm mailing list
> linuxarm@huawei.com
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm

As suggested, it is ok to remove the query tmf.

Thanks,
John

[toc] | [prev] | [next] | [standalone]

#1335309 — [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure()

In high-datarate aging tests, it is found that
the SCSI framework can periodically
issue lu resets to the device. This is because scsi
commands begin to timeout. It is found that TASK SET
FULL may be returned many times for the same command,
causing the timeouts.
To overcome this, the queue depth for the device needs
to be reduced to 64 (from 256, set in
sas_slave_configure()).

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

diff --git a/drivers/scsi/hisi_sas/hisi_sas_main.c b/drivers/scsi/hisi_sas/hisi_sas_main.c
index 65509eb..dde7c5a 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_main.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_main.c
@@ -454,6 +454,19 @@ static int hisi_sas_dev_found(struct domain_device *device)
 	return 0;
 }
 
+static int hisi_sas_slave_configure(struct scsi_device *sdev)
+{
+	struct domain_device *dev = sdev_to_domain_dev(sdev);
+	int ret = sas_slave_configure(sdev);
+
+	if (ret)
+		return ret;
+	if (!dev_is_sata(dev))
+		sas_change_queue_depth(sdev, 64);
+
+	return 0;
+}
+
 static void hisi_sas_scan_start(struct Scsi_Host *shost)
 {
 	struct hisi_hba *hisi_hba = shost_priv(shost);
@@ -997,7 +1010,7 @@ static struct scsi_host_template hisi_sas_sht = {
 	.name			= DRV_NAME,
 	.queuecommand		= sas_queuecommand,
 	.target_alloc		= sas_target_alloc,
-	.slave_configure	= sas_slave_configure,
+	.slave_configure	= hisi_sas_slave_configure,
 	.scan_finished		= hisi_sas_scan_finished,
 	.scan_start		= hisi_sas_scan_start,
 	.change_queue_depth	= sas_change_queue_depth,
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1335546 — Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure()

On 02/16/2016 01:22 PM, John Garry wrote:
> In high-datarate aging tests, it is found that
> the SCSI framework can periodically
> issue lu resets to the device. This is because scsi
> commands begin to timeout. It is found that TASK SET
> FULL may be returned many times for the same command,
> causing the timeouts.
> To overcome this, the queue depth for the device needs
> to be reduced to 64 (from 256, set in
> sas_slave_configure()).
> 
Hmm. TASK SET FULL should cause the queue depth to be reduced
automatically, no?

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1335639 — Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure()

On 16/02/2016 15:33, Hannes Reinecke wrote:
> On 02/16/2016 01:22 PM, John Garry wrote:
>> In high-datarate aging tests, it is found that
>> the SCSI framework can periodically
>> issue lu resets to the device. This is because scsi
>> commands begin to timeout. It is found that TASK SET
>> FULL may be returned many times for the same command,
>> causing the timeouts.
>> To overcome this, the queue depth for the device needs
>> to be reduced to 64 (from 256, set in
>> sas_slave_configure()).
>>
> Hmm. TASK SET FULL should cause the queue depth to be reduced
> automatically, no?
>
> Cheers,
>
> Hannes
>

I need to double-check if Task set full reduces the depth, I don't think 
it does.

Regardless I found we were getting a combination of commands being 
retried due to Task Set Full and also SAS_QUEUE_FULL errors. For sure 
the SAS_QUEUE_FULL task errors reduce the queue depth in 
scsi_track_queue_full(). However I found it to be very slow in tracking, 
and we were getting commands timing out before the queue depth fell enough.

It would be nice to change default queue depth in sas_slave_configure() 
to a lower value so we can avoid this patch, but I am not sure if other 
vendor's HBA performance would be affected. From looking at the history 
of sas_slave_configure(), it would seem the value of 256 was inherited 
from mpt2sas driver, so I'm not sure.

Cheers,
John

[toc] | [prev] | [next] | [standalone]

#1337102 — Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure()

On 02/16/2016 05:56 PM, John Garry wrote:
> On 16/02/2016 15:33, Hannes Reinecke wrote:
>> On 02/16/2016 01:22 PM, John Garry wrote:
>>> In high-datarate aging tests, it is found that
>>> the SCSI framework can periodically
>>> issue lu resets to the device. This is because scsi
>>> commands begin to timeout. It is found that TASK SET
>>> FULL may be returned many times for the same command,
>>> causing the timeouts.
>>> To overcome this, the queue depth for the device needs
>>> to be reduced to 64 (from 256, set in
>>> sas_slave_configure()).
>>>
>> Hmm. TASK SET FULL should cause the queue depth to be reduced
>> automatically, no?
>>
>> Cheers,
>>
>> Hannes
>>
> 
> I need to double-check if Task set full reduces the depth, I don't
> think it does.
> 
> Regardless I found we were getting a combination of commands being
> retried due to Task Set Full and also SAS_QUEUE_FULL errors. For
> sure the SAS_QUEUE_FULL task errors reduce the queue depth in
> scsi_track_queue_full(). However I found it to be very slow in
> tracking, and we were getting commands timing out before the queue
> depth fell enough.
> 
> It would be nice to change default queue depth in
> sas_slave_configure() to a lower value so we can avoid this patch,
> but I am not sure if other vendor's HBA performance would be
> affected. From looking at the history of sas_slave_configure(), it
> would seem the value of 256 was inherited from mpt2sas driver, so
> I'm not sure.
> 
Well, the classical thing would be to associate each request tag
with a SAS task; or, in your case, associate each slot index with a
request tag.
You probably would need to reserve some slots for TMFs, ie you'd
need to decrease the resulting ->can_queue variable by that.
But once you've done that you shouldn't hit any QUEUE_FULL issues,
as the block layer will ensure that no tags will be reused while the
command is in flight.
Plus this is something you really need to be doing if you ever
consider moving to scsi-mq ...

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1337226 — Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure()

On 18/02/2016 07:40, Hannes Reinecke wrote:
> On 02/16/2016 05:56 PM, John Garry wrote:
>> On 16/02/2016 15:33, Hannes Reinecke wrote:
>>> On 02/16/2016 01:22 PM, John Garry wrote:
>>>> In high-datarate aging tests, it is found that
>>>> the SCSI framework can periodically
>>>> issue lu resets to the device. This is because scsi
>>>> commands begin to timeout. It is found that TASK SET
>>>> FULL may be returned many times for the same command,
>>>> causing the timeouts.
>>>> To overcome this, the queue depth for the device needs
>>>> to be reduced to 64 (from 256, set in
>>>> sas_slave_configure()).
>>>>
>>> Hmm. TASK SET FULL should cause the queue depth to be reduced
>>> automatically, no?
>>>
>>> Cheers,
>>>
>>> Hannes
>>>
>>
>> I need to double-check if Task set full reduces the depth, I don't
>> think it does.
>>
>> Regardless I found we were getting a combination of commands being
>> retried due to Task Set Full and also SAS_QUEUE_FULL errors. For
>> sure the SAS_QUEUE_FULL task errors reduce the queue depth in
>> scsi_track_queue_full(). However I found it to be very slow in
>> tracking, and we were getting commands timing out before the queue
>> depth fell enough.
>>
>> It would be nice to change default queue depth in
>> sas_slave_configure() to a lower value so we can avoid this patch,
>> but I am not sure if other vendor's HBA performance would be
>> affected. From looking at the history of sas_slave_configure(), it
>> would seem the value of 256 was inherited from mpt2sas driver, so
>> I'm not sure.
>>
> Well, the classical thing would be to associate each request tag
> with a SAS task; or, in your case, associate each slot index with a
> request tag.
> You probably would need to reserve some slots for TMFs, ie you'd
> need to decrease the resulting ->can_queue variable by that.
> But once you've done that you shouldn't hit any QUEUE_FULL issues,
> as the block layer will ensure that no tags will be reused while the
> command is in flight.
> Plus this is something you really need to be doing if you ever
> consider moving to scsi-mq ...
>
> Cheers,
>
> Hannes
>
Hi,

So would you recommend this method under the assumption that the 
can_queue value for the host is similar to the queue depth for the device?

Regards,
John

[toc] | [prev] | [next] | [standalone]

#1337252 — Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure()

On 02/18/2016 11:12 AM, John Garry wrote:
> On 18/02/2016 07:40, Hannes Reinecke wrote:
[ .. ]
>> Well, the classical thing would be to associate each request tag
>> with a SAS task; or, in your case, associate each slot index with a
>> request tag.
>> You probably would need to reserve some slots for TMFs, ie you'd
>> need to decrease the resulting ->can_queue variable by that.
>> But once you've done that you shouldn't hit any QUEUE_FULL issues,
>> as the block layer will ensure that no tags will be reused while the
>> command is in flight.
>> Plus this is something you really need to be doing if you ever
>> consider moving to scsi-mq ...
>>
>> Cheers,
>>
>> Hannes
>>
> Hi,
> 
> So would you recommend this method under the assumption that the
> can_queue value for the host is similar to the queue depth for the
> device?
> 
That depends.
Typically the can_queue setting reflects the number of commands the
_host_ can queue internally (due to hardware limitations etc).
They do not necessarily reflect the queue depth for the device
(unless you have a single device, of course).
So if the host has a hardware limit on the number of commands it can
queue, it should set the 'can_queue' variable to the appropriate
number; a host-wide shared tag map is always assumed with recent
kernels.

The queue_depth of an individual device is controlled by the
'cmd_per_lun' setting, and of course capped by can_queue.

But yes, I definitely recommend this method.
Is saves one _so much_ time trying to figure out which command slot
to use. Drawback is that you have to have some sort of fixed order
on them slots to do an efficient lookup.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1337266 — Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure()

On 18/02/2016 10:30, Hannes Reinecke wrote:
> On 02/18/2016 11:12 AM, John Garry wrote:
>> On 18/02/2016 07:40, Hannes Reinecke wrote:
> [ .. ]
>>> Well, the classical thing would be to associate each request tag
>>> with a SAS task; or, in your case, associate each slot index with a
>>> request tag.
>>> You probably would need to reserve some slots for TMFs, ie you'd
>>> need to decrease the resulting ->can_queue variable by that.
>>> But once you've done that you shouldn't hit any QUEUE_FULL issues,
>>> as the block layer will ensure that no tags will be reused while the
>>> command is in flight.
>>> Plus this is something you really need to be doing if you ever
>>> consider moving to scsi-mq ...
>>>
>>> Cheers,
>>>
>>> Hannes
>>>
>> Hi,
>>
>> So would you recommend this method under the assumption that the
>> can_queue value for the host is similar to the queue depth for the
>> device?
>>
> That depends.
> Typically the can_queue setting reflects the number of commands the
> _host_ can queue internally (due to hardware limitations etc).
> They do not necessarily reflect the queue depth for the device
> (unless you have a single device, of course).
> So if the host has a hardware limit on the number of commands it can
> queue, it should set the 'can_queue' variable to the appropriate
> number; a host-wide shared tag map is always assumed with recent
> kernels.
>
> The queue_depth of an individual device is controlled by the
> 'cmd_per_lun' setting, and of course capped by can_queue.
>
> But yes, I definitely recommend this method.
> Is saves one _so much_ time trying to figure out which command slot
> to use. Drawback is that you have to have some sort of fixed order
> on them slots to do an efficient lookup.
>
> Cheers,
>
> Hannes
>

I would like to make a point on cmd_per_lun before considering tagging 
slots: For our host the can_queue is considerably greater than 
cmd_per_lun (even though we initially set the same in the host template, 
which would be incorrect). Regardless I find the host cmd_per_lun is 
effectively ignored for the slave device queue depth as it is reset in 
sas_slave_configure() to 256 [if this function is used and tagging 
enabled]. So if we we choose a reasonable cmd_per_lun for our host, it 
is ignored, right? Or am I missing something?

Thanks,
John

[toc] | [prev] | [next] | [standalone]

#1338044 — Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure()

On 18/02/2016 10:57, John Garry wrote:
> On 18/02/2016 10:30, Hannes Reinecke wrote:
>> On 02/18/2016 11:12 AM, John Garry wrote:
>>> On 18/02/2016 07:40, Hannes Reinecke wrote:
>> [ .. ]
>>>> Well, the classical thing would be to associate each request tag
>>>> with a SAS task; or, in your case, associate each slot index with a
>>>> request tag.
>>>> You probably would need to reserve some slots for TMFs, ie you'd
>>>> need to decrease the resulting ->can_queue variable by that.
>>>> But once you've done that you shouldn't hit any QUEUE_FULL issues,
>>>> as the block layer will ensure that no tags will be reused while the
>>>> command is in flight.
>>>> Plus this is something you really need to be doing if you ever
>>>> consider moving to scsi-mq ...
>>>>
>>>> Cheers,
>>>>
>>>> Hannes
>>>>
>>> Hi,
>>>
>>> So would you recommend this method under the assumption that the
>>> can_queue value for the host is similar to the queue depth for the
>>> device?
>>>
>> That depends.
>> Typically the can_queue setting reflects the number of commands the
>> _host_ can queue internally (due to hardware limitations etc).
>> They do not necessarily reflect the queue depth for the device
>> (unless you have a single device, of course).
>> So if the host has a hardware limit on the number of commands it can
>> queue, it should set the 'can_queue' variable to the appropriate
>> number; a host-wide shared tag map is always assumed with recent
>> kernels.
>>
>> The queue_depth of an individual device is controlled by the
>> 'cmd_per_lun' setting, and of course capped by can_queue.
>>
>> But yes, I definitely recommend this method.
>> Is saves one _so much_ time trying to figure out which command slot
>> to use. Drawback is that you have to have some sort of fixed order
>> on them slots to do an efficient lookup.
>>
>> Cheers,
>>
>> Hannes
>>
>
> I would like to make a point on cmd_per_lun before considering tagging
> slots: For our host the can_queue is considerably greater than
> cmd_per_lun (even though we initially set the same in the host template,
> which would be incorrect). Regardless I find the host cmd_per_lun is
> effectively ignored for the slave device queue depth as it is reset in
> sas_slave_configure() to 256 [if this function is used and tagging
> enabled]. So if we we choose a reasonable cmd_per_lun for our host, it
> is ignored, right? Or am I missing something?
>
> Thanks,
> John
>
>

I would like to make another point about why I am making this change in 
case it is not clear. The queue full events are form 
TRANS_TX_CREDIT_TIMEOUT_ERR and TRANS_TX_CLOSE_NORMAL_ERR errors in the 
slot: I want the slot retried when this occurs, so I set status as 
SAS_QUEUE_FULL just so we will report DID_SOFT_ERR to SCSI midlayer so 
we get a retry. I could use SAS_OPEN_REJECT alternatively as the error 
which would have the same affect.
The queue full are not from all slots being consumed in the HBA.

thanks again,
John


> _______________________________________________
> linuxarm mailing list
> linuxarm@huawei.com
> http://rnd-openeuler.huawei.com/mailman/listinfo/linuxarm

[toc] | [prev] | [next] | [standalone]

#1338234 — Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure()

On 02/19/2016 11:46 AM, John Garry wrote:
> On 18/02/2016 10:57, John Garry wrote:
>> On 18/02/2016 10:30, Hannes Reinecke wrote:
>>> On 02/18/2016 11:12 AM, John Garry wrote:
>>>> On 18/02/2016 07:40, Hannes Reinecke wrote:
>>> [ .. ]
>>>>> Well, the classical thing would be to associate each request tag
>>>>> with a SAS task; or, in your case, associate each slot index
>>>>> with a
>>>>> request tag.
>>>>> You probably would need to reserve some slots for TMFs, ie you'd
>>>>> need to decrease the resulting ->can_queue variable by that.
>>>>> But once you've done that you shouldn't hit any QUEUE_FULL issues,
>>>>> as the block layer will ensure that no tags will be reused
>>>>> while the
>>>>> command is in flight.
>>>>> Plus this is something you really need to be doing if you ever
>>>>> consider moving to scsi-mq ...
>>>>>
>>>>> Cheers,
>>>>>
>>>>> Hannes
>>>>>
>>>> Hi,
>>>>
>>>> So would you recommend this method under the assumption that the
>>>> can_queue value for the host is similar to the queue depth for the
>>>> device?
>>>>
>>> That depends.
>>> Typically the can_queue setting reflects the number of commands the
>>> _host_ can queue internally (due to hardware limitations etc).
>>> They do not necessarily reflect the queue depth for the device
>>> (unless you have a single device, of course).
>>> So if the host has a hardware limit on the number of commands it can
>>> queue, it should set the 'can_queue' variable to the appropriate
>>> number; a host-wide shared tag map is always assumed with recent
>>> kernels.
>>>
>>> The queue_depth of an individual device is controlled by the
>>> 'cmd_per_lun' setting, and of course capped by can_queue.
>>>
>>> But yes, I definitely recommend this method.
>>> Is saves one _so much_ time trying to figure out which command slot
>>> to use. Drawback is that you have to have some sort of fixed order
>>> on them slots to do an efficient lookup.
>>>
>>> Cheers,
>>>
>>> Hannes
>>>
>>
>> I would like to make a point on cmd_per_lun before considering
>> tagging slots: For our host the can_queue is considerably greater than
>> cmd_per_lun (even though we initially set the same in the host
>> template, which would be incorrect).
That is common behaviour; most hosts support more than one LUN, and
setting cmd_per_lun to a lower value will attempt to distribute the
available commands across all LUNs.

>> Regardless I find the host cmd_per_lun is effectively ignored for
>> the slave device queue depth as it is reset in
>> sas_slave_configure() to 256 [if this function is used and tagging
>> enabled]. So if we we choose a reasonable cmd_per_lun for our
>> host, it is ignored, right? Or am I missing something?
>>
Basically, yes.
As said above, the cmd_per_lun is a _very_ rough attempt to
distribute commands across several LUNs.
However, in general there's no need to rely on that, as each queue
(which is associated with the LUN) will be controlled individually
via the queue_depth ramp-down/ramp-up mechanism.

> I would like to make another point about why I am making this change
> in case it is not clear. The queue full events are form
> TRANS_TX_CREDIT_TIMEOUT_ERR and TRANS_TX_CLOSE_NORMAL_ERR errors in
> the slot: I want the slot retried when this occurs, so I set status
> as SAS_QUEUE_FULL just so we will report DID_SOFT_ERR to SCSI
> midlayer so we get a retry. I could use SAS_OPEN_REJECT
> alternatively as the error which would have the same affect.
> The queue full are not from all slots being consumed in the HBA.
> 
Ah, right. So you might be getting those errors even with some free
slots on the HBA. As such they are roughly equivalent to a
QUEUE_FULL SCSI statue, right?
So after reading SPL I guess you are right here; using tags wouldn't
help for this situation.

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

#1339311 — Re: [PATCH 5/6] hisi_sas: add hisi_sas_slave_configure()

>> I would like to make another point about why I am making this change
>> in case it is not clear. The queue full events are form
>> TRANS_TX_CREDIT_TIMEOUT_ERR and TRANS_TX_CLOSE_NORMAL_ERR errors in
>> the slot: I want the slot retried when this occurs, so I set status
>> as SAS_QUEUE_FULL just so we will report DID_SOFT_ERR to SCSI
>> midlayer so we get a retry. I could use SAS_OPEN_REJECT
>> alternatively as the error which would have the same affect.
>> The queue full are not from all slots being consumed in the HBA.
>>
> Ah, right. So you might be getting those errors even with some free
> slots on the HBA. As such they are roughly equivalent to a
> QUEUE_FULL SCSI statue, right?
> So after reading SPL I guess you are right here; using tags wouldn't
> help for this situation.
>

Yes, I think we have 90% of slots free in the host when this occurs for 
one particular test - Our v2 hw has 2K slots, which is >> cmd_per_lun. 
The errors are equivalent to queue full for the device.

Thanks,
John

> Cheers,
>
> Hannes
>

[toc] | [prev] | [next] | [standalone]

#1335310 — [PATCH 3/6] hisi_sas: use slot abort in v1 hw

When TRANS_TX_CREDIT_TIMEOUT_ERR or
TRANS_TX_CLOSE_NORMAL_ERR errors occur for a
command, the command should be re-attempted.

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

diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
index ce5f65d..34f71a1c 100644
--- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
+++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
@@ -1118,9 +1118,8 @@ static int prep_ssp_v1_hw(struct hisi_hba *hisi_hba,
 }
 
 /* by default, task resp is complete */
-static void slot_err_v1_hw(struct hisi_hba *hisi_hba,
-			   struct sas_task *task,
-			   struct hisi_sas_slot *slot)
+static void slot_err_v1_hw(struct hisi_hba *hisi_hba, struct sas_task *task,
+			   struct hisi_sas_slot *slot, int *abort_slot)
 {
 	struct task_status_struct *ts = &task->task_status;
 	struct hisi_sas_err_record_v1 *err_record = slot->status_buffer;
@@ -1212,6 +1211,14 @@ static void slot_err_v1_hw(struct hisi_hba *hisi_hba,
 			ts->stat = SAS_NAK_R_ERR;
 			break;
 		}
+		case TRANS_TX_CREDIT_TIMEOUT_ERR:
+		case TRANS_TX_CLOSE_NORMAL_ERR:
+		{
+			/* This will request a retry */
+			ts->stat = SAS_QUEUE_FULL;
+			++(*abort_slot);
+			break;
+		}
 		default:
 		{
 			ts->stat = SAM_STAT_CHECK_CONDITION;
@@ -1317,8 +1324,14 @@ static int slot_complete_v1_hw(struct hisi_hba *hisi_hba,
 
 	if (cmplt_hdr_data & CMPLT_HDR_ERR_RCRD_XFRD_MSK &&
 		!(cmplt_hdr_data & CMPLT_HDR_RSPNS_XFRD_MSK)) {
+		int abort_slot = 0;
 
-		slot_err_v1_hw(hisi_hba, task, slot);
+		slot_err_v1_hw(hisi_hba, task, slot,  &abort_slot);
+		if (unlikely(abort_slot)) {
+			queue_work(hisi_hba->wq, &slot->abort_slot);
+			sts = ts->stat;
+			goto out_1;
+		}
 		goto out;
 	}
 
@@ -1375,6 +1388,7 @@ out:
 
 	if (task->task_done)
 		task->task_done(task);
+out_1:
 
 	return sts;
 }
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1335543 — Re: [PATCH 3/6] hisi_sas: use slot abort in v1 hw

On 02/16/2016 01:22 PM, John Garry wrote:
> When TRANS_TX_CREDIT_TIMEOUT_ERR or
> TRANS_TX_CLOSE_NORMAL_ERR errors occur for a
> command, the command should be re-attempted.
> 
> Signed-off-by: John Garry <john.garry@huawei.com>
> ---
>  drivers/scsi/hisi_sas/hisi_sas_v1_hw.c | 22 ++++++++++++++++++----
>  1 file changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> index ce5f65d..34f71a1c 100644
> --- a/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> +++ b/drivers/scsi/hisi_sas/hisi_sas_v1_hw.c
> @@ -1118,9 +1118,8 @@ static int prep_ssp_v1_hw(struct hisi_hba *hisi_hba,
>  }
>  
>  /* by default, task resp is complete */
> -static void slot_err_v1_hw(struct hisi_hba *hisi_hba,
> -			   struct sas_task *task,
> -			   struct hisi_sas_slot *slot)
> +static void slot_err_v1_hw(struct hisi_hba *hisi_hba, struct sas_task *task,
> +			   struct hisi_sas_slot *slot, int *abort_slot)
>  {
>  	struct task_status_struct *ts = &task->task_status;
>  	struct hisi_sas_err_record_v1 *err_record = slot->status_buffer;
> @@ -1212,6 +1211,14 @@ static void slot_err_v1_hw(struct hisi_hba *hisi_hba,
>  			ts->stat = SAS_NAK_R_ERR;
>  			break;
>  		}
> +		case TRANS_TX_CREDIT_TIMEOUT_ERR:
> +		case TRANS_TX_CLOSE_NORMAL_ERR:
> +		{
> +			/* This will request a retry */
> +			ts->stat = SAS_QUEUE_FULL;
> +			++(*abort_slot);
> +			break;
> +		}
>  		default:
>  		{
>  			ts->stat = SAM_STAT_CHECK_CONDITION;
> @@ -1317,8 +1324,14 @@ static int slot_complete_v1_hw(struct hisi_hba *hisi_hba,
>  
>  	if (cmplt_hdr_data & CMPLT_HDR_ERR_RCRD_XFRD_MSK &&
>  		!(cmplt_hdr_data & CMPLT_HDR_RSPNS_XFRD_MSK)) {
> +		int abort_slot = 0;
>  
> -		slot_err_v1_hw(hisi_hba, task, slot);
> +		slot_err_v1_hw(hisi_hba, task, slot,  &abort_slot);
> +		if (unlikely(abort_slot)) {
> +			queue_work(hisi_hba->wq, &slot->abort_slot);
> +			sts = ts->stat;
> +			goto out_1;
> +		}
>  		goto out;
>  	}
>  
What is the 'abort_slot' variable for?
Currently it's just a counter, no?
So why the weird pointer passing?

And it does feel weird. Apparently the driver does get a message,
but still has to abort the command. Why?
Isn't the message an indicator that the command has been aborted?

Cheers,

Hannes

-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: F. Imendörffer, J. Smithard, J. Guild, D. Upmanyu, G. Norton
HRB 21284 (AG Nürnberg)

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web