On 04/14/15 21:41, Christoph Hellwig wrote:
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.
OK, will do.
+ 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.
OK, I will restore that comment.
-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.
Eliminating that 'aborted' flag is possible but would take some work.
Would you perhaps like to see that ft_recv_seq() is modified such that
it invokes target_submit_tmr() if IS_ERR(fp) == true ? Anyway, removing
the cmd->aborted check from ft_get_task_tag() can be done easily via a
separate patch. The untested patch below even fixes a race condition:
diff --git a/drivers/target/tcm_fc/tcm_fc.h b/drivers/target/tcm_fc/tcm_fc.h
index 881deb3..c9275d0 100644
--- a/drivers/target/tcm_fc/tcm_fc.h
+++ b/drivers/target/tcm_fc/tcm_fc.h
@@ -116,6 +116,7 @@ struct ft_cmd {
struct ft_sess *sess; /* session held for cmd */
struct fc_seq *seq; /* sequence in exchange mgr */
struct se_cmd se_cmd; /* Local TCM I/O descriptor */
+ u16 rxid;
struct fc_frame *req_frame;
u32 write_data_len; /* data received on writes */
struct work_struct work;
diff --git a/drivers/target/tcm_fc/tfc_cmd.c
b/drivers/target/tcm_fc/tfc_cmd.c
index edcafa4..d7972ff 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -251,9 +251,7 @@ 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;
- return fc_seq_exch(cmd->seq)->rxid;
+ return cmd->rxid;
}
int ft_get_cmd_state(struct se_cmd *se_cmd)
@@ -528,6 +526,8 @@ static void ft_send_work(struct work_struct *work)
if (fcp->fc_flags & FCP_CFL_LEN_MASK)
goto err; /* not handling longer CDBs yet */
+ cmd->rxid = fc_seq_exch(cmd->seq)->rxid;
+
/*
* Check for FCP task management flags
*/
- 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?
I know this is not a good reason but I had not yet had the time to
figure out how to handle the USBG_IS_BOT case properly. Since the tag
and bot_tag members are used in many places, the size of the diff can be
reduced by keeping these members. Felipe, can you review whether the
changes below don't break anything in the TCM USB gadget driver ?
diff --git a/drivers/usb/gadget/legacy/tcm_usb_gadget.c
b/drivers/usb/gadget/legacy/tcm_usb_gadget.c
index 8b80add..ae4cbac 100644
--- a/drivers/usb/gadget/legacy/tcm_usb_gadget.c
+++ b/drivers/usb/gadget/legacy/tcm_usb_gadget.c
@@ -1098,6 +1098,7 @@ static int usbg_submit_command(struct f_uas *fu,
if (!cmd)
return -ENOMEM;
+ se_cmd = &cmd->se_cmd;
cmd->fu = fu;
/* XXX until I figure out why I can't free in on complete */
@@ -1112,6 +1113,7 @@ static int usbg_submit_command(struct f_uas *fu,
memcpy(cmd->cmd_buf, cmd_iu->cdb, cmd_len);
cmd->tag = be16_to_cpup(&cmd_iu->tag);
+ se_cmd->tag = cmd->tag;
if (fu->flags & USBG_USE_STREAMS) {
if (cmd->tag > UASP_SS_EP_COMP_NUM_STREAMS)
goto err;
@@ -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);
@@ -1245,6 +1246,7 @@ static int bot_submit_command(struct f_uas *fu,
cmd->unpacked_lun = cbw->Lun;
cmd->is_read = cbw->Flags & US_BULK_FLAG_IN ? 1 : 0;
cmd->data_len = le32_to_cpu(cbw->DataTransferLength);
+ se_cmd->tag = le32_to_cpu(cmd->bot_tag);
INIT_WORK(&cmd->work, bot_cmd_work);
ret = queue_work(tpg->workqueue, &cmd->work);
@@ -1448,18 +1450,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;
-}
-
static int usbg_get_cmd_state(struct se_cmd *se_cmd)
{
return 0;
@@ -1889,7 +1879,6 @@ static const struct target_core_fabric_ops
usbg_ops = {
.write_pending = usbg_send_write_request,
.write_pending_status = usbg_write_pending_status,
.set_default_node_attributes = usbg_set_default_node_attrs,
- .get_task_tag = usbg_get_task_tag,
.get_cmd_state = usbg_get_cmd_state,
.queue_data_in = usbg_send_read_response,
.queue_status = usbg_send_status_response,
--
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
