Hi Bart/Daniel,
On 3/3/20, 12:36 AM, "linux-scsi-owner@xxxxxxxxxxxxxxx on behalf of Bart Van Assche" <linux-scsi-owner@xxxxxxxxxxxxxxx on behalf of bvanassche@xxxxxxx> wrote:
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.
We had made sure driver works on big endian system and the reason these were not changed was to make sure it does not break big endian architecture. It's been a few years since I verified big endian compatibility with the driver, but I am hesitant to go change things without making sure it works on big endian systems. We are taking big risk of blindly changing things.
I'll ask our testing folks to test this out and see if we discover issue with these changes. Until then I'll hold off on Accepting changes.
>> @@ -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.
