[PATCH 0/3] scsi: qla2xxx: fixes for FW trace/dump buffers

[PATCH 0/3] scsi: qla2xxx: fixes for FW trace/dump buffers

[Martin Wilck]

From: Martin Wilck <mwilck@suse.com>

Hi Himanshu, hi Martin,

Please consider this series for merging.

The first patch of the series is a fix for a memory corruption we
saw in a test where qla2xxx was loaded/unloaded repeatedly under
memory pressure. The other ones are consistency fixes that occured
to me while working on this part of the code.

The behavior in the failure case was not consistent in the current
code; patch 3/3 makes the code do what I think makes most sense in
a failure scenario.

Regards,
Martin

Martin Wilck (3):
  scsi: qla2xxx: qla2x00_alloc_fw_dump: set ha->eft
  scsi: qla2xxx: unset RCE/EFT fields in failure case
  scsi: qla2xxx: calculate dump size if EFT alloc fails

 drivers/scsi/qla2xxx/qla_init.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

-- 
2.22.0

[PATCH 1/3] scsi: qla2xxx: qla2x00_alloc_fw_dump: set ha->eft

[Martin Wilck]

From: Martin Wilck <mwilck@suse.com>

In qla2x00_alloc_fw_dump(), an existing EFT buffer (e.g. from
previous invocation of qla2x00_alloc_offload_mem()) is freed.
The buffer is then re-allocated, but without setting the eft and
eft_dma fields to the new values.

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 | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index 535dc21..6dd68be 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -3197,6 +3197,8 @@ qla2x00_alloc_fw_dump(scsi_qla_host_t *vha)
 		ql_dbg(ql_dbg_init, vha, 0x00c3,
 		    "Allocated (%d KB) EFT ...\n", EFT_SIZE / 1024);
 		eft_size = EFT_SIZE;
+		ha->eft_dma = tc_dma;
+		ha->eft = tc;
 	}
 
 	if (IS_QLA27XX(ha) || IS_QLA28XX(ha)) {
-- 
2.22.0

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

[Martin Wilck]

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;
 		}
 
@@ -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);
-- 
2.22.0

[PATCH 3/3] scsi: qla2xxx: calculate dump size if EFT alloc fails

[Martin Wilck]

From: Martin Wilck <mwilck@suse.com>

It seems right to try and calculate the dump size properly
even in the error case, before allocating the dump buffers.

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 | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index ca9c3f3..8427436 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -3191,7 +3191,7 @@ qla2x00_alloc_fw_dump(scsi_qla_host_t *vha)
 			    EFT_SIZE / 1024);
 			ha->eft = NULL;
 			ha->eft_dma = 0;
-			goto allocate;
+			goto calc_dump_size;
 		}
 
 		rval = qla2x00_enable_eft_trace(vha, tc_dma, EFT_NUM_BUFFERS);
@@ -3202,7 +3202,7 @@ qla2x00_alloc_fw_dump(scsi_qla_host_t *vha)
 			    tc_dma);
 			ha->eft = NULL;
 			ha->eft_dma = 0;
-			goto allocate;
+			goto calc_dump_size;
 		}
 		ql_dbg(ql_dbg_init, vha, 0x00c3,
 		    "Allocated (%d KB) EFT ...\n", EFT_SIZE / 1024);
@@ -3211,6 +3211,7 @@ qla2x00_alloc_fw_dump(scsi_qla_host_t *vha)
 		ha->eft = tc;
 	}
 
+calc_dump_size:
 	if (IS_QLA27XX(ha) || IS_QLA28XX(ha)) {
 		struct fwdt *fwdt = ha->fwdt;
 		uint j;
-- 
2.22.0

Re: [PATCH 1/3] scsi: qla2xxx: qla2x00_alloc_fw_dump: set ha->eft

[Hannes Reinecke]

On 8/13/19 10:31 PM, Martin Wilck wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> In qla2x00_alloc_fw_dump(), an existing EFT buffer (e.g. from
> previous invocation of qla2x00_alloc_offload_mem()) is freed.
> The buffer is then re-allocated, but without setting the eft and
> eft_dma fields to the new values.
> 
> 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 | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index 535dc21..6dd68be 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -3197,6 +3197,8 @@ qla2x00_alloc_fw_dump(scsi_qla_host_t *vha)
>  		ql_dbg(ql_dbg_init, vha, 0x00c3,
>  		    "Allocated (%d KB) EFT ...\n", EFT_SIZE / 1024);
>  		eft_size = EFT_SIZE;
> +		ha->eft_dma = tc_dma;
> +		ha->eft = tc;
>  	}
>  
>  	if (IS_QLA27XX(ha) || IS_QLA28XX(ha)) {
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

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

[Hannes Reinecke]

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
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

Re: [PATCH 3/3] scsi: qla2xxx: calculate dump size if EFT alloc fails

[Hannes Reinecke]

On 8/13/19 10:31 PM, Martin Wilck wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> It seems right to try and calculate the dump size properly
> even in the error case, before allocating the dump buffers.
> 
> 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 | 5 +++--
>  1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index ca9c3f3..8427436 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -3191,7 +3191,7 @@ qla2x00_alloc_fw_dump(scsi_qla_host_t *vha)
>  			    EFT_SIZE / 1024);
>  			ha->eft = NULL;
>  			ha->eft_dma = 0;
> -			goto allocate;
> +			goto calc_dump_size;
>  		}
>  
>  		rval = qla2x00_enable_eft_trace(vha, tc_dma, EFT_NUM_BUFFERS);
> @@ -3202,7 +3202,7 @@ qla2x00_alloc_fw_dump(scsi_qla_host_t *vha)
>  			    tc_dma);
>  			ha->eft = NULL;
>  			ha->eft_dma = 0;
> -			goto allocate;
> +			goto calc_dump_size;
>  		}
>  		ql_dbg(ql_dbg_init, vha, 0x00c3,
>  		    "Allocated (%d KB) EFT ...\n", EFT_SIZE / 1024);
> @@ -3211,6 +3211,7 @@ qla2x00_alloc_fw_dump(scsi_qla_host_t *vha)
>  		ha->eft = tc;
>  	}
>  
> +calc_dump_size:
>  	if (IS_QLA27XX(ha) || IS_QLA28XX(ha)) {
>  		struct fwdt *fwdt = ha->fwdt;
>  		uint j;
> 
Reviewed-by: Hannes Reinecke <hare@suse.com>

Cheers,

Hannes
-- 
Dr. Hannes Reinecke		   Teamlead Storage & Networking
hare@suse.de			               +49 911 74053 688
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Mary Higgins, Sri Rasiah
HRB 21284 (AG Nürnberg)

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

[Martin Wilck]

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

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

[Himanshu Madhani]

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