[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 at amd.com>
>>> Cc: Christian König <christian.koenig at amd.com>
>>> Cc: Alex Deucher <alexander.deucher at amd.com>
>>> Cc: "Pan, Xinhui" <Xinhui.Pan at amd.com>
>>> Cc: Le Ma <le.ma at amd.com>
>>> Cc: Hawking Zhang <Hawking.Zhang at amd.com>
>>> Signed-off-by: Srinivasan Shanmugam <srinivasan.shanmugam at 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;
>>>   }
>>>     /**
>>