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

[Thomas Zimmermann <tzimmermann@suse.de>]

[-- Attachment #1.1: Type: text/plain, Size: 3351 bytes --]

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@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.

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]