* [PATCH] target: Allow for target_submit_cmd() returning errors
@ 2012-07-17 23:37 Nicholas A. Bellinger
2012-07-18 0:05 ` Nicholas A. Bellinger
0 siblings, 1 reply; 4+ messages in thread
From: Nicholas A. Bellinger @ 2012-07-17 23:37 UTC (permalink / raw)
To: target-devel
Cc: linux-scsi, Roland Dreier, Chad Dupuis, Arun Easi, Chris Boot,
Stefan Richter, Mark Rustad, Sebastian Andrzej Siewior,
Felipe Balbi, Andy Grover
From: Roland Dreier <roland@purestorage.com>
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)
Cc: Chad Dupuis <chad.dupuis@qlogic.com>
Cc: Arun Easi <arun.easi@qlogic.com>
Cc: Chris Boot <bootc@bootc.net>
Cc: Stefan Richter <stefanr@s5r6.in-berlin.de>
Cc: Mark Rustad <mark.d.rustad@intel.com>
Cc: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
Cc: Felipe Balbi <balbi@ti.com>
Cc: Andy Grover <agrover@redhat.com>
Signed-off-by: Roland Dreier <roland@purestorage.com>
Signed-off-by: Nicholas Bellinger <nab@linux-iscsi.org>
---
drivers/scsi/qla2xxx/tcm_qla2xxx.c | 3 +--
drivers/target/sbp/sbp_target.c | 8 +++++---
drivers/target/target_core_transport.c | 14 +++++++++-----
drivers/target/tcm_fc/tfc_cmd.c | 8 +++++---
drivers/usb/gadget/tcm_usb_gadget.c | 20 ++++++++------------
include/target/target_core_fabric.h | 2 +-
6 files changed, 29 insertions(+), 26 deletions(-)
diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
index 65a7ed9..4752f65 100644
--- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c
+++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c
@@ -597,10 +597,9 @@ static int tcm_qla2xxx_handle_cmd(scsi_qla_host_t *vha, struct qla_tgt_cmd *cmd,
return -EINVAL;
}
- target_submit_cmd(se_cmd, se_sess, cdb, &cmd->sense_buffer[0],
+ return target_submit_cmd(se_cmd, se_sess, cdb, &cmd->sense_buffer[0],
cmd->unpacked_lun, data_length, fcp_task_attr,
data_dir, flags);
- return 0;
}
static void tcm_qla2xxx_handle_data_work(struct work_struct *work)
diff --git a/drivers/target/sbp/sbp_target.c b/drivers/target/sbp/sbp_target.c
index e10e622..39ddba5 100644
--- a/drivers/target/sbp/sbp_target.c
+++ b/drivers/target/sbp/sbp_target.c
@@ -1235,9 +1235,11 @@ static void sbp_handle_command(struct sbp_target_request *req)
pr_debug("sbp_handle_command ORB:0x%llx unpacked_lun:%d data_len:%d data_dir:%d\n",
req->orb_pointer, unpacked_lun, data_length, data_dir);
- target_submit_cmd(&req->se_cmd, sess->se_sess, req->cmd_buf,
- req->sense_buf, unpacked_lun, data_length,
- MSG_SIMPLE_TAG, data_dir, 0);
+ if (target_submit_cmd(&req->se_cmd, sess->se_sess, req->cmd_buf,
+ req->sense_buf, unpacked_lun, data_length,
+ MSG_SIMPLE_TAG, data_dir, 0))
+ goto err;
+
return;
err:
diff --git a/drivers/target/target_core_transport.c b/drivers/target/target_core_transport.c
index 14e54b4..7647eca 100644
--- a/drivers/target/target_core_transport.c
+++ b/drivers/target/target_core_transport.c
@@ -1447,10 +1447,14 @@ EXPORT_SYMBOL(transport_handle_cdb_direct);
* @data_dir: DMA data direction
* @flags: flags for command submission from target_sc_flags_tables
*
+ * Returns non zero to signal active I/O shutdown failure. All other
+ * setup exceptions will be returned as a SCSI CHECK_CONDITION response,
+ * but still return zero here.
+ *
* This may only be called from process context, and also currently
* assumes internal allocation of fabric payload buffer by target-core.
**/
-void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
+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)
{
@@ -1478,7 +1482,7 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
*/
rc = target_get_sess_cmd(se_sess, se_cmd, (flags & TARGET_SCF_ACK_KREF));
if (rc)
- return;
+ return rc;
/*
* Signal bidirectional data payloads to target-core
*/
@@ -1491,13 +1495,13 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
transport_send_check_condition_and_sense(se_cmd,
se_cmd->scsi_sense_reason, 0);
target_put_sess_cmd(se_sess, se_cmd);
- return;
+ return 0;
}
rc = target_setup_cmd_from_cdb(se_cmd, cdb);
if (rc != 0) {
transport_generic_request_failure(se_cmd);
- return;
+ return 0;
}
/*
@@ -1507,7 +1511,7 @@ void target_submit_cmd(struct se_cmd *se_cmd, struct se_session *se_sess,
core_alua_check_nonop_delay(se_cmd);
transport_handle_cdb_direct(se_cmd);
- return;
+ return 0;
}
EXPORT_SYMBOL(target_submit_cmd);
diff --git a/drivers/target/tcm_fc/tfc_cmd.c b/drivers/target/tcm_fc/tfc_cmd.c
index 4ad58ac..b9cb500 100644
--- a/drivers/target/tcm_fc/tfc_cmd.c
+++ b/drivers/target/tcm_fc/tfc_cmd.c
@@ -543,9 +543,11 @@ static void ft_send_work(struct work_struct *work)
* Use a single se_cmd->cmd_kref as we expect to release se_cmd
* directly from ft_check_stop_free callback in response path.
*/
- target_submit_cmd(&cmd->se_cmd, cmd->sess->se_sess, fcp->fc_cdb,
- &cmd->ft_sense_buffer[0], scsilun_to_int(&fcp->fc_lun),
- ntohl(fcp->fc_dl), task_attr, data_dir, 0);
+ if (target_submit_cmd(&cmd->se_cmd, cmd->sess->se_sess, fcp->fc_cdb,
+ &cmd->ft_sense_buffer[0], scsilun_to_int(&fcp->fc_lun),
+ ntohl(fcp->fc_dl), task_attr, data_dir, 0))
+ goto err;
+
pr_debug("r_ctl %x alloc target_submit_cmd\n", fh->fh_r_ctl);
return;
diff --git a/drivers/usb/gadget/tcm_usb_gadget.c b/drivers/usb/gadget/tcm_usb_gadget.c
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);
}
static int bot_submit_command(struct f_uas *fu,
diff --git a/include/target/target_core_fabric.h b/include/target/target_core_fabric.h
index 815e064..69fb3cf 100644
--- a/include/target/target_core_fabric.h
+++ b/include/target/target_core_fabric.h
@@ -102,7 +102,7 @@ void transport_init_se_cmd(struct se_cmd *, struct target_core_fabric_ops *,
struct se_session *, u32, int, int, unsigned char *);
int transport_lookup_cmd_lun(struct se_cmd *, u32);
int target_setup_cmd_from_cdb(struct se_cmd *, unsigned char *);
-void target_submit_cmd(struct se_cmd *, struct se_session *, unsigned char *,
+int target_submit_cmd(struct se_cmd *, struct se_session *, unsigned char *,
unsigned char *, u32, u32, int, int, int);
int target_submit_tmr(struct se_cmd *se_cmd, struct se_session *se_sess,
unsigned char *sense, u32 unpacked_lun,
--
1.7.2.5
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] target: Allow for target_submit_cmd() returning errors
2012-07-17 23:37 [PATCH] target: Allow for target_submit_cmd() returning errors Nicholas A. Bellinger
@ 2012-07-18 0:05 ` Nicholas A. Bellinger
[not found] ` <1342569948.18004.541.camel-Y1+j5t8j3WgjMeEPmliV8E/sVC8ogwMJ@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Nicholas A. Bellinger @ 2012-07-18 0:05 UTC (permalink / raw)
To: target-devel
Cc: linux-scsi, Roland Dreier, Sebastian Andrzej Siewior,
Felipe Balbi, Andy Grover, linux-usb
On Tue, 2012-07-17 at 23:37 +0000, Nicholas A. Bellinger wrote:
> From: Roland Dreier <roland@purestorage.com>
>
> 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,
^ permalink raw reply related [flat|nested] 4+ messages in thread
* Re: [PATCH] target: Allow for target_submit_cmd() returning errors
[not found] ` <1342569948.18004.541.camel-Y1+j5t8j3WgjMeEPmliV8E/sVC8ogwMJ@public.gmane.org>
@ 2012-07-18 9:10 ` Sebastian Andrzej Siewior
[not found] ` <50067D85.5090900-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
0 siblings, 1 reply; 4+ messages in thread
From: Sebastian Andrzej Siewior @ 2012-07-18 9:10 UTC (permalink / raw)
To: Nicholas A. Bellinger
Cc: target-devel, linux-scsi, Roland Dreier, Felipe Balbi,
Andy Grover, linux-usb
On 07/18/2012 02:05 AM, Nicholas A. Bellinger wrote:
>> 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,
>
> 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..?)
For UAS the status/ sense URB is sent right away (without any data) and
this worked the last time I tested.
For BOT things are a little complicated. What I do is to send empty
data (to fill the data pipe) and send a bad status so the other side
learns that the transfer failed somehow. The sense information is lost.
What I should do is to queue this sense data for the next MODE_SENSE
request which will be coming soon. Right now WinXP repeats a failed
command thrice if MODE_SENSE returns no error. "This" is on my to-fix
list…
> 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.
The sense code should only be sent once and
transport_send_check_condition_and_sense() sets
SCF_SENT_CHECK_CONDITION and checks for it so doing it twice should not
do any harm right? But grep does not say when this flag gets removed.
>
> Thanks!
>
> --nab
Sebastian
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [PATCH] target: Allow for target_submit_cmd() returning errors
[not found] ` <50067D85.5090900-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
@ 2012-07-18 23:38 ` Nicholas A. Bellinger
0 siblings, 0 replies; 4+ messages in thread
From: Nicholas A. Bellinger @ 2012-07-18 23:38 UTC (permalink / raw)
To: Sebastian Andrzej Siewior
Cc: target-devel, linux-scsi, Roland Dreier, Felipe Balbi,
Andy Grover, linux-usb
On Wed, 2012-07-18 at 11:10 +0200, Sebastian Andrzej Siewior wrote:
> On 07/18/2012 02:05 AM, Nicholas A. Bellinger wrote:
> >> 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,
> >
> > 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..?)
>
> For UAS the status/ sense URB is sent right away (without any data) and
> this worked the last time I tested.
<nod>, thanks for the re-clarification on this.. ;)
> For BOT things are a little complicated. What I do is to send empty
> data (to fill the data pipe) and send a bad status so the other side
> learns that the transfer failed somehow. The sense information is lost.
> What I should do is to queue this sense data for the next MODE_SENSE
> request which will be coming soon. Right now WinXP repeats a failed
> command thrice if MODE_SENSE returns no error. "This" is on my to-fix
> list…
>
Ok, so I think the special case for usb-gadget where is able to call
transport_send_check_condition_and_sense() should be OK for the single
target_get_sess_cmd() -> target_submit_cmd() failure case..
This is not going to be safe for just about every other fabric AFAICT,
so I think we need a comment here to describe that fact..
> > 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.
>
> The sense code should only be sent once and
> transport_send_check_condition_and_sense() sets
> SCF_SENT_CHECK_CONDITION and checks for it so doing it twice should not
> do any harm right? But grep does not say when this flag gets removed.
>
Correct. The SCF_SENT_CHECK_CONDITION bit does not get removed ahead of
fabric descriptor release.
--
To unsubscribe from this list: send the line "unsubscribe linux-usb" in
the body of a message to majordomo-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2012-07-18 23:38 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-17 23:37 [PATCH] target: Allow for target_submit_cmd() returning errors Nicholas A. Bellinger
2012-07-18 0:05 ` Nicholas A. Bellinger
[not found] ` <1342569948.18004.541.camel-Y1+j5t8j3WgjMeEPmliV8E/sVC8ogwMJ@public.gmane.org>
2012-07-18 9:10 ` Sebastian Andrzej Siewior
[not found] ` <50067D85.5090900-hfZtesqFncYOwBW4kG4KsQ@public.gmane.org>
2012-07-18 23:38 ` Nicholas A. Bellinger
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.