Re: [PATCH 3/4] qla2xxx: Fix endianness annotations in source files

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

On 2020-03-02 10:40, Daniel Wagner wrote:
> On Sun, Mar 01, 2020 at 07:30:22PM -0800, Bart Van Assche wrote:
>> Fix all endianness complaints reported by sparse (C=2).
> 
> [...]
> 
>>  int
>> -qla24xx_dump_ram(struct qla_hw_data *ha, uint32_t addr, uint32_t *ram,
>> -    uint32_t ram_dwords, void **nxt)
>> +qla24xx_dump_ram(struct qla_hw_data *ha, uint32_t addr, __be32 *ram,
>> +		 uint32_t ram_dwords, void **nxt)
>>  {
>>  	int rval = QLA_FUNCTION_FAILED;
>>  	struct device_reg_24xx __iomem *reg = &ha->iobase->isp24;
>>  	dma_addr_t dump_dma = ha->gid_list_dma;
>> -	uint32_t *chunk = (void *)ha->gid_list;
>> +	uint32_t *chunk = (uint32_t *)ha->gid_list;
>>  	uint32_t dwords = qla2x00_gid_list_size(ha) / 4;
>>  	uint32_t stat;
>>  	ulong i, j, timer = 6000000;
>> @@ -252,9 +252,9 @@ qla24xx_dump_ram(struct qla_hw_data *ha, uint32_t addr, uint32_t *ram,
>>  			return rval;
>>  		}
>>  		for (j = 0; j < dwords; j++) {
>> -			ram[i + j] =
>> -			    (IS_QLA27XX(ha) || IS_QLA28XX(ha)) ?
>> -			    chunk[j] : swab32(chunk[j]);
>> +			ram[i + j] = (__force __be32)
>> +				((IS_QLA27XX(ha) || IS_QLA28XX(ha)) ?
>> +				 chunk[j] : swab32(chunk[j]));
> 
> Isn't this assuming the host runs in little endian mode? Because later down...

My goal was not to change the behavior of the code on x86. Bugs on big
endian systems can be fixed later on (my guess is that this driver does
not work reliably on big endian), and searching through the code for
__force casts probably provides some good starting points.

>> @@ -439,7 +439,7 @@ qla2xxx_dump_ram(struct qla_hw_data *ha, uint32_t addr, uint16_t *ram,
>>  		if (test_and_clear_bit(MBX_INTERRUPT, &ha->mbx_cmd_flags)) {
>>  			rval = mb0 & MBS_MASK;
>>  			for (idx = 0; idx < words; idx++)
>> -				ram[cnt + idx] = swab16(dump[idx]);
>> +				ram[cnt + idx] = cpu_to_be16(le16_to_cpu(dump[idx]));
> 
> ... cpu_to_be16() is used.

The above code implements swab16() but without triggering sparse
endianness warnings.

>>  		if (!*srisc_addr) {
>>  			*srisc_addr = risc_addr;
>> -			risc_attr = be32_to_cpu(dcode[9]);
>> +			risc_attr = swab32(dcode[9]);
>>  		}
> 
> also here, this looks like hardcoded endianess.

It was not clear to me whether the purpose of the code was to convert
from __be32 to CPU endianness or from __be32 to __le32.

>> @@ -3398,7 +3399,7 @@ qla82xx_start_scsi(srb_t *sp)
>>  
>>  		memcpy(ctx->fcp_cmnd->cdb, cmd->cmnd, cmd->cmd_len);
>>  
>> -		fcp_dl = (uint32_t *)(ctx->fcp_cmnd->cdb + 16 +
>> +		fcp_dl = (void *)(ctx->fcp_cmnd->cdb + 16 +
>>  		    additional_cdb_len);
> 
> Shouldn't this be (__be32*)?

Good catch. I will change the cast.

>> @@ -3537,7 +3540,7 @@ qla24xx_get_flash_version(scsi_qla_host_t *vha, void *mbuf)
>>  	}
>>  
>>  	for (i = 0; i < 4; i++)
>> -		ha->gold_fw_version[i] = be32_to_cpu(dcode[4+i]);
>> +		ha->gold_fw_version[i] = swab32(dcode[4+i]);
>>  
>>  	return ret;
>>  }
> 
> Here again why the swab32() call.
I will restore the be32_to_cpu() call.

Bart.