Hi,
While working with a framebuffer displays, I noticed that the displays
sporadically do not show the image as I expect.
After investigation: this is because my devices use deferred IO, and by
closing the framebuffers, all pending deferred IO get cancelled. This
causes the image I sent to the devices to just vanish. Using fsync() does
not always help, because the driver's implementation of fsync() does not
guarantee that all pending work is flushed on return.
This series solves the problem by flush the workqueue in .release(). Also
flush the workqueue in .fsync(), as it is supposed to do.
Nam Cao (2):
fb: flush deferred work in fb_deferred_io_fsync()
fb: flush deferred IO before closing
drivers/video/fbdev/core/fb_defio.c | 8 ++------
1 file changed, 2 insertions(+), 6 deletions(-)
--
2.39.2
The driver's fsync() is supposed to flush any pending operation to
hardware. It is implemented in this driver by cancelling the queued
deferred IO first, then schedule it for "immediate execution" by calling
schedule_delayed_work() again with delay=0. However, setting delay=0
only means the work is scheduled immediately, it does not mean the work
is executed immediately. There is no guarantee that the work is finished
after schedule_delayed_work() returns. After this driver's fsync()
returns, there can still be pending work. Furthermore, if close() is
called by users immediately after fsync(), the pending work gets
cancelled and fsync() may do nothing.
To ensure that the deferred IO completes, use flush_delayed_work()
instead. Write operations to this driver either write to the device
directly, or invoke schedule_delayed_work(); so by flushing the
workqueue, it can be guaranteed that all previous writes make it to the
device.
Fixes: 5e841b88d23d ("fb: fsync() method for deferred I/O flush.")
Cc: [email protected]
Signed-off-by: Nam Cao <[email protected]>
Reviewed-by: Sebastian Andrzej Siewior <[email protected]>
---
drivers/video/fbdev/core/fb_defio.c | 6 +-----
1 file changed, 1 insertion(+), 5 deletions(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index 274f5d0fa247..6c8b81c452f0 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -132,11 +132,7 @@ int fb_deferred_io_fsync(struct file *file, loff_t start, loff_t end, int datasy
return 0;
inode_lock(inode);
- /* Kill off the delayed work */
- cancel_delayed_work_sync(&info->deferred_work);
-
- /* Run it immediately */
- schedule_delayed_work(&info->deferred_work, 0);
+ flush_delayed_work(&info->deferred_work);
inode_unlock(inode);
return 0;
--
2.39.2
When framebuffer gets closed, the queued deferred IO gets cancelled. This
can cause some last display data to vanish. This is problematic for users
who send a still image to the framebuffer, then close the file: the image
may never appear.
To ensure none of display data get lost, flush the queued deferred IO
first before closing.
Another possible solution is to delete the cancel_delayed_work_sync()
instead. The difference is that the display may appear some time after
closing. However, the clearing of page mapping after this needs to be
removed too, because the page mapping is used by the deferred work. It is
not completely obvious whether it is okay to not clear the page mapping.
For a patch intended for stable trees, go with the simple and obvious
solution.
Fixes: 60b59beafba8 ("fbdev: mm: Deferred IO support")
Cc: [email protected]
Signed-off-by: Nam Cao <[email protected]>
Reviewed-by: Sebastian Andrzej Siewior <[email protected]>
---
drivers/video/fbdev/core/fb_defio.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/fbdev/core/fb_defio.c b/drivers/video/fbdev/core/fb_defio.c
index 6c8b81c452f0..1ae1d35a5942 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -313,7 +313,7 @@ static void fb_deferred_io_lastclose(struct fb_info *info)
struct page *page;
int i;
- cancel_delayed_work_sync(&info->deferred_work);
+ flush_delayed_work(&info->deferred_work);
/* clear out the mapping that we setup */
for (i = 0 ; i < info->fix.smem_len; i += PAGE_SIZE) {
--
2.39.2
On 12/18/23 10:57, Nam Cao wrote:
> The driver's fsync() is supposed to flush any pending operation to
> hardware. It is implemented in this driver by cancelling the queued
> deferred IO first, then schedule it for "immediate execution" by calling
> schedule_delayed_work() again with delay=0. However, setting delay=0
> only means the work is scheduled immediately, it does not mean the work
> is executed immediately. There is no guarantee that the work is finished
> after schedule_delayed_work() returns. After this driver's fsync()
> returns, there can still be pending work. Furthermore, if close() is
> called by users immediately after fsync(), the pending work gets
> cancelled and fsync() may do nothing.
>
> To ensure that the deferred IO completes, use flush_delayed_work()
> instead. Write operations to this driver either write to the device
> directly, or invoke schedule_delayed_work(); so by flushing the
> workqueue, it can be guaranteed that all previous writes make it to the
> device.
>
> Fixes: 5e841b88d23d ("fb: fsync() method for deferred I/O flush.")
> Cc: [email protected]
> Signed-off-by: Nam Cao <[email protected]>
> Reviewed-by: Sebastian Andrzej Siewior <[email protected]>
> ---
> drivers/video/fbdev/core/fb_defio.c | 6 +-----
> 1 file changed, 1 insertion(+), 5 deletions(-)
both patches applied to fbdev for-next git tree.
Thanks!
Helge