fbdev: hyperv_fb: Allow graceful removal of framebuffer

Message ID	1739611240-9512-1-git-send-email-ssengar@linux.microsoft.com (mailing list archive)
State	New
Headers	show Return-Path: <dri-devel-bounces@lists.freedesktop.org> From: Saurabh Sengar <ssengar@linux.microsoft.com> To: kys@microsoft.com, haiyangz@microsoft.com, wei.liu@kernel.org, decui@microsoft.com, deller@gmx.de, akpm@linux-foundation.org, linux-hyperv@vger.kernel.org, linux-fbdev@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Cc: ssengar@microsoft.com, mhklinux@outlook.com Subject: [PATCH] fbdev: hyperv_fb: Allow graceful removal of framebuffer Date: Sat, 15 Feb 2025 01:20:40 -0800 Message-Id: <1739611240-9512-1-git-send-email-ssengar@linux.microsoft.com> Precedence: list Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" <dri-devel-bounces@lists.freedesktop.org>
Series	fbdev: hyperv_fb: Allow graceful removal of framebuffer \| expand fbdev: hyperv_fb: Allow graceful removal of framebuffer

From: Saurabh Sengar <ssengar@linux.microsoft.com> Sent: Saturday, February 15, 2025 1:21 AM > > When a Hyper-V framebuffer device is unbind, hyperv_fb driver tries to > release the framebuffer forcefully. If this framebuffer is in use it > produce the following WARN and hence this framebuffer is never released. > > [ 44.111220] WARNING: CPU: 35 PID: 1882 at drivers/video/fbdev/core/fb_info.c:70 framebuffer_release+0x2c/0x40 > < snip > > [ 44.111289] Call Trace: > [ 44.111290] <TASK> > [ 44.111291] ? show_regs+0x6c/0x80 > [ 44.111295] ? __warn+0x8d/0x150 > [ 44.111298] ? framebuffer_release+0x2c/0x40 > [ 44.111300] ? report_bug+0x182/0x1b0 > [ 44.111303] ? handle_bug+0x6e/0xb0 > [ 44.111306] ? exc_invalid_op+0x18/0x80 > [ 44.111308] ? asm_exc_invalid_op+0x1b/0x20 > [ 44.111311] ? framebuffer_release+0x2c/0x40 > [ 44.111313] ? hvfb_remove+0x86/0xa0 [hyperv_fb] > [ 44.111315] vmbus_remove+0x24/0x40 [hv_vmbus] > [ 44.111323] device_remove+0x40/0x80 > [ 44.111325] device_release_driver_internal+0x20b/0x270 > [ 44.111327] ? bus_find_device+0xb3/0xf0 > > Fix this by moving the release of framebuffer to fb_ops.fb_destroy function > so that framebuffer framework handles it gracefully These changes look good for solving the specific problem where the reference count WARN is produced. But there is another problem of the same type that happens when doing unbind of a hyperv_fb device that is in use (i.e., /dev/fb0 is open and mmap'ed by some user space program). For this additional problem, there are three sub-cases, depending on what memory gets mmap'ed into user space. Two of the three sub-cases have a problem. 1) When Hyper-V FB uses deferred I/O, the vmalloc dio memory is what get mapped into user space. When hyperv_fb is unbound, the vmalloc dio memory is freed. But the memory doesn't actually get freed if it is still mmap'ed into user space. The deferred I/O mechanism is stopped, but user space can keep writing to the memory even though the pixels don't get copied to the actual framebuffer any longer. When the user space program terminates (or unmaps the memory), the memory will be freed. So this case is OK, though perhaps a bit dubious. 2) When Hyper-V FB is in a Gen 1 VM, and the frame buffer size is <= 4 MiB, a normal kernel allocation is used for the memory that is mmap'ed to user space. If this memory is freed when hyperv_fb is unbound, bad things happen because the memory is still being written to via the user space mmap. There are multiple "BUG: Bad page state in process bash pfn:106c65" errors followed by stack traces. 3) Similarly in a Gen 1 VM, if the frame buffer size is > 4 MiB, CMA memory is allocated (assuming it is available). This CMA memory gets mapped into user space. When hyperv_fb is unbound, that memory is freed. But CMA complains that the ref count on the pages is not zero. Here's the dmesg output: [ 191.629780] ------------[ cut here ]------------ [ 191.629784] 200 pages are still in use! [ 191.629789] WARNING: CPU: 3 PID: 1115 at mm/page_alloc.c:6757 free_contig_range+0x15e/0x170 Stack trace is: [ 191.629847] ? __warn+0x97/0x160 [ 191.629849] ? free_contig_range+0x15e/0x170 [ 191.629849] ? report_bug+0x1bb/0x1d0 [ 191.629851] ? console_unlock+0xdd/0x1e0 [ 191.629854] ? handle_bug+0x60/0xa0 [ 191.629857] ? exc_invalid_op+0x1d/0x80 [ 191.629859] ? asm_exc_invalid_op+0x1f/0x30 [ 191.629862] ? free_contig_range+0x15e/0x170 [ 191.629862] ? free_contig_range+0x15e/0x170 [ 191.629863] cma_release+0xc6/0x150 [ 191.629865] dma_free_contiguous+0x34/0x70 [ 191.629868] dma_direct_free+0xd3/0x130 [ 191.629869] dma_free_attrs+0x6b/0x130 [ 191.629872] hvfb_putmem.isra.0+0x99/0xd0 [hyperv_fb] [ 191.629874] hvfb_remove+0x75/0x80 [hyperv_fb] [ 191.629876] vmbus_remove+0x28/0x40 [hv_vmbus] [ 191.629883] device_remove+0x43/0x70 [ 191.629886] device_release_driver_internal+0xbd/0x140 [ 191.629888] device_driver_detach+0x18/0x20 [ 191.629890] unbind_store+0x8f/0xa0 [ 191.629891] drv_attr_store+0x25/0x40 [ 191.629892] sysfs_kf_write+0x3f/0x50 [ 191.629894] kernfs_fop_write_iter+0x142/0x1d0 [ 191.629896] vfs_write+0x31b/0x450 [ 191.629898] ksys_write+0x6e/0xe0 [ 191.629899] __x64_sys_write+0x1e/0x30 [ 191.629900] x64_sys_call+0x16bf/0x2150 [ 191.629903] do_syscall_64+0x4e/0x110 [ 191.629904] entry_SYSCALL_64_after_hwframe+0x76/0x7e For all three cases, I think the memory freeing and iounmap() operations can be moved to the new hvfb_destroy() function so that the memory is cleaned up only when there aren't any users. While these additional changes could be done as a separate patch, it seems to me like they are all part of the same underlying issue as the reference count problem, and could be combined into this patch. Michael > > While we fix this, also replace manual registrations/unregistration of > framebuffer with devm_register_framebuffer. > > Fixes: 68a2d20b79b1 ("drivers/video: add Hyper-V Synthetic Video Frame Buffer Driver") > Signed-off-by: Saurabh Sengar <ssengar@linux.microsoft.com> > --- > drivers/video/fbdev/hyperv_fb.c | 16 ++++++++++++---- > 1 file changed, 12 insertions(+), 4 deletions(-) > > diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c > index 363e4ccfcdb7..83b1ab4da984 100644 > --- a/drivers/video/fbdev/hyperv_fb.c > +++ b/drivers/video/fbdev/hyperv_fb.c > @@ -862,6 +862,16 @@ static void hvfb_ops_damage_area(struct fb_info *info, u32 x, u32 y, u32 width, > hvfb_ondemand_refresh_throttle(par, x, y, width, height); > } > > +/* > + * fb_ops.fb_destroy is called by the last put_fb_info() call at the end > + * of unregister_framebuffer() or fb_release(). Do any cleanup related to > + * framebuffer here. > + */ > +static void hvfb_destroy(struct fb_info *info) > +{ > + framebuffer_release(info); > +} > + > /* > * TODO: GEN1 codepaths allocate from system or DMA-able memory. Fix the > * driver to use the _SYSMEM_ or _DMAMEM_ helpers in these cases. > @@ -877,6 +887,7 @@ static const struct fb_ops hvfb_ops = { > .fb_set_par = hvfb_set_par, > .fb_setcolreg = hvfb_setcolreg, > .fb_blank = hvfb_blank, > + .fb_destroy = hvfb_destroy, > }; > > /* Get options from kernel paramenter "video=" */ > @@ -1172,7 +1183,7 @@ static int hvfb_probe(struct hv_device *hdev, > if (ret) > goto error; > > - ret = register_framebuffer(info); > + ret = devm_register_framebuffer(&hdev->device, info); > if (ret) { > pr_err("Unable to register framebuffer\n"); > goto error; > @@ -1220,14 +1231,11 @@ static void hvfb_remove(struct hv_device *hdev) > > fb_deferred_io_cleanup(info); > > - unregister_framebuffer(info); > cancel_delayed_work_sync(&par->dwork); > > vmbus_close(hdev->channel); > - hv_set_drvdata(hdev, NULL); > > hvfb_putmem(hdev, info); > - framebuffer_release(info); > } > > static int hvfb_suspend(struct hv_device *hdev) > -- > 2.43.0

fbdev: hyperv_fb: Allow graceful removal of framebuffer

Commit Message

Comments

Patch