On Tue, 2012-07-17 at 23:37 +0000, Nicholas A. Bellinger wrote:
> From: Roland Dreier <roland@xxxxxxxxxxxxxxx>
>
> We want it to be possible for target_submit_cmd() to return errors up
> to its fabric module callers. For now just update the prototype to
> return an int, and update all callers to handle non-zero return values
> as an error.
>
> This is immediately useful for tcm_qla2xxx to fix a long-standing active
> I/O session shutdown race, but tcm_fc, usb-gadget, and sbp-target the
> fabric maintainers need to check + ACK that handling a target_submit_cmd()
> failure due to session shutdown does not introduce regressions
>
> (nab: Respin against for-next after initial NACK + update docbook comment)
>
<SNIP> + trimming CC to USB folks:
> index 02ace18..9ddf315 100644
> --- a/drivers/usb/gadget/tcm_usb_gadget.c
> +++ b/drivers/usb/gadget/tcm_usb_gadget.c
> @@ -1060,7 +1060,10 @@ static void usbg_cmd_work(struct work_struct *work)
> tpg = cmd->fu->tpg;
> tv_nexus = tpg->tpg_nexus;
> dir = get_cmd_dir(cmd->cmd_buf);
> - if (dir < 0) {
> + if (dir < 0 ||
> + target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
> + cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
> + 0, cmd->prio_attr, dir, TARGET_SCF_UNKNOWN_SIZE)) {
> transport_init_se_cmd(se_cmd,
> tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo,
> tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE,
> @@ -1069,12 +1072,7 @@ static void usbg_cmd_work(struct work_struct *work)
> transport_send_check_condition_and_sense(se_cmd,
> TCM_UNSUPPORTED_SCSI_OPCODE, 1);
> usbg_cleanup_cmd(cmd);
> - return;
> }
> -
> - target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
> - cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
> - 0, cmd->prio_attr, dir, TARGET_SCF_UNKNOWN_SIZE);
> }
>
> static int usbg_submit_command(struct f_uas *fu,
> @@ -1172,7 +1170,10 @@ static void bot_cmd_work(struct work_struct *work)
> tpg = cmd->fu->tpg;
> tv_nexus = tpg->tpg_nexus;
> dir = get_cmd_dir(cmd->cmd_buf);
> - if (dir < 0) {
> + if (dir < 0 ||
> + target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
> + cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
> + cmd->data_len, cmd->prio_attr, dir, 0)) {
> transport_init_se_cmd(se_cmd,
> tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo,
> tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE,
> @@ -1181,12 +1182,7 @@ static void bot_cmd_work(struct work_struct *work)
> transport_send_check_condition_and_sense(se_cmd,
> TCM_UNSUPPORTED_SCSI_OPCODE, 1);
> usbg_cleanup_cmd(cmd);
> - return;
> }
> -
> - target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
> - cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
> - cmd->data_len, cmd->prio_attr, dir, 0);
> }
>
Looking at this part again target_submit_cmd() has been moved ahead of
target_init_se_cmd() in the exception path, which ends up getting called
twice during a target_get_sess_cmd() failure..
It looks like usbg_cmd_work() + bot_cmd_work() will need some work if
they intends to use a non zero failure to signal exception here, without
invoking a CHECK_CONDITION and sense. Actually, I'm not even sure
sending a CHECK_CONDITION here after the target_submit_cmd() is going to
work for other fabrics drivers, but it appears to work with usb-gadget.
(Sebastian..?)
So for the moment, lets fix the double se_cmd init and make things a
little easier to read.. Sebastian, please have a look and double check
that the (dir < 0) + target_submit_cmd() failures cases are both going
to work as expected whilst sending a CHECK_CONDITION response.
Thanks!
--nab
diff --git a/drivers/usb/gadget/tcm_usb_gadget.c b/drivers/usb/gadget/tcm_usb_gadget.c
index 9ddf315..5444866 100644
--- a/drivers/usb/gadget/tcm_usb_gadget.c
+++ b/drivers/usb/gadget/tcm_usb_gadget.c
@@ -1060,19 +1060,25 @@ static void usbg_cmd_work(struct work_struct *work)
tpg = cmd->fu->tpg;
tv_nexus = tpg->tpg_nexus;
dir = get_cmd_dir(cmd->cmd_buf);
- if (dir < 0 ||
- target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
- cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
- 0, cmd->prio_attr, dir, TARGET_SCF_UNKNOWN_SIZE)) {
+ if (dir < 0) {
transport_init_se_cmd(se_cmd,
tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo,
tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE,
cmd->prio_attr, cmd->sense_iu.sense);
-
- transport_send_check_condition_and_sense(se_cmd,
- TCM_UNSUPPORTED_SCSI_OPCODE, 1);
- usbg_cleanup_cmd(cmd);
+ goto out;
}
+
+ if (target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
+ cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
+ 0, cmd->prio_attr, dir, TARGET_SCF_UNKNOWN_SIZE) < 0)
+ goto out;
+
+ return;
+
+out:
+ transport_send_check_condition_and_sense(se_cmd,
+ TCM_UNSUPPORTED_SCSI_OPCODE, 1);
+ usbg_cleanup_cmd(cmd);
}
static int usbg_submit_command(struct f_uas *fu,
@@ -1170,19 +1176,25 @@ static void bot_cmd_work(struct work_struct *work)
tpg = cmd->fu->tpg;
tv_nexus = tpg->tpg_nexus;
dir = get_cmd_dir(cmd->cmd_buf);
- if (dir < 0 ||
- target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
- cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
- cmd->data_len, cmd->prio_attr, dir, 0)) {
+ if (dir < 0) {
transport_init_se_cmd(se_cmd,
tv_nexus->tvn_se_sess->se_tpg->se_tpg_tfo,
tv_nexus->tvn_se_sess, cmd->data_len, DMA_NONE,
cmd->prio_attr, cmd->sense_iu.sense);
+ goto out;
+ }
+
+ if (target_submit_cmd(se_cmd, tv_nexus->tvn_se_sess,
+ cmd->cmd_buf, cmd->sense_iu.sense, cmd->unpacked_lun,
+ cmd->data_len, cmd->prio_attr, dir, 0) < 0)
+ goto out;
+
+ return;
- transport_send_check_condition_and_sense(se_cmd,
+out:
+ transport_send_check_condition_and_sense(se_cmd,
TCM_UNSUPPORTED_SCSI_OPCODE, 1);
- usbg_cleanup_cmd(cmd);
- }
+ usbg_cleanup_cmd(cmd);
}
static int bot_submit_command(struct f_uas *fu,
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
