[PATCH]SRP: fix task management handle in srp

[PATCH]SRP: fix task management handle in srp

[Jack Wang]

[-- Attachment #1: Type: text/plain, Size: 3399 bytes --]

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

>From 5f5af6de8dd72e37448841b7d7735d3eea4d3d83 Mon Sep 17 00:00:00 2001
From: Jack Wang <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
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 <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
Reviewed-by: Dongsu Park <dongsu.park-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
b/drivers/infiniband/ulp/srp/ib_srp.c
index f93baf8..5e1f1bf 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -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];
@@ -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)
@@ -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) {
 		struct srp_request *req = &target->req_ring[i];
-- 
1.8.4

[-- Attachment #2: 0001-IB-srp-fix-task-management-handle-in-srp.patch --]
[-- Type: text/x-patch, Size: 2061 bytes --]

>From 5f5af6de8dd72e37448841b7d7735d3eea4d3d83 Mon Sep 17 00:00:00 2001
From: Jack Wang <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
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 <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
Reviewed-by: Dongsu Park <dongsu.park-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
---
 drivers/infiniband/ulp/srp/ib_srp.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/infiniband/ulp/srp/ib_srp.c b/drivers/infiniband/ulp/srp/ib_srp.c
index f93baf8..5e1f1bf 100644
--- a/drivers/infiniband/ulp/srp/ib_srp.c
+++ b/drivers/infiniband/ulp/srp/ib_srp.c
@@ -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];
@@ -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)
@@ -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) {
 		struct srp_request *req = &target->req_ring[i];
-- 
1.8.4

Re: [PATCH]SRP: fix task management handle in srp

[Bart Van Assche]

On 09/27/13 11:20, Jack Wang wrote:
> 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
>
>>From 5f5af6de8dd72e37448841b7d7735d3eea4d3d83 Mon Sep 17 00:00:00 2001
> From: Jack Wang <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
> 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 <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
> Reviewed-by: Dongsu Park <dongsu.park-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
> ---
>   drivers/infiniband/ulp/srp/ib_srp.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
> b/drivers/infiniband/ulp/srp/ib_srp.c
> index f93baf8..5e1f1bf 100644
> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> @@ -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];
> @@ -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)
> @@ -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) {
>   		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.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]SRP: fix task management handle in srp

[Jack Wang]

On 09/27/2013 12:30 PM, Bart Van Assche wrote:
> On 09/27/13 11:20, Jack Wang wrote:
>> 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
>>
>>> From 5f5af6de8dd72e37448841b7d7735d3eea4d3d83 Mon Sep 17 00:00:00 2001
>> From: Jack Wang <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
>> 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 <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
>> Reviewed-by: Dongsu Park <dongsu.park-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
>> ---
>>   drivers/infiniband/ulp/srp/ib_srp.c | 12 ++++++------
>>   1 file changed, 6 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
>> b/drivers/infiniband/ulp/srp/ib_srp.c
>> index f93baf8..5e1f1bf 100644
>> --- a/drivers/infiniband/ulp/srp/ib_srp.c
>> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
>> @@ -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];
>> @@ -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)
>> @@ -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) {
>>           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.

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:)

Cheers,
Jack




--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH]SRP: fix task management handle in srp

[Nicholas A. Bellinger]

On Fri, 2013-09-27 at 12:55 +0200, Jack Wang wrote:
> On 09/27/2013 12:30 PM, Bart Van Assche wrote:
> > On 09/27/13 11:20, Jack Wang wrote:
> >> 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
> >>
> >>> From 5f5af6de8dd72e37448841b7d7735d3eea4d3d83 Mon Sep 17 00:00:00 2001
> >> From: Jack Wang <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
> >> 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 <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
> >> Reviewed-by: Dongsu Park <dongsu.park-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>
> >> ---
> >>   drivers/infiniband/ulp/srp/ib_srp.c | 12 ++++++------
> >>   1 file changed, 6 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/infiniband/ulp/srp/ib_srp.c
> >> b/drivers/infiniband/ulp/srp/ib_srp.c
> >> index f93baf8..5e1f1bf 100644
> >> --- a/drivers/infiniband/ulp/srp/ib_srp.c
> >> +++ b/drivers/infiniband/ulp/srp/ib_srp.c
> >> @@ -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];
> >> @@ -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)
> >> @@ -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) {
> >>           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.
> 
> 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:)

It would be even better if someone sent a patch for srpt in mainline as
well, so I don't have to fish out bug fixes from an out of tree codebase
months (or years) after the fact.

--nab

--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html