fb_write unaligned writes

fb_write unaligned writes

[James Hogan]

[-- Attachment #1: Type: text/plain, Size: 1027 bytes --]

Hi,

I came across a problem in fb_write (drivers/video/fbmem.c) which doesn't make 
any effort to avoid unaligned writes. It copies up to a page at a time from 
userspace into a buffer, then fb_writel's as much as possible from the buffer 
into the framebuffer, then fb_writeb's any remaining bytes. However on 
architectures which don't perform unaligned writes this can cause a bus error 
or silently fail when it tries to fb_writel to an unaligned framebuffer 
position.

My question is how best should this be fixed? I know I could avoid writing 
more than 3 bytes to an unaligned framebuffer offset, but the standard write 
syscall should be able to handle this, and Documentation/unaligned-memory-
access.txt makes it clear that unaligned accesses are to be avoided and with 
good reason.

Is it possible to just copy_from_user straight into the framebuffer, or should 
I define an fb_memcpy to go with the other definitions around line 925 of 
include/linux/fb.h, or something else?

Thanks
James

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: fb_write unaligned writes

[Florian Tobias Schandinat]

Hi,

James Hogan schrieb:
> Hi,
> 
> I came across a problem in fb_write (drivers/video/fbmem.c) which doesn't make 
> any effort to avoid unaligned writes. It copies up to a page at a time from 
> userspace into a buffer, then fb_writel's as much as possible from the buffer 
> into the framebuffer, then fb_writeb's any remaining bytes. However on 
> architectures which don't perform unaligned writes this can cause a bus error 
> or silently fail when it tries to fb_writel to an unaligned framebuffer 
> position.
> 
> My question is how best should this be fixed? I know I could avoid writing 
> more than 3 bytes to an unaligned framebuffer offset, but the standard write 
> syscall should be able to handle this, and Documentation/unaligned-memory-
> access.txt makes it clear that unaligned accesses are to be avoided and with 
> good reason.
> 
> Is it possible to just copy_from_user straight into the framebuffer, or should 
> I define an fb_memcpy to go with the other definitions around line 925 of 
> include/linux/fb.h, or something else?

Just doing copy_from_user straight into the framebuffer sounds like the right 
thing to me but my knowledge is limited to x86. As far as I can see this would 
make the functions (doing the same for fb_read can't hurt, can it?) in fbmem.c 
very similar to those in fb_sys_fops.c. Most users of the later functions can be 
easily eliminated as simply removing ".fb_{read,write} = fb_sys_{read,write}" is 
sufficient, only xen-fbfront.c looks more difficult as it also refreshes the 
screen so I'd suggest the following:
Merging fb_sys_fops.c in fbmem.c, adjusting the fb_{read,write} functions to use 
the the fb_sys_{read,write} copying and removing all trivial uses of 
fb_sys_{read,write}. As far as I can see this should solve your problem and 
would be a cleanup at the same time.
But it would be good if someone who knows other archs (especially sparc as they 
currently seem to perform some assembler that I can't decipher) could comment on 
this topic.


Thanks,

Florian Tobias Schandinat

[PATCH] fbmem: Fix fb_read, fb_write unaligned accesses.

[James Hogan]

Comments? I'd also appreciate if somebody familiar with sbus on sparc
could check this patch is sane since I know virtually nothing about sbus
and am not in a position to compile for sparc, let alone test on it:

fb_{read,write} access the framebuffer using lots of fb_{read,write}l's
but don't check that the file position is aligned which can cause
problems on some architectures which do not support unaligned accesses.

Since the operations are essentially memcpy_{from,to}io, new
fb_memcpy_{from,to}fb macros have been defined and these are used
instead.

For Sparc, fb_{read,write} macros use sbus_{read,write}, so this defines
new sbus_memcpy_{from,to}io functions the same as memcpy_{from,to}io but
using sbus_{read,write}b instead of {read,write}b.

Signed-off-by: James Hogan <james@albanarts.com>
---
 arch/sparc/include/asm/io_32.h |   31 +++++++++++++++++++++++++++
 arch/sparc/include/asm/io_64.h |   31 +++++++++++++++++++++++++++
 drivers/video/fbmem.c          |   46 ++++++++++++---------------------------
 include/linux/fb.h             |    6 +++++
 4 files changed, 82 insertions(+), 32 deletions(-)

diff --git a/arch/sparc/include/asm/io_32.h b/arch/sparc/include/asm/io_32.h
index 2889574..c2ced21 100644
--- a/arch/sparc/include/asm/io_32.h
+++ b/arch/sparc/include/asm/io_32.h
@@ -208,6 +208,21 @@ _memset_io(volatile void __iomem *dst, int c, __kernel_size_t n)
 #define memset_io(d,c,sz)	_memset_io(d,c,sz)
 
 static inline void
+_sbus_memcpy_fromio(void *dst, const volatile void __iomem *src,
+		    __kernel_size_t n)
+{
+	char *d = dst;
+
+	while (n--) {
+		char tmp = sbus_readb(src);
+		*d++ = tmp;
+		src++;
+	}
+}
+
+#define sbus_memcpy_fromio(d, s, sz)	_sbus_memcpy_fromio(d, s, sz)
+
+static inline void
 _memcpy_fromio(void *dst, const volatile void __iomem *src, __kernel_size_t n)
 {
 	char *d = dst;
@@ -222,6 +237,22 @@ _memcpy_fromio(void *dst, const volatile void __iomem *src, 
__kernel_size_t n)
 #define memcpy_fromio(d,s,sz)	_memcpy_fromio(d,s,sz)
 
 static inline void
+_sbus_memcpy_toio(volatile void __iomem *dst, const void *src,
+		  __kernel_size_t n)
+{
+	const char *s = src;
+	volatile void __iomem *d = dst;
+
+	while (n--) {
+		char tmp = *s++;
+		sbus_writeb(tmp, d);
+		d++;
+	}
+}
+
+#define sbus_memcpy_toio(d, s, sz)	_sbus_memcpy_toio(d, s, sz)
+
+static inline void
 _memcpy_toio(volatile void __iomem *dst, const void *src, __kernel_size_t n)
 {
 	const char *s = src;
diff --git a/arch/sparc/include/asm/io_64.h b/arch/sparc/include/asm/io_64.h
index 9517d06..9c89654 100644
--- a/arch/sparc/include/asm/io_64.h
+++ b/arch/sparc/include/asm/io_64.h
@@ -419,6 +419,21 @@ _memset_io(volatile void __iomem *dst, int c, __kernel_size_t n)
 #define memset_io(d,c,sz)	_memset_io(d,c,sz)
 
 static inline void
+_sbus_memcpy_fromio(void *dst, const volatile void __iomem *src,
+		    __kernel_size_t n)
+{
+	char *d = dst;
+
+	while (n--) {
+		char tmp = sbus_readb(src);
+		*d++ = tmp;
+		src++;
+	}
+}
+
+#define sbus_memcpy_fromio(d, s, sz)	_sbus_memcpy_fromio(d, s, sz)
+
+static inline void
 _memcpy_fromio(void *dst, const volatile void __iomem *src, __kernel_size_t n)
 {
 	char *d = dst;
@@ -433,6 +448,22 @@ _memcpy_fromio(void *dst, const volatile void __iomem *src, 
__kernel_size_t n)
 #define memcpy_fromio(d,s,sz)	_memcpy_fromio(d,s,sz)
 
 static inline void
+_sbus_memcpy_toio(volatile void __iomem *dst, const void *src,
+		  __kernel_size_t n)
+{
+	const char *s = src;
+	volatile void __iomem *d = dst;
+
+	while (n--) {
+		char tmp = *s++;
+		sbus_writeb(tmp, d);
+		d++;
+	}
+}
+
+#define sbus_memcpy_toio(d, s, sz)	_sbus_memcpy_toio(d, s, sz)
+
+static inline void
 _memcpy_toio(volatile void __iomem *dst, const void *src, __kernel_size_t n)
 {
 	const char *s = src;
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index b066475..925f25d 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -697,9 +697,9 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t 
*ppos)
 	struct inode *inode = file->f_path.dentry->d_inode;
 	int fbidx = iminor(inode);
 	struct fb_info *info = registered_fb[fbidx];
-	u32 *buffer, *dst;
-	u32 __iomem *src;
-	int c, i, cnt = 0, err = 0;
+	u8 *buffer, *dst;
+	u8 __iomem *src;
+	int c, cnt = 0, err = 0;
 	unsigned long total_size;
 
 	if (!info || ! info->screen_base)
@@ -730,7 +730,7 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t 
*ppos)
 	if (!buffer)
 		return -ENOMEM;
 
-	src = (u32 __iomem *) (info->screen_base + p);
+	src = (u8 __iomem *) (info->screen_base + p);
 
 	if (info->fbops->fb_sync)
 		info->fbops->fb_sync(info);
@@ -738,17 +738,9 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t 
*ppos)
 	while (count) {
 		c  = (count > PAGE_SIZE) ? PAGE_SIZE : count;
 		dst = buffer;
-		for (i = c >> 2; i--; )
-			*dst++ = fb_readl(src++);
-		if (c & 3) {
-			u8 *dst8 = (u8 *) dst;
-			u8 __iomem *src8 = (u8 __iomem *) src;
-
-			for (i = c & 3; i--;)
-				*dst8++ = fb_readb(src8++);
-
-			src = (u32 __iomem *) src8;
-		}
+		fb_memcpy_fromfb(dst, src, c);
+		dst += c;
+		src += c;
 
 		if (copy_to_user(buf, buffer, c)) {
 			err = -EFAULT;
@@ -772,9 +764,9 @@ fb_write(struct file *file, const char __user *buf, size_t count, 
loff_t *ppos)
 	struct inode *inode = file->f_path.dentry->d_inode;
 	int fbidx = iminor(inode);
 	struct fb_info *info = registered_fb[fbidx];
-	u32 *buffer, *src;
-	u32 __iomem *dst;
-	int c, i, cnt = 0, err = 0;
+	u8 *buffer, *src;
+	u8 __iomem *dst;
+	int c, cnt = 0, err = 0;
 	unsigned long total_size;
 
 	if (!info || !info->screen_base)
@@ -811,7 +803,7 @@ fb_write(struct file *file, const char __user *buf, size_t count, 
loff_t *ppos)
 	if (!buffer)
 		return -ENOMEM;
 
-	dst = (u32 __iomem *) (info->screen_base + p);
+	dst = (u8 __iomem *) (info->screen_base + p);
 
 	if (info->fbops->fb_sync)
 		info->fbops->fb_sync(info);
@@ -825,19 +817,9 @@ fb_write(struct file *file, const char __user *buf, size_t count, 
loff_t *ppos)
 			break;
 		}
 
-		for (i = c >> 2; i--; )
-			fb_writel(*src++, dst++);
-
-		if (c & 3) {
-			u8 *src8 = (u8 *) src;
-			u8 __iomem *dst8 = (u8 __iomem *) dst;
-
-			for (i = c & 3; i--; )
-				fb_writeb(*src8++, dst8++);
-
-			dst = (u32 __iomem *) dst8;
-		}
-
+		fb_memcpy_tofb(dst, src, c);
+		dst += c;
+		src += c;
 		*ppos += c;
 		buf += c;
 		cnt += c;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index f0268de..7fca3dc 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -931,6 +931,8 @@ static inline struct apertures_struct *alloc_apertures(unsigned int 
max_num) {
 #define fb_writel sbus_writel
 #define fb_writeq sbus_writeq
 #define fb_memset sbus_memset_io
+#define fb_memcpy_fromfb sbus_memcpy_fromio
+#define fb_memcpy_tofb sbus_memcpy_toio
 
 #elif defined(__i386__) || defined(__alpha__) || defined(__x86_64__) || 
defined(__hppa__) || defined(__sh__) || defined(__powerpc__) || defined(__avr32__) || 
defined(__bfin__)
 
@@ -943,6 +945,8 @@ static inline struct apertures_struct *alloc_apertures(unsigned int 
max_num) {
 #define fb_writel __raw_writel
 #define fb_writeq __raw_writeq
 #define fb_memset memset_io
+#define fb_memcpy_fromfb memcpy_fromio
+#define fb_memcpy_tofb memcpy_toio
 
 #else
 
@@ -955,6 +959,8 @@ static inline struct apertures_struct *alloc_apertures(unsigned int 
max_num) {
 #define fb_writel(b,addr) (*(volatile u32 *) (addr) = (b))
 #define fb_writeq(b,addr) (*(volatile u64 *) (addr) = (b))
 #define fb_memset memset
+#define fb_memcpy_fromfb memcpy
+#define fb_memcpy_tofb memcpy
 
 #endif
 
-- 
1.7.2.3

Re: [PATCH] fbmem: Fix fb_read, fb_write unaligned accesses.

[David Miller]

Nobody can test your patch because your email client corrupted
it, by breaking up long lines.

[PATCH] fbmem: Fix fb_read, fb_write unaligned accesses.

[James Hogan]

Apologies for corrupted patch. I'll try again.
Comments? I'd also appreciate if somebody familiar with sbus on sparc
could check this patch is sane since I know virtually nothing about sbus
and am not in a position to compile for sparc, let alone test on it:

fb_{read,write} access the framebuffer using lots of fb_{read,write}l's
but don't check that the file position is aligned which can cause
problems on some architectures which do not support unaligned accesses.

Since the operations are essentially memcpy_{from,to}io, new
fb_memcpy_{from,to}fb macros have been defined and these are used
instead.

For Sparc, fb_{read,write} macros use sbus_{read,write}, so this defines
new sbus_memcpy_{from,to}io functions the same as memcpy_{from,to}io but
using sbus_{read,write}b instead of {read,write}b.

Signed-off-by: James Hogan <james@albanarts.com>
---
 arch/sparc/include/asm/io_32.h |   31 +++++++++++++++++++++++++++
 arch/sparc/include/asm/io_64.h |   31 +++++++++++++++++++++++++++
 drivers/video/fbmem.c          |   46 ++++++++++++---------------------------
 include/linux/fb.h             |    6 +++++
 4 files changed, 82 insertions(+), 32 deletions(-)

diff --git a/arch/sparc/include/asm/io_32.h b/arch/sparc/include/asm/io_32.h
index 2889574..c2ced21 100644
--- a/arch/sparc/include/asm/io_32.h
+++ b/arch/sparc/include/asm/io_32.h
@@ -208,6 +208,21 @@ _memset_io(volatile void __iomem *dst, int c, __kernel_size_t n)
 #define memset_io(d,c,sz)	_memset_io(d,c,sz)
 
 static inline void
+_sbus_memcpy_fromio(void *dst, const volatile void __iomem *src,
+		    __kernel_size_t n)
+{
+	char *d = dst;
+
+	while (n--) {
+		char tmp = sbus_readb(src);
+		*d++ = tmp;
+		src++;
+	}
+}
+
+#define sbus_memcpy_fromio(d, s, sz)	_sbus_memcpy_fromio(d, s, sz)
+
+static inline void
 _memcpy_fromio(void *dst, const volatile void __iomem *src, __kernel_size_t n)
 {
 	char *d = dst;
@@ -222,6 +237,22 @@ _memcpy_fromio(void *dst, const volatile void __iomem *src, __kernel_size_t n)
 #define memcpy_fromio(d,s,sz)	_memcpy_fromio(d,s,sz)
 
 static inline void
+_sbus_memcpy_toio(volatile void __iomem *dst, const void *src,
+		  __kernel_size_t n)
+{
+	const char *s = src;
+	volatile void __iomem *d = dst;
+
+	while (n--) {
+		char tmp = *s++;
+		sbus_writeb(tmp, d);
+		d++;
+	}
+}
+
+#define sbus_memcpy_toio(d, s, sz)	_sbus_memcpy_toio(d, s, sz)
+
+static inline void
 _memcpy_toio(volatile void __iomem *dst, const void *src, __kernel_size_t n)
 {
 	const char *s = src;
diff --git a/arch/sparc/include/asm/io_64.h b/arch/sparc/include/asm/io_64.h
index 9517d06..9c89654 100644
--- a/arch/sparc/include/asm/io_64.h
+++ b/arch/sparc/include/asm/io_64.h
@@ -419,6 +419,21 @@ _memset_io(volatile void __iomem *dst, int c, __kernel_size_t n)
 #define memset_io(d,c,sz)	_memset_io(d,c,sz)
 
 static inline void
+_sbus_memcpy_fromio(void *dst, const volatile void __iomem *src,
+		    __kernel_size_t n)
+{
+	char *d = dst;
+
+	while (n--) {
+		char tmp = sbus_readb(src);
+		*d++ = tmp;
+		src++;
+	}
+}
+
+#define sbus_memcpy_fromio(d, s, sz)	_sbus_memcpy_fromio(d, s, sz)
+
+static inline void
 _memcpy_fromio(void *dst, const volatile void __iomem *src, __kernel_size_t n)
 {
 	char *d = dst;
@@ -433,6 +448,22 @@ _memcpy_fromio(void *dst, const volatile void __iomem *src, __kernel_size_t n)
 #define memcpy_fromio(d,s,sz)	_memcpy_fromio(d,s,sz)
 
 static inline void
+_sbus_memcpy_toio(volatile void __iomem *dst, const void *src,
+		  __kernel_size_t n)
+{
+	const char *s = src;
+	volatile void __iomem *d = dst;
+
+	while (n--) {
+		char tmp = *s++;
+		sbus_writeb(tmp, d);
+		d++;
+	}
+}
+
+#define sbus_memcpy_toio(d, s, sz)	_sbus_memcpy_toio(d, s, sz)
+
+static inline void
 _memcpy_toio(volatile void __iomem *dst, const void *src, __kernel_size_t n)
 {
 	const char *s = src;
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index b066475..925f25d 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -697,9 +697,9 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	struct inode *inode = file->f_path.dentry->d_inode;
 	int fbidx = iminor(inode);
 	struct fb_info *info = registered_fb[fbidx];
-	u32 *buffer, *dst;
-	u32 __iomem *src;
-	int c, i, cnt = 0, err = 0;
+	u8 *buffer, *dst;
+	u8 __iomem *src;
+	int c, cnt = 0, err = 0;
 	unsigned long total_size;
 
 	if (!info || ! info->screen_base)
@@ -730,7 +730,7 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	if (!buffer)
 		return -ENOMEM;
 
-	src = (u32 __iomem *) (info->screen_base + p);
+	src = (u8 __iomem *) (info->screen_base + p);
 
 	if (info->fbops->fb_sync)
 		info->fbops->fb_sync(info);
@@ -738,17 +738,9 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
 	while (count) {
 		c  = (count > PAGE_SIZE) ? PAGE_SIZE : count;
 		dst = buffer;
-		for (i = c >> 2; i--; )
-			*dst++ = fb_readl(src++);
-		if (c & 3) {
-			u8 *dst8 = (u8 *) dst;
-			u8 __iomem *src8 = (u8 __iomem *) src;
-
-			for (i = c & 3; i--;)
-				*dst8++ = fb_readb(src8++);
-
-			src = (u32 __iomem *) src8;
-		}
+		fb_memcpy_fromfb(dst, src, c);
+		dst += c;
+		src += c;
 
 		if (copy_to_user(buf, buffer, c)) {
 			err = -EFAULT;
@@ -772,9 +764,9 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 	struct inode *inode = file->f_path.dentry->d_inode;
 	int fbidx = iminor(inode);
 	struct fb_info *info = registered_fb[fbidx];
-	u32 *buffer, *src;
-	u32 __iomem *dst;
-	int c, i, cnt = 0, err = 0;
+	u8 *buffer, *src;
+	u8 __iomem *dst;
+	int c, cnt = 0, err = 0;
 	unsigned long total_size;
 
 	if (!info || !info->screen_base)
@@ -811,7 +803,7 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 	if (!buffer)
 		return -ENOMEM;
 
-	dst = (u32 __iomem *) (info->screen_base + p);
+	dst = (u8 __iomem *) (info->screen_base + p);
 
 	if (info->fbops->fb_sync)
 		info->fbops->fb_sync(info);
@@ -825,19 +817,9 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
 			break;
 		}
 
-		for (i = c >> 2; i--; )
-			fb_writel(*src++, dst++);
-
-		if (c & 3) {
-			u8 *src8 = (u8 *) src;
-			u8 __iomem *dst8 = (u8 __iomem *) dst;
-
-			for (i = c & 3; i--; )
-				fb_writeb(*src8++, dst8++);
-
-			dst = (u32 __iomem *) dst8;
-		}
-
+		fb_memcpy_tofb(dst, src, c);
+		dst += c;
+		src += c;
 		*ppos += c;
 		buf += c;
 		cnt += c;
diff --git a/include/linux/fb.h b/include/linux/fb.h
index f0268de..7fca3dc 100644
--- a/include/linux/fb.h
+++ b/include/linux/fb.h
@@ -931,6 +931,8 @@ static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {
 #define fb_writel sbus_writel
 #define fb_writeq sbus_writeq
 #define fb_memset sbus_memset_io
+#define fb_memcpy_fromfb sbus_memcpy_fromio
+#define fb_memcpy_tofb sbus_memcpy_toio
 
 #elif defined(__i386__) || defined(__alpha__) || defined(__x86_64__) || defined(__hppa__) || defined(__sh__) || defined(__powerpc__) || defined(__avr32__) || defined(__bfin__)
 
@@ -943,6 +945,8 @@ static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {
 #define fb_writel __raw_writel
 #define fb_writeq __raw_writeq
 #define fb_memset memset_io
+#define fb_memcpy_fromfb memcpy_fromio
+#define fb_memcpy_tofb memcpy_toio
 
 #else
 
@@ -955,6 +959,8 @@ static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {
 #define fb_writel(b,addr) (*(volatile u32 *) (addr) = (b))
 #define fb_writeq(b,addr) (*(volatile u64 *) (addr) = (b))
 #define fb_memset memset
+#define fb_memcpy_fromfb memcpy
+#define fb_memcpy_tofb memcpy
 
 #endif
 
-- 
1.7.2.3

Re: [PATCH] fbmem: Fix fb_read, fb_write unaligned accesses.

[David Miller]

From: James Hogan <james@albanarts.com>
Date: Sat, 18 Sep 2010 01:23:47 +0100

> Apologies for corrupted patch. I'll try again.
> Comments? I'd also appreciate if somebody familiar with sbus on sparc
> could check this patch is sane since I know virtually nothing about sbus
> and am not in a position to compile for sparc, let alone test on it:
> 
> fb_{read,write} access the framebuffer using lots of fb_{read,write}l's
> but don't check that the file position is aligned which can cause
> problems on some architectures which do not support unaligned accesses.
> 
> Since the operations are essentially memcpy_{from,to}io, new
> fb_memcpy_{from,to}fb macros have been defined and these are used
> instead.
> 
> For Sparc, fb_{read,write} macros use sbus_{read,write}, so this defines
> new sbus_memcpy_{from,to}io functions the same as memcpy_{from,to}io but
> using sbus_{read,write}b instead of {read,write}b.
> 
> Signed-off-by: James Hogan <james@albanarts.com>

Compiles cleanly on sparc and looks correct to me:

Acked-by: David S. Miller <davem@davemloft.net>

Re: [PATCH] fbmem: Fix fb_read, fb_write unaligned accesses.

[Florian Tobias Schandinat]

James Hogan schrieb:
> Apologies for corrupted patch. I'll try again.
> Comments? I'd also appreciate if somebody familiar with sbus on sparc
> could check this patch is sane since I know virtually nothing about sbus
> and am not in a position to compile for sparc, let alone test on it:
> 
> fb_{read,write} access the framebuffer using lots of fb_{read,write}l's
> but don't check that the file position is aligned which can cause
> problems on some architectures which do not support unaligned accesses.
> 
> Since the operations are essentially memcpy_{from,to}io, new
> fb_memcpy_{from,to}fb macros have been defined and these are used
> instead.
> 
> For Sparc, fb_{read,write} macros use sbus_{read,write}, so this defines
> new sbus_memcpy_{from,to}io functions the same as memcpy_{from,to}io but
> using sbus_{read,write}b instead of {read,write}b.
> 
> Signed-off-by: James Hogan <james@albanarts.com>

Looks correct and is an improvement.
I think it wouldn't be bad in terms of performance if we could avoid the double 
copy operation altogether but it might be overkill to add an 
fb_memcpy_{toio_from_user,fromio_to_user} just for that. But that's something 
that could be done in a later patch if anyone considers it worth the effort (I 
think most users just mmap the framebuffer anyway).

Acked-by: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>

> ---
>  arch/sparc/include/asm/io_32.h |   31 +++++++++++++++++++++++++++
>  arch/sparc/include/asm/io_64.h |   31 +++++++++++++++++++++++++++
>  drivers/video/fbmem.c          |   46 ++++++++++++---------------------------
>  include/linux/fb.h             |    6 +++++
>  4 files changed, 82 insertions(+), 32 deletions(-)
> 
> diff --git a/arch/sparc/include/asm/io_32.h b/arch/sparc/include/asm/io_32.h
> index 2889574..c2ced21 100644
> --- a/arch/sparc/include/asm/io_32.h
> +++ b/arch/sparc/include/asm/io_32.h
> @@ -208,6 +208,21 @@ _memset_io(volatile void __iomem *dst, int c, __kernel_size_t n)
>  #define memset_io(d,c,sz)	_memset_io(d,c,sz)
>  
>  static inline void
> +_sbus_memcpy_fromio(void *dst, const volatile void __iomem *src,
> +		    __kernel_size_t n)
> +{
> +	char *d = dst;
> +
> +	while (n--) {
> +		char tmp = sbus_readb(src);
> +		*d++ = tmp;
> +		src++;
> +	}
> +}
> +
> +#define sbus_memcpy_fromio(d, s, sz)	_sbus_memcpy_fromio(d, s, sz)
> +
> +static inline void
>  _memcpy_fromio(void *dst, const volatile void __iomem *src, __kernel_size_t n)
>  {
>  	char *d = dst;
> @@ -222,6 +237,22 @@ _memcpy_fromio(void *dst, const volatile void __iomem *src, __kernel_size_t n)
>  #define memcpy_fromio(d,s,sz)	_memcpy_fromio(d,s,sz)
>  
>  static inline void
> +_sbus_memcpy_toio(volatile void __iomem *dst, const void *src,
> +		  __kernel_size_t n)
> +{
> +	const char *s = src;
> +	volatile void __iomem *d = dst;
> +
> +	while (n--) {
> +		char tmp = *s++;
> +		sbus_writeb(tmp, d);
> +		d++;
> +	}
> +}
> +
> +#define sbus_memcpy_toio(d, s, sz)	_sbus_memcpy_toio(d, s, sz)
> +
> +static inline void
>  _memcpy_toio(volatile void __iomem *dst, const void *src, __kernel_size_t n)
>  {
>  	const char *s = src;
> diff --git a/arch/sparc/include/asm/io_64.h b/arch/sparc/include/asm/io_64.h
> index 9517d06..9c89654 100644
> --- a/arch/sparc/include/asm/io_64.h
> +++ b/arch/sparc/include/asm/io_64.h
> @@ -419,6 +419,21 @@ _memset_io(volatile void __iomem *dst, int c, __kernel_size_t n)
>  #define memset_io(d,c,sz)	_memset_io(d,c,sz)
>  
>  static inline void
> +_sbus_memcpy_fromio(void *dst, const volatile void __iomem *src,
> +		    __kernel_size_t n)
> +{
> +	char *d = dst;
> +
> +	while (n--) {
> +		char tmp = sbus_readb(src);
> +		*d++ = tmp;
> +		src++;
> +	}
> +}
> +
> +#define sbus_memcpy_fromio(d, s, sz)	_sbus_memcpy_fromio(d, s, sz)
> +
> +static inline void
>  _memcpy_fromio(void *dst, const volatile void __iomem *src, __kernel_size_t n)
>  {
>  	char *d = dst;
> @@ -433,6 +448,22 @@ _memcpy_fromio(void *dst, const volatile void __iomem *src, __kernel_size_t n)
>  #define memcpy_fromio(d,s,sz)	_memcpy_fromio(d,s,sz)
>  
>  static inline void
> +_sbus_memcpy_toio(volatile void __iomem *dst, const void *src,
> +		  __kernel_size_t n)
> +{
> +	const char *s = src;
> +	volatile void __iomem *d = dst;
> +
> +	while (n--) {
> +		char tmp = *s++;
> +		sbus_writeb(tmp, d);
> +		d++;
> +	}
> +}
> +
> +#define sbus_memcpy_toio(d, s, sz)	_sbus_memcpy_toio(d, s, sz)
> +
> +static inline void
>  _memcpy_toio(volatile void __iomem *dst, const void *src, __kernel_size_t n)
>  {
>  	const char *s = src;
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index b066475..925f25d 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -697,9 +697,9 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>  	struct inode *inode = file->f_path.dentry->d_inode;
>  	int fbidx = iminor(inode);
>  	struct fb_info *info = registered_fb[fbidx];
> -	u32 *buffer, *dst;
> -	u32 __iomem *src;
> -	int c, i, cnt = 0, err = 0;
> +	u8 *buffer, *dst;
> +	u8 __iomem *src;
> +	int c, cnt = 0, err = 0;
>  	unsigned long total_size;
>  
>  	if (!info || ! info->screen_base)
> @@ -730,7 +730,7 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>  	if (!buffer)
>  		return -ENOMEM;
>  
> -	src = (u32 __iomem *) (info->screen_base + p);
> +	src = (u8 __iomem *) (info->screen_base + p);
>  
>  	if (info->fbops->fb_sync)
>  		info->fbops->fb_sync(info);
> @@ -738,17 +738,9 @@ fb_read(struct file *file, char __user *buf, size_t count, loff_t *ppos)
>  	while (count) {
>  		c  = (count > PAGE_SIZE) ? PAGE_SIZE : count;
>  		dst = buffer;
> -		for (i = c >> 2; i--; )
> -			*dst++ = fb_readl(src++);
> -		if (c & 3) {
> -			u8 *dst8 = (u8 *) dst;
> -			u8 __iomem *src8 = (u8 __iomem *) src;
> -
> -			for (i = c & 3; i--;)
> -				*dst8++ = fb_readb(src8++);
> -
> -			src = (u32 __iomem *) src8;
> -		}
> +		fb_memcpy_fromfb(dst, src, c);
> +		dst += c;
> +		src += c;
>  
>  		if (copy_to_user(buf, buffer, c)) {
>  			err = -EFAULT;
> @@ -772,9 +764,9 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
>  	struct inode *inode = file->f_path.dentry->d_inode;
>  	int fbidx = iminor(inode);
>  	struct fb_info *info = registered_fb[fbidx];
> -	u32 *buffer, *src;
> -	u32 __iomem *dst;
> -	int c, i, cnt = 0, err = 0;
> +	u8 *buffer, *src;
> +	u8 __iomem *dst;
> +	int c, cnt = 0, err = 0;
>  	unsigned long total_size;
>  
>  	if (!info || !info->screen_base)
> @@ -811,7 +803,7 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
>  	if (!buffer)
>  		return -ENOMEM;
>  
> -	dst = (u32 __iomem *) (info->screen_base + p);
> +	dst = (u8 __iomem *) (info->screen_base + p);
>  
>  	if (info->fbops->fb_sync)
>  		info->fbops->fb_sync(info);
> @@ -825,19 +817,9 @@ fb_write(struct file *file, const char __user *buf, size_t count, loff_t *ppos)
>  			break;
>  		}
>  
> -		for (i = c >> 2; i--; )
> -			fb_writel(*src++, dst++);
> -
> -		if (c & 3) {
> -			u8 *src8 = (u8 *) src;
> -			u8 __iomem *dst8 = (u8 __iomem *) dst;
> -
> -			for (i = c & 3; i--; )
> -				fb_writeb(*src8++, dst8++);
> -
> -			dst = (u32 __iomem *) dst8;
> -		}
> -
> +		fb_memcpy_tofb(dst, src, c);
> +		dst += c;
> +		src += c;
>  		*ppos += c;
>  		buf += c;
>  		cnt += c;
> diff --git a/include/linux/fb.h b/include/linux/fb.h
> index f0268de..7fca3dc 100644
> --- a/include/linux/fb.h
> +++ b/include/linux/fb.h
> @@ -931,6 +931,8 @@ static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {
>  #define fb_writel sbus_writel
>  #define fb_writeq sbus_writeq
>  #define fb_memset sbus_memset_io
> +#define fb_memcpy_fromfb sbus_memcpy_fromio
> +#define fb_memcpy_tofb sbus_memcpy_toio
>  
>  #elif defined(__i386__) || defined(__alpha__) || defined(__x86_64__) || defined(__hppa__) || defined(__sh__) || defined(__powerpc__) || defined(__avr32__) || defined(__bfin__)
>  
> @@ -943,6 +945,8 @@ static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {
>  #define fb_writel __raw_writel
>  #define fb_writeq __raw_writeq
>  #define fb_memset memset_io
> +#define fb_memcpy_fromfb memcpy_fromio
> +#define fb_memcpy_tofb memcpy_toio
>  
>  #else
>  
> @@ -955,6 +959,8 @@ static inline struct apertures_struct *alloc_apertures(unsigned int max_num) {
>  #define fb_writel(b,addr) (*(volatile u32 *) (addr) = (b))
>  #define fb_writeq(b,addr) (*(volatile u64 *) (addr) = (b))
>  #define fb_memset memset
> +#define fb_memcpy_fromfb memcpy
> +#define fb_memcpy_tofb memcpy
>  
>  #endif
>  

Re: [PATCH] fbmem: Fix fb_read, fb_write unaligned accesses.

[Andrew Morton]

On Sat, 18 Sep 2010 01:23:47 +0100
James Hogan <james@albanarts.com> wrote:

> Apologies for corrupted patch. I'll try again.
> Comments? I'd also appreciate if somebody familiar with sbus on sparc
> could check this patch is sane since I know virtually nothing about sbus
> and am not in a position to compile for sparc, let alone test on it:
> 
> fb_{read,write} access the framebuffer using lots of fb_{read,write}l's
> but don't check that the file position is aligned which can cause
> problems on some architectures which do not support unaligned accesses.

What are these "problems"?

I'd have thought they would be fairly fatal, in which case this is a
high-priority patch.  But I'd also have thought that the problems would
have been noted before now.

So I assume that you're doing something which nobody has done before.

Confused.  Help?

> Since the operations are essentially memcpy_{from,to}io, new
> fb_memcpy_{from,to}fb macros have been defined and these are used
> instead.
> 
> For Sparc, fb_{read,write} macros use sbus_{read,write}, so this defines
> new sbus_memcpy_{from,to}io functions the same as memcpy_{from,to}io but
> using sbus_{read,write}b instead of {read,write}b.

Re: [PATCH] fbmem: Fix fb_read, fb_write unaligned accesses.

[James Hogan]

[-- Attachment #1: Type: Text/Plain, Size: 2217 bytes --]

On Wednesday 22 September 2010 00:19:50 Andrew Morton wrote:
> On Sat, 18 Sep 2010 01:23:47 +0100
> 
> James Hogan <james@albanarts.com> wrote:
> > Apologies for corrupted patch. I'll try again.
> > Comments? I'd also appreciate if somebody familiar with sbus on sparc
> > could check this patch is sane since I know virtually nothing about sbus
> > and am not in a position to compile for sparc, let alone test on it:
> > 
> > fb_{read,write} access the framebuffer using lots of fb_{read,write}l's
> > but don't check that the file position is aligned which can cause
> > problems on some architectures which do not support unaligned accesses.
> 
> What are these "problems"?

On the arch I hit this on (which isn't in tree) I experienced a fault at the 
point of the unaligned write in kernel code, but this is only because 
unaligned access checking is switched on which isn't always possible 
(otherwise the write would have just silently failed/done something else). It 
terminated the program, but didn't cause any other problems.
Documentation/unaligned-memory-access.txt also mentions that some 
architectures raise exceptions that can't be worked around, or fail silently 
and may actually perform different writes to the one requested.

> I'd have thought they would be fairly fatal, in which case this is a
> high-priority patch.  But I'd also have thought that the problems would
> have been noted before now.

The common way to access the framebuffer is by mmapping it into userland, so I 
don't think the read/write syscalls are commonly used on it, and in any case 
colour data tends to be naturally aligned so unaligned writes would be 
uncommon.

> 
> So I assume that you're doing something which nobody has done before.
> 
> Confused.  Help?

The actualy case that hit this is admitedly rather silly. I did a quick test 
of the framebuffer by typing this at a shell:
yes > /dev/fb0
I'd have to check it again to see exactly how lots of 2 byte writes ("y\n") 
ended up unaligned, but at the point of the crash the dest pointer was 
definitely unaligned (the hex address ended in a 5) and the patch fixed it for 
me.

Hope that helps.

Cheers
James

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: fb_write unaligned writes

[James Hogan]

[-- Attachment #1: Type: Text/Plain, Size: 1973 bytes --]

Thanks for the response Florian,

> > I came across a problem in fb_write (drivers/video/fbmem.c) which doesn't
> > make any effort to avoid unaligned writes. It copies up to a page at a
> > time from userspace into a buffer, then fb_writel's as much as possible
> > from the buffer into the framebuffer, then fb_writeb's any remaining
> > bytes. However on architectures which don't perform unaligned writes
> > this can cause a bus error or silently fail when it tries to fb_writel
> > to an unaligned framebuffer position.
> > 
> > My question is how best should this be fixed? I know I could avoid
> > writing more than 3 bytes to an unaligned framebuffer offset, but the
> > standard write syscall should be able to handle this, and
> > Documentation/unaligned-memory- access.txt makes it clear that unaligned
> > accesses are to be avoided and with good reason.
> > 
> > Is it possible to just copy_from_user straight into the framebuffer, or
> > should I define an fb_memcpy to go with the other definitions around
> > line 925 of include/linux/fb.h, or something else?
> 
> Just doing copy_from_user straight into the framebuffer sounds like the
> right thing to me but my knowledge is limited to x86. As far as I can see
> this would make the functions (doing the same for fb_read can't hurt, can
> it?) in fbmem.c very similar to those in fb_sys_fops.c. Most users of the
> later functions can be easily eliminated as simply removing
> ".fb_{read,write} = fb_sys_{read,write}" is sufficient, only xen-fbfront.c
> looks more difficult as it also refreshes the screen so I'd suggest the
> following:

Yes, I noticed fb_read has the same issue after I sent the first email.
Having looked a bit deeper though I'm not so sure copy_from_user straight into 
iomem is a good idea, since as far as I can see iomem isn't necessarily 
readable or writeable in the normal way (c pointer dereferences) on all 
architectures.

Cheers
James

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]