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

[Felix Kuehling <felix.kuehling@amd.com>]

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;
>>   }
>>     /**
>