| Message ID | 20170810121659.7k2pqemqnd3px2qi@mwanda (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <dan.carpenter@oracle.com> 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-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/amd-gfx
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 <dan.carpenter@oracle.com> > > 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
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 <dan.carpenter@oracle.com> > > 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
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 <dan.carpenter@oracle.com> >> 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 >
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 <dan.carpenter@oracle.com> > > > 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
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 <dan.carpenter@oracle.com> >>> 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
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);
"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 <dan.carpenter@oracle.com>