From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-2.5 required=3.0 tests=DKIM_SIGNED,DKIM_VALID, DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS,MAILING_LIST_MULTI,SPF_PASS, URIBL_BLOCKED,USER_AGENT_MUTT autolearn=ham autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 5C570C43387 for ; Sun, 23 Dec 2018 01:09:12 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by mail.kernel.org (Postfix) with ESMTP id 1C74F21783 for ; Sun, 23 Dec 2018 01:09:12 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lekensteyn.nl header.i=@lekensteyn.nl header.b="xai5o1BC" Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S2404423AbeLWBJL (ORCPT ); Sat, 22 Dec 2018 20:09:11 -0500 Received: from lekensteyn.nl ([178.21.112.251]:36033 "EHLO lekensteyn.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S2404343AbeLWBJK (ORCPT ); Sat, 22 Dec 2018 20:09:10 -0500 DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lekensteyn.nl; s=s2048-2015-q1; h=In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date; bh=TXE+lWZd4TkTM8iRa2yt9w/9Z2ErrJwKlEer0I3g9mE=; b=xai5o1BCUtsnBxTzlLSog9ZObT+Wb9TQyInmfmlLNg9pkFfQIy62Q/qFfSurKfkDTvdWZJNAmJUuAfPRoXj8i0BvpF4azB7at1zWwfGzIuJre0qZGhmSyb0yBv0vdc3qDOfIM65uFHiYD3iB1A/k93NUthmIbhxy9hg6MMhKzKIEtLfghNo/UjwxjxqpaKINh1MTvkYUj7x53sa9FdPpD/6mO1qB0BoWwvW8EA26XmAgqwyQsGtE7GFmzvuCzG5CfInOqe8S8ukDV5O+sqO3aXa6ZzcXZI37trdHS2XrOSocLasvL8BqJbw81eiM4rpF3BPDzO9DY/Rhk2tNkK6W7g==; Received: by lekensteyn.nl with esmtpsa (TLS1.2:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.84_2) (envelope-from ) id 1garrR-0004DA-H4; Sun, 23 Dec 2018 01:43:19 +0100 Date: Sun, 23 Dec 2018 01:43:15 +0100 From: Peter Wu To: Linus Torvalds Cc: rong.a.chen@intel.com, kraxel@redhat.com, Daniel Vetter , Linux List Kernel Mailing , lkp@01.org, Noralf =?iso-8859-1?Q?Tr=F8nnes?= , dri-devel@lists.freedesktop.org Subject: Re: [LKP] [bochs] df2052cc92: WARNING:at_drivers/gpu/drm/drm_mode_config.c:#drm_mode_config_cleanup Message-ID: <20181223004315.GA11455@al> References: <20181221083226.GI23332@shao2-debian> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.11.1 (2018-12-01) Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Fri, Dec 21, 2018 at 11:25:41AM -0800, Linus Torvalds wrote: > On Fri, Dec 21, 2018 at 12:32 AM kernel test robot > wrote: > > > > FYI, we noticed commit df2052cc9221 ("bochs: convert to > > drm_fb_helper_fbdev_setup/teardown") caused > > > > [ 487.591733] WARNING: CPU: 0 PID: 1 at drivers/gpu/drm/drm_mode_config.c:478 drm_mode_config_cleanup+0x270/0x290 > > Ok, this is apparently just a leak for what appears to be a not > particularly interesting error case, but the warning is new to 4.20 > (*) so it would be nice to have somebody look at it. > > That commit is supposed to fix a leak, but there's apparently > something still there. > (*) the *problem* is probably not new, it's just now exposed by the > switch to drm_mode_config_cleanup(). I concur, the issue was only revealed because a (not so interesting error path was triggered). Reproduced this on current master (v4.20-rc7-274-g23203e3f34c9), the trace leading up to the warning is the same: [ 50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support this framebuffer [ 50.009436] bochs-drm 0000:00:02.0: [drm:drm_fb_helper_fbdev_setup] *ERROR* fbdev: Failed to set configuration (ret=-38) [ 50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 for 0000:00:02.0 on minor 2 [ 50.013604] WARNING: CPU: 1 PID: 1 at drivers/gpu/drm/drm_mode_config.c:477 drm_mode_config_cleanup+0x280/0x2a0 [ 50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G T 4.20.0-rc7 #1 [ 50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0 ... [ 50.023155] Call Trace: [ 50.023155] ? bochs_kms_fini+0x1e/0x30 [ 50.023155] ? bochs_unload+0x18/0x40 [ 50.023155] ? bochs_pci_remove+0x18/0x30 [ 50.023155] ? pci_device_remove+0x1c/0x50 [ 50.031880] ? really_probe+0xf3/0x2d0 [ 50.031880] ? driver_probe_device+0x23/0xa0 The warning suggests that drm_framebuffer_init was called at some point without a matching call to drm_framebuffer_cleanup. Adding dump_stack() reveals: [ 97.673399] drm_framebuffer_init+0x17d/0x190 [ 97.674134] drm_gem_fb_alloc+0xbe/0x120 [ 97.674678] drm_gem_fbdev_fb_create+0x184/0x1c0 [ 97.675322] ? drm_gem_fb_simple_display_pipe_prepare_fb+0x20/0x20 [ 97.676771] ? drm_fb_helper_alloc_fbi+0xe1/0x120 [ 97.677408] bochsfb_create+0x245/0x5f0 [ 97.677935] ? bochsfb_mmap+0x60/0x60 [ 97.678421] __drm_fb_helper_initial_config_and_unlock+0x3d3/0x7b0 [ 97.678827] ? drm_setup_crtcs+0x1430/0x1430 [ 97.678827] drm_fb_helper_fbdev_setup+0x12b/0x230 [ 97.678827] bochs_fbdev_init+0x33/0x40 [ 97.678827] bochs_pci_probe+0x197/0x1a0 [ 97.678827] pci_device_probe+0xe9/0x180 [ 97.693848] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support this framebuffer [ 97.694880] bochs-drm 0000:00:02.0: [drm:drm_fb_helper_fbdev_setup] *ERROR* fbdev: Failed to set configuration (ret=-38) More precisely, this is the call chain (obtained via GDB): drm_fb_helper_fbdev_setup -> drm_fb_helper_initial_config -> __drm_fb_helper_initial_config_and_unlock -> drm_fb_helper_single_fb_probe -> bochsfb_create -> drm_gem_fbdev_fb_create -> drm_gem_fb_alloc -> drm_framebuffer_init Let's have a look at the source of the error message, keep in mind that drm_fb_helper_fini is called on the error path: int drm_fb_helper_fbdev_setup(struct drm_device *dev, ...) { /* ... */ ret = drm_fb_helper_initial_config(fb_helper, preferred_bpp); if (ret < 0) { DRM_DEV_ERROR(dev->dev, "fbdev: Failed to set configuration (ret=%d)\n", ret); goto err_drm_fb_helper_fini; } return 0; err_drm_fb_helper_fini: drm_fb_helper_fini(fb_helper); return ret; } The CONFIG_FB_LITTLE_ENDIAN error above suggests that this code path is reached *after* calling bochsfb_create: drm_fb_helper_fbdev_setup -> drm_fb_helper_initial_config -> __drm_fb_helper_initial_config_and_unlock -> register_framebuffer -> do_register_framebuffer -> fb_check_foreignness (prints error and propagates error code back to drm_fb_helper_fbdev_setup). What does "drm_fb_helper_fini" do? Among other things it basically kfree's memory associated with "fb_helper->fbdev" which was created using "drm_fb_helper_alloc_fbi" in the "fb_probe" callback. This is sufficient for "drm_fb_helper_generic_probe" (introduced by Noralf), but not for "bochsfb_create" which additionally calls "drm_gem_fbdev_fb_create": info = drm_fb_helper_alloc_fbi(helper); if (IS_ERR(info)) { DRM_ERROR("Failed to allocate fbi: %ld\n", PTR_ERR(info)); return PTR_ERR(info); } info->par = &bochs->fb.helper; fb = drm_gem_fbdev_fb_create(bochs->dev, sizes, 0, gobj, NULL); if (IS_ERR(fb)) { DRM_ERROR("Failed to create framebuffer: %ld\n", PTR_ERR(fb)); return PTR_ERR(fb); } /* setup helper */ bochs->fb.helper.fb = fb; Note that "fb" is unhandled by "drm_fb_helper_fini", so it leaks. What is the usual behavior? drm_fb_helper_fbdev_setup succeeds and on unload drm_fb_helper_fbdev_teardown is called which properly releases "fb": void drm_fb_helper_fbdev_teardown(struct drm_device *dev) { struct drm_fb_helper *fb_helper = dev->fb_helper; struct fb_ops *fbops = NULL; if (!fb_helper) return; /* Unregister if it hasn't been done already */ if (fb_helper->fbdev && fb_helper->fbdev->dev) drm_fb_helper_unregister_fbi(fb_helper); if (fb_helper->fbdev && fb_helper->fbdev->fbdefio) { fb_deferred_io_cleanup(fb_helper->fbdev); kfree(fb_helper->fbdev->fbdefio); fbops = fb_helper->fbdev->fbops; } drm_fb_helper_fini(fb_helper); kfree(fbops); if (fb_helper->fb) drm_framebuffer_remove(fb_helper->fb); // yay! } Due to calling "drm_fb_helper_fini" however, "dev->fb_helper" will be NULL and thus this function does nothing on the error path. So in summary, "drm_fb_helper_fbdev_setup" calls the driver callback drm_fb_helper_funcs::fb_probe, detects an error but does not properly release all resources from the callback even after calling "drm_fb_helper_fini". On unload, "drm_fb_helper_fbdev_teardown" has no effect because the earlier call to "drm_fb_helper_fini" and skips the required "drm_framebuffer_remove" call. I'll send a proposed patch in a reply. -- Kind regards, Peter Wu https://lekensteyn.nl From mboxrd@z Thu Jan 1 00:00:00 1970 From: Peter Wu Subject: Re: [LKP] [bochs] df2052cc92: WARNING:at_drivers/gpu/drm/drm_mode_config.c:#drm_mode_config_cleanup Date: Sun, 23 Dec 2018 01:43:15 +0100 Message-ID: <20181223004315.GA11455@al> References: <20181221083226.GI23332@shao2-debian> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail.lekensteyn.nl (mail.lekensteyn.nl [IPv6:2a02:2308::360:1:25]) by gabe.freedesktop.org (Postfix) with ESMTPS id 34D216E449 for ; Sun, 23 Dec 2018 01:09:10 +0000 (UTC) Content-Disposition: inline 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: Linus Torvalds Cc: rong.a.chen@intel.com, Daniel Vetter , Linux List Kernel Mailing , dri-devel@lists.freedesktop.org, kraxel@redhat.com, lkp@01.org List-Id: dri-devel@lists.freedesktop.org T24gRnJpLCBEZWMgMjEsIDIwMTggYXQgMTE6MjU6NDFBTSAtMDgwMCwgTGludXMgVG9ydmFsZHMg d3JvdGU6Cj4gT24gRnJpLCBEZWMgMjEsIDIwMTggYXQgMTI6MzIgQU0ga2VybmVsIHRlc3Qgcm9i b3QKPiA8cm9uZy5hLmNoZW5AaW50ZWwuY29tPiB3cm90ZToKPiA+Cj4gPiBGWUksIHdlIG5vdGlj ZWQgY29tbWl0IGRmMjA1MmNjOTIyMSAoImJvY2hzOiBjb252ZXJ0IHRvCj4gPiBkcm1fZmJfaGVs cGVyX2ZiZGV2X3NldHVwL3RlYXJkb3duIikgY2F1c2VkCj4gPgo+ID4gWyAgNDg3LjU5MTczM10g V0FSTklORzogQ1BVOiAwIFBJRDogMSBhdCBkcml2ZXJzL2dwdS9kcm0vZHJtX21vZGVfY29uZmln LmM6NDc4IGRybV9tb2RlX2NvbmZpZ19jbGVhbnVwKzB4MjcwLzB4MjkwCj4gCj4gT2ssIHRoaXMg aXMgYXBwYXJlbnRseSBqdXN0IGEgbGVhayBmb3Igd2hhdCBhcHBlYXJzIHRvIGJlIGEgbm90Cj4g cGFydGljdWxhcmx5IGludGVyZXN0aW5nIGVycm9yIGNhc2UsIGJ1dCB0aGUgd2FybmluZyBpcyBu ZXcgdG8gNC4yMAo+ICgqKSBzbyBpdCB3b3VsZCBiZSBuaWNlIHRvIGhhdmUgc29tZWJvZHkgbG9v ayBhdCBpdC4KPiAKPiBUaGF0IGNvbW1pdCBpcyBzdXBwb3NlZCB0byBmaXggYSBsZWFrLCBidXQg dGhlcmUncyBhcHBhcmVudGx5Cj4gc29tZXRoaW5nIHN0aWxsIHRoZXJlLgoKPiAoKikgdGhlICpw cm9ibGVtKiBpcyBwcm9iYWJseSBub3QgbmV3LCBpdCdzIGp1c3Qgbm93IGV4cG9zZWQgYnkgdGhl Cj4gc3dpdGNoIHRvIGRybV9tb2RlX2NvbmZpZ19jbGVhbnVwKCkuCgpJIGNvbmN1ciwgdGhlIGlz c3VlIHdhcyBvbmx5IHJldmVhbGVkIGJlY2F1c2UgYSAobm90IHNvIGludGVyZXN0aW5nCmVycm9y IHBhdGggd2FzIHRyaWdnZXJlZCkuIFJlcHJvZHVjZWQgdGhpcyBvbiBjdXJyZW50IG1hc3Rlcgoo djQuMjAtcmM3LTI3NC1nMjMyMDNlM2YzNGM5KSwgdGhlIHRyYWNlIGxlYWRpbmcgdXAgdG8gdGhl IHdhcm5pbmcgaXMKdGhlIHNhbWU6CgogICAgWyAgIDUwLjAwODAzMF0gYm9jaHNkcm1mYjogZW5h YmxlIENPTkZJR19GQl9MSVRUTEVfRU5ESUFOIHRvIHN1cHBvcnQgdGhpcyBmcmFtZWJ1ZmZlcgog ICAgWyAgIDUwLjAwOTQzNl0gYm9jaHMtZHJtIDAwMDA6MDA6MDIuMDogW2RybTpkcm1fZmJfaGVs cGVyX2ZiZGV2X3NldHVwXSAqRVJST1IqIGZiZGV2OiBGYWlsZWQgdG8gc2V0IGNvbmZpZ3VyYXRp b24gKHJldD0tMzgpCiAgICBbICAgNTAuMDExNDU2XSBbZHJtXSBJbml0aWFsaXplZCBib2Nocy1k cm0gMS4wLjAgMjAxMzA5MjUgZm9yIDAwMDA6MDA6MDIuMCBvbiBtaW5vciAyCiAgICBbICAgNTAu MDEzNjA0XSBXQVJOSU5HOiBDUFU6IDEgUElEOiAxIGF0IGRyaXZlcnMvZ3B1L2RybS9kcm1fbW9k ZV9jb25maWcuYzo0NzcgZHJtX21vZGVfY29uZmlnX2NsZWFudXArMHgyODAvMHgyYTAKICAgIFsg ICA1MC4wMTYxNzVdIENQVTogMSBQSUQ6IDEgQ29tbTogc3dhcHBlci8wIFRhaW50ZWQ6IEcgICAg ICAgICAgICAgICAgVCA0LjIwLjAtcmM3ICMxCiAgICBbICAgNTAuMDE3NzMyXSBFSVA6IGRybV9t b2RlX2NvbmZpZ19jbGVhbnVwKzB4MjgwLzB4MmEwCiAgICAuLi4KICAgIFsgICA1MC4wMjMxNTVd IENhbGwgVHJhY2U6CiAgICBbICAgNTAuMDIzMTU1XSAgPyBib2Noc19rbXNfZmluaSsweDFlLzB4 MzAKICAgIFsgICA1MC4wMjMxNTVdICA/IGJvY2hzX3VubG9hZCsweDE4LzB4NDAKICAgIFsgICA1 MC4wMjMxNTVdICA/IGJvY2hzX3BjaV9yZW1vdmUrMHgxOC8weDMwCiAgICBbICAgNTAuMDIzMTU1 XSAgPyBwY2lfZGV2aWNlX3JlbW92ZSsweDFjLzB4NTAKICAgIFsgICA1MC4wMzE4ODBdICA/IHJl YWxseV9wcm9iZSsweGYzLzB4MmQwCiAgICBbICAgNTAuMDMxODgwXSAgPyBkcml2ZXJfcHJvYmVf ZGV2aWNlKzB4MjMvMHhhMAoKVGhlIHdhcm5pbmcgc3VnZ2VzdHMgdGhhdCBkcm1fZnJhbWVidWZm ZXJfaW5pdCB3YXMgY2FsbGVkIGF0IHNvbWUgcG9pbnQgd2l0aG91dAphIG1hdGNoaW5nIGNhbGwg dG8gZHJtX2ZyYW1lYnVmZmVyX2NsZWFudXAuIEFkZGluZyBkdW1wX3N0YWNrKCkgcmV2ZWFsczoK CiAgICBbICAgOTcuNjczMzk5XSAgZHJtX2ZyYW1lYnVmZmVyX2luaXQrMHgxN2QvMHgxOTAKICAg IFsgICA5Ny42NzQxMzRdICBkcm1fZ2VtX2ZiX2FsbG9jKzB4YmUvMHgxMjAKICAgIFsgICA5Ny42 NzQ2NzhdICBkcm1fZ2VtX2ZiZGV2X2ZiX2NyZWF0ZSsweDE4NC8weDFjMAogICAgWyAgIDk3LjY3 NTMyMl0gID8gZHJtX2dlbV9mYl9zaW1wbGVfZGlzcGxheV9waXBlX3ByZXBhcmVfZmIrMHgyMC8w eDIwCiAgICBbICAgOTcuNjc2NzcxXSAgPyBkcm1fZmJfaGVscGVyX2FsbG9jX2ZiaSsweGUxLzB4 MTIwCiAgICBbICAgOTcuNjc3NDA4XSAgYm9jaHNmYl9jcmVhdGUrMHgyNDUvMHg1ZjAKICAgIFsg ICA5Ny42Nzc5MzVdICA/IGJvY2hzZmJfbW1hcCsweDYwLzB4NjAKICAgIFsgICA5Ny42Nzg0MjFd ICBfX2RybV9mYl9oZWxwZXJfaW5pdGlhbF9jb25maWdfYW5kX3VubG9jaysweDNkMy8weDdiMAog ICAgWyAgIDk3LjY3ODgyN10gID8gZHJtX3NldHVwX2NydGNzKzB4MTQzMC8weDE0MzAKICAgIFsg ICA5Ny42Nzg4MjddICBkcm1fZmJfaGVscGVyX2ZiZGV2X3NldHVwKzB4MTJiLzB4MjMwCiAgICBb ICAgOTcuNjc4ODI3XSAgYm9jaHNfZmJkZXZfaW5pdCsweDMzLzB4NDAKICAgIFsgICA5Ny42Nzg4 MjddICBib2Noc19wY2lfcHJvYmUrMHgxOTcvMHgxYTAKICAgIFsgICA5Ny42Nzg4MjddICBwY2lf ZGV2aWNlX3Byb2JlKzB4ZTkvMHgxODAKICAgIFsgICA5Ny42OTM4NDhdIGJvY2hzZHJtZmI6IGVu YWJsZSBDT05GSUdfRkJfTElUVExFX0VORElBTiB0byBzdXBwb3J0IHRoaXMgZnJhbWVidWZmZXIK ICAgIFsgICA5Ny42OTQ4ODBdIGJvY2hzLWRybSAwMDAwOjAwOjAyLjA6IFtkcm06ZHJtX2ZiX2hl bHBlcl9mYmRldl9zZXR1cF0gKkVSUk9SKiBmYmRldjogRmFpbGVkIHRvIHNldCBjb25maWd1cmF0 aW9uIChyZXQ9LTM4KQoKTW9yZSBwcmVjaXNlbHksIHRoaXMgaXMgdGhlIGNhbGwgY2hhaW4gKG9i dGFpbmVkIHZpYSBHREIpOgoKICAgIGRybV9mYl9oZWxwZXJfZmJkZXZfc2V0dXAKICAgIC0+IGRy bV9mYl9oZWxwZXJfaW5pdGlhbF9jb25maWcKICAgIC0+IF9fZHJtX2ZiX2hlbHBlcl9pbml0aWFs X2NvbmZpZ19hbmRfdW5sb2NrCiAgICAtPiBkcm1fZmJfaGVscGVyX3NpbmdsZV9mYl9wcm9iZQog ICAgLT4gYm9jaHNmYl9jcmVhdGUKICAgIC0+IGRybV9nZW1fZmJkZXZfZmJfY3JlYXRlCiAgICAt PiBkcm1fZ2VtX2ZiX2FsbG9jCiAgICAtPiBkcm1fZnJhbWVidWZmZXJfaW5pdAoKTGV0J3MgaGF2 ZSBhIGxvb2sgYXQgdGhlIHNvdXJjZSBvZiB0aGUgZXJyb3IgbWVzc2FnZSwga2VlcCBpbiBtaW5k IHRoYXQKZHJtX2ZiX2hlbHBlcl9maW5pIGlzIGNhbGxlZCBvbiB0aGUgZXJyb3IgcGF0aDoKCiAg ICBpbnQgZHJtX2ZiX2hlbHBlcl9mYmRldl9zZXR1cChzdHJ1Y3QgZHJtX2RldmljZSAqZGV2LCAu Li4pIHsKICAgICAgICAvKiAuLi4gKi8KICAgICAgICByZXQgPSBkcm1fZmJfaGVscGVyX2luaXRp YWxfY29uZmlnKGZiX2hlbHBlciwgcHJlZmVycmVkX2JwcCk7CiAgICAgICAgaWYgKHJldCA8IDAp IHsKICAgICAgICAgICAgRFJNX0RFVl9FUlJPUihkZXYtPmRldiwgImZiZGV2OiBGYWlsZWQgdG8g c2V0IGNvbmZpZ3VyYXRpb24gKHJldD0lZClcbiIsIHJldCk7CiAgICAgICAgICAgIGdvdG8gZXJy X2RybV9mYl9oZWxwZXJfZmluaTsKICAgICAgICB9CgogICAgICAgIHJldHVybiAwOwoKICAgIGVy cl9kcm1fZmJfaGVscGVyX2Zpbmk6CiAgICAgICAgZHJtX2ZiX2hlbHBlcl9maW5pKGZiX2hlbHBl cik7CgogICAgICAgIHJldHVybiByZXQ7CiAgICB9CgpUaGUgQ09ORklHX0ZCX0xJVFRMRV9FTkRJ QU4gZXJyb3IgYWJvdmUgc3VnZ2VzdHMgdGhhdCB0aGlzIGNvZGUgcGF0aCBpcyByZWFjaGVkCiph ZnRlciogY2FsbGluZyBib2Noc2ZiX2NyZWF0ZToKCiAgICBkcm1fZmJfaGVscGVyX2ZiZGV2X3Nl dHVwCiAgICAtPiBkcm1fZmJfaGVscGVyX2luaXRpYWxfY29uZmlnCiAgICAtPiBfX2RybV9mYl9o ZWxwZXJfaW5pdGlhbF9jb25maWdfYW5kX3VubG9jawogICAgLT4gcmVnaXN0ZXJfZnJhbWVidWZm ZXIKICAgIC0+IGRvX3JlZ2lzdGVyX2ZyYW1lYnVmZmVyCiAgICAtPiBmYl9jaGVja19mb3JlaWdu bmVzcwogICAgKHByaW50cyBlcnJvciBhbmQgcHJvcGFnYXRlcyBlcnJvciBjb2RlIGJhY2sgdG8g ZHJtX2ZiX2hlbHBlcl9mYmRldl9zZXR1cCkuCgpXaGF0IGRvZXMgImRybV9mYl9oZWxwZXJfZmlu aSIgZG8/IEFtb25nIG90aGVyIHRoaW5ncyBpdCBiYXNpY2FsbHkga2ZyZWUncwptZW1vcnkgYXNz b2NpYXRlZCB3aXRoICJmYl9oZWxwZXItPmZiZGV2IiB3aGljaCB3YXMgY3JlYXRlZCB1c2luZwoi ZHJtX2ZiX2hlbHBlcl9hbGxvY19mYmkiIGluIHRoZSAiZmJfcHJvYmUiIGNhbGxiYWNrLiBUaGlz IGlzIHN1ZmZpY2llbnQgZm9yCiJkcm1fZmJfaGVscGVyX2dlbmVyaWNfcHJvYmUiIChpbnRyb2R1 Y2VkIGJ5IE5vcmFsZiksIGJ1dCBub3QgZm9yCiJib2Noc2ZiX2NyZWF0ZSIgd2hpY2ggYWRkaXRp b25hbGx5IGNhbGxzICJkcm1fZ2VtX2ZiZGV2X2ZiX2NyZWF0ZSI6CgogICAgaW5mbyA9IGRybV9m Yl9oZWxwZXJfYWxsb2NfZmJpKGhlbHBlcik7CiAgICBpZiAoSVNfRVJSKGluZm8pKSB7CiAgICAg ICAgRFJNX0VSUk9SKCJGYWlsZWQgdG8gYWxsb2NhdGUgZmJpOiAlbGRcbiIsIFBUUl9FUlIoaW5m bykpOwogICAgICAgIHJldHVybiBQVFJfRVJSKGluZm8pOwogICAgfQoKICAgIGluZm8tPnBhciA9 ICZib2Nocy0+ZmIuaGVscGVyOwoKICAgIGZiID0gZHJtX2dlbV9mYmRldl9mYl9jcmVhdGUoYm9j aHMtPmRldiwgc2l6ZXMsIDAsIGdvYmosIE5VTEwpOwogICAgaWYgKElTX0VSUihmYikpIHsKICAg ICAgICBEUk1fRVJST1IoIkZhaWxlZCB0byBjcmVhdGUgZnJhbWVidWZmZXI6ICVsZFxuIiwgUFRS X0VSUihmYikpOwogICAgICAgIHJldHVybiBQVFJfRVJSKGZiKTsKICAgIH0KCiAgICAvKiBzZXR1 cCBoZWxwZXIgKi8KICAgIGJvY2hzLT5mYi5oZWxwZXIuZmIgPSBmYjsKCk5vdGUgdGhhdCAiZmIi IGlzIHVuaGFuZGxlZCBieSAiZHJtX2ZiX2hlbHBlcl9maW5pIiwgc28gaXQgbGVha3MuCgpXaGF0 IGlzIHRoZSB1c3VhbCBiZWhhdmlvcj8gZHJtX2ZiX2hlbHBlcl9mYmRldl9zZXR1cCBzdWNjZWVk cyBhbmQgb24gdW5sb2FkCmRybV9mYl9oZWxwZXJfZmJkZXZfdGVhcmRvd24gaXMgY2FsbGVkIHdo aWNoIHByb3Blcmx5IHJlbGVhc2VzICJmYiI6CgogICAgdm9pZCBkcm1fZmJfaGVscGVyX2ZiZGV2 X3RlYXJkb3duKHN0cnVjdCBkcm1fZGV2aWNlICpkZXYpCiAgICB7CiAgICAgICAgc3RydWN0IGRy bV9mYl9oZWxwZXIgKmZiX2hlbHBlciA9IGRldi0+ZmJfaGVscGVyOwogICAgICAgIHN0cnVjdCBm Yl9vcHMgKmZib3BzID0gTlVMTDsKCiAgICAgICAgaWYgKCFmYl9oZWxwZXIpCiAgICAgICAgICAg IHJldHVybjsKCiAgICAgICAgLyogVW5yZWdpc3RlciBpZiBpdCBoYXNuJ3QgYmVlbiBkb25lIGFs cmVhZHkgKi8KICAgICAgICBpZiAoZmJfaGVscGVyLT5mYmRldiAmJiBmYl9oZWxwZXItPmZiZGV2 LT5kZXYpCiAgICAgICAgICAgIGRybV9mYl9oZWxwZXJfdW5yZWdpc3Rlcl9mYmkoZmJfaGVscGVy KTsKCiAgICAgICAgaWYgKGZiX2hlbHBlci0+ZmJkZXYgJiYgZmJfaGVscGVyLT5mYmRldi0+ZmJk ZWZpbykgewogICAgICAgICAgICBmYl9kZWZlcnJlZF9pb19jbGVhbnVwKGZiX2hlbHBlci0+ZmJk ZXYpOwogICAgICAgICAgICBrZnJlZShmYl9oZWxwZXItPmZiZGV2LT5mYmRlZmlvKTsKICAgICAg ICAgICAgZmJvcHMgPSBmYl9oZWxwZXItPmZiZGV2LT5mYm9wczsKICAgICAgICB9CgogICAgICAg IGRybV9mYl9oZWxwZXJfZmluaShmYl9oZWxwZXIpOwogICAgICAgIGtmcmVlKGZib3BzKTsKCiAg ICAgICAgaWYgKGZiX2hlbHBlci0+ZmIpCiAgICAgICAgICAgIGRybV9mcmFtZWJ1ZmZlcl9yZW1v dmUoZmJfaGVscGVyLT5mYik7ICAvLyB5YXkhCiAgICB9CgpEdWUgdG8gY2FsbGluZyAiZHJtX2Zi X2hlbHBlcl9maW5pIiBob3dldmVyLCAiZGV2LT5mYl9oZWxwZXIiIHdpbGwgYmUgTlVMTCBhbmQK dGh1cyB0aGlzIGZ1bmN0aW9uIGRvZXMgbm90aGluZyBvbiB0aGUgZXJyb3IgcGF0aC4KClNvIGlu IHN1bW1hcnksICJkcm1fZmJfaGVscGVyX2ZiZGV2X3NldHVwIiBjYWxscyB0aGUgZHJpdmVyIGNh bGxiYWNrCmRybV9mYl9oZWxwZXJfZnVuY3M6OmZiX3Byb2JlLCBkZXRlY3RzIGFuIGVycm9yIGJ1 dCBkb2VzIG5vdCBwcm9wZXJseSByZWxlYXNlCmFsbCByZXNvdXJjZXMgZnJvbSB0aGUgY2FsbGJh Y2sgZXZlbiBhZnRlciBjYWxsaW5nICJkcm1fZmJfaGVscGVyX2ZpbmkiLiBPbgp1bmxvYWQsICJk cm1fZmJfaGVscGVyX2ZiZGV2X3RlYXJkb3duIiBoYXMgbm8gZWZmZWN0IGJlY2F1c2UgdGhlIGVh cmxpZXIgY2FsbCB0bwoiZHJtX2ZiX2hlbHBlcl9maW5pIiBhbmQgc2tpcHMgdGhlIHJlcXVpcmVk ICJkcm1fZnJhbWVidWZmZXJfcmVtb3ZlIiBjYWxsLgoKSSdsbCBzZW5kIGEgcHJvcG9zZWQgcGF0 Y2ggaW4gYSByZXBseS4KLS0gCktpbmQgcmVnYXJkcywKUGV0ZXIgV3UKaHR0cHM6Ly9sZWtlbnN0 ZXluLm5sCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRy aS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRw czovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Content-Type: multipart/mixed; boundary="===============2597513739874441301==" MIME-Version: 1.0 From: Peter Wu To: lkp@lists.01.org Subject: Re: [bochs] df2052cc92: WARNING:at_drivers/gpu/drm/drm_mode_config.c:#drm_mode_config_cleanup Date: Sun, 23 Dec 2018 01:43:15 +0100 Message-ID: <20181223004315.GA11455@al> In-Reply-To: List-Id: --===============2597513739874441301== Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable On Fri, Dec 21, 2018 at 11:25:41AM -0800, Linus Torvalds wrote: > On Fri, Dec 21, 2018 at 12:32 AM kernel test robot > wrote: > > > > FYI, we noticed commit df2052cc9221 ("bochs: convert to > > drm_fb_helper_fbdev_setup/teardown") caused > > > > [ 487.591733] WARNING: CPU: 0 PID: 1 at drivers/gpu/drm/drm_mode_confi= g.c:478 drm_mode_config_cleanup+0x270/0x290 > = > Ok, this is apparently just a leak for what appears to be a not > particularly interesting error case, but the warning is new to 4.20 > (*) so it would be nice to have somebody look at it. > = > That commit is supposed to fix a leak, but there's apparently > something still there. > (*) the *problem* is probably not new, it's just now exposed by the > switch to drm_mode_config_cleanup(). I concur, the issue was only revealed because a (not so interesting error path was triggered). Reproduced this on current master (v4.20-rc7-274-g23203e3f34c9), the trace leading up to the warning is the same: [ 50.008030] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support th= is framebuffer [ 50.009436] bochs-drm 0000:00:02.0: [drm:drm_fb_helper_fbdev_setup] = *ERROR* fbdev: Failed to set configuration (ret=3D-38) [ 50.011456] [drm] Initialized bochs-drm 1.0.0 20130925 for 0000:00:0= 2.0 on minor 2 [ 50.013604] WARNING: CPU: 1 PID: 1 at drivers/gpu/drm/drm_mode_confi= g.c:477 drm_mode_config_cleanup+0x280/0x2a0 [ 50.016175] CPU: 1 PID: 1 Comm: swapper/0 Tainted: G = T 4.20.0-rc7 #1 [ 50.017732] EIP: drm_mode_config_cleanup+0x280/0x2a0 ... [ 50.023155] Call Trace: [ 50.023155] ? bochs_kms_fini+0x1e/0x30 [ 50.023155] ? bochs_unload+0x18/0x40 [ 50.023155] ? bochs_pci_remove+0x18/0x30 [ 50.023155] ? pci_device_remove+0x1c/0x50 [ 50.031880] ? really_probe+0xf3/0x2d0 [ 50.031880] ? driver_probe_device+0x23/0xa0 The warning suggests that drm_framebuffer_init was called at some point wit= hout a matching call to drm_framebuffer_cleanup. Adding dump_stack() reveals: [ 97.673399] drm_framebuffer_init+0x17d/0x190 [ 97.674134] drm_gem_fb_alloc+0xbe/0x120 [ 97.674678] drm_gem_fbdev_fb_create+0x184/0x1c0 [ 97.675322] ? drm_gem_fb_simple_display_pipe_prepare_fb+0x20/0x20 [ 97.676771] ? drm_fb_helper_alloc_fbi+0xe1/0x120 [ 97.677408] bochsfb_create+0x245/0x5f0 [ 97.677935] ? bochsfb_mmap+0x60/0x60 [ 97.678421] __drm_fb_helper_initial_config_and_unlock+0x3d3/0x7b0 [ 97.678827] ? drm_setup_crtcs+0x1430/0x1430 [ 97.678827] drm_fb_helper_fbdev_setup+0x12b/0x230 [ 97.678827] bochs_fbdev_init+0x33/0x40 [ 97.678827] bochs_pci_probe+0x197/0x1a0 [ 97.678827] pci_device_probe+0xe9/0x180 [ 97.693848] bochsdrmfb: enable CONFIG_FB_LITTLE_ENDIAN to support th= is framebuffer [ 97.694880] bochs-drm 0000:00:02.0: [drm:drm_fb_helper_fbdev_setup] = *ERROR* fbdev: Failed to set configuration (ret=3D-38) More precisely, this is the call chain (obtained via GDB): drm_fb_helper_fbdev_setup -> drm_fb_helper_initial_config -> __drm_fb_helper_initial_config_and_unlock -> drm_fb_helper_single_fb_probe -> bochsfb_create -> drm_gem_fbdev_fb_create -> drm_gem_fb_alloc -> drm_framebuffer_init Let's have a look at the source of the error message, keep in mind that drm_fb_helper_fini is called on the error path: int drm_fb_helper_fbdev_setup(struct drm_device *dev, ...) { /* ... */ ret =3D drm_fb_helper_initial_config(fb_helper, preferred_bpp); if (ret < 0) { DRM_DEV_ERROR(dev->dev, "fbdev: Failed to set configuration (re= t=3D%d)\n", ret); goto err_drm_fb_helper_fini; } return 0; err_drm_fb_helper_fini: drm_fb_helper_fini(fb_helper); return ret; } The CONFIG_FB_LITTLE_ENDIAN error above suggests that this code path is rea= ched *after* calling bochsfb_create: drm_fb_helper_fbdev_setup -> drm_fb_helper_initial_config -> __drm_fb_helper_initial_config_and_unlock -> register_framebuffer -> do_register_framebuffer -> fb_check_foreignness (prints error and propagates error code back to drm_fb_helper_fbdev_set= up). What does "drm_fb_helper_fini" do? Among other things it basically kfree's memory associated with "fb_helper->fbdev" which was created using "drm_fb_helper_alloc_fbi" in the "fb_probe" callback. This is sufficient for "drm_fb_helper_generic_probe" (introduced by Noralf), but not for "bochsfb_create" which additionally calls "drm_gem_fbdev_fb_create": info =3D drm_fb_helper_alloc_fbi(helper); if (IS_ERR(info)) { DRM_ERROR("Failed to allocate fbi: %ld\n", PTR_ERR(info)); return PTR_ERR(info); } info->par =3D &bochs->fb.helper; fb =3D drm_gem_fbdev_fb_create(bochs->dev, sizes, 0, gobj, NULL); if (IS_ERR(fb)) { DRM_ERROR("Failed to create framebuffer: %ld\n", PTR_ERR(fb)); return PTR_ERR(fb); } /* setup helper */ bochs->fb.helper.fb =3D fb; Note that "fb" is unhandled by "drm_fb_helper_fini", so it leaks. What is the usual behavior? drm_fb_helper_fbdev_setup succeeds and on unload drm_fb_helper_fbdev_teardown is called which properly releases "fb": void drm_fb_helper_fbdev_teardown(struct drm_device *dev) { struct drm_fb_helper *fb_helper =3D dev->fb_helper; struct fb_ops *fbops =3D NULL; if (!fb_helper) return; /* Unregister if it hasn't been done already */ if (fb_helper->fbdev && fb_helper->fbdev->dev) drm_fb_helper_unregister_fbi(fb_helper); if (fb_helper->fbdev && fb_helper->fbdev->fbdefio) { fb_deferred_io_cleanup(fb_helper->fbdev); kfree(fb_helper->fbdev->fbdefio); fbops =3D fb_helper->fbdev->fbops; } drm_fb_helper_fini(fb_helper); kfree(fbops); if (fb_helper->fb) drm_framebuffer_remove(fb_helper->fb); // yay! } Due to calling "drm_fb_helper_fini" however, "dev->fb_helper" will be NULL = and thus this function does nothing on the error path. So in summary, "drm_fb_helper_fbdev_setup" calls the driver callback drm_fb_helper_funcs::fb_probe, detects an error but does not properly relea= se all resources from the callback even after calling "drm_fb_helper_fini". On unload, "drm_fb_helper_fbdev_teardown" has no effect because the earlier ca= ll to "drm_fb_helper_fini" and skips the required "drm_framebuffer_remove" call. I'll send a proposed patch in a reply. -- = Kind regards, Peter Wu https://lekensteyn.nl --===============2597513739874441301==--