* [PATCH] framebuffer: fix screen corruption when copying
@ 2014-09-16 16:38 ` Mikulas Patocka
0 siblings, 0 replies; 8+ messages in thread
From: Mikulas Patocka @ 2014-09-16 16:38 UTC (permalink / raw)
To: Tomi Valkeinen, Jean-Christophe Plagniol-Villard
Cc: linux-fbdev, linux-kernel
The function bitcpy_rev has a bug that may result in screen corruption.
The bug happens under these conditions:
* the end of the destination area of a copy operation is aligned on a long
word boundary
* the end of the source area is not aligned on a long word boundary
* we are copying more than one long word
In this case, the variable shift is non-zero and the variable first is
zero. The statements FB_WRITEL(comp(d0, FB_READL(dst), first), dst) reads
the last long word of the destination and writes it back unchanged
(because first is zero). Correctly, we should write the variable d0 to the
last word of the destination in this case.
This patch fixes the bug by introducing and extra test if first is zero.
The patch also removes the references to fb_memmove in the code that is
commented out because fb_memmove was removed from framebuffer subsystem.
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org
Index: linux-3.16.1/drivers/video/fbdev/core/cfbcopyarea.c
=================================--- linux-3.16.1.orig/drivers/video/cfbcopyarea.c
+++ linux-3.16.1/drivers/video/fbdev/core/cfbcopyarea.c
@@ -55,8 +55,8 @@ bitcpy(struct fb_info *p, unsigned long
* If you suspect bug in this function, compare it with this simple
* memmove implementation.
*/
- fb_memmove((char *)dst + ((dst_idx & (bits - 1))) / 8,
- (char *)src + ((src_idx & (bits - 1))) / 8, n / 8);
+ memmove((char *)dst + ((dst_idx & (bits - 1))) / 8,
+ (char *)src + ((src_idx & (bits - 1))) / 8, n / 8);
return;
#endif
@@ -221,8 +221,8 @@ bitcpy_rev(struct fb_info *p, unsigned l
* If you suspect bug in this function, compare it with this simple
* memmove implementation.
*/
- fb_memmove((char *)dst + ((dst_idx & (bits - 1))) / 8,
- (char *)src + ((src_idx & (bits - 1))) / 8, n / 8);
+ memmove((char *)dst + ((dst_idx & (bits - 1))) / 8,
+ (char *)src + ((src_idx & (bits - 1))) / 8, n / 8);
return;
#endif
@@ -324,7 +324,10 @@ bitcpy_rev(struct fb_info *p, unsigned l
d0 = d0 << left | d1 >> right;
}
d0 = fb_rev_pixels_in_long(d0, bswapmask);
- FB_WRITEL(comp(d0, FB_READL(dst), first), dst);
+ if (!first)
+ FB_WRITEL(d0, dst);
+ else
+ FB_WRITEL(comp(d0, FB_READL(dst), first), dst);
d0 = d1;
dst--;
n -= dst_idx+1;
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] framebuffer: fix screen corruption when copying
@ 2014-09-16 16:38 ` Mikulas Patocka
0 siblings, 0 replies; 8+ messages in thread
From: Mikulas Patocka @ 2014-09-16 16:38 UTC (permalink / raw)
To: Tomi Valkeinen, Jean-Christophe Plagniol-Villard
Cc: linux-fbdev, linux-kernel
The function bitcpy_rev has a bug that may result in screen corruption.
The bug happens under these conditions:
* the end of the destination area of a copy operation is aligned on a long
word boundary
* the end of the source area is not aligned on a long word boundary
* we are copying more than one long word
In this case, the variable shift is non-zero and the variable first is
zero. The statements FB_WRITEL(comp(d0, FB_READL(dst), first), dst) reads
the last long word of the destination and writes it back unchanged
(because first is zero). Correctly, we should write the variable d0 to the
last word of the destination in this case.
This patch fixes the bug by introducing and extra test if first is zero.
The patch also removes the references to fb_memmove in the code that is
commented out because fb_memmove was removed from framebuffer subsystem.
Signed-off-by: Mikulas Patocka <mpatocka@redhat.com>
Cc: stable@vger.kernel.org
Index: linux-3.16.1/drivers/video/fbdev/core/cfbcopyarea.c
===================================================================
--- linux-3.16.1.orig/drivers/video/cfbcopyarea.c
+++ linux-3.16.1/drivers/video/fbdev/core/cfbcopyarea.c
@@ -55,8 +55,8 @@ bitcpy(struct fb_info *p, unsigned long
* If you suspect bug in this function, compare it with this simple
* memmove implementation.
*/
- fb_memmove((char *)dst + ((dst_idx & (bits - 1))) / 8,
- (char *)src + ((src_idx & (bits - 1))) / 8, n / 8);
+ memmove((char *)dst + ((dst_idx & (bits - 1))) / 8,
+ (char *)src + ((src_idx & (bits - 1))) / 8, n / 8);
return;
#endif
@@ -221,8 +221,8 @@ bitcpy_rev(struct fb_info *p, unsigned l
* If you suspect bug in this function, compare it with this simple
* memmove implementation.
*/
- fb_memmove((char *)dst + ((dst_idx & (bits - 1))) / 8,
- (char *)src + ((src_idx & (bits - 1))) / 8, n / 8);
+ memmove((char *)dst + ((dst_idx & (bits - 1))) / 8,
+ (char *)src + ((src_idx & (bits - 1))) / 8, n / 8);
return;
#endif
@@ -324,7 +324,10 @@ bitcpy_rev(struct fb_info *p, unsigned l
d0 = d0 << left | d1 >> right;
}
d0 = fb_rev_pixels_in_long(d0, bswapmask);
- FB_WRITEL(comp(d0, FB_READL(dst), first), dst);
+ if (!first)
+ FB_WRITEL(d0, dst);
+ else
+ FB_WRITEL(comp(d0, FB_READL(dst), first), dst);
d0 = d1;
dst--;
n -= dst_idx+1;
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] framebuffer: fix screen corruption when copying
2014-09-16 16:38 ` Mikulas Patocka
@ 2014-09-17 6:44 ` Geert Uytterhoeven
-1 siblings, 0 replies; 8+ messages in thread
From: Geert Uytterhoeven @ 2014-09-17 6:44 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Linux Fbdev development list, linux-kernel@vger.kernel.org
Hi Mikulas,
On Tue, Sep 16, 2014 at 6:38 PM, Mikulas Patocka <mpatocka@redhat.com> wrote:
> The patch also removes the references to fb_memmove in the code that is
> commented out because fb_memmove was removed from framebuffer subsystem.
I'd leave it as-is. Using plain memmove() is not correct, as it should operate
on MMIO space, not RAM. On x86 it works with memmove(), though, but
not on all architectures.
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] framebuffer: fix screen corruption when copying
@ 2014-09-17 6:44 ` Geert Uytterhoeven
0 siblings, 0 replies; 8+ messages in thread
From: Geert Uytterhoeven @ 2014-09-17 6:44 UTC (permalink / raw)
To: Mikulas Patocka
Cc: Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Linux Fbdev development list, linux-kernel@vger.kernel.org
Hi Mikulas,
On Tue, Sep 16, 2014 at 6:38 PM, Mikulas Patocka <mpatocka@redhat.com> wrote:
> The patch also removes the references to fb_memmove in the code that is
> commented out because fb_memmove was removed from framebuffer subsystem.
I'd leave it as-is. Using plain memmove() is not correct, as it should operate
on MMIO space, not RAM. On x86 it works with memmove(), though, but
not on all architectures.
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
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] framebuffer: fix screen corruption when copying
2014-09-17 6:44 ` Geert Uytterhoeven
@ 2014-09-17 11:44 ` Mikulas Patocka
-1 siblings, 0 replies; 8+ messages in thread
From: Mikulas Patocka @ 2014-09-17 11:44 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Linux Fbdev development list, linux-kernel@vger.kernel.org
On Wed, 17 Sep 2014, Geert Uytterhoeven wrote:
> Hi Mikulas,
>
> On Tue, Sep 16, 2014 at 6:38 PM, Mikulas Patocka <mpatocka@redhat.com> wrote:
> > The patch also removes the references to fb_memmove in the code that is
> > commented out because fb_memmove was removed from framebuffer subsystem.
>
> I'd leave it as-is. Using plain memmove() is not correct, as it should operate
> on MMIO space, not RAM. On x86 it works with memmove(), though, but
> not on all architectures.
>
> Gr{oetje,eeting}s,
>
> Geert
That code is disabled with #if 0. So it doesn't matter if it's incorrect
in some specific situation.
The purpose of that disabled code is to be able to verify that the bug is
in bitcpy_rev. I get screen corruption - so I enable that "memmove"
statement - the screen corruption goes away - so I know that the bug is in
bitcpy_rev.
Mikulas
^ permalink raw reply [flat|nested] 8+ messages in thread* Re: [PATCH] framebuffer: fix screen corruption when copying
@ 2014-09-17 11:44 ` Mikulas Patocka
0 siblings, 0 replies; 8+ messages in thread
From: Mikulas Patocka @ 2014-09-17 11:44 UTC (permalink / raw)
To: Geert Uytterhoeven
Cc: Tomi Valkeinen, Jean-Christophe Plagniol-Villard,
Linux Fbdev development list, linux-kernel@vger.kernel.org
On Wed, 17 Sep 2014, Geert Uytterhoeven wrote:
> Hi Mikulas,
>
> On Tue, Sep 16, 2014 at 6:38 PM, Mikulas Patocka <mpatocka@redhat.com> wrote:
> > The patch also removes the references to fb_memmove in the code that is
> > commented out because fb_memmove was removed from framebuffer subsystem.
>
> I'd leave it as-is. Using plain memmove() is not correct, as it should operate
> on MMIO space, not RAM. On x86 it works with memmove(), though, but
> not on all architectures.
>
> Gr{oetje,eeting}s,
>
> Geert
That code is disabled with #if 0. So it doesn't matter if it's incorrect
in some specific situation.
The purpose of that disabled code is to be able to verify that the bug is
in bitcpy_rev. I get screen corruption - so I enable that "memmove"
statement - the screen corruption goes away - so I know that the bug is in
bitcpy_rev.
Mikulas
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] framebuffer: fix screen corruption when copying
2014-09-16 16:38 ` Mikulas Patocka
@ 2014-09-30 10:41 ` Tomi Valkeinen
-1 siblings, 0 replies; 8+ messages in thread
From: Tomi Valkeinen @ 2014-09-30 10:41 UTC (permalink / raw)
To: Mikulas Patocka, Jean-Christophe Plagniol-Villard
Cc: linux-fbdev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 998 bytes --]
On 16/09/14 19:38, Mikulas Patocka wrote:
> The function bitcpy_rev has a bug that may result in screen corruption.
> The bug happens under these conditions:
> * the end of the destination area of a copy operation is aligned on a long
> word boundary
> * the end of the source area is not aligned on a long word boundary
> * we are copying more than one long word
>
> In this case, the variable shift is non-zero and the variable first is
> zero. The statements FB_WRITEL(comp(d0, FB_READL(dst), first), dst) reads
> the last long word of the destination and writes it back unchanged
> (because first is zero). Correctly, we should write the variable d0 to the
> last word of the destination in this case.
>
> This patch fixes the bug by introducing and extra test if first is zero.
>
> The patch also removes the references to fb_memmove in the code that is
> commented out because fb_memmove was removed from framebuffer subsystem.
Thanks, queued for 3.18.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] framebuffer: fix screen corruption when copying
@ 2014-09-30 10:41 ` Tomi Valkeinen
0 siblings, 0 replies; 8+ messages in thread
From: Tomi Valkeinen @ 2014-09-30 10:41 UTC (permalink / raw)
To: Mikulas Patocka, Jean-Christophe Plagniol-Villard
Cc: linux-fbdev, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 998 bytes --]
On 16/09/14 19:38, Mikulas Patocka wrote:
> The function bitcpy_rev has a bug that may result in screen corruption.
> The bug happens under these conditions:
> * the end of the destination area of a copy operation is aligned on a long
> word boundary
> * the end of the source area is not aligned on a long word boundary
> * we are copying more than one long word
>
> In this case, the variable shift is non-zero and the variable first is
> zero. The statements FB_WRITEL(comp(d0, FB_READL(dst), first), dst) reads
> the last long word of the destination and writes it back unchanged
> (because first is zero). Correctly, we should write the variable d0 to the
> last word of the destination in this case.
>
> This patch fixes the bug by introducing and extra test if first is zero.
>
> The patch also removes the references to fb_memmove in the code that is
> commented out because fb_memmove was removed from framebuffer subsystem.
Thanks, queued for 3.18.
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-09-30 10:41 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2014-09-16 16:38 [PATCH] framebuffer: fix screen corruption when copying Mikulas Patocka
2014-09-16 16:38 ` Mikulas Patocka
2014-09-17 6:44 ` Geert Uytterhoeven
2014-09-17 6:44 ` Geert Uytterhoeven
2014-09-17 11:44 ` Mikulas Patocka
2014-09-17 11:44 ` Mikulas Patocka
2014-09-30 10:41 ` Tomi Valkeinen
2014-09-30 10:41 ` Tomi Valkeinen
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.