On 2020-05-07 02:05, Hannes Reinecke wrote:
> On 5/7/20 6:28 AM, Bart Van Assche wrote:
>> @@ -2679,8 +2680,8 @@ qla24xx_els_logo_iocb(srb_t *sp, struct
>> els_entry_24xx *els_iocb)
>> els_iocb->sys_define = 0;
>> els_iocb->entry_status = 0;
>> els_iocb->handle = sp->handle;
>> - els_iocb->nport_handle = cpu_to_le16(sp->fcport->loop_id);
>> - els_iocb->tx_dsd_count = 1;
>> + els_iocb->nport_handle = sp->fcport->loop_id;
>> + els_iocb->tx_dsd_count = cpu_to_le16(1);
>> els_iocb->vp_index = vha->vp_idx;
>> els_iocb->sof_type = EST_SOFI3;
>> els_iocb->rx_dsd_count = 0;
>
> Why did you drop the cpu_to_le16 for the loop_id?
> I was under the impression we'll store it in machine-native format,
> don't we?
Thanks for having pointed this out. I will restore the cpu_to_le16()
call and annotate els_iocb->nport_handle as little endian.
>> @@ -3022,7 +3023,7 @@ qla24xx_els_iocb(srb_t *sp, struct
>> els_entry_24xx *els_iocb)
>> els_iocb->sys_define = 0;
>> els_iocb->entry_status = 0;
>> els_iocb->handle = sp->handle;
>> - els_iocb->nport_handle = cpu_to_le16(sp->fcport->loop_id);
>> + els_iocb->nport_handle = sp->fcport->loop_id;
>> els_iocb->tx_dsd_count =
>> cpu_to_le16(bsg_job->request_payload.sg_cnt);
>> els_iocb->vp_index = sp->vha->vp_idx;
>> els_iocb->sof_type = EST_SOFI3;
>
> Same here.
I will restore this cpu_to_le16() call too.
>> @@ -1817,23 +1817,22 @@ qla24xx_els_ct_entry(scsi_qla_host_t *vha,
>> struct req_que *req,
>> }
>> comp_status = fw_status[0] = le16_to_cpu(pkt->comp_status);
>> - fw_status[1] = le16_to_cpu(((struct els_sts_entry_24xx
>> *)pkt)->error_subcode_1);
>> - fw_status[2] = le16_to_cpu(((struct els_sts_entry_24xx
>> *)pkt)->error_subcode_2);
>> + fw_status[1] = le32_to_cpu(ese->error_subcode_1);
>> + fw_status[2] = le32_to_cpu(ese->error_subcode_2);
>> if (iocb_type == ELS_IOCB_TYPE) {
>> els = &sp->u.iocb_cmd;
>> els->u.els_plogi.fw_status[0] = fw_status[0];
>> els->u.els_plogi.fw_status[1] = fw_status[1];
>> els->u.els_plogi.fw_status[2] = fw_status[2];
>> - els->u.els_plogi.comp_status = fw_status[0];
>> + els->u.els_plogi.comp_status = cpu_to_le16(fw_status[0]);
>
> ???
> Why only this line?
> fw_status is kept in host-endianness; shouldn't all of the above
> assignments being done with cpu_to_le16?
Will fix this too.
>> @@ -1842,8 +1841,7 @@ qla24xx_els_ct_entry(scsi_qla_host_t *vha,
>> struct req_que *req,
>> ql_dbg(ql_dbg_user, vha, 0x503f,
>> "ELS IOCB Done -%s error hdl=%x comp_status=0x%x error
>> subcode 1=0x%x error subcode 2=0x%x total_byte=0x%x\n",
>> type, sp->handle, comp_status, fw_status[1], fw_status[2],
>> - le16_to_cpu(((struct els_sts_entry_24xx *)
>> - pkt)->total_byte_count));
>> + le32_to_cpu(ese->total_byte_count));
>> goto els_ct_done;
>> }
>>
>
> Switch from 16 to 32 bits?
total_byte_count is a 32-bits variable, hence le32_to_cpu() instead of
le16_to_cpu(). Sparse checks whether the proper conversion function is used.
>> ql_dbg(ql_dbg_user, vha, 0x5040,
>> "ELS-CT pass-through-%s error hdl=%x
>> comp_status-status=0x%x "
>> "error subcode 1=0x%x error subcode 2=0x%x.\n",
>> type, sp->handle, comp_status,
>> - le16_to_cpu(((struct els_sts_entry_24xx *)
>> - pkt)->error_subcode_1),
>> - le16_to_cpu(((struct els_sts_entry_24xx *)
>> - pkt)->error_subcode_2));
>> + le32_to_cpu(ese->error_subcode_1),
>> + le32_to_cpu(ese->error_subcode_2));
>> res = DID_ERROR << 16;
>> bsg_reply->reply_payload_rcv_len = 0;
>> }
>
> Same here.
As you may know error_subcode_1 and 2 are 32-bits variables.
>> @@ -3110,8 +3109,8 @@ qla24xx_get_isp_stats(scsi_qla_host_t *vha,
>> struct link_statistics *stats,
>> mc.mb[6] = MSW(MSD(stats_dma));
>> mc.mb[7] = LSW(MSD(stats_dma));
>> mc.mb[8] = dwords;
>> - mc.mb[9] = cpu_to_le16(vha->vp_idx);
>> - mc.mb[10] = cpu_to_le16(options);
>> + mc.mb[9] = vha->vp_idx;
>> + mc.mb[10] = options;
>> rval = qla24xx_send_mb_cmd(vha, &mc);
>>
>
> Why has the converstion been dropped here?
> 'vp_idx' surely is in machine-native endianness?
I think that his has been explained in the description of this patch:
"Annotate the mb[] arrays as CPU-endian because communication of the
mb[] values with the hardware happens through the readw() and writew()
functions. readw() converts from __le16 to u16 and writew() converts
from u16 to __le16."
>> @@ -4743,7 +4742,7 @@ qla82xx_set_driver_version(scsi_qla_host_t *vha,
>> char *version)
>> mcp->mb[1] = RNID_TYPE_SET_VERSION << 8;
>> mcp->out_mb = MBX_1|MBX_0;
>> for (i = 4; i < 16 && len; i++, str++, len -= 2) {
>> - mcp->mb[i] = cpu_to_le16p(str);
>> + mcp->mb[i] = le16_to_cpup(str);
>> mcp->out_mb |= 1<<i;
>> }
>> for (; i < 16; i++) {
>
> That looks _soo_ wrong.
> The mailbox is most likely in firmware/HBA endianness, so why the
> conversion?
Same explanation as above: rd_reg_word() is used to read mailbox
registers. Hence, mcp->mb[] has CPU-endianness.
>> break;
>> case MBA_REJECTED_FCP_CMD:
>> ql_dbg(ql_dbg_tgt_mgt, vha, 0xf017,
>> "qla_target(%d): Async event LS_REJECT occurred
>> (m[0]=%x, m[1]=%x, m[2]=%x, m[3]=%x)",
>> vha->vp_idx,
>> - le16_to_cpu(mailbox[0]), le16_to_cpu(mailbox[1]),
>> - le16_to_cpu(mailbox[2]), le16_to_cpu(mailbox[3]));
>> + mailbox[0], mailbox[1], mailbox[2], mailbox[3]);
>> - if (le16_to_cpu(mailbox[3]) == 1) {
>> + if (mailbox[3] == 1) {
>> /* exchange starvation. */
>> vha->hw->exch_starvation++;
>> if (vha->hw->exch_starvation > 5) {
>
> I would argue that we should keep the conversions here; otherwise the
> mailbox contents will be really hard to read on big endian.
Doesn't rd_reg_word() convert already from little endian to CPU endian?
>> @@ -6729,7 +6729,7 @@ qlt_init_atio_q_entries(struct scsi_qla_host *vha)
>> return;
>> for (cnt = 0; cnt < ha->tgt.atio_q_length; cnt++) {
>> - pkt->u.raw.signature = ATIO_PROCESSED;
>> + pkt->u.raw.signature = cpu_to_le32(ATIO_PROCESSED);
>> pkt++;
>> }
>> @@ -6764,7 +6764,7 @@ qlt_24xx_process_atio_queue(struct
>> scsi_qla_host *vha, uint8_t ha_locked)
>> "corrupted fcp frame SID[%3phN] OXID[%04x] EXCG[%x]
>> %64phN\n",
>> &pkt->u.isp24.fcp_hdr.s_id,
>> be16_to_cpu(pkt->u.isp24.fcp_hdr.ox_id),
>> - le32_to_cpu(pkt->u.isp24.exchange_addr), pkt);
>> + pkt->u.isp24.exchange_addr, pkt);
>> adjust_corrupted_atio(pkt);
>> qlt_send_term_exchange(ha->base_qpair, NULL, pkt,
>
> Why did you drop the conversion for the exchange address?
> Is it in machine native format already?
I will restore that conversion and I will annotate the exchange
addresses as little endian. Apparently that got overlooked.
>> diff --git a/drivers/scsi/qla2xxx/qla_tmpl.c
>> b/drivers/scsi/qla2xxx/qla_tmpl.c
>> index 3f52d5af3e8a..d241929d6dd5 100644
>> --- a/drivers/scsi/qla2xxx/qla_tmpl.c
>> +++ b/drivers/scsi/qla2xxx/qla_tmpl.c
>> @@ -922,9 +922,9 @@ qla27xx_firmware_info(struct scsi_qla_host *vha,
>> tmp->firmware_version[0] = vha->hw->fw_major_version;
>> tmp->firmware_version[1] = vha->hw->fw_minor_version;
>> tmp->firmware_version[2] = vha->hw->fw_subminor_version;
>> - tmp->firmware_version[3] = cpu_to_le32(
>> + tmp->firmware_version[3] = (__force u32)cpu_to_le32(
>> vha->hw->fw_attributes_h << 16 | vha->hw->fw_attributes);
>> - tmp->firmware_version[4] = cpu_to_le32(
>> + tmp->firmware_version[4] = (__force u32)cpu_to_le32(
>> vha->hw->fw_attributes_ext[1] << 16 |
>> vha->hw->fw_attributes_ext[0]);
>> }
>>
> In general this is really hard to review, as you move the function
> arguments around together with the missing/wrong annotations.
> Can't you split it off into one patch for the missing annotations and
> another one which moves the function argument (and annotations)?
> (Or, maybe, the other way around; the first one moving the function
> arguments and annotations touched by this, and the second one for
> the remaining fixes...)
Did I really move function arguments around in this patch? This patch
should only includes changes related to endianness.
Thanks for the review,
Bart.
