* likely signedness bug in drm and nvidia drivers @ 2015-07-20 20:46 Rasmus Villemoes 2015-07-21 2:44 ` Ilia Mirkin 0 siblings, 1 reply; 3+ messages in thread From: Rasmus Villemoes @ 2015-07-20 20:46 UTC (permalink / raw) To: Ben Skeggs, David Airlie, Antonino Daplas, Tomi Valkeinen Cc: linux-fbdev, dri-devel Hi, The files drivers/gpu/drm/nouveau/nv50_fbcon.c drivers/gpu/drm/nouveau/nvc0_fbcon.c drivers/video/fbdev/nvidia/nv_accel.c all contain a right-shift of ~0 (aka -1) - just grep for '~0 >>'. gcc always does arithmetic right shift of signed types, which means that the result is always -1 again [type promotion/conversion doesn't kick in until after the shift subexpression has been evaluated], independent of the second operand. I can hardly believe that is intended (the result is used as a mask, which thus consists of all 1s). If these are indeed bugs, the patch is obvious (just make the literal an unsigned 0). Rasmus _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: likely signedness bug in drm and nvidia drivers 2015-07-20 20:46 likely signedness bug in drm and nvidia drivers Rasmus Villemoes @ 2015-07-21 2:44 ` Ilia Mirkin 2015-07-21 8:56 ` Daniel Thompson 0 siblings, 1 reply; 3+ messages in thread From: Ilia Mirkin @ 2015-07-21 2:44 UTC (permalink / raw) To: Rasmus Villemoes; +Cc: linux-fbdev, dri-devel, Tomi Valkeinen, Ben Skeggs I think you're right. The intent is to mask off the bits above bits_per_pixel. So if bits_per_pixel is 24, the mask would be 0xff000000. If it's 16, then the mask would be 0xffff0000. If it's 32, then the mask is 0. In reality, bits_per_pixel is almost exclusively 32, which will end up with a mask of 0 (note that the shift result is inverted at the end). So for the majority case, there's not bug... just a useless operation. I took a look at linux/bitops.h, and there's nothing particularly great there. GENMASK, I guess, but it's not quite right. Just switching to 0U should be fine there. Now that I think about it, I believe these patches have already been sent in the past. I also just did the grep that I did before: $ git grep -- '~0 >>' arch/m32r/include/asm/thread_info.h: ti->flags = (ti->flags & (~0 >> (32 - TI_FLAG_FAULT_CODE_SHIFT))) arch/sh/include/asm/thread_info.h: ti->flags = (ti->flags & (~0 >> (32 - TI_FLAG_FAULT_CODE_SHIFT))) drivers/gpu/drm/nouveau/nv50_fbcon.c: uint32_t mask = ~(~0 >> (32 - info->var.bits_per_pixel)); drivers/gpu/drm/nouveau/nvc0_fbcon.c: uint32_t mask = ~(~0 >> (32 - info->var.bits_per_pixel)); drivers/video/fbdev/nvidia/nv_accel.c: u32 fg, bg, mask = ~(~0 >> (32 - info->var.bits_per_pixel)); which shows that these are the only ones. See email with subject "[PATCH] nvidia/noveau: Fix color mask" from June 17, 2015. -ilia On Mon, Jul 20, 2015 at 4:46 PM, Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote: > Hi, > > The files > > drivers/gpu/drm/nouveau/nv50_fbcon.c > drivers/gpu/drm/nouveau/nvc0_fbcon.c > drivers/video/fbdev/nvidia/nv_accel.c > > all contain a right-shift of ~0 (aka -1) - just grep for '~0 >>'. gcc > always does arithmetic right shift of signed types, which means that the > result is always -1 again [type promotion/conversion doesn't kick in > until after the shift subexpression has been evaluated], independent of > the second operand. I can hardly believe that is intended (the result is > used as a mask, which thus consists of all 1s). If these are indeed > bugs, the patch is obvious (just make the literal an unsigned 0). > > Rasmus > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/dri-devel _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: likely signedness bug in drm and nvidia drivers 2015-07-21 2:44 ` Ilia Mirkin @ 2015-07-21 8:56 ` Daniel Thompson 0 siblings, 0 replies; 3+ messages in thread From: Daniel Thompson @ 2015-07-21 8:56 UTC (permalink / raw) To: Ilia Mirkin, Rasmus Villemoes Cc: linux-fbdev, Tomi Valkeinen, Ben Skeggs, dri-devel On 21/07/15 03:44, Ilia Mirkin wrote: > I think you're right. The intent is to mask off the bits above> bits_per_pixel. So if bits_per_pixel is 24, the mask would be> 0xff000000. If it's 16, then the mask would be 0xffff0000. If it's 32,> then the mask is 0.> > In reality, bits_per_pixel is almost exclusively 32, which will end up> with a mask of 0 (note that the shift result is inverted at the end).> So for the majority case, there's not bug... just a useless operation.> > I took a look at linux/bitops.h, and there's nothing particularly> great there. GENMASK, I guess, but it's not quite right. Just> switching to 0U should be fine there. I really don't see GENMASK() isn't quite right. Try: uint32_t mask = GENMASK(32, info->var.bits_per_pixel); Versus: uint32_t mask = ~(~0u >> (32 - info->var.bits_per_pixel)); For me, the GENMASK() is obvious whilst the later takes a good bit of mental decoding. Daniel. _______________________________________________ dri-devel mailing list dri-devel@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/dri-devel ^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2015-07-21 8:56 UTC | newest] Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2015-07-20 20:46 likely signedness bug in drm and nvidia drivers Rasmus Villemoes 2015-07-21 2:44 ` Ilia Mirkin 2015-07-21 8:56 ` Daniel Thompson
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).