On Tue, Dec 10, 2019 at 11:46:17AM +0100, Martin Wilck wrote:
> Hello Bart,
>
> On Mon, 2019-12-09 at 10:02 -0800, Bart Van Assche wrote:
> > Instead of changing endianness in-place and copying the data in two
> > steps,
> > do this in one step. This patch makes is a preparation step for
> > fixing the
> > endianness warnings reported by 'sparse' for the qla2xxx driver.
> >
> > 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.
>
PLOGI payload is returned from FW in little-endian 32-bit words, but ELS
IOCB should have big-endian payload. That's why the conversion happens.
init_cb is only temporary container to get the results from FW.
The big-endian ELS payload is copied from plogi_els_payld.data to
elsio->u.els_plogi.els_plogi_pyld->data in qla24xx_els_dcmd2_iocb()
before being sent to FW/ASIC.
Thanks,
Roman
