On 2018-04-10 11:47 AM, Emil Velikov wrote: > 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? 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