Martin, Please see my replies inline. Thanks, Chaitra -----Original Message----- From: Martin K. Petersen [mailto:martin.petersen@xxxxxxxxxx] Sent: Saturday, April 21, 2018 3:52 AM To: Chaitra P B Cc: linux-scsi@xxxxxxxxxxxxxxx; Sathya.Prakash@xxxxxxxxxxxx; sreekanth.reddy@xxxxxxxxxxxx; suganath-prabu.subramani@xxxxxxxxxxxx Subject: Re: [PATCH v2 01/14] mpt3sas: Bug fix for big endian systems. Chaitra, A few comments: > @@ -426,7 +427,7 @@ static void _clone_sg_entries(struct MPT3SAS_ADAPTER *ioc, > dst_addr_phys = _base_get_chain_phys(ioc, > smid, sge_chain_count); > WARN_ON(dst_addr_phys > U32_MAX); > - sgel->Address = (u32)dst_addr_phys; > + sgel->Address = cpu_to_le32((u32)dst_addr_phys); I tend to prefer lower_32_bits() but that's your choice. <Chaitra> Accepted, lower_32_bits() can be used here. > @@ -3040,8 +3047,9 @@ mpt3sas_base_map_resources(struct MPT3SAS_ADAPTER *ioc) > } > > for (i = 0; i < ioc->combined_reply_index_count; i++) { > - ioc->replyPostRegisterIndex[i] = (resource_size_t *) > - ((u8 *)&ioc->chip->Doorbell + > + ioc->replyPostRegisterIndex[i] = > + (volatile void __iomem *) > + ((u8 __force *)&ioc->chip->Doorbell + > MPI25_SUP_REPLY_POST_HOST_INDEX_OFFSET + > (i * MPT3_SUP_REPLY_POST_HOST_INDEX_REG_OFFSET)); Do you really need volatile here? The existing resource_size_t didn't imply volatile. Why the double type casts? You've already changed replyPostRegisterIndex to be 'volatile void __iomem **' in the header file. So why not: ioc->replyPostRegisterIndex[i] = &ioc->chip->Doorbell + MPI25_SUP_REPLY_POST_HOST_INDEX_OFFSET + i * MPT3_SUP_REPLY_POST_HOST_INDEX_REG_OFFSET; Also looks like ioc->reply_post_host_index handling a few lines further down could lose the type casts. <Chaitra> Accepted, volatile is not really needed. I shall remove volatile. > @@ -3386,7 +3394,7 @@ _base_put_smid_mpi_ep_scsi_io(struct MPT3SAS_ADAPTER *ioc, u16 smid, u16 handle) > __le32 *mfp = (__le32 *)mpt3sas_base_get_msg_frame(ioc, smid); > > _clone_sg_entries(ioc, (void *) mfp, smid); > - mpi_req_iomem = (void *)ioc->chip + > + mpi_req_iomem = (void __force *)ioc->chip + > MPI_FRAME_START_OFFSET + (smid * ioc->request_sz); > _base_clone_mpi_to_sys_mem(mpi_req_iomem, (void *)mfp, > ioc->request_sz); Wouldn't it be better to add __iomem to the definition of mpi_req_iomem? <Chaitra> With this change I still see the below warnings: warning: cast removes address space of expression warning: incorrect type in assignment (different address spaces) expected void [noderef] <asn:2>*mpi_req_iomem got void * warning: incorrect type in argument 1 (different address spaces) expected void *dst_iomem got void [noderef] <asn:2>*mpi_req_iome > + nvme_encap_request->ErrorResponseBaseAddress = > + cpu_to_le64(ioc->sense_dma & 0xFFFFFFFF00000000UL); upper_32_bits()? <Chaitra> since upper_32_bits() returns only upper 32 bits. But here after bitwise & below we are doing bitwise | with dma_address lower 32 bits , so in this case use of upper_32_bits() will yield wrong address for below assignment. Hence upper_32_bits() can't be used. nvme_encap_request->ErrorResponseBaseAddress |= cpu_to_le64(le32_to_cpu( mpt3sas_base_get_sense_buffer_dma(ioc, smid))); -- Martin K. Petersen Oracle Linux Engineering
