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

From: Hannes Reinecke <hare@suse.de>
To: Martin Wilck <Martin.Wilck@suse.com>,
	"Martin K. Petersen" <martin.petersen@oracle.com>,
	Himanshu Madhani <hmadhani@marvell.com>
Cc: Bart Van Assche <Bart.VanAssche@sandisk.com>,
	Joe Carnuccio <joe.carnuccio@cavium.com>,
	Quinn Tran <qutran@marvell.com>,
	"linux-scsi@vger.kernel.org" <linux-scsi@vger.kernel.org>,
	Bart Van Assche <bvanassche@acm.org>
Subject: Re: [PATCH 2/3] scsi: qla2xxx: unset RCE/EFT fields in failure case
Date: Wed, 14 Aug 2019 08:24:10 +0200	[thread overview]
Message-ID: <9d479501-27bd-7932-9517-4545231e6ae9@suse.de> (raw)
In-Reply-To: <20190813203034.7354-3-martin.wilck@suse.com>

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)