Re: [PATCH 5/6] fbdev: Move CFB read and write code into helper functions

[Geert Uytterhoeven <geert@linux-m68k.org>]

Hi Thomas,

On Tue, Apr 25, 2023 at 4:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Move the existing CFB read and write code for I/O memory into
> the new helpers fb_cfb_read() and fb_cfb_write(). Make them the
> default fp_ops. No functional changes.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/video/fbdev/core/Makefile      |   2 +-
>  drivers/video/fbdev/core/fb_cfb_fops.c | 126 +++++++++++++++++++++++++
>  drivers/video/fbdev/core/fbmem.c       | 113 +---------------------
>  include/linux/fb.h                     |  10 ++
>  4 files changed, 139 insertions(+), 112 deletions(-)
>  create mode 100644 drivers/video/fbdev/core/fb_cfb_fops.c

While the general idea is fine, please do not call any of this "cfb",
as it is not related to chunky color frame buffer formats.
All of these operate on the raw frame buffer contents.

> --- /dev/null
> +++ b/drivers/video/fbdev/core/fb_cfb_fops.c
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/fb.h>
> +#include <linux/module.h>
> +#include <linux/uaccess.h>
> +
> +ssize_t fb_cfb_read(struct fb_info *info, char __user *buf, size_t count, loff_t *ppos)
> +{

[...]

> +       while (count) {
> +               c  = (count > PAGE_SIZE) ? PAGE_SIZE : count;
> +               dst = buffer;
> +               fb_memcpy_fromfb(dst, src, c);
> +               dst += c;
> +               src += c;
> +
> +               if (copy_to_user(buf, buffer, c)) {

So here's still the buggy copy_to_user() handling, copied from fb_read().

> +                       err = -EFAULT;
> +                       break;
> +               }
> +               *ppos += c;
> +               buf += c;
> +               cnt += c;
> +               count -= c;
> +       }
> +
> +       kfree(buffer);
> +
> +       return cnt ? cnt : err;
> +}
> +EXPORT_SYMBOL(fb_cfb_read);
> +
> +ssize_t fb_cfb_write(struct fb_info *info, const char __user *buf, size_t count, loff_t *ppos)
> +{

[...]

> +       while (count) {
> +               c = (count > PAGE_SIZE) ? PAGE_SIZE : count;
> +               src = buffer;
> +
> +               if (copy_from_user(src, buf, c)) {

And copy_from_user(), too...

> +                       err = -EFAULT;
> +                       break;
> +               }
> +
> +               fb_memcpy_tofb(dst, src, c);
> +               dst += c;
> +               src += c;
> +               *ppos += c;
> +               buf += c;
> +               cnt += c;
> +               count -= c;
> +       }
> +
> +       kfree(buffer);
> +
> +       return (cnt) ? cnt : err;
> +}

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