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

[Jack Wang <jinpu.wang-EIkl63zCoXaH+58JC4qpiA@public.gmane.org>]

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