On 10 April 2018 at 10:58, Michel Dänzer <michel at daenzer.net> wrote: > 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. > 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
