results of endianness review of qla2xxx

Assorted endianness problems and uncertain places:

1) In qla24xx_nvram_config():

	    nv->nvram_version < __constant_cpu_to_le16(ICB_VERSION)) {

comparisons work better in host-endian...

2) Same function:

	icb->login_timeout = cpu_to_le16(nv->login_timeout);

both are little-endian

3) qla24xx_login_fabric():

	lg->vp_index = cpu_to_le16(ha->vp_idx);

->vp_index is 8bit, ha->vp_idx is host-endian (and small)

4) qla24xx_fabric_logout():
same story.

5) qla24xx_report_id_acquisition():

	if (rptid_entry->entry_status != 0)
		return;
	if (rptid_entry->entry_status != __constant_cpu_to_le16(CS_COMPLETE))
		return;

For one thing, ->entry_status is 8bit. For another, CS_COMPLETE is 0, so this pair of checks looks bogus anyway.

6) Same function:

		vp_idx = LSB(rptid_entry->vp_idx);
->vp_idx is little-endian 16bit; LSB expects host-endian
		if (MSB(rptid_entry->vp_idx) == 1)

... and so does MSB.

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).

8) qla2x00_fdmi_rpa():

	max_frame_size = IS_FWI2_CAPABLE(ha) ?
		(uint32_t) icb24->frame_payload_size:
		(uint32_t) ha->init_cb->frame_payload_size;
	eiter->a.max_frame_size = cpu_to_be32(max_frame_size);

and AFAICS ->frame_payload_size is little-endian 16bit.

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).

10)

typedef struct vf_id {
        uint16_t id : 12;
	uint16_t priority : 4;
} vf_id_t;

and

typedef struct vf_hopct {
	uint16_t reserved : 8;
	uint16_t hopct : 8;
} vf_hopct_t;

Buggered, since (a) vf_id is severely endian-dependent and (b) both will happily get padding on e.g. arm; since they are embedded into hardware-shared structures, extra 16 bits tacked onto each is Not Good(tm).

11) The same changeset that had introduced vf_id has a related oddity:

-       uint8_t reserved_4[32];
+       uint16_t flags;
+       struct vf_id    id;
+       uint16_t reserved_4;
+       struct vf_hopct  hopct;
+       uint8_t reserved_5[8];

had been bogus - 2 + 2 + 2 + 2 + 8 != 32; 16 bytes got cut.

Thanks, we'll take a look at these and provide relevant fixes.

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().

Yes indeed, le16_to_cpu() need to surround the
init_cb/icb24->frame_payload_size references.

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...

We'll look into these as well...

Thanks,
AV

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.

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...

Yes, all mb[] "values" are going to be in host-endian format.

But, what's missing here is an understanding of what mb[2] and mb[1] are representing during MBA_SCSI_COMPLETION and MBA_CMPLT_2_32BIT handling. Upon command submission, a 32bit handle (cookie) is associated with each IOCB:

	...
	cmd_pkt = (cmd_entry_t *)ha->request_ring_ptr;
	cmd_pkt->handle = handle;
	/* Zero out remaining portion of packet. */
	clr_ptr = (uint32_t *)cmd_pkt + 2;
	memset(clr_ptr, 0, REQUEST_ENTRY_SIZE - 8);
	cmd_pkt->dseg_count = cpu_to_le16(tot_dsds);

	/* Set target ID and LUN number*/
	SET_TARGET_ID(ha, cmd_pkt->target, sp->fcport->loop_id);
	cmd_pkt->lun = cpu_to_le16(sp->cmd->device->lun);
	...

Notice that the 'handle'-value is assigned to the command-IOCB structure in host-endian format and *not* the typical little-endian form used to fill-in the other non-byte-sized members of the structure. So, for example, start with a hypothetical 'handle' value of 0x12345678. On an BE machine, the layout in memory of the value written to the structure will be:

	mem[0]	mem[1]	mem[2]	mem[3]
	------	------	-----	------
	12	34	56	78

On an LE machine:

	mem[0]	mem[1]	mem[2]	mem[3]
	------	------	-----	------
	78	56	34	12

'handle' sent as a host-endian value is done for a reason, as typically, firmware reports IOCB completion via a structure called the status-IOCB [2]:

#define STATUS_TYPE 0x03 /* Status entry. */
typedef struct {
uint8_t entry_type; /* Entry type. */
uint8_t entry_count; /* Entry count. */
uint8_t sys_define; /* System defined. */
uint8_t entry_status; /* Entry Status. */
uint32_t handle; /* System handle. */
uint16_t scsi_status; /* SCSI status. */
...

where, during creation of the status-IOCB by firmware, the 'handle'-value (at offset 0x4) is copied from the 'handle' specified from the command-IOCB. Since this 'handle' is a a simple index (in host-endian format) into the outstanding_cmds array of the HBA, there's really no reason to convert the value to little-endian format at submission time, then convert from little-endian to host-endian form during status-IOCB handling within the driver [3]:

	...
	/* Fast path completion. */
	if (comp_status == CS_COMPLETE && scsi_status == 0) {
		qla2x00_process_completed_request(ha, sts->handle);

		return;
	}

So, getting back to MBA_SCSI_COMPLETION and MBA_CMPLT_2_32BIT completions, the "value" of the mailbox registers returned by firmware are:

	mb[1]: mem[1] << 8 | mem[0]
	mb[2]: mem[3] << 8 | mem[2]

which on an LE machine result in:

	mb[1]: 0x5678
	mb[2]: 0x1234

	handle[0] = le32_to_cpu((uint32_t)((mb[2] << 16) | mb[1]));
	handle[0] = (0x1234 << 16) | 0x5678)
	handle[0] = 0x12345678

and on an BE machine the values are:

	mb[1]: 0x3412
	mb[2]: 0x7856

	handle[0] = le32_to_cpu((uint32_t)((mb[2] << 16) | mb[1]));
	handle[0] = swab32((0x7856 << 16) | 0x3412)
	handle[0] = swab32(0x78563412)
	handle[0] = 0x12345678

One would hope so, yes...

Anyway, hope that helped...

--
av

[1] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/scsi/qla2xxx/qla_iocb.c;h=5489d5024673a326b5345f5545151a7dab37a533;hb=HEAD#l346

[2] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/scsi/qla2xxx/qla_def.h;h=094d95f0764c6027cb421e4d3e5da9bf17012dfd;hb=HEAD#l1259

[3] http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=blob;f=drivers/scsi/qla2xxx/qla_isr.c;h=285479b62d8f02084ad0daebc12ab249f16348c0;hb=HEAD#l944

More of that stuff (added by [SCSI] qla2xxx: Add hardware
trace-logging support):

                        qla24xx_write_flash_dword(ha, faddr++,
                            cpu_to_le32(jiffies));
                        qla24xx_write_flash_dword(ha, faddr++, 0);
                        qla24xx_write_flash_dword(ha, faddr++, *fdata++);
                        qla24xx_write_flash_dword(ha, faddr++, *fdata);

in qla2xxx_hw_event_store(). All other callers pass host-endian last argument and function sure as hell looks like it's expecting one...

... and while we are at it,

                        for (miter = 0, s = optrom, d = dwptr;
                            miter < OPTROM_BURST_DWORDS; miter++, s++, d++)
                                *s = cpu_to_le32(*d);

                        ret = qla2x00_load_ram(ha, optrom_dma,
                            flash_data_to_access_addr(faddr),
                            OPTROM_BURST_DWORDS);

in qla24xx_write_flash_data() looks bloody odd. We have dwptr pointing to fixed-endian data, so conversion gets us host-endian and then we pass the area filled with results of conversion for DMA?

And yes, dwptr points to fixed-endian - it comes from qla24xx_write_optrom_data(struct scsi_qla_host *ha, uint8_t *buf,
uint32_t offset, uint32_t length)
{
...

        rval = qla24xx_write_flash_data(ha, (uint32_t *)buf, offset >> 2,
            length >> 2);

Comments?

Sure, you're not understanding the format of the various pieces of data as they flow through the driver (often times opaquely)...

Look at this code from qla_init.c:qla24xx_load_risc_flash():

	...
	qla24xx_read_flash_data(ha, dcode, faddr, dlen);
	for (i = 0; i < dlen; i++)
		dcode[i] = swab32(dcode[i]);

	rval = qla2x00_load_ram(ha, ha->request_dma, risc_addr, dlen);
	...

why the explicit swab32()? Because at production time the data was written in big-endian format. Yet, when the driver used to carry the firmware within the driver, the FW folks built the corresponding .C file of the array of 32-bit words in little-endian format... There's a similar case for the code in qla24xx_load_risc() where the .bin file read via request_firmware() was generated in big-endian format... Why? Well...sometimes "stuff" happens and we accept things...

Look, rather than pick apart (the rat hole) each access-region within flash and determine which endian-format is used during read and write, let me just say that we'll continue to be extra-cautious when adding features which interact with the ISPs little-endian RISC and various BE/LE regions of flash...

Thanks again for your endianess-scrubbing of qla2xxx, I do appreciate your time and efforts and should have patches submitted shortly for much of the 'badness' reported.

Thanks,
Andrew Vasquez

The data portions of 'fdata[]' are composites of:

	#define QMARK(a, b, c, d) \
	    cpu_to_le32(LSB(a) << 24 | LSB(b) << 16 | LSB(c) << 8 | LSB(d))
	...
	/* Create marker. */
	marker[0] = QMARK('L', ha->fw_major_version,
	    ha->fw_minor_version, ha->fw_subminor_version);
	marker[1] = QMARK(QLA_DRIVER_MAJOR_VER, QLA_DRIVER_MINOR_VER,
	    QLA_DRIVER_PATCH_VER, QLA_DRIVER_BETA_VER);

and:

        /* Store error.  */
        fdata[0] = cpu_to_le32(code << 16 | d1);
        fdata[1] = cpu_to_le32(d2 << 16 | d3);
        rval = qla2xxx_hw_event_store(ha, fdata);

I'm not entirely clear on what you are driving at here... The data in question is written to flash in little-endian format...