[PATCH] RFC: framebuffer: provide generic get_fb_unmapped_area

This patch makes mmapping the simple-framebuffer device work on a no-MMU
ARM target. The code is mostly taken from
arch/blackfin/kernel/sys_bfin.c.

Note this is only tested on this no-MMU machine and I don't know enough
about framebuffers and mm to decide if this patch is sane. Also I'm
unsure about the size check because it triggers if userspace page aligns
the len parameter. (I don't know how usual it is to do, I'd say it's
wrong, but my test program (fbtest by Geert Uytterhoeven) does it.)

Signed-off-by: Uwe Kleine-König <redacted>
---
 drivers/video/fbmem.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index dacaf74..70b328c 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c

@@ -1483,6 +1483,24 @@ __releases(&info->lock)
 	return 0;
 }
 
+#ifdef HAVE_ARCH_FB_UNMAPPED_AREA
+#define fb_get_unmapped_area get_fb_unmapped_area
+#else
+unsigned long fb_get_unmapped_area(struct file *filp, unsigned long orig_addr,
+		unsigned long len, unsigned long pgoff, unsigned long flags)
+{
+	struct fb_info * const info = filp->private_data;
+	unsigned long screen_size = info->screen_size ?: info->fix.smem_len;
+
+	if (len > screen_size) {
+		pr_info("%lu > %lu (%lu, %lu)\n", len, screen_size, info->screen_size, info->fix.smem_len);
+		return -EINVAL;
+	}
+
+	return (unsigned long)info->screen_base;
+}
+#endif
+
 static const struct file_operations fb_fops = {
 	.owner =	THIS_MODULE,
 	.read =		fb_read,

@@ -1494,9 +1512,7 @@ static const struct file_operations fb_fops = {
 	.mmap =		fb_mmap,
 	.open =		fb_open,
 	.release =	fb_release,
-#ifdef HAVE_ARCH_FB_UNMAPPED_AREA
-	.get_unmapped_area = get_fb_unmapped_area,
-#endif
+	.get_unmapped_area = fb_get_unmapped_area,
 #ifdef CONFIG_FB_DEFERRED_IO
 	.fsync =	fb_deferred_io_fsync,
 #endif

-- 
1.8.4.2

On Mon, Nov 18, 2013 at 11:57 AM, Uwe Kleine-König
[off-list ref] wrote:

This patch makes mmapping the simple-framebuffer device work on a no-MMU
ARM target. The code is mostly taken from
arch/blackfin/kernel/sys_bfin.c.

Note this is only tested on this no-MMU machine and I don't know enough
about framebuffers and mm to decide if this patch is sane. Also I'm
unsure about the size check because it triggers if userspace page aligns
the len parameter. (I don't know how usual it is to do, I'd say it's
wrong, but my test program (fbtest by Geert Uytterhoeven) does it.)

It's quite common: the granularity of mmap() is PAGE_SIZE, i.e. if you
try to map a partial page, you'll get access to the full page anyway
(with MMU; without MMU, you can access everything anyway).
Fbtest always mmap()s the full (page aligned) smem_len.

Why restrict this to screen_size? Fbtest will map the whole frame buffer memory.

Typically screen_size is not a multiple of PAGE_SIZE, so this is another
reason why your size check fails.

Shouldn't you take into account pgoff?

+}
+#endif

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Hello Geert,

On Mon, Nov 18, 2013 at 12:59:40PM +0100, Geert Uytterhoeven wrote:

On Mon, Nov 18, 2013 at 11:57 AM, Uwe Kleine-König
[off-list ref] wrote:

quoted

This patch makes mmapping the simple-framebuffer device work on a no-MMU
ARM target. The code is mostly taken from
arch/blackfin/kernel/sys_bfin.c.

Note this is only tested on this no-MMU machine and I don't know enough
about framebuffers and mm to decide if this patch is sane. Also I'm
unsure about the size check because it triggers if userspace page aligns
the len parameter. (I don't know how usual it is to do, I'd say it's
wrong, but my test program (fbtest by Geert Uytterhoeven) does it.)

It's quite common: the granularity of mmap() is PAGE_SIZE, i.e. if you
try to map a partial page, you'll get access to the full page anyway
(with MMU; without MMU, you can access everything anyway).
Fbtest always mmap()s the full (page aligned) smem_len.

quoted

Signed-off-by: Uwe Kleine-König <redacted>
---
 drivers/video/fbmem.c | 22 +++++++++++++++++++---
 1 file changed, 19 insertions(+), 3 deletions(-)

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index dacaf74..70b328c 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c

@@ -1483,6 +1483,24 @@ __releases(&info->lock)
        return 0;
 }

+#ifdef HAVE_ARCH_FB_UNMAPPED_AREA
+#define fb_get_unmapped_area get_fb_unmapped_area
+#else
+unsigned long fb_get_unmapped_area(struct file *filp, unsigned long orig_addr,
+               unsigned long len, unsigned long pgoff, unsigned long flags)
+{
+       struct fb_info * const info = filp->private_data;
+       unsigned long screen_size = info->screen_size ?: info->fix.smem_len;

Why restrict this to screen_size? Fbtest will map the whole frame buffer memory.

For me screen_size is zero. The logic to determine the size is copied
from fb_read. In the meantine I'm using

	if (len > PAGE_ALIGN(screen_size))

because even if userspace passes an unaligned size it gets aligned
somewhere on the path to fb_get_unmapped_area.

Typically screen_size is not a multiple of PAGE_SIZE, so this is another
reason why your size check fails.

quoted

+       if (len > screen_size) {
+               pr_info("%lu > %lu (%lu, %lu)\n", len, screen_size, info->screen_size, info->fix.smem_len);
+               return -EINVAL;
+       }
+
+       return (unsigned long)info->screen_base;

Shouldn't you take into account pgoff?

Sounds sensible. Then the same applies to blackfin's
get_fb_unmapped_area.

So is it:

	unsigned long screen_size = info->screen_size ?: info->fix.smem_len;

	screen_size = PAGE_ALIGN(screen_size);

	if (pgoff > screen_size || pgoff + len > screen_size)
		return -EINVAL;

	return (unsigned long)info->screen_base + pgoff;

? Or should I drop the size check?

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Hello again,

On Mon, Nov 18, 2013 at 07:59:59PM +0100, Uwe Kleine-König wrote:

	if (pgoff > screen_size || pgoff + len > screen_size)

This must be:

	if (pgoff > screen_size || len > screen_size - pgoff)

to do what I intended.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

On Mon, Nov 18, 2013 at 7:59 PM, Uwe Kleine-König
[off-list ref] wrote:

quoted

quoted

+unsigned long fb_get_unmapped_area(struct file *filp, unsigned long orig_addr,
+               unsigned long len, unsigned long pgoff, unsigned long flags)
+{
+       struct fb_info * const info = filp->private_data;
+       unsigned long screen_size = info->screen_size ?: info->fix.smem_len;

Why restrict this to screen_size? Fbtest will map the whole frame buffer memory.

For me screen_size is zero. The logic to determine the size is copied
from fb_read.

fb_read() only allows reading the visible screen, not the full frame
buffer memory.
fb_mmap() does allow mapping the full frame buffer memory (and the optional
MMIO registers, but you can't easily do that the same way on nommu, as it's
a discontiguous mapping).
So please use PAGE_ALIGN(info->fix.smem_len) as the limit.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds