Re: [PATCH v2 01/14] mpt3sas: Bug fix for big endian systems.
On Wed, May 2, 2018 at 9:21 AM, Martin K. Petersen
wrote:
>
> Hi Chaitra,
>
>>> 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));
>>
>> 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;
>
> You didn't address my question about why the type casts were required in
> the first place? I get cautious when I see nested casting...
>
Martin,
In v3 patch,we have removed this nested casting and just used (u8
__force) to fix the sparse warning.
~ Sreekanth
>> Also looks like ioc->reply_post_host_index handling a few lines further
>> down could lose the type casts.
>
> See above.
>
> --
> Martin K. Petersen Oracle Linux Engineering
Re: [PATCH v2 01/14] mpt3sas: Bug fix for big endian systems.
Hi Chaitra,
>> 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));
>
> 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;
You didn't address my question about why the type casts were required in
the first place? I get cautious when I see nested casting...
> Also looks like ioc->reply_post_host_index handling a few lines further
> down could lose the type casts.
See above.
--
Martin K. Petersen Oracle Linux Engineering
RE: [PATCH v2 01/14] mpt3sas: Bug fix for big endian systems.
Martin,
Please see my replies inline.
Thanks,
Chaitra
-Original Message-
From: Martin K. Petersen [mailto:martin.peter...@oracle.com]
Sent: Saturday, April 21, 2018 3:52 AM
To: Chaitra P B
Cc: linux-scsi@vger.kernel.org; sathya.prak...@broadcom.com;
sreekanth.re...@broadcom.com; suganath-prabu.subram...@broadcom.com
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.
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.
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?
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] *mpi_req_iomem
got void *
warning: incorrect type in argument 1 (different address
spaces)
expected void *dst_iomem
got void [noderef] *mpi_req_iome
> + nvme_encap_request->ErrorResponseBaseAddress =
> + cpu_to_le64(ioc->sense_dma & 0xUL);
upper_32_bits()?
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
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.
> @@ -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.
> @@ -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?
> + nvme_encap_request->ErrorResponseBaseAddress =
> + cpu_to_le64(ioc->sense_dma & 0xUL);
upper_32_bits()?
--
Martin K. Petersen Oracle Linux Engineering
[PATCH v2 01/14] mpt3sas: Bug fix for big endian systems.
This patch fixes bug for big endian systems.
Signed-off-by: Chaitra P B
Signed-off-by: Suganath Prabu S
---
drivers/scsi/mpt3sas/mpi/mpi2_init.h | 2 +-
drivers/scsi/mpt3sas/mpt3sas_base.c | 57 +-
drivers/scsi/mpt3sas/mpt3sas_base.h | 6 ++--
drivers/scsi/mpt3sas/mpt3sas_ctl.c | 11 +++---
drivers/scsi/mpt3sas/mpt3sas_scsih.c | 59 +++-
drivers/scsi/mpt3sas/mpt3sas_warpdrive.c | 3 +-
6 files changed, 73 insertions(+), 65 deletions(-)
diff --git a/drivers/scsi/mpt3sas/mpi/mpi2_init.h
b/drivers/scsi/mpt3sas/mpi/mpi2_init.h
index 948a3ba..6213ce6 100644
--- a/drivers/scsi/mpt3sas/mpi/mpi2_init.h
+++ b/drivers/scsi/mpt3sas/mpi/mpi2_init.h
@@ -75,7 +75,7 @@
typedef struct _MPI2_SCSI_IO_CDB_EEDP32 {
U8 CDB[20]; /*0x00 */
- U32 PrimaryReferenceTag;/*0x14 */
+ __be32 PrimaryReferenceTag; /*0x14 */
U16 PrimaryApplicationTag; /*0x18 */
U16 PrimaryApplicationTagMask; /*0x1A */
U32 TransferLength; /*0x1C */
diff --git a/drivers/scsi/mpt3sas/mpt3sas_base.c
b/drivers/scsi/mpt3sas/mpt3sas_base.c
index 0a0e7aa..4767690 100644
--- a/drivers/scsi/mpt3sas/mpt3sas_base.c
+++ b/drivers/scsi/mpt3sas/mpt3sas_base.c
@@ -394,13 +394,14 @@ static void _clone_sg_entries(struct MPT3SAS_ADAPTER *ioc,
buff_ptr_phys = buffer_iomem_phys;
WARN_ON(buff_ptr_phys > U32_MAX);
- if (sgel->FlagsLength &
+ if (le32_to_cpu(sgel->FlagsLength) &
(MPI2_SGE_FLAGS_HOST_TO_IOC << MPI2_SGE_FLAGS_SHIFT))
is_write = 1;
for (i = 0; i < MPT_MIN_PHYS_SEGMENTS + ioc->facts.MaxChainDepth; i++) {
- sgl_flags = (sgel->FlagsLength >> MPI2_SGE_FLAGS_SHIFT);
+ sgl_flags =
+ (le32_to_cpu(sgel->FlagsLength) >> MPI2_SGE_FLAGS_SHIFT);
switch (sgl_flags & MPI2_SGE_FLAGS_ELEMENT_MASK) {
case MPI2_SGE_FLAGS_CHAIN_ELEMENT:
@@ -411,7 +412,7 @@ static void _clone_sg_entries(struct MPT3SAS_ADAPTER *ioc,
*/
sgel_next =
_base_get_chain_buffer_dma_to_chain_buffer(ioc,
- sgel->Address);
+ le32_to_cpu(sgel->Address));
if (sgel_next == NULL)
return;
/*
@@ -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);
sgel = sgel_next;
sge_chain_count++;
break;
@@ -435,22 +436,28 @@ static void _clone_sg_entries(struct MPT3SAS_ADAPTER *ioc,
if (is_scsiio_req) {
_base_clone_to_sys_mem(buff_ptr,
sg_virt(sg_scmd),
- (sgel->FlagsLength & 0x00ff));
+ (le32_to_cpu(sgel->FlagsLength) &
+ 0x00ff));
/*
* FIXME: this relies on a a zero
* PCI mem_offset.
*/
- sgel->Address = (u32)buff_ptr_phys;
+ sgel->Address =
+ cpu_to_le32((u32)buff_ptr_phys);
} else {
_base_clone_to_sys_mem(buff_ptr,
ioc->config_vaddr,
- (sgel->FlagsLength & 0x00ff));
- sgel->Address = (u32)buff_ptr_phys;
+ (le32_to_cpu(sgel->FlagsLength) &
+ 0x00ff));
+ sgel->Address =
+ cpu_to_le32((u32)buff_ptr_phys);
}
}
- buff_ptr += (sgel->FlagsLength & 0x00ff);
- buff_ptr_phys += (sgel->FlagsLength & 0x00ff);
- if ((sgel->FlagsLength &
+ buff_ptr += (le32_to_cpu(sgel->FlagsLength) &
+ 0x00ff);
+ buff_ptr_phys += (le32_to_cpu(sgel->FlagsLength) &
+
