[PATCH 1/2] scsi: qla2xxx: initialize fc4_type_priority

[PATCH 1/2] scsi: qla2xxx: initialize fc4_type_priority

[Martin Wilck]

From: Martin Wilck <mwilck@suse.com>

ha->fc4_type_priority is currently initialized only in
qla81xx_nvram_config(). That makes it default to NVMe for other adapters.
Fix it.

Fixes: 84ed362ac40c ("scsi: qla2xxx: Dual FCP-NVMe target port support")
Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/qla2xxx/qla_init.c | 12 ++++++++++--
 1 file changed, 10 insertions(+), 2 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
index 7cb7545..2a016a8 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -2214,8 +2214,18 @@ qla2x00_initialize_adapter(scsi_qla_host_t *vha)
 	ql_dbg(ql_dbg_init, vha, 0x0061,
 	    "Configure NVRAM parameters...\n");
 
+	/* Let priority default to FCP, can be overridden by nvram_config */
+	ha->fc4_type_priority = FC4_PRIORITY_FCP;
+
 	ha->isp_ops->nvram_config(vha);
 
+	if (ha->fc4_type_priority != FC4_PRIORITY_FCP &&
+	    ha->fc4_type_priority != FC4_PRIORITY_NVME)
+		ha->fc4_type_priority = FC4_PRIORITY_FCP;
+
+	ql_log(ql_log_info, vha, 0xffff, "FC4 priority set to %s\n",
+	       ha->fc4_type_priority == FC4_PRIORITY_FCP ? "FCP" : "NVMe");
+
 	if (ha->flags.disable_serdes) {
 		/* Mask HBA via NVRAM settings? */
 		ql_log(ql_log_info, vha, 0x0077,
@@ -8521,8 +8531,6 @@ qla81xx_nvram_config(scsi_qla_host_t *vha)
 
 	/* Determine NVMe/FCP priority for target ports */
 	ha->fc4_type_priority = qla2xxx_get_fc4_priority(vha);
-	ql_log(ql_log_info, vha, 0xffff, "FC4 priority set to %s\n",
-	    ha->fc4_type_priority & BIT_0 ? "FCP" : "NVMe");
 
 	if (rval) {
 		ql_log(ql_log_warn, vha, 0x0076,
-- 
2.23.0

[PATCH 2/2] scsi: qla2xxx: don't use zero for FC4_PRIORITY_NVME

[Martin Wilck]

From: Martin Wilck <mwilck@suse.com>

Avoid an uninitialized value being falsely treated as NVMe priority.

Signed-off-by: Martin Wilck <mwilck@suse.com>
---
 drivers/scsi/qla2xxx/qla_def.h    | 6 ++++--
 drivers/scsi/qla2xxx/qla_inline.h | 2 +-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_def.h b/drivers/scsi/qla2xxx/qla_def.h
index 721ee7f..86c5155 100644
--- a/drivers/scsi/qla2xxx/qla_def.h
+++ b/drivers/scsi/qla2xxx/qla_def.h
@@ -2476,8 +2476,10 @@ typedef struct fc_port {
 	u16 n2n_chip_reset;
 } fc_port_t;
 
-#define FC4_PRIORITY_NVME	0
-#define FC4_PRIORITY_FCP	1
+enum {
+	FC4_PRIORITY_NVME = 1,
+	FC4_PRIORITY_FCP  = 2,
+};
 
 #define QLA_FCPORT_SCAN		1
 #define QLA_FCPORT_FOUND	2
diff --git a/drivers/scsi/qla2xxx/qla_inline.h b/drivers/scsi/qla2xxx/qla_inline.h
index d728b17..352aba4 100644
--- a/drivers/scsi/qla2xxx/qla_inline.h
+++ b/drivers/scsi/qla2xxx/qla_inline.h
@@ -317,5 +317,5 @@ qla2xxx_get_fc4_priority(struct scsi_qla_host *vha)
 	    ((uint8_t *)vha->hw->nvram)[NVRAM_DUAL_FCP_NVME_FLAG_OFFSET];
 
 
-	return ((data >> 6) & BIT_0);
+	return (data >> 6) & BIT_0 ? FC4_PRIORITY_FCP : FC4_PRIORITY_NVME;
 }
-- 
2.23.0

Re: [PATCH 1/2] scsi: qla2xxx: initialize fc4_type_priority

[Martin Wilck]

On Thu, 2019-11-07 at 16:49 +0000, Martin Wilck wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> ha->fc4_type_priority is currently initialized only in
> qla81xx_nvram_config(). That makes it default to NVMe for other
> adapters.
> Fix it.
> 
> Fixes: 84ed362ac40c ("scsi: qla2xxx: Dual FCP-NVMe target port
> support")
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  drivers/scsi/qla2xxx/qla_init.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)

Tested-by: David Bond <dbond@suse.com>

Re: [PATCH 2/2] scsi: qla2xxx: don't use zero for FC4_PRIORITY_NVME

[Martin Wilck]

On Thu, 2019-11-07 at 16:49 +0000, Martin Wilck wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> Avoid an uninitialized value being falsely treated as NVMe priority.
> 
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>  drivers/scsi/qla2xxx/qla_def.h    | 6 ++++--
>  drivers/scsi/qla2xxx/qla_inline.h | 2 +-
>  2 files changed, 5 insertions(+), 3 deletions(-)

Tested-by: David Bond <dbond@suse.com>

Re: [PATCH 2/2] scsi: qla2xxx: don't use zero for FC4_PRIORITY_NVME

[Bart Van Assche]

On 11/7/19 8:49 AM, Martin Wilck wrote:
> Avoid an uninitialized value being falsely treated as NVMe priority.

Although this patch looks fine to me: which uninitialized value are you 
referring to and how does this patch make a difference?

Thanks,

Bart.

Re: [PATCH 1/2] scsi: qla2xxx: initialize fc4_type_priority

[Bart Van Assche]

On 11/7/19 8:49 AM, Martin Wilck wrote:
> From: Martin Wilck <mwilck@suse.com>
> 
> ha->fc4_type_priority is currently initialized only in
> qla81xx_nvram_config(). That makes it default to NVMe for other adapters.
> Fix it.
> 
> Fixes: 84ed362ac40c ("scsi: qla2xxx: Dual FCP-NVMe target port support")
> Signed-off-by: Martin Wilck <mwilck@suse.com>
> ---
>   drivers/scsi/qla2xxx/qla_init.c | 12 ++++++++++--
>   1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
> index 7cb7545..2a016a8 100644
> --- a/drivers/scsi/qla2xxx/qla_init.c
> +++ b/drivers/scsi/qla2xxx/qla_init.c
> @@ -2214,8 +2214,18 @@ qla2x00_initialize_adapter(scsi_qla_host_t *vha)
>   	ql_dbg(ql_dbg_init, vha, 0x0061,
>   	    "Configure NVRAM parameters...\n");
>   
> +	/* Let priority default to FCP, can be overridden by nvram_config */
> +	ha->fc4_type_priority = FC4_PRIORITY_FCP;
> +
>   	ha->isp_ops->nvram_config(vha);
>   
> +	if (ha->fc4_type_priority != FC4_PRIORITY_FCP &&
> +	    ha->fc4_type_priority != FC4_PRIORITY_NVME)
> +		ha->fc4_type_priority = FC4_PRIORITY_FCP;
> +
> +	ql_log(ql_log_info, vha, 0xffff, "FC4 priority set to %s\n",
> +	       ha->fc4_type_priority == FC4_PRIORITY_FCP ? "FCP" : "NVMe");
> +
>   	if (ha->flags.disable_serdes) {
>   		/* Mask HBA via NVRAM settings? */
>   		ql_log(ql_log_info, vha, 0x0077,
> @@ -8521,8 +8531,6 @@ qla81xx_nvram_config(scsi_qla_host_t *vha)
>   
>   	/* Determine NVMe/FCP priority for target ports */
>   	ha->fc4_type_priority = qla2xxx_get_fc4_priority(vha);
> -	ql_log(ql_log_info, vha, 0xffff, "FC4 priority set to %s\n",
> -	    ha->fc4_type_priority & BIT_0 ? "FCP" : "NVMe");
>   
>   	if (rval) {
>   		ql_log(ql_log_warn, vha, 0x0076,

Reviewed-by: Bart Van Assche <bvanassche@acm.org>

Re: [PATCH 2/2] scsi: qla2xxx: don't use zero for FC4_PRIORITY_NVME

[Bart Van Assche]

On 11/7/19 1:26 PM, Bart Van Assche wrote:
> On 11/7/19 8:49 AM, Martin Wilck wrote:
>> Avoid an uninitialized value being falsely treated as NVMe priority.
> 
> Although this patch looks fine to me: which uninitialized value are you 
> referring to and how does this patch make a difference?

Does your comment refer to ha->fc4_type_priority ? You may want to 
mention this in the commit message since that variable does not occur in 
the code touched by this patch.

Thanks,

Bart.

Re: [PATCH 2/2] scsi: qla2xxx: don't use zero for FC4_PRIORITY_NVME

[Martin Wilck]

On Thu, 2019-11-07 at 13:36 -0800, Bart Van Assche wrote:
> On 11/7/19 1:26 PM, Bart Van Assche wrote:
> > On 11/7/19 8:49 AM, Martin Wilck wrote:
> > > Avoid an uninitialized value being falsely treated as NVMe
> > > priority.
> > 
> > Although this patch looks fine to me: which uninitialized value are
> > you 
> > referring to and how does this patch make a difference?
> 
> Does your comment refer to ha->fc4_type_priority ? You may want to 
> mention this in the commit message since that variable does not occur
> in 
> the code touched by this patch.

Right. Thanks for pointing that out.