From patchwork Mon Sep 14 15:23:57 2015 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Archit Taneja X-Patchwork-Id: 7176731 Return-Path: X-Original-To: patchwork-dri-devel@patchwork.kernel.org Delivered-To: patchwork-parsemail@patchwork2.web.kernel.org Received: from mail.kernel.org (mail.kernel.org [198.145.29.136]) by patchwork2.web.kernel.org (Postfix) with ESMTP id 0AF08BEEC1 for ; Mon, 14 Sep 2015 15:24:15 +0000 (UTC) Received: from mail.kernel.org (localhost [127.0.0.1]) by mail.kernel.org (Postfix) with ESMTP id 1104D2060F for ; Mon, 14 Sep 2015 15:24:14 +0000 (UTC) Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) by mail.kernel.org (Postfix) with ESMTP id 16CE12060D for ; Mon, 14 Sep 2015 15:24:13 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id 34E9E6E8B8; Mon, 14 Sep 2015 08:24:11 -0700 (PDT) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org 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) Received: from smtp.codeaurora.org (localhost [127.0.0.1]) by smtp.codeaurora.org (Postfix) with ESMTP id 899B5141CD2; Mon, 14 Sep 2015 15:24:09 +0000 (UTC) Received: by smtp.codeaurora.org (Postfix, from userid 486) id 756EC141CD6; Mon, 14 Sep 2015 15:24:09 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on mail.kernel.org X-Spam-Level: X-Spam-Status: No, score=-4.2 required=5.0 tests=BAYES_00, RCVD_IN_DNSWL_MED, T_RP_MATCHES_RCVD, UNPARSEABLE_RELAY autolearn=unavailable version=3.3.1 Received: from [192.168.1.6] (unknown [122.172.163.10]) (using TLSv1.2 with cipher DHE-RSA-AES128-SHA (128/128 bits)) (No client certificate requested) (Authenticated sender: architt@smtp.codeaurora.org) by smtp.codeaurora.org (Postfix) with ESMTPSA id 36886141CD2; Mon, 14 Sep 2015 15:24:02 +0000 (UTC) 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> 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: X-Virus-Scanned: ClamAV using ClamSMTP Cc: Daniel Vetter , LKML , dri-devel , Archit Taneja , Sudip Mukherjee X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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)