| Message ID | 20180320213614.3oymhl7nenvh6vke@xylophone.i.decadent.org.uk (mailing list archive) |
|---|---|
| State | Accepted |
| Headers | show |
Hi Ben, > On Mar 20, 2018, at 2:36 PM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > > qla2x00_init_timer() calls add_timer() on the iocb timeout timer, > which means the timeout function pointer and any data that the > function depends on must be initialised beforehand. > > Move this initialisation before each call to qla2x00_init_timer(). In > some cases qla2x00_init_timer() initialises a completion structure > needed by the timeout function, so move the call to add_timer() after > that. > > Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> > --- > Changes from v1: > > Rebased to remove textual dependency on a patch I haven't yet sent > (oops). > > Ben. > > drivers/scsi/qla2xxx/qla_gs.c | 12 +++++++----- > drivers/scsi/qla2xxx/qla_init.c | 37 +++++++++++++++++++++++-------------- > drivers/scsi/qla2xxx/qla_inline.h | 2 +- > drivers/scsi/qla2xxx/qla_iocb.c | 8 +++++--- > drivers/scsi/qla2xxx/qla_mbx.c | 8 ++++---- > drivers/scsi/qla2xxx/qla_mid.c | 2 +- > drivers/scsi/qla2xxx/qla_mr.c | 5 +++-- > drivers/scsi/qla2xxx/qla_target.c | 2 +- > 8 files changed, 45 insertions(+), 31 deletions(-) > > diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c > index 403fa096f8c8..fcde5ea203c0 100644 > --- a/drivers/scsi/qla2xxx/qla_gs.c > +++ b/drivers/scsi/qla2xxx/qla_gs.c > @@ -3796,6 +3796,7 @@ int qla24xx_async_gffid(scsi_qla_host_t *vha, fc_port_t *fcport) > sp->gen1 = fcport->rscn_gen; > sp->gen2 = fcport->login_gen; > > + sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; > qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > > /* CT_IU preamble */ > @@ -3814,7 +3815,6 @@ int qla24xx_async_gffid(scsi_qla_host_t *vha, fc_port_t *fcport) > sp->u.iocb_cmd.u.ctarg.rsp_size = GFF_ID_RSP_SIZE; > sp->u.iocb_cmd.u.ctarg.nport_handle = NPH_SNS; > > - sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; > sp->done = qla24xx_async_gffid_sp_done; > > rval = qla2x00_start_sp(sp); > @@ -4121,6 +4121,8 @@ static int qla24xx_async_gnnft(scsi_qla_host_t *vha, struct srb *sp, > sp->name = "gnnft"; > sp->gen1 = vha->hw->base_qpair->chip_reset; > sp->gen2 = fc4_type; > + > + sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; > qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > > memset(sp->u.iocb_cmd.u.ctarg.rsp, 0, sp->u.iocb_cmd.u.ctarg.rsp_size); > @@ -4137,7 +4139,6 @@ static int qla24xx_async_gnnft(scsi_qla_host_t *vha, struct srb *sp, > sp->u.iocb_cmd.u.ctarg.req_size = GNN_FT_REQ_SIZE; > sp->u.iocb_cmd.u.ctarg.nport_handle = NPH_SNS; > > - sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; > sp->done = qla2x00_async_gpnft_gnnft_sp_done; > > rval = qla2x00_start_sp(sp); > @@ -4210,6 +4211,8 @@ int qla24xx_async_gpnft(scsi_qla_host_t *vha, u8 fc4_type) > sp->name = "gpnft"; > sp->gen1 = vha->hw->base_qpair->chip_reset; > sp->gen2 = fc4_type; > + > + sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; > qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > > sp->u.iocb_cmd.u.ctarg.req = dma_zalloc_coherent(&vha->hw->pdev->dev, > @@ -4248,7 +4251,6 @@ int qla24xx_async_gpnft(scsi_qla_host_t *vha, u8 fc4_type) > sp->u.iocb_cmd.u.ctarg.rsp_size = rspsz; > sp->u.iocb_cmd.u.ctarg.nport_handle = NPH_SNS; > > - sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; > sp->done = qla2x00_async_gpnft_gnnft_sp_done; > > rval = qla2x00_start_sp(sp); > @@ -4356,6 +4358,7 @@ int qla24xx_async_gnnid(scsi_qla_host_t *vha, fc_port_t *fcport) > sp->gen1 = fcport->rscn_gen; > sp->gen2 = fcport->login_gen; > > + sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; > qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > > /* CT_IU preamble */ > @@ -4377,7 +4380,6 @@ int qla24xx_async_gnnid(scsi_qla_host_t *vha, fc_port_t *fcport) > sp->u.iocb_cmd.u.ctarg.rsp_size = GNN_ID_RSP_SIZE; > sp->u.iocb_cmd.u.ctarg.nport_handle = NPH_SNS; > > - sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; > sp->done = qla2x00_async_gnnid_sp_done; > > rval = qla2x00_start_sp(sp); > @@ -4493,6 +4495,7 @@ int qla24xx_async_gfpnid(scsi_qla_host_t *vha, fc_port_t *fcport) > sp->gen1 = fcport->rscn_gen; > sp->gen2 = fcport->login_gen; > > + sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; > qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > > /* CT_IU preamble */ > @@ -4514,7 +4517,6 @@ int qla24xx_async_gfpnid(scsi_qla_host_t *vha, fc_port_t *fcport) > sp->u.iocb_cmd.u.ctarg.rsp_size = GFPN_ID_RSP_SIZE; > sp->u.iocb_cmd.u.ctarg.nport_handle = NPH_SNS; > > - sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; > sp->done = qla2x00_async_gfpnid_sp_done; > > rval = qla2x00_start_sp(sp); > diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c > index b0aa8cc96f0f..45319119606a 100644 > --- a/drivers/scsi/qla2xxx/qla_init.c > +++ b/drivers/scsi/qla2xxx/qla_init.c > @@ -183,10 +183,11 @@ qla2x00_async_login(struct scsi_qla_host *vha, fc_port_t *fcport, > sp->name = "login"; > sp->gen1 = fcport->rscn_gen; > sp->gen2 = fcport->login_gen; > - qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > > lio = &sp->u.iocb_cmd; > lio->timeout = qla2x00_async_iocb_timeout; > + qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > + > sp->done = qla2x00_async_login_sp_done; > lio->u.logio.flags |= SRB_LOGIN_COND_PLOGI; > > @@ -245,10 +246,11 @@ qla2x00_async_logout(struct scsi_qla_host *vha, fc_port_t *fcport) > > sp->type = SRB_LOGOUT_CMD; > sp->name = "logout"; > - qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > > lio = &sp->u.iocb_cmd; > lio->timeout = qla2x00_async_iocb_timeout; > + qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > + > sp->done = qla2x00_async_logout_sp_done; > rval = qla2x00_start_sp(sp); > if (rval != QLA_SUCCESS) > @@ -307,10 +309,11 @@ qla2x00_async_prlo(struct scsi_qla_host *vha, fc_port_t *fcport) > > sp->type = SRB_PRLO_CMD; > sp->name = "prlo"; > - qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > > lio = &sp->u.iocb_cmd; > lio->timeout = qla2x00_async_iocb_timeout; > + qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > + > sp->done = qla2x00_async_prlo_sp_done; > rval = qla2x00_start_sp(sp); > if (rval != QLA_SUCCESS) > @@ -412,10 +415,11 @@ qla2x00_async_adisc(struct scsi_qla_host *vha, fc_port_t *fcport, > > sp->type = SRB_ADISC_CMD; > sp->name = "adisc"; > - qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > > lio = &sp->u.iocb_cmd; > lio->timeout = qla2x00_async_iocb_timeout; > + qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > + > sp->done = qla2x00_async_adisc_sp_done; > if (data[1] & QLA_LOGIO_LOGIN_RETRIED) > lio->u.logio.flags |= SRB_LOGIN_RETRIED; > @@ -745,6 +749,8 @@ int qla24xx_async_gnl(struct scsi_qla_host *vha, fc_port_t *fcport) > sp->gen1 = fcport->rscn_gen; > sp->gen2 = fcport->login_gen; > > + mbx = &sp->u.iocb_cmd; > + mbx->timeout = qla2x00_async_iocb_timeout; > qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha)+2); > > mb = sp->u.iocb_cmd.u.mbx.out_mb; > @@ -757,9 +763,6 @@ int qla24xx_async_gnl(struct scsi_qla_host *vha, fc_port_t *fcport) > mb[8] = vha->gnl.size; > mb[9] = vha->vp_idx; > > - mbx = &sp->u.iocb_cmd; > - mbx->timeout = qla2x00_async_iocb_timeout; > - > sp->done = qla24xx_async_gnl_sp_done; > > rval = qla2x00_start_sp(sp); > @@ -888,10 +891,11 @@ qla24xx_async_prli(struct scsi_qla_host *vha, fc_port_t *fcport) > > sp->type = SRB_PRLI_CMD; > sp->name = "prli"; > - qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > > lio = &sp->u.iocb_cmd; > lio->timeout = qla2x00_async_iocb_timeout; > + qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > + > sp->done = qla2x00_async_prli_sp_done; > lio->u.logio.flags = 0; > > @@ -956,6 +960,9 @@ int qla24xx_async_gpdb(struct scsi_qla_host *vha, fc_port_t *fcport, u8 opt) > sp->name = "gpdb"; > sp->gen1 = fcport->rscn_gen; > sp->gen2 = fcport->login_gen; > + > + mbx = &sp->u.iocb_cmd; > + mbx->timeout = qla2x00_async_iocb_timeout; > qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > > pd = dma_pool_zalloc(ha->s_dma_pool, GFP_KERNEL, &pd_dma); > @@ -975,8 +982,6 @@ int qla24xx_async_gpdb(struct scsi_qla_host *vha, fc_port_t *fcport, u8 opt) > mb[9] = vha->vp_idx; > mb[10] = opt; > > - mbx = &sp->u.iocb_cmd; > - mbx->timeout = qla2x00_async_iocb_timeout; > mbx->u.mbx.in = (void *)pd; > mbx->u.mbx.in_dma = pd_dma; > > @@ -1486,13 +1491,15 @@ qla2x00_async_tm_cmd(fc_port_t *fcport, uint32_t flags, uint32_t lun, > tm_iocb = &sp->u.iocb_cmd; > sp->type = SRB_TM_CMD; > sp->name = "tmf"; > + > + tm_iocb->timeout = qla2x00_tmf_iocb_timeout; > + init_completion(&tm_iocb->u.tmf.comp); > qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha)); > + > tm_iocb->u.tmf.flags = flags; > tm_iocb->u.tmf.lun = lun; > tm_iocb->u.tmf.data = tag; > sp->done = qla2x00_tmf_sp_done; > - tm_iocb->timeout = qla2x00_tmf_iocb_timeout; > - init_completion(&tm_iocb->u.tmf.comp); > > rval = qla2x00_start_sp(sp); > if (rval != QLA_SUCCESS) > @@ -1566,7 +1573,11 @@ qla24xx_async_abort_cmd(srb_t *cmd_sp) > abt_iocb = &sp->u.iocb_cmd; > sp->type = SRB_ABT_CMD; > sp->name = "abort"; > + > + abt_iocb->timeout = qla24xx_abort_iocb_timeout; > + init_completion(&abt_iocb->u.abt.comp); > qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha)); > + > abt_iocb->u.abt.cmd_hndl = cmd_sp->handle; > > if (vha->flags.qpairs_available && cmd_sp->qpair) > @@ -1576,8 +1587,6 @@ qla24xx_async_abort_cmd(srb_t *cmd_sp) > abt_iocb->u.abt.req_que_no = cpu_to_le16(vha->req->id); > > sp->done = qla24xx_abort_sp_done; > - abt_iocb->timeout = qla24xx_abort_iocb_timeout; > - init_completion(&abt_iocb->u.abt.comp); > > rval = qla2x00_start_sp(sp); > if (rval != QLA_SUCCESS) > diff --git a/drivers/scsi/qla2xxx/qla_inline.h b/drivers/scsi/qla2xxx/qla_inline.h > index 4d32426393c7..06c4a843e2ad 100644 > --- a/drivers/scsi/qla2xxx/qla_inline.h > +++ b/drivers/scsi/qla2xxx/qla_inline.h > @@ -271,13 +271,13 @@ qla2x00_init_timer(srb_t *sp, unsigned long tmo) > { > timer_setup(&sp->u.iocb_cmd.timer, qla2x00_sp_timeout, 0); > sp->u.iocb_cmd.timer.expires = jiffies + tmo * HZ; > - add_timer(&sp->u.iocb_cmd.timer); > sp->free = qla2x00_sp_free; > init_completion(&sp->comp); > if (IS_QLAFX00(sp->vha->hw) && (sp->type == SRB_FXIOCB_DCMD)) > init_completion(&sp->u.iocb_cmd.u.fxiocb.fxiocb_comp); > if (sp->type == SRB_ELS_DCMD) > init_completion(&sp->u.iocb_cmd.u.els_logo.comp); > + add_timer(&sp->u.iocb_cmd.timer); > } > > static inline int > diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c > index 8d00d559bd26..6a719c1f8af5 100644 > --- a/drivers/scsi/qla2xxx/qla_iocb.c > +++ b/drivers/scsi/qla2xxx/qla_iocb.c > @@ -2458,8 +2458,8 @@ qla24xx_els_dcmd_iocb(scsi_qla_host_t *vha, int els_opcode, > sp->type = SRB_ELS_DCMD; > sp->name = "ELS_DCMD"; > sp->fcport = fcport; > - qla2x00_init_timer(sp, ELS_DCMD_TIMEOUT); > elsio->timeout = qla2x00_els_dcmd_iocb_timeout; > + qla2x00_init_timer(sp, ELS_DCMD_TIMEOUT); > sp->done = qla2x00_els_dcmd_sp_done; > sp->free = qla2x00_els_dcmd_sp_free; > > @@ -2656,8 +2656,11 @@ qla24xx_els_dcmd2_iocb(scsi_qla_host_t *vha, int els_opcode, > sp->type = SRB_ELS_DCMD; > sp->name = "ELS_DCMD"; > sp->fcport = fcport; > - qla2x00_init_timer(sp, ELS_DCMD_TIMEOUT); > + > elsio->timeout = qla2x00_els_dcmd2_iocb_timeout; > + init_completion(&elsio->u.els_plogi.comp); > + qla2x00_init_timer(sp, ELS_DCMD_TIMEOUT); > + > sp->done = qla2x00_els_dcmd2_sp_done; > sp->free = qla2x00_els_dcmd2_sp_free; > > @@ -2694,7 +2697,6 @@ qla24xx_els_dcmd2_iocb(scsi_qla_host_t *vha, int els_opcode, > ql_dump_buffer(ql_dbg_io + ql_dbg_buffer, vha, 0x0109, > (uint8_t *)elsio->u.els_plogi.els_plogi_pyld, 0x70); > > - init_completion(&elsio->u.els_plogi.comp); > rval = qla2x00_start_sp(sp); > if (rval != QLA_SUCCESS) { > rval = QLA_FUNCTION_FAILED; > diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c > index 7397aeddd96c..4984570e210b 100644 > --- a/drivers/scsi/qla2xxx/qla_mbx.c > +++ b/drivers/scsi/qla2xxx/qla_mbx.c > @@ -6006,14 +6006,14 @@ int qla24xx_send_mb_cmd(struct scsi_qla_host *vha, mbx_cmd_t *mcp) > sp->type = SRB_MB_IOCB; > sp->name = mb_to_str(mcp->mb[0]); > > - qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > - > - memcpy(sp->u.iocb_cmd.u.mbx.out_mb, mcp->mb, SIZEOF_IOCB_MB_REG); > - > c = &sp->u.iocb_cmd; > c->timeout = qla2x00_async_iocb_timeout; > init_completion(&c->u.mbx.comp); > > + qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > + > + memcpy(sp->u.iocb_cmd.u.mbx.out_mb, mcp->mb, SIZEOF_IOCB_MB_REG); > + > sp->done = qla2x00_async_mb_sp_done; > > rval = qla2x00_start_sp(sp); > diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c > index e965b16f21e3..07ed3aaedb4a 100644 > --- a/drivers/scsi/qla2xxx/qla_mid.c > +++ b/drivers/scsi/qla2xxx/qla_mid.c > @@ -935,8 +935,8 @@ int qla24xx_control_vp(scsi_qla_host_t *vha, int cmd) > sp->type = SRB_CTRL_VP; > sp->name = "ctrl_vp"; > sp->done = qla_ctrlvp_sp_done; > - qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; > + qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); > sp->u.iocb_cmd.u.ctrlvp.cmd = cmd; > sp->u.iocb_cmd.u.ctrlvp.vp_index = vp_index; > > diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c > index d5da3981cefe..134f2b8a49fe 100644 > --- a/drivers/scsi/qla2xxx/qla_mr.c > +++ b/drivers/scsi/qla2xxx/qla_mr.c > @@ -1821,9 +1821,11 @@ qlafx00_fx_disc(scsi_qla_host_t *vha, fc_port_t *fcport, uint16_t fx_type) > > sp->type = SRB_FXIOCB_DCMD; > sp->name = "fxdisc"; > - qla2x00_init_timer(sp, FXDISC_TIMEOUT); > > fdisc = &sp->u.iocb_cmd; > + fdisc->timeout = qla2x00_fxdisc_iocb_timeout; > + qla2x00_init_timer(sp, FXDISC_TIMEOUT); > + > switch (fx_type) { > case FXDISC_GET_CONFIG_INFO: > fdisc->u.fxiocb.flags = > @@ -1924,7 +1926,6 @@ qlafx00_fx_disc(scsi_qla_host_t *vha, fc_port_t *fcport, uint16_t fx_type) > goto done_unmap_req; > } > > - fdisc->timeout = qla2x00_fxdisc_iocb_timeout; > fdisc->u.fxiocb.req_func_type = cpu_to_le16(fx_type); > sp->done = qla2x00_fxdisc_sp_done; > > diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c > index b49ac85f3de2..84ca07356da1 100644 > --- a/drivers/scsi/qla2xxx/qla_target.c > +++ b/drivers/scsi/qla2xxx/qla_target.c > @@ -664,10 +664,10 @@ int qla24xx_async_notify_ack(scsi_qla_host_t *vha, fc_port_t *fcport, > sp->type = type; > sp->name = "nack"; > > + sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; > qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha)+2); > > sp->u.iocb_cmd.u.nack.ntfy = ntfy; > - sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; > sp->done = qla2x00_async_nack_sp_done; > > rval = qla2x00_start_sp(sp); > -- > 2.15.0.rc0 > What motivated this patch? are you debugging any crash which was helped by moving code around? What I see from this patch is that its moving iocb.cmd.timeout field before qla2x00_init_timer(), which are completely different from each other. Thanks, - Himanshu
On Wed, 2018-03-21 at 17:17 +0000, Madhani, Himanshu wrote: > Hi Ben, > > > On Mar 20, 2018, at 2:36 PM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > > > > qla2x00_init_timer() calls add_timer() on the iocb timeout timer, > > which means the timeout function pointer and any data that the > > function depends on must be initialised beforehand. > > > > Move this initialisation before each call to qla2x00_init_timer(). In > > some cases qla2x00_init_timer() initialises a completion structure > > needed by the timeout function, so move the call to add_timer() after > > that. [...] > What motivated this patch? are you debugging any crash which was > helped by moving code around? I saw the recent fix that added a call to del_timer(), noticed the lack of synchronisation and then kept looking further. > What I see from this patch is that its moving iocb.cmd.timeout field > before qla2x00_init_timer(), > which are completely different from each other. How are they "completely different"? qla2x00_init_timer() starts a timer that will call qla2x00_sp_timeout(), which in turn uses the iocb_cmd.timeout function pointer. We should not assume that any initialisation code after the call to add_timer() will run before the timer expires, since the scheduler might run other tasks for the whole time. It's unlikely but not impossible. Ben.
Hi Ben, > On Mar 21, 2018, at 10:45 AM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > > On Wed, 2018-03-21 at 17:17 +0000, Madhani, Himanshu wrote: >> Hi Ben, >> >>> On Mar 20, 2018, at 2:36 PM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: >>> >>> qla2x00_init_timer() calls add_timer() on the iocb timeout timer, >>> which means the timeout function pointer and any data that the >>> function depends on must be initialised beforehand. >>> >>> Move this initialisation before each call to qla2x00_init_timer(). In >>> some cases qla2x00_init_timer() initialises a completion structure >>> needed by the timeout function, so move the call to add_timer() after >>> that. > [...] >> What motivated this patch? are you debugging any crash which was >> helped by moving code around? > > I saw the recent fix that added a call to del_timer(), noticed the lack > of synchronisation and then kept looking further. > >> What I see from this patch is that its moving iocb.cmd.timeout field >> before qla2x00_init_timer(), >> which are completely different from each other. > > How are they "completely different"? qla2x00_init_timer() starts a > timer that will call qla2x00_sp_timeout(), which in turn uses the > iocb_cmd.timeout function pointer. We should not assume that any > initialisation code after the call to add_timer() will run before the > timer expires, since the scheduler might run other tasks for the whole > time. It's unlikely but not impossible. > I see. I used “completely different” due to the lack of better wording. I wanted to get context and understand if there was any issue that would have helped with these changes. your explanation does help and I agree that there is window where timer() will pop before timeout is initialized. Let me put this patch in our test cycle for couple days and will respond on this one > Ben. > > -- > Ben Hutchings > Software Developer, Codethink Ltd. > Thanks, - Himanshu
On Wed, 2018-03-21 at 17:55 +0000, Madhani, Himanshu wrote: > Hi Ben, > > > On Mar 21, 2018, at 10:45 AM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > > > > On Wed, 2018-03-21 at 17:17 +0000, Madhani, Himanshu wrote: > > > Hi Ben, > > > > > > > On Mar 20, 2018, at 2:36 PM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > > > > > > > > qla2x00_init_timer() calls add_timer() on the iocb timeout timer, > > > > which means the timeout function pointer and any data that the > > > > function depends on must be initialised beforehand. > > > > > > > > Move this initialisation before each call to qla2x00_init_timer(). In > > > > some cases qla2x00_init_timer() initialises a completion structure > > > > needed by the timeout function, so move the call to add_timer() after > > > > that. > > > > [...] > > > What motivated this patch? are you debugging any crash which was > > > helped by moving code around? > > > > I saw the recent fix that added a call to del_timer(), noticed the lack > > of synchronisation and then kept looking further. > > > > > What I see from this patch is that its moving iocb.cmd.timeout field > > > before qla2x00_init_timer(), > > > which are completely different from each other. > > > > How are they "completely different"? qla2x00_init_timer() starts a > > timer that will call qla2x00_sp_timeout(), which in turn uses the > > iocb_cmd.timeout function pointer. We should not assume that any > > initialisation code after the call to add_timer() will run before the > > timer expires, since the scheduler might run other tasks for the whole > > time. It's unlikely but not impossible. > > > > I see. I used “completely different” due to the lack of better wording. > > I wanted to get context and understand if there was any issue that would > have helped with these changes. > > your explanation does help and I agree that there is window where timer() will > pop before timeout is initialized. > > Let me put this patch in our test cycle for couple days and will respond on this one Thanks. Ben.
Hi Ben, > On Mar 21, 2018, at 10:58 AM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > > On Wed, 2018-03-21 at 17:55 +0000, Madhani, Himanshu wrote: >> Hi Ben, >> >>> On Mar 21, 2018, at 10:45 AM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: >>> >>> On Wed, 2018-03-21 at 17:17 +0000, Madhani, Himanshu wrote: >>>> Hi Ben, >>>> >>>>> On Mar 20, 2018, at 2:36 PM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: >>>>> >>>>> qla2x00_init_timer() calls add_timer() on the iocb timeout timer, >>>>> which means the timeout function pointer and any data that the >>>>> function depends on must be initialised beforehand. >>>>> >>>>> Move this initialisation before each call to qla2x00_init_timer(). In >>>>> some cases qla2x00_init_timer() initialises a completion structure >>>>> needed by the timeout function, so move the call to add_timer() after >>>>> that. >>> >>> [...] >>>> What motivated this patch? are you debugging any crash which was >>>> helped by moving code around? >>> >>> I saw the recent fix that added a call to del_timer(), noticed the lack >>> of synchronisation and then kept looking further. >>> >>>> What I see from this patch is that its moving iocb.cmd.timeout field >>>> before qla2x00_init_timer(), >>>> which are completely different from each other. >>> >>> How are they "completely different"? qla2x00_init_timer() starts a >>> timer that will call qla2x00_sp_timeout(), which in turn uses the >>> iocb_cmd.timeout function pointer. We should not assume that any >>> initialisation code after the call to add_timer() will run before the >>> timer expires, since the scheduler might run other tasks for the whole >>> time. It's unlikely but not impossible. >>> >> >> I see. I used “completely different” due to the lack of better wording. >> >> I wanted to get context and understand if there was any issue that would >> have helped with these changes. >> >> your explanation does help and I agree that there is window where timer() will >> pop before timeout is initialized. >> >> Let me put this patch in our test cycle for couple days and will respond on this one > > Thanks. > Just to update, we are still going thru review and testing and will take some extra cycle to conclude. I will update as soon as we are able to reach conclusion. > Ben. > > -- > Ben Hutchings > Software Developer, Codethink Ltd. Thanks, - Himanshu
Hi Ben, > On Mar 21, 2018, at 10:58 AM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: > > On Wed, 2018-03-21 at 17:55 +0000, Madhani, Himanshu wrote: >> Hi Ben, >> >>> On Mar 21, 2018, at 10:45 AM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: >>> >>> On Wed, 2018-03-21 at 17:17 +0000, Madhani, Himanshu wrote: >>>> Hi Ben, >>>> >>>>> On Mar 20, 2018, at 2:36 PM, Ben Hutchings <ben.hutchings@codethink.co.uk> wrote: >>>>> >>>>> qla2x00_init_timer() calls add_timer() on the iocb timeout timer, >>>>> which means the timeout function pointer and any data that the >>>>> function depends on must be initialised beforehand. >>>>> >>>>> Move this initialisation before each call to qla2x00_init_timer(). In >>>>> some cases qla2x00_init_timer() initialises a completion structure >>>>> needed by the timeout function, so move the call to add_timer() after >>>>> that. >>> >>> [...] >>>> What motivated this patch? are you debugging any crash which was >>>> helped by moving code around? >>> >>> I saw the recent fix that added a call to del_timer(), noticed the lack >>> of synchronisation and then kept looking further. >>> >>>> What I see from this patch is that its moving iocb.cmd.timeout field >>>> before qla2x00_init_timer(), >>>> which are completely different from each other. >>> >>> How are they "completely different"? qla2x00_init_timer() starts a >>> timer that will call qla2x00_sp_timeout(), which in turn uses the >>> iocb_cmd.timeout function pointer. We should not assume that any >>> initialisation code after the call to add_timer() will run before the >>> timer expires, since the scheduler might run other tasks for the whole >>> time. It's unlikely but not impossible. >>> >> >> I see. I used “completely different” due to the lack of better wording. >> >> I wanted to get context and understand if there was any issue that would >> have helped with these changes. >> >> your explanation does help and I agree that there is window where timer() will >> pop before timeout is initialized. >> >> Let me put this patch in our test cycle for couple days and will respond on this one > > Thanks. > > Ben. > > -- > Ben Hutchings > Software Developer, Codethink Ltd. Thanks again for the patch. Testing was successful. Acked-by: Himanshu Madhani <himanshu.madhani@cavium.com> Thanks, - Himanshu
Himanshu, > Thanks again for the patch. Testing was successful. > > Acked-by: Himanshu Madhani <himanshu.madhani@cavium.com> Applied to 4.17/scsi-fixes. Thanks!
diff --git a/drivers/scsi/qla2xxx/qla_gs.c b/drivers/scsi/qla2xxx/qla_gs.c index 403fa096f8c8..fcde5ea203c0 100644 --- a/drivers/scsi/qla2xxx/qla_gs.c +++ b/drivers/scsi/qla2xxx/qla_gs.c @@ -3796,6 +3796,7 @@ int qla24xx_async_gffid(scsi_qla_host_t *vha, fc_port_t *fcport) sp->gen1 = fcport->rscn_gen; sp->gen2 = fcport->login_gen; + sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); /* CT_IU preamble */ @@ -3814,7 +3815,6 @@ int qla24xx_async_gffid(scsi_qla_host_t *vha, fc_port_t *fcport) sp->u.iocb_cmd.u.ctarg.rsp_size = GFF_ID_RSP_SIZE; sp->u.iocb_cmd.u.ctarg.nport_handle = NPH_SNS; - sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; sp->done = qla24xx_async_gffid_sp_done; rval = qla2x00_start_sp(sp); @@ -4121,6 +4121,8 @@ static int qla24xx_async_gnnft(scsi_qla_host_t *vha, struct srb *sp, sp->name = "gnnft"; sp->gen1 = vha->hw->base_qpair->chip_reset; sp->gen2 = fc4_type; + + sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); memset(sp->u.iocb_cmd.u.ctarg.rsp, 0, sp->u.iocb_cmd.u.ctarg.rsp_size); @@ -4137,7 +4139,6 @@ static int qla24xx_async_gnnft(scsi_qla_host_t *vha, struct srb *sp, sp->u.iocb_cmd.u.ctarg.req_size = GNN_FT_REQ_SIZE; sp->u.iocb_cmd.u.ctarg.nport_handle = NPH_SNS; - sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; sp->done = qla2x00_async_gpnft_gnnft_sp_done; rval = qla2x00_start_sp(sp); @@ -4210,6 +4211,8 @@ int qla24xx_async_gpnft(scsi_qla_host_t *vha, u8 fc4_type) sp->name = "gpnft"; sp->gen1 = vha->hw->base_qpair->chip_reset; sp->gen2 = fc4_type; + + sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); sp->u.iocb_cmd.u.ctarg.req = dma_zalloc_coherent(&vha->hw->pdev->dev, @@ -4248,7 +4251,6 @@ int qla24xx_async_gpnft(scsi_qla_host_t *vha, u8 fc4_type) sp->u.iocb_cmd.u.ctarg.rsp_size = rspsz; sp->u.iocb_cmd.u.ctarg.nport_handle = NPH_SNS; - sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; sp->done = qla2x00_async_gpnft_gnnft_sp_done; rval = qla2x00_start_sp(sp); @@ -4356,6 +4358,7 @@ int qla24xx_async_gnnid(scsi_qla_host_t *vha, fc_port_t *fcport) sp->gen1 = fcport->rscn_gen; sp->gen2 = fcport->login_gen; + sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); /* CT_IU preamble */ @@ -4377,7 +4380,6 @@ int qla24xx_async_gnnid(scsi_qla_host_t *vha, fc_port_t *fcport) sp->u.iocb_cmd.u.ctarg.rsp_size = GNN_ID_RSP_SIZE; sp->u.iocb_cmd.u.ctarg.nport_handle = NPH_SNS; - sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; sp->done = qla2x00_async_gnnid_sp_done; rval = qla2x00_start_sp(sp); @@ -4493,6 +4495,7 @@ int qla24xx_async_gfpnid(scsi_qla_host_t *vha, fc_port_t *fcport) sp->gen1 = fcport->rscn_gen; sp->gen2 = fcport->login_gen; + sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); /* CT_IU preamble */ @@ -4514,7 +4517,6 @@ int qla24xx_async_gfpnid(scsi_qla_host_t *vha, fc_port_t *fcport) sp->u.iocb_cmd.u.ctarg.rsp_size = GFPN_ID_RSP_SIZE; sp->u.iocb_cmd.u.ctarg.nport_handle = NPH_SNS; - sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; sp->done = qla2x00_async_gfpnid_sp_done; rval = qla2x00_start_sp(sp); diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c index b0aa8cc96f0f..45319119606a 100644 --- a/drivers/scsi/qla2xxx/qla_init.c +++ b/drivers/scsi/qla2xxx/qla_init.c @@ -183,10 +183,11 @@ qla2x00_async_login(struct scsi_qla_host *vha, fc_port_t *fcport, sp->name = "login"; sp->gen1 = fcport->rscn_gen; sp->gen2 = fcport->login_gen; - qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); lio = &sp->u.iocb_cmd; lio->timeout = qla2x00_async_iocb_timeout; + qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); + sp->done = qla2x00_async_login_sp_done; lio->u.logio.flags |= SRB_LOGIN_COND_PLOGI; @@ -245,10 +246,11 @@ qla2x00_async_logout(struct scsi_qla_host *vha, fc_port_t *fcport) sp->type = SRB_LOGOUT_CMD; sp->name = "logout"; - qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); lio = &sp->u.iocb_cmd; lio->timeout = qla2x00_async_iocb_timeout; + qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); + sp->done = qla2x00_async_logout_sp_done; rval = qla2x00_start_sp(sp); if (rval != QLA_SUCCESS) @@ -307,10 +309,11 @@ qla2x00_async_prlo(struct scsi_qla_host *vha, fc_port_t *fcport) sp->type = SRB_PRLO_CMD; sp->name = "prlo"; - qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); lio = &sp->u.iocb_cmd; lio->timeout = qla2x00_async_iocb_timeout; + qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); + sp->done = qla2x00_async_prlo_sp_done; rval = qla2x00_start_sp(sp); if (rval != QLA_SUCCESS) @@ -412,10 +415,11 @@ qla2x00_async_adisc(struct scsi_qla_host *vha, fc_port_t *fcport, sp->type = SRB_ADISC_CMD; sp->name = "adisc"; - qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); lio = &sp->u.iocb_cmd; lio->timeout = qla2x00_async_iocb_timeout; + qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); + sp->done = qla2x00_async_adisc_sp_done; if (data[1] & QLA_LOGIO_LOGIN_RETRIED) lio->u.logio.flags |= SRB_LOGIN_RETRIED; @@ -745,6 +749,8 @@ int qla24xx_async_gnl(struct scsi_qla_host *vha, fc_port_t *fcport) sp->gen1 = fcport->rscn_gen; sp->gen2 = fcport->login_gen; + mbx = &sp->u.iocb_cmd; + mbx->timeout = qla2x00_async_iocb_timeout; qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha)+2); mb = sp->u.iocb_cmd.u.mbx.out_mb; @@ -757,9 +763,6 @@ int qla24xx_async_gnl(struct scsi_qla_host *vha, fc_port_t *fcport) mb[8] = vha->gnl.size; mb[9] = vha->vp_idx; - mbx = &sp->u.iocb_cmd; - mbx->timeout = qla2x00_async_iocb_timeout; - sp->done = qla24xx_async_gnl_sp_done; rval = qla2x00_start_sp(sp); @@ -888,10 +891,11 @@ qla24xx_async_prli(struct scsi_qla_host *vha, fc_port_t *fcport) sp->type = SRB_PRLI_CMD; sp->name = "prli"; - qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); lio = &sp->u.iocb_cmd; lio->timeout = qla2x00_async_iocb_timeout; + qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); + sp->done = qla2x00_async_prli_sp_done; lio->u.logio.flags = 0; @@ -956,6 +960,9 @@ int qla24xx_async_gpdb(struct scsi_qla_host *vha, fc_port_t *fcport, u8 opt) sp->name = "gpdb"; sp->gen1 = fcport->rscn_gen; sp->gen2 = fcport->login_gen; + + mbx = &sp->u.iocb_cmd; + mbx->timeout = qla2x00_async_iocb_timeout; qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); pd = dma_pool_zalloc(ha->s_dma_pool, GFP_KERNEL, &pd_dma); @@ -975,8 +982,6 @@ int qla24xx_async_gpdb(struct scsi_qla_host *vha, fc_port_t *fcport, u8 opt) mb[9] = vha->vp_idx; mb[10] = opt; - mbx = &sp->u.iocb_cmd; - mbx->timeout = qla2x00_async_iocb_timeout; mbx->u.mbx.in = (void *)pd; mbx->u.mbx.in_dma = pd_dma; @@ -1486,13 +1491,15 @@ qla2x00_async_tm_cmd(fc_port_t *fcport, uint32_t flags, uint32_t lun, tm_iocb = &sp->u.iocb_cmd; sp->type = SRB_TM_CMD; sp->name = "tmf"; + + tm_iocb->timeout = qla2x00_tmf_iocb_timeout; + init_completion(&tm_iocb->u.tmf.comp); qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha)); + tm_iocb->u.tmf.flags = flags; tm_iocb->u.tmf.lun = lun; tm_iocb->u.tmf.data = tag; sp->done = qla2x00_tmf_sp_done; - tm_iocb->timeout = qla2x00_tmf_iocb_timeout; - init_completion(&tm_iocb->u.tmf.comp); rval = qla2x00_start_sp(sp); if (rval != QLA_SUCCESS) @@ -1566,7 +1573,11 @@ qla24xx_async_abort_cmd(srb_t *cmd_sp) abt_iocb = &sp->u.iocb_cmd; sp->type = SRB_ABT_CMD; sp->name = "abort"; + + abt_iocb->timeout = qla24xx_abort_iocb_timeout; + init_completion(&abt_iocb->u.abt.comp); qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha)); + abt_iocb->u.abt.cmd_hndl = cmd_sp->handle; if (vha->flags.qpairs_available && cmd_sp->qpair) @@ -1576,8 +1587,6 @@ qla24xx_async_abort_cmd(srb_t *cmd_sp) abt_iocb->u.abt.req_que_no = cpu_to_le16(vha->req->id); sp->done = qla24xx_abort_sp_done; - abt_iocb->timeout = qla24xx_abort_iocb_timeout; - init_completion(&abt_iocb->u.abt.comp); rval = qla2x00_start_sp(sp); if (rval != QLA_SUCCESS) diff --git a/drivers/scsi/qla2xxx/qla_inline.h b/drivers/scsi/qla2xxx/qla_inline.h index 4d32426393c7..06c4a843e2ad 100644 --- a/drivers/scsi/qla2xxx/qla_inline.h +++ b/drivers/scsi/qla2xxx/qla_inline.h @@ -271,13 +271,13 @@ qla2x00_init_timer(srb_t *sp, unsigned long tmo) { timer_setup(&sp->u.iocb_cmd.timer, qla2x00_sp_timeout, 0); sp->u.iocb_cmd.timer.expires = jiffies + tmo * HZ; - add_timer(&sp->u.iocb_cmd.timer); sp->free = qla2x00_sp_free; init_completion(&sp->comp); if (IS_QLAFX00(sp->vha->hw) && (sp->type == SRB_FXIOCB_DCMD)) init_completion(&sp->u.iocb_cmd.u.fxiocb.fxiocb_comp); if (sp->type == SRB_ELS_DCMD) init_completion(&sp->u.iocb_cmd.u.els_logo.comp); + add_timer(&sp->u.iocb_cmd.timer); } static inline int diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c index 8d00d559bd26..6a719c1f8af5 100644 --- a/drivers/scsi/qla2xxx/qla_iocb.c +++ b/drivers/scsi/qla2xxx/qla_iocb.c @@ -2458,8 +2458,8 @@ qla24xx_els_dcmd_iocb(scsi_qla_host_t *vha, int els_opcode, sp->type = SRB_ELS_DCMD; sp->name = "ELS_DCMD"; sp->fcport = fcport; - qla2x00_init_timer(sp, ELS_DCMD_TIMEOUT); elsio->timeout = qla2x00_els_dcmd_iocb_timeout; + qla2x00_init_timer(sp, ELS_DCMD_TIMEOUT); sp->done = qla2x00_els_dcmd_sp_done; sp->free = qla2x00_els_dcmd_sp_free; @@ -2656,8 +2656,11 @@ qla24xx_els_dcmd2_iocb(scsi_qla_host_t *vha, int els_opcode, sp->type = SRB_ELS_DCMD; sp->name = "ELS_DCMD"; sp->fcport = fcport; - qla2x00_init_timer(sp, ELS_DCMD_TIMEOUT); + elsio->timeout = qla2x00_els_dcmd2_iocb_timeout; + init_completion(&elsio->u.els_plogi.comp); + qla2x00_init_timer(sp, ELS_DCMD_TIMEOUT); + sp->done = qla2x00_els_dcmd2_sp_done; sp->free = qla2x00_els_dcmd2_sp_free; @@ -2694,7 +2697,6 @@ qla24xx_els_dcmd2_iocb(scsi_qla_host_t *vha, int els_opcode, ql_dump_buffer(ql_dbg_io + ql_dbg_buffer, vha, 0x0109, (uint8_t *)elsio->u.els_plogi.els_plogi_pyld, 0x70); - init_completion(&elsio->u.els_plogi.comp); rval = qla2x00_start_sp(sp); if (rval != QLA_SUCCESS) { rval = QLA_FUNCTION_FAILED; diff --git a/drivers/scsi/qla2xxx/qla_mbx.c b/drivers/scsi/qla2xxx/qla_mbx.c index 7397aeddd96c..4984570e210b 100644 --- a/drivers/scsi/qla2xxx/qla_mbx.c +++ b/drivers/scsi/qla2xxx/qla_mbx.c @@ -6006,14 +6006,14 @@ int qla24xx_send_mb_cmd(struct scsi_qla_host *vha, mbx_cmd_t *mcp) sp->type = SRB_MB_IOCB; sp->name = mb_to_str(mcp->mb[0]); - qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); - - memcpy(sp->u.iocb_cmd.u.mbx.out_mb, mcp->mb, SIZEOF_IOCB_MB_REG); - c = &sp->u.iocb_cmd; c->timeout = qla2x00_async_iocb_timeout; init_completion(&c->u.mbx.comp); + qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); + + memcpy(sp->u.iocb_cmd.u.mbx.out_mb, mcp->mb, SIZEOF_IOCB_MB_REG); + sp->done = qla2x00_async_mb_sp_done; rval = qla2x00_start_sp(sp); diff --git a/drivers/scsi/qla2xxx/qla_mid.c b/drivers/scsi/qla2xxx/qla_mid.c index e965b16f21e3..07ed3aaedb4a 100644 --- a/drivers/scsi/qla2xxx/qla_mid.c +++ b/drivers/scsi/qla2xxx/qla_mid.c @@ -935,8 +935,8 @@ int qla24xx_control_vp(scsi_qla_host_t *vha, int cmd) sp->type = SRB_CTRL_VP; sp->name = "ctrl_vp"; sp->done = qla_ctrlvp_sp_done; - qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; + qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha) + 2); sp->u.iocb_cmd.u.ctrlvp.cmd = cmd; sp->u.iocb_cmd.u.ctrlvp.vp_index = vp_index; diff --git a/drivers/scsi/qla2xxx/qla_mr.c b/drivers/scsi/qla2xxx/qla_mr.c index d5da3981cefe..134f2b8a49fe 100644 --- a/drivers/scsi/qla2xxx/qla_mr.c +++ b/drivers/scsi/qla2xxx/qla_mr.c @@ -1821,9 +1821,11 @@ qlafx00_fx_disc(scsi_qla_host_t *vha, fc_port_t *fcport, uint16_t fx_type) sp->type = SRB_FXIOCB_DCMD; sp->name = "fxdisc"; - qla2x00_init_timer(sp, FXDISC_TIMEOUT); fdisc = &sp->u.iocb_cmd; + fdisc->timeout = qla2x00_fxdisc_iocb_timeout; + qla2x00_init_timer(sp, FXDISC_TIMEOUT); + switch (fx_type) { case FXDISC_GET_CONFIG_INFO: fdisc->u.fxiocb.flags = @@ -1924,7 +1926,6 @@ qlafx00_fx_disc(scsi_qla_host_t *vha, fc_port_t *fcport, uint16_t fx_type) goto done_unmap_req; } - fdisc->timeout = qla2x00_fxdisc_iocb_timeout; fdisc->u.fxiocb.req_func_type = cpu_to_le16(fx_type); sp->done = qla2x00_fxdisc_sp_done; diff --git a/drivers/scsi/qla2xxx/qla_target.c b/drivers/scsi/qla2xxx/qla_target.c index b49ac85f3de2..84ca07356da1 100644 --- a/drivers/scsi/qla2xxx/qla_target.c +++ b/drivers/scsi/qla2xxx/qla_target.c @@ -664,10 +664,10 @@ int qla24xx_async_notify_ack(scsi_qla_host_t *vha, fc_port_t *fcport, sp->type = type; sp->name = "nack"; + sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; qla2x00_init_timer(sp, qla2x00_get_async_timeout(vha)+2); sp->u.iocb_cmd.u.nack.ntfy = ntfy; - sp->u.iocb_cmd.timeout = qla2x00_async_iocb_timeout; sp->done = qla2x00_async_nack_sp_done; rval = qla2x00_start_sp(sp);
qla2x00_init_timer() calls add_timer() on the iocb timeout timer, which means the timeout function pointer and any data that the function depends on must be initialised beforehand. Move this initialisation before each call to qla2x00_init_timer(). In some cases qla2x00_init_timer() initialises a completion structure needed by the timeout function, so move the call to add_timer() after that. Signed-off-by: Ben Hutchings <ben.hutchings@codethink.co.uk> --- Changes from v1: Rebased to remove textual dependency on a patch I haven't yet sent (oops). Ben. drivers/scsi/qla2xxx/qla_gs.c | 12 +++++++----- drivers/scsi/qla2xxx/qla_init.c | 37 +++++++++++++++++++++++-------------- drivers/scsi/qla2xxx/qla_inline.h | 2 +- drivers/scsi/qla2xxx/qla_iocb.c | 8 +++++--- drivers/scsi/qla2xxx/qla_mbx.c | 8 ++++---- drivers/scsi/qla2xxx/qla_mid.c | 2 +- drivers/scsi/qla2xxx/qla_mr.c | 5 +++-- drivers/scsi/qla2xxx/qla_target.c | 2 +- 8 files changed, 45 insertions(+), 31 deletions(-)