| Message ID | 20210316035655.2835-2-bvanassche@acm.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | qla2xxx patches for kernel v5.12 and v5.13 | expand |
On Mon, Mar 15, 2021 at 08:56:49PM -0700, Bart Van Assche wrote: > Calling vha->hw->tgt.tgt_ops->free_cmd() from qlt_xmit_response() is wrong > since the command for which a response is sent must remain valid until the > SCSI target core calls .release_cmd(). The commit message from 0dcec41acb85 ("scsi: qla2xxx: Make sure that aborted commands are freed") says 'avoids that the code for removing a session hangs due to commands that do not make progress'. As this patch reverts the change, is the problem mentioned gone? Did some other change fix it? Just wondering.
Hi Bart, On Tue, Mar 16, 2021 at 01:36:34PM -0700, Bart Van Assche wrote: > Commit 0dcec41acb85 was the result of source reading. The changes made > by this patch in qlt_xmit_response() are wrong and may lead to a kernel > crash. Since triggering the other code paths that are modified by that > patch, I'd like to revert that patch in its entirety. Thanks for the explanation. I was hoping that you have a fix for something I see in our customer bug reports :) Reviewed-by: Daniel Wagner <dwagner@suse.de>
> On Mar 16, 2021, at 3:28 PM, Bart Van Assche <bvanassche@acm.org> wrote: > > On 3/16/21 9:25 AM, Himanshu Madhani wrote: >> Curious …. What triggered this revert? Can you share your motivation for this revert. > > Hi Himanshu, > > It has been observed that the following scenario triggers a kernel crash: > - qlt_xmit_response() calls qlt_check_reserve_free_req(). > - qlt_check_reserve_free_req() returns -EAGAIN. > - qlt_xmit_response() calls vha->hw->tgt.tgt_ops->free_cmd(cmd). > - transport_handle_queue_full() tries to retransmit the response. > > I will add this information to the patch description. > > Bart. Thanks, once you add more information to the patch, you can add my R-B — Himanshu Madhani Oracle Linux Engineering
diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c index d6366a46283e..5e8b2653e134 100644 --- a/drivers/scsi/qla2xxx/qla_target.c +++ b/drivers/scsi/qla2xxx/qla_target.c @@ -3222,8 +3222,7 @@ int qlt_xmit_response(struct qla_tgt_cmd *cmd, int xmit_type, if (!qpair->fw_started || (cmd->reset_count != qpair->chip_reset) || (cmd->sess && cmd->sess->deleted)) { cmd->state = QLA_TGT_STATE_PROCESSED; - res = 0; - goto free; + return 0; } ql_dbg_qp(ql_dbg_tgt, qpair, 0xe018, @@ -3234,8 +3233,9 @@ int qlt_xmit_response(struct qla_tgt_cmd *cmd, int xmit_type, res = qlt_pre_xmit_response(cmd, &prm, xmit_type, scsi_status, &full_req_cnt); - if (unlikely(res != 0)) - goto free; + if (unlikely(res != 0)) { + return res; + } spin_lock_irqsave(qpair->qp_lock_ptr, flags); @@ -3255,8 +3255,7 @@ int qlt_xmit_response(struct qla_tgt_cmd *cmd, int xmit_type, vha->flags.online, qla2x00_reset_active(vha), cmd->reset_count, qpair->chip_reset); spin_unlock_irqrestore(qpair->qp_lock_ptr, flags); - res = 0; - goto free; + return 0; } /* Does F/W have an IOCBs for this request */ @@ -3359,8 +3358,6 @@ int qlt_xmit_response(struct qla_tgt_cmd *cmd, int xmit_type, qlt_unmap_sg(vha, cmd); spin_unlock_irqrestore(qpair->qp_lock_ptr, flags); -free: - vha->hw->tgt.tgt_ops->free_cmd(cmd); return res; } EXPORT_SYMBOL(qlt_xmit_response); diff --git a/drivers/scsi/qla2xxx/tcm_qla2xxx.c b/drivers/scsi/qla2xxx/tcm_qla2xxx.c index 30959f8da065..15650a0bde09 100644 --- a/drivers/scsi/qla2xxx/tcm_qla2xxx.c +++ b/drivers/scsi/qla2xxx/tcm_qla2xxx.c @@ -653,7 +653,6 @@ static int tcm_qla2xxx_queue_data_in(struct se_cmd *se_cmd) { struct qla_tgt_cmd *cmd = container_of(se_cmd, struct qla_tgt_cmd, se_cmd); - struct scsi_qla_host *vha = cmd->vha; if (cmd->aborted) { /* Cmd can loop during Q-full. tcm_qla2xxx_aborted_task @@ -666,7 +665,6 @@ static int tcm_qla2xxx_queue_data_in(struct se_cmd *se_cmd) cmd->se_cmd.transport_state, cmd->se_cmd.t_state, cmd->se_cmd.se_cmd_flags); - vha->hw->tgt.tgt_ops->free_cmd(cmd); return 0; } @@ -694,7 +692,6 @@ static int tcm_qla2xxx_queue_status(struct se_cmd *se_cmd) { struct qla_tgt_cmd *cmd = container_of(se_cmd, struct qla_tgt_cmd, se_cmd); - struct scsi_qla_host *vha = cmd->vha; int xmit_type = QLA_TGT_XMIT_STATUS; if (cmd->aborted) { @@ -708,7 +705,6 @@ static int tcm_qla2xxx_queue_status(struct se_cmd *se_cmd) cmd, kref_read(&cmd->se_cmd.cmd_kref), cmd->se_cmd.transport_state, cmd->se_cmd.t_state, cmd->se_cmd.se_cmd_flags); - vha->hw->tgt.tgt_ops->free_cmd(cmd); return 0; } cmd->bufflen = se_cmd->data_length;
Calling vha->hw->tgt.tgt_ops->free_cmd() from qlt_xmit_response() is wrong since the command for which a response is sent must remain valid until the SCSI target core calls .release_cmd(). Fixes: 0dcec41acb85 ("scsi: qla2xxx: Make sure that aborted commands are freed") Cc: Quinn Tran <qutran@marvell.com> Cc: Mike Christie <michael.christie@oracle.com> Cc: Himanshu Madhani <himanshu.madhani@oracle.com> Cc: Daniel Wagner <dwagner@suse.de> Signed-off-by: Bart Van Assche <bvanassche@acm.org> --- drivers/scsi/qla2xxx/qla_target.c | 13 +++++-------- drivers/scsi/qla2xxx/tcm_qla2xxx.c | 4 ---- 2 files changed, 5 insertions(+), 12 deletions(-)