On Mon, 30 Jan 2023 09:52:43 +0100,
Takashi Iwai wrote:
>
> > There's a call to cancel_delayed_work_sync() in the new helper
> > fb_deferred_io_release(). Is this the right function? Maybe
> > flush_delayed_work() is a better choice.
>
> I thought of that, but took a shorter path.
> OK, let's check whether this keeps working with that change.
Interestingly, this still makes the bug happening at the very same
place. (The tested patch is below.)
So, more looking and testing, more confusing the problem becomes :-<
Takashi
-- 8< --
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -313,20 +313,31 @@ void fb_deferred_io_open(struct fb_info *info,
}
EXPORT_SYMBOL_GPL(fb_deferred_io_open);
-void fb_deferred_io_cleanup(struct fb_info *info)
+/* clear out the mapping that we setup */
+static void fb_deferred_io_clear_mapping(struct fb_info *info)
{
- struct fb_deferred_io *fbdefio = info->fbdefio;
struct page *page;
int i;
- BUG_ON(!fbdefio);
- cancel_delayed_work_sync(&info->deferred_work);
-
- /* clear out the mapping that we setup */
for (i = 0 ; i < info->fix.smem_len; i += PAGE_SIZE) {
page = fb_deferred_io_page(info, i);
page->mapping = NULL;
}
+}
+
+void fb_deferred_io_release(struct fb_info *info)
+{
+ flush_delayed_work(&info->deferred_work);
+ fb_deferred_io_clear_mapping(info);
+}
+
+void fb_deferred_io_cleanup(struct fb_info *info)
+{
+ struct fb_deferred_io *fbdefio = info->fbdefio;
+
+ BUG_ON(!fbdefio);
+ cancel_delayed_work_sync(&info->deferred_work);
+ fb_deferred_io_clear_mapping(info);
kvfree(info->pagerefs);
mutex_destroy(&fbdefio->lock);
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -1454,6 +1454,10 @@ __releases(&info->lock)
struct fb_info * const info = file->private_data;
lock_fb_info(info);
+#if IS_ENABLED(CONFIG_FB_DEFERRED_IO)
+ if (info->fbdefio)
+ fb_deferred_io_release(info);
+#endif
if (info->fbops->fb_release)
info->fbops->fb_release(info,1);
module_put(info->fbops->owner);
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -662,6 +662,7 @@ extern int fb_deferred_io_init(struct fb_info *info);
extern void fb_deferred_io_open(struct fb_info *info,
struct inode *inode,
struct file *file);
+extern void fb_deferred_io_release(struct fb_info *info);
extern void fb_deferred_io_cleanup(struct fb_info *info);
extern int fb_deferred_io_fsync(struct file *file, loff_t start,
loff_t end, int datasync);
--
2.35.3
>
> > > Reviewed-by: Patrik Jakobsson <[email protected]>
> > > Cc: <[email protected]>
> > > Signed-off-by: Takashi Iwai <[email protected]>
> >
> > This could use a Fixes tag. It's not exactly clear to me when this
> > problem got originally introduced, but the recent refactoring seems a
> > candidate.
> >
> > Fixes: 56c134f7f1b5 ("fbdev: Track deferred-I/O pages in pageref struct")
>
> Hrm, this might be. Maybe Patrik can test with the revert of this?
>
> > Cc: Thomas Zimmermann <[email protected]>
> > Cc: Javier Martinez Canillas <[email protected]>
> > Cc: Maarten Lankhorst <[email protected]>
> > Cc: Maxime Ripard <[email protected]>
> > Cc: Zack Rusin <[email protected]>
> > Cc: VMware Graphics Reviewers <[email protected]>
> > Cc: Jaya Kumar <[email protected]>
> > Cc: Daniel Vetter <[email protected]>
> > Cc: "K. Y. Srinivasan" <[email protected]>
> > Cc: Haiyang Zhang <[email protected]>
> > Cc: Wei Liu <[email protected]>
> > Cc: Dexuan Cui <[email protected]>
> > Cc: Steve Glendinning <[email protected]>
> > Cc: Bernie Thompson <[email protected]>
> > Cc: Helge Deller <[email protected]>
> > Cc: Andy Shevchenko <[email protected]>
> > Cc: Greg Kroah-Hartman <[email protected]>
> > Cc: Stephen Kitt <[email protected]>
> > Cc: Peter Suti <[email protected]>
> > Cc: Sam Ravnborg <[email protected]>
> > Cc: Geert Uytterhoeven <[email protected]>
> > Cc: ye xingchen <[email protected]>
> > Cc: Petr Mladek <[email protected]>
> > Cc: John Ogness <[email protected]>
> > Cc: Tom Rix <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: <[email protected]> # v5.19+
>
> Nah, please don't. Too many Cc's, literally a spam.
>
> > > ---
> > > v1->v2: Fix build error without CONFIG_FB_DEFERRED_IO
> > >
> > > drivers/video/fbdev/core/fb_defio.c | 10 +++++++++-
> > > drivers/video/fbdev/core/fbmem.c | 4 ++++
> > > include/linux/fb.h | 1 +
> > > 3 files changed, 14 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/video/fbdev/core/fb_defio.c
> > > b/drivers/video/fbdev/core/fb_defio.c
> > > index c730253ab85c..583cbcf09446 100644
> > > --- a/drivers/video/fbdev/core/fb_defio.c
> > > +++ b/drivers/video/fbdev/core/fb_defio.c
> > > @@ -313,7 +313,7 @@ void fb_deferred_io_open(struct fb_info *info,
> > > }
> > > EXPORT_SYMBOL_GPL(fb_deferred_io_open);
> > > -void fb_deferred_io_cleanup(struct fb_info *info)
> > > +void fb_deferred_io_release(struct fb_info *info)
> > > {
> > > struct fb_deferred_io *fbdefio = info->fbdefio;
> > > struct page *page;
> > > @@ -327,6 +327,14 @@ void fb_deferred_io_cleanup(struct fb_info *info)
> > > page = fb_deferred_io_page(info, i);
> > > page->mapping = NULL;
> > > }
> > > +}
> > > +EXPORT_SYMBOL_GPL(fb_deferred_io_release);
> >
> > It's all in the same module. No need to export this symbol.
>
> I noticed it, too, but just keep the same style as other functions :)
> That said, the other exported symbols are also useless. I can prepare
> another patch to clean it up.
>
>
> thanks,
>
> Takashi
