From: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
To: James Hogan <james@albanarts.com>
Cc: sparclinux@vger.kernel.org, linux-kernel@vger.kernel.org,
linux-fbdev@vger.kernel.org,
"David S. Miller" <davem@davemloft.net>,
Dave Airlie <airlied@redhat.com>,
Andrew Morton <akpm@linux-foundation.org>,
Marcin Slusarz <marcin.slusarz@gmail.com>,
Denys Vlasenko <vda.linux@googlemail.com>,
Jesse Barnes <jbarnes@virtuousgeek.org>,
James Simmons <jsimmons@infradead.org>
Subject: Re: [PATCH] fbmem: Fix fb_read, fb_write unaligned accesses.
Date: Mon, 20 Sep 2010 19:27:21 +0000 [thread overview]
Message-ID: <4C97B599.8080207@gmx.de> (raw)
In-Reply-To: <201009180123.48303.james@albanarts.com>
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
>
next prev parent reply other threads:[~2010-09-20 19:27 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
2010-09-09 22:38 fb_write unaligned writes James Hogan
2010-09-17 18:11 ` Florian Tobias Schandinat
2010-09-17 23:57 ` [PATCH] fbmem: Fix fb_read, fb_write unaligned accesses James Hogan
2010-09-18 0:15 ` David Miller
2010-09-18 0:23 ` James Hogan
2010-09-18 3:17 ` David Miller
2010-09-20 19:27 ` Florian Tobias Schandinat [this message]
2010-09-21 23:19 ` Andrew Morton
2010-09-22 0:00 ` James Hogan
2010-09-18 0:11 ` fb_write unaligned writes James Hogan
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=4C97B599.8080207@gmx.de \
--to=florianschandinat@gmx.de \
--cc=airlied@redhat.com \
--cc=akpm@linux-foundation.org \
--cc=davem@davemloft.net \
--cc=james@albanarts.com \
--cc=jbarnes@virtuousgeek.org \
--cc=jsimmons@infradead.org \
--cc=linux-fbdev@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=marcin.slusarz@gmail.com \
--cc=sparclinux@vger.kernel.org \
--cc=vda.linux@googlemail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).