On Tue, Apr 14, 2015 at 01:26:44PM +0200, Bart Van Assche wrote:
> Simplify target core and target drivers by storing the task tag
> a.k.a. command identifier inside struct se_cmd.
I think this should have your 3rd patch for 64-bit tags squashed in.
> + cmd->se_cmd.tag = (__force u32)cmd->init_task_tag;
> cmd->sense_reason = target_setup_cmd_from_cdb(&cmd->se_cmd, hdr->cdb);
> if (cmd->sense_reason) {
> if (cmd->sense_reason == TCM_OUT_OF_RESOURCES) {
> diff --git a/drivers/target/iscsi/iscsi_target_configfs.c b/drivers/target/iscsi/iscsi_target_configfs.c
> index 469fce4..8af7105 100644
> --- a/drivers/target/iscsi/iscsi_target_configfs.c
> +++ b/drivers/target/iscsi/iscsi_target_configfs.c
> @@ -1735,14 +1735,6 @@ static char *iscsi_get_fabric_name(void)
> return "iSCSI";
> }
>
> -static u32 iscsi_get_task_tag(struct se_cmd *se_cmd)
> -{
> - struct iscsi_cmd *cmd = container_of(se_cmd, struct iscsi_cmd, se_cmd);
> -
> - /* only used for printks or comparism with ->ref_task_tag */
> - return (__force u32)cmd->init_task_tag;
You lost this comment while moving the assignment around. Given that
each use of __force really needs a comment it should be brought back.
> -u32 ft_get_task_tag(struct se_cmd *se_cmd)
> -{
> - struct ft_cmd *cmd = container_of(se_cmd, struct ft_cmd, se_cmd);
> -
> - if (cmd->aborted)
> - return ~0;
The description needs a blurb why we can remove this check, or even
better the removal should be a separate patch.
As far as I can tell it's fine as ->get_task_tag is only used either
for printks, or ABORT_TASK. For the first removing this hack obviously
is an improvement, but I can't really see how you keep the behavior
for ABORT_TASK.
That being said the whole local ->aborted flag in tcm_fc looks like
a bad hack to me.
> - cmd->tag = be16_to_cpup(&cmd_iu->tag);
> + se_cmd->tag = be16_to_cpup(&cmd_iu->tag);
> if (fu->flags & USBG_USE_STREAMS) {
> - if (cmd->tag > UASP_SS_EP_COMP_NUM_STREAMS)
> + if (se_cmd->tag > UASP_SS_EP_COMP_NUM_STREAMS)
> goto err;
> - if (!cmd->tag)
> + if (!se_cmd->tag)
> cmd->stream = &fu->stream[0];
> else
> - cmd->stream = &fu->stream[cmd->tag - 1];
> + cmd->stream = &fu->stream[se_cmd->tag - 1];
> } else {
> cmd->stream = &fu->stream[0];
> }
> @@ -1147,7 +1149,6 @@ static int usbg_submit_command(struct f_uas *fu,
> break;
> }
>
> - se_cmd = &cmd->se_cmd;
> cmd->unpacked_lun = scsilun_to_int(&cmd_iu->lun);
>
> INIT_WORK(&cmd->work, usbg_cmd_work);
> @@ -1448,18 +1449,6 @@ static void usbg_set_default_node_attrs(struct se_node_acl *nacl)
> return;
> }
>
> -static u32 usbg_get_task_tag(struct se_cmd *se_cmd)
> -{
> - struct usbg_cmd *cmd = container_of(se_cmd, struct usbg_cmd,
> - se_cmd);
> - struct f_uas *fu = cmd->fu;
> -
> - if (fu->flags & USBG_IS_BOT)
> - return le32_to_cpu(cmd->bot_tag);
> - else
> - return cmd->tag;
> -}
Why don't we need the USBG_IS_BOT case now?
--
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
