Re: [PATCH 10/15] IB/srpt: Fix srpt_handle_cmd() error paths

[Sagi Grimberg]

On 05/01/2016 16:25, Bart Van Assche wrote:

The target core function that should be called if target_submit_cmd()
fails is target_put_sess_cmd(). Additionally, change the return type
of srpt_handle_cmd() from int into void.

Signed-off-by: Bart Van Assche 
Cc: Christoph Hellwig 
---
  drivers/infiniband/ulp/srpt/ib_srpt.c | 24 
  1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c 
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 1857d17..395bc1b 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1482,15 +1482,14 @@ static int srpt_check_stop_free(struct se_cmd *cmd)
  /**
   * srpt_handle_cmd() - Process SRP_CMD.
   */
-static int srpt_handle_cmd(struct srpt_rdma_ch *ch,
-  struct srpt_recv_ioctx *recv_ioctx,
-  struct srpt_send_ioctx *send_ioctx)
+static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
+   struct srpt_recv_ioctx *recv_ioctx,
+   struct srpt_send_ioctx *send_ioctx)
  {
struct se_cmd *cmd;
struct srp_cmd *srp_cmd;
u64 data_len;
enum dma_data_direction dir;
-   sense_reason_t ret;
int rc;

BUG_ON(!send_ioctx);
@@ -1518,8 +1517,7 @@ static int srpt_handle_cmd(struct srpt_rdma_ch *ch,
if (srpt_get_desc_tbl(send_ioctx, srp_cmd, , _len)) {
pr_err("0x%llx: parsing SRP descriptor table failed.\n",
   srp_cmd->tag);
-   ret = TCM_INVALID_CDB_FIELD;
-   goto send_sense;
+   goto put;


Do you still need to call target_put_sess_cmd() in this failure?
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 10/15] IB/srpt: Fix srpt_handle_cmd() error paths

[Bart Van Assche]

On 01/06/2016 03:31 PM, Sagi Grimberg wrote:

On 05/01/2016 16:25, Bart Van Assche wrote:

@@ -1518,8 +1517,7 @@ static int srpt_handle_cmd(struct srpt_rdma_ch *ch,
  if (srpt_get_desc_tbl(send_ioctx, srp_cmd, , _len)) {
  pr_err("0x%llx: parsing SRP descriptor table failed.\n",
 srp_cmd->tag);
-ret = TCM_INVALID_CDB_FIELD;
-goto send_sense;
+goto put;


Do you still need to call target_put_sess_cmd() in this failure?


Good catch. I will update this patch before I repost this patch series.

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

[PATCH 10/15] IB/srpt: Fix srpt_handle_cmd() error paths

[Bart Van Assche]

The target core function that should be called if target_submit_cmd()
fails is target_put_sess_cmd(). Additionally, change the return type
of srpt_handle_cmd() from int into void.

Signed-off-by: Bart Van Assche 
Cc: Christoph Hellwig 
---
 drivers/infiniband/ulp/srpt/ib_srpt.c | 24 
 1 file changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/infiniband/ulp/srpt/ib_srpt.c 
b/drivers/infiniband/ulp/srpt/ib_srpt.c
index 1857d17..395bc1b 100644
--- a/drivers/infiniband/ulp/srpt/ib_srpt.c
+++ b/drivers/infiniband/ulp/srpt/ib_srpt.c
@@ -1482,15 +1482,14 @@ static int srpt_check_stop_free(struct se_cmd *cmd)
 /**
  * srpt_handle_cmd() - Process SRP_CMD.
  */
-static int srpt_handle_cmd(struct srpt_rdma_ch *ch,
-  struct srpt_recv_ioctx *recv_ioctx,
-  struct srpt_send_ioctx *send_ioctx)
+static void srpt_handle_cmd(struct srpt_rdma_ch *ch,
+   struct srpt_recv_ioctx *recv_ioctx,
+   struct srpt_send_ioctx *send_ioctx)
 {
struct se_cmd *cmd;
struct srp_cmd *srp_cmd;
u64 data_len;
enum dma_data_direction dir;
-   sense_reason_t ret;
int rc;
 
BUG_ON(!send_ioctx);
@@ -1518,8 +1517,7 @@ static int srpt_handle_cmd(struct srpt_rdma_ch *ch,
if (srpt_get_desc_tbl(send_ioctx, srp_cmd, , _len)) {
pr_err("0x%llx: parsing SRP descriptor table failed.\n",
   srp_cmd->tag);
-   ret = TCM_INVALID_CDB_FIELD;
-   goto send_sense;
+   goto put;
}
 
rc = target_submit_cmd(cmd, ch->sess, srp_cmd->cdb,
@@ -1527,14 +1525,16 @@ static int srpt_handle_cmd(struct srpt_rdma_ch *ch,
   scsilun_to_int(_cmd->lun), data_len,
   TCM_SIMPLE_TAG, dir, TARGET_SCF_ACK_KREF);
if (rc != 0) {
-   ret = TCM_LOGICAL_UNIT_COMMUNICATION_FAILURE;
-   goto send_sense;
+   pr_debug("target_submit_cmd() returned %d for tag %#llx\n", rc,
+srp_cmd->tag);
+   goto put;
}
-   return 0;
+   return;
 
-send_sense:
-   transport_send_check_condition_and_sense(cmd, ret, 0);
-   return -1;
+put:
+   send_ioctx->state = SRPT_STATE_DONE;
+   target_put_sess_cmd(cmd);
+   return;
 }
 
 static int srp_tmr_to_tcm(int fn)
-- 
2.1.4

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

Re: [PATCH 10/15] IB/srpt: Fix srpt_handle_cmd() error paths

[Christoph Hellwig]

On Tue, Jan 05, 2016 at 03:25:13PM +0100, Bart Van Assche wrote:
> The target core function that should be called if target_submit_cmd()
> fails is target_put_sess_cmd(). Additionally, change the return type
> of srpt_handle_cmd() from int into void.

I actually ran into this bug a long time ago with a modified srpt driver
and forgot to send a similar fix..

Looks good:

Reviewed-by: Christoph Hellwig 

Minor nitpick below:

> + send_ioctx->state = SRPT_STATE_DONE;
> + target_put_sess_cmd(cmd);
> + return;
>  }

no need for that return statement.
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html