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

[Roman Bolshakov <r.bolshakov@yadro.com>]

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