[PATCH] drm/amdgpu: Use READ_ONCE() when reading the values in 'sdma_v4_4_2_ring_get_rptr'

[PATCH] drm/amdgpu: Use READ_ONCE() when reading the values in 'sdma_v4_4_2_ring_get_rptr'

[Srinivasan Shanmugam]

Instead of declaring pointers use READ_ONCE(), when accessing those
values to make sure that the compiler doesn't voilate any cache
coherences

Cc: Guchun Chen <guchun.chen@amd.com>
Cc: Christian König <christian.koenig@amd.com>
Cc: Alex Deucher <alexander.deucher@amd.com>
Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
Cc: Le Ma <le.ma@amd.com>
Cc: Hawking Zhang <Hawking.Zhang@amd.com>
Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
index f413898dda37..267c1b7b8dcd 100644
--- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
+++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
@@ -154,13 +154,13 @@ static int sdma_v4_4_2_init_microcode(struct amdgpu_device *adev)
  */
 static uint64_t sdma_v4_4_2_ring_get_rptr(struct amdgpu_ring *ring)
 {
-	u64 *rptr;
+	u64 rptr;
 
 	/* XXX check if swapping is necessary on BE */
-	rptr = ((u64 *)&ring->adev->wb.wb[ring->rptr_offs]);
+	rptr = READ_ONCE(*((u64 *)&ring->adev->wb.wb[ring->rptr_offs]));
 
-	DRM_DEBUG("rptr before shift == 0x%016llx\n", *rptr);
-	return ((*rptr) >> 2);
+	DRM_DEBUG("rptr before shift == 0x%016llx\n", rptr);
+	return rptr >> 2;
 }
 
 /**
-- 
2.25.1

RE: [PATCH] drm/amdgpu: Use READ_ONCE() when reading the values in 'sdma_v4_4_2_ring_get_rptr'

[Ma, Le]

[AMD Official Use Only - General]

Reviewed-by: Le Ma <le.ma@amd.com>

> -----Original Message-----
> From: SHANMUGAM, SRINIVASAN <SRINIVASAN.SHANMUGAM@amd.com>
> Sent: Friday, August 4, 2023 1:47 PM
> To: Koenig, Christian <Christian.Koenig@amd.com>; Deucher, Alexander
> <Alexander.Deucher@amd.com>; Chen, Guchun <Guchun.Chen@amd.com>;
> Pan, Xinhui <Xinhui.Pan@amd.com>
> Cc: amd-gfx@lists.freedesktop.org; SHANMUGAM, SRINIVASAN
> <SRINIVASAN.SHANMUGAM@amd.com>; Ma, Le <Le.Ma@amd.com>; Zhang,
> Hawking <Hawking.Zhang@amd.com>
> Subject: [PATCH] drm/amdgpu: Use READ_ONCE() when reading the values in
> 'sdma_v4_4_2_ring_get_rptr'
>
> Instead of declaring pointers use READ_ONCE(), when accessing those values to
> make sure that the compiler doesn't voilate any cache coherences
>
> Cc: Guchun Chen <guchun.chen@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
> Cc: Le Ma <le.ma@amd.com>
> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> index f413898dda37..267c1b7b8dcd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> @@ -154,13 +154,13 @@ static int sdma_v4_4_2_init_microcode(struct
> amdgpu_device *adev)
>   */
>  static uint64_t sdma_v4_4_2_ring_get_rptr(struct amdgpu_ring *ring)  {
> -     u64 *rptr;
> +     u64 rptr;
>
>       /* XXX check if swapping is necessary on BE */
> -     rptr = ((u64 *)&ring->adev->wb.wb[ring->rptr_offs]);
> +     rptr = READ_ONCE(*((u64 *)&ring->adev->wb.wb[ring->rptr_offs]));
>
> -     DRM_DEBUG("rptr before shift == 0x%016llx\n", *rptr);
> -     return ((*rptr) >> 2);
> +     DRM_DEBUG("rptr before shift == 0x%016llx\n", rptr);
> +     return rptr >> 2;
>  }
>
>  /**
> --
> 2.25.1

Re: [PATCH] drm/amdgpu: Use READ_ONCE() when reading the values in 'sdma_v4_4_2_ring_get_rptr'

[Christian König]

Am 04.08.23 um 07:46 schrieb Srinivasan Shanmugam:
> Instead of declaring pointers use READ_ONCE(), when accessing those
> values to make sure that the compiler doesn't voilate any cache
> coherences

That commit message is a bit confusing and not 100% technically correct.

The compiler is not causing any cache coherency issues, but potentially 
re-ordering things or reading the value multiple times.

Just write something like "Use READ_ONCE() instead of declaring the 
pointer volatile.". The background explanation would exceed the 
information suitable for a commit message anyway.

Apart from that looks good to me,
Christian.

>
> Cc: Guchun Chen <guchun.chen@amd.com>
> Cc: Christian König <christian.koenig@amd.com>
> Cc: Alex Deucher <alexander.deucher@amd.com>
> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
> Cc: Le Ma <le.ma@amd.com>
> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> index f413898dda37..267c1b7b8dcd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> @@ -154,13 +154,13 @@ static int sdma_v4_4_2_init_microcode(struct amdgpu_device *adev)
>    */
>   static uint64_t sdma_v4_4_2_ring_get_rptr(struct amdgpu_ring *ring)
>   {
> -	u64 *rptr;
> +	u64 rptr;
>   
>   	/* XXX check if swapping is necessary on BE */
> -	rptr = ((u64 *)&ring->adev->wb.wb[ring->rptr_offs]);
> +	rptr = READ_ONCE(*((u64 *)&ring->adev->wb.wb[ring->rptr_offs]));
>   
> -	DRM_DEBUG("rptr before shift == 0x%016llx\n", *rptr);
> -	return ((*rptr) >> 2);
> +	DRM_DEBUG("rptr before shift == 0x%016llx\n", rptr);
> +	return rptr >> 2;
>   }
>   
>   /**

Re: [PATCH] drm/amdgpu: Use READ_ONCE() when reading the values in 'sdma_v4_4_2_ring_get_rptr'

[Felix Kuehling]

Would it make sense to include a link to a better explanation of the 
underlying issue? E.g. https://lwn.net/Articles/624126/?

Regards,
   Felix


On 2023-08-21 07:23, Christian König wrote:
> Am 04.08.23 um 07:46 schrieb Srinivasan Shanmugam:
>> Instead of declaring pointers use READ_ONCE(), when accessing those
>> values to make sure that the compiler doesn't voilate any cache
>> coherences
>
> That commit message is a bit confusing and not 100% technically correct.
>
> The compiler is not causing any cache coherency issues, but 
> potentially re-ordering things or reading the value multiple times.
>
> Just write something like "Use READ_ONCE() instead of declaring the 
> pointer volatile.". The background explanation would exceed the 
> information suitable for a commit message anyway.
>
> Apart from that looks good to me,
> Christian.
>
>>
>> Cc: Guchun Chen <guchun.chen@amd.com>
>> Cc: Christian König <christian.koenig@amd.com>
>> Cc: Alex Deucher <alexander.deucher@amd.com>
>> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
>> Cc: Le Ma <le.ma@amd.com>
>> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
>> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
>> ---
>>   drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 8 ++++----
>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c 
>> b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>> index f413898dda37..267c1b7b8dcd 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>> @@ -154,13 +154,13 @@ static int sdma_v4_4_2_init_microcode(struct 
>> amdgpu_device *adev)
>>    */
>>   static uint64_t sdma_v4_4_2_ring_get_rptr(struct amdgpu_ring *ring)
>>   {
>> -    u64 *rptr;
>> +    u64 rptr;
>>         /* XXX check if swapping is necessary on BE */
>> -    rptr = ((u64 *)&ring->adev->wb.wb[ring->rptr_offs]);
>> +    rptr = READ_ONCE(*((u64 *)&ring->adev->wb.wb[ring->rptr_offs]));
>>   -    DRM_DEBUG("rptr before shift == 0x%016llx\n", *rptr);
>> -    return ((*rptr) >> 2);
>> +    DRM_DEBUG("rptr before shift == 0x%016llx\n", rptr);
>> +    return rptr >> 2;
>>   }
>>     /**
>

Re: [PATCH] drm/amdgpu: Use READ_ONCE() when reading the values in 'sdma_v4_4_2_ring_get_rptr'

[Christian König]

Yes, that's a very good idea as well. Going to keep that link around if 
anybody asks why we do this.

Christian.

Am 21.08.23 um 22:49 schrieb Felix Kuehling:
> Would it make sense to include a link to a better explanation of the 
> underlying issue? E.g. https://lwn.net/Articles/624126/?
>
> Regards,
>   Felix
>
>
> On 2023-08-21 07:23, Christian König wrote:
>> Am 04.08.23 um 07:46 schrieb Srinivasan Shanmugam:
>>> Instead of declaring pointers use READ_ONCE(), when accessing those
>>> values to make sure that the compiler doesn't voilate any cache
>>> coherences
>>
>> That commit message is a bit confusing and not 100% technically correct.
>>
>> The compiler is not causing any cache coherency issues, but 
>> potentially re-ordering things or reading the value multiple times.
>>
>> Just write something like "Use READ_ONCE() instead of declaring the 
>> pointer volatile.". The background explanation would exceed the 
>> information suitable for a commit message anyway.
>>
>> Apart from that looks good to me,
>> Christian.
>>
>>>
>>> Cc: Guchun Chen <guchun.chen@amd.com>
>>> Cc: Christian König <christian.koenig@amd.com>
>>> Cc: Alex Deucher <alexander.deucher@amd.com>
>>> Cc: "Pan, Xinhui" <Xinhui.Pan@amd.com>
>>> Cc: Le Ma <le.ma@amd.com>
>>> Cc: Hawking Zhang <Hawking.Zhang@amd.com>
>>> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam@amd.com>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c | 8 ++++----
>>>   1 file changed, 4 insertions(+), 4 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c 
>>> b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>>> index f413898dda37..267c1b7b8dcd 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
>>> @@ -154,13 +154,13 @@ static int sdma_v4_4_2_init_microcode(struct 
>>> amdgpu_device *adev)
>>>    */
>>>   static uint64_t sdma_v4_4_2_ring_get_rptr(struct amdgpu_ring *ring)
>>>   {
>>> -    u64 *rptr;
>>> +    u64 rptr;
>>>         /* XXX check if swapping is necessary on BE */
>>> -    rptr = ((u64 *)&ring->adev->wb.wb[ring->rptr_offs]);
>>> +    rptr = READ_ONCE(*((u64 *)&ring->adev->wb.wb[ring->rptr_offs]));
>>>   -    DRM_DEBUG("rptr before shift == 0x%016llx\n", *rptr);
>>> -    return ((*rptr) >> 2);
>>> +    DRM_DEBUG("rptr before shift == 0x%016llx\n", rptr);
>>> +    return rptr >> 2;
>>>   }
>>>     /**
>>