[PATCH 0/6] drm,fbdev: Use fbdev's I/O helpers

[PATCH 0/6] drm,fbdev: Use fbdev's I/O helpers

[Thomas Zimmermann]

Make fbdev's built-in helpers for reading and writing I/O and system
memory available to DRM. Replace DRM's internal helpers.

The first patch resolves a bug that's been in the fbdev code for
more than 15 years. Makes the read/write helpers work successfully
with the IGT tests.

Patches 2 to 4 streamline fbdev's file-I/O code and remove a few
duplicate checks.

Patch 5 moves the default-I/O code into the new helpers fb_cfb_read()
and fb_cfb_write(); patch 6 uses them in DRM. This allows us to remove
quite a bit of code from DRM's internal fbdev helpers.

Tested with i915 and simpledrm.

The next step here is to remove the drm_fb_helper_{cfb,sys}_*()
entirely. They where mostly introduced because fbdev doesn't protect
it's public interfaces with an CONFIG_FB preprocessor guards. But all
of DRM driver's fbdev emulation won't be build without CONFIG_FB, so
this is not an issue in practice. Removing the DRM wrappers will
further simplify the DRM code.

Thomas Zimmermann (6):
  fbdev: Return number of bytes read or written
  fbdev: Use screen_buffer in fb_sys_{read,write}()
  fbdev: Don't re-validate info->state in fb_ops implementations
  fbdev: Validate info->screen_{base,buffer} in fb_ops implementations
  fbdev: Move CFB read and write code into helper functions
  drm/fb-helper: Use fb_{cfb,sys}_{read, write}()

 drivers/gpu/drm/drm_fb_helper.c        | 174 +------------------------
 drivers/video/fbdev/cobalt_lcdfb.c     |   6 +
 drivers/video/fbdev/core/Makefile      |   2 +-
 drivers/video/fbdev/core/fb_cfb_fops.c | 126 ++++++++++++++++++
 drivers/video/fbdev/core/fb_sys_fops.c |  36 ++---
 drivers/video/fbdev/core/fbmem.c       | 111 +---------------
 drivers/video/fbdev/sm712fb.c          |  10 +-
 include/linux/fb.h                     |  10 ++
 8 files changed, 173 insertions(+), 302 deletions(-)
 create mode 100644 drivers/video/fbdev/core/fb_cfb_fops.c

-- 
2.40.0

[PATCH 1/6] fbdev: Return number of bytes read or written

[Thomas Zimmermann]

Always return the number of bytes read or written within the
framebuffer. Only return an errno code if framebuffer memory
was not touched. This is the semantics required by POSIX and
makes fb_read() and fb_write() compatible with IGT tests. [1]

This bug has been fixed for fb_write() long ago by
commit 6a2a88668e90 ("[PATCH] fbdev: Fix return error of
fb_write"). The code in fb_read() and the corresponding fb_sys_()
helpers was forgotten.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Link: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/fbdev.c # 1
---
 drivers/video/fbdev/core/fb_sys_fops.c | 24 ++++++++++++++----------
 drivers/video/fbdev/core/fbmem.c       |  2 +-
 2 files changed, 15 insertions(+), 11 deletions(-)

diff --git a/drivers/video/fbdev/core/fb_sys_fops.c b/drivers/video/fbdev/core/fb_sys_fops.c
index ff275d7f3eaf..cefb77b9546d 100644
--- a/drivers/video/fbdev/core/fb_sys_fops.c
+++ b/drivers/video/fbdev/core/fb_sys_fops.c
@@ -19,7 +19,8 @@ ssize_t fb_sys_read(struct fb_info *info, char __user *buf, size_t count,
 	unsigned long p = *ppos;
 	void *src;
 	int err = 0;
-	unsigned long total_size;
+	unsigned long total_size, c;
+	ssize_t ret;
 
 	if (info->state != FBINFO_STATE_RUNNING)
 		return -EPERM;
@@ -43,13 +44,14 @@ ssize_t fb_sys_read(struct fb_info *info, char __user *buf, size_t count,
 	if (info->fbops->fb_sync)
 		info->fbops->fb_sync(info);
 
-	if (copy_to_user(buf, src, count))
+	c = copy_to_user(buf, src, count);
+	if (c)
 		err = -EFAULT;
+	ret = count - c;
 
-	if  (!err)
-		*ppos += count;
+	*ppos += ret;
 
-	return (err) ? err : count;
+	return ret ? ret : err;
 }
 EXPORT_SYMBOL_GPL(fb_sys_read);
 
@@ -59,7 +61,8 @@ ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
 	unsigned long p = *ppos;
 	void *dst;
 	int err = 0;
-	unsigned long total_size;
+	unsigned long total_size, c;
+	size_t ret;
 
 	if (info->state != FBINFO_STATE_RUNNING)
 		return -EPERM;
@@ -89,13 +92,14 @@ ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
 	if (info->fbops->fb_sync)
 		info->fbops->fb_sync(info);
 
-	if (copy_from_user(dst, buf, count))
+	c = copy_from_user(dst, buf, count);
+	if (c)
 		err = -EFAULT;
+	ret = count - c;
 
-	if  (!err)
-		*ppos += count;
+	*ppos += ret;
 
-	return (err) ? err : count;
+	return ret ? ret : err;
 }
 EXPORT_SYMBOL_GPL(fb_sys_write);
 
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 3fd95a79e4c3..abc92d79a295 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -820,7 +820,7 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 
 	kfree(buffer);
 
-	return (err) ? err : cnt;
+	return cnt ? cnt : err;
 }
 
 static ssize_t
-- 
2.40.0

Re: [PATCH 1/6] fbdev: Return number of bytes read or written

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

Hello Thomas,

> Always return the number of bytes read or written within the
> framebuffer. Only return an errno code if framebuffer memory
> was not touched. This is the semantics required by POSIX and
> makes fb_read() and fb_write() compatible with IGT tests. [1]
>
> This bug has been fixed for fb_write() long ago by
> commit 6a2a88668e90 ("[PATCH] fbdev: Fix return error of
> fb_write"). The code in fb_read() and the corresponding fb_sys_()
> helpers was forgotten.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Link: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/fbdev.c # 1
> ---

The patch looks good to me and indeed the correct semantics AFAICT.

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [1/6] fbdev: Return number of bytes read or written

[Sui Jingfeng]

Hi,

The whole patch set is tested with fbdev of IGT, on LoongArch with 
drm/radeon and efifb driver. Test results say SUCCESS. Tested-by: Sui 
Jingfeng <suijingfeng@loongson.cn>

On 2023/4/25 22:28, Thomas Zimmermann wrote:
> Always return the number of bytes read or written within the
> framebuffer. Only return an errno code if framebuffer memory
> was not touched. This is the semantics required by POSIX and
> makes fb_read() and fb_write() compatible with IGT tests. [1]
>
> This bug has been fixed for fb_write() long ago by
> commit 6a2a88668e90 ("[PATCH] fbdev: Fix return error of
> fb_write"). The code in fb_read() and the corresponding fb_sys_()
> helpers was forgotten.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Link: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/fbdev.c # 1
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>   drivers/video/fbdev/core/fb_sys_fops.c | 24 ++++++++++++++----------
>   drivers/video/fbdev/core/fbmem.c       |  2 +-
>   2 files changed, 15 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fb_sys_fops.c b/drivers/video/fbdev/core/fb_sys_fops.c
> index ff275d7f3eaf..cefb77b9546d 100644
> --- a/drivers/video/fbdev/core/fb_sys_fops.c
> +++ b/drivers/video/fbdev/core/fb_sys_fops.c
> @@ -19,7 +19,8 @@ ssize_t fb_sys_read(struct fb_info *info, char __user *buf, size_t count,
>   	unsigned long p = *ppos;
>   	void *src;
>   	int err = 0;
> -	unsigned long total_size;
> +	unsigned long total_size, c;
> +	ssize_t ret;
>   
>   	if (info->state != FBINFO_STATE_RUNNING)
>   		return -EPERM;
> @@ -43,13 +44,14 @@ ssize_t fb_sys_read(struct fb_info *info, char __user *buf, size_t count,
>   	if (info->fbops->fb_sync)
>   		info->fbops->fb_sync(info);
>   
> -	if (copy_to_user(buf, src, count))
> +	c = copy_to_user(buf, src, count);
> +	if (c)
>   		err = -EFAULT;
> +	ret = count - c;
>   
> -	if  (!err)
> -		*ppos += count;
> +	*ppos += ret;
>   
> -	return (err) ? err : count;
> +	return ret ? ret : err;
>   }
>   EXPORT_SYMBOL_GPL(fb_sys_read);
>   
> @@ -59,7 +61,8 @@ ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
>   	unsigned long p = *ppos;
>   	void *dst;
>   	int err = 0;
> -	unsigned long total_size;
> +	unsigned long total_size, c;
> +	size_t ret;
>   
>   	if (info->state != FBINFO_STATE_RUNNING)
>   		return -EPERM;
> @@ -89,13 +92,14 @@ ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
>   	if (info->fbops->fb_sync)
>   		info->fbops->fb_sync(info);
>   
> -	if (copy_from_user(dst, buf, count))
> +	c = copy_from_user(dst, buf, count);
> +	if (c)
>   		err = -EFAULT;
> +	ret = count - c;
>   
> -	if  (!err)
> -		*ppos += count;
> +	*ppos += ret;
>   
> -	return (err) ? err : count;
> +	return ret ? ret : err;
>   }
>   EXPORT_SYMBOL_GPL(fb_sys_write);
>   
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 3fd95a79e4c3..abc92d79a295 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -820,7 +820,7 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>   
>   	kfree(buffer);
>   
> -	return (err) ? err : cnt;
> +	return cnt ? cnt : err;
>   }
>   
>   static ssize_t

Re: [PATCH 1/6] fbdev: Return number of bytes read or written

[Geert Uytterhoeven]

Hi Thomas,

On Tue, Apr 25, 2023 at 4:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Always return the number of bytes read or written within the
> framebuffer. Only return an errno code if framebuffer memory
> was not touched. This is the semantics required by POSIX and
> makes fb_read() and fb_write() compatible with IGT tests. [1]
>
> This bug has been fixed for fb_write() long ago by
> commit 6a2a88668e90 ("[PATCH] fbdev: Fix return error of
> fb_write"). The code in fb_read() and the corresponding fb_sys_()
> helpers was forgotten.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Link: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/fbdev.c # 1

Thanks for your patch!

> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -820,7 +820,7 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>
>         kfree(buffer);
>
> -       return (err) ? err : cnt;
> +       return cnt ? cnt : err;
>  }

Looks all good to me, so
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

However, shouldn't the copy_to_user() handling in fb_read() be fixed,
too?

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

Re: [PATCH 1/6] fbdev: Return number of bytes read or written

[Thomas Zimmermann]

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

Hi

Am 26.04.23 um 16:41 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> On Tue, Apr 25, 2023 at 4:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Always return the number of bytes read or written within the
>> framebuffer. Only return an errno code if framebuffer memory
>> was not touched. This is the semantics required by POSIX and
>> makes fb_read() and fb_write() compatible with IGT tests. [1]
>>
>> This bug has been fixed for fb_write() long ago by
>> commit 6a2a88668e90 ("[PATCH] fbdev: Fix return error of
>> fb_write"). The code in fb_read() and the corresponding fb_sys_()
>> helpers was forgotten.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Link: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/fbdev.c # 1
> 
> Thanks for your patch!
> 
>> --- a/drivers/video/fbdev/core/fbmem.c
>> +++ b/drivers/video/fbdev/core/fbmem.c
>> @@ -820,7 +820,7 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>>
>>          kfree(buffer);
>>
>> -       return (err) ? err : cnt;
>> +       return cnt ? cnt : err;
>>   }
> 
> Looks all good to me, so
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> However, shouldn't the copy_to_user() handling in fb_read() be fixed,
> too?

That's a good point. It doesn't necessarily copy all given bytes and can 
thus return the wrong result. The IGT tests passed anyway, but I'll fix 
it in v2.

Best regards
Thomas

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

[PATCH 2/6] fbdev: Use screen_buffer in fb_sys_{read,write}()

[Thomas Zimmermann]

Use info->screen_buffer when reading and writing framebuffers in
system memory. It's the correct pointer for this address space.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/core/fb_sys_fops.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/video/fbdev/core/fb_sys_fops.c b/drivers/video/fbdev/core/fb_sys_fops.c
index cefb77b9546d..6589123f4127 100644
--- a/drivers/video/fbdev/core/fb_sys_fops.c
+++ b/drivers/video/fbdev/core/fb_sys_fops.c
@@ -39,7 +39,7 @@ ssize_t fb_sys_read(struct fb_info *info, char __user *buf, size_t count,
 	if (count + p > total_size)
 		count = total_size - p;
 
-	src = (void __force *)(info->screen_base + p);
+	src = info->screen_buffer + p;
 
 	if (info->fbops->fb_sync)
 		info->fbops->fb_sync(info);
@@ -87,7 +87,7 @@ ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
 		count = total_size - p;
 	}
 
-	dst = (void __force *) (info->screen_base + p);
+	dst = info->screen_buffer + p;
 
 	if (info->fbops->fb_sync)
 		info->fbops->fb_sync(info);
-- 
2.40.0

Re: [PATCH 2/6] fbdev: Use screen_buffer in fb_sys_{read,write}()

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Use info->screen_buffer when reading and writing framebuffers in
> system memory. It's the correct pointer for this address space.
>

Maybe can expand the explanation a little bit with something like this?

"The struct fb_info has a union to store the framebuffer memory. This can
either be info->screen_base if the framebuffer is stored in I/O memory,
or info->screen_buffer if the framebuffer is stored in system memory.

Since the fb_sys_{read,write}() functions operate on the latter address
space, it is wrong to use .screen_base and .screen_buffer must be used
instead. This also get rids of all the casting needed due not using the
correct data type."

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH 2/6] fbdev: Use screen_buffer in fb_sys_{read,write}()

[Thomas Zimmermann]

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

Hi

Am 25.04.23 um 18:35 schrieb Javier Martinez Canillas:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
> 
>> Use info->screen_buffer when reading and writing framebuffers in
>> system memory. It's the correct pointer for this address space.
>>
> 
> Maybe can expand the explanation a little bit with something like this?
> 
> "The struct fb_info has a union to store the framebuffer memory. This can
> either be info->screen_base if the framebuffer is stored in I/O memory,
> or info->screen_buffer if the framebuffer is stored in system memory.
> 
> Since the fb_sys_{read,write}() functions operate on the latter address
> space, it is wrong to use .screen_base and .screen_buffer must be used
> instead. This also get rids of all the casting needed due not using the
> correct data type."

Thanks. I'll add this text as-is in the next iteration.

Best regards
Thomas

> 
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> ---
> 
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> 

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

Re: [PATCH 2/6] fbdev: Use screen_buffer in fb_sys_{read,write}()

[Geert Uytterhoeven]

On Tue, Apr 25, 2023 at 6:36 PM Javier Martinez Canillas
<javierm@redhat.com> wrote:
> Thomas Zimmermann <tzimmermann@suse.de> writes:
>
> > Use info->screen_buffer when reading and writing framebuffers in
> > system memory. It's the correct pointer for this address space.
> >
>
> Maybe can expand the explanation a little bit with something like this?
>
> "The struct fb_info has a union to store the framebuffer memory. This can
> either be info->screen_base if the framebuffer is stored in I/O memory,
> or info->screen_buffer if the framebuffer is stored in system memory.
>
> Since the fb_sys_{read,write}() functions operate on the latter address
> space, it is wrong to use .screen_base and .screen_buffer must be used
> instead. This also get rids of all the casting needed due not using the

... due to not ...

> correct data type."

+1

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

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

Re: [2/6] fbdev: Use screen_buffer in fb_sys_{read,write}()

[Sui Jingfeng]

Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>

On 2023/4/25 22:28, Thomas Zimmermann wrote:
> Use info->screen_buffer when reading and writing framebuffers in
> system memory. It's the correct pointer for this address space.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>   drivers/video/fbdev/core/fb_sys_fops.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fb_sys_fops.c b/drivers/video/fbdev/core/fb_sys_fops.c
> index cefb77b9546d..6589123f4127 100644
> --- a/drivers/video/fbdev/core/fb_sys_fops.c
> +++ b/drivers/video/fbdev/core/fb_sys_fops.c
> @@ -39,7 +39,7 @@ ssize_t fb_sys_read(struct fb_info *info, char __user *buf, size_t count,
>   	if (count + p > total_size)
>   		count = total_size - p;
>   
> -	src = (void __force *)(info->screen_base + p);
> +	src = info->screen_buffer + p;
>   
>   	if (info->fbops->fb_sync)
>   		info->fbops->fb_sync(info);
> @@ -87,7 +87,7 @@ ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
>   		count = total_size - p;
>   	}
>   
> -	dst = (void __force *) (info->screen_base + p);
> +	dst = info->screen_buffer + p;
>   
>   	if (info->fbops->fb_sync)
>   		info->fbops->fb_sync(info);

[PATCH 3/6] fbdev: Don't re-validate info->state in fb_ops implementations

[Thomas Zimmermann]

The file-op entry points fb_read() and fb_write() verify that
info->state has been set to FBINFO_STATE_RUNNING. Remove the same
test from the implementations of struct fb_ops.{fb_read,fb_write}.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/core/fb_sys_fops.c | 6 ------
 drivers/video/fbdev/sm712fb.c          | 6 ------
 2 files changed, 12 deletions(-)

diff --git a/drivers/video/fbdev/core/fb_sys_fops.c b/drivers/video/fbdev/core/fb_sys_fops.c
index 6589123f4127..7dee5d3c7fb1 100644
--- a/drivers/video/fbdev/core/fb_sys_fops.c
+++ b/drivers/video/fbdev/core/fb_sys_fops.c
@@ -22,9 +22,6 @@ ssize_t fb_sys_read(struct fb_info *info, char __user *buf, size_t count,
 	unsigned long total_size, c;
 	ssize_t ret;
 
-	if (info->state != FBINFO_STATE_RUNNING)
-		return -EPERM;
-
 	total_size = info->screen_size;
 
 	if (total_size == 0)
@@ -64,9 +61,6 @@ ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
 	unsigned long total_size, c;
 	size_t ret;
 
-	if (info->state != FBINFO_STATE_RUNNING)
-		return -EPERM;
-
 	total_size = info->screen_size;
 
 	if (total_size == 0)
diff --git a/drivers/video/fbdev/sm712fb.c b/drivers/video/fbdev/sm712fb.c
index b528776c7612..6f852cd756c5 100644
--- a/drivers/video/fbdev/sm712fb.c
+++ b/drivers/video/fbdev/sm712fb.c
@@ -1031,9 +1031,6 @@ static ssize_t smtcfb_read(struct fb_info *info, char __user *buf,
 	if (!info || !info->screen_base)
 		return -ENODEV;
 
-	if (info->state != FBINFO_STATE_RUNNING)
-		return -EPERM;
-
 	total_size = info->screen_size;
 
 	if (total_size == 0)
@@ -1097,9 +1094,6 @@ static ssize_t smtcfb_write(struct fb_info *info, const char __user *buf,
 	if (!info || !info->screen_base)
 		return -ENODEV;
 
-	if (info->state != FBINFO_STATE_RUNNING)
-		return -EPERM;
-
 	total_size = info->screen_size;
 
 	if (total_size == 0)
-- 
2.40.0

Re: [PATCH 3/6] fbdev: Don't re-validate info->state in fb_ops implementations

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> The file-op entry points fb_read() and fb_write() verify that
> info->state has been set to FBINFO_STATE_RUNNING. Remove the same
> test from the implementations of struct fb_ops.{fb_read,fb_write}.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [3/6] fbdev: Don't re-validate info->state in fb_ops implementations

[Sui Jingfeng]

Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>


On 2023/4/25 22:28, Thomas Zimmermann wrote:
> The file-op entry points fb_read() and fb_write() verify that
> info->state has been set to FBINFO_STATE_RUNNING. Remove the same
> test from the implementations of struct fb_ops.{fb_read,fb_write}.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>   drivers/video/fbdev/core/fb_sys_fops.c | 6 ------
>   drivers/video/fbdev/sm712fb.c          | 6 ------
>   2 files changed, 12 deletions(-)
>
> diff --git a/drivers/video/fbdev/core/fb_sys_fops.c b/drivers/video/fbdev/core/fb_sys_fops.c
> index 6589123f4127..7dee5d3c7fb1 100644
> --- a/drivers/video/fbdev/core/fb_sys_fops.c
> +++ b/drivers/video/fbdev/core/fb_sys_fops.c
> @@ -22,9 +22,6 @@ ssize_t fb_sys_read(struct fb_info *info, char __user *buf, size_t count,
>   	unsigned long total_size, c;
>   	ssize_t ret;
>   
> -	if (info->state != FBINFO_STATE_RUNNING)
> -		return -EPERM;
> -
>   	total_size = info->screen_size;
>   
>   	if (total_size == 0)
> @@ -64,9 +61,6 @@ ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
>   	unsigned long total_size, c;
>   	size_t ret;
>   
> -	if (info->state != FBINFO_STATE_RUNNING)
> -		return -EPERM;
> -
>   	total_size = info->screen_size;
>   
>   	if (total_size == 0)
> diff --git a/drivers/video/fbdev/sm712fb.c b/drivers/video/fbdev/sm712fb.c
> index b528776c7612..6f852cd756c5 100644
> --- a/drivers/video/fbdev/sm712fb.c
> +++ b/drivers/video/fbdev/sm712fb.c
> @@ -1031,9 +1031,6 @@ static ssize_t smtcfb_read(struct fb_info *info, char __user *buf,
>   	if (!info || !info->screen_base)
>   		return -ENODEV;
>   
> -	if (info->state != FBINFO_STATE_RUNNING)
> -		return -EPERM;
> -
>   	total_size = info->screen_size;
>   
>   	if (total_size == 0)
> @@ -1097,9 +1094,6 @@ static ssize_t smtcfb_write(struct fb_info *info, const char __user *buf,
>   	if (!info || !info->screen_base)
>   		return -ENODEV;
>   
> -	if (info->state != FBINFO_STATE_RUNNING)
> -		return -EPERM;
> -
>   	total_size = info->screen_size;
>   
>   	if (total_size == 0)

Re: [PATCH 3/6] fbdev: Don't re-validate info->state in fb_ops implementations

[Geert Uytterhoeven]

Hi Thomas,

On Tue, Apr 25, 2023 at 4:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> The file-op entry points fb_read() and fb_write() verify that
> info->state has been set to FBINFO_STATE_RUNNING. Remove the same
> test from the implementations of struct fb_ops.{fb_read,fb_write}.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Thanks for your patch!

>  drivers/video/fbdev/core/fb_sys_fops.c | 6 ------
>  drivers/video/fbdev/sm712fb.c          | 6 ------

Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>

But there are several other fbdev drivers to fix:
drivers/media/pci/ivtv/ivtvfb.c
drivers/video/fbdev/broadsheetfb.c
drivers/video/fbdev/hecubafb.c
drivers/video/fbdev/metronomefb.c

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

Re: [PATCH 3/6] fbdev: Don't re-validate info->state in fb_ops implementations

[Thomas Zimmermann]

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

Hi

Am 26.04.23 um 16:49 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> On Tue, Apr 25, 2023 at 4:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> The file-op entry points fb_read() and fb_write() verify that
>> info->state has been set to FBINFO_STATE_RUNNING. Remove the same
>> test from the implementations of struct fb_ops.{fb_read,fb_write}.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Thanks for your patch!
> 
>>   drivers/video/fbdev/core/fb_sys_fops.c | 6 ------
>>   drivers/video/fbdev/sm712fb.c          | 6 ------
> 
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> 
> But there are several other fbdev drivers to fix:
> drivers/media/pci/ivtv/ivtvfb.c
> drivers/video/fbdev/broadsheetfb.c
> drivers/video/fbdev/hecubafb.c
> drivers/video/fbdev/metronomefb.c

Ok, I'll go through the various patches in this patchset and check if 
they need to be done elsewhere as well.

Best regards
Thomas

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

[PATCH 4/6] fbdev: Validate info->screen_{base,buffer} in fb_ops implementations

[Thomas Zimmermann]

Push the test for info->screen_base from fb_read() and fb_write() into
the implementations of struct fb_ops.{fb_read,fb_write}. In cases where
the driver operates on info->screen_buffer, test this field instead.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/cobalt_lcdfb.c     |  6 ++++++
 drivers/video/fbdev/core/fb_sys_fops.c |  6 ++++++
 drivers/video/fbdev/core/fbmem.c       | 10 ++++++++--
 drivers/video/fbdev/sm712fb.c          |  4 ++--
 4 files changed, 22 insertions(+), 4 deletions(-)

diff --git a/drivers/video/fbdev/cobalt_lcdfb.c b/drivers/video/fbdev/cobalt_lcdfb.c
index 5f8b6324d2e8..26dbd1c78195 100644
--- a/drivers/video/fbdev/cobalt_lcdfb.c
+++ b/drivers/video/fbdev/cobalt_lcdfb.c
@@ -129,6 +129,9 @@ static ssize_t cobalt_lcdfb_read(struct fb_info *info, char __user *buf,
 	unsigned long pos;
 	int len, retval = 0;
 
+	if (!info->screen_base)
+		return -ENODEV;
+
 	pos = *ppos;
 	if (pos >= LCD_CHARS_MAX || count == 0)
 		return 0;
@@ -175,6 +178,9 @@ static ssize_t cobalt_lcdfb_write(struct fb_info *info, const char __user *buf,
 	unsigned long pos;
 	int len, retval = 0;
 
+	if (!info->screen_base)
+		return -ENODEV;
+
 	pos = *ppos;
 	if (pos >= LCD_CHARS_MAX || count == 0)
 		return 0;
diff --git a/drivers/video/fbdev/core/fb_sys_fops.c b/drivers/video/fbdev/core/fb_sys_fops.c
index 7dee5d3c7fb1..0cb0989abda6 100644
--- a/drivers/video/fbdev/core/fb_sys_fops.c
+++ b/drivers/video/fbdev/core/fb_sys_fops.c
@@ -22,6 +22,9 @@ ssize_t fb_sys_read(struct fb_info *info, char __user *buf, size_t count,
 	unsigned long total_size, c;
 	ssize_t ret;
 
+	if (!info->screen_buffer)
+		return -ENODEV;
+
 	total_size = info->screen_size;
 
 	if (total_size == 0)
@@ -61,6 +64,9 @@ ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
 	unsigned long total_size, c;
 	size_t ret;
 
+	if (!info->screen_buffer)
+		return -ENODEV;
+
 	total_size = info->screen_size;
 
 	if (total_size == 0)
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index abc92d79a295..b993bb97058f 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -768,7 +768,7 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	int c, cnt = 0, err = 0;
 	unsigned long total_size;
 
-	if (!info || ! info->screen_base)
+	if (!info)
 		return -ENODEV;
 
 	if (info->state != FBINFO_STATE_RUNNING)
@@ -777,6 +777,9 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	if (info->fbops->fb_read)
 		return info->fbops->fb_read(info, buf, count, ppos);
 
+	if (!info->screen_base)
+		return -ENODEV;
+
 	total_size = info->screen_size;
 
 	if (total_size == 0)
@@ -833,7 +836,7 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 	int c, cnt = 0, err = 0;
 	unsigned long total_size;
 
-	if (!info || !info->screen_base)
+	if (!info)
 		return -ENODEV;
 
 	if (info->state != FBINFO_STATE_RUNNING)
@@ -842,6 +845,9 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 	if (info->fbops->fb_write)
 		return info->fbops->fb_write(info, buf, count, ppos);
 
+	if (!info->screen_base)
+		return -ENODEV;
+
 	total_size = info->screen_size;
 
 	if (total_size == 0)
diff --git a/drivers/video/fbdev/sm712fb.c b/drivers/video/fbdev/sm712fb.c
index 6f852cd756c5..b7ad3c644e13 100644
--- a/drivers/video/fbdev/sm712fb.c
+++ b/drivers/video/fbdev/sm712fb.c
@@ -1028,7 +1028,7 @@ static ssize_t smtcfb_read(struct fb_info *info, char __user *buf,
 	int c, i, cnt = 0, err = 0;
 	unsigned long total_size;
 
-	if (!info || !info->screen_base)
+	if (!info->screen_base)
 		return -ENODEV;
 
 	total_size = info->screen_size;
@@ -1091,7 +1091,7 @@ static ssize_t smtcfb_write(struct fb_info *info, const char __user *buf,
 	int c, i, cnt = 0, err = 0;
 	unsigned long total_size;
 
-	if (!info || !info->screen_base)
+	if (!info->screen_base)
 		return -ENODEV;
 
 	total_size = info->screen_size;
-- 
2.40.0

Re: [PATCH 4/6] fbdev: Validate info->screen_{base,buffer} in fb_ops implementations

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Push the test for info->screen_base from fb_read() and fb_write() into
> the implementations of struct fb_ops.{fb_read,fb_write}. In cases where
> the driver operates on info->screen_buffer, test this field instead.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [4/6] fbdev: Validate info->screen_{base, buffer} in fb_ops implementations

[Sui Jingfeng]

Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>

On 2023/4/25 22:28, Thomas Zimmermann wrote:
> Push the test for info->screen_base from fb_read() and fb_write() into
> the implementations of struct fb_ops.{fb_read,fb_write}. In cases where
> the driver operates on info->screen_buffer, test this field instead.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>   drivers/video/fbdev/cobalt_lcdfb.c     |  6 ++++++
>   drivers/video/fbdev/core/fb_sys_fops.c |  6 ++++++
>   drivers/video/fbdev/core/fbmem.c       | 10 ++++++++--
>   drivers/video/fbdev/sm712fb.c          |  4 ++--
>   4 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/video/fbdev/cobalt_lcdfb.c b/drivers/video/fbdev/cobalt_lcdfb.c
> index 5f8b6324d2e8..26dbd1c78195 100644
> --- a/drivers/video/fbdev/cobalt_lcdfb.c
> +++ b/drivers/video/fbdev/cobalt_lcdfb.c
> @@ -129,6 +129,9 @@ static ssize_t cobalt_lcdfb_read(struct fb_info *info, char __user *buf,
>   	unsigned long pos;
>   	int len, retval = 0;
>   
> +	if (!info->screen_base)
> +		return -ENODEV;
> +
>   	pos = *ppos;
>   	if (pos >= LCD_CHARS_MAX || count == 0)
>   		return 0;
> @@ -175,6 +178,9 @@ static ssize_t cobalt_lcdfb_write(struct fb_info *info, const char __user *buf,
>   	unsigned long pos;
>   	int len, retval = 0;
>   
> +	if (!info->screen_base)
> +		return -ENODEV;
> +
>   	pos = *ppos;
>   	if (pos >= LCD_CHARS_MAX || count == 0)
>   		return 0;
> diff --git a/drivers/video/fbdev/core/fb_sys_fops.c b/drivers/video/fbdev/core/fb_sys_fops.c
> index 7dee5d3c7fb1..0cb0989abda6 100644
> --- a/drivers/video/fbdev/core/fb_sys_fops.c
> +++ b/drivers/video/fbdev/core/fb_sys_fops.c
> @@ -22,6 +22,9 @@ ssize_t fb_sys_read(struct fb_info *info, char __user *buf, size_t count,
>   	unsigned long total_size, c;
>   	ssize_t ret;
>   
> +	if (!info->screen_buffer)
> +		return -ENODEV;
> +
>   	total_size = info->screen_size;
>   
>   	if (total_size == 0)
> @@ -61,6 +64,9 @@ ssize_t fb_sys_write(struct fb_info *info, const char __user *buf,
>   	unsigned long total_size, c;
>   	size_t ret;
>   
> +	if (!info->screen_buffer)
> +		return -ENODEV;
> +
>   	total_size = info->screen_size;
>   
>   	if (total_size == 0)
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index abc92d79a295..b993bb97058f 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -768,7 +768,7 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>   	int c, cnt = 0, err = 0;
>   	unsigned long total_size;
>   
> -	if (!info || ! info->screen_base)
> +	if (!info)
>   		return -ENODEV;
>   
>   	if (info->state != FBINFO_STATE_RUNNING)
> @@ -777,6 +777,9 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>   	if (info->fbops->fb_read)
>   		return info->fbops->fb_read(info, buf, count, ppos);
>   
> +	if (!info->screen_base)
> +		return -ENODEV;
> +
>   	total_size = info->screen_size;
>   
>   	if (total_size == 0)
> @@ -833,7 +836,7 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
>   	int c, cnt = 0, err = 0;
>   	unsigned long total_size;
>   
> -	if (!info || !info->screen_base)
> +	if (!info)
>   		return -ENODEV;
>   
>   	if (info->state != FBINFO_STATE_RUNNING)
> @@ -842,6 +845,9 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
>   	if (info->fbops->fb_write)
>   		return info->fbops->fb_write(info, buf, count, ppos);
>   
> +	if (!info->screen_base)
> +		return -ENODEV;
> +
>   	total_size = info->screen_size;
>   
>   	if (total_size == 0)
> diff --git a/drivers/video/fbdev/sm712fb.c b/drivers/video/fbdev/sm712fb.c
> index 6f852cd756c5..b7ad3c644e13 100644
> --- a/drivers/video/fbdev/sm712fb.c
> +++ b/drivers/video/fbdev/sm712fb.c
> @@ -1028,7 +1028,7 @@ static ssize_t smtcfb_read(struct fb_info *info, char __user *buf,
>   	int c, i, cnt = 0, err = 0;
>   	unsigned long total_size;
>   
> -	if (!info || !info->screen_base)
> +	if (!info->screen_base)
>   		return -ENODEV;
>   
>   	total_size = info->screen_size;
> @@ -1091,7 +1091,7 @@ static ssize_t smtcfb_write(struct fb_info *info, const char __user *buf,
>   	int c, i, cnt = 0, err = 0;
>   	unsigned long total_size;
>   
> -	if (!info || !info->screen_base)
> +	if (!info->screen_base)
>   		return -ENODEV;
>   
>   	total_size = info->screen_size;

Re: [PATCH 4/6] fbdev: Validate info->screen_{base,buffer} in fb_ops implementations

[Geert Uytterhoeven]

Hi Thomas,


On Tue, Apr 25, 2023 at 4:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Push the test for info->screen_base from fb_read() and fb_write() into
> the implementations of struct fb_ops.{fb_read,fb_write}. In cases where
> the driver operates on info->screen_buffer, test this field instead.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Thanks for your patch!
You forgot to mention why it is a good idea to duplicate this in all
the implementations, instead of doing it once in the core?

>  drivers/video/fbdev/cobalt_lcdfb.c     |  6 ++++++
>  drivers/video/fbdev/core/fb_sys_fops.c |  6 ++++++
>  drivers/video/fbdev/core/fbmem.c       | 10 ++++++++--
>  drivers/video/fbdev/sm712fb.c          |  4 ++--
>  4 files changed, 22 insertions(+), 4 deletions(-)

Aren't there more fbdev drivers to fix, before you can move the checks
in drivers/video/fbdev/core/fbmem.c?

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

Re: [PATCH 4/6] fbdev: Validate info->screen_{base,buffer} in fb_ops implementations

[Thomas Zimmermann]

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

Hi

Am 26.04.23 um 16:56 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> 
> On Tue, Apr 25, 2023 at 4:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Push the test for info->screen_base from fb_read() and fb_write() into
>> the implementations of struct fb_ops.{fb_read,fb_write}. In cases where
>> the driver operates on info->screen_buffer, test this field instead.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Thanks for your patch!
> You forgot to mention why it is a good idea to duplicate this in all
> the implementations, instead of doing it once in the core?
> 
>>   drivers/video/fbdev/cobalt_lcdfb.c     |  6 ++++++
>>   drivers/video/fbdev/core/fb_sys_fops.c |  6 ++++++
>>   drivers/video/fbdev/core/fbmem.c       | 10 ++++++++--
>>   drivers/video/fbdev/sm712fb.c          |  4 ++--
>>   4 files changed, 22 insertions(+), 4 deletions(-)
> 
> Aren't there more fbdev drivers to fix, before you can move the checks
> in drivers/video/fbdev/core/fbmem.c?

I've found a few. And I've also found quite a number of drivers that use 
screen_base when they should use screen_buffer instead. I'll fix them as 
well.

Best regards
Thomas

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

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

[Thomas Zimmermann]

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

diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
index 08fabce76b74..cb7534a80305 100644
--- a/drivers/video/fbdev/core/Makefile
+++ b/drivers/video/fbdev/core/Makefile
@@ -2,7 +2,7 @@
 obj-$(CONFIG_FB_NOTIFY)           += fb_notify.o
 obj-$(CONFIG_FB)                  += fb.o
 fb-y                              := fbmem.o fbmon.o fbcmap.o fbsysfs.o \
-                                     modedb.o fbcvt.o fb_cmdline.o
+                                     modedb.o fbcvt.o fb_cmdline.o fb_cfb_fops.o
 fb-$(CONFIG_FB_DEFERRED_IO)       += fb_defio.o
 
 ifeq ($(CONFIG_FRAMEBUFFER_CONSOLE),y)
diff --git a/drivers/video/fbdev/core/fb_cfb_fops.c b/drivers/video/fbdev/core/fb_cfb_fops.c
new file mode 100644
index 000000000000..f6000166eda4
--- /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)
+{
+	unsigned long p = *ppos;
+	u8 *buffer, *dst;
+	u8 __iomem *src;
+	int c, cnt = 0, err = 0;
+	unsigned long total_size;
+
+	if (!info->screen_base)
+		return -ENODEV;
+
+	total_size = info->screen_size;
+
+	if (total_size == 0)
+		total_size = info->fix.smem_len;
+
+	if (p >= total_size)
+		return 0;
+
+	if (count >= total_size)
+		count = total_size;
+
+	if (count + p > total_size)
+		count = total_size - p;
+
+	buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	src = (u8 __iomem *)(info->screen_base + p);
+
+	if (info->fbops->fb_sync)
+		info->fbops->fb_sync(info);
+
+	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)) {
+			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)
+{
+	unsigned long p = *ppos;
+	u8 *buffer, *src;
+	u8 __iomem *dst;
+	int c, cnt = 0, err = 0;
+	unsigned long total_size;
+
+	if (!info->screen_base)
+		return -ENODEV;
+
+	total_size = info->screen_size;
+
+	if (total_size == 0)
+		total_size = info->fix.smem_len;
+
+	if (p > total_size)
+		return -EFBIG;
+
+	if (count > total_size) {
+		err = -EFBIG;
+		count = total_size;
+	}
+
+	if (count + p > total_size) {
+		if (!err)
+			err = -ENOSPC;
+
+		count = total_size - p;
+	}
+
+	buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count, GFP_KERNEL);
+	if (!buffer)
+		return -ENOMEM;
+
+	dst = (u8 __iomem *)(info->screen_base + p);
+
+	if (info->fbops->fb_sync)
+		info->fbops->fb_sync(info);
+
+	while (count) {
+		c = (count > PAGE_SIZE) ? PAGE_SIZE : count;
+		src = buffer;
+
+		if (copy_from_user(src, buf, c)) {
+			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;
+}
+EXPORT_SYMBOL(fb_cfb_write);
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index b993bb97058f..be6c75f3dfd0 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -761,12 +761,7 @@ static struct fb_info *file_fb_info(struct file *file)
 static ssize_t
 fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 {
-	unsigned long p = *ppos;
 	struct fb_info *info = file_fb_info(file);
-	u8 *buffer, *dst;
-	u8 __iomem *src;
-	int c, cnt = 0, err = 0;
-	unsigned long total_size;
 
 	if (!info)
 		return -ENODEV;
@@ -777,64 +772,13 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	if (info->fbops->fb_read)
 		return info->fbops->fb_read(info, buf, count, ppos);
 
-	if (!info->screen_base)
-		return -ENODEV;
-
-	total_size = info->screen_size;
-
-	if (total_size == 0)
-		total_size = info->fix.smem_len;
-
-	if (p >= total_size)
-		return 0;
-
-	if (count >= total_size)
-		count = total_size;
-
-	if (count + p > total_size)
-		count = total_size - p;
-
-	buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
-			 GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
-
-	src = (u8 __iomem *) (info->screen_base + p);
-
-	if (info->fbops->fb_sync)
-		info->fbops->fb_sync(info);
-
-	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)) {
-			err = -EFAULT;
-			break;
-		}
-		*ppos += c;
-		buf += c;
-		cnt += c;
-		count -= c;
-	}
-
-	kfree(buffer);
-
-	return cnt ? cnt : err;
+	return fb_cfb_read(info, buf, count, ppos);
 }
 
 static ssize_t
 fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 {
-	unsigned long p = *ppos;
 	struct fb_info *info = file_fb_info(file);
-	u8 *buffer, *src;
-	u8 __iomem *dst;
-	int c, cnt = 0, err = 0;
-	unsigned long total_size;
 
 	if (!info)
 		return -ENODEV;
@@ -845,60 +789,7 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 	if (info->fbops->fb_write)
 		return info->fbops->fb_write(info, buf, count, ppos);
 
-	if (!info->screen_base)
-		return -ENODEV;
-
-	total_size = info->screen_size;
-
-	if (total_size == 0)
-		total_size = info->fix.smem_len;
-
-	if (p > total_size)
-		return -EFBIG;
-
-	if (count > total_size) {
-		err = -EFBIG;
-		count = total_size;
-	}
-
-	if (count + p > total_size) {
-		if (!err)
-			err = -ENOSPC;
-
-		count = total_size - p;
-	}
-
-	buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
-			 GFP_KERNEL);
-	if (!buffer)
-		return -ENOMEM;
-
-	dst = (u8 __iomem *) (info->screen_base + p);
-
-	if (info->fbops->fb_sync)
-		info->fbops->fb_sync(info);
-
-	while (count) {
-		c = (count > PAGE_SIZE) ? PAGE_SIZE : count;
-		src = buffer;
-
-		if (copy_from_user(src, buf, c)) {
-			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;
+	return fb_cfb_write(info, buf, count, ppos);
 }
 
 int
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 08cb47da71f8..3b1644c79973 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -576,9 +576,19 @@ struct fb_info {
 extern int fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var);
 extern int fb_pan_display(struct fb_info *info, struct fb_var_screeninfo *var);
 extern int fb_blank(struct fb_info *info, int blank);
+
+/*
+ * Drawing operations where framebuffer is in video RAM
+ */
+
 extern void cfb_fillrect(struct fb_info *info, const struct fb_fillrect *rect);
 extern void cfb_copyarea(struct fb_info *info, const struct fb_copyarea *area);
 extern void cfb_imageblit(struct fb_info *info, const struct fb_image *image);
+extern ssize_t fb_cfb_read(struct fb_info *info, char __user *buf, size_t count,
+			   loff_t *ppos);
+extern ssize_t fb_cfb_write(struct fb_info *info, const char __user *buf,
+			    size_t count, loff_t *ppos);
+
 /*
  * Drawing operations where framebuffer is in system RAM
  */
-- 
2.40.0

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

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

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

It would be nice to get an explanation here about why moving these
make sense. I guess you are doing this because is going to be used
in DRM but still would be good to explain it in the commit message.

> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

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

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master next-20230425]
[cannot apply to v6.3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/fbdev-Return-number-of-bytes-read-or-written/20230425-223011
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20230425142846.730-6-tzimmermann%40suse.de
patch subject: [PATCH 5/6] fbdev: Move CFB read and write code into helper functions
config: riscv-randconfig-s033-20230423 (https://download.01.org/0day-ci/archive/20230426/202304261317.QAEwArcB-lkp@intel.com/config)
compiler: riscv64-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/d4a150f3dfa8e73f2e92f1c7efc9271e17632cc2
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Thomas-Zimmermann/fbdev-Return-number-of-bytes-read-or-written/20230425-223011
        git checkout d4a150f3dfa8e73f2e92f1c7efc9271e17632cc2
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=riscv olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=riscv SHELL=/bin/bash drivers/video/fbdev/core/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304261317.QAEwArcB-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
   WARNING: invalid argument to '-march': '_zihintpause'
>> drivers/video/fbdev/core/fb_cfb_fops.c:44:39: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void const * @@     got unsigned char [noderef] [usertype] __iomem *[assigned] src @@
   drivers/video/fbdev/core/fb_cfb_fops.c:44:39: sparse:     expected void const *
   drivers/video/fbdev/core/fb_cfb_fops.c:44:39: sparse:     got unsigned char [noderef] [usertype] __iomem *[assigned] src
>> drivers/video/fbdev/core/fb_cfb_fops.c:113:32: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void * @@     got unsigned char [noderef] [usertype] __iomem *[assigned] dst @@
   drivers/video/fbdev/core/fb_cfb_fops.c:113:32: sparse:     expected void *
   drivers/video/fbdev/core/fb_cfb_fops.c:113:32: sparse:     got unsigned char [noderef] [usertype] __iomem *[assigned] dst

vim +44 drivers/video/fbdev/core/fb_cfb_fops.c

     6	
     7	ssize_t fb_cfb_read(struct fb_info *info, char __user *buf, size_t count, loff_t *ppos)
     8	{
     9		unsigned long p = *ppos;
    10		u8 *buffer, *dst;
    11		u8 __iomem *src;
    12		int c, cnt = 0, err = 0;
    13		unsigned long total_size;
    14	
    15		if (!info->screen_base)
    16			return -ENODEV;
    17	
    18		total_size = info->screen_size;
    19	
    20		if (total_size == 0)
    21			total_size = info->fix.smem_len;
    22	
    23		if (p >= total_size)
    24			return 0;
    25	
    26		if (count >= total_size)
    27			count = total_size;
    28	
    29		if (count + p > total_size)
    30			count = total_size - p;
    31	
    32		buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count, GFP_KERNEL);
    33		if (!buffer)
    34			return -ENOMEM;
    35	
    36		src = (u8 __iomem *)(info->screen_base + p);
    37	
    38		if (info->fbops->fb_sync)
    39			info->fbops->fb_sync(info);
    40	
    41		while (count) {
    42			c  = (count > PAGE_SIZE) ? PAGE_SIZE : count;
    43			dst = buffer;
  > 44			fb_memcpy_fromfb(dst, src, c);
    45			dst += c;
    46			src += c;
    47	
    48			if (copy_to_user(buf, buffer, c)) {
    49				err = -EFAULT;
    50				break;
    51			}
    52			*ppos += c;
    53			buf += c;
    54			cnt += c;
    55			count -= c;
    56		}
    57	
    58		kfree(buffer);
    59	
    60		return cnt ? cnt : err;
    61	}
    62	EXPORT_SYMBOL(fb_cfb_read);
    63	
    64	ssize_t fb_cfb_write(struct fb_info *info, const char __user *buf, size_t count, loff_t *ppos)
    65	{
    66		unsigned long p = *ppos;
    67		u8 *buffer, *src;
    68		u8 __iomem *dst;
    69		int c, cnt = 0, err = 0;
    70		unsigned long total_size;
    71	
    72		if (!info->screen_base)
    73			return -ENODEV;
    74	
    75		total_size = info->screen_size;
    76	
    77		if (total_size == 0)
    78			total_size = info->fix.smem_len;
    79	
    80		if (p > total_size)
    81			return -EFBIG;
    82	
    83		if (count > total_size) {
    84			err = -EFBIG;
    85			count = total_size;
    86		}
    87	
    88		if (count + p > total_size) {
    89			if (!err)
    90				err = -ENOSPC;
    91	
    92			count = total_size - p;
    93		}
    94	
    95		buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count, GFP_KERNEL);
    96		if (!buffer)
    97			return -ENOMEM;
    98	
    99		dst = (u8 __iomem *)(info->screen_base + p);
   100	
   101		if (info->fbops->fb_sync)
   102			info->fbops->fb_sync(info);
   103	
   104		while (count) {
   105			c = (count > PAGE_SIZE) ? PAGE_SIZE : count;
   106			src = buffer;
   107	
   108			if (copy_from_user(src, buf, c)) {
   109				err = -EFAULT;
   110				break;
   111			}
   112	
 > 113			fb_memcpy_tofb(dst, src, c);

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[Thomas Zimmermann]

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

Hi

Am 26.04.23 um 07:16 schrieb kernel test robot:
> Hi Thomas,
> 
> kernel test robot noticed the following build warnings:

FYI these errors come from systems that use volatile __iomem pointers 
with plain memcpy(). See my patchset at [1] for an improvement.

Best regards
Thomas

[1] https://patchwork.freedesktop.org/series/116985/

> 
> [auto build test WARNING on drm-misc/drm-misc-next]
> [also build test WARNING on linus/master next-20230425]
> [cannot apply to v6.3]
> [If your patch is applied to the wrong git tree, kindly drop us a note.
> And when submitting patch, we suggest to use '--base' as documented in
> https://git-scm.com/docs/git-format-patch#_base_tree_information]
> 
> url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/fbdev-Return-number-of-bytes-read-or-written/20230425-223011
> base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
> patch link:    https://lore.kernel.org/r/20230425142846.730-6-tzimmermann%40suse.de
> patch subject: [PATCH 5/6] fbdev: Move CFB read and write code into helper functions
> config: riscv-randconfig-s033-20230423 (https://download.01.org/0day-ci/archive/20230426/202304261317.QAEwArcB-lkp@intel.com/config)
> compiler: riscv64-linux-gcc (GCC) 12.1.0
> reproduce:
>          wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
>          chmod +x ~/bin/make.cross
>          # apt-get install sparse
>          # sparse version: v0.6.4-39-gce1a6720-dirty
>          # https://github.com/intel-lab-lkp/linux/commit/d4a150f3dfa8e73f2e92f1c7efc9271e17632cc2
>          git remote add linux-review https://github.com/intel-lab-lkp/linux
>          git fetch --no-tags linux-review Thomas-Zimmermann/fbdev-Return-number-of-bytes-read-or-written/20230425-223011
>          git checkout d4a150f3dfa8e73f2e92f1c7efc9271e17632cc2
>          # save the config file
>          mkdir build_dir && cp config build_dir/.config
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=riscv olddefconfig
>          COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=riscv SHELL=/bin/bash drivers/video/fbdev/core/
> 
> If you fix the issue, kindly add following tag where applicable
> | Reported-by: kernel test robot <lkp@intel.com>
> | Link: https://lore.kernel.org/oe-kbuild-all/202304261317.QAEwArcB-lkp@intel.com/
> 
> sparse warnings: (new ones prefixed by >>)
>     WARNING: invalid argument to '-march': '_zihintpause'
>>> drivers/video/fbdev/core/fb_cfb_fops.c:44:39: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void const * @@     got unsigned char [noderef] [usertype] __iomem *[assigned] src @@
>     drivers/video/fbdev/core/fb_cfb_fops.c:44:39: sparse:     expected void const *
>     drivers/video/fbdev/core/fb_cfb_fops.c:44:39: sparse:     got unsigned char [noderef] [usertype] __iomem *[assigned] src
>>> drivers/video/fbdev/core/fb_cfb_fops.c:113:32: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void * @@     got unsigned char [noderef] [usertype] __iomem *[assigned] dst @@
>     drivers/video/fbdev/core/fb_cfb_fops.c:113:32: sparse:     expected void *
>     drivers/video/fbdev/core/fb_cfb_fops.c:113:32: sparse:     got unsigned char [noderef] [usertype] __iomem *[assigned] dst
> 
> vim +44 drivers/video/fbdev/core/fb_cfb_fops.c
> 
>       6	
>       7	ssize_t fb_cfb_read(struct fb_info *info, char __user *buf, size_t count, loff_t *ppos)
>       8	{
>       9		unsigned long p = *ppos;
>      10		u8 *buffer, *dst;
>      11		u8 __iomem *src;
>      12		int c, cnt = 0, err = 0;
>      13		unsigned long total_size;
>      14	
>      15		if (!info->screen_base)
>      16			return -ENODEV;
>      17	
>      18		total_size = info->screen_size;
>      19	
>      20		if (total_size == 0)
>      21			total_size = info->fix.smem_len;
>      22	
>      23		if (p >= total_size)
>      24			return 0;
>      25	
>      26		if (count >= total_size)
>      27			count = total_size;
>      28	
>      29		if (count + p > total_size)
>      30			count = total_size - p;
>      31	
>      32		buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count, GFP_KERNEL);
>      33		if (!buffer)
>      34			return -ENOMEM;
>      35	
>      36		src = (u8 __iomem *)(info->screen_base + p);
>      37	
>      38		if (info->fbops->fb_sync)
>      39			info->fbops->fb_sync(info);
>      40	
>      41		while (count) {
>      42			c  = (count > PAGE_SIZE) ? PAGE_SIZE : count;
>      43			dst = buffer;
>    > 44			fb_memcpy_fromfb(dst, src, c);
>      45			dst += c;
>      46			src += c;
>      47	
>      48			if (copy_to_user(buf, buffer, c)) {
>      49				err = -EFAULT;
>      50				break;
>      51			}
>      52			*ppos += c;
>      53			buf += c;
>      54			cnt += c;
>      55			count -= c;
>      56		}
>      57	
>      58		kfree(buffer);
>      59	
>      60		return cnt ? cnt : err;
>      61	}
>      62	EXPORT_SYMBOL(fb_cfb_read);
>      63	
>      64	ssize_t fb_cfb_write(struct fb_info *info, const char __user *buf, size_t count, loff_t *ppos)
>      65	{
>      66		unsigned long p = *ppos;
>      67		u8 *buffer, *src;
>      68		u8 __iomem *dst;
>      69		int c, cnt = 0, err = 0;
>      70		unsigned long total_size;
>      71	
>      72		if (!info->screen_base)
>      73			return -ENODEV;
>      74	
>      75		total_size = info->screen_size;
>      76	
>      77		if (total_size == 0)
>      78			total_size = info->fix.smem_len;
>      79	
>      80		if (p > total_size)
>      81			return -EFBIG;
>      82	
>      83		if (count > total_size) {
>      84			err = -EFBIG;
>      85			count = total_size;
>      86		}
>      87	
>      88		if (count + p > total_size) {
>      89			if (!err)
>      90				err = -ENOSPC;
>      91	
>      92			count = total_size - p;
>      93		}
>      94	
>      95		buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count, GFP_KERNEL);
>      96		if (!buffer)
>      97			return -ENOMEM;
>      98	
>      99		dst = (u8 __iomem *)(info->screen_base + p);
>     100	
>     101		if (info->fbops->fb_sync)
>     102			info->fbops->fb_sync(info);
>     103	
>     104		while (count) {
>     105			c = (count > PAGE_SIZE) ? PAGE_SIZE : count;
>     106			src = buffer;
>     107	
>     108			if (copy_from_user(src, buf, c)) {
>     109				err = -EFAULT;
>     110				break;
>     111			}
>     112	
>   > 113			fb_memcpy_tofb(dst, src, c);
> 

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

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

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master next-20230425]
[cannot apply to v6.3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/fbdev-Return-number-of-bytes-read-or-written/20230425-223011
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20230425142846.730-6-tzimmermann%40suse.de
patch subject: [PATCH 5/6] fbdev: Move CFB read and write code into helper functions
config: nios2-randconfig-s031-20230423 (https://download.01.org/0day-ci/archive/20230426/202304261333.9giYEbEl-lkp@intel.com/config)
compiler: nios2-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/d4a150f3dfa8e73f2e92f1c7efc9271e17632cc2
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Thomas-Zimmermann/fbdev-Return-number-of-bytes-read-or-written/20230425-223011
        git checkout d4a150f3dfa8e73f2e92f1c7efc9271e17632cc2
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=nios2 olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=nios2 SHELL=/bin/bash drivers/video/fbdev/core/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304261333.9giYEbEl-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/video/fbdev/core/fb_cfb_fops.c:44:39: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void const *s @@     got unsigned char [noderef] [usertype] __iomem *[assigned] src @@
   drivers/video/fbdev/core/fb_cfb_fops.c:44:39: sparse:     expected void const *s
   drivers/video/fbdev/core/fb_cfb_fops.c:44:39: sparse:     got unsigned char [noderef] [usertype] __iomem *[assigned] src
>> drivers/video/fbdev/core/fb_cfb_fops.c:113:32: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void *d @@     got unsigned char [noderef] [usertype] __iomem *[assigned] dst @@
   drivers/video/fbdev/core/fb_cfb_fops.c:113:32: sparse:     expected void *d
   drivers/video/fbdev/core/fb_cfb_fops.c:113:32: sparse:     got unsigned char [noderef] [usertype] __iomem *[assigned] dst

vim +44 drivers/video/fbdev/core/fb_cfb_fops.c

     6	
     7	ssize_t fb_cfb_read(struct fb_info *info, char __user *buf, size_t count, loff_t *ppos)
     8	{
     9		unsigned long p = *ppos;
    10		u8 *buffer, *dst;
    11		u8 __iomem *src;
    12		int c, cnt = 0, err = 0;
    13		unsigned long total_size;
    14	
    15		if (!info->screen_base)
    16			return -ENODEV;
    17	
    18		total_size = info->screen_size;
    19	
    20		if (total_size == 0)
    21			total_size = info->fix.smem_len;
    22	
    23		if (p >= total_size)
    24			return 0;
    25	
    26		if (count >= total_size)
    27			count = total_size;
    28	
    29		if (count + p > total_size)
    30			count = total_size - p;
    31	
    32		buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count, GFP_KERNEL);
    33		if (!buffer)
    34			return -ENOMEM;
    35	
    36		src = (u8 __iomem *)(info->screen_base + p);
    37	
    38		if (info->fbops->fb_sync)
    39			info->fbops->fb_sync(info);
    40	
    41		while (count) {
    42			c  = (count > PAGE_SIZE) ? PAGE_SIZE : count;
    43			dst = buffer;
  > 44			fb_memcpy_fromfb(dst, src, c);
    45			dst += c;
    46			src += c;
    47	
    48			if (copy_to_user(buf, buffer, c)) {
    49				err = -EFAULT;
    50				break;
    51			}
    52			*ppos += c;
    53			buf += c;
    54			cnt += c;
    55			count -= c;
    56		}
    57	
    58		kfree(buffer);
    59	
    60		return cnt ? cnt : err;
    61	}
    62	EXPORT_SYMBOL(fb_cfb_read);
    63	
    64	ssize_t fb_cfb_write(struct fb_info *info, const char __user *buf, size_t count, loff_t *ppos)
    65	{
    66		unsigned long p = *ppos;
    67		u8 *buffer, *src;
    68		u8 __iomem *dst;
    69		int c, cnt = 0, err = 0;
    70		unsigned long total_size;
    71	
    72		if (!info->screen_base)
    73			return -ENODEV;
    74	
    75		total_size = info->screen_size;
    76	
    77		if (total_size == 0)
    78			total_size = info->fix.smem_len;
    79	
    80		if (p > total_size)
    81			return -EFBIG;
    82	
    83		if (count > total_size) {
    84			err = -EFBIG;
    85			count = total_size;
    86		}
    87	
    88		if (count + p > total_size) {
    89			if (!err)
    90				err = -ENOSPC;
    91	
    92			count = total_size - p;
    93		}
    94	
    95		buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count, GFP_KERNEL);
    96		if (!buffer)
    97			return -ENOMEM;
    98	
    99		dst = (u8 __iomem *)(info->screen_base + p);
   100	
   101		if (info->fbops->fb_sync)
   102			info->fbops->fb_sync(info);
   103	
   104		while (count) {
   105			c = (count > PAGE_SIZE) ? PAGE_SIZE : count;
   106			src = buffer;
   107	
   108			if (copy_from_user(src, buf, c)) {
   109				err = -EFAULT;
   110				break;
   111			}
   112	
 > 113			fb_memcpy_tofb(dst, src, c);

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[kernel test robot]

Hi Thomas,

kernel test robot noticed the following build warnings:

[auto build test WARNING on drm-misc/drm-misc-next]
[also build test WARNING on linus/master next-20230425]
[cannot apply to v6.3]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Thomas-Zimmermann/fbdev-Return-number-of-bytes-read-or-written/20230425-223011
base:   git://anongit.freedesktop.org/drm/drm-misc drm-misc-next
patch link:    https://lore.kernel.org/r/20230425142846.730-6-tzimmermann%40suse.de
patch subject: [PATCH 5/6] fbdev: Move CFB read and write code into helper functions
config: openrisc-randconfig-s052-20230423 (https://download.01.org/0day-ci/archive/20230426/202304261419.LvfW9HTa-lkp@intel.com/config)
compiler: or1k-linux-gcc (GCC) 12.1.0
reproduce:
        wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # apt-get install sparse
        # sparse version: v0.6.4-39-gce1a6720-dirty
        # https://github.com/intel-lab-lkp/linux/commit/d4a150f3dfa8e73f2e92f1c7efc9271e17632cc2
        git remote add linux-review https://github.com/intel-lab-lkp/linux
        git fetch --no-tags linux-review Thomas-Zimmermann/fbdev-Return-number-of-bytes-read-or-written/20230425-223011
        git checkout d4a150f3dfa8e73f2e92f1c7efc9271e17632cc2
        # save the config file
        mkdir build_dir && cp config build_dir/.config
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=openrisc olddefconfig
        COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-12.1.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=openrisc SHELL=/bin/bash drivers/video/fbdev/core/

If you fix the issue, kindly add following tag where applicable
| Reported-by: kernel test robot <lkp@intel.com>
| Link: https://lore.kernel.org/oe-kbuild-all/202304261419.LvfW9HTa-lkp@intel.com/

sparse warnings: (new ones prefixed by >>)
>> drivers/video/fbdev/core/fb_cfb_fops.c:44:39: sparse: sparse: incorrect type in argument 2 (different address spaces) @@     expected void const *src @@     got unsigned char [noderef] [usertype] __iomem *[assigned] src @@
   drivers/video/fbdev/core/fb_cfb_fops.c:44:39: sparse:     expected void const *src
   drivers/video/fbdev/core/fb_cfb_fops.c:44:39: sparse:     got unsigned char [noderef] [usertype] __iomem *[assigned] src
>> drivers/video/fbdev/core/fb_cfb_fops.c:113:32: sparse: sparse: incorrect type in argument 1 (different address spaces) @@     expected void *dest @@     got unsigned char [noderef] [usertype] __iomem *[assigned] dst @@
   drivers/video/fbdev/core/fb_cfb_fops.c:113:32: sparse:     expected void *dest
   drivers/video/fbdev/core/fb_cfb_fops.c:113:32: sparse:     got unsigned char [noderef] [usertype] __iomem *[assigned] dst

vim +44 drivers/video/fbdev/core/fb_cfb_fops.c

     6	
     7	ssize_t fb_cfb_read(struct fb_info *info, char __user *buf, size_t count, loff_t *ppos)
     8	{
     9		unsigned long p = *ppos;
    10		u8 *buffer, *dst;
    11		u8 __iomem *src;
    12		int c, cnt = 0, err = 0;
    13		unsigned long total_size;
    14	
    15		if (!info->screen_base)
    16			return -ENODEV;
    17	
    18		total_size = info->screen_size;
    19	
    20		if (total_size == 0)
    21			total_size = info->fix.smem_len;
    22	
    23		if (p >= total_size)
    24			return 0;
    25	
    26		if (count >= total_size)
    27			count = total_size;
    28	
    29		if (count + p > total_size)
    30			count = total_size - p;
    31	
    32		buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count, GFP_KERNEL);
    33		if (!buffer)
    34			return -ENOMEM;
    35	
    36		src = (u8 __iomem *)(info->screen_base + p);
    37	
    38		if (info->fbops->fb_sync)
    39			info->fbops->fb_sync(info);
    40	
    41		while (count) {
    42			c  = (count > PAGE_SIZE) ? PAGE_SIZE : count;
    43			dst = buffer;
  > 44			fb_memcpy_fromfb(dst, src, c);
    45			dst += c;
    46			src += c;
    47	
    48			if (copy_to_user(buf, buffer, c)) {
    49				err = -EFAULT;
    50				break;
    51			}
    52			*ppos += c;
    53			buf += c;
    54			cnt += c;
    55			count -= c;
    56		}
    57	
    58		kfree(buffer);
    59	
    60		return cnt ? cnt : err;
    61	}
    62	EXPORT_SYMBOL(fb_cfb_read);
    63	
    64	ssize_t fb_cfb_write(struct fb_info *info, const char __user *buf, size_t count, loff_t *ppos)
    65	{
    66		unsigned long p = *ppos;
    67		u8 *buffer, *src;
    68		u8 __iomem *dst;
    69		int c, cnt = 0, err = 0;
    70		unsigned long total_size;
    71	
    72		if (!info->screen_base)
    73			return -ENODEV;
    74	
    75		total_size = info->screen_size;
    76	
    77		if (total_size == 0)
    78			total_size = info->fix.smem_len;
    79	
    80		if (p > total_size)
    81			return -EFBIG;
    82	
    83		if (count > total_size) {
    84			err = -EFBIG;
    85			count = total_size;
    86		}
    87	
    88		if (count + p > total_size) {
    89			if (!err)
    90				err = -ENOSPC;
    91	
    92			count = total_size - p;
    93		}
    94	
    95		buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count, GFP_KERNEL);
    96		if (!buffer)
    97			return -ENOMEM;
    98	
    99		dst = (u8 __iomem *)(info->screen_base + p);
   100	
   101		if (info->fbops->fb_sync)
   102			info->fbops->fb_sync(info);
   103	
   104		while (count) {
   105			c = (count > PAGE_SIZE) ? PAGE_SIZE : count;
   106			src = buffer;
   107	
   108			if (copy_from_user(src, buf, c)) {
   109				err = -EFAULT;
   110				break;
   111			}
   112	
 > 113			fb_memcpy_tofb(dst, src, c);

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests

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

[Sui Jingfeng]

Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>

On 2023/4/25 22:28, Thomas Zimmermann 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>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>   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
>
> diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
> index 08fabce76b74..cb7534a80305 100644
> --- a/drivers/video/fbdev/core/Makefile
> +++ b/drivers/video/fbdev/core/Makefile
> @@ -2,7 +2,7 @@
>   obj-$(CONFIG_FB_NOTIFY)           += fb_notify.o
>   obj-$(CONFIG_FB)                  += fb.o
>   fb-y                              := fbmem.o fbmon.o fbcmap.o fbsysfs.o \
> -                                     modedb.o fbcvt.o fb_cmdline.o
> +                                     modedb.o fbcvt.o fb_cmdline.o fb_cfb_fops.o
>   fb-$(CONFIG_FB_DEFERRED_IO)       += fb_defio.o
>   
>   ifeq ($(CONFIG_FRAMEBUFFER_CONSOLE),y)
> diff --git a/drivers/video/fbdev/core/fb_cfb_fops.c b/drivers/video/fbdev/core/fb_cfb_fops.c
> new file mode 100644
> index 000000000000..f6000166eda4
> --- /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)
> +{
> +	unsigned long p = *ppos;
> +	u8 *buffer, *dst;
> +	u8 __iomem *src;
> +	int c, cnt = 0, err = 0;
> +	unsigned long total_size;
> +
> +	if (!info->screen_base)
> +		return -ENODEV;
> +
> +	total_size = info->screen_size;
> +
> +	if (total_size == 0)
> +		total_size = info->fix.smem_len;
> +
> +	if (p >= total_size)
> +		return 0;
> +
> +	if (count >= total_size)
> +		count = total_size;
> +
> +	if (count + p > total_size)
> +		count = total_size - p;
> +
> +	buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count, GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	src = (u8 __iomem *)(info->screen_base + p);
> +
> +	if (info->fbops->fb_sync)
> +		info->fbops->fb_sync(info);
> +
> +	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)) {
> +			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)
> +{
> +	unsigned long p = *ppos;
> +	u8 *buffer, *src;
> +	u8 __iomem *dst;
> +	int c, cnt = 0, err = 0;
> +	unsigned long total_size;
> +
> +	if (!info->screen_base)
> +		return -ENODEV;
> +
> +	total_size = info->screen_size;
> +
> +	if (total_size == 0)
> +		total_size = info->fix.smem_len;
> +
> +	if (p > total_size)
> +		return -EFBIG;
> +
> +	if (count > total_size) {
> +		err = -EFBIG;
> +		count = total_size;
> +	}
> +
> +	if (count + p > total_size) {
> +		if (!err)
> +			err = -ENOSPC;
> +
> +		count = total_size - p;
> +	}
> +
> +	buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count, GFP_KERNEL);
> +	if (!buffer)
> +		return -ENOMEM;
> +
> +	dst = (u8 __iomem *)(info->screen_base + p);
> +
> +	if (info->fbops->fb_sync)
> +		info->fbops->fb_sync(info);
> +
> +	while (count) {
> +		c = (count > PAGE_SIZE) ? PAGE_SIZE : count;
> +		src = buffer;
> +
> +		if (copy_from_user(src, buf, c)) {
> +			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;
> +}
> +EXPORT_SYMBOL(fb_cfb_write);
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index b993bb97058f..be6c75f3dfd0 100644
> --- a/drivers/video/fbdev/core/fbmem.c
> +++ b/drivers/video/fbdev/core/fbmem.c
> @@ -761,12 +761,7 @@ static struct fb_info *file_fb_info(struct file *file)
>   static ssize_t
>   fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>   {
> -	unsigned long p = *ppos;
>   	struct fb_info *info = file_fb_info(file);
> -	u8 *buffer, *dst;
> -	u8 __iomem *src;
> -	int c, cnt = 0, err = 0;
> -	unsigned long total_size;
>   
>   	if (!info)
>   		return -ENODEV;
> @@ -777,64 +772,13 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>   	if (info->fbops->fb_read)
>   		return info->fbops->fb_read(info, buf, count, ppos);
>   
> -	if (!info->screen_base)
> -		return -ENODEV;
> -
> -	total_size = info->screen_size;
> -
> -	if (total_size == 0)
> -		total_size = info->fix.smem_len;
> -
> -	if (p >= total_size)
> -		return 0;
> -
> -	if (count >= total_size)
> -		count = total_size;
> -
> -	if (count + p > total_size)
> -		count = total_size - p;
> -
> -	buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
> -			 GFP_KERNEL);
> -	if (!buffer)
> -		return -ENOMEM;
> -
> -	src = (u8 __iomem *) (info->screen_base + p);
> -
> -	if (info->fbops->fb_sync)
> -		info->fbops->fb_sync(info);
> -
> -	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)) {
> -			err = -EFAULT;
> -			break;
> -		}
> -		*ppos += c;
> -		buf += c;
> -		cnt += c;
> -		count -= c;
> -	}
> -
> -	kfree(buffer);
> -
> -	return cnt ? cnt : err;
> +	return fb_cfb_read(info, buf, count, ppos);
>   }
>   
>   static ssize_t
>   fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
>   {
> -	unsigned long p = *ppos;
>   	struct fb_info *info = file_fb_info(file);
> -	u8 *buffer, *src;
> -	u8 __iomem *dst;
> -	int c, cnt = 0, err = 0;
> -	unsigned long total_size;
>   
>   	if (!info)
>   		return -ENODEV;
> @@ -845,60 +789,7 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
>   	if (info->fbops->fb_write)
>   		return info->fbops->fb_write(info, buf, count, ppos);
>   
> -	if (!info->screen_base)
> -		return -ENODEV;
> -
> -	total_size = info->screen_size;
> -
> -	if (total_size == 0)
> -		total_size = info->fix.smem_len;
> -
> -	if (p > total_size)
> -		return -EFBIG;
> -
> -	if (count > total_size) {
> -		err = -EFBIG;
> -		count = total_size;
> -	}
> -
> -	if (count + p > total_size) {
> -		if (!err)
> -			err = -ENOSPC;
> -
> -		count = total_size - p;
> -	}
> -
> -	buffer = kmalloc((count > PAGE_SIZE) ? PAGE_SIZE : count,
> -			 GFP_KERNEL);
> -	if (!buffer)
> -		return -ENOMEM;
> -
> -	dst = (u8 __iomem *) (info->screen_base + p);
> -
> -	if (info->fbops->fb_sync)
> -		info->fbops->fb_sync(info);
> -
> -	while (count) {
> -		c = (count > PAGE_SIZE) ? PAGE_SIZE : count;
> -		src = buffer;
> -
> -		if (copy_from_user(src, buf, c)) {
> -			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;
> +	return fb_cfb_write(info, buf, count, ppos);
>   }
>   
>   int
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index 08cb47da71f8..3b1644c79973 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -576,9 +576,19 @@ struct fb_info {
>   extern int fb_set_var(struct fb_info *info, struct fb_var_screeninfo *var);
>   extern int fb_pan_display(struct fb_info *info, struct fb_var_screeninfo *var);
>   extern int fb_blank(struct fb_info *info, int blank);
> +
> +/*
> + * Drawing operations where framebuffer is in video RAM
> + */
> +
>   extern void cfb_fillrect(struct fb_info *info, const struct fb_fillrect *rect);
>   extern void cfb_copyarea(struct fb_info *info, const struct fb_copyarea *area);
>   extern void cfb_imageblit(struct fb_info *info, const struct fb_image *image);
> +extern ssize_t fb_cfb_read(struct fb_info *info, char __user *buf, size_t count,
> +			   loff_t *ppos);
> +extern ssize_t fb_cfb_write(struct fb_info *info, const char __user *buf,
> +			    size_t count, loff_t *ppos);
> +
>   /*
>    * Drawing operations where framebuffer is in system RAM
>    */

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

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

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

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

[Thomas Zimmermann]

[-- 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 --]

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

[Geert Uytterhoeven]

Hi Thomas,

On Wed, Apr 26, 2023 at 5:06 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 26.04.23 um 17:01 schrieb Geert Uytterhoeven:
> > 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_()?

Given fb_memcpy_fromfb() is mapped to memcpy_fromio() on
most architectures, I'd go for fb_io_*().

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

That's because your drawing helpers operate (only) on chunky color
frame buffer formats ;-)

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

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

[Thomas Zimmermann]

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

Hi

Am 26.04.23 um 17:21 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> On Wed, Apr 26, 2023 at 5:06 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 26.04.23 um 17:01 schrieb Geert Uytterhoeven:
>>> 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_()?
> 
> Given fb_memcpy_fromfb() is mapped to memcpy_fromio() on
> most architectures, I'd go for fb_io_*().

Ok, makes sense.

> 
>> CFB is used by the drawing helpers, which are usually used together with
>> this code. Hence the current naming.
> 
> That's because your drawing helpers operate (only) on chunky color
> frame buffer formats ;-)

Should we rename the CFB drawing functions to fb_io_ then? AFAICT they 
are the same algorithms as in the fb_sys_ functions; just with I/O memory.

Best regards
Thomas

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

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

[Geert Uytterhoeven]

Hi Thomas,

On Fri, Apr 28, 2023 at 1:20 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 26.04.23 um 17:21 schrieb Geert Uytterhoeven:
> > On Wed, Apr 26, 2023 at 5:06 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> >> Am 26.04.23 um 17:01 schrieb Geert Uytterhoeven:
> >>> 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_()?
> >
> > Given fb_memcpy_fromfb() is mapped to memcpy_fromio() on
> > most architectures, I'd go for fb_io_*().
>
> Ok, makes sense.
>
> >> CFB is used by the drawing helpers, which are usually used together with
> >> this code. Hence the current naming.
> >
> > That's because your drawing helpers operate (only) on chunky color
> > frame buffer formats ;-)
>
> Should we rename the CFB drawing functions to fb_io_ then? AFAICT they
> are the same algorithms as in the fb_sys_ functions; just with I/O memory.

I don't know if that's worth the churn.
Historically, the frame buffer was usually located in dedicated memory,
hence the drawing operations operated on I/O memory.
With the advent of unified memory architectures, the fb_sys_*()
functions were introduced.

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

[PATCH 6/6] drm/fb-helper: Use fb_{cfb,sys}_{read, write}()

[Thomas Zimmermann]

Implement DRM fbdev helpers for reading and writing framebuffer
memory with the respective fbdev functions. Removes duplicate
code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/gpu/drm/drm_fb_helper.c | 174 +-------------------------------
 1 file changed, 4 insertions(+), 170 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index 6bb1b8b27d7a..e11858470fa7 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -714,95 +714,6 @@ void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagerefli
 }
 EXPORT_SYMBOL(drm_fb_helper_deferred_io);
 
-typedef ssize_t (*drm_fb_helper_read_screen)(struct fb_info *info, char __user *buf,
-					     size_t count, loff_t pos);
-
-static ssize_t __drm_fb_helper_read(struct fb_info *info, char __user *buf, size_t count,
-				    loff_t *ppos, drm_fb_helper_read_screen read_screen)
-{
-	loff_t pos = *ppos;
-	size_t total_size;
-	ssize_t ret;
-
-	if (info->screen_size)
-		total_size = info->screen_size;
-	else
-		total_size = info->fix.smem_len;
-
-	if (pos >= total_size)
-		return 0;
-	if (count >= total_size)
-		count = total_size;
-	if (total_size - count < pos)
-		count = total_size - pos;
-
-	if (info->fbops->fb_sync)
-		info->fbops->fb_sync(info);
-
-	ret = read_screen(info, buf, count, pos);
-	if (ret > 0)
-		*ppos += ret;
-
-	return ret;
-}
-
-typedef ssize_t (*drm_fb_helper_write_screen)(struct fb_info *info, const char __user *buf,
-					      size_t count, loff_t pos);
-
-static ssize_t __drm_fb_helper_write(struct fb_info *info, const char __user *buf, size_t count,
-				     loff_t *ppos, drm_fb_helper_write_screen write_screen)
-{
-	loff_t pos = *ppos;
-	size_t total_size;
-	ssize_t ret;
-	int err = 0;
-
-	if (info->screen_size)
-		total_size = info->screen_size;
-	else
-		total_size = info->fix.smem_len;
-
-	if (pos > total_size)
-		return -EFBIG;
-	if (count > total_size) {
-		err = -EFBIG;
-		count = total_size;
-	}
-	if (total_size - count < pos) {
-		if (!err)
-			err = -ENOSPC;
-		count = total_size - pos;
-	}
-
-	if (info->fbops->fb_sync)
-		info->fbops->fb_sync(info);
-
-	/*
-	 * Copy to framebuffer even if we already logged an error. Emulates
-	 * the behavior of the original fbdev implementation.
-	 */
-	ret = write_screen(info, buf, count, pos);
-	if (ret < 0)
-		return ret; /* return last error, if any */
-	else if (!ret)
-		return err; /* return previous error, if any */
-
-	*ppos += ret;
-
-	return ret;
-}
-
-static ssize_t drm_fb_helper_read_screen_buffer(struct fb_info *info, char __user *buf,
-						size_t count, loff_t pos)
-{
-	const char *src = info->screen_buffer + pos;
-
-	if (copy_to_user(buf, src, count))
-		return -EFAULT;
-
-	return count;
-}
-
 /**
  * drm_fb_helper_sys_read - Implements struct &fb_ops.fb_read for system memory
  * @info: fb_info struct pointer
@@ -816,21 +727,10 @@ static ssize_t drm_fb_helper_read_screen_buffer(struct fb_info *info, char __use
 ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
 			       size_t count, loff_t *ppos)
 {
-	return __drm_fb_helper_read(info, buf, count, ppos, drm_fb_helper_read_screen_buffer);
+	return fb_sys_read(info, buf, count, ppos);
 }
 EXPORT_SYMBOL(drm_fb_helper_sys_read);
 
-static ssize_t drm_fb_helper_write_screen_buffer(struct fb_info *info, const char __user *buf,
-						 size_t count, loff_t pos)
-{
-	char *dst = info->screen_buffer + pos;
-
-	if (copy_from_user(dst, buf, count))
-		return -EFAULT;
-
-	return count;
-}
-
 /**
  * drm_fb_helper_sys_write - Implements struct &fb_ops.fb_write for system memory
  * @info: fb_info struct pointer
@@ -849,7 +749,7 @@ ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
 	ssize_t ret;
 	struct drm_rect damage_area;
 
-	ret = __drm_fb_helper_write(info, buf, count, ppos, drm_fb_helper_write_screen_buffer);
+	ret = fb_sys_write(info, buf, count, ppos);
 	if (ret <= 0)
 		return ret;
 
@@ -921,39 +821,6 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info,
 }
 EXPORT_SYMBOL(drm_fb_helper_sys_imageblit);
 
-static ssize_t fb_read_screen_base(struct fb_info *info, char __user *buf, size_t count,
-				   loff_t pos)
-{
-	const char __iomem *src = info->screen_base + pos;
-	size_t alloc_size = min_t(size_t, count, PAGE_SIZE);
-	ssize_t ret = 0;
-	int err = 0;
-	char *tmp;
-
-	tmp = kmalloc(alloc_size, GFP_KERNEL);
-	if (!tmp)
-		return -ENOMEM;
-
-	while (count) {
-		size_t c = min_t(size_t, count, alloc_size);
-
-		memcpy_fromio(tmp, src, c);
-		if (copy_to_user(buf, tmp, c)) {
-			err = -EFAULT;
-			break;
-		}
-
-		src += c;
-		buf += c;
-		ret += c;
-		count -= c;
-	}
-
-	kfree(tmp);
-
-	return ret ? ret : err;
-}
-
 /**
  * drm_fb_helper_cfb_read - Implements struct &fb_ops.fb_read for I/O memory
  * @info: fb_info struct pointer
@@ -967,43 +834,10 @@ static ssize_t fb_read_screen_base(struct fb_info *info, char __user *buf, size_
 ssize_t drm_fb_helper_cfb_read(struct fb_info *info, char __user *buf,
 			       size_t count, loff_t *ppos)
 {
-	return __drm_fb_helper_read(info, buf, count, ppos, fb_read_screen_base);
+	return fb_cfb_read(info, buf, count, ppos);
 }
 EXPORT_SYMBOL(drm_fb_helper_cfb_read);
 
-static ssize_t fb_write_screen_base(struct fb_info *info, const char __user *buf, size_t count,
-				    loff_t pos)
-{
-	char __iomem *dst = info->screen_base + pos;
-	size_t alloc_size = min_t(size_t, count, PAGE_SIZE);
-	ssize_t ret = 0;
-	int err = 0;
-	u8 *tmp;
-
-	tmp = kmalloc(alloc_size, GFP_KERNEL);
-	if (!tmp)
-		return -ENOMEM;
-
-	while (count) {
-		size_t c = min_t(size_t, count, alloc_size);
-
-		if (copy_from_user(tmp, buf, c)) {
-			err = -EFAULT;
-			break;
-		}
-		memcpy_toio(dst, tmp, c);
-
-		dst += c;
-		buf += c;
-		ret += c;
-		count -= c;
-	}
-
-	kfree(tmp);
-
-	return ret ? ret : err;
-}
-
 /**
  * drm_fb_helper_cfb_write - Implements struct &fb_ops.fb_write for I/O memory
  * @info: fb_info struct pointer
@@ -1022,7 +856,7 @@ ssize_t drm_fb_helper_cfb_write(struct fb_info *info, const char __user *buf,
 	ssize_t ret;
 	struct drm_rect damage_area;
 
-	ret = __drm_fb_helper_write(info, buf, count, ppos, fb_write_screen_base);
+	ret = fb_cfb_write(info, buf, count, ppos);
 	if (ret <= 0)
 		return ret;
 
-- 
2.40.0

Re: [PATCH 6/6] drm/fb-helper: Use fb_{cfb,sys}_{read, write}()

[Javier Martinez Canillas]

Thomas Zimmermann <tzimmermann@suse.de> writes:

> Implement DRM fbdev helpers for reading and writing framebuffer
> memory with the respective fbdev functions. Removes duplicate
> code.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> ---
>  drivers/gpu/drm/drm_fb_helper.c | 174 +-------------------------------
>  1 file changed, 4 insertions(+), 170 deletions(-)
>

Very nice cleanup!

Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [6/6] drm/fb-helper: Use fb_{cfb,sys}_{read, write}()

[Sui Jingfeng]

Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>

On 2023/4/25 22:28, Thomas Zimmermann wrote:
> Implement DRM fbdev helpers for reading and writing framebuffer
> memory with the respective fbdev functions. Removes duplicate
> code.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>   drivers/gpu/drm/drm_fb_helper.c | 174 +-------------------------------
>   1 file changed, 4 insertions(+), 170 deletions(-)
>
> diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
> index 6bb1b8b27d7a..e11858470fa7 100644
> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c
> @@ -714,95 +714,6 @@ void drm_fb_helper_deferred_io(struct fb_info *info, struct list_head *pagerefli
>   }
>   EXPORT_SYMBOL(drm_fb_helper_deferred_io);
>   
> -typedef ssize_t (*drm_fb_helper_read_screen)(struct fb_info *info, char __user *buf,
> -					     size_t count, loff_t pos);
> -
> -static ssize_t __drm_fb_helper_read(struct fb_info *info, char __user *buf, size_t count,
> -				    loff_t *ppos, drm_fb_helper_read_screen read_screen)
> -{
> -	loff_t pos = *ppos;
> -	size_t total_size;
> -	ssize_t ret;
> -
> -	if (info->screen_size)
> -		total_size = info->screen_size;
> -	else
> -		total_size = info->fix.smem_len;
> -
> -	if (pos >= total_size)
> -		return 0;
> -	if (count >= total_size)
> -		count = total_size;
> -	if (total_size - count < pos)
> -		count = total_size - pos;
> -
> -	if (info->fbops->fb_sync)
> -		info->fbops->fb_sync(info);
> -
> -	ret = read_screen(info, buf, count, pos);
> -	if (ret > 0)
> -		*ppos += ret;
> -
> -	return ret;
> -}
> -
> -typedef ssize_t (*drm_fb_helper_write_screen)(struct fb_info *info, const char __user *buf,
> -					      size_t count, loff_t pos);
> -
> -static ssize_t __drm_fb_helper_write(struct fb_info *info, const char __user *buf, size_t count,
> -				     loff_t *ppos, drm_fb_helper_write_screen write_screen)
> -{
> -	loff_t pos = *ppos;
> -	size_t total_size;
> -	ssize_t ret;
> -	int err = 0;
> -
> -	if (info->screen_size)
> -		total_size = info->screen_size;
> -	else
> -		total_size = info->fix.smem_len;
> -
> -	if (pos > total_size)
> -		return -EFBIG;
> -	if (count > total_size) {
> -		err = -EFBIG;
> -		count = total_size;
> -	}
> -	if (total_size - count < pos) {
> -		if (!err)
> -			err = -ENOSPC;
> -		count = total_size - pos;
> -	}
> -
> -	if (info->fbops->fb_sync)
> -		info->fbops->fb_sync(info);
> -
> -	/*
> -	 * Copy to framebuffer even if we already logged an error. Emulates
> -	 * the behavior of the original fbdev implementation.
> -	 */
> -	ret = write_screen(info, buf, count, pos);
> -	if (ret < 0)
> -		return ret; /* return last error, if any */
> -	else if (!ret)
> -		return err; /* return previous error, if any */
> -
> -	*ppos += ret;
> -
> -	return ret;
> -}
> -
> -static ssize_t drm_fb_helper_read_screen_buffer(struct fb_info *info, char __user *buf,
> -						size_t count, loff_t pos)
> -{
> -	const char *src = info->screen_buffer + pos;
> -
> -	if (copy_to_user(buf, src, count))
> -		return -EFAULT;
> -
> -	return count;
> -}
> -
>   /**
>    * drm_fb_helper_sys_read - Implements struct &fb_ops.fb_read for system memory
>    * @info: fb_info struct pointer
> @@ -816,21 +727,10 @@ static ssize_t drm_fb_helper_read_screen_buffer(struct fb_info *info, char __use
>   ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
>   			       size_t count, loff_t *ppos)
>   {
> -	return __drm_fb_helper_read(info, buf, count, ppos, drm_fb_helper_read_screen_buffer);
> +	return fb_sys_read(info, buf, count, ppos);
>   }
>   EXPORT_SYMBOL(drm_fb_helper_sys_read);
>   
> -static ssize_t drm_fb_helper_write_screen_buffer(struct fb_info *info, const char __user *buf,
> -						 size_t count, loff_t pos)
> -{
> -	char *dst = info->screen_buffer + pos;
> -
> -	if (copy_from_user(dst, buf, count))
> -		return -EFAULT;
> -
> -	return count;
> -}
> -
>   /**
>    * drm_fb_helper_sys_write - Implements struct &fb_ops.fb_write for system memory
>    * @info: fb_info struct pointer
> @@ -849,7 +749,7 @@ ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
>   	ssize_t ret;
>   	struct drm_rect damage_area;
>   
> -	ret = __drm_fb_helper_write(info, buf, count, ppos, drm_fb_helper_write_screen_buffer);
> +	ret = fb_sys_write(info, buf, count, ppos);
>   	if (ret <= 0)
>   		return ret;
>   
> @@ -921,39 +821,6 @@ void drm_fb_helper_sys_imageblit(struct fb_info *info,
>   }
>   EXPORT_SYMBOL(drm_fb_helper_sys_imageblit);
>   
> -static ssize_t fb_read_screen_base(struct fb_info *info, char __user *buf, size_t count,
> -				   loff_t pos)
> -{
> -	const char __iomem *src = info->screen_base + pos;
> -	size_t alloc_size = min_t(size_t, count, PAGE_SIZE);
> -	ssize_t ret = 0;
> -	int err = 0;
> -	char *tmp;
> -
> -	tmp = kmalloc(alloc_size, GFP_KERNEL);
> -	if (!tmp)
> -		return -ENOMEM;
> -
> -	while (count) {
> -		size_t c = min_t(size_t, count, alloc_size);
> -
> -		memcpy_fromio(tmp, src, c);
> -		if (copy_to_user(buf, tmp, c)) {
> -			err = -EFAULT;
> -			break;
> -		}
> -
> -		src += c;
> -		buf += c;
> -		ret += c;
> -		count -= c;
> -	}
> -
> -	kfree(tmp);
> -
> -	return ret ? ret : err;
> -}
> -
>   /**
>    * drm_fb_helper_cfb_read - Implements struct &fb_ops.fb_read for I/O memory
>    * @info: fb_info struct pointer
> @@ -967,43 +834,10 @@ static ssize_t fb_read_screen_base(struct fb_info *info, char __user *buf, size_
>   ssize_t drm_fb_helper_cfb_read(struct fb_info *info, char __user *buf,
>   			       size_t count, loff_t *ppos)
>   {
> -	return __drm_fb_helper_read(info, buf, count, ppos, fb_read_screen_base);
> +	return fb_cfb_read(info, buf, count, ppos);
>   }
>   EXPORT_SYMBOL(drm_fb_helper_cfb_read);
>   
> -static ssize_t fb_write_screen_base(struct fb_info *info, const char __user *buf, size_t count,
> -				    loff_t pos)
> -{
> -	char __iomem *dst = info->screen_base + pos;
> -	size_t alloc_size = min_t(size_t, count, PAGE_SIZE);
> -	ssize_t ret = 0;
> -	int err = 0;
> -	u8 *tmp;
> -
> -	tmp = kmalloc(alloc_size, GFP_KERNEL);
> -	if (!tmp)
> -		return -ENOMEM;
> -
> -	while (count) {
> -		size_t c = min_t(size_t, count, alloc_size);
> -
> -		if (copy_from_user(tmp, buf, c)) {
> -			err = -EFAULT;
> -			break;
> -		}
> -		memcpy_toio(dst, tmp, c);
> -
> -		dst += c;
> -		buf += c;
> -		ret += c;
> -		count -= c;
> -	}
> -
> -	kfree(tmp);
> -
> -	return ret ? ret : err;
> -}
> -
>   /**
>    * drm_fb_helper_cfb_write - Implements struct &fb_ops.fb_write for I/O memory
>    * @info: fb_info struct pointer
> @@ -1022,7 +856,7 @@ ssize_t drm_fb_helper_cfb_write(struct fb_info *info, const char __user *buf,
>   	ssize_t ret;
>   	struct drm_rect damage_area;
>   
> -	ret = __drm_fb_helper_write(info, buf, count, ppos, fb_write_screen_base);
> +	ret = fb_cfb_write(info, buf, count, ppos);
>   	if (ret <= 0)
>   		return ret;
>   

Re: [PATCH 6/6] drm/fb-helper: Use fb_{cfb,sys}_{read, write}()

[Geert Uytterhoeven]

Hi Thomas,

On Tue, Apr 25, 2023 at 4:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Implement DRM fbdev helpers for reading and writing framebuffer
> memory with the respective fbdev functions. Removes duplicate
> code.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>

Thanks for your patch!

> --- a/drivers/gpu/drm/drm_fb_helper.c
> +++ b/drivers/gpu/drm/drm_fb_helper.c

> @@ -816,21 +727,10 @@ static ssize_t drm_fb_helper_read_screen_buffer(struct fb_info *info, char __use
>  ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
>                                size_t count, loff_t *ppos)
>  {
> -       return __drm_fb_helper_read(info, buf, count, ppos, drm_fb_helper_read_screen_buffer);
> +       return fb_sys_read(info, buf, count, ppos);
>  }
>  EXPORT_SYMBOL(drm_fb_helper_sys_read);

I guess drm_fb_helper_sys_read() can just be removed?

> @@ -849,7 +749,7 @@ ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
>         ssize_t ret;
>         struct drm_rect damage_area;
>
> -       ret = __drm_fb_helper_write(info, buf, count, ppos, drm_fb_helper_write_screen_buffer);
> +       ret = fb_sys_write(info, buf, count, ppos);
>         if (ret <= 0)
>                 return ret;
>

drm_fb_helper_sys_write() cannot be removed yet, because it does damage
handling below.  If the fb_ops.fb_sync() callback would be enhanced to pass
a region, drm_fb_helper could implement .fb_sync() instead of .fb_write().

Likewise for the "cfb" (which is a misnomer) variants below.

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

Re: [PATCH 6/6] drm/fb-helper: Use fb_{cfb,sys}_{read, write}()

[Thomas Zimmermann]

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

Hi

Am 26.04.23 um 17:15 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> On Tue, Apr 25, 2023 at 4:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Implement DRM fbdev helpers for reading and writing framebuffer
>> memory with the respective fbdev functions. Removes duplicate
>> code.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> 
> Thanks for your patch!
> 
>> --- a/drivers/gpu/drm/drm_fb_helper.c
>> +++ b/drivers/gpu/drm/drm_fb_helper.c
> 
>> @@ -816,21 +727,10 @@ static ssize_t drm_fb_helper_read_screen_buffer(struct fb_info *info, char __use
>>   ssize_t drm_fb_helper_sys_read(struct fb_info *info, char __user *buf,
>>                                 size_t count, loff_t *ppos)
>>   {
>> -       return __drm_fb_helper_read(info, buf, count, ppos, drm_fb_helper_read_screen_buffer);
>> +       return fb_sys_read(info, buf, count, ppos);
>>   }
>>   EXPORT_SYMBOL(drm_fb_helper_sys_read);
> 
> I guess drm_fb_helper_sys_read() can just be removed?

Yes, soon.

> 
>> @@ -849,7 +749,7 @@ ssize_t drm_fb_helper_sys_write(struct fb_info *info, const char __user *buf,
>>          ssize_t ret;
>>          struct drm_rect damage_area;
>>
>> -       ret = __drm_fb_helper_write(info, buf, count, ppos, drm_fb_helper_write_screen_buffer);
>> +       ret = fb_sys_write(info, buf, count, ppos);
>>          if (ret <= 0)
>>                  return ret;
>>
> 
> drm_fb_helper_sys_write() cannot be removed yet, because it does damage
> handling below.  If the fb_ops.fb_sync() callback would be enhanced to pass
> a region, drm_fb_helper could implement .fb_sync() instead of .fb_write().

Most DRM's fbdev support can soon use regular fbdev helpers from this 
patchset. Only DRM's i915 and the generic fbdev need damage handling. 
But they are both special in their own way, so each will get its own 
implementation. I have prototype patches to make this happen.

Best regards
Thomas

> 
> Likewise for the "cfb" (which is a misnomer) variants below.
> 
> 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 --]

Re: [PATCH 0/6] drm,fbdev: Use fbdev's I/O helpers

[Helge Deller]

On 4/25/23 16:28, Thomas Zimmermann wrote:
> Make fbdev's built-in helpers for reading and writing I/O and system
> memory available to DRM. Replace DRM's internal helpers.
>
> The first patch resolves a bug that's been in the fbdev code for
> more than 15 years. Makes the read/write helpers work successfully
> with the IGT tests.
>
> Patches 2 to 4 streamline fbdev's file-I/O code and remove a few
> duplicate checks.
>
> Patch 5 moves the default-I/O code into the new helpers fb_cfb_read()
> and fb_cfb_write(); patch 6 uses them in DRM. This allows us to remove
> quite a bit of code from DRM's internal fbdev helpers.
>
> Tested with i915 and simpledrm.
>
> The next step here is to remove the drm_fb_helper_{cfb,sys}_*()
> entirely. They where mostly introduced because fbdev doesn't protect
> it's public interfaces with an CONFIG_FB preprocessor guards. But all
> of DRM driver's fbdev emulation won't be build without CONFIG_FB, so
> this is not an issue in practice. Removing the DRM wrappers will
> further simplify the DRM code.

This series does a very nice cleanup!

You may add:
Acked-by: Helge Deller <deller@gmx.de>

Thanks!
Helge

>
> Thomas Zimmermann (6):
>    fbdev: Return number of bytes read or written
>    fbdev: Use screen_buffer in fb_sys_{read,write}()
>    fbdev: Don't re-validate info->state in fb_ops implementations
>    fbdev: Validate info->screen_{base,buffer} in fb_ops implementations
>    fbdev: Move CFB read and write code into helper functions
>    drm/fb-helper: Use fb_{cfb,sys}_{read, write}()
>
>   drivers/gpu/drm/drm_fb_helper.c        | 174 +------------------------
>   drivers/video/fbdev/cobalt_lcdfb.c     |   6 +
>   drivers/video/fbdev/core/Makefile      |   2 +-
>   drivers/video/fbdev/core/fb_cfb_fops.c | 126 ++++++++++++++++++
>   drivers/video/fbdev/core/fb_sys_fops.c |  36 ++---
>   drivers/video/fbdev/core/fbmem.c       | 111 +---------------
>   drivers/video/fbdev/sm712fb.c          |  10 +-
>   include/linux/fb.h                     |  10 ++
>   8 files changed, 173 insertions(+), 302 deletions(-)
>   create mode 100644 drivers/video/fbdev/core/fb_cfb_fops.c
>

Re: [PATCH 0/6] drm,fbdev: Use fbdev's I/O helpers

[Sui Jingfeng]

Hi,


The whole patch set is tested with fbdev of IGT, on LoongArch with
drm/radeon and efifb driver. Test results say SUCCESS.


On 2023/4/25 22:28, Thomas Zimmermann wrote:
> Make fbdev's built-in helpers for reading and writing I/O and system
> memory available to DRM. Replace DRM's internal helpers.
>
> The first patch resolves a bug that's been in the fbdev code for
> more than 15 years. Makes the read/write helpers work successfully
> with the IGT tests.
>
> Patches 2 to 4 streamline fbdev's file-I/O code and remove a few
> duplicate checks.
>
> Patch 5 moves the default-I/O code into the new helpers fb_cfb_read()
> and fb_cfb_write(); patch 6 uses them in DRM. This allows us to remove
> quite a bit of code from DRM's internal fbdev helpers.
>
> Tested with i915 and simpledrm.
>
> The next step here is to remove the drm_fb_helper_{cfb,sys}_*()
> entirely. They where mostly introduced because fbdev doesn't protect
> it's public interfaces with an CONFIG_FB preprocessor guards. But all
> of DRM driver's fbdev emulation won't be build without CONFIG_FB, so
> this is not an issue in practice. Removing the DRM wrappers will
> further simplify the DRM code.
>
> Thomas Zimmermann (6):
>    fbdev: Return number of bytes read or written
>    fbdev: Use screen_buffer in fb_sys_{read,write}()
>    fbdev: Don't re-validate info->state in fb_ops implementations
>    fbdev: Validate info->screen_{base,buffer} in fb_ops implementations
>    fbdev: Move CFB read and write code into helper functions
>    drm/fb-helper: Use fb_{cfb,sys}_{read, write}()
>
>   drivers/gpu/drm/drm_fb_helper.c        | 174 +------------------------
>   drivers/video/fbdev/cobalt_lcdfb.c     |   6 +
>   drivers/video/fbdev/core/Makefile      |   2 +-
>   drivers/video/fbdev/core/fb_cfb_fops.c | 126 ++++++++++++++++++
>   drivers/video/fbdev/core/fb_sys_fops.c |  36 ++---
>   drivers/video/fbdev/core/fbmem.c       | 111 +---------------
>   drivers/video/fbdev/sm712fb.c          |  10 +-
>   include/linux/fb.h                     |  10 ++
>   8 files changed, 173 insertions(+), 302 deletions(-)
>   create mode 100644 drivers/video/fbdev/core/fb_cfb_fops.c
>

Re: [PATCH 0/6] drm,fbdev: Use fbdev's I/O helpers

[Thomas Zimmermann]

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



Am 26.04.23 um 12:33 schrieb Sui Jingfeng:
> Hi,
> 
> 
> The whole patch set is tested with fbdev of IGT, on LoongArch with
> drm/radeon and efifb driver. Test results say SUCCESS.

Thanks a lot.

> 
> 
> On 2023/4/25 22:28, Thomas Zimmermann wrote:
>> Make fbdev's built-in helpers for reading and writing I/O and system
>> memory available to DRM. Replace DRM's internal helpers.
>>
>> The first patch resolves a bug that's been in the fbdev code for
>> more than 15 years. Makes the read/write helpers work successfully
>> with the IGT tests.
>>
>> Patches 2 to 4 streamline fbdev's file-I/O code and remove a few
>> duplicate checks.
>>
>> Patch 5 moves the default-I/O code into the new helpers fb_cfb_read()
>> and fb_cfb_write(); patch 6 uses them in DRM. This allows us to remove
>> quite a bit of code from DRM's internal fbdev helpers.
>>
>> Tested with i915 and simpledrm.
>>
>> The next step here is to remove the drm_fb_helper_{cfb,sys}_*()
>> entirely. They where mostly introduced because fbdev doesn't protect
>> it's public interfaces with an CONFIG_FB preprocessor guards. But all
>> of DRM driver's fbdev emulation won't be build without CONFIG_FB, so
>> this is not an issue in practice. Removing the DRM wrappers will
>> further simplify the DRM code.
>>
>> Thomas Zimmermann (6):
>>    fbdev: Return number of bytes read or written
>>    fbdev: Use screen_buffer in fb_sys_{read,write}()
>>    fbdev: Don't re-validate info->state in fb_ops implementations
>>    fbdev: Validate info->screen_{base,buffer} in fb_ops implementations
>>    fbdev: Move CFB read and write code into helper functions
>>    drm/fb-helper: Use fb_{cfb,sys}_{read, write}()
>>
>>   drivers/gpu/drm/drm_fb_helper.c        | 174 +------------------------
>>   drivers/video/fbdev/cobalt_lcdfb.c     |   6 +
>>   drivers/video/fbdev/core/Makefile      |   2 +-
>>   drivers/video/fbdev/core/fb_cfb_fops.c | 126 ++++++++++++++++++
>>   drivers/video/fbdev/core/fb_sys_fops.c |  36 ++---
>>   drivers/video/fbdev/core/fbmem.c       | 111 +---------------
>>   drivers/video/fbdev/sm712fb.c          |  10 +-
>>   include/linux/fb.h                     |  10 ++
>>   8 files changed, 173 insertions(+), 302 deletions(-)
>>   create mode 100644 drivers/video/fbdev/core/fb_cfb_fops.c
>>

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