diff mbox series

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

Message ID 20190813203034.7354-3-martin.wilck@suse.com (mailing list archive)
State Superseded
Headers show
Series scsi: qla2xxx: fixes for FW trace/dump buffers | expand

Commit Message

Martin Wilck Aug. 13, 2019, 8:31 p.m. UTC 
From: Martin Wilck <mwilck@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.carnuccio@cavium.com>
Cc: Quinn Tran <qutran@marvell.com>
Cc: Himanshu Madhani <hmadhani@marvell.com>
Cc: Bart Van Assche <bvanassche@acm.org>
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/qla2xxx/qla_init.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

Comments

Hannes Reinecke Aug. 14, 2019, 6:24 a.m. UTC | #1 
On 8/13/19 10:31 PM, Martin Wilck wrote:
> From: Martin Wilck <mwilck@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.carnuccio@cavium.com>
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Himanshu Madhani <hmadhani@marvell.com>
> Cc: Bart Van Assche <bvanassche@acm.org>
> Signed-off-by: Martin Wilck <mwilck@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.
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.

> @@ -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.
Himanshu?

Cheers,

Hannes
Martin Wilck Aug. 14, 2019, 11:14 a.m. UTC | #2 
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@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.carnuccio@cavium.com>
> > Cc: Quinn Tran <qutran@marvell.com>
> > Cc: Himanshu Madhani <hmadhani@marvell.com>
> > Cc: Bart Van Assche <bvanassche@acm.org>
> > Signed-off-by: Martin Wilck <mwilck@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.

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?
> 

Thanks,
Martin
Himanshu Madhani Aug. 14, 2019, 2:18 p.m. UTC | #3 
On 8/14/19, 6:30 AM, "linux-scsi-owner@vger.kernel.org on behalf of Martin Wilck" <linux-scsi-owner@vger.kernel.org on behalf of Martin.Wilck@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 <mwilck@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.carnuccio@cavium.com>
    > > Cc: Quinn Tran <qutran@marvell.com>
    > > Cc: Himanshu Madhani <hmadhani@marvell.com>
    > > Cc: Bart Van Assche <bvanassche@acm.org>
    > > Signed-off-by: Martin Wilck <mwilck@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
diff mbox series

Patch

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;
 		}
 
@@ -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;
 }
 
@@ -3184,6 +3189,8 @@  qla2x00_alloc_fw_dump(scsi_qla_host_t *vha)
 			ql_log(ql_log_warn, vha, 0x00c1,
 			    "Unable to allocate (%d KB) for EFT.\n",
 			    EFT_SIZE / 1024);
+			ha->eft = NULL;
+			ha->eft_dma = 0;
 			goto allocate;
 		}
 
@@ -3193,6 +3200,9 @@  qla2x00_alloc_fw_dump(scsi_qla_host_t *vha)
 			    "Unable to initialize EFT (%d).\n", rval);
 			dma_free_coherent(&ha->pdev->dev, EFT_SIZE, tc,
 			    tc_dma);
+			ha->eft = NULL;
+			ha->eft_dma = 0;
+			goto allocate;
 		}
 		ql_dbg(ql_dbg_init, vha, 0x00c3,
 		    "Allocated (%d KB) EFT ...\n", EFT_SIZE / 1024);