On Thu, 2010-10-28 at 11:48 -0700, Kiran Patil wrote:
> From: Kiran Patil <[email protected]>
>
> Fix: Added support in TCM/LIO target to offload large FC receives.
>
> Technical Details:
> 1. Modified TCM/LIO files (tfc*) to add support for receive offload.
> 2. Setup target to support task_sg_chaining by setting bit during
> registration with TCM fabric.
> 3. Fix the panic (NULL pointer access) in function transport_calc_sg_num.
>
> Signed-off-by: Kiran Patil <[email protected]>
> ---
>
> drivers/target/target_core_transport.c | 8 ++++
> drivers/target/tcm_fc/tcm_fc.h | 3 ++
> drivers/target/tcm_fc/tfc_cmd.c | 33 ++++++++++++++++-
> drivers/target/tcm_fc/tfc_conf.c | 4 ++
> drivers/target/tcm_fc/tfc_io.c | 64
> ++++++++++++++++++++++++++++++++
> 5 files changed, 109 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/target/target_core_transport.c
> b/drivers/target/target_core_transport.c
> index 0a35a5c..9ad38c5 100644
> --- a/drivers/target/target_core_transport.c
> +++ b/drivers/target/target_core_transport.c
> @@ -7324,7 +7324,13 @@ next:
> */
> if (task->task_padded_sg) {
> sg_mark_end(&task->task_sg[task->task_sg_num - 1]);
> - sg_mark_end(&task->task_sg_bidi[task->task_sg_num - 1]);
> + /*
> + * Added the 'if' check before marking end of bi-directional
> + * scatterlist (which gets created only in case of request
> + * (RD + WR).
> + */
> + if (task->task_sg_bidi)
> + sg_mark_end(&task->task_sg_bidi[task->task_sg_num - 1]);
> }
>
> DEBUG_SC("Successfully allocated task->task_sg_num(%u),"
Wow, good catch on this one for the BIDI operation case in
transport_calc_sg_num().
I will go ahead and merge this one as a seperate bugfix patch
> diff --git a/drivers/target/tcm_fc/tcm_fc.h b/drivers/target/tcm_fc/tcm_fc.h
> index 210c33e..8468736 100644
> --- a/drivers/target/tcm_fc/tcm_fc.h
> +++ b/drivers/target/tcm_fc/tcm_fc.h
> @@ -153,6 +153,9 @@ struct ft_cmd {
> u32 write_data_len; /* data received on writes */
> struct se_queue_req se_req;
> unsigned char ft_sense_buffer[TRANSPORT_SENSE_BUFFER]; /* Local sense
> buffer */
> + u32 was_ddp_setup:1; /* Set only if ddp is setup */
> + struct scatterlist *sg; /* Set only if DDP is setup */
> + u32 sg_cnt; /* No. of item in scatterlist */
> };
>
> extern struct list_head ft_lport_list;
> diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
> index e5b6aa1..aaf9b26 100644
> --- a/drivers/target/tcm_fc/tfc_cmd.c
> +++ b/drivers/target/tcm_fc/tfc_cmd.c
> @@ -72,9 +72,9 @@ void ft_dump_cmd(struct ft_cmd *cmd, const char *caller)
> printk(KERN_INFO "%s: cmd %p lun %d\n", caller, cmd, cmd->lun);
>
> task = T_TASK(se_cmd);
> - printk(KERN_INFO "%s: cmd %p task %p se_num %u buf %p len %u\n",
> + printk(KERN_INFO "%s: cmd %p task %p se_num %u buf %p len %u
> se_cmd_flags <0x%x>\n",
> caller, cmd, task, task->t_tasks_se_num,
> - task->t_task_buf, se_cmd->data_length);
> + task->t_task_buf, se_cmd->data_length, se_cmd->se_cmd_flags);
> if (task->t_mem_list)
> list_for_each_entry(mem, task->t_mem_list, se_list)
> printk(KERN_INFO "%s: cmd %p mem %p page %p "
> @@ -247,6 +247,8 @@ int ft_write_pending(struct se_cmd *se_cmd)
> struct fcp_txrdy *txrdy;
> struct fc_lport *lport;
> struct fc_exch *ep;
> + struct fc_frame_header *fh;
> + u32 f_ctl;
>
> ft_dump_cmd(cmd, __func__);
>
> @@ -264,6 +266,33 @@ int ft_write_pending(struct se_cmd *se_cmd)
> fc_fill_fc_hdr(fp, FC_RCTL_DD_DATA_DESC, ep->did, ep->sid, FC_TYPE_FCP,
> FC_FC_EX_CTX | FC_FC_END_SEQ | FC_FC_SEQ_INIT, 0);
>
> + fh = fc_frame_header_get(fp);
> + f_ctl = ntoh24(fh->fh_f_ctl);
> +
> + /* Only if it is 'Exchange Responder' */
> + if (f_ctl & FC_FC_EX_CTX) {
> + /* Target is 'exchange responder' and sending XFER_READY
> + * to 'exchange initiator (initiator)'
> + */
> + if (ep->xid <= lport->lro_xid &&
> + fh->fh_r_ctl == FC_RCTL_DD_DATA_DESC) {
I think that checkpatch.pl will complain w/ the extra () around the
inidividual conditionals here.. I will fix this up, but just a FYI..
> + if (se_cmd->se_cmd_flags & SCF_SCSI_DATA_SG_IO_CDB) {
> + /*
> + * Map se_mem list to scatterlist, so that
> + * DDP can be setup. DDP setup function require
> + * scatterlist. se_mem_list is internal to
> + * TCM/LIO target
> + */
> + transport_do_task_sg_chain(se_cmd);
> + cmd->sg = T_TASK(se_cmd)->t_tasks_sg_chained;
> + cmd->sg_cnt =
> + T_TASK(se_cmd)->t_tasks_sg_chained_no;
> + }
> + if (cmd->sg && lport->tt.ddp_setup(lport, ep->xid,
> + cmd->sg, cmd->sg_cnt))
> + cmd->was_ddp_setup = 1;
> + }
> + }
> lport->tt.seq_send(lport, cmd->seq, fp);
> return 0;
> }
> diff --git a/drivers/target/tcm_fc/tfc_conf.c
> b/drivers/target/tcm_fc/tfc_conf.c
> index 604c97d..3668c04 100644
> --- a/drivers/target/tcm_fc/tfc_conf.c
> +++ b/drivers/target/tcm_fc/tfc_conf.c
> @@ -598,6 +598,10 @@ int ft_register_configfs(void)
> return -1;
> }
> fabric->tf_ops = ft_fabric_ops;
> +
> + /* Allowing support for task_sg_chaining */
> + fabric->tf_ops.task_sg_chaining = 1;
> +
> /*
> * Setup default attribute lists for various fabric->tf_cit_tmpl
> */
> diff --git a/drivers/target/tcm_fc/tfc_io.c b/drivers/target/tcm_fc/tfc_io.c
> index 2080891..e77a45a 100644
> --- a/drivers/target/tcm_fc/tfc_io.c
> +++ b/drivers/target/tcm_fc/tfc_io.c
> @@ -188,6 +188,9 @@ int ft_queue_data_in(struct se_cmd *se_cmd)
> void ft_recv_write_data(struct ft_cmd *cmd, struct fc_frame *fp)
> {
> struct se_cmd *se_cmd = &cmd->se_cmd;
> + struct fc_seq *seq = cmd->seq;
> + struct fc_exch *ep;
> + struct fc_lport *lport;
> struct se_transport_task *task;
> struct fc_frame_header *fh;
> struct se_mem *mem;
> @@ -200,6 +203,8 @@ void ft_recv_write_data(struct ft_cmd *cmd, struct
> fc_frame *fp)
> void *page_addr;
> void *from;
> void *to;
> + u32 f_ctl;
> + void *buf;
>
> task = T_TASK(se_cmd);
> BUG_ON(!task);
> @@ -207,6 +212,64 @@ void ft_recv_write_data(struct ft_cmd *cmd, struct
> fc_frame *fp)
> fh = fc_frame_header_get(fp);
> if (!(ntoh24(fh->fh_f_ctl) & FC_FC_REL_OFF))
> goto drop;
> +
> + /*
> + * Doesn't expect even single byte of payload. Payload
> + * is expected to be copied directly to user buffers
> + * due to DDP (Large Rx offload) feature, hence
> + * BUG_ON if BUF is non-NULL
> + */
> + buf = fc_frame_payload_get(fp, 1);
> + if (cmd->was_ddp_setup && buf) {
> + printk(KERN_INFO "%s: When DDP was setup, not expected to"
> + "receive frame with payload, Payload shall be"
> + "copied directly to buffer instead of coming "
> + "via. legacy receive queues\n", __func__);
> + BUG_ON(buf);
> + }
> +
> + /*
> + * 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.
> + */
> + ep = fc_seq_exch(seq);
> + if (cmd->was_ddp_setup) {
> + BUG_ON(!ep);
> + lport = ep->lp;
> + BUG_ON(!lport);
> + }
> + if (cmd->was_ddp_setup && ep->xid != FC_XID_UNKNOWN) {
Fixing up.
> + f_ctl = ntoh24(fh->fh_f_ctl);
> + /*
> + * 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
> + */
> + if (f_ctl & FC_FC_SEQ_INIT) { /* TSI bit set in FC frame */
> + cmd->write_data_len = lport->tt.ddp_done(lport,
> + ep->xid);
> + 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
> + */
> + printk(KERN_ERR "%s: Received frame with TSI bit not"
> + " being SET, dropping the frame, "
> + "cmd->sg <%p>, cmd->sg_cnt <0x%x>\n",
> + __func__, cmd->sg, cmd->sg_cnt);
> + cmd->write_data_len = lport->tt.ddp_done(lport,
> + ep->xid);
> + lport->tt.seq_exch_abort(cmd->seq, 0);
> + goto drop;
> + }
> + }
> +
> rel_off = ntohl(fh->fh_parm_offset);
> frame_len = fr_len(fp);
> if (frame_len <= sizeof(*fh))
> @@ -273,6 +336,7 @@ void ft_recv_write_data(struct ft_cmd *cmd, struct
> fc_frame *fp)
> mem_len -= tlen;
> cmd->write_data_len += tlen;
> }
> +last_frame:
> if (cmd->write_data_len == se_cmd->data_length)
> transport_generic_handle_data(se_cmd);
> drop:
>
Aside from the minor nit+fixup, I think this code all looks very good.
Joe, please have a look at the libfc and tcm_fc pieces and ACK
accordingly and I will get this series pushed into lio-core.2.6.git and
the bugfix into my mainline tree.
Many thanks for your contribution Kiran!
--nab
_______________________________________________
devel mailing list
[email protected]
http://www.open-fcoe.org/mailman/listinfo/devel
