Re: [PATCH] drm/amdgpu: potential shift wrapping bug
Am 10.08.2017 15:02, schrieb Christian König: > Am 10.08.2017 um 14:53 schrieb Dan Carpenter: >> On Thu, Aug 10, 2017 at 02:30:15PM +0200, Christian König wrote: >>> Am 10.08.2017 um 14:16 schrieb Dan Carpenter: "frag_align" is a u64, so presumably we want to use the high bits as well instead of shift wrapping. Fixes: 6be7adb37d9b ("drm/amdgpu: increase fragmentation size for Vega10 v2") Signed-off-by: Dan Carpenter >>> The fragment field has only 5bits in hardware and can never be more >>> than 31, >>> so the correct fix would actually be using uint32_t here instead. >>> >> Changing it to uint32_t introduces a new static checker warning: >> >> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1465 amdgpu_vm_frag_ptes() >> warn: was expecting a 64 bit value instead of '~(frag_align - 1)' >> >> Unfortunately, I get so many thousands of those I can't normally even >> review that sort of bug... >> >> Let me resend the original patch but with a modified changelog to say >> that the bug is a false positive. > > Ah, yes of course that's why I made it a 64bit value in the first place. > > Mhm, could we use something like (u32)(1 << pages_per_frag) instead to > silence the static checker warning? > > It doesn't make much sense to use a 64bit shift here. > > Christian. > Why not keeping Dan 1. patch and add a comment that pages_per_frag is always >31 ? Using 32bit in a 64bit is not forbidden, and changing it causes more problems than it solves. But doing so should be done in a clean way. just my 2 cents, re, wh ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdgpu: potential shift wrapping bug
Am 10.08.2017 um 14:53 schrieb Dan Carpenter: On Thu, Aug 10, 2017 at 02:30:15PM +0200, Christian König wrote: Am 10.08.2017 um 14:16 schrieb Dan Carpenter: "frag_align" is a u64, so presumably we want to use the high bits as well instead of shift wrapping. Fixes: 6be7adb37d9b ("drm/amdgpu: increase fragmentation size for Vega10 v2") Signed-off-by: Dan Carpenter The fragment field has only 5bits in hardware and can never be more than 31, so the correct fix would actually be using uint32_t here instead. Changing it to uint32_t introduces a new static checker warning: drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1465 amdgpu_vm_frag_ptes() warn: was expecting a 64 bit value instead of '~(frag_align - 1)' Unfortunately, I get so many thousands of those I can't normally even review that sort of bug... Let me resend the original patch but with a modified changelog to say that the bug is a false positive. Ah, yes of course that's why I made it a 64bit value in the first place. Mhm, could we use something like (u32)(1 << pages_per_frag) instead to silence the static checker warning? It doesn't make much sense to use a 64bit shift here. Christian. regards, dan carpenter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdgpu: potential shift wrapping bug
On Thu, Aug 10, 2017 at 03:02:53PM +0200, Christian König wrote: > Am 10.08.2017 um 14:53 schrieb Dan Carpenter: > > On Thu, Aug 10, 2017 at 02:30:15PM +0200, Christian König wrote: > > > Am 10.08.2017 um 14:16 schrieb Dan Carpenter: > > > > "frag_align" is a u64, so presumably we want to use the high bits as > > > > well instead of shift wrapping. > > > > > > > > Fixes: 6be7adb37d9b ("drm/amdgpu: increase fragmentation size for > > > > Vega10 v2") > > > > Signed-off-by: Dan Carpenter > > > The fragment field has only 5bits in hardware and can never be more than > > > 31, > > > so the correct fix would actually be using uint32_t here instead. > > > > > Changing it to uint32_t introduces a new static checker warning: > > > > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1465 amdgpu_vm_frag_ptes() > > warn: was expecting a 64 bit value instead of '~(frag_align - 1)' > > > > Unfortunately, I get so many thousands of those I can't normally even > > review that sort of bug... > > > > Let me resend the original patch but with a modified changelog to say > > that the bug is a false positive. > > Ah, yes of course that's why I made it a 64bit value in the first place. > > Mhm, could we use something like (u32)(1 << pages_per_frag) instead to > silence the static checker warning? That wouldn't silence it and I think that's not super pretty either. > > It doesn't make much sense to use a 64bit shift here. > I'm just going to ignore the warning. This driver isn't part of my .config so I'm not really compiling it the way it was designed which means I don't have the cross function database enabled. Probably if I compiled this normally, I wouldn't even get the warning. regards, dan carpenter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdgpu: potential shift wrapping bug
On Thu, Aug 10, 2017 at 02:30:15PM +0200, Christian König wrote: > Am 10.08.2017 um 14:16 schrieb Dan Carpenter: > > "frag_align" is a u64, so presumably we want to use the high bits as > > well instead of shift wrapping. > > > > Fixes: 6be7adb37d9b ("drm/amdgpu: increase fragmentation size for Vega10 > > v2") > > Signed-off-by: Dan Carpenter > > The fragment field has only 5bits in hardware and can never be more than 31, > so the correct fix would actually be using uint32_t here instead. > Changing it to uint32_t introduces a new static checker warning: drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:1465 amdgpu_vm_frag_ptes() warn: was expecting a 64 bit value instead of '~(frag_align - 1)' Unfortunately, I get so many thousands of those I can't normally even review that sort of bug... Let me resend the original patch but with a modified changelog to say that the bug is a false positive. regards, dan carpenter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdgpu: potential shift wrapping bug
On Thu, Aug 10, 2017 at 02:30:15PM +0200, Christian König wrote: > Am 10.08.2017 um 14:16 schrieb Dan Carpenter: > > "frag_align" is a u64, so presumably we want to use the high bits as > > well instead of shift wrapping. > > > > Fixes: 6be7adb37d9b ("drm/amdgpu: increase fragmentation size for Vega10 > > v2") > > Signed-off-by: Dan Carpenter > > The fragment field has only 5bits in hardware and can never be more than 31, > so the correct fix would actually be using uint32_t here instead. > Alright. Thanks. I'll resend. regards, dan carpenter ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
Re: [PATCH] drm/amdgpu: potential shift wrapping bug
Am 10.08.2017 um 14:16 schrieb Dan Carpenter: "frag_align" is a u64, so presumably we want to use the high bits as well instead of shift wrapping. Fixes: 6be7adb37d9b ("drm/amdgpu: increase fragmentation size for Vega10 v2") Signed-off-by: Dan Carpenter The fragment field has only 5bits in hardware and can never be more than 31, so the correct fix would actually be using uint32_t here instead. Christian. diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index ba0407d12525..d9a8e942ac3b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -1459,7 +1459,7 @@ static int amdgpu_vm_frag_ptes(struct amdgpu_pte_update_params*params, /* SI and newer are optimized for 64KB */ unsigned pages_per_frag = AMDGPU_LOG2_PAGES_PER_FRAG(params->adev); uint64_t frag_flags = AMDGPU_PTE_FRAG(pages_per_frag); - uint64_t frag_align = 1 << pages_per_frag; + uint64_t frag_align = 1ULL << pages_per_frag; uint64_t frag_start = ALIGN(start, frag_align); uint64_t frag_end = end & ~(frag_align - 1); ___ amd-gfx mailing list amd-...@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel
[PATCH] drm/amdgpu: potential shift wrapping bug
"frag_align" is a u64, so presumably we want to use the high bits as well instead of shift wrapping. Fixes: 6be7adb37d9b ("drm/amdgpu: increase fragmentation size for Vega10 v2") Signed-off-by: Dan Carpenter diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c index ba0407d12525..d9a8e942ac3b 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c @@ -1459,7 +1459,7 @@ static int amdgpu_vm_frag_ptes(struct amdgpu_pte_update_params*params, /* SI and newer are optimized for 64KB */ unsigned pages_per_frag = AMDGPU_LOG2_PAGES_PER_FRAG(params->adev); uint64_t frag_flags = AMDGPU_PTE_FRAG(pages_per_frag); - uint64_t frag_align = 1 << pages_per_frag; + uint64_t frag_align = 1ULL << pages_per_frag; uint64_t frag_start = ALIGN(start, frag_align); uint64_t frag_end = end & ~(frag_align - 1); ___ dri-devel mailing list dri-devel@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/dri-devel