On Sun, 2011-11-20 at 16:23 -0500, Christoph Hellwig wrote:
> On Sat, Nov 19, 2011 at 05:08:35AM +0000, Nicholas A. Bellinger wrote:
> > +/**
> > + * target_submit_cmd() - Lookup unpacked lun and submit a previously
> > + * uninitialized se_cmd descriptor to struct se_lun->lun_se_dev
> > + * backend using struct se_sess nexus.
> > + *
> > + * This may only be called from process context, and also currently
> > + * assumes internal allocation of fabric payload buffer by target-core.
> > + *
> > + * @se_cmd: command descriptor to submit
> > + * @se_sess: associated se_sess for endpoint
> > + * @cdb: pointer to SCSI CDB
> > + * @sense: pointer to SCSI sense buffer
> > + * @unpacked_lun: unpacked LUN to reference for struct se_lun
> > + * @data_length: fabric expected data transfer length
> > + * @task_addr: SAM task attribute
> > + * @data_dir: DMA data direction
> > + * @bidi: signal bidirectional data payload
> > + **/
>
> kerneldoc should just have a single line description at the top,
> and then real comments below, e.g.:
>
>
> /**
> * target_submit_cmd - submit a command to the target core
> *
> * @se_cmd: command descriptor to submit
> * @se_sess: associated se_sess for endpoint
> * @cdb: pointer to SCSI CDB
> * @sense: pointer to SCSI sense buffer
> * @unpacked_lun: logic unit number
> * @data_length: fabric expected data transfer length
> * @task_addr: SAM task attribute
> * @data_dir: DMA data direction
> * @flags: target core submission flags
> */
>
Er, sorry, fixed.
> > +int target_submit_cmd(
> > + struct se_cmd *se_cmd,
> > + struct se_session *se_sess,
> > + unsigned char *cdb,
> > + unsigned char *sense,
> > + u32 unpacked_lun,
> > + u32 data_length,
> > + int task_attr,
> > + int data_dir,
> > + int bidi)
>
> Please use standard formatting, e.g.
>
> int target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
> unsigned char *cdb, unsigned char *sense, u32 unpacked_lun,
> u32 data_length, int task_attr, int data_dir, int flags)
>
> note that I also replaced bidi with a flags argument. This is genrally
> more extensible, and with the memory allocation and session linkage we
> already have two additionals ones that should be implemented from the
> start.
>
<nod>, adding TARGET_SCF_BIDI_OP in enum target_sc_flags_table now and
changing this usage.
> > + if (!se_cmd || !se_sess) {
> > + pr_err("Missing se_cmd or se_sess descriptor\n");
> > + return -EINVAL;
> > + }
>
> No need to check this, we'll faill ASAP anyway.
>
OK
> > + se_tpg = se_sess->se_tpg;
> > + if (!se_tpg) {
> > + pr_err("Unable to locate se_tpg in target_submit_cmd\n");
> > + return -EINVAL;
> > + }
>
> if this should be done at all this should be a BUG_ON.
changed to BUG_ON
>
> > + if (se_cmd->se_tfo || se_cmd->se_sess) {
> > + pr_err("struct se_cmd already initialized\n");
> > + return -EEXIST;
> > + }
>
ditto
>
> > + if (in_interrupt()) {
> > + pr_err("target_submit_cmd() may only be used from process context\n");
> > + return -ENOSYS;
> > + }
>
> Same.
>
ditto
> > + /*
> > + * Initialize se_cmd for target operation. From this point
> > + * exceptions are handled by sending exception status via
> > + * target_core_fabric_ops->queue_status() callback
> > + */
> > + transport_init_se_cmd(se_cmd, se_tpg->se_tpg_tfo, se_sess,
> > + data_length, data_dir, task_attr, sense);
> > + /*
> > + * Obtain struct se_cmd->cmd_kref references and add new cmd to
> > + * se_sess->sess_cmd_list
> > + */
>
> Please don't add lengthy comments that only explain what the functions
> called are intended to do. That should be documented in those functions
> if at all. Comments here should only describe something that is non-
> obvious from the code.
>
This comment actually needs to mention a double vs. single kref case for
the default loopback case. That said, I'll go ahead and add a second
TARGET_SCF flag to signal single se_cmd->cmd_kref operation when we are
not expecting an external HW interrupt response to queue into wq context
and call the second kref_put().
> > + transport_send_check_condition_and_sense(se_cmd,
> > + se_cmd->scsi_sense_reason, 0);
> > + return 0;
> > + }
>
>
> > + transport_send_check_condition_and_sense(se_cmd,
> > + se_cmd->scsi_sense_reason, 0);
> > + return 0;
>
> There probably should be a common goto label for these.
>
>
changed
> Also any chance we could add more users for it quickly? tcm_fc is
> a really easy one, but it really should replace
> transport_handle_cdb_direct in the short to medium term to really
> make sense.
I think so.. I'll look at getting tcm_fc converted over first, and see
if anything else needs to be done for iscsi-target to work as expected
with single kref TARGET_SCF_* usage.
--
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
