| Message ID | 1740473808-9754-1-git-send-email-ssengar@linux.microsoft.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [v2] fbdev: hyperv_fb: Allow graceful removal of framebuffer | expand |
From: Saurabh Sengar <ssengar@linux.microsoft.com> > > 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 and assosiated memory > to fb_ops.fb_destroy function, so that framebuffer framework handles > it gracefully. > > 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> > --- > V2 : Move hvfb_putmem into destroy() > > drivers/video/fbdev/hyperv_fb.c | 28 ++++++++++++++++++++++------ > 1 file changed, 22 insertions(+), 6 deletions(-) > > diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c > index 363e4ccfcdb7..89ee49f1c3dc 100644 > --- a/drivers/video/fbdev/hyperv_fb.c > +++ b/drivers/video/fbdev/hyperv_fb.c > @@ -282,6 +282,8 @@ static uint screen_depth; > static uint screen_fb_size; > static uint dio_fb_size; /* FB size for deferred IO */ > > +static void hvfb_putmem(struct hv_device *hdev, struct fb_info *info); > + > /* Send message to Hyper-V host */ > static inline int synthvid_send(struct hv_device *hdev, > struct synthvid_msg *msg) > @@ -862,6 +864,24 @@ 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) > +{ > + struct hv_device *hdev; > + struct device *dev; > + void *driver_data = (void *)info; > + > + dev = container_of(driver_data, struct device, driver_data); I don't think the above is right. The struct fb_info was allocated with kzalloc() in framebuffer_alloc(). You would use container_of() if fb_info was embedded in a struct device, but that's not the case here. The driver_data field within the struct device is a pointer to the fb_info, but that doesn't help find the struct device. What does help is that info->device (not to be confused with info->dev, which is a different struct device) points to the struct device that you need here. That "device" field is set in framebuffer_alloc(). So that's an easy fix. > + hdev = container_of(dev, struct hv_device, device); > + > + hvfb_putmem(hdev, info); Another observation: The interface to hvfb_putmem() is more complicated than it needs to be. The hdev argument could be dropped because it is used only to get the device pointer, which is already stored in info->device. hvfb_release_phymem() would also need to be updated to take a struct device instead of struct hv_device. But if you made those changes, then the container_of() to get the hdev wouldn't be needed either. A similar simplification could be applied to hvfb_getmem(). Maybe do two patches -- the first to simplify the interfaces, and the second to do this patch but with reduced complexity because of the simpler interfaces. Michael > + 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 +897,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 +1193,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 +1241,9 @@ 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
On Tue, Feb 25, 2025 at 04:46:12PM +0000, Michael Kelley wrote: > From: Saurabh Sengar <ssengar@linux.microsoft.com> > > > > 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 and assosiated memory > > to fb_ops.fb_destroy function, so that framebuffer framework handles > > it gracefully. > > > > 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> > > --- > > V2 : Move hvfb_putmem into destroy() > > > > drivers/video/fbdev/hyperv_fb.c | 28 ++++++++++++++++++++++------ > > 1 file changed, 22 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c > > index 363e4ccfcdb7..89ee49f1c3dc 100644 > > --- a/drivers/video/fbdev/hyperv_fb.c > > +++ b/drivers/video/fbdev/hyperv_fb.c > > @@ -282,6 +282,8 @@ static uint screen_depth; > > static uint screen_fb_size; > > static uint dio_fb_size; /* FB size for deferred IO */ > > > > +static void hvfb_putmem(struct hv_device *hdev, struct fb_info *info); > > + > > /* Send message to Hyper-V host */ > > static inline int synthvid_send(struct hv_device *hdev, > > struct synthvid_msg *msg) > > @@ -862,6 +864,24 @@ 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) > > +{ > > + struct hv_device *hdev; > > + struct device *dev; > > + void *driver_data = (void *)info; > > + > > + dev = container_of(driver_data, struct device, driver_data); > > I don't think the above is right. The struct fb_info was allocated > with kzalloc() in framebuffer_alloc(). You would use container_of() > if fb_info was embedded in a struct device, but that's not the case > here. The driver_data field within the struct device is a pointer to the > fb_info, but that doesn't help find the struct device. Thanks for catching this. I should have been more careful. > > What does help is that info->device (not to be confused with info->dev, > which is a different struct device) points to the struct device that > you need here. That "device" field is set in framebuffer_alloc(). > So that's an easy fix. Right, thanks. > > > + hdev = container_of(dev, struct hv_device, device); > > + > > + hvfb_putmem(hdev, info); > > Another observation: The interface to hvfb_putmem() is more > complicated than it needs to be. The hdev argument could be > dropped because it is used only to get the device pointer, > which is already stored in info->device. hvfb_release_phymem() > would also need to be updated to take a struct device instead of > struct hv_device. But if you made those changes, then the > container_of() to get the hdev wouldn't be needed either. Make sense. > > A similar simplification could be applied to hvfb_getmem(). > > Maybe do two patches -- the first to simplify the interfaces, > and the second to do this patch but with reduced > complexity because of the simpler interfaces. Agree, let me do it in V3. - Saurabh > > Michael > <snip>
diff --git a/drivers/video/fbdev/hyperv_fb.c b/drivers/video/fbdev/hyperv_fb.c index 363e4ccfcdb7..89ee49f1c3dc 100644 --- a/drivers/video/fbdev/hyperv_fb.c +++ b/drivers/video/fbdev/hyperv_fb.c @@ -282,6 +282,8 @@ static uint screen_depth; static uint screen_fb_size; static uint dio_fb_size; /* FB size for deferred IO */ +static void hvfb_putmem(struct hv_device *hdev, struct fb_info *info); + /* Send message to Hyper-V host */ static inline int synthvid_send(struct hv_device *hdev, struct synthvid_msg *msg) @@ -862,6 +864,24 @@ 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) +{ + struct hv_device *hdev; + struct device *dev; + void *driver_data = (void *)info; + + dev = container_of(driver_data, struct device, driver_data); + hdev = container_of(dev, struct hv_device, device); + + hvfb_putmem(hdev, 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 +897,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 +1193,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 +1241,9 @@ 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)
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 and assosiated memory to fb_ops.fb_destroy function, so that framebuffer framework handles it gracefully. 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> --- V2 : Move hvfb_putmem into destroy() drivers/video/fbdev/hyperv_fb.c | 28 ++++++++++++++++++++++------ 1 file changed, 22 insertions(+), 6 deletions(-)