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