Re: [PATCH 4/4] qla2xxx: Micro-optimize qla2x00_configure_local_loop()

[Bart Van Assche <bvanassche@acm.org>]

On 2019-12-10 02:46, Martin Wilck wrote:
> On Mon, 2019-12-09 at 10:02 -0800, Bart Van Assche wrote:
>> diff --git a/drivers/scsi/qla2xxx/qla_init.c b/drivers/scsi/qla2xxx/qla_init.c
>> index 6c28f38f8021..ddd8bf7997a8 100644
>> --- a/drivers/scsi/qla2xxx/qla_init.c
>> +++ b/drivers/scsi/qla2xxx/qla_init.c
>> @@ -5047,13 +5047,12 @@ qla2x00_configure_local_loop(scsi_qla_host_t *vha)
>>  			rval = qla24xx_get_port_login_templ(vha,
>>  			    ha->init_cb_dma, (void *)ha->init_cb, sz);
>>  			if (rval == QLA_SUCCESS) {
>> +				__be32 *q = &ha->plogi_els_payld.data[0];
>> +
>>  				bp = (uint32_t *)ha->init_cb;
>> -				for (i = 0; i < sz/4 ; i++, bp++)
>> -					*bp = cpu_to_be32(*bp);
>> +				for (i = 0; i < sz/4 ; i++, bp++, q++)
>> +					*q = cpu_to_be32(*bp);
>>  
>> -				memcpy(&ha->plogi_els_payld.data,
>> -				    (void *)ha->init_cb,
>> -				    sizeof(ha->plogi_els_payld.data));
>>  				set_bit(RELOGIN_NEEDED, &vha->dpc_flags);
>>  			} else {
>>  				ql_dbg(ql_dbg_init, vha, 0x00d1,
> 
> A side effect of this patch would be that after return from
> qla2x00_configure_local_loop(), the byte ordering in ha->init_cb
> remains in CPU byte ordering, whereas before your patch, it would have
> been converted to be32. I'm uncertain if that would matter later on.
> 
> The following is not a problems with your patch, but what's really
> weird is that in qla24xx_get_port_login_templ() (which is only called
> from here), the buffer is converted from le32 to CPU endianness, and
> then here, in a second step, from CPU to be32. I'm wondering which byte
> order this buffer is supposed to have, and whether that's different
> depending on which mode the controller is operating in (the be32
> conversion seems to be applied in N2N mode only). Moreover, looking at
> the definition of init_cb_t in qla_def.h, this data structure actually
> has mixed endianness, making me doubt that changing the endianness of
> the whole buffer makes sense at all. Or is ha->init_cb simply being
> abused in this part of the code?
> 
> I guess only Himanshu or Quinn can tell.

Hi Martin,

I will rework this patch such that this function again changes the
ha->init_cb buffer into big endian byte order.

Bart.