On 10 April 2018 at 09:28, Michel Dänzer <michel at daenzer.net> wrote: > On 2018-04-04 04:29 PM, Emil Velikov wrote: >> From: Emil Velikov <emil.velikov at collabora.com> >> >> Seems like we've been leaking this for years. It became more obvious >> with the recent refactoring. >> >> Signed-off-by: Emil Velikov <emil.velikov at collabora.com> >> --- >> 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
