Re: [PATCH] target: Allow for target_submit_cmd() returning errors
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. , 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-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] target: Allow for target_submit_cmd() returning errors
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-scsi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] target: Allow for target_submit_cmd() returning errors
On Tue, 2012-07-17 at 23:37 +, Nicholas A. Bellinger wrote: > From: Roland Dreier > > 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) > + 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); - -
[PATCH] target: Allow for target_submit_cmd() returning errors
From: Roland Dreier 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 Cc: Arun Easi Cc: Chris Boot Cc: Stefan Richter Cc: Mark Rustad Cc: Sebastian Andrzej Siewior Cc: Felipe Balbi Cc: Andy Grover Signed-off-by: Roland Dreier Signed-off-by: Nicholas Bellinger --- 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