From: Saurabh Singh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx> Sent: Sunday, February 23, 2025 6:10 AM > > On Sat, Feb 22, 2025 at 08:16:53PM +0000, Michael Kelley wrote: > > From: Saurabh Singh Sengar <ssengar@xxxxxxxxxxxxxxxxxxx> Sent: Saturday, February 22, 2025 9:27 AM > > > [anip] > > > > > > I had considered moving the entire `hvfb_putmem()` function to `destroy`, > > > but I was hesitant for two reasons: > > > > > > 1. I wasn’t aware of any scenario where this would be useful. However, > > > your explanation has convinced me that it is necessary. > > > 2. `hvfb_release_phymem()` relies on the `hdev` pointer, which requires > > > multiple `container_of` operations to derive it from the `info` pointer. > > > I was unsure if the complexity was justified, but it seems worthwhile now. > > > > > > I will move `hvfb_putmem()` to the `destroy` function in V2, and I hope this > > > will address all the cases you mentioned. > > > > > > > Yes, that's what I expect needs to happen, though I haven't looked at the > > details of making sure all the needed data structures are still around. Like > > you, I just had this sense that hvfb_putmem() might need to be moved as > > well, so I tried to produce a failure scenario to prove it, which turned out > > to be easy. > > > > Michael > > I will add this in V2 as well. But I have found an another issue which is > not very frequent. > > > [ 176.562153] ------------[ cut here ]------------ > [ 176.562159] fb0: fb_WARN_ON_ONCE(pageref->page != page) > [ 176.562176] WARNING: CPU: 50 PID: 1522 at drivers/video/fbdev/core/fb_defio.c:67 > fb_deferred_io_mkwrite+0x215/0x280 > > <snip> > > [ 176.562258] Call Trace: > [ 176.562260] <TASK> > [ 176.562263] ? show_regs+0x6c/0x80 > [ 176.562269] ? __warn+0x8d/0x150 > [ 176.562273] ? fb_deferred_io_mkwrite+0x215/0x280 > [ 176.562275] ? report_bug+0x182/0x1b0 > [ 176.562280] ? handle_bug+0x133/0x1a0 > [ 176.562283] ? exc_invalid_op+0x18/0x80 > [ 176.562284] ? asm_exc_invalid_op+0x1b/0x20 > [ 176.562289] ? fb_deferred_io_mkwrite+0x215/0x280 > [ 176.562291] ? fb_deferred_io_mkwrite+0x215/0x280 > [ 176.562293] do_page_mkwrite+0x4d/0xb0 > [ 176.562296] do_wp_page+0xe8/0xd50 > [ 176.562300] ? ___pte_offset_map+0x1c/0x1b0 > [ 176.562304] __handle_mm_fault+0xbe1/0x10e0 > [ 176.562307] handle_mm_fault+0x17f/0x2e0 > [ 176.562309] do_user_addr_fault+0x2d1/0x8d0 > [ 176.562314] exc_page_fault+0x85/0x1e0 > [ 176.562318] asm_exc_page_fault+0x27/0x30 > > Looks this is because driver is unbind still Xorg is trying to write > to memory which is causing some page faults. I have confirmed PID 1522 > is of Xorg. I think this is because we need to cancel the framebuffer > deferred work after flushing it. Does this new issue occur even after moving hvfb_putmem() into the destroy() function? I'm hoping it doesn't. I've looked at the fb_deferred_io code, and can't quite figure out how that deferred I/O work is supposed to get cancelled. Or maybe it's just not supposed to get started again after the flush. If the new issue still happens, that seems like more of a flaw in the fb deferred I/O mechanism not shutting itself down properly. Michael > > After adding the below in hvfb_remove I don't see this issue anymore. > Although as the issue is not very frequent I am not 100% sure. > > cancel_delayed_work_sync(&info->deferred_work); > > If you think this is reasonable I can add this as well in V2. >
