On Wed, 2011-07-27 at 14:28 -0700, Kiran Patil wrote:
> From: Kiran Patil <kiran.patil@xxxxxxxxx>
>
> Problem: HW DDP context wasn;t invalidated in case of ABORTS, etc...
> This leads to the problem where memory pages which are used for DDP
> as user descriptor could get reused for some other purpose (such as to
> satisfy new memory allocation request either by kernel or user mode threads)
> and since HW DDP context was not invalidated, HW continue to write to
> those pages, hence causing memory corruption.
>
Ouch...
> Fix: Either on incoming ABORTS or due to exchange time out, allowed the
> target to cleanup HW DDP context if it was setup for respective ft_cmd.
> Added new function to perform this cleanup, furthur it can be enhanced
> for other cleanup activity.
>
> Additinal Notes: To avoid calling ddp_done from multiple places, composed
> the functionality in helper function "ft_invl_hw_context" and it is being
> called from multiple places. Cleaned up code in function "ft_recv_write_data"
> w.r.t DDP.
>
Ok, along with fixing up the pr_debug / pr_err stuff to follow LIO
upstream, I did notice one thing below that I think is bogus.. Please
have a look:
> Signed-off-by: Kiran Patil <kiran.patil@xxxxxxxxx>
> ---
>
> drivers/target/tcm_fc/tcm_fc.h | 5 ++
> drivers/target/tcm_fc/tfc_cmd.c | 1
> drivers/target/tcm_fc/tfc_io.c | 101 +++++++++++++++++++++++++--------------
> 3 files changed, 70 insertions(+), 37 deletions(-)
>
> diff --git a/drivers/target/tcm_fc/tcm_fc.h b/drivers/target/tcm_fc/tcm_fc.h
> index 7b82f1b..22d8f17 100644
> --- a/drivers/target/tcm_fc/tcm_fc.h
> +++ b/drivers/target/tcm_fc/tcm_fc.h
> @@ -212,4 +212,9 @@ void ft_dump_cmd(struct ft_cmd *, const char *caller);
>
> ssize_t ft_format_wwn(char *, size_t, u64);
>
> +/*
> + * Underlying HW specific helper function
> + */
> +void ft_invl_hw_context(struct ft_cmd *);
> +
> #endif /* __TCM_FC_H__ */
> diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
> index 328ea2b..8a24028 100644
> --- a/drivers/target/tcm_fc/tfc_cmd.c
> +++ b/drivers/target/tcm_fc/tfc_cmd.c
> @@ -329,6 +329,7 @@ static void ft_recv_seq(struct fc_seq *sp, struct fc_frame *fp, void *arg)
> default:
> printk(KERN_INFO "%s: unhandled frame r_ctl %x\n",
> __func__, fh->fh_r_ctl);
> + ft_invl_hw_context(cmd);
> fc_frame_free(fp);
> transport_generic_free_cmd(&cmd->se_cmd, 0, 1, 0);
> break;
> diff --git a/drivers/target/tcm_fc/tfc_io.c b/drivers/target/tcm_fc/tfc_io.c
> index c80ba9f..74be59e 100644
> --- a/drivers/target/tcm_fc/tfc_io.c
> +++ b/drivers/target/tcm_fc/tfc_io.c
> @@ -249,49 +249,40 @@ void ft_recv_write_data(struct ft_cmd *cmd, struct fc_frame *fp)
> if (cmd->was_ddp_setup) {
> BUG_ON(!ep);
> BUG_ON(!lport);
> - }
> - /*
> - * Doesn't expect payload if DDP is setup. Payload
> - * is expected to be copied directly to user buffers
> - * due to DDP (Large Rx offload),
> - */
> - buf = fc_frame_payload_get(fp, 1);
> - if (cmd->was_ddp_setup && buf) {
> - printk(KERN_ERR "%s: xid 0x%x, f_ctl 0x%x, cmd->sg %p, "
> + /*
> + * Since DDP (Large Rx offload) was setup for this request,
> + * payload is expected to be copied directly to user buffers.
> + */
> + buf = fc_frame_payload_get(fp, 1);
> + if (buf)
> + printk(KERN_ERR "%s: xid 0x%x, f_ctl 0x%x, cmd->sg %p, "
> "cmd->sg_cnt 0x%x. DDP was setup"
> " hence not expected to receive frame with "
> - "payload, Frame will be dropped if TSI bit"
> - "in f_ctl is not set\n",
> - __func__, ep->xid, f_ctl, cmd->sg, cmd->sg_cnt);
> - }
The 'if (buf)' check from fc_frame_payload_get() looks incorrect because
1) it's not checking for cmd->was_ddp_setup, and 2) because it's scope
is only the printk(), and not the ft_invl_hw_context() call, et al. that
I think you originally had in mind..
> - /*
> - * If ft_cmd indicated 'ddp_setup', in that case only the last frame
> - * should come with 'TSI bit being set'. If 'TSI bit is not set and if
> - * data frame appears here, means error condition. In both the cases
> - * release the DDP context (ddp_put) and in error case, as well
> - * initiate error recovery mechanism.
> - */
> - if (cmd->was_ddp_setup && ep->xid != FC_XID_UNKNOWN) {
> + "payload, Frame will be dropped if"
> + "'Sequence Initiative' bit in f_ctl is"
> + "not set\n", __func__, ep->xid, f_ctl,
> + cmd->sg, cmd->sg_cnt);
> /*
> - * If TSI bit set in f_ctl, means last write data frame is
> - * received successfully where payload is posted directly
> - * to user buffer and only the last frame's header is posted
> - * in legacy receive queue
> + * Invalidate HW DDP context if it was setup for respective
> + * command. Invalidation of HW DDP context is requited in both
> + * situation (success and error).
> */
> - if (f_ctl & FC_FC_SEQ_INIT) { /* TSI bit set in FC frame */
> - cmd->write_data_len = lport->tt.ddp_done(lport,
> - ep->xid);
> + ft_invl_hw_context(cmd);
> +
> + /*
> + * If "Sequence Initiative (TSI)" bit set in f_ctl, means last
> + * write data frame is received successfully where payload is
> + * posted directly to user buffer and only the last frame's
> + * header is posted in receive queue.
> + *
> + * If "Sequence Initiative (TSI)" bit is not set, means error
> + * condition w.r.t. DDP, hence drop the packet and let explict
> + * ABORTS from other end of exchange timer trigger the recovery.
> + */
> + if (f_ctl & FC_FC_SEQ_INIT)
> goto last_frame;
> - } else {
> - /*
> - * Updating the write_data_len may be meaningless at
> - * this point, but just in case if required in future
> - * for debugging or any other purpose
> - */
> - cmd->write_data_len = lport->tt.ddp_done(lport,
> - ep->xid);
> + else
> goto drop;
> - }
> }
>
> rel_off = ntohl(fh->fh_parm_offset);
<SNIP>
So the end result of the change to ft_recv_write_data() should look like
the following, yes..?
/*
* Doesn't expect payload if DDP is setup. Payload
* is expected to be copied directly to user buffers
* due to DDP (Large Rx offload),
*/
buf = fc_frame_payload_get(fp, 1);
if (cmd->was_ddp_setup && buf) {
pr_err("%s: xid 0x%x, f_ctl 0x%x, cmd->sg %p, "
"cmd->sg_cnt 0x%x. DDP was setup"
" hence not expected to receive frame with "
"payload, Frame will be dropped if"
"'Sequence Initiative' bit in f_ctl is"
"not set\n", __func__, ep->xid, f_ctl,
cmd->sg, cmd->sg_cnt);
/*
* Invalidate HW DDP context if it was setup for respective
* command. Invalidation of HW DDP context is requited in both
* situation (success and error).
*/
ft_invl_hw_context(cmd);
/*
* If "Sequence Initiative (TSI)" bit set in f_ctl, means last
* write data frame is received successfully where payload is
* posted directly to user buffer and only the last frame's
* header is posted in receive queue.
*
* If "Sequence Initiative (TSI)" bit is not set, means error
* condition w.r.t. DDP, hence drop the packet and let explict
* ABORTS from other end of exchange timer trigger the recovery.
*/
if (f_ctl & FC_FC_SEQ_INIT)
goto last_frame;
else
goto drop;
}
--
To unsubscribe from this list: send the line "unsubscribe target-devel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
