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

[Bart Van Assche <bvanassche-HInyCGIudOg@public.gmane.org>]

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