| Message ID | 1449535747-2850-12-git-send-email-himanshu.madhani@qlogic.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c > index 842fcca..2e9c194 100644 > --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c > +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c > @@ -617,6 +617,26 @@ static int tcm_qla2xxx_queue_status(struct se_cmd *se_cmd) > struct qla_tgt_cmd, se_cmd); > int xmit_type = QLA_TGT_XMIT_STATUS; > > + if (se_cmd->transport_state & CMD_T_ABORTED) { > + /* For TCM TAS support n kernel >= 3.15: > + * This cmd is attempting to respond with "Task Aborted Status". > + */ > + if (cmd->aborted) { > + return 0; > + } else if ((cmd->state == QLA_TGT_STATE_NEED_DATA) && > + cmd->cmd_sent_to_fw) { > + qlt_abort_cmd(cmd); > + return 0; > + } else if (cmd->state == QLA_TGT_STATE_PROCESSED) { > + if (cmd->cmd_sent_to_fw) { > + qlt_abort_cmd(cmd); > + return 0; > + } else { /* about to be free */ > + return 0; > + } > + } > + } > + This is really something that should be explicitly communicated from the core instead of having to second guess it. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 12/08/2015 01:48 AM, Himanshu Madhani wrote: > From: Quinn Tran <quinn.tran@qlogic.com> > > For kernel 3.15 and newer with TCM API change, add detection > for TCM support of TAS. Instead of default command terminate > for LUN/TARGET reset error handling, allow SCSI status to go > out if we know sequece Initiative is own by FW (cmd_sent_to_fw=0) > > Signed-off-by: Quinn Tran <quinn.tran@qlogic.com> > Signed-off-by: Himanshu Madhani <himanshu.madhani@qlogic.com> > --- > drivers/scsi/qla2xxx/qla_target.c | 36 +++++++++++++++++------------------- > drivers/scsi/qla2xxx/tcm_qla2xxx.c | 20 ++++++++++++++++++++ > 2 files changed, 37 insertions(+), 19 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c > index 4d42b79..5fca846 100644 > --- a/drivers/scsi/qla2xxx/qla_target.c > +++ b/drivers/scsi/qla2xxx/qla_target.c > @@ -3239,36 +3239,34 @@ int qlt_abort_cmd(struct qla_tgt_cmd *cmd) > struct qla_tgt *tgt = cmd->tgt; > struct scsi_qla_host *vha = tgt->vha; > struct se_cmd *se_cmd = &cmd->se_cmd; > - unsigned long flags,refcount; > + unsigned long flags, refcount; > > ql_dbg(ql_dbg_tgt_mgt, vha, 0xf014, > "qla_target(%d): terminating exchange for aborted cmd=%p " > "(se_cmd=%p, tag=%llu)", vha->vp_idx, cmd, &cmd->se_cmd, > se_cmd->tag); > > - spin_lock_irqsave(&cmd->cmd_lock, flags); > - if (cmd->aborted) { > - spin_unlock_irqrestore(&cmd->cmd_lock, flags); > + spin_lock_irqsave(&cmd->cmd_lock, flags); > > - /* It's normal to see 2 calls in this path: > - * 1) XFER Rdy completion + CMD_T_ABORT > - * 2) TCM TMR - drain_state_list > - */ > - refcount = atomic_read(&cmd->se_cmd.cmd_kref.refcount); > - ql_dbg(ql_dbg_tgt_mgt, vha, 0xffff, > - "multiple abort. %p refcount %lx" > - "transport_state %x, t_state %x, se_cmd_flags %x \n", > - cmd, refcount,cmd->se_cmd.transport_state, > - cmd->se_cmd.t_state,cmd->se_cmd.se_cmd_flags); > + if (cmd->aborted) { > + spin_unlock_irqrestore(&cmd->cmd_lock, flags); > + /* It's normal to see 2 calls in this path: > + * 1) XFER Rdy completion + CMD_T_ABORT > + * 2) TCM TMR - drain_state_list > + */ > + refcount = atomic_read(&cmd->se_cmd.cmd_kref.refcount); > + ql_dbg(ql_dbg_tgt_mgt, vha, 0xffff, > + "multiple abort. %p refcount %lx" > + "transport_state %x, t_state %x, se_cmd_flags %x \n", > + cmd, refcount, cmd->se_cmd.transport_state, > + cmd->se_cmd.t_state, cmd->se_cmd.se_cmd_flags); > > - return EIO; > - } > + return EIO; > + } > > cmd->aborted = 1; > cmd->cmd_flags |= BIT_6; > - spin_unlock_irqrestore(&cmd->cmd_lock, flags); > - > - qlt_send_term_exchange(vha, cmd, &cmd->atio, 0, 1); > + spin_unlock_irqrestore(&cmd->cmd_lock, flags); > > return 0; > } > diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c > index 842fcca..2e9c194 100644 > --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c > +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c > @@ -617,6 +617,26 @@ static int tcm_qla2xxx_queue_status(struct se_cmd *se_cmd) > struct qla_tgt_cmd, se_cmd); > int xmit_type = QLA_TGT_XMIT_STATUS; > > + if (se_cmd->transport_state & CMD_T_ABORTED) { > + /* For TCM TAS support n kernel >= 3.15: > + * This cmd is attempting to respond with "Task Aborted Status". > + */ > + if (cmd->aborted) { > + return 0; > + } else if ((cmd->state == QLA_TGT_STATE_NEED_DATA) && > + cmd->cmd_sent_to_fw) { > + qlt_abort_cmd(cmd); > + return 0; > + } else if (cmd->state == QLA_TGT_STATE_PROCESSED) { > + if (cmd->cmd_sent_to_fw) { > + qlt_abort_cmd(cmd); > + return 0; > + } else { /* about to be free */ > + return 0; > + } > + } > + } > + > cmd->bufflen = se_cmd->data_length; > cmd->sg = NULL; > cmd->sg_cnt = 0; > Reviewed-by: Hannes Reinecke <hare@suse.com> Cheers, Hannes
On 12/7/15, 6:48 PM, "target-devel-owner@vger.kernel.org on behalf of Christoph Hellwig" <target-devel-owner@vger.kernel.org on behalf of hch@infradead.org> wrote: >> diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c >>b/drivers/scsi/qla2xxx/tcm_qla2xxx.c >> index 842fcca..2e9c194 100644 >> --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c >> +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c >> @@ -617,6 +617,26 @@ static int tcm_qla2xxx_queue_status(struct se_cmd >>*se_cmd) >> struct qla_tgt_cmd, se_cmd); >> int xmit_type = QLA_TGT_XMIT_STATUS; >> >> + if (se_cmd->transport_state & CMD_T_ABORTED) { >> + /* For TCM TAS support n kernel >= 3.15: >> + * This cmd is attempting to respond with "Task Aborted Status". >> + */ >> + if (cmd->aborted) { >> + return 0; >> + } else if ((cmd->state == QLA_TGT_STATE_NEED_DATA) && >> + cmd->cmd_sent_to_fw) { >> + qlt_abort_cmd(cmd); >> + return 0; >> + } else if (cmd->state == QLA_TGT_STATE_PROCESSED) { >> + if (cmd->cmd_sent_to_fw) { >> + qlt_abort_cmd(cmd); >> + return 0; >> + } else { /* about to be free */ >> + return 0; >> + } >> + } >> + } >> + > >This is really something that should be explicitly communicated >from the core instead of having to second guess it. QT> The extra protection of the code here is to reduce erroneous error message and interaction error with our firmware. I think communicating back to the core at this stage might add undue complication. It¹s best to allow the initiator to re-drive the command at this point.
On Wed, Dec 09, 2015 at 08:24:57PM +0000, Quinn Tran wrote: > >> + if (se_cmd->transport_state & CMD_T_ABORTED) { > >> + /* For TCM TAS support n kernel >= 3.15: > >> + * This cmd is attempting to respond with "Task Aborted Status". > >> + */ > >> + if (cmd->aborted) { > >> + return 0; > >> + } else if ((cmd->state == QLA_TGT_STATE_NEED_DATA) && > >> + cmd->cmd_sent_to_fw) { > >> + qlt_abort_cmd(cmd); > >> + return 0; > >> + } else if (cmd->state == QLA_TGT_STATE_PROCESSED) { > >> + if (cmd->cmd_sent_to_fw) { > >> + qlt_abort_cmd(cmd); > >> + return 0; > >> + } else { /* about to be free */ > >> + return 0; > >> + } > >> + } > >> + } > >> + > > > >This is really something that should be explicitly communicated > >from the core instead of having to second guess it. > > QT> The extra protection of the code here is to reduce erroneous error > message and interaction error with our firmware. I think communicating > back to the core at this stage might add undue complication. It?s best to > allow the initiator to re-drive the command at this point. I agree with that statement, just not how it's done. Having parallel command state machines in the driver and core isn't something that's maintainable. We need to attack the root cause instead of trying to hack around it in drivers. -- To unsubscribe from this list: send the line "unsubscribe linux-scsi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Christoph, Thanks for reviewing. I¹ll withdraw this patch. Will rework with new code and submit at a later time. Regards, Quinn Tran On 12/14/15, 2:37 AM, "Christoph Hellwig" <hch@infradead.org> wrote: >On Wed, Dec 09, 2015 at 08:24:57PM +0000, Quinn Tran wrote: >> >> + if (se_cmd->transport_state & CMD_T_ABORTED) { >> >> + /* For TCM TAS support n kernel >= 3.15: >> >> + * This cmd is attempting to respond with "Task Aborted Status". >> >> + */ >> >> + if (cmd->aborted) { >> >> + return 0; >> >> + } else if ((cmd->state == QLA_TGT_STATE_NEED_DATA) && >> >> + cmd->cmd_sent_to_fw) { >> >> + qlt_abort_cmd(cmd); >> >> + return 0; >> >> + } else if (cmd->state == QLA_TGT_STATE_PROCESSED) { >> >> + if (cmd->cmd_sent_to_fw) { >> >> + qlt_abort_cmd(cmd); >> >> + return 0; >> >> + } else { /* about to be free */ >> >> + return 0; >> >> + } >> >> + } >> >> + } >> >> + >> > >> >This is really something that should be explicitly communicated >> >from the core instead of having to second guess it. >> >> QT> The extra protection of the code here is to reduce erroneous error >> message and interaction error with our firmware. I think communicating >> back to the core at this stage might add undue complication. It?s best >>to >> allow the initiator to re-drive the command at this point. > >I agree with that statement, just not how it's done. Having parallel >command state machines in the driver and core isn't something that's >maintainable. We need to attack the root cause instead of trying >to hack around it in drivers.
diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c index 4d42b79..5fca846 100644 --- a/drivers/scsi/qla2xxx/qla_target.c +++ b/drivers/scsi/qla2xxx/qla_target.c @@ -3239,36 +3239,34 @@ int qlt_abort_cmd(struct qla_tgt_cmd *cmd) struct qla_tgt *tgt = cmd->tgt; struct scsi_qla_host *vha = tgt->vha; struct se_cmd *se_cmd = &cmd->se_cmd; - unsigned long flags,refcount; + unsigned long flags, refcount; ql_dbg(ql_dbg_tgt_mgt, vha, 0xf014, "qla_target(%d): terminating exchange for aborted cmd=%p " "(se_cmd=%p, tag=%llu)", vha->vp_idx, cmd, &cmd->se_cmd, se_cmd->tag); - spin_lock_irqsave(&cmd->cmd_lock, flags); - if (cmd->aborted) { - spin_unlock_irqrestore(&cmd->cmd_lock, flags); + spin_lock_irqsave(&cmd->cmd_lock, flags); - /* It's normal to see 2 calls in this path: - * 1) XFER Rdy completion + CMD_T_ABORT - * 2) TCM TMR - drain_state_list - */ - refcount = atomic_read(&cmd->se_cmd.cmd_kref.refcount); - ql_dbg(ql_dbg_tgt_mgt, vha, 0xffff, - "multiple abort. %p refcount %lx" - "transport_state %x, t_state %x, se_cmd_flags %x \n", - cmd, refcount,cmd->se_cmd.transport_state, - cmd->se_cmd.t_state,cmd->se_cmd.se_cmd_flags); + if (cmd->aborted) { + spin_unlock_irqrestore(&cmd->cmd_lock, flags); + /* It's normal to see 2 calls in this path: + * 1) XFER Rdy completion + CMD_T_ABORT + * 2) TCM TMR - drain_state_list + */ + refcount = atomic_read(&cmd->se_cmd.cmd_kref.refcount); + ql_dbg(ql_dbg_tgt_mgt, vha, 0xffff, + "multiple abort. %p refcount %lx" + "transport_state %x, t_state %x, se_cmd_flags %x \n", + cmd, refcount, cmd->se_cmd.transport_state, + cmd->se_cmd.t_state, cmd->se_cmd.se_cmd_flags); - return EIO; - } + return EIO; + } cmd->aborted = 1; cmd->cmd_flags |= BIT_6; - spin_unlock_irqrestore(&cmd->cmd_lock, flags); - - qlt_send_term_exchange(vha, cmd, &cmd->atio, 0, 1); + spin_unlock_irqrestore(&cmd->cmd_lock, flags); return 0; } diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c index 842fcca..2e9c194 100644 --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c @@ -617,6 +617,26 @@ static int tcm_qla2xxx_queue_status(struct se_cmd *se_cmd) struct qla_tgt_cmd, se_cmd); int xmit_type = QLA_TGT_XMIT_STATUS; + if (se_cmd->transport_state & CMD_T_ABORTED) { + /* For TCM TAS support n kernel >= 3.15: + * This cmd is attempting to respond with "Task Aborted Status". + */ + if (cmd->aborted) { + return 0; + } else if ((cmd->state == QLA_TGT_STATE_NEED_DATA) && + cmd->cmd_sent_to_fw) { + qlt_abort_cmd(cmd); + return 0; + } else if (cmd->state == QLA_TGT_STATE_PROCESSED) { + if (cmd->cmd_sent_to_fw) { + qlt_abort_cmd(cmd); + return 0; + } else { /* about to be free */ + return 0; + } + } + } + cmd->bufflen = se_cmd->data_length; cmd->sg = NULL; cmd->sg_cnt = 0;