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"
"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;
}
_______________________________________________
devel mailing list
devel@xxxxxxxxxxxxx
https://lists.open-fcoe.org/mailman/listinfo/devel
