| Message ID | 20230425142846.730-2-tzimmermann@suse.de |
|---|---|
| State | New |
| Headers | show |
| Series | drm,fbdev: Use fbdev's I/O helpers | expand |
Thomas Zimmermann <tzimmermann@suse.de> writes: Hello Thomas, > Always return the number of bytes read or written within the > framebuffer. Only return an errno code if framebuffer memory > was not touched. This is the semantics required by POSIX and > makes fb_read() and fb_write() compatible with IGT tests. [1] > > This bug has been fixed for fb_write() long ago by > commit 6a2a88668e90 ("[PATCH] fbdev: Fix return error of > fb_write"). The code in fb_read() and the corresponding fb_sys_() > helpers was forgotten. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Link: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/fbdev.c # 1 > --- The patch looks good to me and indeed the correct semantics AFAICT. Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Hi, The whole patch set is tested with fbdev of IGT, on LoongArch with drm/radeon and efifb driver. Test results say SUCCESS. Tested-by: Sui Jingfeng <suijingfeng@loongson.cn> On 2023/4/25 22:28, Thomas Zimmermann wrote: > Always return the number of bytes read or written within the > framebuffer. Only return an errno code if framebuffer memory > was not touched. This is the semantics required by POSIX and > makes fb_read() and fb_write() compatible with IGT tests. [1] > > This bug has been fixed for fb_write() long ago by > commit 6a2a88668e90 ("[PATCH] fbdev: Fix return error of > fb_write"). The code in fb_read() and the corresponding fb_sys_() > helpers was forgotten. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Link: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/fbdev.c # 1 > Reviewed-by: Javier Martinez Canillas <javierm@redhat.com> > --- > drivers/video/fbdev/core/fb_sys_fops.c | 24 ++++++++++++++---------- > drivers/video/fbdev/core/fbmem.c | 2 +- > 2 files changed, 15 insertions(+), 11 deletions(-) > > diff --git a/drivers/video/fbdev/core/fb_sys_fops.c b/drivers/video/fbdev/core/fb_sys_fops.c > index ff275d7f3eaf..cefb77b9546d 100644 > --- a/drivers/video/fbdev/core/fb_sys_fops.c > +++ b/drivers/video/fbdev/core/fb_sys_fops.c > @@ -19,7 +19,8 @@ ssize_t fb_sys_read(struct fb_info *info, char __user *buf, size_t count, > unsigned long p = *ppos; > void *src; > int err = 0; > - unsigned long total_size; > + unsigned long total_size, c; > + ssize_t ret; > > if (info->state != FBINFO_STATE_RUNNING) > return -EPERM; > @@ -43,13 +44,14 @@ ssize_t fb_sys_read(struct fb_info *info, char __user *buf, size_t count, > if (info->fbops->fb_sync) > info->fbops->fb_sync(info); > > - if (copy_to_user(buf, src, count)) > + c = copy_to_user(buf, src, count); > + if (c) > err = -EFAULT; > + ret = count - c; > > - if (!err) > - *ppos += count; > + *ppos += ret; > > - return (err) ? err : count; > + return ret ? ret : err; > } > EXPORT_SYMBOL_GPL(fb_sys_read); > > @@ -59,7 +61,8 @@ ssize_t fb_sys_write(struct fb_info *info, const char __user *buf, > unsigned long p = *ppos; > void *dst; > int err = 0; > - unsigned long total_size; > + unsigned long total_size, c; > + size_t ret; > > if (info->state != FBINFO_STATE_RUNNING) > return -EPERM; > @@ -89,13 +92,14 @@ ssize_t fb_sys_write(struct fb_info *info, const char __user *buf, > if (info->fbops->fb_sync) > info->fbops->fb_sync(info); > > - if (copy_from_user(dst, buf, count)) > + c = copy_from_user(dst, buf, count); > + if (c) > err = -EFAULT; > + ret = count - c; > > - if (!err) > - *ppos += count; > + *ppos += ret; > > - return (err) ? err : count; > + return ret ? ret : err; > } > EXPORT_SYMBOL_GPL(fb_sys_write); > > diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c > index 3fd95a79e4c3..abc92d79a295 100644 > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -820,7 +820,7 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos) > > kfree(buffer); > > - return (err) ? err : cnt; > + return cnt ? cnt : err; > } > > static ssize_t
Hi Thomas, On Tue, Apr 25, 2023 at 4:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: > Always return the number of bytes read or written within the > framebuffer. Only return an errno code if framebuffer memory > was not touched. This is the semantics required by POSIX and > makes fb_read() and fb_write() compatible with IGT tests. [1] > > This bug has been fixed for fb_write() long ago by > commit 6a2a88668e90 ("[PATCH] fbdev: Fix return error of > fb_write"). The code in fb_read() and the corresponding fb_sys_() > helpers was forgotten. > > Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> > Link: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/fbdev.c # 1 Thanks for your patch! > --- a/drivers/video/fbdev/core/fbmem.c > +++ b/drivers/video/fbdev/core/fbmem.c > @@ -820,7 +820,7 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos) > > kfree(buffer); > > - return (err) ? err : cnt; > + return cnt ? cnt : err; > } Looks all good to me, so Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> However, shouldn't the copy_to_user() handling in fb_read() be fixed, too? Gr{oetje,eeting}s, Geert
Hi Am 26.04.23 um 16:41 schrieb Geert Uytterhoeven: > Hi Thomas, > > On Tue, Apr 25, 2023 at 4:28 PM Thomas Zimmermann <tzimmermann@suse.de> wrote: >> Always return the number of bytes read or written within the >> framebuffer. Only return an errno code if framebuffer memory >> was not touched. This is the semantics required by POSIX and >> makes fb_read() and fb_write() compatible with IGT tests. [1] >> >> This bug has been fixed for fb_write() long ago by >> commit 6a2a88668e90 ("[PATCH] fbdev: Fix return error of >> fb_write"). The code in fb_read() and the corresponding fb_sys_() >> helpers was forgotten. >> >> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> >> Link: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/fbdev.c # 1 > > Thanks for your patch! > >> --- a/drivers/video/fbdev/core/fbmem.c >> +++ b/drivers/video/fbdev/core/fbmem.c >> @@ -820,7 +820,7 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos) >> >> kfree(buffer); >> >> - return (err) ? err : cnt; >> + return cnt ? cnt : err; >> } > > Looks all good to me, so > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> > > However, shouldn't the copy_to_user() handling in fb_read() be fixed, > too? That's a good point. It doesn't necessarily copy all given bytes and can thus return the wrong result. The IGT tests passed anyway, but I'll fix it in v2. Best regards Thomas > > Gr{oetje,eeting}s, > > Geert >
diff --git a/drivers/video/fbdev/core/fb_sys_fops.c b/drivers/video/fbdev/core/fb_sys_fops.c index ff275d7f3eaf..cefb77b9546d 100644 --- a/drivers/video/fbdev/core/fb_sys_fops.c +++ b/drivers/video/fbdev/core/fb_sys_fops.c @@ -19,7 +19,8 @@ ssize_t fb_sys_read(struct fb_info *info, char __user *buf, size_t count, unsigned long p = *ppos; void *src; int err = 0; - unsigned long total_size; + unsigned long total_size, c; + ssize_t ret; if (info->state != FBINFO_STATE_RUNNING) return -EPERM; @@ -43,13 +44,14 @@ ssize_t fb_sys_read(struct fb_info *info, char __user *buf, size_t count, if (info->fbops->fb_sync) info->fbops->fb_sync(info); - if (copy_to_user(buf, src, count)) + c = copy_to_user(buf, src, count); + if (c) err = -EFAULT; + ret = count - c; - if (!err) - *ppos += count; + *ppos += ret; - return (err) ? err : count; + return ret ? ret : err; } EXPORT_SYMBOL_GPL(fb_sys_read); @@ -59,7 +61,8 @@ ssize_t fb_sys_write(struct fb_info *info, const char __user *buf, unsigned long p = *ppos; void *dst; int err = 0; - unsigned long total_size; + unsigned long total_size, c; + size_t ret; if (info->state != FBINFO_STATE_RUNNING) return -EPERM; @@ -89,13 +92,14 @@ ssize_t fb_sys_write(struct fb_info *info, const char __user *buf, if (info->fbops->fb_sync) info->fbops->fb_sync(info); - if (copy_from_user(dst, buf, count)) + c = copy_from_user(dst, buf, count); + if (c) err = -EFAULT; + ret = count - c; - if (!err) - *ppos += count; + *ppos += ret; - return (err) ? err : count; + return ret ? ret : err; } EXPORT_SYMBOL_GPL(fb_sys_write); diff --git a/drivers/video/fbdev/core/fbmem.c b/drivers/video/fbdev/core/fbmem.c index 3fd95a79e4c3..abc92d79a295 100644 --- a/drivers/video/fbdev/core/fbmem.c +++ b/drivers/video/fbdev/core/fbmem.c @@ -820,7 +820,7 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos) kfree(buffer); - return (err) ? err : cnt; + return cnt ? cnt : err; } static ssize_t
Always return the number of bytes read or written within the framebuffer. Only return an errno code if framebuffer memory was not touched. This is the semantics required by POSIX and makes fb_read() and fb_write() compatible with IGT tests. [1] This bug has been fixed for fb_write() long ago by commit 6a2a88668e90 ("[PATCH] fbdev: Fix return error of fb_write"). The code in fb_read() and the corresponding fb_sys_() helpers was forgotten. Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de> Link: https://gitlab.freedesktop.org/drm/igt-gpu-tools/-/blob/master/tests/fbdev.c # 1 --- drivers/video/fbdev/core/fb_sys_fops.c | 24 ++++++++++++++---------- drivers/video/fbdev/core/fbmem.c | 2 +- 2 files changed, 15 insertions(+), 11 deletions(-)