Re: [PATCH xf86-video-amdgpu 11/19] Don't leak a AMDGPUEntRec instance if amdgpu_device_setup fails

[Emil Velikov]

On 10 April 2018 at 10:58, Michel Dänzer  wrote:
> On 2018-04-10 11:47 AM, Emil Velikov wrote:
>> On 10 April 2018 at 09:28, Michel Dänzer  wrote:
>>> On 2018-04-04 04:29 PM, Emil Velikov wrote:
 From: Emil Velikov 

 Seems like we've been leaking this for years. It became more obvious
 with the recent refactoring.

 Signed-off-by: Emil Velikov 
 ---
  src/amdgpu_probe.c | 2 ++
  1 file changed, 2 insertions(+)

 diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
 index 537d44c..588891c 100644
 --- a/src/amdgpu_probe.c
 +++ b/src/amdgpu_probe.c
 @@ -243,6 +243,8 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
   return TRUE;

  error:
 + free(pPriv->ptr);
 + pPriv->ptr = NULL;
   return FALSE;
  }


>>>
>>> valgrind doesn't report a leak if I force this error path; presumably
>>> Xorg frees the private after returning FALSE here.
>>>
>> Just double-checked and Xorg does not know anything about ptr. The
>> only one who clears it up is AMDGPUFreeScreen_KMS.
>>
>> The magic (for this and the other 'leak') seems to be happening in
>> xf86platformAddDevice. Namely:
>>  - ::platformProbe is called via doPlatformProbe
>>  - the driver explicitly calls xf86AllocateScreen, yet fails later on
>>  - back in Xorg, the "if (old_screens == xf86NumGPUDrivers)" is false
>>  - ::PreInit fails, ::configured is false
>>  - xf86DeleteScreen() gets called, which dives into ::FreeScreen (aka
>> AMDGPUFreeScreen_KMS)
>>
>> Eventually, I could unwrap all that although it makes sense to keep
>> things simpler. As effectively done by the patch.
>>
>> I believe you'll agree?
>
> I'm afraid not. There's no leak because it's getting cleaned up as
> designed, so there's no need for this change.
>
Fair enough. I'll swap the commit with a comment one for v2.
This way, the next person will be less tempted to send the same patch.

Something like:

pPriv->ptr is freed in our ::FreeScreen callback. Latter of which gets
called by xf86DeleteScreen() as the driver ::*Probe call fails.

-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 11/19] Don't leak a AMDGPUEntRec instance if amdgpu_device_setup fails

[Michel Dänzer]

On 2018-04-10 11:47 AM, Emil Velikov wrote:
> On 10 April 2018 at 09:28, Michel Dänzer  wrote:
>> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>>> From: Emil Velikov 
>>>
>>> Seems like we've been leaking this for years. It became more obvious
>>> with the recent refactoring.
>>>
>>> Signed-off-by: Emil Velikov 
>>> ---
>>>  src/amdgpu_probe.c | 2 ++
>>>  1 file changed, 2 insertions(+)
>>>
>>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>>> index 537d44c..588891c 100644
>>> --- a/src/amdgpu_probe.c
>>> +++ b/src/amdgpu_probe.c
>>> @@ -243,6 +243,8 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
>>>   return TRUE;
>>>
>>>  error:
>>> + free(pPriv->ptr);
>>> + pPriv->ptr = NULL;
>>>   return FALSE;
>>>  }
>>>
>>>
>>
>> valgrind doesn't report a leak if I force this error path; presumably
>> Xorg frees the private after returning FALSE here.
>>
> Just double-checked and Xorg does not know anything about ptr. The
> only one who clears it up is AMDGPUFreeScreen_KMS.
> 
> The magic (for this and the other 'leak') seems to be happening in
> xf86platformAddDevice. Namely:
>  - ::platformProbe is called via doPlatformProbe
>  - the driver explicitly calls xf86AllocateScreen, yet fails later on
>  - back in Xorg, the "if (old_screens == xf86NumGPUDrivers)" is false
>  - ::PreInit fails, ::configured is false
>  - xf86DeleteScreen() gets called, which dives into ::FreeScreen (aka
> AMDGPUFreeScreen_KMS)
> 
> Eventually, I could unwrap all that although it makes sense to keep
> things simpler. As effectively done by the patch.
> 
> I believe you'll agree?

I'm afraid not. There's no leak because it's getting cleaned up as
designed, so there's no need for this change.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 11/19] Don't leak a AMDGPUEntRec instance if amdgpu_device_setup fails

[Emil Velikov]

On 10 April 2018 at 09:28, Michel Dänzer  wrote:
> On 2018-04-04 04:29 PM, Emil Velikov wrote:
>> From: Emil Velikov 
>>
>> Seems like we've been leaking this for years. It became more obvious
>> with the recent refactoring.
>>
>> Signed-off-by: Emil Velikov 
>> ---
>>  src/amdgpu_probe.c | 2 ++
>>  1 file changed, 2 insertions(+)
>>
>> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
>> index 537d44c..588891c 100644
>> --- a/src/amdgpu_probe.c
>> +++ b/src/amdgpu_probe.c
>> @@ -243,6 +243,8 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
>>   return TRUE;
>>
>>  error:
>> + free(pPriv->ptr);
>> + pPriv->ptr = NULL;
>>   return FALSE;
>>  }
>>
>>
>
> valgrind doesn't report a leak if I force this error path; presumably
> Xorg frees the private after returning FALSE here.
>
Just double-checked and Xorg does not know anything about ptr. The
only one who clears it up is AMDGPUFreeScreen_KMS.

The magic (for this and the other 'leak') seems to be happening in
xf86platformAddDevice. Namely:
 - ::platformProbe is called via doPlatformProbe
 - the driver explicitly calls xf86AllocateScreen, yet fails later on
 - back in Xorg, the "if (old_screens == xf86NumGPUDrivers)" is false
 - ::PreInit fails, ::configured is false
 - xf86DeleteScreen() gets called, which dives into ::FreeScreen (aka
AMDGPUFreeScreen_KMS)

Eventually, I could unwrap all that although it makes sense to keep
things simpler. As effectively done by the patch.

I believe you'll agree?
-Emil
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

Re: [PATCH xf86-video-amdgpu 11/19] Don't leak a AMDGPUEntRec instance if amdgpu_device_setup fails

[Michel Dänzer]

On 2018-04-04 04:29 PM, Emil Velikov wrote:
> From: Emil Velikov 
> 
> Seems like we've been leaking this for years. It became more obvious
> with the recent refactoring.
> 
> Signed-off-by: Emil Velikov 
> ---
>  src/amdgpu_probe.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
> index 537d44c..588891c 100644
> --- a/src/amdgpu_probe.c
> +++ b/src/amdgpu_probe.c
> @@ -243,6 +243,8 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
>   return TRUE;
>  
>  error:
> + free(pPriv->ptr);
> + pPriv->ptr = NULL;
>   return FALSE;
>  }
>  
> 

valgrind doesn't report a leak if I force this error path; presumably
Xorg frees the private after returning FALSE here.


-- 
Earthling Michel Dänzer   |   http://www.amd.com
Libre software enthusiast | Mesa and X developer
___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx

[PATCH xf86-video-amdgpu 11/19] Don't leak a AMDGPUEntRec instance if amdgpu_device_setup fails

[Emil Velikov]

From: Emil Velikov 

Seems like we've been leaking this for years. It became more obvious
with the recent refactoring.

Signed-off-by: Emil Velikov 
---
 src/amdgpu_probe.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/src/amdgpu_probe.c b/src/amdgpu_probe.c
index 537d44c..588891c 100644
--- a/src/amdgpu_probe.c
+++ b/src/amdgpu_probe.c
@@ -243,6 +243,8 @@ amdgpu_probe(ScrnInfoPtr pScrn, int entity_num,
return TRUE;
 
 error:
+   free(pPriv->ptr);
+   pPriv->ptr = NULL;
return FALSE;
 }
 
-- 
2.16.0

___
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx