diff mbox series

drm/loongson: Fix two warnings because of passing wrong type

Message ID 20230710100931.255234-1-suijingfeng@loongson.cn
State New
Headers
Series drm/loongson: Fix two warnings because of passing wrong type |

Commit Message

Sui Jingfeng July 10, 2023, 10:09 a.m. UTC 
  When accessing I/O memory, we should pass '__iomem *' type instead of
'void *' simply, otherwise sparse tests will complain. After applied
this patch, the following two sparse warnings got fixed.

1) drivers/gpu/drm/loongson/lsdc_benchmark.c:27:35:
   sparse:     expected void volatile [noderef] __iomem *
   sparse:     got void *kptr

2) drivers/gpu/drm/loongson/lsdc_benchmark.c:42:51:
   sparse:     expected void const volatile [noderef] __iomem *
   sparse:     got void *kptr

Reported-by: kernel test robot <lkp@intel.com>
Closes: https://lore.kernel.org/oe-kbuild-all/202307100243.v3hv6aes-lkp@intel.com/
Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
---
 drivers/gpu/drm/loongson/lsdc_benchmark.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

Comments

Jani Nikula July 10, 2023, 10:26 a.m. UTC | #1 
On Mon, 10 Jul 2023, Sui Jingfeng <suijingfeng@loongson.cn> wrote:
> When accessing I/O memory, we should pass '__iomem *' type instead of
> 'void *' simply, otherwise sparse tests will complain. After applied
> this patch, the following two sparse warnings got fixed.

Usually the commit message should explain why it's okay to cast away the
warning.

Because realistically this doesn't "fix" the warning, this merely hides
it.

BR,
Jani.

>
> 1) drivers/gpu/drm/loongson/lsdc_benchmark.c:27:35:
>    sparse:     expected void volatile [noderef] __iomem *
>    sparse:     got void *kptr
>
> 2) drivers/gpu/drm/loongson/lsdc_benchmark.c:42:51:
>    sparse:     expected void const volatile [noderef] __iomem *
>    sparse:     got void *kptr
>
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202307100243.v3hv6aes-lkp@intel.com/
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>  drivers/gpu/drm/loongson/lsdc_benchmark.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/loongson/lsdc_benchmark.c b/drivers/gpu/drm/loongson/lsdc_benchmark.c
> index b088646a2ff9..36e352820bdb 100644
> --- a/drivers/gpu/drm/loongson/lsdc_benchmark.c
> +++ b/drivers/gpu/drm/loongson/lsdc_benchmark.c
> @@ -24,7 +24,7 @@ static void lsdc_copy_gtt_to_vram_cpu(struct lsdc_bo *src_bo,
>  	lsdc_bo_kmap(dst_bo);
>  
>  	while (n--)
> -		memcpy_toio(dst_bo->kptr, src_bo->kptr, size);
> +		memcpy_toio((void __iomem *)dst_bo->kptr, src_bo->kptr, size);
>  
>  	lsdc_bo_kunmap(src_bo);
>  	lsdc_bo_kunmap(dst_bo);
> @@ -39,7 +39,7 @@ static void lsdc_copy_vram_to_gtt_cpu(struct lsdc_bo *src_bo,
>  	lsdc_bo_kmap(dst_bo);
>  
>  	while (n--)
> -		memcpy_fromio(dst_bo->kptr, src_bo->kptr, size);
> +		memcpy_fromio(dst_bo->kptr, (void __iomem *)src_bo->kptr, size);
>  
>  	lsdc_bo_kunmap(src_bo);
>  	lsdc_bo_kunmap(dst_bo);
Thomas Zimmermann July 10, 2023, 10:36 a.m. UTC | #2 
Hi

Am 10.07.23 um 12:09 schrieb Sui Jingfeng:
> When accessing I/O memory, we should pass '__iomem *' type instead of
> 'void *' simply, otherwise sparse tests will complain. After applied
> this patch, the following two sparse warnings got fixed.
> 
> 1) drivers/gpu/drm/loongson/lsdc_benchmark.c:27:35:
>     sparse:     expected void volatile [noderef] __iomem *
>     sparse:     got void *kptr
> 
> 2) drivers/gpu/drm/loongson/lsdc_benchmark.c:42:51:
>     sparse:     expected void const volatile [noderef] __iomem *
>     sparse:     got void *kptr
> 
> Reported-by: kernel test robot <lkp@intel.com>
> Closes: https://lore.kernel.org/oe-kbuild-all/202307100243.v3hv6aes-lkp@intel.com/
> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
> ---
>   drivers/gpu/drm/loongson/lsdc_benchmark.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/loongson/lsdc_benchmark.c b/drivers/gpu/drm/loongson/lsdc_benchmark.c
> index b088646a2ff9..36e352820bdb 100644
> --- a/drivers/gpu/drm/loongson/lsdc_benchmark.c
> +++ b/drivers/gpu/drm/loongson/lsdc_benchmark.c
> @@ -24,7 +24,7 @@ static void lsdc_copy_gtt_to_vram_cpu(struct lsdc_bo *src_bo,
>   	lsdc_bo_kmap(dst_bo);
>   
>   	while (n--)
> -		memcpy_toio(dst_bo->kptr, src_bo->kptr, size);
> +		memcpy_toio((void __iomem *)dst_bo->kptr, src_bo->kptr, size);

If this is I/O memory, better add the __iomem qualifier to dst_bo->kptr 
rather than casting here. If if can be both, you could use an unnamed 
union like this:

struct lscd_bo {
   union {
     void __iomem *kptr_iomem;
     void *kptr;
   };
};

Best regards
Thomas

>   
>   	lsdc_bo_kunmap(src_bo);
>   	lsdc_bo_kunmap(dst_bo);
> @@ -39,7 +39,7 @@ static void lsdc_copy_vram_to_gtt_cpu(struct lsdc_bo *src_bo,
>   	lsdc_bo_kmap(dst_bo);
>   
>   	while (n--)
> -		memcpy_fromio(dst_bo->kptr, src_bo->kptr, size);
> +		memcpy_fromio(dst_bo->kptr, (void __iomem *)src_bo->kptr, size);
>   
>   	lsdc_bo_kunmap(src_bo);
>   	lsdc_bo_kunmap(dst_bo);

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)
Sui Jingfeng July 10, 2023, 1:10 p.m. UTC | #3 
Hi,

On 2023/7/10 18:26, Jani Nikula wrote:
> On Mon, 10 Jul 2023, Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>> When accessing I/O memory, we should pass '__iomem *' type instead of
>> 'void *' simply, otherwise sparse tests will complain. After applied
>> this patch, the following two sparse warnings got fixed.
> Usually the commit message should explain why it's okay to cast away the
> warning.
>
> Because realistically this doesn't "fix" the warning, this merely hides
> it.


My understanding is that a point itself is just a variable where store a 
address,

if this address originally point to I/O memory,

then, we can other cast it to u64 type, then cast it back to '__iomem *' 
again.

as long as the type's  bit-width is width enough to store this address, 
we won't lost the information.


'void *' or 'u64' is just a intermediate represent of the address.

we can other cast it to u64 type, then cast it back to 'void __iomem *' 
or 'void *' again.


Why it's okay ? My answer is that

As long as a address is really point to the I/O memory, cast it to 'void 
__iomem *' is OK.

As long as a address is really point to the system memory, cast it to 
'void *' is OK.


> BR,
> Jani.
>
>> 1) drivers/gpu/drm/loongson/lsdc_benchmark.c:27:35:
>>     sparse:     expected void volatile [noderef] __iomem *
>>     sparse:     got void *kptr
>>
>> 2) drivers/gpu/drm/loongson/lsdc_benchmark.c:42:51:
>>     sparse:     expected void const volatile [noderef] __iomem *
>>     sparse:     got void *kptr
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202307100243.v3hv6aes-lkp@intel.com/
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/gpu/drm/loongson/lsdc_benchmark.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/loongson/lsdc_benchmark.c b/drivers/gpu/drm/loongson/lsdc_benchmark.c
>> index b088646a2ff9..36e352820bdb 100644
>> --- a/drivers/gpu/drm/loongson/lsdc_benchmark.c
>> +++ b/drivers/gpu/drm/loongson/lsdc_benchmark.c
>> @@ -24,7 +24,7 @@ static void lsdc_copy_gtt_to_vram_cpu(struct lsdc_bo *src_bo,
>>   	lsdc_bo_kmap(dst_bo);
>>   
>>   	while (n--)
>> -		memcpy_toio(dst_bo->kptr, src_bo->kptr, size);
>> +		memcpy_toio((void __iomem *)dst_bo->kptr, src_bo->kptr, size);
>>   
>>   	lsdc_bo_kunmap(src_bo);
>>   	lsdc_bo_kunmap(dst_bo);
>> @@ -39,7 +39,7 @@ static void lsdc_copy_vram_to_gtt_cpu(struct lsdc_bo *src_bo,
>>   	lsdc_bo_kmap(dst_bo);
>>   
>>   	while (n--)
>> -		memcpy_fromio(dst_bo->kptr, src_bo->kptr, size);
>> +		memcpy_fromio(dst_bo->kptr, (void __iomem *)src_bo->kptr, size);
>>   
>>   	lsdc_bo_kunmap(src_bo);
>>   	lsdc_bo_kunmap(dst_bo);
Sui Jingfeng July 10, 2023, 1:50 p.m. UTC | #4 
Hi,

On 2023/7/10 18:26, Jani Nikula wrote:
> On Mon, 10 Jul 2023, Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>> When accessing I/O memory, we should pass '__iomem *' type instead of
>> 'void *' simply, otherwise sparse tests will complain. After applied
>> this patch, the following two sparse warnings got fixed.
> Usually the commit message should explain why it's okay to cast away the
> warning.
>
> Because realistically this doesn't "fix" the warning, this merely hides
> it.


The reason why we don't fix this at the very beginning is that

we are following the ttm_kmap_obj_virtual() and the ttm_bo_kmap() function.

Our lsdc_bo_kmap() is implemented with the ttm_bo_kmap() function.


Another reason is that this warning don't emerge when compile with W=1,

at least this is true on our platform.


We don't think this warning is harmful, because implicit cast will do 
the cast for us.

It is just that we need eliminate the noise as a programmer.


Again, for the code at here, before you do the de-reference operation,

As long as a address is really(originally) point to the I/O memory, cast 
it to 'void __iomem *' is OK.

As long as a address is really(originally) point to the system memory, 
cast it to 'void *' is OK.


> BR,
> Jani.
>
>> 1) drivers/gpu/drm/loongson/lsdc_benchmark.c:27:35:
>>     sparse:     expected void volatile [noderef] __iomem *
>>     sparse:     got void *kptr
>>
>> 2) drivers/gpu/drm/loongson/lsdc_benchmark.c:42:51:
>>     sparse:     expected void const volatile [noderef] __iomem *
>>     sparse:     got void *kptr
>>
>> Reported-by: kernel test robot <lkp@intel.com>
>> Closes: https://lore.kernel.org/oe-kbuild-all/202307100243.v3hv6aes-lkp@intel.com/
>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> ---
>>   drivers/gpu/drm/loongson/lsdc_benchmark.c | 4 ++--
>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/loongson/lsdc_benchmark.c b/drivers/gpu/drm/loongson/lsdc_benchmark.c
>> index b088646a2ff9..36e352820bdb 100644
>> --- a/drivers/gpu/drm/loongson/lsdc_benchmark.c
>> +++ b/drivers/gpu/drm/loongson/lsdc_benchmark.c
>> @@ -24,7 +24,7 @@ static void lsdc_copy_gtt_to_vram_cpu(struct lsdc_bo *src_bo,
>>   	lsdc_bo_kmap(dst_bo);
>>   
>>   	while (n--)
>> -		memcpy_toio(dst_bo->kptr, src_bo->kptr, size);
>> +		memcpy_toio((void __iomem *)dst_bo->kptr, src_bo->kptr, size);
>>   
>>   	lsdc_bo_kunmap(src_bo);
>>   	lsdc_bo_kunmap(dst_bo);
>> @@ -39,7 +39,7 @@ static void lsdc_copy_vram_to_gtt_cpu(struct lsdc_bo *src_bo,
>>   	lsdc_bo_kmap(dst_bo);
>>   
>>   	while (n--)
>> -		memcpy_fromio(dst_bo->kptr, src_bo->kptr, size);
>> +		memcpy_fromio(dst_bo->kptr, (void __iomem *)src_bo->kptr, size);
>>   
>>   	lsdc_bo_kunmap(src_bo);
>>   	lsdc_bo_kunmap(dst_bo);
Jani Nikula July 11, 2023, 8:03 a.m. UTC | #5 
On Mon, 10 Jul 2023, suijingfeng <suijingfeng@loongson.cn> wrote:
> Hi,
>
> On 2023/7/10 18:26, Jani Nikula wrote:
>> On Mon, 10 Jul 2023, Sui Jingfeng <suijingfeng@loongson.cn> wrote:
>>> When accessing I/O memory, we should pass '__iomem *' type instead of
>>> 'void *' simply, otherwise sparse tests will complain. After applied
>>> this patch, the following two sparse warnings got fixed.
>> Usually the commit message should explain why it's okay to cast away the
>> warning.
>>
>> Because realistically this doesn't "fix" the warning, this merely hides
>> it.
>
>
> My understanding is that a point itself is just a variable where store a 
> address,
>
> if this address originally point to I/O memory,
>
> then, we can other cast it to u64 type, then cast it back to '__iomem *' 
> again.
>
> as long as the type's  bit-width is width enough to store this address, 
> we won't lost the information.
>
>
> 'void *' or 'u64' is just a intermediate represent of the address.
>
> we can other cast it to u64 type, then cast it back to 'void __iomem *' 
> or 'void *' again.
>
>
> Why it's okay ? My answer is that
>
> As long as a address is really point to the I/O memory, cast it to 'void 
> __iomem *' is OK.
>
> As long as a address is really point to the system memory, cast it to 
> 'void *' is OK.

The point of __iomem is to have sparse help you in tracking that, so you
don't accidentally mix up the two.

BR,
Jani.


>
>
>> BR,
>> Jani.
>>
>>> 1) drivers/gpu/drm/loongson/lsdc_benchmark.c:27:35:
>>>     sparse:     expected void volatile [noderef] __iomem *
>>>     sparse:     got void *kptr
>>>
>>> 2) drivers/gpu/drm/loongson/lsdc_benchmark.c:42:51:
>>>     sparse:     expected void const volatile [noderef] __iomem *
>>>     sparse:     got void *kptr
>>>
>>> Reported-by: kernel test robot <lkp@intel.com>
>>> Closes: https://lore.kernel.org/oe-kbuild-all/202307100243.v3hv6aes-lkp@intel.com/
>>> Signed-off-by: Sui Jingfeng <suijingfeng@loongson.cn>
>>> ---
>>>   drivers/gpu/drm/loongson/lsdc_benchmark.c | 4 ++--
>>>   1 file changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/loongson/lsdc_benchmark.c b/drivers/gpu/drm/loongson/lsdc_benchmark.c
>>> index b088646a2ff9..36e352820bdb 100644
>>> --- a/drivers/gpu/drm/loongson/lsdc_benchmark.c
>>> +++ b/drivers/gpu/drm/loongson/lsdc_benchmark.c
>>> @@ -24,7 +24,7 @@ static void lsdc_copy_gtt_to_vram_cpu(struct lsdc_bo *src_bo,
>>>   	lsdc_bo_kmap(dst_bo);
>>>   
>>>   	while (n--)
>>> -		memcpy_toio(dst_bo->kptr, src_bo->kptr, size);
>>> +		memcpy_toio((void __iomem *)dst_bo->kptr, src_bo->kptr, size);
>>>   
>>>   	lsdc_bo_kunmap(src_bo);
>>>   	lsdc_bo_kunmap(dst_bo);
>>> @@ -39,7 +39,7 @@ static void lsdc_copy_vram_to_gtt_cpu(struct lsdc_bo *src_bo,
>>>   	lsdc_bo_kmap(dst_bo);
>>>   
>>>   	while (n--)
>>> -		memcpy_fromio(dst_bo->kptr, src_bo->kptr, size);
>>> +		memcpy_fromio(dst_bo->kptr, (void __iomem *)src_bo->kptr, size);
>>>   
>>>   	lsdc_bo_kunmap(src_bo);
>>>   	lsdc_bo_kunmap(dst_bo);
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/loongson/lsdc_benchmark.c b/drivers/gpu/drm/loongson/lsdc_benchmark.c
index b088646a2ff9..36e352820bdb 100644
--- a/drivers/gpu/drm/loongson/lsdc_benchmark.c
+++ b/drivers/gpu/drm/loongson/lsdc_benchmark.c
@@ -24,7 +24,7 @@  static void lsdc_copy_gtt_to_vram_cpu(struct lsdc_bo *src_bo,
 	lsdc_bo_kmap(dst_bo);
 
 	while (n--)
-		memcpy_toio(dst_bo->kptr, src_bo->kptr, size);
+		memcpy_toio((void __iomem *)dst_bo->kptr, src_bo->kptr, size);
 
 	lsdc_bo_kunmap(src_bo);
 	lsdc_bo_kunmap(dst_bo);
@@ -39,7 +39,7 @@  static void lsdc_copy_vram_to_gtt_cpu(struct lsdc_bo *src_bo,
 	lsdc_bo_kmap(dst_bo);
 
 	while (n--)
-		memcpy_fromio(dst_bo->kptr, src_bo->kptr, size);
+		memcpy_fromio(dst_bo->kptr, (void __iomem *)src_bo->kptr, size);
 
 	lsdc_bo_kunmap(src_bo);
 	lsdc_bo_kunmap(dst_bo);