On 8/14/19, 6:30 AM, "linux-scsi-ow...@vger.kernel.org on behalf of Martin
Wilck" <linux-scsi-ow...@vger.kernel.org on behalf of martin.wi...@suse.com>
wrote:
On Wed, 2019-08-14 at 08:24 +0200, Hannes Reinecke wrote:
> On 8/13/19 10:31 PM, Martin Wilck wrote:
> > From: Martin Wilck <mwi...@suse.com>
> >
> > Reset ha->rce, ha->eft and the respective dma fields if
> > the buffers aren't mapped for some reason. Also, treat
> > both failure cases (allocation and initialization failure)
> > equally. The next patch modifies the failure behavior
> > slightly again.
> >
> > Fixes: ad0a0b01f088 "scsi: qla2xxx: Fix Firmware dump size for
> > Extended
> > login and Exchange Offload"
> > Fixes: a28d9e4ef997 "scsi: qla2xxx: Add support for multiple fwdump
> > templates/segments"
> > Cc: Joe Carnuccio <joe.carnuc...@cavium.com>
> > Cc: Quinn Tran <qut...@marvell.com>
> > Cc: Himanshu Madhani <hmadh...@marvell.com>
> > Cc: Bart Van Assche <bvanass...@acm.org>
> > Signed-off-by: Martin Wilck <mwi...@suse.com>
> > ---
> > drivers/scsi/qla2xxx/qla_init.c | 10 ++++++++++
> > 1 file changed, 10 insertions(+)
> >
> > diff --git a/drivers/scsi/qla2xxx/qla_init.c
> > b/drivers/scsi/qla2xxx/qla_init.c
> > index 6dd68be..ca9c3f3 100644
> > --- a/drivers/scsi/qla2xxx/qla_init.c
> > +++ b/drivers/scsi/qla2xxx/qla_init.c
> > @@ -3063,6 +3063,8 @@ qla2x00_alloc_offload_mem(scsi_qla_host_t
> > *vha)
> > ql_log(ql_log_warn, vha, 0x00be,
> > "Unable to allocate (%d KB) for FCE.\n",
> > FCE_SIZE / 1024);
> > + ha->fce_dma = 0;
> > + ha->fce = NULL;
> > goto try_eft;
> > }
> >
> Actually, I would set this earlier here:
>
> if (ha->fce)
> dma_free_coherent(&ha->pdev->dev,
> FCE_SIZE, ha->fce, ha->fce_dma);
>
> which is the logical place to clear 'ha->fce' and 'ha->fce_dma' IMO.
Fine with me.
> Alsoe there is this call later on:
>
> rval = qla2x00_enable_fce_trace(vha, tc_dma,
> FCE_NUM_BUFFERS,
> ha->fce_mb, &ha->fce_bufs);
> if (rval) {
> ql_log(ql_log_warn, vha, 0x00bf,
> "Unable to initialize FCE (%d).\n", rval);
> dma_free_coherent(&ha->pdev->dev, FCE_SIZE, tc,
> tc_dma);
> ha->flags.fce_enabled = 0;
> goto try_eft;
> }
>
> which also needs to be protected.
Right.
> > @@ -3111,9 +3113,12 @@ qla2x00_alloc_offload_mem(scsi_qla_host_t
> > *vha)
> >
> > ha->eft_dma = tc_dma;
> > ha->eft = tc;
> > + return;
> > }
> >
> > eft_err:
> > + ha->eft = NULL;
> > + ha->eft_dma = 0;
> > return;
> > }
> >
> I wonder why this is even there.
>
> Right at the start we have:
> if (ha->eft) {
> ql_dbg(ql_dbg_init, vha, 0x00bd,
> "%s: Offload Mem is already allocated.\n",
> __func__);
> return;
> }
>
> IE the second half of this function really should be unreachable
> code.
I do agree that second half of the function was unreachable code.
This check is only in qla2x00_alloc_offload_mem(), not in
qla2x00_alloc_fw_dump(), where EFT allocation is attempted again (and
qla2x00_alloc_offload_mem() is called first). It looks like an
oversight, indeed. IMO this part of the code needs cleanup; for now
I tried to keep the patches small.
> Himanshu?
>
I see you sent out v2 already of this series with cleanups suggested by Hannes.
I'll review your v2 on the list.
Thanks,
Himanshu
Thanks,
Martin
