| Message ID | 8737ewvf8v.fsf_-_@dilma.collabora.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Mar 01, 2017 at 06:50:08PM -0300, Gabriel Krisman Bertazi wrote: > Daniel Vetter <daniel@ffwll.ch> writes: > > Hi Daniel, thanks again for your review. > > > Hm, I bit silly that we need #ifdef here, all this stuff is meant to Just > > Work. Can't we just fix the oops with suitable checks in the fini paths > > instead? > > Heh. I expected someone would bring up this objection. My rationale is > that on the driver side, and for many drivers, fbdev emulation code is > always executed, even if it is only calling a bunch of NOP functions on > the drm core, which is a bit silly. I wonder if struct drm_driver could > have a new callback for fbdev_initialization, which we could avoid > calling in drm_core (maybe at registering time) if FBDEV_EMULATION is > disabled. (well, this would go a bit in the opposite direction of having > open coded probe sequences). > > Anyway, for the Oops I mentioned, can you consider the patch below, > instead? lgtm. -Daniel > > > > > Also 0day has hit some oops in bochs, it might suffer from similar bugs. > > I can take a look at that too. > > -- >8 -- > Subject: [PATCH] drm: qxl: Don't clean up uninitialized fbdev framebuffer > > If fbdev emulation is disabled, the QXL shutdown path still tries to > clean the framebuffer, which wasn't initialized, hitting the Oops below. > We can check for the qfb->obj element to ensure the fb was initialized, > because the qfb always has a bo object associated with it since > initialization time. > > [ 24.284684] BUG: unable to handle kernel NULL pointer dereference at 00000000000002e0 > [ 24.285627] IP: mutex_lock+0x18/0x30 > [ 24.286049] PGD 78cdf067 > [ 24.286050] PUD 7940f067 > [ 24.286344] PMD 0 > [ 24.286649] > [ 24.287072] Oops: 0002 [#1] SMP > [ 24.287422] Modules linked in: qxl > [ 24.287806] CPU: 0 PID: 2328 Comm: bash Not tainted 4.10.0-rc5+ #97 > [ 24.288515] Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.9.3-20161025_171302-gandalf 04/01/2014 > [ 24.289681] task: ffff88007c4c0000 task.stack: ffffc90001b58000 > [ 24.290354] RIP: 0010:mutex_lock+0x18/0x30 > [ 24.290812] RSP: 0018:ffffc90001b5bcb0 EFLAGS: 00010246 > [ 24.291401] RAX: 0000000000000000 RBX: 00000000000002e0 RCX: 0000000000000000 > [ 24.292209] RDX: ffff88007c4c0000 RSI: 0000000000000001 RDI: 00000000000002e0 > [ 24.292987] RBP: ffffc90001b5bcb8 R08: fffffffffffffffe R09: 0000000000000001 > [ 24.293797] R10: ffff880078d80b80 R11: 0000000000011400 R12: 0000000000000000 > [ 24.294601] R13: 00000000000002e0 R14: ffffffffa0009c28 R15: 0000000000000060 > [ 24.295439] FS: 00007f30e3acbb40(0000) GS:ffff88007fc00000(0000) knlGS:0000000000000000 > [ 24.296364] CS: 0010 DS: 0000 ES: 0000 CR0: 0000000080050033 > [ 24.296997] CR2: 00000000000002e0 CR3: 0000000078c7b000 CR4: 00000000000006f0 > [ 24.297813] Call Trace: > [ 24.298097] drm_framebuffer_cleanup+0x1f/0x70 > [ 24.298612] qxl_fbdev_fini+0x68/0x90 [qxl] > [ 24.299074] qxl_modeset_fini+0xd/0x30 [qxl] > [ 24.299562] qxl_pci_remove+0x22/0x50 [qxl] > [ 24.300025] pci_device_remove+0x34/0xb0 > [ 24.300507] device_release_driver_internal+0x150/0x200 > [ 24.301082] device_release_driver+0xd/0x10 > [ 24.301587] unbind_store+0x108/0x150 > [ 24.301993] drv_attr_store+0x20/0x30 > [ 24.302402] sysfs_kf_write+0x32/0x40 > [ 24.302827] kernfs_fop_write+0x108/0x190 > [ 24.303269] __vfs_write+0x23/0x120 > [ 24.303678] ? security_file_permission+0x36/0xb0 > [ 24.304193] ? rw_verify_area+0x49/0xb0 > [ 24.304636] vfs_write+0xb0/0x190 > [ 24.305004] SyS_write+0x41/0xa0 > [ 24.305362] entry_SYSCALL_64_fastpath+0x1a/0xa9 > [ 24.305887] RIP: 0033:0x7f30e31d9620 > [ 24.306285] RSP: 002b:00007ffc54b47e68 EFLAGS: 00000246 ORIG_RAX: 0000000000000001 > [ 24.307128] RAX: ffffffffffffffda RBX: 00007f30e3497600 RCX: 00007f30e31d9620 > [ 24.307928] RDX: 000000000000000d RSI: 0000000000da2008 RDI: 0000000000000001 > [ 24.308727] RBP: 000000000070bc60 R08: 00007f30e3498760 R09: 00007f30e3acbb40 > [ 24.309504] R10: 0000000000000073 R11: 0000000000000246 R12: 0000000000000001 > [ 24.310295] R13: 0000000000000000 R14: 0000000000000000 R15: 00007ffc54b47f34 > [ 24.311095] Code: 0e 01 e9 7b fe ff ff 66 90 66 2e 0f 1f 84 00 00 00 00 00 > 55 48 89 e5 53 48 89 fb e8 83 e8 ff ff 65 48 8b 14 25 40 c4 00 00 31 c0 <3e> > 48 0f b1 13 48 85 c0 74 08 48 89 df e8 66 fd ff ff 5b 5d c3 > [ 24.313182] RIP: mutex_lock+0x18/0x30 RSP: ffffc90001b5bcb0 > [ 24.313811] CR2: 00000000000002e0 > [ 24.314208] ---[ end trace 29669c1593cae14b ]--- > > Signed-off-by: Gabriel Krisman Bertazi <krisman@collabora.co.uk> > --- > drivers/gpu/drm/qxl/qxl_fb.c | 11 +++++++---- > 1 file changed, 7 insertions(+), 4 deletions(-) > > diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c > index 35124737666e..4d6c311e8e5e 100644 > --- a/drivers/gpu/drm/qxl/qxl_fb.c > +++ b/drivers/gpu/drm/qxl/qxl_fb.c > @@ -351,13 +351,16 @@ static int qxl_fbdev_destroy(struct drm_device *dev, struct qxl_fbdev *qfbdev) > > drm_fb_helper_unregister_fbi(&qfbdev->helper); > > - if (qfb->obj) { > + if (qfb->obj) > qxlfb_destroy_pinned_object(qfb->obj); > - qfb->obj = NULL; > - } > + > drm_fb_helper_fini(&qfbdev->helper); > vfree(qfbdev->shadow); > - drm_framebuffer_cleanup(&qfb->base); > + > + if (qfb->obj) > + drm_framebuffer_cleanup(&qfb->base); btw if you're looking for more cleanup work: Embeddeding drm_framebuffer like this is deprecated, since it wreaks the refcounting. There's comments around the relevant functions, and iirc we also have a todo somewhere. -Daniel > + > + qfb->obj = NULL; > > return 0; > } > -- > 2.11.0 >
diff --git a/drivers/gpu/drm/qxl/qxl_fb.c b/drivers/gpu/drm/qxl/qxl_fb.c index 35124737666e..4d6c311e8e5e 100644 --- a/drivers/gpu/drm/qxl/qxl_fb.c +++ b/drivers/gpu/drm/qxl/qxl_fb.c @@ -351,13 +351,16 @@ static int qxl_fbdev_destroy(struct drm_device *dev, struct qxl_fbdev *qfbdev) drm_fb_helper_unregister_fbi(&qfbdev->helper); - if (qfb->obj) { + if (qfb->obj) qxlfb_destroy_pinned_object(qfb->obj); - qfb->obj = NULL; - } + drm_fb_helper_fini(&qfbdev->helper); vfree(qfbdev->shadow); - drm_framebuffer_cleanup(&qfb->base); + + if (qfb->obj) + drm_framebuffer_cleanup(&qfb->base); + + qfb->obj = NULL; return 0; }