[PATCH] drm/amdgpu: fix the memory corruption on S3

[Christian König]

Indeed a nice catch.

Just skimming over the PSP code, wouldn't it be simpler to temporary 
allocate the cmd buffer in psp_np_fw_load()?

Doesn't looks like that is used outside that function. Might even be 
possible to just allocate the buffer on the stack.

Regards,
Christian.

Am 29.06.2017 um 09:59 schrieb Huang Rui:
> On Thu, Jun 29, 2017 at 03:34:57PM +0800, Michel Dänzer wrote:
>> On 29/06/17 04:03 PM, Huang Rui wrote:
>>> psp->cmd will be used on resume phase, so we can not free it on hw_init.
>>> Otherwise, a memory corruption will be triggered.
>>>
>>> Signed-off-by: Huang Rui <ray.huang at amd.com>
>>> ---
>>>
>>> Alex, Christian,
>>>
>>> This is the final fix for vega10 S3. The random memory corruption issue is
>> root
>>> caused.
>>>
>>> Thanks,
>>> Ray
>>>
>>> ---
>>>   drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c | 8 ++++++--
>>>   1 file changed, 6 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c b/drivers/gpu/drm/amd/
>> amdgpu/amdgpu_psp.c
>>> index 5041073..fcdd542 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_psp.c
>>> @@ -372,8 +372,6 @@ static int psp_load_fw(struct amdgpu_device *adev)
>>>         if (ret)
>>>                 goto failed_mem;
>>>
>>> -     kfree(cmd);
>>> -
>>>         return 0;
>> This looks like a good catch.
>>
>>
>>> @@ -384,6 +382,7 @@ static int psp_load_fw(struct amdgpu_device *adev)
>>>                               &psp->fw_pri_mc_addr, &psp->fw_pri_buf);
>>>   failed:
>>>         kfree(cmd);
>>> +     cmd = NULL;
>> This should probably be
>>
>>          psp->cmd = NULL;
>>
>> instead?
>>
> Actually, we set psp->cmd = cmd before.
>
> But anyway, we needn't "cmd" member any more.
>
>>> @@ -443,6 +442,11 @@ static int psp_hw_fini(void *handle)
>>>                 amdgpu_bo_free_kernel(&psp->fence_buf_bo,
>>>                                       &psp->fence_buf_mc_addr, &psp->
>> fence_buf);
>>> +     if (!psp->cmd) {
>>> +             kfree(psp->cmd);
>>> +             psp->cmd = NULL;
>>> +     }
>> This should probably be
>>
>>          if (psp->cmd) {
>>
>> ? If so, you could skip the if altogether, since kfree(NULL) is safe.
> Nice catch. My typo. ;-)
>
> You're right. Will update it in V2.
>
> Thanks,
> Rui