Re: [RFC] results of endianness review of qla2xxx

[Andrew Vasquez <andrew.vasquez@qlogic.com>]

On Wed, 16 Apr 2008, Al Viro wrote:

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

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

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

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

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

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


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

We'll look into these as well...

Thanks,
AV