[PATCH v2 00/19] drm,fbdev: Use fbdev's I/O helpers

[PATCH v2 00/19] 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 13 patches and patch 15 fix the use of screen_base and
screen_buffer. A number of drivers mix them up.

Patch 14 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 16 and 17 streamline fbdev's file-I/O code and remove a few
duplicate checks.

Patch 18 moves the default-I/O code into the new helpers fb_io_read()
and fb_io_write(); patch 19 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.

v2:
	* fix screen_base vs. screen_buffer usage
	* fix copy_{from,to}_user() return value (Geert)
	* use fb_io_() prefix (Geert)
	* improved commit messages

Thomas Zimmermann (19):
  auxdisplay/cfag12864bfb: Use struct fb_info.screen_buffer
  auxdisplay/ht16k33: Use struct fb_info.screen_buffer
  hid/hid-picolcd_fb: Use struct fb_info.screen_buffer
  fbdev/arcfb: Use struct fb_info.screen_buffer
  fbdev/au1200fb: Use struct fb_info.screen_buffer
  fbdev/broadsheetfb: Use struct fb_info.screen_buffer
  fbdev/hecubafb: Use struct fb_info.screen_buffer
  fbdev/metronomefb: Use struct fb_info.screen_buffer
  fbdev/ps3fb: Use struct fb_info.screen_buffer
  fbdev/smscufx: Use struct fb_info.screen_buffer
  fbdev/udlfb: Use struct fb_info.screen_buffer
  fbdev/vfb: Use struct fb_info.screen_buffer
  fbdev/xen-fbfront: Use struct fb_info.screen_buffer
  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 I/O read and write code into helper functions
  drm/fb-helper: Use fb_{cfb,sys}_{read, write}()

 drivers/auxdisplay/cfag12864bfb.c      |   2 +-
 drivers/auxdisplay/ht16k33.c           |   2 +-
 drivers/gpu/drm/drm_fb_helper.c        | 174 +------------------------
 drivers/hid/hid-picolcd_fb.c           |   4 +-
 drivers/media/pci/ivtv/ivtvfb.c        |   4 +-
 drivers/video/fbdev/arcfb.c            |  11 +-
 drivers/video/fbdev/au1200fb.c         |   2 +-
 drivers/video/fbdev/broadsheetfb.c     |  16 +--
 drivers/video/fbdev/cobalt_lcdfb.c     |   6 +
 drivers/video/fbdev/core/Makefile      |   2 +-
 drivers/video/fbdev/core/fb_io_fops.c  | 133 +++++++++++++++++++
 drivers/video/fbdev/core/fb_sys_fops.c |  36 ++---
 drivers/video/fbdev/core/fbmem.c       | 111 +---------------
 drivers/video/fbdev/hecubafb.c         |  12 +-
 drivers/video/fbdev/metronomefb.c      |  16 +--
 drivers/video/fbdev/ps3fb.c            |   4 +-
 drivers/video/fbdev/pvr2fb.c           |   3 +
 drivers/video/fbdev/sm712fb.c          |  10 +-
 drivers/video/fbdev/smscufx.c          |  14 +-
 drivers/video/fbdev/ssd1307fb.c        |   3 +
 drivers/video/fbdev/udlfb.c            |  12 +-
 drivers/video/fbdev/vfb.c              |   2 +-
 drivers/video/fbdev/xen-fbfront.c      |   2 +-
 include/linux/fb.h                     |  10 ++
 24 files changed, 239 insertions(+), 352 deletions(-)
 create mode 100644 drivers/video/fbdev/core/fb_io_fops.c

-- 
2.40.0

[PATCH v2 01/19] auxdisplay/cfag12864bfb: Use struct fb_info.screen_buffer

[Thomas Zimmermann]

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

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.

As the driver operates on the latter address space, it is wrong to use
.screen_base and .screen_buffer must be used instead. This also gets
rid of casting needed due to not using the correct data type.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/auxdisplay/cfag12864bfb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/auxdisplay/cfag12864bfb.c b/drivers/auxdisplay/cfag12864bfb.c
index 0df474506fb9..c2cab7e2b126 100644
--- a/drivers/auxdisplay/cfag12864bfb.c
+++ b/drivers/auxdisplay/cfag12864bfb.c
@@ -72,7 +72,7 @@ static int cfag12864bfb_probe(struct platform_device *device)
 	if (!info)
 		goto none;
 
-	info->screen_base = (char __iomem *) cfag12864b_buffer;
+	info->screen_buffer = cfag12864b_buffer;
 	info->screen_size = CFAG12864B_SIZE;
 	info->fbops = &cfag12864bfb_ops;
 	info->fix = cfag12864bfb_fix;
-- 
2.40.0

Re: [PATCH v2 01/19] auxdisplay/cfag12864bfb: Use struct fb_info.screen_buffer

[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.
>
> 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.
>
> As the driver operates on the latter address space, it is wrong to use
> .screen_base and .screen_buffer must be used instead. This also gets
> rid of casting needed due to 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

[PATCH v2 02/19] auxdisplay/ht16k33: Use struct fb_info.screen_buffer

[Thomas Zimmermann]

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

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.

As the driver operates on the latter address space, it is wrong to use
.screen_base and .screen_buffer must be used instead. This also gets
rid of casting needed due to not using the correct data type.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/auxdisplay/ht16k33.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/auxdisplay/ht16k33.c b/drivers/auxdisplay/ht16k33.c
index 02425991c159..edaf92b7ea77 100644
--- a/drivers/auxdisplay/ht16k33.c
+++ b/drivers/auxdisplay/ht16k33.c
@@ -640,7 +640,7 @@ static int ht16k33_fbdev_probe(struct device *dev, struct ht16k33_priv *priv,
 
 	INIT_DELAYED_WORK(&priv->work, ht16k33_fb_update);
 	fbdev->info->fbops = &ht16k33_fb_ops;
-	fbdev->info->screen_base = (char __iomem *) fbdev->buffer;
+	fbdev->info->screen_buffer = fbdev->buffer;
 	fbdev->info->screen_size = HT16K33_FB_SIZE;
 	fbdev->info->fix = ht16k33_fb_fix;
 	fbdev->info->var = ht16k33_fb_var;
-- 
2.40.0

Re: [PATCH v2 02/19] auxdisplay/ht16k33: Use struct fb_info.screen_buffer

[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.
>
> 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.
>
> As the driver operates on the latter address space, it is wrong to use
> .screen_base and .screen_buffer must be used instead. This also gets
> rid of casting needed due to 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

[PATCH v2 03/19] hid/hid-picolcd_fb: Use struct fb_info.screen_buffer

[Thomas Zimmermann]

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

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.

As the driver operates on the latter address space, it is wrong to use
.screen_base and .screen_buffer must be used instead. This also gets
rid of casting needed due to not using the correct data type.

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

diff --git a/drivers/hid/hid-picolcd_fb.c b/drivers/hid/hid-picolcd_fb.c
index de61c08fabea..dabcd054dad9 100644
--- a/drivers/hid/hid-picolcd_fb.c
+++ b/drivers/hid/hid-picolcd_fb.c
@@ -221,7 +221,7 @@ int picolcd_fb_reset(struct picolcd_data *data, int clear)
 	return 0;
 }
 
-/* Update fb_vbitmap from the screen_base and send changed tiles to device */
+/* Update fb_vbitmap from the screen_buffer and send changed tiles to device */
 static void picolcd_fb_update(struct fb_info *info)
 {
 	int chip, tile, n;
@@ -541,7 +541,7 @@ int picolcd_init_framebuffer(struct picolcd_data *data)
 		dev_err(dev, "can't get a free page for framebuffer\n");
 		goto err_nomem;
 	}
-	info->screen_base = (char __force __iomem *)fbdata->bitmap;
+	info->screen_buffer = fbdata->bitmap;
 	info->fix.smem_start = (unsigned long)fbdata->bitmap;
 	memset(fbdata->vbitmap, 0xff, PICOLCDFB_SIZE);
 	data->fb_info = info;
-- 
2.40.0

Re: [PATCH v2 03/19] hid/hid-picolcd_fb: Use struct fb_info.screen_buffer

[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.
>
> 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.
>
> As the driver operates on the latter address space, it is wrong to use
> .screen_base and .screen_buffer must be used instead. This also gets
> rid of casting needed due to 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

[PATCH v2 04/19] fbdev/arcfb: Use struct fb_info.screen_buffer

[Thomas Zimmermann]

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

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.

As the driver operates on the latter address space, it is wrong to use
.screen_base and .screen_buffer must be used instead. This also gets
rid of casting needed due to not using the correct data type.

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

diff --git a/drivers/video/fbdev/arcfb.c b/drivers/video/fbdev/arcfb.c
index 45e64016db32..088c4b30fd31 100644
--- a/drivers/video/fbdev/arcfb.c
+++ b/drivers/video/fbdev/arcfb.c
@@ -260,7 +260,7 @@ static void arcfb_lcd_update_page(struct arcfb_par *par, unsigned int upper,
 	ks108_set_yaddr(par, chipindex, upper/8);
 
 	linesize = par->info->var.xres/8;
-	src = (unsigned char __force *) par->info->screen_base + (left/8) +
+	src = (unsigned char *)par->info->screen_buffer + (left/8) +
 		(upper * linesize);
 	ks108_set_xaddr(par, chipindex, left);
 
@@ -468,7 +468,7 @@ static ssize_t arcfb_write(struct fb_info *info, const char __user *buf,
 	if (count) {
 		char *base_addr;
 
-		base_addr = (char __force *)info->screen_base;
+		base_addr = info->screen_buffer;
 		count -= copy_from_user(base_addr + p, buf, count);
 		*ppos += count;
 		err = -EFAULT;
@@ -525,7 +525,7 @@ static int arcfb_probe(struct platform_device *dev)
 	if (!info)
 		goto err;
 
-	info->screen_base = (char __iomem *)videomemory;
+	info->screen_buffer = videomemory;
 	info->fbops = &arcfb_ops;
 
 	info->var = arcfb_var;
@@ -595,7 +595,7 @@ static int arcfb_remove(struct platform_device *dev)
 		unregister_framebuffer(info);
 		if (irq)
 			free_irq(((struct arcfb_par *)(info->par))->irq, info);
-		vfree((void __force *)info->screen_base);
+		vfree(info->screen_buffer);
 		framebuffer_release(info);
 	}
 	return 0;
-- 
2.40.0

Re: [PATCH v2 04/19] fbdev/arcfb: Use struct fb_info.screen_buffer

[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.
>
> 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.
>
> As the driver operates on the latter address space, it is wrong to use
> .screen_base and .screen_buffer must be used instead. This also gets
> rid of casting needed due to 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

[PATCH v2 05/19] fbdev/au1200fb: Use struct fb_info.screen_buffer

[Thomas Zimmermann]

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

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.

As the driver operates on the latter address space, it is wrong to use
.screen_base and .screen_buffer must be used instead. This also gets
rid of casting needed due to not using the correct data type.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/au1200fb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/au1200fb.c b/drivers/video/fbdev/au1200fb.c
index b6b22fa4a8a0..847f82ed52ff 100644
--- a/drivers/video/fbdev/au1200fb.c
+++ b/drivers/video/fbdev/au1200fb.c
@@ -1568,7 +1568,7 @@ static int au1200fb_init_fbinfo(struct au1200fb_device *fbdev)
 	fbi->fix.mmio_len = 0;
 	fbi->fix.accel = FB_ACCEL_NONE;
 
-	fbi->screen_base = (char __iomem *) fbdev->fb_mem;
+	fbi->screen_buffer = fbdev->fb_mem;
 
 	au1200fb_update_fbinfo(fbi);
 
-- 
2.40.0

Re: [PATCH v2 05/19] fbdev/au1200fb: Use struct fb_info.screen_buffer

[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.
>
> 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.
>
> As the driver operates on the latter address space, it is wrong to use
> .screen_base and .screen_buffer must be used instead. This also gets
> rid of casting needed due to 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

[PATCH v2 06/19] fbdev/broadsheetfb: Use struct fb_info.screen_buffer

[Thomas Zimmermann]

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

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.

As the driver operates on the latter address space, it is wrong to use
.screen_base and .screen_buffer must be used instead. This also gets
rid of casting needed due to not using the correct data type.

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

diff --git a/drivers/video/fbdev/broadsheetfb.c b/drivers/video/fbdev/broadsheetfb.c
index 55e62dd96f9b..65dc86b7081e 100644
--- a/drivers/video/fbdev/broadsheetfb.c
+++ b/drivers/video/fbdev/broadsheetfb.c
@@ -824,7 +824,7 @@ static void broadsheet_init_display(struct broadsheetfb_par *par)
 
 	broadsheet_burst_write(par, (panel_table[par->panel_index].w *
 					panel_table[par->panel_index].h)/2,
-					(u16 *) par->info->screen_base);
+					(u16 *)par->info->screen_buffer);
 
 	broadsheet_send_command(par, BS_CMD_LD_IMG_END);
 
@@ -865,7 +865,7 @@ static void broadsheetfb_dpy_update_pages(struct broadsheetfb_par *par,
 						u16 y1, u16 y2)
 {
 	u16 args[5];
-	unsigned char *buf = (unsigned char *)par->info->screen_base;
+	unsigned char *buf = par->info->screen_buffer;
 
 	mutex_lock(&(par->io_lock));
 	/* y1 must be a multiple of 4 so drop the lower bits */
@@ -913,7 +913,7 @@ static void broadsheetfb_dpy_update(struct broadsheetfb_par *par)
 	broadsheet_send_cmdargs(par, BS_CMD_WR_REG, 1, args);
 	broadsheet_burst_write(par, (panel_table[par->panel_index].w *
 					panel_table[par->panel_index].h)/2,
-					(u16 *) par->info->screen_base);
+					(u16 *)par->info->screen_buffer);
 
 	broadsheet_send_command(par, BS_CMD_LD_IMG_END);
 
@@ -1033,7 +1033,7 @@ static ssize_t broadsheetfb_write(struct fb_info *info, const char __user *buf,
 		count = total_size - p;
 	}
 
-	dst = (void *)(info->screen_base + p);
+	dst = info->screen_buffer + p;
 
 	if (copy_from_user(dst, buf, count))
 		err = -EFAULT;
@@ -1109,7 +1109,7 @@ static int broadsheetfb_probe(struct platform_device *dev)
 	if (!videomemory)
 		goto err_fb_rel;
 
-	info->screen_base = (char *)videomemory;
+	info->screen_buffer = videomemory;
 	info->fbops = &broadsheetfb_ops;
 
 	broadsheetfb_var.xres = dpyw;
@@ -1205,7 +1205,7 @@ static int broadsheetfb_remove(struct platform_device *dev)
 		fb_deferred_io_cleanup(info);
 		par->board->cleanup(par);
 		fb_dealloc_cmap(&info->cmap);
-		vfree((void *)info->screen_base);
+		vfree(info->screen_buffer);
 		module_put(par->board->owner);
 		framebuffer_release(info);
 	}
-- 
2.40.0

Re: [PATCH v2 06/19] fbdev/broadsheetfb: Use struct fb_info.screen_buffer

[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.
>
> 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.
>
> As the driver operates on the latter address space, it is wrong to use
> .screen_base and .screen_buffer must be used instead. This also gets
> rid of casting needed due to 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

[PATCH v2 07/19] fbdev/hecubafb: Use struct fb_info.screen_buffer

[Thomas Zimmermann]

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

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.

As the driver operates on the latter address space, it is wrong to use
.screen_base and .screen_buffer must be used instead. This also gets
rid of casting needed due to not using the correct data type.

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

diff --git a/drivers/video/fbdev/hecubafb.c b/drivers/video/fbdev/hecubafb.c
index eb1eaadc1bbb..ddfa2853cc41 100644
--- a/drivers/video/fbdev/hecubafb.c
+++ b/drivers/video/fbdev/hecubafb.c
@@ -102,7 +102,7 @@ static void apollo_send_command(struct hecubafb_par *par, unsigned char data)
 static void hecubafb_dpy_update(struct hecubafb_par *par)
 {
 	int i;
-	unsigned char *buf = (unsigned char __force *)par->info->screen_base;
+	unsigned char *buf = par->info->screen_buffer;
 
 	apollo_send_command(par, APOLLO_START_NEW_IMG);
 
@@ -183,7 +183,7 @@ static ssize_t hecubafb_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 (copy_from_user(dst, buf, count))
 		err = -EFAULT;
@@ -239,7 +239,7 @@ static int hecubafb_probe(struct platform_device *dev)
 	if (!info)
 		goto err_fballoc;
 
-	info->screen_base = (char __force __iomem *)videomemory;
+	info->screen_buffer = videomemory;
 	info->fbops = &hecubafb_ops;
 
 	info->var = hecubafb_var;
@@ -287,7 +287,7 @@ static int hecubafb_remove(struct platform_device *dev)
 		struct hecubafb_par *par = info->par;
 		fb_deferred_io_cleanup(info);
 		unregister_framebuffer(info);
-		vfree((void __force *)info->screen_base);
+		vfree(info->screen_buffer);
 		if (par->board->remove)
 			par->board->remove(par);
 		module_put(par->board->owner);
-- 
2.40.0

Re: [PATCH v2 07/19] fbdev/hecubafb: Use struct fb_info.screen_buffer

[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.
>
> 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.
>
> As the driver operates on the latter address space, it is wrong to use
> .screen_base and .screen_buffer must be used instead. This also gets
> rid of casting needed due to 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

[PATCH v2 08/19] fbdev/metronomefb: Use struct fb_info.screen_buffer

[Thomas Zimmermann]

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

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.

As the driver operates on the latter address space, it is wrong to use
.screen_base and .screen_buffer must be used instead. This also gets
rid of casting needed due to not using the correct data type.

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

diff --git a/drivers/video/fbdev/metronomefb.c b/drivers/video/fbdev/metronomefb.c
index 9fd4bb85d735..afa9b41f5a87 100644
--- a/drivers/video/fbdev/metronomefb.c
+++ b/drivers/video/fbdev/metronomefb.c
@@ -438,7 +438,7 @@ static void metronomefb_dpy_update(struct metronomefb_par *par)
 {
 	int fbsize;
 	u16 cksum;
-	unsigned char *buf = (unsigned char __force *)par->info->screen_base;
+	unsigned char *buf = par->info->screen_buffer;
 
 	fbsize = par->info->fix.smem_len;
 	/* copy from vm to metromem */
@@ -453,7 +453,7 @@ static u16 metronomefb_dpy_update_page(struct metronomefb_par *par, int index)
 {
 	int i;
 	u16 csum = 0;
-	u16 *buf = (u16 __force *)(par->info->screen_base + index);
+	u16 *buf = (u16 *)(par->info->screen_buffer + index);
 	u16 *img = (u16 *)(par->metromem_img + index);
 
 	/* swizzle from vm to metromem and recalc cksum at the same time*/
@@ -543,7 +543,7 @@ static ssize_t metronomefb_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 (copy_from_user(dst, buf, count))
 		err = -EFAULT;
@@ -599,7 +599,7 @@ static int metronomefb_probe(struct platform_device *dev)
 		goto err;
 
 	/* we have two blocks of memory.
-	info->screen_base which is vm, and is the fb used by apps.
+	info->screen_buffer which is vm, and is the fb used by apps.
 	par->metromem which is physically contiguous memory and
 	contains the display controller commands, waveform,
 	processed image data and padding. this is the data pulled
@@ -634,7 +634,7 @@ static int metronomefb_probe(struct platform_device *dev)
 	if (!videomemory)
 		goto err_fb_rel;
 
-	info->screen_base = (char __force __iomem *)videomemory;
+	info->screen_buffer = videomemory;
 	info->fbops = &metronomefb_ops;
 
 	metronomefb_fix.line_length = fw;
@@ -756,7 +756,7 @@ static int metronomefb_remove(struct platform_device *dev)
 		fb_dealloc_cmap(&info->cmap);
 		par->board->cleanup(par);
 		vfree(par->csum_table);
-		vfree((void __force *)info->screen_base);
+		vfree(info->screen_buffer);
 		module_put(par->board->owner);
 		dev_dbg(&dev->dev, "calling release\n");
 		framebuffer_release(info);
-- 
2.40.0

Re: [PATCH v2 08/19] fbdev/metronomefb: Use struct fb_info.screen_buffer

[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.
>
> 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.
>
> As the driver operates on the latter address space, it is wrong to use
> .screen_base and .screen_buffer must be used instead. This also gets
> rid of casting needed due to 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

[PATCH v2 09/19] fbdev/ps3fb: Use struct fb_info.screen_buffer

[Thomas Zimmermann]

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

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.

As the driver operates on the latter address space, it is wrong to use
.screen_base and .screen_buffer must be used instead. This also gets
rid of casting needed due to not using the correct data type.

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

diff --git a/drivers/video/fbdev/ps3fb.c b/drivers/video/fbdev/ps3fb.c
index 98aaa330a193..d4abcf8aff75 100644
--- a/drivers/video/fbdev/ps3fb.c
+++ b/drivers/video/fbdev/ps3fb.c
@@ -650,7 +650,7 @@ static int ps3fb_set_par(struct fb_info *info)
 	}
 
 	/* Clear XDR frame buffer memory */
-	memset((void __force *)info->screen_base, 0, info->fix.smem_len);
+	memset(info->screen_buffer, 0, info->fix.smem_len);
 
 	/* Clear DDR frame buffer memory */
 	lines = vmode->yres * par->num_frames;
@@ -1140,7 +1140,7 @@ static int ps3fb_probe(struct ps3_system_bus_device *dev)
 	 * memory
 	 */
 	fb_start = ps3fb_videomemory.address + GPU_FB_START;
-	info->screen_base = (char __force __iomem *)fb_start;
+	info->screen_buffer = fb_start;
 	info->fix.smem_start = __pa(fb_start);
 	info->fix.smem_len = ps3fb_videomemory.size - GPU_FB_START;
 
-- 
2.40.0

Re: [PATCH v2 09/19] fbdev/ps3fb: Use struct fb_info.screen_buffer

[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.
>
> 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.
>
> As the driver operates on the latter address space, it is wrong to use
> .screen_base and .screen_buffer must be used instead. This also gets
> rid of casting needed due to 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

[PATCH v2 10/19] fbdev/smscufx: Use struct fb_info.screen_buffer

[Thomas Zimmermann]

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

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.

As the driver operates on the latter address space, it is wrong to use
.screen_base and .screen_buffer must be used instead.

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

diff --git a/drivers/video/fbdev/smscufx.c b/drivers/video/fbdev/smscufx.c
index 2ad6e98ce10d..17cec62cc65d 100644
--- a/drivers/video/fbdev/smscufx.c
+++ b/drivers/video/fbdev/smscufx.c
@@ -1150,7 +1150,7 @@ static void ufx_free_framebuffer(struct ufx_data *dev)
 		fb_dealloc_cmap(&info->cmap);
 	if (info->monspecs.modedb)
 		fb_destroy_modedb(info->monspecs.modedb);
-	vfree(info->screen_base);
+	vfree(info->screen_buffer);
 
 	fb_destroy_modelist(&info->modelist);
 
@@ -1257,7 +1257,7 @@ static int ufx_ops_set_par(struct fb_info *info)
 
 	if ((result == 0) && (dev->fb_count == 0)) {
 		/* paint greenscreen */
-		pix_framebuffer = (u16 *) info->screen_base;
+		pix_framebuffer = (u16 *)info->screen_buffer;
 		for (i = 0; i < info->fix.smem_len / 2; i++)
 			pix_framebuffer[i] = 0x37e6;
 
@@ -1303,7 +1303,7 @@ static int ufx_realloc_framebuffer(struct ufx_data *dev, struct fb_info *info)
 {
 	int old_len = info->fix.smem_len;
 	int new_len;
-	unsigned char *old_fb = info->screen_base;
+	unsigned char *old_fb = info->screen_buffer;
 	unsigned char *new_fb;
 
 	pr_debug("Reallocating framebuffer. Addresses will change!");
@@ -1318,12 +1318,12 @@ static int ufx_realloc_framebuffer(struct ufx_data *dev, struct fb_info *info)
 		if (!new_fb)
 			return -ENOMEM;
 
-		if (info->screen_base) {
+		if (info->screen_buffer) {
 			memcpy(new_fb, old_fb, old_len);
-			vfree(info->screen_base);
+			vfree(info->screen_buffer);
 		}
 
-		info->screen_base = new_fb;
+		info->screen_buffer = new_fb;
 		info->fix.smem_len = PAGE_ALIGN(new_len);
 		info->fix.smem_start = (unsigned long) new_fb;
 		info->flags = smscufx_info_flags;
@@ -1746,7 +1746,7 @@ static int ufx_usb_probe(struct usb_interface *interface,
 	atomic_set(&dev->usb_active, 0);
 setup_modes:
 	fb_destroy_modedb(info->monspecs.modedb);
-	vfree(info->screen_base);
+	vfree(info->screen_buffer);
 	fb_destroy_modelist(&info->modelist);
 error:
 	fb_dealloc_cmap(&info->cmap);
-- 
2.40.0

Re: [PATCH v2 10/19] fbdev/smscufx: Use struct fb_info.screen_buffer

[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.
>
> 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.
>
> As the driver operates on the latter address space, it is wrong to use
> .screen_base and .screen_buffer must be used 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

[PATCH v2 11/19] fbdev/udlfb: Use struct fb_info.screen_buffer

[Thomas Zimmermann]

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

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.

As the driver operates on the latter address space, it is wrong to use
.screen_base and .screen_buffer must be used instead. This also gets
rid of casting needed due to not using the correct data type.

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

diff --git a/drivers/video/fbdev/udlfb.c b/drivers/video/fbdev/udlfb.c
index 216d49c9d47e..09cf9381075a 100644
--- a/drivers/video/fbdev/udlfb.c
+++ b/drivers/video/fbdev/udlfb.c
@@ -1006,7 +1006,7 @@ static void dlfb_ops_destroy(struct fb_info *info)
 		fb_dealloc_cmap(&info->cmap);
 	if (info->monspecs.modedb)
 		fb_destroy_modedb(info->monspecs.modedb);
-	vfree(info->screen_base);
+	vfree(info->screen_buffer);
 
 	fb_destroy_modelist(&info->modelist);
 
@@ -1120,7 +1120,7 @@ static int dlfb_ops_set_par(struct fb_info *info)
 
 		/* paint greenscreen */
 
-		pix_framebuffer = (u16 *) info->screen_base;
+		pix_framebuffer = (u16 *)info->screen_buffer;
 		for (i = 0; i < info->fix.smem_len / 2; i++)
 			pix_framebuffer[i] = 0x37e6;
 	}
@@ -1219,7 +1219,7 @@ static void dlfb_deferred_vfree(struct dlfb_data *dlfb, void *mem)
 static int dlfb_realloc_framebuffer(struct dlfb_data *dlfb, struct fb_info *info, u32 new_len)
 {
 	u32 old_len = info->fix.smem_len;
-	const void *old_fb = (const void __force *)info->screen_base;
+	const void *old_fb = info->screen_buffer;
 	unsigned char *new_fb;
 	unsigned char *new_back = NULL;
 
@@ -1236,12 +1236,12 @@ static int dlfb_realloc_framebuffer(struct dlfb_data *dlfb, struct fb_info *info
 		}
 		memset(new_fb, 0xff, new_len);
 
-		if (info->screen_base) {
+		if (info->screen_buffer) {
 			memcpy(new_fb, old_fb, old_len);
-			dlfb_deferred_vfree(dlfb, (void __force *)info->screen_base);
+			dlfb_deferred_vfree(dlfb, info->screen_buffer);
 		}
 
-		info->screen_base = (char __iomem *)new_fb;
+		info->screen_buffer = new_fb;
 		info->fix.smem_len = new_len;
 		info->fix.smem_start = (unsigned long) new_fb;
 		info->flags = udlfb_info_flags;
-- 
2.40.0

Re: [PATCH v2 11/19] fbdev/udlfb: Use struct fb_info.screen_buffer

[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.
>
> 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.
>
> As the driver operates on the latter address space, it is wrong to use
> .screen_base and .screen_buffer must be used instead. This also gets
> rid of casting needed due to 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

[PATCH v2 12/19] fbdev/vfb: Use struct fb_info.screen_buffer

[Thomas Zimmermann]

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

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.

As the driver operates on the latter address space, it is wrong to use
.screen_base and .screen_buffer must be used instead. This also gets
rid of casting needed due to not using the correct data type.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/vfb.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/vfb.c b/drivers/video/fbdev/vfb.c
index a0a2009e003e..43c5ac90d527 100644
--- a/drivers/video/fbdev/vfb.c
+++ b/drivers/video/fbdev/vfb.c
@@ -440,7 +440,7 @@ static int vfb_probe(struct platform_device *dev)
 	if (!info)
 		goto err;
 
-	info->screen_base = (char __iomem *)videomemory;
+	info->screen_buffer = videomemory;
 	info->fbops = &vfb_ops;
 
 	if (!fb_find_mode(&info->var, info, mode_option,
-- 
2.40.0

Re: [PATCH v2 12/19] fbdev/vfb: Use struct fb_info.screen_buffer

[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.
>
> 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.
>
> As the driver operates on the latter address space, it is wrong to use
> .screen_base and .screen_buffer must be used instead. This also gets
> rid of casting needed due to 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

[PATCH v2 13/19] fbdev/xen-fbfront: Use struct fb_info.screen_buffer

[Thomas Zimmermann]

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

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.

As the driver operates on the latter address space, it is wrong to use
.screen_base and .screen_buffer must be used instead.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
---
 drivers/video/fbdev/xen-fbfront.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/video/fbdev/xen-fbfront.c b/drivers/video/fbdev/xen-fbfront.c
index d7f3e6281ce4..9b2a786621a6 100644
--- a/drivers/video/fbdev/xen-fbfront.c
+++ b/drivers/video/fbdev/xen-fbfront.c
@@ -429,7 +429,7 @@ static int xenfb_probe(struct xenbus_device *dev,
 	fb_info->pseudo_palette = fb_info->par;
 	fb_info->par = info;
 
-	fb_info->screen_base = info->fb;
+	fb_info->screen_buffer = info->fb;
 
 	fb_info->fbops = &xenfb_fb_ops;
 	fb_info->var.xres_virtual = fb_info->var.xres = video[KPARAM_WIDTH];
-- 
2.40.0

Re: [PATCH v2 13/19] fbdev/xen-fbfront: Use struct fb_info.screen_buffer

[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.
>
> 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.
>
> As the driver operates on the latter address space, it is wrong to use
> .screen_base and .screen_buffer must be used 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

[PATCH v2 14/19] 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.

It can happen that copy_{from, to}_user() only partially copies
the given buffer. Take this into account when calculating the
number of bytes.

v2:
	* consider return value from copy_{from,to}_user() (Geert)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Helge Deller <deller@gmx.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       | 15 ++++++++++-----
 2 files changed, 24 insertions(+), 15 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..b0881348c27f 100644
--- a/drivers/video/fbdev/core/fbmem.c
+++ b/drivers/video/fbdev/core/fbmem.c
@@ -766,7 +766,7 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	u8 *buffer, *dst;
 	u8 __iomem *src;
 	int c, cnt = 0, err = 0;
-	unsigned long total_size;
+	unsigned long total_size, trailing;
 
 	if (!info || ! info->screen_base)
 		return -ENODEV;
@@ -808,10 +808,13 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 		dst += c;
 		src += c;
 
-		if (copy_to_user(buf, buffer, c)) {
+		trailing = copy_to_user(buf, buffer, c);
+		if (trailing == c) {
 			err = -EFAULT;
 			break;
 		}
+		c -= trailing;
+
 		*ppos += c;
 		buf += c;
 		cnt += c;
@@ -820,7 +823,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
@@ -831,7 +834,7 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 	u8 *buffer, *src;
 	u8 __iomem *dst;
 	int c, cnt = 0, err = 0;
-	unsigned long total_size;
+	unsigned long total_size, trailing;
 
 	if (!info || !info->screen_base)
 		return -ENODEV;
@@ -876,10 +879,12 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 		c = (count > PAGE_SIZE) ? PAGE_SIZE : count;
 		src = buffer;
 
-		if (copy_from_user(src, buf, c)) {
+		trailing = copy_from_user(src, buf, c);
+		if (trailing == c) {
 			err = -EFAULT;
 			break;
 		}
+		c -= trailing;
 
 		fb_memcpy_tofb(dst, src, c);
 		dst += c;
-- 
2.40.0

[PATCH v2 15/19] 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.

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 gets rid of all the casting needed due to not using
the correct data type.

v2:
	* add detailed commit message (Javier)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Helge Deller <deller@gmx.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

[PATCH v2 16/19] 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}.

v2:
	* also remove test from ivtvfb, braodsheetfb, hecubafb and
	  metronomefb (Geert)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Helge Deller <deller@gmx.de>
---
 drivers/media/pci/ivtv/ivtvfb.c        | 3 ---
 drivers/video/fbdev/broadsheetfb.c     | 3 ---
 drivers/video/fbdev/core/fb_sys_fops.c | 6 ------
 drivers/video/fbdev/hecubafb.c         | 3 ---
 drivers/video/fbdev/metronomefb.c      | 3 ---
 drivers/video/fbdev/sm712fb.c          | 6 ------
 6 files changed, 24 deletions(-)

diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
index 00ac94d4ab19..22123a25daea 100644
--- a/drivers/media/pci/ivtv/ivtvfb.c
+++ b/drivers/media/pci/ivtv/ivtvfb.c
@@ -378,9 +378,6 @@ static ssize_t ivtvfb_write(struct fb_info *info, const char __user *buf,
 	unsigned long dma_size;
 	u16 lead = 0, tail = 0;
 
-	if (info->state != FBINFO_STATE_RUNNING)
-		return -EPERM;
-
 	total_size = info->screen_size;
 
 	if (total_size == 0)
diff --git a/drivers/video/fbdev/broadsheetfb.c b/drivers/video/fbdev/broadsheetfb.c
index 65dc86b7081e..691de5df581b 100644
--- a/drivers/video/fbdev/broadsheetfb.c
+++ b/drivers/video/fbdev/broadsheetfb.c
@@ -1013,9 +1013,6 @@ static ssize_t broadsheetfb_write(struct fb_info *info, const char __user *buf,
 	int err = 0;
 	unsigned long total_size;
 
-	if (info->state != FBINFO_STATE_RUNNING)
-		return -EPERM;
-
 	total_size = info->fix.smem_len;
 
 	if (p > total_size)
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/hecubafb.c b/drivers/video/fbdev/hecubafb.c
index ddfa2853cc41..a2996d39f918 100644
--- a/drivers/video/fbdev/hecubafb.c
+++ b/drivers/video/fbdev/hecubafb.c
@@ -163,9 +163,6 @@ static ssize_t hecubafb_write(struct fb_info *info, const char __user *buf,
 	int err = 0;
 	unsigned long total_size;
 
-	if (info->state != FBINFO_STATE_RUNNING)
-		return -EPERM;
-
 	total_size = info->fix.smem_len;
 
 	if (p > total_size)
diff --git a/drivers/video/fbdev/metronomefb.c b/drivers/video/fbdev/metronomefb.c
index afa9b41f5a87..2bb068cadac6 100644
--- a/drivers/video/fbdev/metronomefb.c
+++ b/drivers/video/fbdev/metronomefb.c
@@ -523,9 +523,6 @@ static ssize_t metronomefb_write(struct fb_info *info, const char __user *buf,
 	int err = 0;
 	unsigned long total_size;
 
-	if (info->state != FBINFO_STATE_RUNNING)
-		return -EPERM;
-
 	total_size = info->fix.smem_len;
 
 	if (p > total_size)
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

[PATCH v2 17/19] 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.

While bothi fields, screen_base and screen_buffer, are stored in the
same location, they refer to different address spaces. For correctness,
we want to test each field in exactly the code that uses it.

v2:
	* also test screen_base in pvr2fb (Geert)
	* also test screen_buffer in ivtvfb, arcfb, broadsheetfb,
	  hecubafb, metronomefb and ssd1307fb (Geert)
	* give a rational for the change (Geert)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Helge Deller <deller@gmx.de>
---
 drivers/media/pci/ivtv/ivtvfb.c        |  3 +++
 drivers/video/fbdev/arcfb.c            |  3 +++
 drivers/video/fbdev/broadsheetfb.c     |  3 +++
 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/hecubafb.c         |  3 +++
 drivers/video/fbdev/metronomefb.c      |  3 +++
 drivers/video/fbdev/pvr2fb.c           |  3 +++
 drivers/video/fbdev/sm712fb.c          |  4 ++--
 drivers/video/fbdev/ssd1307fb.c        |  3 +++
 11 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/drivers/media/pci/ivtv/ivtvfb.c b/drivers/media/pci/ivtv/ivtvfb.c
index 22123a25daea..0aeb9daaee4c 100644
--- a/drivers/media/pci/ivtv/ivtvfb.c
+++ b/drivers/media/pci/ivtv/ivtvfb.c
@@ -378,6 +378,9 @@ static ssize_t ivtvfb_write(struct fb_info *info, const char __user *buf,
 	unsigned long dma_size;
 	u16 lead = 0, tail = 0;
 
+	if (!info->screen_base)
+		return -ENODEV;
+
 	total_size = info->screen_size;
 
 	if (total_size == 0)
diff --git a/drivers/video/fbdev/arcfb.c b/drivers/video/fbdev/arcfb.c
index 088c4b30fd31..7750e020839e 100644
--- a/drivers/video/fbdev/arcfb.c
+++ b/drivers/video/fbdev/arcfb.c
@@ -451,6 +451,9 @@ static ssize_t arcfb_write(struct fb_info *info, const char __user *buf,
 	struct arcfb_par *par;
 	unsigned int xres;
 
+	if (!info->screen_buffer)
+		return -ENODEV;
+
 	p = *ppos;
 	par = info->par;
 	xres = info->var.xres;
diff --git a/drivers/video/fbdev/broadsheetfb.c b/drivers/video/fbdev/broadsheetfb.c
index 691de5df581b..e9c5d5c04062 100644
--- a/drivers/video/fbdev/broadsheetfb.c
+++ b/drivers/video/fbdev/broadsheetfb.c
@@ -1013,6 +1013,9 @@ static ssize_t broadsheetfb_write(struct fb_info *info, const char __user *buf,
 	int err = 0;
 	unsigned long total_size;
 
+	if (!info->screen_buffer)
+		return -ENODEV;
+
 	total_size = info->fix.smem_len;
 
 	if (p > total_size)
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 b0881348c27f..3a80d13afd26 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, trailing;
 
-	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)
@@ -836,7 +839,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, trailing;
 
-	if (!info || !info->screen_base)
+	if (!info)
 		return -ENODEV;
 
 	if (info->state != FBINFO_STATE_RUNNING)
@@ -845,6 +848,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/hecubafb.c b/drivers/video/fbdev/hecubafb.c
index a2996d39f918..72308d4e0c22 100644
--- a/drivers/video/fbdev/hecubafb.c
+++ b/drivers/video/fbdev/hecubafb.c
@@ -163,6 +163,9 @@ static ssize_t hecubafb_write(struct fb_info *info, const char __user *buf,
 	int err = 0;
 	unsigned long total_size;
 
+	if (!info->screen_buffer)
+		return -ENODEV;
+
 	total_size = info->fix.smem_len;
 
 	if (p > total_size)
diff --git a/drivers/video/fbdev/metronomefb.c b/drivers/video/fbdev/metronomefb.c
index 2bb068cadac6..7fc59466fe6c 100644
--- a/drivers/video/fbdev/metronomefb.c
+++ b/drivers/video/fbdev/metronomefb.c
@@ -523,6 +523,9 @@ static ssize_t metronomefb_write(struct fb_info *info, const char __user *buf,
 	int err = 0;
 	unsigned long total_size;
 
+	if (!info->screen_buffer)
+		return -ENODEV;
+
 	total_size = info->fix.smem_len;
 
 	if (p > total_size)
diff --git a/drivers/video/fbdev/pvr2fb.c b/drivers/video/fbdev/pvr2fb.c
index 6888127a5eb8..550fdb5b4d41 100644
--- a/drivers/video/fbdev/pvr2fb.c
+++ b/drivers/video/fbdev/pvr2fb.c
@@ -647,6 +647,9 @@ static ssize_t pvr2fb_write(struct fb_info *info, const char *buf,
 	struct page **pages;
 	int ret, i;
 
+	if (!info->screen_base)
+		return -ENODEV;
+
 	nr_pages = (count + PAGE_SIZE - 1) >> PAGE_SHIFT;
 
 	pages = kmalloc_array(nr_pages, sizeof(struct page *), GFP_KERNEL);
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;
diff --git a/drivers/video/fbdev/ssd1307fb.c b/drivers/video/fbdev/ssd1307fb.c
index 046b9990d27c..a8f2975de76b 100644
--- a/drivers/video/fbdev/ssd1307fb.c
+++ b/drivers/video/fbdev/ssd1307fb.c
@@ -301,6 +301,9 @@ static ssize_t ssd1307fb_write(struct fb_info *info, const char __user *buf,
 	void *dst;
 	int ret;
 
+	if (!info->screen_buffer)
+		return -ENODEV;
+
 	total_size = info->fix.smem_len;
 
 	if (p > total_size)
-- 
2.40.0

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

[Geert Uytterhoeven]

On Fri, Apr 28, 2023 at 2:26 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.
>
> While bothi fields, screen_base and screen_buffer, are stored in the

both

> same location, they refer to different address spaces. For correctness,
> we want to test each field in exactly the code that uses it.

Not a direct comment for this patch: and later the union can be split
in two separate fields, to protect against misuse?

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 v2 17/19] fbdev: Validate info->screen_{base,buffer} in fb_ops implementations

[Thomas Zimmermann]

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

Hi

Am 03.05.23 um 11:51 schrieb Geert Uytterhoeven:
> On Fri, Apr 28, 2023 at 2:26 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.
>>
>> While bothi fields, screen_base and screen_buffer, are stored in the
> 
> both
> 
>> same location, they refer to different address spaces. For correctness,
>> we want to test each field in exactly the code that uses it.
> 
> Not a direct comment for this patch: and later the union can be split
> in two separate fields, to protect against misuse?

No idea. Currently we have sparse that warns about mismatching address 
spaces if the fields are mixed up. That's good enough, as far I'm concerned.

Best regards
Thomas

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

-- 
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 v2 17/19] fbdev: Validate info->screen_{base,buffer} in fb_ops implementations

[Geert Uytterhoeven]

Hi Thomas,

On Wed, May 3, 2023 at 4:30 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 03.05.23 um 11:51 schrieb Geert Uytterhoeven:
> > On Fri, Apr 28, 2023 at 2:26 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.
> >>
> >> While bothi fields, screen_base and screen_buffer, are stored in the
> >
> > both
> >
> >> same location, they refer to different address spaces. For correctness,
> >> we want to test each field in exactly the code that uses it.
> >
> > Not a direct comment for this patch: and later the union can be split
> > in two separate fields, to protect against misuse?
>
> No idea. Currently we have sparse that warns about mismatching address
> spaces if the fields are mixed up. That's good enough, as far I'm concerned.

The potential issue that is still present is that an fbdev driver uses
fb_info.screen_base, and configures the use of drawing ops that use
fb_info.screen_buffer (or vice-versa), which will happily use the wrong
type of pointer.  Sparse doesn't protect against that.

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 v2 17/19] fbdev: Validate info->screen_{base,buffer} in fb_ops implementations

[Thomas Zimmermann]

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

Hi

Am 03.05.23 um 17:02 schrieb Geert Uytterhoeven:
> Hi Thomas,
> 
> On Wed, May 3, 2023 at 4:30 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 03.05.23 um 11:51 schrieb Geert Uytterhoeven:
>>> On Fri, Apr 28, 2023 at 2:26 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.
>>>>
>>>> While bothi fields, screen_base and screen_buffer, are stored in the
>>>
>>> both
>>>
>>>> same location, they refer to different address spaces. For correctness,
>>>> we want to test each field in exactly the code that uses it.
>>>
>>> Not a direct comment for this patch: and later the union can be split
>>> in two separate fields, to protect against misuse?
>>
>> No idea. Currently we have sparse that warns about mismatching address
>> spaces if the fields are mixed up. That's good enough, as far I'm concerned.
> 
> The potential issue that is still present is that an fbdev driver uses
> fb_info.screen_base, and configures the use of drawing ops that use
> fb_info.screen_buffer (or vice-versa), which will happily use the wrong
> type of pointer.  Sparse doesn't protect against that.

Right. From a quick grep, I've found quite a cases where cfb_ functions 
operate on non-__iomem memory. I'm sure that the opposite with sys_ 
functions exists as well. Fixing this will be a good follow-up patchset. 
Thanks for the suggestion.

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 v2 18/19] fbdev: Move I/O read and write code into helper functions

[Thomas Zimmermann]

Move the existing I/O 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.

In the near term, the new functions will be useful to the DRM
subsystem, which currently provides it's own implementation. It
can then use the shared code. In the longer term, it might make
sense to revise the I/O helper's default status and make them
opt-in by the driver. Systems that don't use them would not
contain the code any longer.

v2:
	* add detailed commit message (Javier)
	* rename fb_cfb_() to fb_io_() (Geert)
	* add fixes that got lost while moving the code (Geert)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Helge Deller <deller@gmx.de>
---
 drivers/video/fbdev/core/Makefile     |   2 +-
 drivers/video/fbdev/core/fb_io_fops.c | 133 ++++++++++++++++++++++++++
 drivers/video/fbdev/core/fbmem.c      | 118 +----------------------
 include/linux/fb.h                    |  10 ++
 4 files changed, 146 insertions(+), 117 deletions(-)
 create mode 100644 drivers/video/fbdev/core/fb_io_fops.c

diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
index 08fabce76b74..8f0060160ffb 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_io_fops.o
 fb-$(CONFIG_FB_DEFERRED_IO)       += fb_defio.o
 
 ifeq ($(CONFIG_FRAMEBUFFER_CONSOLE),y)
diff --git a/drivers/video/fbdev/core/fb_io_fops.c b/drivers/video/fbdev/core/fb_io_fops.c
new file mode 100644
index 000000000000..f5299d50f33b
--- /dev/null
+++ b/drivers/video/fbdev/core/fb_io_fops.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0
+
+#include <linux/fb.h>
+#include <linux/module.h>
+#include <linux/uaccess.h>
+
+ssize_t fb_io_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, trailing;
+
+	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;
+
+		trailing = copy_to_user(buf, buffer, c);
+		if (trailing == c) {
+			err = -EFAULT;
+			break;
+		}
+		c -= trailing;
+
+		*ppos += c;
+		buf += c;
+		cnt += c;
+		count -= c;
+	}
+
+	kfree(buffer);
+
+	return cnt ? cnt : err;
+}
+EXPORT_SYMBOL(fb_io_read);
+
+ssize_t fb_io_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, trailing;
+
+	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;
+
+		trailing = copy_from_user(src, buf, c);
+		if (trailing == c) {
+			err = -EFAULT;
+			break;
+		}
+		c -= trailing;
+
+		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_io_write);
diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
index 3a80d13afd26..4035a57df116 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, trailing;
 
 	if (!info)
 		return -ENODEV;
@@ -777,67 +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;
-
-		trailing = copy_to_user(buf, buffer, c);
-		if (trailing == c) {
-			err = -EFAULT;
-			break;
-		}
-		c -= trailing;
-
-		*ppos += c;
-		buf += c;
-		cnt += c;
-		count -= c;
-	}
-
-	kfree(buffer);
-
-	return cnt ? cnt : err;
+	return fb_io_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, trailing;
 
 	if (!info)
 		return -ENODEV;
@@ -848,62 +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;
-
-		trailing = copy_from_user(src, buf, c);
-		if (trailing == c) {
-			err = -EFAULT;
-			break;
-		}
-		c -= trailing;
-
-		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_io_write(info, buf, count, ppos);
 }
 
 int
diff --git a/include/linux/fb.h b/include/linux/fb.h
index 08cb47da71f8..ec978a4969a9 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 I/O memory
+ */
+
 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_io_read(struct fb_info *info, char __user *buf,
+			  size_t count, loff_t *ppos);
+extern ssize_t fb_io_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 v2 18/19] fbdev: Move I/O read and write code into helper functions

[Sam Ravnborg]

Hi Thomas,

On Fri, Apr 28, 2023 at 02:24:51PM +0200, Thomas Zimmermann wrote:
> Move the existing I/O read and write code for I/O memory into
> the new helpers fb_cfb_read() and fb_cfb_write(). Make them the
You may want to update the changelog to mention the new names.

A few comments below, but reading it once more I realize this are
all comments to the original code, so not something to fix in this
patch.


> default fp_ops. No functional changes.
> 
> In the near term, the new functions will be useful to the DRM
> subsystem, which currently provides it's own implementation. It
> can then use the shared code. In the longer term, it might make
> sense to revise the I/O helper's default status and make them
> opt-in by the driver. Systems that don't use them would not
> contain the code any longer.
> 
> v2:
> 	* add detailed commit message (Javier)
> 	* rename fb_cfb_() to fb_io_() (Geert)
> 	* add fixes that got lost while moving the code (Geert)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Acked-by: Helge Deller <deller@gmx.de>
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>
> ---
>  drivers/video/fbdev/core/Makefile     |   2 +-
>  drivers/video/fbdev/core/fb_io_fops.c | 133 ++++++++++++++++++++++++++
>  drivers/video/fbdev/core/fbmem.c      | 118 +----------------------
>  include/linux/fb.h                    |  10 ++
>  4 files changed, 146 insertions(+), 117 deletions(-)
>  create mode 100644 drivers/video/fbdev/core/fb_io_fops.c
> 
> diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
> index 08fabce76b74..8f0060160ffb 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_io_fops.o
>  fb-$(CONFIG_FB_DEFERRED_IO)       += fb_defio.o
>  
>  ifeq ($(CONFIG_FRAMEBUFFER_CONSOLE),y)
> diff --git a/drivers/video/fbdev/core/fb_io_fops.c b/drivers/video/fbdev/core/fb_io_fops.c
> new file mode 100644
> index 000000000000..f5299d50f33b
> --- /dev/null
> +++ b/drivers/video/fbdev/core/fb_io_fops.c
> @@ -0,0 +1,133 @@
> +// SPDX-License-Identifier: GPL-2.0
> +
> +#include <linux/fb.h>
> +#include <linux/module.h>
> +#include <linux/uaccess.h>
> +
> +ssize_t fb_io_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, trailing;
> +
> +	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);
screen_base is char __iomem *, so I think the cast can go away.
> +
> +	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;
I see no reason to use dst here, as it is always equal to buffer when
used.
But this is copied from the original code, so it would be wrong to
change this in the current patch. I just noticed.

> +		src += c;
> +
> +		trailing = copy_to_user(buf, buffer, c);
> +		if (trailing == c) {
> +			err = -EFAULT;
> +			break;
> +		}
> +		c -= trailing;
> +
> +		*ppos += c;
> +		buf += c;
> +		cnt += c;
> +		count -= c;
> +	}
> +
> +	kfree(buffer);
> +
> +	return cnt ? cnt : err;
> +}
> +EXPORT_SYMBOL(fb_io_read);
> +
> +ssize_t fb_io_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, trailing;
> +
> +	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);
Cast can go away.

> +
> +	if (info->fbops->fb_sync)
> +		info->fbops->fb_sync(info);
> +
> +	while (count) {
> +		c = (count > PAGE_SIZE) ? PAGE_SIZE : count;
> +		src = buffer;
> +
> +		trailing = copy_from_user(src, buf, c);
> +		if (trailing == c) {
> +			err = -EFAULT;
> +			break;
> +		}
> +		c -= trailing;
> +
> +		fb_memcpy_tofb(dst, src, c);
> +		dst += c;
> +		src += c;
src is incremented here, but the value are not used.
Again, from the original code so not something to change here.

> +		*ppos += c;
> +		buf += c;
> +		cnt += c;
> +		count -= c;
> +	}
> +
> +	kfree(buffer);
> +
> +	return (cnt) ? cnt : err;
> +}
> +EXPORT_SYMBOL(fb_io_write);
> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
> index 3a80d13afd26..4035a57df116 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, trailing;
>  
>  	if (!info)
>  		return -ENODEV;
> @@ -777,67 +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;
> -
> -		trailing = copy_to_user(buf, buffer, c);
> -		if (trailing == c) {
> -			err = -EFAULT;
> -			break;
> -		}
> -		c -= trailing;
> -
> -		*ppos += c;
> -		buf += c;
> -		cnt += c;
> -		count -= c;
> -	}
> -
> -	kfree(buffer);
> -
> -	return cnt ? cnt : err;
> +	return fb_io_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, trailing;
>  
>  	if (!info)
>  		return -ENODEV;
> @@ -848,62 +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;
> -
> -		trailing = copy_from_user(src, buf, c);
> -		if (trailing == c) {
> -			err = -EFAULT;
> -			break;
> -		}
> -		c -= trailing;
> -
> -		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_io_write(info, buf, count, ppos);
>  }
>  
>  int
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index 08cb47da71f8..ec978a4969a9 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 I/O memory
> + */
> +
>  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_io_read(struct fb_info *info, char __user *buf,
> +			  size_t count, loff_t *ppos);
> +extern ssize_t fb_io_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 v2 18/19] fbdev: Move I/O read and write code into helper functions

[Thomas Zimmermann]

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

Hi Sam

Am 30.04.23 um 20:14 schrieb Sam Ravnborg:
> Hi Thomas,
> 
> On Fri, Apr 28, 2023 at 02:24:51PM +0200, Thomas Zimmermann wrote:
>> Move the existing I/O read and write code for I/O memory into
>> the new helpers fb_cfb_read() and fb_cfb_write(). Make them the
> You may want to update the changelog to mention the new names.
> 
> A few comments below, but reading it once more I realize this are
> all comments to the original code, so not something to fix in this
> patch.

Yes, it's probably worth a follow-up patch.

> 
> 
>> default fp_ops. No functional changes.
>>
>> In the near term, the new functions will be useful to the DRM
>> subsystem, which currently provides it's own implementation. It
>> can then use the shared code. In the longer term, it might make
>> sense to revise the I/O helper's default status and make them
>> opt-in by the driver. Systems that don't use them would not
>> contain the code any longer.
>>
>> v2:
>> 	* add detailed commit message (Javier)
>> 	* rename fb_cfb_() to fb_io_() (Geert)
>> 	* add fixes that got lost while moving the code (Geert)
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>> Acked-by: Helge Deller <deller@gmx.de>
> Reviewed-by: Sam Ravnborg <sam@ravnborg.org>

Thanks for reviewing. If nothing else comes in, I'll probably merge this 
series in the next few days.

Best regards
Thomas

>> ---
>>   drivers/video/fbdev/core/Makefile     |   2 +-
>>   drivers/video/fbdev/core/fb_io_fops.c | 133 ++++++++++++++++++++++++++
>>   drivers/video/fbdev/core/fbmem.c      | 118 +----------------------
>>   include/linux/fb.h                    |  10 ++
>>   4 files changed, 146 insertions(+), 117 deletions(-)
>>   create mode 100644 drivers/video/fbdev/core/fb_io_fops.c
>>
>> diff --git a/drivers/video/fbdev/core/Makefile b/drivers/video/fbdev/core/Makefile
>> index 08fabce76b74..8f0060160ffb 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_io_fops.o
>>   fb-$(CONFIG_FB_DEFERRED_IO)       += fb_defio.o
>>   
>>   ifeq ($(CONFIG_FRAMEBUFFER_CONSOLE),y)
>> diff --git a/drivers/video/fbdev/core/fb_io_fops.c b/drivers/video/fbdev/core/fb_io_fops.c
>> new file mode 100644
>> index 000000000000..f5299d50f33b
>> --- /dev/null
>> +++ b/drivers/video/fbdev/core/fb_io_fops.c
>> @@ -0,0 +1,133 @@
>> +// SPDX-License-Identifier: GPL-2.0
>> +
>> +#include <linux/fb.h>
>> +#include <linux/module.h>
>> +#include <linux/uaccess.h>
>> +
>> +ssize_t fb_io_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, trailing;
>> +
>> +	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);
> screen_base is char __iomem *, so I think the cast can go away.
>> +
>> +	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;
> I see no reason to use dst here, as it is always equal to buffer when
> used.
> But this is copied from the original code, so it would be wrong to
> change this in the current patch. I just noticed.
> 
>> +		src += c;
>> +
>> +		trailing = copy_to_user(buf, buffer, c);
>> +		if (trailing == c) {
>> +			err = -EFAULT;
>> +			break;
>> +		}
>> +		c -= trailing;
>> +
>> +		*ppos += c;
>> +		buf += c;
>> +		cnt += c;
>> +		count -= c;
>> +	}
>> +
>> +	kfree(buffer);
>> +
>> +	return cnt ? cnt : err;
>> +}
>> +EXPORT_SYMBOL(fb_io_read);
>> +
>> +ssize_t fb_io_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, trailing;
>> +
>> +	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);
> Cast can go away.
> 
>> +
>> +	if (info->fbops->fb_sync)
>> +		info->fbops->fb_sync(info);
>> +
>> +	while (count) {
>> +		c = (count > PAGE_SIZE) ? PAGE_SIZE : count;
>> +		src = buffer;
>> +
>> +		trailing = copy_from_user(src, buf, c);
>> +		if (trailing == c) {
>> +			err = -EFAULT;
>> +			break;
>> +		}
>> +		c -= trailing;
>> +
>> +		fb_memcpy_tofb(dst, src, c);
>> +		dst += c;
>> +		src += c;
> src is incremented here, but the value are not used.
> Again, from the original code so not something to change here.
> 
>> +		*ppos += c;
>> +		buf += c;
>> +		cnt += c;
>> +		count -= c;
>> +	}
>> +
>> +	kfree(buffer);
>> +
>> +	return (cnt) ? cnt : err;
>> +}
>> +EXPORT_SYMBOL(fb_io_write);
>> diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c
>> index 3a80d13afd26..4035a57df116 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, trailing;
>>   
>>   	if (!info)
>>   		return -ENODEV;
>> @@ -777,67 +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;
>> -
>> -		trailing = copy_to_user(buf, buffer, c);
>> -		if (trailing == c) {
>> -			err = -EFAULT;
>> -			break;
>> -		}
>> -		c -= trailing;
>> -
>> -		*ppos += c;
>> -		buf += c;
>> -		cnt += c;
>> -		count -= c;
>> -	}
>> -
>> -	kfree(buffer);
>> -
>> -	return cnt ? cnt : err;
>> +	return fb_io_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, trailing;
>>   
>>   	if (!info)
>>   		return -ENODEV;
>> @@ -848,62 +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;
>> -
>> -		trailing = copy_from_user(src, buf, c);
>> -		if (trailing == c) {
>> -			err = -EFAULT;
>> -			break;
>> -		}
>> -		c -= trailing;
>> -
>> -		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_io_write(info, buf, count, ppos);
>>   }
>>   
>>   int
>> diff --git a/include/linux/fb.h b/include/linux/fb.h
>> index 08cb47da71f8..ec978a4969a9 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 I/O memory
>> + */
>> +
>>   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_io_read(struct fb_info *info, char __user *buf,
>> +			  size_t count, loff_t *ppos);
>> +extern ssize_t fb_io_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

-- 
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 v2 19/19] 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.

v2:
	* rename fb_cfb_() to fb_io_() (Geert)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Acked-by: Helge Deller <deller@gmx.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..f0e9549b6bd7 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_io_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_io_write(info, buf, count, ppos);
 	if (ret <= 0)
 		return ret;
 
-- 
2.40.0

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

[Sam Ravnborg]

On Fri, Apr 28, 2023 at 02:24:52PM +0200, Thomas Zimmermann wrote:
> Implement DRM fbdev helpers for reading and writing framebuffer
> memory with the respective fbdev functions. Removes duplicate
> code.
> 
> v2:
> 	* rename fb_cfb_() to fb_io_() (Geert)
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Tested-by: Sui Jingfeng <suijingfeng@loongson.cn>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> Acked-by: Helge Deller <deller@gmx.de>

Nice cleanup.
Reviewed-by: Sam Ravnborg <sam@ravnborg.org>