From: Antonino A. D. <ad...@ho...> - 2004-08-30 02:47:23
|
Hi, This patch fixes a prblem reported by David S. Miller <da...@re...> "I just noticed that fb_{read,write}() uses copy_*_user() with the kernel buffer being the frame buffer. It needs to use the proper device address accessor functions." Tony Signed-off-by: Antonino Daplas <ad...@po...> --- diff -uprN linux-2.6.9-rc1-mm1-orig/drivers/video/fbmem.c linux-2.6.9-rc1-mm1/drivers/video/fbmem.c --- linux-2.6.9-rc1-mm1-orig/drivers/video/fbmem.c 2004-08-27 05:24:32.000000000 +0800 +++ linux-2.6.9-rc1-mm1/drivers/video/fbmem.c 2004-08-27 05:25:02.200341392 +0800 @@ -878,6 +878,8 @@ fb_read(struct file *file, char __user * struct inode *inode = file->f_dentry->d_inode; int fbidx = iminor(inode); struct fb_info *info = registered_fb[fbidx]; + u32 *buffer, *dst, *src; + int c, i, cnt = 0, err = 0; if (!info || ! info->screen_base) return -ENODEV; @@ -894,18 +896,40 @@ fb_read(struct file *file, char __user * count = info->fix.smem_len; if (count + p > info->fix.smem_len) count = info->fix.smem_len - p; + + cnt = 0; + buffer = kmalloc((count > 64 * 1024) ? 64 * 1024 : count, + GFP_KERNEL); + if (!buffer) + return -ENOMEM; + + src = (u32 *) (info->screen_base + p); + if (info->fbops->fb_sync) info->fbops->fb_sync(info); - if (count) { - char *base_addr; - - base_addr = info->screen_base; - count -= copy_to_user(buf, base_addr+p, count); - if (!count) - return -EFAULT; - *ppos += count; + + while (count) { + c = (count > 64 * 1024) ? 64 * 1024 : count; + dst = buffer; + for (i = c >> 2; i--; ) + *dst++ = fb_readl(src++); + if (c & 3) { + for (i = c & 3; i--;) + *((u8 *)dst++) = fb_readb((u8 *) src++); + } + + if (copy_to_user(buf, buffer, c)) { + err = -EFAULT; + break; + } + *ppos += c; + buf += c; + cnt += c; + count -= c; } - return count; + + kfree(buffer); + return (err) ? err : cnt; } static ssize_t @@ -915,7 +939,8 @@ fb_write(struct file *file, const char _ struct inode *inode = file->f_dentry->d_inode; int fbidx = iminor(inode); struct fb_info *info = registered_fb[fbidx]; - int err; + u32 *buffer, *dst, *src; + int c, i, cnt = 0, err; if (!info || !info->screen_base) return -ENODEV; @@ -935,19 +960,38 @@ fb_write(struct file *file, const char _ count = info->fix.smem_len - p; err = -ENOSPC; } + cnt = 0; + buffer = kmalloc((count > 64 * 1024) ? 64 * 1024 : count, + GFP_KERNEL); + if (!buffer) + return -ENOMEM; + + dst = (u32 *) (info->screen_base + p); + if (info->fbops->fb_sync) info->fbops->fb_sync(info); - if (count) { - char *base_addr; - base_addr = info->screen_base; - count -= copy_from_user(base_addr+p, buf, count); - *ppos += count; - err = -EFAULT; + while (count) { + c = (count > 64 * 1024) ? 64 * 1024 : count; + src = buffer; + if (copy_from_user(src, buf, c)) { + err = -EFAULT; + break; + } + for (i = c >> 2; i--; ) + fb_writel(*src++, dst++); + if (c & 3) { + for (i = c & 3; i--; ) + fb_writeb(*(u8 *)src++, (u8 *)dst++); + } + *ppos += c; + buf += c; + cnt += c; + count -= c; } - if (count) - return count; - return err; + kfree(buffer); + + return (err) ? err : cnt; } #ifdef CONFIG_KMOD |
From: David S. M. <da...@da...> - 2004-08-30 03:17:20
|
On Mon, 30 Aug 2004 10:47:28 +0800 "Antonino A. Daplas" <ad...@ho...> wrote: > This patch fixes a prblem reported by David S. Miller <da...@re...> > > "I just noticed that fb_{read,write}() uses copy_*_user() with > the kernel buffer being the frame buffer. It needs to use > the proper device address accessor functions." Looks great Tony, thanks for fixing this. |
From: Geert U. <ge...@li...> - 2004-08-30 09:10:40
|
On Mon, 30 Aug 2004, Antonino A. Daplas wrote: > This patch fixes a prblem reported by David S. Miller <da...@re...> > > "I just noticed that fb_{read,write}() uses copy_*_user() with > the kernel buffer being the frame buffer. It needs to use > the proper device address accessor functions." > > Tony > > Signed-off-by: Antonino Daplas <ad...@po...> > --- > > diff -uprN linux-2.6.9-rc1-mm1-orig/drivers/video/fbmem.c linux-2.6.9-rc1-mm1/drivers/video/fbmem.c > --- linux-2.6.9-rc1-mm1-orig/drivers/video/fbmem.c 2004-08-27 05:24:32.000000000 +0800 > +++ linux-2.6.9-rc1-mm1/drivers/video/fbmem.c 2004-08-27 05:25:02.200341392 +0800 > @@ -878,6 +878,8 @@ fb_read(struct file *file, char __user * > struct inode *inode = file->f_dentry->d_inode; > int fbidx = iminor(inode); > struct fb_info *info = registered_fb[fbidx]; > + u32 *buffer, *dst, *src; > + int c, i, cnt = 0, err = 0; > > if (!info || ! info->screen_base) > return -ENODEV; > @@ -894,18 +896,40 @@ fb_read(struct file *file, char __user * > count = info->fix.smem_len; > if (count + p > info->fix.smem_len) > count = info->fix.smem_len - p; > + > + cnt = 0; > + buffer = kmalloc((count > 64 * 1024) ? 64 * 1024 : count, ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > + GFP_KERNEL); > + if (!buffer) > + return -ENOMEM; Woops, what's the probability of a 64 kiB kmalloc() to fail? [...] > + while (count) { > + c = (count > 64 * 1024) ? 64 * 1024 : count; > + dst = buffer; > + for (i = c >> 2; i--; ) > + *dst++ = fb_readl(src++); > + if (c & 3) { > + for (i = c & 3; i--;) > + *((u8 *)dst++) = fb_readb((u8 *) src++); > + } > + > + if (copy_to_user(buf, buffer, c)) { > + err = -EFAULT; > + break; > + } > + *ppos += c; > + buf += c; > + cnt += c; > + count -= c; > } I don't expect the penalty for always using a buffer of PAGE_SIZE to be very large. Or am I missing something? Gr{oetje,eeting}s, Geert -- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@li... 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 |
From: Andrew M. <ak...@os...> - 2004-08-30 09:17:40
|
Geert Uytterhoeven <ge...@li...> wrote: > > > + > > + cnt = 0; > > + buffer = kmalloc((count > 64 * 1024) ? 64 * 1024 : count, > ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ > > + GFP_KERNEL); > > + if (!buffer) > > + return -ENOMEM; > > Woops, what's the probability of a 64 kiB kmalloc() to fail? Relatively high. > [...] > > > + while (count) { > > + c = (count > 64 * 1024) ? 64 * 1024 : count; > > + dst = buffer; > > + for (i = c >> 2; i--; ) > > + *dst++ = fb_readl(src++); > > + if (c & 3) { > > + for (i = c & 3; i--;) > > + *((u8 *)dst++) = fb_readb((u8 *) src++); > > + } > > + > > + if (copy_to_user(buf, buffer, c)) { > > + err = -EFAULT; > > + break; > > + } > > + *ppos += c; > > + buf += c; > > + cnt += c; > > + count -= c; > > } > > I don't expect the penalty for always using a buffer of PAGE_SIZE to be very > large. Or am I missing something? The 64k copy buffer will most likely be slower than using an 8k one. it depends, of course, upon the vintage of the CPU. 4k or 8k should be fine here. |
From: Antonino A. D. <ad...@ho...> - 2004-08-30 11:58:48
|
On Monday 30 August 2004 17:10, Geert Uytterhoeven wrote: > > I don't expect the penalty for always using a buffer of PAGE_SIZE to be > very large. Or am I missing something? > You're absolutely correct, there is no significant performance penalty whether using a small or large-sized buffer. Will resubmit a patch. Tony |