Re: [PATCH 2/3] scsi: qla2xxx: unset RCE/EFT fields in failure case

[Himanshu Madhani <hmadhani@xxxxxxxxxxx>]

On 8/14/19, 6:30 AM, "linux-scsi-owner@xxxxxxxxxxxxxxx on behalf of Martin Wilck" <linux-scsi-owner@xxxxxxxxxxxxxxx on behalf of Martin.Wilck@xxxxxxxx> 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 <mwilck@xxxxxxxx>
    > > 
    > > 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.carnuccio@xxxxxxxxxx>
    > > Cc: Quinn Tran <qutran@xxxxxxxxxxx>
    > > Cc: Himanshu Madhani <hmadhani@xxxxxxxxxxx>
    > > Cc: Bart Van Assche <bvanassche@xxxxxxx>
    > > Signed-off-by: Martin Wilck <mwilck@xxxxxxxx>
    > > ---
    > >  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