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

[Thomas Zimmermann <tzimmermann@xxxxxxx>]

Hi

Am 26.04.23 um 17:01 schrieb Geert Uytterhoeven:
> Hi Thomas,
>
> On Tue, Apr 25, 2023 at 4:28 PM Thomas Zimmermann <tzimmermann@xxxxxxx> 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@xxxxxxx>
> > ---
> >   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.

Shall I call it fb_raw_() or fb_io_()?

CFB is used by the drawing helpers, which are usually used together with 
this code. Hence the current naming.

Best regards
Thomas


> > --- /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

--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

Attachment: OpenPGP_signature
Description: OpenPGP digital signature