[PATCH] drm/amdgpu: clear seq64 memory on free

[Paneer Selvam, Arunpravin]

Hi Christian,

On 4/16/2024 6:24 PM, Christian König wrote:
> Am 16.04.24 um 14:34 schrieb Paneer Selvam, Arunpravin:
>> Hi Christian,
>>
>> On 4/16/2024 5:47 PM, Christian König wrote:
>>> Am 16.04.24 um 14:16 schrieb Paneer Selvam, Arunpravin:
>>>> Hi Christian,
>>>>
>>>> On 4/16/2024 2:35 PM, Christian König wrote:
>>>>> Am 15.04.24 um 20:48 schrieb Arunpravin Paneer Selvam:
>>>>>> We should clear the memory on free. Otherwise,
>>>>>> there is a chance that we will access the previous
>>>>>> application data and this would leads to an abnormal
>>>>>> behaviour in the current application.
>>>>>
>>>>> Mhm, I would strongly expect that we initialize the seq number 
>>>>> after allocation.
>>>>>
>>>>> It could be that we also have situations were the correct start 
>>>>> value is 0xffffffff or something like that instead.
>>>>>
>>>>> Why does this matter?
>>>> when the user queue A's u64 address (fence address) is allocated to 
>>>> the newly created user queue B, we see a problem.
>>>> User queue B calls the signal IOCTL which creates a new fence 
>>>> having the wptr as the seq number, in
>>>> amdgpu_userq_fence_create function we have a check where we are 
>>>> comparing the rptr and wptr value (rptr >= wptr).
>>>> since the rptr value is read from the u64 address which has the 
>>>> user queue A's wptr data, this rptr >= wptr condition
>>>> gets satisfied and we are dropping the reference before the actual 
>>>> command gets processed in the hardware.
>>>> If we clear this u64 value on free, we dont see this problem.
>>>
>>> Yeah, but that doesn't belongs into the seq64 handling.
>>>
>>> Instead the code which allocates the seq64 during userqueue created 
>>> needs to clear it to 0.
>> sure, got it.
>
> Yeah, but fixing that aside. We should probably initialize the seq64 
> array to something like 0xdeadbeef or a similar pattern to find issues 
> were we forget to initialize the allocated slots.
yes, I will prepare a patch and send for the review.

Thanks,
Arun.
>
> Regards,
> Christian.
>
>>
>> Thanks,
>> Arun.
>>>
>>> Regards,
>>> Christian.
>>>
>>>>
>>>> Thanks,
>>>> Arun.
>>>>>
>>>>> Regards,
>>>>> Christian.
>>>>>
>>>>>>
>>>>>> Signed-off-by: Arunpravin Paneer Selvam 
>>>>>> <Arunpravin.PaneerSelvam at amd.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c | 6 +++++-
>>>>>>   1 file changed, 5 insertions(+), 1 deletion(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c 
>>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
>>>>>> index 4b9afc4df031..9613992c9804 100644
>>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
>>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_seq64.c
>>>>>> @@ -189,10 +189,14 @@ int amdgpu_seq64_alloc(struct amdgpu_device 
>>>>>> *adev, u64 *va, u64 **cpu_addr)
>>>>>>   void amdgpu_seq64_free(struct amdgpu_device *adev, u64 va)
>>>>>>   {
>>>>>>       unsigned long bit_pos;
>>>>>> +    u64 *cpu_addr;
>>>>>>         bit_pos = (va - amdgpu_seq64_get_va_base(adev)) / 
>>>>>> sizeof(u64);
>>>>>> -    if (bit_pos < adev->seq64.num_sem)
>>>>>> +    if (bit_pos < adev->seq64.num_sem) {
>>>>>> +        cpu_addr = bit_pos + adev->seq64.cpu_base_addr;
>>>>>> +        memset(cpu_addr, 0, sizeof(u64));
>>>>>>           __clear_bit(bit_pos, adev->seq64.used);
>>>>>> +    }
>>>>>>   }
>>>>>>     /**
>>>>>
>>>>
>>>
>>
>