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
*/
> +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.
> + 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.
> + 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.
> + if (se_cmd->se_tfo || se_cmd->se_sess) {
> + pr_err("struct se_cmd already initialized\n");
> + return -EEXIST;
> + }
Same.
> + if (in_interrupt()) {
> + pr_err("target_submit_cmd() may only be used from process context\n");
> + return -ENOSYS;
> + }
Same.
> + /*
> + * 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.
> + 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.
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.
--
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
