SRP: fix task management handle in srp

From 5f5af6de8dd72e37448841b7d7735d3eea4d3d83 Mon Sep 17 00:00:00 2001

From 5f5af6de8dd72e37448841b7d7735d3eea4d3d83 Mon Sep 17 00:00:00 2001

Hi all,

Currently handle of srp_rsp for task management is broken.

in 6.9
T10/1415-D revision 16a
SRP_RSP responses that contain either
RESPONSE DATA or SENSE DATA shall be sent as the minimum length message containing those fields.
LENGTH field specify the number of bytes in the RESPONSE DATA field. RSPVALID set to one also indicates that the
contents of the STATUS field shall be ignored by the SRP initiator port.

If response data is provided, RSPVALID shall be set to one and the RESPONSE DATA LIST LENGTH field shall specify
the number of bytes in the RESPONSE DATA field (see table 23). The RESPONSE DATA LIST LENGTH field shall
contain the value four. Other lengths are reserved for future standardization.
If no response data is provided, RSPVALID shall be set to zero. The SRP initiator port shall ignore the RESPONSE
DATA LIST LENGTH field and shall assume a length of zero. Response data shall be provided in any SRP_RSP response that is sent in response to an SRP_TSK_MGMT
request (see 6.7). The information in the RSP_CODE field (see table 24) shall indicate the completion status of
the task management function.
Response data shall not be provided in any SRP_RSP response that returns a non-zero status code in the
STATUS field.
The STATUS field contains the status of a task that completes.

Patch made against v3.12-rc1

Quoted 1 line

From 5f5af6de8dd72e37448841b7d7735d3eea4d3d83 Mon Sep 17 00:00:00 2001

From: Jack Wang <[email protected]>
Date: Fri, 27 Sep 2013 11:10:05 +0200
Subject: [PATCH] IB/srp: fix task management handle in srp

Currently the srp driver process task manamgement command in wrong way it just ignore the return srp_rsp for successful case eg rsp->status is success, fix this.

Signed-off-by Jack Wang <[email protected]>

Reviewed-by Dongsu Park <[email protected]>

---
  drivers/infiniband/ulp/srp/ib_srp.c | 12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +5 -3

index f93baf8..5e1f1bf 100644
@@ -1145,9 +1145,11 @@ static void srp_process_rsp(struct
 srp_target_port *target, struct srp_rsp *rsp)
  		target->req_lim += be32_to_cpu(rsp->req_lim_delta);
  		spin_unlock_irqrestore(&target->lock, flags);
 
-		target->tsk_mgmt_status = -1;
-		if (be32_to_cpu(rsp->resp_data_len) >= 4)
-			target->tsk_mgmt_status = rsp->data[3];
+		target->tsk_mgmt_status = rsp->status;
+		if (rsp->flags & SRP_RSP_FLAG_RSPVALID) {
+			if (be32_to_cpu(rsp->resp_data_len) >= 4)
+				target->tsk_mgmt_status = rsp->data[3];
+		}
  		complete(&target->tsk_mgmt_done);
  	} else {

  		req = &target->req_ring[rsp->tag];

drivers/infiniband/ulp/srp/ib_srp.c +1 -1

@@ -1739,7 +1741,7 @@ static int srp_send_tsk_mgmt(struct
 srp_target_port *target,
  					 msecs_to_jiffies(SRP_ABORT_TIMEOUT_MS)))
  		return -1;
 
-	return 0;
+	return target->tsk_mgmt_status;
  }
 

static int srp_abort(struct scsi_cmnd *scmnd)

Quoted 89 lines · Jack Wang

Hi all,

Currently handle of srp_rsp for task management is broken.

in 6.9
T10/1415-D revision 16a
SRP_RSP responses that contain either
RESPONSE DATA or SENSE DATA shall be sent as the minimum length message containing those fields.
LENGTH field specify the number of bytes in the RESPONSE DATA field. RSPVALID set to one also indicates that the
contents of the STATUS field shall be ignored by the SRP initiator port.

If response data is provided, RSPVALID shall be set to one and the RESPONSE DATA LIST LENGTH field shall specify
the number of bytes in the RESPONSE DATA field (see table 23). The RESPONSE DATA LIST LENGTH field shall
contain the value four. Other lengths are reserved for future standardization.
If no response data is provided, RSPVALID shall be set to zero. The SRP initiator port shall ignore the RESPONSE
DATA LIST LENGTH field and shall assume a length of zero. Response data shall be provided in any SRP_RSP response that is sent in response to an SRP_TSK_MGMT
request (see 6.7). The information in the RSP_CODE field (see table 24) shall indicate the completion status of
the task management function.
Response data shall not be provided in any SRP_RSP response that returns a non-zero status code in the
STATUS field.
The STATUS field contains the status of a task that completes.

Patch made against v3.12-rc1

Quoted 1 line

From 5f5af6de8dd72e37448841b7d7735d3eea4d3d83 Mon Sep 17 00:00:00 2001

From: Jack Wang <[email protected]>
Date: Fri, 27 Sep 2013 11:10:05 +0200
Subject: [PATCH] IB/srp: fix task management handle in srp

Currently the srp driver process task manamgement command in wrong way it just ignore the return srp_rsp for successful case eg rsp->status is success, fix this.

Signed-off-by Jack Wang <[email protected]>

Reviewed-by Dongsu Park <[email protected]>

---
  drivers/infiniband/ulp/srp/ib_srp.c | 12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +5 -3

index f93baf8..5e1f1bf 100644
@@ -1145,9 +1145,11 @@ static void srp_process_rsp(struct
 srp_target_port *target, struct srp_rsp *rsp)
          target->req_lim += be32_to_cpu(rsp->req_lim_delta);
          spin_unlock_irqrestore(&target->lock, flags);
 
-        target->tsk_mgmt_status = -1;
-        if (be32_to_cpu(rsp->resp_data_len) >= 4)
-            target->tsk_mgmt_status = rsp->data[3];
+        target->tsk_mgmt_status = rsp->status;
+        if (rsp->flags & SRP_RSP_FLAG_RSPVALID) {
+            if (be32_to_cpu(rsp->resp_data_len) >= 4)
+                target->tsk_mgmt_status = rsp->data[3];
+        }
          complete(&target->tsk_mgmt_done);
      } else {

          req = &target->req_ring[rsp->tag];

drivers/infiniband/ulp/srp/ib_srp.c +1 -1

@@ -1739,7 +1741,7 @@ static int srp_send_tsk_mgmt(struct
 srp_target_port *target,
                       msecs_to_jiffies(SRP_ABORT_TIMEOUT_MS)))
          return -1;
 
-    return 0;
+    return target->tsk_mgmt_status;
  }
 

static int srp_abort(struct scsi_cmnd *scmnd)

drivers/infiniband/ulp/srp/ib_srp.c +0 -2

@@ -1776,8 +1778,6 @@ static int srp_reset_device(struct scsi_cmnd
 *scmnd)
      if (srp_send_tsk_mgmt(target, SRP_TAG_NO_REQ, scmnd->device->lun,
                    SRP_TSK_LUN_RESET))
          return FAILED;
-    if (target->tsk_mgmt_status)
-        return FAILED;
 
      for (i = 0; i < SRP_CMD_SQ_SIZE; ++i) {

Quoted 2 lines

          struct srp_request *req = &target->req_ring[i];

Good catch, however:
- I think the STATUS field only has a meaning in replies to regular SCSI commands but not in SRP_TSK_MGMT replies.
- If the spec says that a target driver should always provide response data in response to a SRP_TSK_MGMT request, isn't it the target that should be modified instead of the initiator ?

Bart.

Quoted 100 lines · Bart Van Assche

Quoted 89 lines · Jack Wang

Hi all,

Currently handle of srp_rsp for task management is broken.

in 6.9
T10/1415-D revision 16a
SRP_RSP responses that contain either
RESPONSE DATA or SENSE DATA shall be sent as the minimum length message containing those fields.
LENGTH field specify the number of bytes in the RESPONSE DATA field. RSPVALID set to one also indicates that the
contents of the STATUS field shall be ignored by the SRP initiator port.

If response data is provided, RSPVALID shall be set to one and the RESPONSE DATA LIST LENGTH field shall specify
the number of bytes in the RESPONSE DATA field (see table 23). The RESPONSE DATA LIST LENGTH field shall
contain the value four. Other lengths are reserved for future standardization.
If no response data is provided, RSPVALID shall be set to zero. The SRP initiator port shall ignore the RESPONSE
DATA LIST LENGTH field and shall assume a length of zero. Response data shall be provided in any SRP_RSP response that is sent in response to an SRP_TSK_MGMT
request (see 6.7). The information in the RSP_CODE field (see table 24) shall indicate the completion status of
the task management function.
Response data shall not be provided in any SRP_RSP response that returns a non-zero status code in the
STATUS field.
The STATUS field contains the status of a task that completes.

Patch made against v3.12-rc1

Quoted 1 line

From 5f5af6de8dd72e37448841b7d7735d3eea4d3d83 Mon Sep 17 00:00:00 2001

From: Jack Wang <[email protected]>
Date: Fri, 27 Sep 2013 11:10:05 +0200
Subject: [PATCH] IB/srp: fix task management handle in srp

Currently the srp driver process task manamgement command in wrong way it just ignore the return srp_rsp for successful case eg rsp->status is success, fix this.

Signed-off-by Jack Wang <[email protected]>

Reviewed-by Dongsu Park <[email protected]>

---
  drivers/infiniband/ulp/srp/ib_srp.c | 12 ++++++------
  1 file changed, 6 insertions(+), 6 deletions(-)

drivers/infiniband/ulp/srp/ib_srp.c +5 -3

index f93baf8..5e1f1bf 100644
@@ -1145,9 +1145,11 @@ static void srp_process_rsp(struct
 srp_target_port *target, struct srp_rsp *rsp)
          target->req_lim += be32_to_cpu(rsp->req_lim_delta);
          spin_unlock_irqrestore(&target->lock, flags);
 
-        target->tsk_mgmt_status = -1;
-        if (be32_to_cpu(rsp->resp_data_len) >= 4)
-            target->tsk_mgmt_status = rsp->data[3];
+        target->tsk_mgmt_status = rsp->status;
+        if (rsp->flags & SRP_RSP_FLAG_RSPVALID) {
+            if (be32_to_cpu(rsp->resp_data_len) >= 4)
+                target->tsk_mgmt_status = rsp->data[3];
+        }
          complete(&target->tsk_mgmt_done);
      } else {

          req = &target->req_ring[rsp->tag];

drivers/infiniband/ulp/srp/ib_srp.c +1 -1

@@ -1739,7 +1741,7 @@ static int srp_send_tsk_mgmt(struct
 srp_target_port *target,
                       msecs_to_jiffies(SRP_ABORT_TIMEOUT_MS)))
          return -1;
 
-    return 0;
+    return target->tsk_mgmt_status;
  }
 

static int srp_abort(struct scsi_cmnd *scmnd)

drivers/infiniband/ulp/srp/ib_srp.c +0 -2

@@ -1776,8 +1778,6 @@ static int srp_reset_device(struct scsi_cmnd
 *scmnd)
      if (srp_send_tsk_mgmt(target, SRP_TAG_NO_REQ, scmnd->device->lun,
                    SRP_TSK_LUN_RESET))
          return FAILED;
-    if (target->tsk_mgmt_status)
-        return FAILED;
 
      for (i = 0; i < SRP_CMD_SQ_SIZE; ++i) {

Quoted 2 lines

          struct srp_request *req = &target->req_ring[i];

Good catch, however:
- I think the STATUS field only has a meaning in replies to regular SCSI commands but not in SRP_TSK_MGMT replies.
- If the spec says that a target driver should always provide response data in response to a SRP_TSK_MGMT request, isn't it the target that should be modified instead of the initiator ?

Quoted 1 line

Bart.

Hello Bart,

It's a little vague in the srp spec about status definition(as it only a draft version several years old without any update), I think it is quite efficient to also use status for succesful task management commands too. SAS has similar usage for that as I know.

But it will be great if you could fix srpt in SCST:)