From mboxrd@z Thu Jan 1 00:00:00 1970 From: Archit Taneja Subject: Re: [PATCH] drm/mgag200: fix memory leak Date: Mon, 14 Sep 2015 20:53:57 +0530 Message-ID: <55F6E68D.8070800@codeaurora.org> References: <1441627110-13783-1-git-send-email-sudipm.mukherjee@gmail.com> <20150913093607.GA6074@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: Received: from smtp.codeaurora.org (smtp.codeaurora.org [198.145.29.96]) by gabe.freedesktop.org (Postfix) with ESMTPS id DAFB36E8B8 for ; Mon, 14 Sep 2015 08:24:09 -0700 (PDT) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Dave Airlie , Ingo Molnar Cc: Daniel Vetter , LKML , dri-devel , Archit Taneja , Sudip Mukherjee List-Id: dri-devel@lists.freedesktop.org SGksCgpPbiA5LzE0LzIwMTUgMzozNSBQTSwgRGF2ZSBBaXJsaWUgd3JvdGU6Cj4gKHRoaXMgdGlt ZSB3aXRoIGNvcnJlY3QgZW1haWwgYWRkcmVzcykuCj4KPiBPbiAxNCBTZXB0ZW1iZXIgMjAxNSBh dCAyMDowNCwgRGF2ZSBBaXJsaWUgPGFpcmxpZWRAZ21haWwuY29tPiB3cm90ZToKPj4+Cj4+Pj4g SWYgZHJtX2ZiX2hlbHBlcl9hbGxvY19mYmkoKSBmYWlscyB0aGVuIHdlIHdlcmUgZGlyZWN0bHkg cmV0dXJuaW5nCj4+Pj4gd2l0aG91dCBmcmVlaW5nIHN5c3JhbS4gQWxzbyBpZiBkcm1fZmJfaGVs cGVyX2FsbG9jX2ZiaSgpIHN1Y2NlZWRzIGJ1dAo+Pj4+IG1nYWcyMDBfZnJhbWVidWZmZXJfaW5p dCgpIGZhaWxzIHRoZW4gd2Ugd2VyZSBub3QgcmVsZWFzaW5nIHN5c3JhbSBhbmQKPj4+PiB3ZSB3 ZXJlIG5vdCByZWxlYXNpbmcgZmJpIGhlbHBlciBhbHNvLgo+Pj4+Cj4+Pj4gU2lnbmVkLW9mZi1i eTogU3VkaXAgTXVraGVyamVlIDxzdWRpcEB2ZWN0b3JpbmRpYS5vcmc+Cj4+Pj4gLS0tCj4+Pj4g ICBkcml2ZXJzL2dwdS9kcm0vbWdhZzIwMC9tZ2FnMjAwX2ZiLmMgfCAxNSArKysrKysrKysrKyst LS0KPj4+PiAgIDEgZmlsZSBjaGFuZ2VkLCAxMiBpbnNlcnRpb25zKCspLCAzIGRlbGV0aW9ucygt KQo+Pj4+Cj4+Pj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9tZ2FnMjAwL21nYWcyMDBf ZmIuYyBiL2RyaXZlcnMvZ3B1L2RybS9tZ2FnMjAwL21nYWcyMDBfZmIuYwo+Pj4+IGluZGV4IDg3 ZGUxNWUuLjVmZTQ3NmEgMTAwNjQ0Cj4+Pj4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL21nYWcyMDAv bWdhZzIwMF9mYi5jCj4+Pj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL21nYWcyMDAvbWdhZzIwMF9m Yi5jCj4+Pj4gQEAgLTE4OSwxNCArMTg5LDE2IEBAIHN0YXRpYyBpbnQgbWdhZzIwMGZiX2NyZWF0 ZShzdHJ1Y3QgZHJtX2ZiX2hlbHBlciAqaGVscGVyLAo+Pj4+ICAgICAgICAgICAgICAgIHJldHVy biAtRU5PTUVNOwo+Pj4+Cj4+Pj4gICAgICAgIGluZm8gPSBkcm1fZmJfaGVscGVyX2FsbG9jX2Zi aShoZWxwZXIpOwo+Pj4+IC0gICAgIGlmIChJU19FUlIoaW5mbykpCj4+Pj4gLSAgICAgICAgICAg ICByZXR1cm4gUFRSX0VSUihpbmZvKTsKPj4+PiArICAgICBpZiAoSVNfRVJSKGluZm8pKSB7Cj4+ Pj4gKyAgICAgICAgICAgICByZXQgPSBQVFJfRVJSKGluZm8pOwo+Pj4+ICsgICAgICAgICAgICAg Z290byBlcnJfYWxsb2NfZmJpOwo+Pj4+ICsgICAgIH0KPj4+Pgo+Pj4+ICAgICAgICBpbmZvLT5w YXIgPSBtZmJkZXY7Cj4+Pj4KPj4+PiAgICAgICAgcmV0ID0gbWdhZzIwMF9mcmFtZWJ1ZmZlcl9p bml0KGRldiwgJm1mYmRldi0+bWZiLCAmbW9kZV9jbWQsIGdvYmopOwo+Pj4+ICAgICAgICBpZiAo cmV0KQo+Pj4+IC0gICAgICAgICAgICAgcmV0dXJuIHJldDsKPj4+PiArICAgICAgICAgICAgIGdv dG8gZXJyX2ZyYW1lYnVmZmVyX2luaXQ7Cj4+Pj4KPj4+PiAgICAgICAgbWZiZGV2LT5zeXNyYW0g PSBzeXNyYW07Cj4+Pj4gICAgICAgIG1mYmRldi0+c2l6ZSA9IHNpemU7Cj4+Pj4gQEAgLTIyNiw2 ICsyMjgsMTMgQEAgc3RhdGljIGludCBtZ2FnMjAwZmJfY3JlYXRlKHN0cnVjdCBkcm1fZmJfaGVs cGVyICpoZWxwZXIsCj4+Pj4gICAgICAgIERSTV9ERUJVR19LTVMoImFsbG9jYXRlZCAlZHglZFxu IiwKPj4+PiAgICAgICAgICAgICAgICAgICAgICBmYi0+d2lkdGgsIGZiLT5oZWlnaHQpOwo+Pj4+ ICAgICAgICByZXR1cm4gMDsKPj4+PiArCj4+Pj4gK2Vycl9mcmFtZWJ1ZmZlcl9pbml0Ogo+Pj4+ ICsgICAgIGRybV9mYl9oZWxwZXJfcmVsZWFzZV9mYmkoaGVscGVyKTsKPj4+PiArCj4+Pj4gK2Vy cl9hbGxvY19mYmk6Cj4+Pj4gKyAgICAgdmZyZWUoc3lzcmFtKTsKPj4+PiArICAgICByZXR1cm4g cmV0Owo+Pj4+ICAgfQo+Pj4+Cj4+Pj4gICBzdGF0aWMgaW50IG1nYV9mYmRldl9kZXN0cm95KHN0 cnVjdCBkcm1fZGV2aWNlICpkZXYsCj4+Pgo+Pj4gVGhlcmUncyBhIG5ldyByZWdyZXNzaW9uOiB2 NC4zLXJjMSBjcmFzaGVzIG9uIGJvb3R1cCBvbiBub24tc3VwcG9ydGVkIGhhcmR3YXJlLCBpZgo+ Pj4gQ09ORklHX0RSTV9NR0FHMjAwPXkgKGJ1aWx0IGludG8gdGhlIGtlcm5lbCkuCj4+Cj4+IEFy Y2hpdCwgSSdtIGd1ZXNzaW5nIHRoaXMgaXMgc29tZSBmYWxsb3V0IGZyb20gdGhlIGZiZGV2IGNo YW5nZXMuCj4+Cj4+IFRoZXJlIGlzIG5vIHJlYXNvbiB3ZSBzaG91bGQgbmVlZCBDT05GSUdfRkJf TElUVExFX0VORElBTiBJIGRvbid0IHRoaW5rLgoKSXQgbG9va3MgbGlrZSB0aGUgbWdhZzIwMCBk cml2ZXIgbG9hZCBmYWlscyBhbmQgd2UgY3Jhc2ggaW4gdGhlIGVycm9yIApoYW5kbGluZyBwYXRo LiBkcm1fZmJfaGVscGVyX2ZpbmkgZW5kcyB1cCBiZWluZyBjYWxsZWQgdHdpY2UuIFRoZSBzZWNv bmQKY2FsbCB0cmllcyB0byBmcmVlIHJlc291cmNlcyB0aGF0IHdlcmUgYWxyZWFkeSBmcmVlJ2Qu CgpUaGUgZXJyb25lb3VzIHBhdGggYWJvdmUgc2hvdWxkIGhhdmUgZXhpc3RlZCBldmVuIHdpdGhv dXQgdGhlIHJlY2VudApmYmRldiBoZWxwZXIgY2hhbmdlcy4gSSdtIG5vdCBzdXJlIHdoYXQncyBj YXVzaW5nIHRoZSBkcml2ZXIgbG9hZAp0byBmYWlsIGluIHRoZSBmaXJzdCBwbGFjZS4gSXQgbG9v a3MgbGlrZSBpdCdzIGZhaWxpbmcgYXQKcmVnaXN0ZXJfZnJhbWVidWZmZXIuIElzIGl0IHBvc3Np YmxlIHRoYXQgd2UgbmV2ZXIgdHJpZWQgcnVubmluZwp0aGlzIGRyaXZlciBiZWZvcmUgd2l0aCBC aWcgRW5kaWFuIHNldD8KClRoZSBwYXRjaCBiZWxvdyBmaXhlcyB0aGUgcHJvYmxlbSB3aXRoIHRo ZSBlcnJvciBwYXRoIG1lbnRpb25lZAphYm92ZS4gQ291bGQgd2UgdHJ5IHRoaXM/CgpGcm9tOiBB cmNoaXQgVGFuZWphIDxhcmNoaXR0QGNvZGVhdXJvcmEub3JnPgpEYXRlOiBNb24sIDE0IFNlcCAy MDE1IDIwOjExOjQzICswNTMwClN1YmplY3Q6IFtQQVRDSF0gZHJtL21nYWcyMDA6IFByZXZlbnQg Y2FsbGluZyBkcm1fZmJfaGVscGVyX2ZpbmkgdHdpY2UKCm1nYWcyMDBfZmJkZXZfaW5pdCdzIGVy cm9yIGhhbmRsaW5nIHBhdGggY2FsbHMgZHJtX2ZiX2hlbHBlcl9maW5pIGJlZm9yZQpiYWlsaW5n IG91dC4gVGhlIGVycm9yIGhhbmRsaW5nIHBhdGggb2YgbWdhZzIwMF9kcml2ZXJfbG9hZCBhbHNv IGVuZHMKdXAgY2FsbGluZyBkcm1fZmJfaGVscGVyX2ZpbmkuCgpUaGlzIHJlc3VsdHMgaW4gZHJt X2ZiX2hlbHBlcl9maW5pIGJlaW5nIGNhbGxlZCB0d2ljZSBpZiB0aGUgZHJpdmVyIGxvYWQKZHJt IG9wIGZhaWxzIHNvbWV3aGVyZSBpbiBiZXR3ZWVuLgoKTWFrZSBvbmx5IG1nYWcyMDBfZHJpdmVy X3VubG9hZCBjYWxsIGRybV9mYl9oZWxwZXJfZmluaSwgcmVtb3ZlIHRoZSBjYWxsCmZyb20gbWdh ZzIwMF9mYmRldl9pbml0LgoKU2lnbmVkLW9mZi1ieTogQXJjaGl0IFRhbmVqYSA8YXJjaGl0dEBj b2RlYXVyb3JhLm9yZz4KLS0tCiAgZHJpdmVycy9ncHUvZHJtL21nYWcyMDAvbWdhZzIwMF9mYi5j IHwgOCArKy0tLS0tLQogIDEgZmlsZSBjaGFuZ2VkLCAyIGluc2VydGlvbnMoKyksIDYgZGVsZXRp b25zKC0pCgpkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL21nYWcyMDAvbWdhZzIwMF9mYi5j IApiL2RyaXZlcnMvZ3B1L2RybS9tZ2FnMjAwL21nYWcyMDBfZmIuYwppbmRleCA4N2RlMTVlLi42 MjU5YjBhIDEwMDY0NAotLS0gYS9kcml2ZXJzL2dwdS9kcm0vbWdhZzIwMC9tZ2FnMjAwX2ZiLmMK KysrIGIvZHJpdmVycy9ncHUvZHJtL21nYWcyMDAvbWdhZzIwMF9mYi5jCkBAIC0yODAsMjAgKzI4 MCwxNiBAQCBpbnQgbWdhZzIwMF9mYmRldl9pbml0KHN0cnVjdCBtZ2FfZGV2aWNlICptZGV2KQoK ICAJcmV0ID0gZHJtX2ZiX2hlbHBlcl9zaW5nbGVfYWRkX2FsbF9jb25uZWN0b3JzKCZtZmJkZXYt PmhlbHBlcik7CiAgCWlmIChyZXQpCi0JCWdvdG8gZmluaTsKKwkJcmV0dXJuIHJldDsKCiAgCS8q IGRpc2FibGUgYWxsIHRoZSBwb3NzaWJsZSBvdXRwdXRzL2NydGNzIGJlZm9yZSBlbnRlcmluZyBL TVMgbW9kZSAqLwogIAlkcm1faGVscGVyX2Rpc2FibGVfdW51c2VkX2Z1bmN0aW9ucyhtZGV2LT5k ZXYpOwoKICAJcmV0ID0gZHJtX2ZiX2hlbHBlcl9pbml0aWFsX2NvbmZpZygmbWZiZGV2LT5oZWxw ZXIsIGJwcF9zZWwpOwogIAlpZiAocmV0KQotCQlnb3RvIGZpbmk7CisJCXJldHVybiByZXQ7Cgog IAlyZXR1cm4gMDsKLQotZmluaToKLQlkcm1fZmJfaGVscGVyX2ZpbmkoJm1mYmRldi0+aGVscGVy KTsKLQlyZXR1cm4gcmV0OwogIH0KCiAgdm9pZCBtZ2FnMjAwX2ZiZGV2X2Zpbmkoc3RydWN0IG1n YV9kZXZpY2UgKm1kZXYpCgoKCi0tIApRdWFsY29tbSBJbm5vdmF0aW9uIENlbnRlciwgSW5jLiBp cyBhIG1lbWJlciBvZiBDb2RlIEF1cm9yYSBGb3J1bSwKYSBMaW51eCBGb3VuZGF0aW9uIENvbGxh Ym9yYXRpdmUgUHJvamVjdApfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3Rv cC5vcmcKaHR0cDovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1k ZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755290AbbINPYM (ORCPT ); Mon, 14 Sep 2015 11:24:12 -0400 Received: from smtp.codeaurora.org ([198.145.29.96]:57323 "EHLO smtp.codeaurora.org" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751414AbbINPYK (ORCPT ); Mon, 14 Sep 2015 11:24:10 -0400 Subject: Re: [PATCH] drm/mgag200: fix memory leak To: Dave Airlie , Ingo Molnar References: <1441627110-13783-1-git-send-email-sudipm.mukherjee@gmail.com> <20150913093607.GA6074@gmail.com> Cc: Sudip Mukherjee , David Airlie , Daniel Vetter , LKML , dri-devel , Archit Taneja From: Archit Taneja Message-ID: <55F6E68D.8070800@codeaurora.org> Date: Mon, 14 Sep 2015 20:53:57 +0530 User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 9/14/2015 3:35 PM, Dave Airlie wrote: > (this time with correct email address). > > On 14 September 2015 at 20:04, Dave Airlie wrote: >>> >>>> If drm_fb_helper_alloc_fbi() fails then we were directly returning >>>> without freeing sysram. Also if drm_fb_helper_alloc_fbi() succeeds but >>>> mgag200_framebuffer_init() fails then we were not releasing sysram and >>>> we were not releasing fbi helper also. >>>> >>>> Signed-off-by: Sudip Mukherjee >>>> --- >>>> drivers/gpu/drm/mgag200/mgag200_fb.c | 15 ++++++++++++--- >>>> 1 file changed, 12 insertions(+), 3 deletions(-) >>>> >>>> diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c >>>> index 87de15e..5fe476a 100644 >>>> --- a/drivers/gpu/drm/mgag200/mgag200_fb.c >>>> +++ b/drivers/gpu/drm/mgag200/mgag200_fb.c >>>> @@ -189,14 +189,16 @@ static int mgag200fb_create(struct drm_fb_helper *helper, >>>> return -ENOMEM; >>>> >>>> info = drm_fb_helper_alloc_fbi(helper); >>>> - if (IS_ERR(info)) >>>> - return PTR_ERR(info); >>>> + if (IS_ERR(info)) { >>>> + ret = PTR_ERR(info); >>>> + goto err_alloc_fbi; >>>> + } >>>> >>>> info->par = mfbdev; >>>> >>>> ret = mgag200_framebuffer_init(dev, &mfbdev->mfb, &mode_cmd, gobj); >>>> if (ret) >>>> - return ret; >>>> + goto err_framebuffer_init; >>>> >>>> mfbdev->sysram = sysram; >>>> mfbdev->size = size; >>>> @@ -226,6 +228,13 @@ static int mgag200fb_create(struct drm_fb_helper *helper, >>>> DRM_DEBUG_KMS("allocated %dx%d\n", >>>> fb->width, fb->height); >>>> return 0; >>>> + >>>> +err_framebuffer_init: >>>> + drm_fb_helper_release_fbi(helper); >>>> + >>>> +err_alloc_fbi: >>>> + vfree(sysram); >>>> + return ret; >>>> } >>>> >>>> static int mga_fbdev_destroy(struct drm_device *dev, >>> >>> There's a new regression: v4.3-rc1 crashes on bootup on non-supported hardware, if >>> CONFIG_DRM_MGAG200=y (built into the kernel). >> >> Archit, I'm guessing this is some fallout from the fbdev changes. >> >> There is no reason we should need CONFIG_FB_LITTLE_ENDIAN I don't think. It looks like the mgag200 driver load fails and we crash in the error handling path. drm_fb_helper_fini ends up being called twice. The second call tries to free resources that were already free'd. The erroneous path above should have existed even without the recent fbdev helper changes. I'm not sure what's causing the driver load to fail in the first place. It looks like it's failing at register_framebuffer. Is it possible that we never tried running this driver before with Big Endian set? The patch below fixes the problem with the error path mentioned above. Could we try this? From: Archit Taneja Date: Mon, 14 Sep 2015 20:11:43 +0530 Subject: [PATCH] drm/mgag200: Prevent calling drm_fb_helper_fini twice mgag200_fbdev_init's error handling path calls drm_fb_helper_fini before bailing out. The error handling path of mgag200_driver_load also ends up calling drm_fb_helper_fini. This results in drm_fb_helper_fini being called twice if the driver load drm op fails somewhere in between. Make only mgag200_driver_unload call drm_fb_helper_fini, remove the call from mgag200_fbdev_init. Signed-off-by: Archit Taneja --- drivers/gpu/drm/mgag200/mgag200_fb.c | 8 ++------ 1 file changed, 2 insertions(+), 6 deletions(-) diff --git a/drivers/gpu/drm/mgag200/mgag200_fb.c b/drivers/gpu/drm/mgag200/mgag200_fb.c index 87de15e..6259b0a 100644 --- a/drivers/gpu/drm/mgag200/mgag200_fb.c +++ b/drivers/gpu/drm/mgag200/mgag200_fb.c @@ -280,20 +280,16 @@ int mgag200_fbdev_init(struct mga_device *mdev) ret = drm_fb_helper_single_add_all_connectors(&mfbdev->helper); if (ret) - goto fini; + return ret; /* disable all the possible outputs/crtcs before entering KMS mode */ drm_helper_disable_unused_functions(mdev->dev); ret = drm_fb_helper_initial_config(&mfbdev->helper, bpp_sel); if (ret) - goto fini; + return ret; return 0; - -fini: - drm_fb_helper_fini(&mfbdev->helper); - return ret; } void mgag200_fbdev_fini(struct mga_device *mdev) -- Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a Linux Foundation Collaborative Project