nvidia/noveau: Fix color mask

PATCH In review Michael Büsch 2 participants 3 messages Jun 17, 2015 – Jun 18, 2015

Subsystems (5)

DRM drivers DRM misc Framebuffer layer nouveau +1 more subsystems

lore

Share & tools

lore

Michael Büsch

author

2015-06-17 17:05 UTC #0 lore raw

PATCH 1/1 nvidia/noveau: Fix color mask

The expression (~0 >> x) will always yield all-ones, because the right shift is an arithmetic right shift that will always shift ones in. Accordingly ~(~0 >> x) will always be zero.
Hence 'mask' will always be zero in this case.

Fix this by forcing a logical right shift instead of an arithmetic right shift by using an unsigned int constant.

Signed-off-by Michael Buesch <[email protected]>

---

This patch is untested, because I do not have the hardware.

drivers/gpu/drm/nouveau/nv50_fbcon.c +1 -1

Index: linux/drivers/gpu/drm/nouveau/nv50_fbcon.c
===================================================================
@@ -96,7 +96,7 @@ nv50_fbcon_imageblit(struct fb_info *inf
 	struct nouveau_drm *drm = nouveau_drm(nfbdev->dev);
 	struct nouveau_channel *chan = drm->channel;
 	uint32_t width, dwords, *data = (uint32_t *)image->data;
-	uint32_t mask = ~(~0 >> (32 - info->var.bits_per_pixel));
+	uint32_t mask = ~(~0U >> (32 - info->var.bits_per_pixel));
 	uint32_t *palette = info->pseudo_palette;
 	int ret;

drivers/gpu/drm/nouveau/nvc0_fbcon.c +1 -1

Index: linux/drivers/gpu/drm/nouveau/nvc0_fbcon.c
===================================================================
@@ -96,7 +96,7 @@ nvc0_fbcon_imageblit(struct fb_info *inf
 	struct nouveau_drm *drm = nouveau_drm(nfbdev->dev);
 	struct nouveau_channel *chan = drm->channel;
 	uint32_t width, dwords, *data = (uint32_t *)image->data;
-	uint32_t mask = ~(~0 >> (32 - info->var.bits_per_pixel));
+	uint32_t mask = ~(~0U >> (32 - info->var.bits_per_pixel));
 	uint32_t *palette = info->pseudo_palette;
 	int ret;

drivers/video/fbdev/nvidia/nv_accel.c +1 -1

Index: linux/drivers/video/fbdev/nvidia/nv_accel.c
===================================================================
@@ -351,7 +351,7 @@ static void nvidiafb_mono_color_expand(s
 				       const struct fb_image *image)
 {
 	struct nvidia_par *par = info->par;
-	u32 fg, bg, mask = ~(~0 >> (32 - info->var.bits_per_pixel));
+	u32 fg, bg, mask = ~(~0U >> (32 - info->var.bits_per_pixel));
 	u32 dsize, width, *data = (u32 *) image->data, tmp;
 	int j, k = 0;

Ilia Mirkin

Reviewed-by on v1 1/1

2015-06-18 00:47 UTC #1 lore raw

Quoted 9 lines · Michael Büsch

The expression (~0 >> x) will always yield all-ones, because the right shift is an arithmetic right shift that will always shift ones in. Accordingly ~(~0 >> x) will always be zero.
Hence 'mask' will always be zero in this case.
Fix this by forcing a logical right shift instead of an arithmetic right shift by using an unsigned int constant.
Signed-off-by Michael Buesch <[email protected]>

Confirmed that this does indeed happen with

#include <stdio.h>
int main(int argc, char *argv[]) {
unsigned mask = ~(~0 >> (32 - (argv[1][0] - '0'))); printf("%08x\n", mask);
}

I guess fbdev/nvidia/nv_accel.c was the source of all this, as the code is identical, and it probably came first.

FWIW this is

Reviewed-by Ilia Mirkin <[email protected]>

Quoted 46 lines

---

This patch is untested, because I do not have the hardware.

Index: linux/drivers/gpu/drm/nouveau/nv50_fbcon.c
===================================================================

drivers/gpu/drm/nouveau/nv50_fbcon.c +1 -1

@@ -96,7 +96,7 @@ nv50_fbcon_imageblit(struct fb_info *inf
        struct nouveau_drm *drm = nouveau_drm(nfbdev->dev);
        struct nouveau_channel *chan = drm->channel;
        uint32_t width, dwords, *data = (uint32_t *)image->data;
-       uint32_t mask = ~(~0 >> (32 - info->var.bits_per_pixel));
+       uint32_t mask = ~(~0U >> (32 - info->var.bits_per_pixel));
        uint32_t *palette = info->pseudo_palette;
        int ret;

Index: linux/drivers/gpu/drm/nouveau/nvc0_fbcon.c
===================================================================

drivers/gpu/drm/nouveau/nvc0_fbcon.c +1 -1

@@ -96,7 +96,7 @@ nvc0_fbcon_imageblit(struct fb_info *inf
        struct nouveau_drm *drm = nouveau_drm(nfbdev->dev);
        struct nouveau_channel *chan = drm->channel;
        uint32_t width, dwords, *data = (uint32_t *)image->data;
-       uint32_t mask = ~(~0 >> (32 - info->var.bits_per_pixel));
+       uint32_t mask = ~(~0U >> (32 - info->var.bits_per_pixel));
        uint32_t *palette = info->pseudo_palette;
        int ret;

Index: linux/drivers/video/fbdev/nvidia/nv_accel.c
===================================================================

drivers/video/fbdev/nvidia/nv_accel.c +1 -1

@@ -351,7 +351,7 @@ static void nvidiafb_mono_color_expand(s
                                       const struct fb_image *image)
 {
        struct nvidia_par *par = info->par;
-       u32 fg, bg, mask = ~(~0 >> (32 - info->var.bits_per_pixel));
+       u32 fg, bg, mask = ~(~0U >> (32 - info->var.bits_per_pixel));
        u32 dsize, width, *data = (u32 *) image->data, tmp;
        int j, k = 0;

Quoted 5 lines

_______________________________________________
dri-devel mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/dri-devel

_______________________________________________
Nouveau mailing list
[email protected]
http://lists.freedesktop.org/mailman/listinfo/nouveau

Michael Büsch

author

2015-06-18 15:31 UTC #2 lore raw

Quoted 13 lines · On Wed, 17 Jun 2015 20:47:17 -0400 Ilia Mirkin <[email protected]> wrote:

Quoted 9 lines · Michael Büsch
The expression (~0 >> x) will always yield all-ones, because the right shift is an arithmetic right shift that will always shift ones in. Accordingly ~(~0 >> x) will always be zero.
Hence 'mask' will always be zero in this case.
Fix this by forcing a logical right shift instead of an arithmetic right shift by using an unsigned int constant.
Signed-off-by Michael Buesch <[email protected]>
Confirmed that this does indeed happen with

Quoted 8 lines

#include <stdio.h>
int main(int argc, char *argv[]) {
unsigned mask = ~(~0 >> (32 - (argv[1][0] - '0'))); printf("%08x\n", mask);
}
I guess fbdev/nvidia/nv_accel.c was the source of all this, as the code is identical, and it probably came first.

If anybody is able to help me in creating a working semantic patch (coccinelle) for this, that would be great.

I found this using a very hacky and incorrect spatch (some version is attached). It throws many false positives, doesn't find all such bugs and does not create correct patch output (especially the #define related rule is just meant as a hint).

Some basic thoughts that come to mind that could possibly be statically checked somehow are:

- right shift of promoted variables. That is stuff like this:
u8 x, y = 0x0F;
x = ~y >> 1;
/* x is 0xF8, not 0x78 as someone might expect. */
- Also check this for typedef'ed types where promotion takes place (that are smaller than int)?
- right shift of signed constants (like in this case). That probably is wrong in most cases.
How to check signedness of constants in spatch? (123 vs 123U) Is that even possible?
- Also detect this stuff, if variables/constants are hidden via #define or such:
#define REGVAL 0x0F
writereg(REGISTER, ~REGVAL >> 1);

Probably more stuff could be checked. Ideas are welcome. :)

Signature · 87 lines

Michael



@@
u8 e;
expression s;
@@
- ~e >> s
+ (u8)~e >> s

@@
s8 e;
expression s;
@@
- ~e >> s
+ (s8)~e >> s

@@
u16 e;
expression s;
@@
- ~e >> s
+ (u16)~e >> s

@@
s16 e;
expression s;
@@
- ~e >> s
+ (s16)~e >> s

@@
char e;
expression s;
@@
- ~e >> s
+ (char)~e >> s

@@
unsigned char e;
expression s;
@@
- ~e >> s
+ (unsigned char)~e >> s

@@
short e;
expression s;
@@
- ~e >> s
+ (short)~e >> s

@@
unsigned short e;
expression s;
@@
- ~e >> s
+ (unsigned short)~e >> s




@@
constant c;
expression s;
@@
- ~c >> s
+ (unsigned int)~c >> s





@sh expression@
identifier val;
expression shift;
@@
val >> shift


@@
expression e;
identifier sh.val;
@@
- #define val ~e
+ #define val e