Re: [RFC] results of endianness review of qla2xxx

[Al Viro <viro@ZenIV.linux.org.uk>]

On Fri, Apr 18, 2008 at 11:17:58AM -0700, Andrew Vasquez wrote:
> > 7) qla2x00_async_event():
> > 		handles[0] = le32_to_cpu((uint32_t)((mb[2] << 16) | mb[1]));
> > 		...
> > 		handles[0] = le32_to_cpu((uint32_t)((mb[2] << 16) | mb[1]));
> > 		handles[1] = le32_to_cpu(
> > 		    ((uint32_t)(RD_MAILBOX_REG(ha, reg, 7) << 16)) |
> > and then those values are passed to qla2x00_process_completed_request() which
> > expects a host-endian (and uses it as index in array, among other things).
> 
> THis is correct (though a bit confuscated), each of the two 16bit
> mailbox-registers makes-up a 32bit LE value, which is converted to
> cpu-endian and passed to qla2x00_process_completed_request().

Um, no.  First of all, other branches in that switch clearly assume
that mb[] is already host-endian.  But leaving that aside, what you
are doing here will _not_ work even if mb[1] and mb[2] are l-e in
this case.

Look what happens:
* l-e host:
	handles[0] = (mb[2] << 16) | mb[1], which is equal to
(le16_to_cpu(mb[2]) << 16) | le16_to_cpu(mb[1])
* on b-e host:
	handles[0] = swab32((mb[2] << 16) | mb[1]), which is equal to
(swab16(mb[1]) << 16) | swab16(mb[2]), which is equal to
(le16_to_cpu(mb[1]) << 16) | le16_to_cpu(mb[2])

IOW, resulting host-endian value will be different with the same contents
in mb[].  Put it another way, suppose you have mb[1] == 0, mb[2] == 0x101.
Then on l-e you'll get handles[0] == 0x10100 and on b-e - 0x101.  The latter
will pass the check for index being too high, the former will fail...

BTW, RD_MAILBOX_REG() boils down to readw(), which is definitely host-endian.
And we have
                        mb[0] = MBA_SCSI_COMPLETION;
                        mb[1] = MSW(stat);
                        mb[2] = RD_MAILBOX_REG(ha, reg, 2);
                        qla2x00_async_event(ha, mb);
so there's at least one path reaching qla2x00_async_event() with mb[0]
equal to MBA_SCSI_COMPLETION and mb[2] containing host-endian...  Looking
at the contents of stat and the things we do to it (switch (stat & 0xff) right
outside that code), mb[1] is host-endian as well...  AFAICS, the same holds
for all codepaths - contents of mb[] is host-endian.

> > 9) qla2x00_clear_nvram_protection():
> >         wprot_old = cpu_to_le16(qla2x00_get_nvram_word(ha, ha->nvram_base));
> > 	stat = qla2x00_write_nvram_word_tmo(ha, ha->nvram_base,
> > 		    __constant_cpu_to_le16(0x1234), 100000);
> > 	wprot = cpu_to_le16(qla2x00_get_nvram_word(ha, ha->nvram_base));
> > 	if (stat != QLA_SUCCESS || wprot != 0x1234) {
> > Odd, since qla2x00_get_nvram_word() returns a host-endian and 
> > qla2x00_write_nvram_word_tmo() expects one (this stuff is bit-banging
> > 16bit values through).
> 
> Odd, yes.  I was never happy with this endianess intermixing, but
> after numerous cycles within our qual-testing, this code works in both
> big and little-endian systems...

It looks like check that location is writable; what happens if you replace
the first instance with 0x1234 and the second (in wprot == ...) with
cpu_to_le16(0x1234)?  One "we won't find it there by accident, must've been
successful write" value seems to be as good as other, unless something else
is going on...