| Message ID | 52454DED.6020309@profitbricks.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
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@profitbricks.com> > 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@profitbricks.com> > Reviewed-by: Dongsu Park <dongsu.park@profitbricks.com> > --- > 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@profitbricks.com> >> 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@profitbricks.com> >> Reviewed-by: Dongsu Park <dongsu.park@profitbricks.com> >> --- >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@profitbricks.com> > >> 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@profitbricks.com> > >> Reviewed-by: Dongsu Park <dongsu.park@profitbricks.com> > >> --- > >> 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@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
From 5f5af6de8dd72e37448841b7d7735d3eea4d3d83 Mon Sep 17 00:00:00 2001 From: Jack Wang <jinpu.wang@profitbricks.com> 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@profitbricks.com> Reviewed-by: Dongsu Park <dongsu.park@profitbricks.com> --- 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