msync on fbdev

msync on fbdev

[Martin Jackson]

Dear fbdev developers,

This relates to the core implementation of fbdev with the
CONFIG_FB_DEFERRED_IO option enabled.

I believe I have found a bug when doing msync(2) on a framebuffer
device. We are using an old (2.6.37) kernel on an embedded plaform,
but if I look at the current torvalds kernel (fbdev kernel looks
rather old!?), the bug seems to still be there.

When looking at the msync(2) man page, msync is meant to return 0 on
success and -1 on failure, however we are seeing it return 1, which is
undocumented and in my opinion wrong.

This is because the fb_deferred_io_fsync method returns the code from
'schedule_delayed_work', which is either 0 or 1 depending on whether
the work was already scheduled, leading to the possibility that msync
returns the value 1 to userland.

Best regards,

Martin Jackson

Re: msync on fbdev

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2389 bytes --]

On 18/12/14 15:55, Martin Jackson wrote:
> Dear fbdev developers,
> 
> This relates to the core implementation of fbdev with the
> CONFIG_FB_DEFERRED_IO option enabled.
> 
> I believe I have found a bug when doing msync(2) on a framebuffer
> device. We are using an old (2.6.37) kernel on an embedded plaform,
> but if I look at the current torvalds kernel (fbdev kernel looks
> rather old!?), the bug seems to still be there.
> 
> When looking at the msync(2) man page, msync is meant to return 0 on
> success and -1 on failure, however we are seeing it return 1, which is
> undocumented and in my opinion wrong.
> 
> This is because the fb_deferred_io_fsync method returns the code from
> 'schedule_delayed_work', which is either 0 or 1 depending on whether
> the work was already scheduled, leading to the possibility that msync
> returns the value 1 to userland.

Indeed, looks broken. schedule_delayed_work() never returns an error, it
just returns a bool telling if the work was already scheduled.

I guess no one really uses deferred io much =).

Here's a patch:

From 9cc80b3cf9d6023ede084d024ed8b5c0cb247bc2 Mon Sep 17 00:00:00 2001
From: Tomi Valkeinen <tomi.valkeinen@ti.com>
Date: Fri, 19 Dec 2014 13:55:41 +0200
Subject: [PATCH] video/fbdev: fix defio's fsync

fb_deferred_io_fsync() returns the value of schedule_delayed_work() as
an error code, but schedule_delayed_work() does not return an error. It
returns true/false depending on whether the work was already queued.

Fix this by ignoring the return value of schedule_delayed_work().

Signed-off-by: Tomi Valkeinen <tomi.valkeinen@ti.com>
---
 drivers/video/fbdev/core/fb_defio.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/core/fb_defio.c
b/drivers/video/fbdev/core/fb_defio.c
index 900aa4ecd617..d6cab1fd9a47 100644
--- a/drivers/video/fbdev/core/fb_defio.c
+++ b/drivers/video/fbdev/core/fb_defio.c
@@ -83,9 +83,10 @@ int fb_deferred_io_fsync(struct file *file, loff_t
start, loff_t end, int datasy
 	cancel_delayed_work_sync(&info->deferred_work);

 	/* Run it immediately */
-	err = schedule_delayed_work(&info->deferred_work, 0);
+	schedule_delayed_work(&info->deferred_work, 0);
 	mutex_unlock(&inode->i_mutex);
-	return err;
+
+	return 0;
 }
 EXPORT_SYMBOL_GPL(fb_deferred_io_fsync);

-- 
2.2.0



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]