[PATCH] Revert "qla2xxx: Fix Nport ID display value"

[PATCH] Revert "qla2xxx: Fix Nport ID display value"

[Bart Van Assche]

The commit mentioned in the subject breaks point-to-point mode for at least
the QLE2562 HBA. Restore point-to-point support by reverting that commit.

Cc: Roman Bolshakov <r.bolshakov@yadro.com>
Cc: Quinn Tran <qutran@marvell.com>
Cc: Himanshu Madhani <hmadhani@marvell.com>
Fixes: 0aabb6b699f7 ("scsi: qla2xxx: Fix Nport ID display value")
Signed-off-by: Bart Van Assche <bvanassche@acm.org>
---
 drivers/scsi/qla2xxx/qla_iocb.c | 7 +++----
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
index b25f87ff8cde..cfd686fab1b1 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -2656,10 +2656,9 @@ qla24xx_els_logo_iocb(srb_t *sp, struct els_entry_24xx *els_iocb)
 	els_iocb->port_id[0] = sp->fcport->d_id.b.al_pa;
 	els_iocb->port_id[1] = sp->fcport->d_id.b.area;
 	els_iocb->port_id[2] = sp->fcport->d_id.b.domain;
-	/* For SID the byte order is different than DID */
-	els_iocb->s_id[1] = vha->d_id.b.al_pa;
-	els_iocb->s_id[2] = vha->d_id.b.area;
-	els_iocb->s_id[0] = vha->d_id.b.domain;
+	els_iocb->s_id[0] = vha->d_id.b.al_pa;
+	els_iocb->s_id[1] = vha->d_id.b.area;
+	els_iocb->s_id[2] = vha->d_id.b.domain;
 
 	if (elsio->u.els_logo.els_cmd == ELS_DCMD_PLOGI) {
 		els_iocb->control_flags = 0;
-- 
2.23.0

Re: [PATCH] Revert "qla2xxx: Fix Nport ID display value"

[Roman Bolshakov]

On Fri, Nov 08, 2019 at 08:21:35PM -0800, Bart Van Assche wrote:
> The commit mentioned in the subject breaks point-to-point mode for at least
> the QLE2562 HBA. Restore point-to-point support by reverting that commit.
> 
> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> Cc: Quinn Tran <qutran@marvell.com>
> Cc: Himanshu Madhani <hmadhani@marvell.com>
> Fixes: 0aabb6b699f7 ("scsi: qla2xxx: Fix Nport ID display value") > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> ---
>  drivers/scsi/qla2xxx/qla_iocb.c | 7 +++----
>  1 file changed, 3 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
> index b25f87ff8cde..cfd686fab1b1 100644
> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> @@ -2656,10 +2656,9 @@ qla24xx_els_logo_iocb(srb_t *sp, struct els_entry_24xx *els_iocb)
>  	els_iocb->port_id[0] = sp->fcport->d_id.b.al_pa;
>  	els_iocb->port_id[1] = sp->fcport->d_id.b.area;
>  	els_iocb->port_id[2] = sp->fcport->d_id.b.domain;
> -	/* For SID the byte order is different than DID */
> -	els_iocb->s_id[1] = vha->d_id.b.al_pa;
> -	els_iocb->s_id[2] = vha->d_id.b.area;
> -	els_iocb->s_id[0] = vha->d_id.b.domain;
> +	els_iocb->s_id[0] = vha->d_id.b.al_pa;
> +	els_iocb->s_id[1] = vha->d_id.b.area;
> +	els_iocb->s_id[2] = vha->d_id.b.domain;
>  
>  	if (elsio->u.els_logo.els_cmd == ELS_DCMD_PLOGI) {
>  		els_iocb->control_flags = 0;
> -- 
> 2.23.0
> 

The original commit definitely fixes P2P mode for QLE2700, the lowest
byte is domain, followed by AL_PA, followed by area. However the
fields are reserved in ELS IOCB for QLE2500, according to FW spec.

Perhaps we should have a switch here for 2500 and the other one for
2600/2700? Or, we should only set the fields only for QLE2700, to comply
with both specs.

Thank you,
Roman

Re: [PATCH] Revert "qla2xxx: Fix Nport ID display value"

[Bart Van Assche]

On 2019-11-11 03:28, Roman Bolshakov wrote:
> On Fri, Nov 08, 2019 at 08:21:35PM -0800, Bart Van Assche wrote:
>> The commit mentioned in the subject breaks point-to-point mode for at least
>> the QLE2562 HBA. Restore point-to-point support by reverting that commit.
>>
>> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
>> Cc: Quinn Tran <qutran@marvell.com>
>> Cc: Himanshu Madhani <hmadhani@marvell.com>
>> Fixes: 0aabb6b699f7 ("scsi: qla2xxx: Fix Nport ID display value") > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>  drivers/scsi/qla2xxx/qla_iocb.c | 7 +++----
>>  1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
>> index b25f87ff8cde..cfd686fab1b1 100644
>> --- a/drivers/scsi/qla2xxx/qla_iocb.c
>> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
>> @@ -2656,10 +2656,9 @@ qla24xx_els_logo_iocb(srb_t *sp, struct els_entry_24xx *els_iocb)
>>  	els_iocb->port_id[0] = sp->fcport->d_id.b.al_pa;
>>  	els_iocb->port_id[1] = sp->fcport->d_id.b.area;
>>  	els_iocb->port_id[2] = sp->fcport->d_id.b.domain;
>> -	/* For SID the byte order is different than DID */
>> -	els_iocb->s_id[1] = vha->d_id.b.al_pa;
>> -	els_iocb->s_id[2] = vha->d_id.b.area;
>> -	els_iocb->s_id[0] = vha->d_id.b.domain;
>> +	els_iocb->s_id[0] = vha->d_id.b.al_pa;
>> +	els_iocb->s_id[1] = vha->d_id.b.area;
>> +	els_iocb->s_id[2] = vha->d_id.b.domain;
>>  
>>  	if (elsio->u.els_logo.els_cmd == ELS_DCMD_PLOGI) {
>>  		els_iocb->control_flags = 0;
> 
> The original commit definitely fixes P2P mode for QLE2700, the lowest
> byte is domain, followed by AL_PA, followed by area. However the
> fields are reserved in ELS IOCB for QLE2500, according to FW spec.
> 
> Perhaps we should have a switch here for 2500 and the other one for
> 2600/2700? Or, we should only set the fields only for QLE2700, to comply
> with both specs.

Himanshu, can you tell us which adapters and/or firmware versions need
which version of the above code?

Thank you,

Bart.

Re: [PATCH] Revert "qla2xxx: Fix Nport ID display value"

[Himanshu Madhani]

Hi Bart/Roman,


> On Nov 11, 2019, at 8:48 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> On 2019-11-11 03:28, Roman Bolshakov wrote:
>> On Fri, Nov 08, 2019 at 08:21:35PM -0800, Bart Van Assche wrote:
>>> The commit mentioned in the subject breaks point-to-point mode for at least
>>> the QLE2562 HBA. Restore point-to-point support by reverting that commit.
>>> 
>>> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
>>> Cc: Quinn Tran <qutran@marvell.com>
>>> Cc: Himanshu Madhani <hmadhani@marvell.com>
>>> Fixes: 0aabb6b699f7 ("scsi: qla2xxx: Fix Nport ID display value") > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>>> ---
>>> drivers/scsi/qla2xxx/qla_iocb.c | 7 +++----
>>> 1 file changed, 3 insertions(+), 4 deletions(-)
>>> 
>>> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
>>> index b25f87ff8cde..cfd686fab1b1 100644
>>> --- a/drivers/scsi/qla2xxx/qla_iocb.c
>>> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
>>> @@ -2656,10 +2656,9 @@ qla24xx_els_logo_iocb(srb_t *sp, struct els_entry_24xx *els_iocb)
>>> 	els_iocb->port_id[0] = sp->fcport->d_id.b.al_pa;
>>> 	els_iocb->port_id[1] = sp->fcport->d_id.b.area;
>>> 	els_iocb->port_id[2] = sp->fcport->d_id.b.domain;
>>> -	/* For SID the byte order is different than DID */
>>> -	els_iocb->s_id[1] = vha->d_id.b.al_pa;
>>> -	els_iocb->s_id[2] = vha->d_id.b.area;
>>> -	els_iocb->s_id[0] = vha->d_id.b.domain;
>>> +	els_iocb->s_id[0] = vha->d_id.b.al_pa;
>>> +	els_iocb->s_id[1] = vha->d_id.b.area;
>>> +	els_iocb->s_id[2] = vha->d_id.b.domain;
>>> 
>>> 	if (elsio->u.els_logo.els_cmd == ELS_DCMD_PLOGI) {
>>> 		els_iocb->control_flags = 0;
>> 
>> The original commit definitely fixes P2P mode for QLE2700, the lowest
>> byte is domain, followed by AL_PA, followed by area. However the
>> fields are reserved in ELS IOCB for QLE2500, according to FW spec.
>> 
>> Perhaps we should have a switch here for 2500 and the other one for
>> 2600/2700? Or, we should only set the fields only for QLE2700, to comply
>> with both specs.
> Himanshu, can you tell us which adapters and/or firmware versions need
> which version of the above code?
> 
> Thank you,
> 
> Bart.

All adapters with FW v4.4 will behave same. If you are running into issue with P2P connection,
it might be something different than specific to this code change. Original code in the driver was not
following firmware spec. This code is now used in initiator mode as well due to introduction of
FC-NVMe handling in the driver.  Also, can you tell me what version of firmware you have on your 
remote port.

Thanks,
Himanshu

Re: [PATCH] Revert "qla2xxx: Fix Nport ID display value"

[Bart Van Assche]

On 2019-11-13 07:29, Himanshu Madhani wrote:
> On Nov 11, 2019, at 8:48 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>> Himanshu, can you tell us which adapters and/or firmware versions need
>> which version of the above code?
> 
> All adapters with FW v4.4 will behave same. If you are running into issue with P2P connection,
> it might be something different than specific to this code change. Original code in the driver was not
> following firmware spec. This code is now used in initiator mode as well due to introduction of
> FC-NVMe handling in the driver.  Also, can you tell me what version of firmware you have on your 
> remote port.

Hi Himanshu,

My test was run on a setup with a single QLE2562 adapter with both FC
ports connected directly to each other. The kernel driver identifies
that adapter as follows: ISP2532: PCIe (5.0GT/s x8) @ 0000:00:0a.0 hdma+
host#=12 fw=8.07.00 (90d5). Please let me know if you need more information.

Thanks,

Bart.

Re: [PATCH] Revert "qla2xxx: Fix Nport ID display value"

[Bart Van Assche]

On 11/13/19 6:34 PM, Bart Van Assche wrote:
> On 2019-11-13 07:29, Himanshu Madhani wrote:
>> On Nov 11, 2019, at 8:48 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>>> Himanshu, can you tell us which adapters and/or firmware versions need
>>> which version of the above code?
>>
>> All adapters with FW v4.4 will behave same. If you are running into issue with P2P connection,
>> it might be something different than specific to this code change. Original code in the driver was not
>> following firmware spec. This code is now used in initiator mode as well due to introduction of
>> FC-NVMe handling in the driver.  Also, can you tell me what version of firmware you have on your
>> remote port.
> 
> Hi Himanshu,
> 
> My test was run on a setup with a single QLE2562 adapter with both FC
> ports connected directly to each other. The kernel driver identifies
> that adapter as follows: ISP2532: PCIe (5.0GT/s x8) @ 0000:00:0a.0 hdma+
> host#=12 fw=8.07.00 (90d5). Please let me know if you need more information.

Ping?

Bart.

Re: [PATCH] Revert "qla2xxx: Fix Nport ID display value"

[Himanshu Madhani]

Hi Bart,


> On Nov 19, 2019, at 6:15 PM, Bart Van Assche <bvanassche@acm.org> wrote:
> 
> On 11/13/19 6:34 PM, Bart Van Assche wrote:
>> On 2019-11-13 07:29, Himanshu Madhani wrote:
>>> On Nov 11, 2019, at 8:48 PM, Bart Van Assche <bvanassche@acm.org> wrote:
>>>> Himanshu, can you tell us which adapters and/or firmware versions need
>>>> which version of the above code?
>>> 
>>> All adapters with FW v4.4 will behave same. If you are running into issue with P2P connection,
>>> it might be something different than specific to this code change. Original code in the driver was not
>>> following firmware spec. This code is now used in initiator mode as well due to introduction of
>>> FC-NVMe handling in the driver.  Also, can you tell me what version of firmware you have on your
>>> remote port.
>> Hi Himanshu,
>> My test was run on a setup with a single QLE2562 adapter with both FC
>> ports connected directly to each other. The kernel driver identifies
>> that adapter as follows: ISP2532: PCIe (5.0GT/s x8) @ 0000:00:0a.0 hdma+
>> host#=12 fw=8.07.00 (90d5). Please let me know if you need more information.
> 
> Ping?
> 
> Bart.


Sorry for delay. Can you send me log file with ql2xextended_error_logging=0x5200b000 to see why you are seeing issue in your setup and if possible trigger FW dump and send me that as well right after you see this issue. I’ll have to check with my firmware guys on the behavior you are seeing.

Thanks,
Himanshu

Re: [PATCH] Revert "qla2xxx: Fix Nport ID display value"

[Bart Van Assche]

On 2019-11-20 13:39, Himanshu Madhani wrote:
> Sorry for delay. Can you send me log file with
> ql2xextended_error_logging=0x5200b000 to see why you are seeing issue
> in your setup and if possible trigger FW dump and send me that as well
> right after you see this issue. I’ll have to check with my firmware
> guys on the behavior you are seeing.

Hi Himanshu,

Logs are available at
https://drive.google.com/file/d/195QDMr2Yj_y4JAbkL6zKlqP5txI0Bvv-/

Creating a firmware dump failed unfortunately:
root@ubuntu-vm:~# qaucli -fwdump 21-00-00-24-FF-46-B8-E9 fwdump.bin
Using config file: /opt/QLogic_Corporation/QConvergeConsoleCLI/qaucli.cfg
Installation directory: /opt/QLogic_Corporation/QConvergeConsoleCLI
Working dir: /root
Option is unsupported with selected HBA (Instance 1 - QLE2562)!

This test was run against Martin's 5.5/scsi-queue branch (commit
65309ef6b258 ("scsi: bnx2fc: timeout calculation invalid for
bnx2fc_eh_abort()")).

Please let me know if you need more information.

Bart.

Re: [PATCH] Revert "qla2xxx: Fix Nport ID display value"

[Bart Van Assche]

On 11/11/19 3:28 AM, Roman Bolshakov wrote:
> On Fri, Nov 08, 2019 at 08:21:35PM -0800, Bart Van Assche wrote:
>> The commit mentioned in the subject breaks point-to-point mode for at least
>> the QLE2562 HBA. Restore point-to-point support by reverting that commit.
>>
>> Cc: Roman Bolshakov <r.bolshakov@yadro.com>
>> Cc: Quinn Tran <qutran@marvell.com>
>> Cc: Himanshu Madhani <hmadhani@marvell.com>
>> Fixes: 0aabb6b699f7 ("scsi: qla2xxx: Fix Nport ID display value") > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
>> ---
>>   drivers/scsi/qla2xxx/qla_iocb.c | 7 +++----
>>   1 file changed, 3 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
>> index b25f87ff8cde..cfd686fab1b1 100644
>> --- a/drivers/scsi/qla2xxx/qla_iocb.c
>> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
>> @@ -2656,10 +2656,9 @@ qla24xx_els_logo_iocb(srb_t *sp, struct els_entry_24xx *els_iocb)
>>   	els_iocb->port_id[0] = sp->fcport->d_id.b.al_pa;
>>   	els_iocb->port_id[1] = sp->fcport->d_id.b.area;
>>   	els_iocb->port_id[2] = sp->fcport->d_id.b.domain;
>> -	/* For SID the byte order is different than DID */
>> -	els_iocb->s_id[1] = vha->d_id.b.al_pa;
>> -	els_iocb->s_id[2] = vha->d_id.b.area;
>> -	els_iocb->s_id[0] = vha->d_id.b.domain;
>> +	els_iocb->s_id[0] = vha->d_id.b.al_pa;
>> +	els_iocb->s_id[1] = vha->d_id.b.area;
>> +	els_iocb->s_id[2] = vha->d_id.b.domain;
>>   
>>   	if (elsio->u.els_logo.els_cmd == ELS_DCMD_PLOGI) {
>>   		els_iocb->control_flags = 0;
> 
> The original commit definitely fixes P2P mode for QLE2700, the lowest
> byte is domain, followed by AL_PA, followed by area. However the
> fields are reserved in ELS IOCB for QLE2500, according to FW spec.
> 
> Perhaps we should have a switch here for 2500 and the other one for
> 2600/2700? Or, we should only set the fields only for QLE2700, to comply
> with both specs.

Hi Roman,

How about the patch below? Leaving out the els_iocb->s_id[] initialization
is not sufficient on 2500 series adapters to restore point-to-point mode.
The patch below works fine on my setup.

Thanks,

Bart.

diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
index b25f87ff8cde..e2e91b3f2e65 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -2656,10 +2656,16 @@ qla24xx_els_logo_iocb(srb_t *sp, struct els_entry_24xx *els_iocb)
  	els_iocb->port_id[0] = sp->fcport->d_id.b.al_pa;
  	els_iocb->port_id[1] = sp->fcport->d_id.b.area;
  	els_iocb->port_id[2] = sp->fcport->d_id.b.domain;
-	/* For SID the byte order is different than DID */
-	els_iocb->s_id[1] = vha->d_id.b.al_pa;
-	els_iocb->s_id[2] = vha->d_id.b.area;
-	els_iocb->s_id[0] = vha->d_id.b.domain;
+	if (IS_QLA23XX(vha->hw) || IS_QLA24XX(vha->hw) || IS_QLA25XX(vha->hw)) {
+		els_iocb->s_id[0] = vha->d_id.b.al_pa;
+		els_iocb->s_id[1] = vha->d_id.b.area;
+		els_iocb->s_id[2] = vha->d_id.b.domain;
+	} else {
+		/* For SID the byte order is different than DID */
+		els_iocb->s_id[1] = vha->d_id.b.al_pa;
+		els_iocb->s_id[2] = vha->d_id.b.area;
+		els_iocb->s_id[0] = vha->d_id.b.domain;
+	}

  	if (elsio->u.els_logo.els_cmd == ELS_DCMD_PLOGI) {
  		els_iocb->control_flags = 0;

Re: [PATCH] Revert "qla2xxx: Fix Nport ID display value"

[Roman Bolshakov]

On Mon, Dec 02, 2019 at 08:40:17AM -0800, Bart Van Assche wrote:
> On 11/11/19 3:28 AM, Roman Bolshakov wrote:
> > On Fri, Nov 08, 2019 at 08:21:35PM -0800, Bart Van Assche wrote:
> > > The commit mentioned in the subject breaks point-to-point mode for at least
> > > the QLE2562 HBA. Restore point-to-point support by reverting that commit.
> > > 
> > > Cc: Roman Bolshakov <r.bolshakov@yadro.com>
> > > Cc: Quinn Tran <qutran@marvell.com>
> > > Cc: Himanshu Madhani <hmadhani@marvell.com>
> > > Fixes: 0aabb6b699f7 ("scsi: qla2xxx: Fix Nport ID display value") > Signed-off-by: Bart Van Assche <bvanassche@acm.org>
> > > ---
> > >   drivers/scsi/qla2xxx/qla_iocb.c | 7 +++----
> > >   1 file changed, 3 insertions(+), 4 deletions(-)
> > > 
> > > diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
> > > index b25f87ff8cde..cfd686fab1b1 100644
> > > --- a/drivers/scsi/qla2xxx/qla_iocb.c
> > > +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> > > @@ -2656,10 +2656,9 @@ qla24xx_els_logo_iocb(srb_t *sp, struct els_entry_24xx *els_iocb)
> > >   	els_iocb->port_id[0] = sp->fcport->d_id.b.al_pa;
> > >   	els_iocb->port_id[1] = sp->fcport->d_id.b.area;
> > >   	els_iocb->port_id[2] = sp->fcport->d_id.b.domain;
> > > -	/* For SID the byte order is different than DID */
> > > -	els_iocb->s_id[1] = vha->d_id.b.al_pa;
> > > -	els_iocb->s_id[2] = vha->d_id.b.area;
> > > -	els_iocb->s_id[0] = vha->d_id.b.domain;
> > > +	els_iocb->s_id[0] = vha->d_id.b.al_pa;
> > > +	els_iocb->s_id[1] = vha->d_id.b.area;
> > > +	els_iocb->s_id[2] = vha->d_id.b.domain;
> > >   	if (elsio->u.els_logo.els_cmd == ELS_DCMD_PLOGI) {
> > >   		els_iocb->control_flags = 0;
> > 
> > The original commit definitely fixes P2P mode for QLE2700, the lowest
> > byte is domain, followed by AL_PA, followed by area. However the
> > fields are reserved in ELS IOCB for QLE2500, according to FW spec.
> > 
> > Perhaps we should have a switch here for 2500 and the other one for
> > 2600/2700? Or, we should only set the fields only for QLE2700, to comply
> > with both specs.
> 
> Hi Roman,
> 
> How about the patch below? Leaving out the els_iocb->s_id[] initialization
> is not sufficient on 2500 series adapters to restore point-to-point mode.
> The patch below works fine on my setup.
> 
> Thanks,
> 
> Bart.
> 
> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
> index b25f87ff8cde..e2e91b3f2e65 100644
> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> @@ -2656,10 +2656,16 @@ qla24xx_els_logo_iocb(srb_t *sp, struct els_entry_24xx *els_iocb)
>  	els_iocb->port_id[0] = sp->fcport->d_id.b.al_pa;
>  	els_iocb->port_id[1] = sp->fcport->d_id.b.area;
>  	els_iocb->port_id[2] = sp->fcport->d_id.b.domain;
> -	/* For SID the byte order is different than DID */
> -	els_iocb->s_id[1] = vha->d_id.b.al_pa;
> -	els_iocb->s_id[2] = vha->d_id.b.area;
> -	els_iocb->s_id[0] = vha->d_id.b.domain;
> +	if (IS_QLA23XX(vha->hw) || IS_QLA24XX(vha->hw) || IS_QLA25XX(vha->hw)) {
> +		els_iocb->s_id[0] = vha->d_id.b.al_pa;
> +		els_iocb->s_id[1] = vha->d_id.b.area;
> +		els_iocb->s_id[2] = vha->d_id.b.domain;
> +	} else {
> +		/* For SID the byte order is different than DID */
> +		els_iocb->s_id[1] = vha->d_id.b.al_pa;
> +		els_iocb->s_id[2] = vha->d_id.b.area;
> +		els_iocb->s_id[0] = vha->d_id.b.domain;
> +	}
> 
>  	if (elsio->u.els_logo.els_cmd == ELS_DCMD_PLOGI) {
>  		els_iocb->control_flags = 0;

Hi Bart,

I'm fine as long as it works for old and new HBAs. I don't have docs to
2300/2400 series and the HBAs. Are you sure it follows the same S_ID
order as 2500?

Regardless of that, it should work for the last 4 series of the HBAs,
Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>

Thanks,
Roman

Re: [PATCH] Revert "qla2xxx: Fix Nport ID display value"

[Bart Van Assche]

On 2019-12-02 12:55, Roman Bolshakov wrote:
> On Mon, Dec 02, 2019 at 08:40:17AM -0800, Bart Van Assche wrote:
>> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
>> index b25f87ff8cde..e2e91b3f2e65 100644
>> --- a/drivers/scsi/qla2xxx/qla_iocb.c
>> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
>> @@ -2656,10 +2656,16 @@ qla24xx_els_logo_iocb(srb_t *sp, struct els_entry_24xx *els_iocb)
>>  	els_iocb->port_id[0] = sp->fcport->d_id.b.al_pa;
>>  	els_iocb->port_id[1] = sp->fcport->d_id.b.area;
>>  	els_iocb->port_id[2] = sp->fcport->d_id.b.domain;
>> -	/* For SID the byte order is different than DID */
>> -	els_iocb->s_id[1] = vha->d_id.b.al_pa;
>> -	els_iocb->s_id[2] = vha->d_id.b.area;
>> -	els_iocb->s_id[0] = vha->d_id.b.domain;
>> +	if (IS_QLA23XX(vha->hw) || IS_QLA24XX(vha->hw) || IS_QLA25XX(vha->hw)) {
>> +		els_iocb->s_id[0] = vha->d_id.b.al_pa;
>> +		els_iocb->s_id[1] = vha->d_id.b.area;
>> +		els_iocb->s_id[2] = vha->d_id.b.domain;
>> +	} else {
>> +		/* For SID the byte order is different than DID */
>> +		els_iocb->s_id[1] = vha->d_id.b.al_pa;
>> +		els_iocb->s_id[2] = vha->d_id.b.area;
>> +		els_iocb->s_id[0] = vha->d_id.b.domain;
>> +	}
>>
>>  	if (elsio->u.els_logo.els_cmd == ELS_DCMD_PLOGI) {
>>  		els_iocb->control_flags = 0;
> 
> Hi Bart,
> 
> I'm fine as long as it works for old and new HBAs. I don't have docs to
> 2300/2400 series and the HBAs. Are you sure it follows the same S_ID
> order as 2500?
> 
> Regardless of that, it should work for the last 4 series of the HBAs,
> Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>

Hi Roman,

Thanks for the review. In my copy of the 25xx firmware manual the s_id[]
field has been marked as RESERVED. I have tried not to write into s_id[]
but that was not sufficient to restore point-to-point mode.

Older versions of the qla2xxx driver did not initialize s_id[]. I think
that commit edd05de19759 ("scsi: qla2xxx: Changes to support N2N
logins") is the commit that introduced initialization of s_id[]. Is that
value passed to the target port? Did that commit perhaps introduce code
that checks the value received by the target?

Bart.

Re: [PATCH] Revert "qla2xxx: Fix Nport ID display value"

[Roman Bolshakov]

On Mon, Dec 02, 2019 at 06:34:43PM -0800, Bart Van Assche wrote:
> On 2019-12-02 12:55, Roman Bolshakov wrote:
> > On Mon, Dec 02, 2019 at 08:40:17AM -0800, Bart Van Assche wrote:
> >> diff --git a/drivers/scsi/qla2xxx/qla_iocb.c b/drivers/scsi/qla2xxx/qla_iocb.c
> >> index b25f87ff8cde..e2e91b3f2e65 100644
> >> --- a/drivers/scsi/qla2xxx/qla_iocb.c
> >> +++ b/drivers/scsi/qla2xxx/qla_iocb.c
> >> @@ -2656,10 +2656,16 @@ qla24xx_els_logo_iocb(srb_t *sp, struct els_entry_24xx *els_iocb)
> >>  	els_iocb->port_id[0] = sp->fcport->d_id.b.al_pa;
> >>  	els_iocb->port_id[1] = sp->fcport->d_id.b.area;
> >>  	els_iocb->port_id[2] = sp->fcport->d_id.b.domain;
> >> -	/* For SID the byte order is different than DID */
> >> -	els_iocb->s_id[1] = vha->d_id.b.al_pa;
> >> -	els_iocb->s_id[2] = vha->d_id.b.area;
> >> -	els_iocb->s_id[0] = vha->d_id.b.domain;
> >> +	if (IS_QLA23XX(vha->hw) || IS_QLA24XX(vha->hw) || IS_QLA25XX(vha->hw)) {
> >> +		els_iocb->s_id[0] = vha->d_id.b.al_pa;
> >> +		els_iocb->s_id[1] = vha->d_id.b.area;
> >> +		els_iocb->s_id[2] = vha->d_id.b.domain;
> >> +	} else {
> >> +		/* For SID the byte order is different than DID */
> >> +		els_iocb->s_id[1] = vha->d_id.b.al_pa;
> >> +		els_iocb->s_id[2] = vha->d_id.b.area;
> >> +		els_iocb->s_id[0] = vha->d_id.b.domain;
> >> +	}
> >>
> >>  	if (elsio->u.els_logo.els_cmd == ELS_DCMD_PLOGI) {
> >>  		els_iocb->control_flags = 0;
> > 
> > Hi Bart,
> > 
> > I'm fine as long as it works for old and new HBAs. I don't have docs to
> > 2300/2400 series and the HBAs. Are you sure it follows the same S_ID
> > order as 2500?
> > 
> > Regardless of that, it should work for the last 4 series of the HBAs,
> > Reviewed-by: Roman Bolshakov <r.bolshakov@yadro.com>
> 
> Hi Roman,
> 
> Thanks for the review. In my copy of the 25xx firmware manual the s_id[]
> field has been marked as RESERVED. I have tried not to write into s_id[]
> but that was not sufficient to restore point-to-point mode.
> 

Hi Bart,

IMO that means the field is undocumented rather than RESERVED on 2500
series chips.

> Older versions of the qla2xxx driver did not initialize s_id[]. I think
> that commit edd05de19759 ("scsi: qla2xxx: Changes to support N2N
> logins") is the commit that introduced initialization of s_id[]. Is that
> value passed to the target port? Did that commit perhaps introduce code
> that checks the value received by the target?
> 

Firmware can do implicit login, and this is how it worked for a while.

Then explicit login was introduced in the commit you referenced by
setting bit 8 in IFCB fimwrare options 3 for 2600/2700 series and
issuing ELS IOCB. However, for 2500 series, bit 7 should be set to
disable implicit logins.

The latest commits that touches the bit is 8777e4314d397 ("scsi: qla2xxx:
Migrate NVME N2N handling into state machine"). It sets the bit in
qla24xx_nvram_config regadless of chip.

Does it help to set bit 7 in IFCB, firmware options 3 for 2500 series
and leave the RESERVED S_ID field untouched?

Thanks,
Roman

Re: [PATCH] Revert "qla2xxx: Fix Nport ID display value"

[Bart Van Assche]

On 2019-12-04 04:07, Roman Bolshakov wrote:
> Firmware can do implicit login, and this is how it worked for a while.
> 
> Then explicit login was introduced in the commit you referenced by
> setting bit 8 in IFCB fimwrare options 3 for 2600/2700 series and
> issuing ELS IOCB. However, for 2500 series, bit 7 should be set to
> disable implicit logins.
> 
> The latest commits that touches the bit is 8777e4314d397 ("scsi: qla2xxx:
> Migrate NVME N2N handling into state machine"). It sets the bit in
> qla24xx_nvram_config regadless of chip.
> 
> Does it help to set bit 7 in IFCB, firmware options 3 for 2500 series
> and leave the RESERVED S_ID field untouched?

Hi Roman,

Although I'm not sure whether the patch below is what you had in mind,
it triggers a long sequence of the following messages:

qla2xxx [0000:00:0a.0]-280d:8: HBA in N P2P topology.
qla2xxx [0000:00:0a.0]-2814:8: Configure loop -- dpc flags = 0x1260.
qla2xxx [0000:00:0a.0]-2811:8: Entries in ID list (1).
qla2xxx [0000:00:0a.0]-286a:8: qla2x00_configure_loop *** FAILED ***.
qla2xxx [0000:00:0a.0]-280d:8: HBA in N P2P topology.
qla2xxx [0000:00:0a.0]-2814:8: Configure loop -- dpc flags = 0x1260.
qla2xxx [0000:00:0a.0]-2811:8: Entries in ID list (1).
qla2xxx [0000:00:0a.0]-286a:8: qla2x00_configure_loop *** FAILED ***.

 diff --git a/drivers/scsi/qla2xxx/qla_init.c
b/drivers/scsi/qla2xxx/qla_init.c
index 6c28f38f8021..b7ab1a5d6b0e 100644
--- a/drivers/scsi/qla2xxx/qla_init.c
+++ b/drivers/scsi/qla2xxx/qla_init.c
@@ -7291,8 +7291,14 @@ qla24xx_nvram_config(scsi_qla_host_t *vha)
 	if (ql2xloginretrycount)
 		ha->login_retry_count = ql2xloginretrycount;

-	/* N2N: driver will initiate Login instead of FW */
-	icb->firmware_options_3 |= BIT_8;
+	if (!(IS_QLA23XX(vha->hw) || IS_QLA24XX(vha->hw) ||
+	      IS_QLA25XX(vha->hw))) {
+		/* N2N: driver will initiate login instead of FW */
+		icb->firmware_options_3 |= BIT_8;
+	} else {
+		/* Disable implicit N2N logins */
+		icb->firmware_options_3 |= BIT_7;
+	}

 	/* Enable ZIO. */
 	if (!vha->flags.init_done) {
diff --git a/drivers/scsi/qla2xxx/qla_iocb.c
b/drivers/scsi/qla2xxx/qla_iocb.c
index b25f87ff8cde..7aaabad88cf2 100644
--- a/drivers/scsi/qla2xxx/qla_iocb.c
+++ b/drivers/scsi/qla2xxx/qla_iocb.c
@@ -2656,10 +2656,13 @@ qla24xx_els_logo_iocb(srb_t *sp, struct
els_entry_24xx *els_iocb)
 	els_iocb->port_id[0] = sp->fcport->d_id.b.al_pa;
 	els_iocb->port_id[1] = sp->fcport->d_id.b.area;
 	els_iocb->port_id[2] = sp->fcport->d_id.b.domain;
-	/* For SID the byte order is different than DID */
-	els_iocb->s_id[1] = vha->d_id.b.al_pa;
-	els_iocb->s_id[2] = vha->d_id.b.area;
-	els_iocb->s_id[0] = vha->d_id.b.domain;
+	if (!(IS_QLA23XX(vha->hw) || IS_QLA24XX(vha->hw) ||
+	      IS_QLA25XX(vha->hw))) {
+		/* For SID the byte order is different than DID */
+		els_iocb->s_id[1] = vha->d_id.b.al_pa;
+		els_iocb->s_id[2] = vha->d_id.b.area;
+		els_iocb->s_id[0] = vha->d_id.b.domain;
+	}

 	if (elsio->u.els_logo.els_cmd == ELS_DCMD_PLOGI) {
 		els_iocb->control_flags = 0;

Re: [PATCH] Revert "qla2xxx: Fix Nport ID display value"

[Martin K. Petersen]

>> Firmware can do implicit login, and this is how it worked for a
>> while.
>> 
>> Then explicit login was introduced in the commit you referenced by
>> setting bit 8 in IFCB fimwrare options 3 for 2600/2700 series and
>> issuing ELS IOCB. However, for 2500 series, bit 7 should be set to
>> disable implicit logins.
>> 
>> The latest commits that touches the bit is 8777e4314d397 ("scsi:
>> qla2xxx: Migrate NVME N2N handling into state machine"). It sets the
>> bit in qla24xx_nvram_config regadless of chip.
>> 
>> Does it help to set bit 7 in IFCB, firmware options 3 for 2500 series
>> and leave the RESERVED S_ID field untouched?

Himanshu: Please advise on how to fix this regression.

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [EXT] Re: [PATCH] Revert "qla2xxx: Fix Nport ID display value"

[Himanshu Madhani]

Hi Bart/Martin/Roman,

On 12/9/19, 5:19 PM, "Martin K. Petersen" <martin.petersen@oracle.com> wrote:

    External Email
    
    ----------------------------------------------------------------------
    
    >> Firmware can do implicit login, and this is how it worked for a
    >> while.
    >> 
    >> Then explicit login was introduced in the commit you referenced by
    >> setting bit 8 in IFCB fimwrare options 3 for 2600/2700 series and
    >> issuing ELS IOCB. However, for 2500 series, bit 7 should be set to
    >> disable implicit logins.
    >> 
    >> The latest commits that touches the bit is 8777e4314d397 ("scsi:
    >> qla2xxx: Migrate NVME N2N handling into state machine"). It sets the
    >> bit in qla24xx_nvram_config regadless of chip.
    >> 
    >> Does it help to set bit 7 in IFCB, firmware options 3 for 2500 series
    >> and leave the RESERVED S_ID field untouched?
    
    Himanshu: Please advise on how to fix this regression.
    
I reviewed the series sent earlier this morning which had revised version of this patch.  I am okay with that change (I'll ack on that patch separately from this thread). Basically with newer patch submitted today, we are reverting to older method of handling N2N login for ISP 25xx. 
    -- 
    Martin K. Petersen	Oracle Linux Engineering