Re: [PATCH v2 01/14] mpt3sas: Bug fix for big endian systems.

2018-05-02 Thread Sreekanth Reddy 
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 *)>chip->Doorbell +
>>> +ioc->replyPostRegisterIndex[i] =
>>> +(volatile void __iomem *)
>>> + ((u8 __force *)>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] =
>> >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.

2018-05-01 Thread Martin K. Petersen 

Hi Chaitra,

>>  for (i = 0; i < ioc->combined_reply_index_count; i++) {
>> -ioc->replyPostRegisterIndex[i] = (resource_size_t
> *)
>> - ((u8 *)>chip->Doorbell +
>> +ioc->replyPostRegisterIndex[i] =
>> +(volatile void __iomem *)
>> + ((u8 __force *)>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] =
> >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.

2018-04-23 Thread Chaitra Basappa 
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 *)>chip->Doorbell +
> + ioc->replyPostRegisterIndex[i] =
> + (volatile void __iomem *)
> +  ((u8 __force *)>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] =
>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.

2018-04-20 Thread Martin K. Petersen 

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 *)>chip->Doorbell +
> + ioc->replyPostRegisterIndex[i] =
> + (volatile void __iomem *)
> +  ((u8 __force *)>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] =
>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