Hello Chunming Zhou,
The patch 57ff96cf471a: "drm/amdgpu: implement cgs gpu memory callbacks" from Apr 24, 2015, leads to the following static checker warning:
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:274 amdgpu_cgs_gmap_gpu_mem() warn: should 'obj->placements[0]->fpfn << 12' be a 64 bit type?
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:275 amdgpu_cgs_gmap_gpu_mem() warn: should 'obj->placements[0]->lpfn << 12' be a 64 bit type?
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c 265 static int amdgpu_cgs_gmap_gpu_mem(void *cgs_device, cgs_handle_t handle, 266 uint64_t *mcaddr) 267 { 268 int r; 269 u64 min_offset, max_offset; 270 struct amdgpu_bo *obj = (struct amdgpu_bo *)handle; 271 272 WARN_ON_ONCE(obj->placement.num_placement > 1); 273 274 min_offset = obj->placements[0].fpfn << PAGE_SHIFT; 275 max_offset = obj->placements[0].lpfn << PAGE_SHIFT;
Both of these.
276 277 r = amdgpu_bo_reserve(obj, false); 278 if (unlikely(r != 0)) 279 return r; 280 r = amdgpu_bo_pin_restricted(obj, AMDGPU_GEM_DOMAIN_GTT, 281 min_offset, max_offset, mcaddr); 282 amdgpu_bo_unreserve(obj); 283 return r; 284 }
There are actually a few of these warnings which were less clear whether the warning was correct or not so I didn't send them.
drivers/gpu/drm/amd/amdgpu/cz_smc.c:463 cz_smu_populate_single_firmware_entry() warn: should '((header->jt_offset)) << 2' be a 64 bit type? drivers/gpu/drm/amd/amdgpu/fiji_smc.c:404 fiji_smu_populate_single_firmware_entry() warn: should '((header->jt_offset)) << 2' be a 64 bit type? drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:724 amdgpu_cgs_get_firmware_info() warn: should '((header->jt_offset)) << 2' be a 64 bit type? drivers/gpu/drm/amd/amdgpu/tonga_smc.c:406 tonga_smu_populate_single_firmware_entry() warn: should '((header->jt_offset)) << 2' be a 64 bit type? drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c:615 amdgpu_gem_op_ioctl() warn: should 'robj->tbo.mem.page_alignment << 12' be a 64 bit type?
regards, dan carpenter
Hi Dan, Thanks for figuring out that.
274 min_offset = obj->placements[0].fpfn << PAGE_SHIFT; 275 max_offset = obj->placements[0].lpfn << PAGE_SHIFT;
Maybe should be: min_offset = obj->placements[0].fpfn; min_offset <<= PAGE_SHIFT; max_offset = obj->placements[0].lpfn; max_offset <<= PAGE_SHIFT;
Regards, David Zhou
-----Original Message----- From: Dan Carpenter [mailto:dan.carpenter@oracle.com] Sent: Saturday, August 22, 2015 12:24 AM To: Zhou, David(ChunMing) Cc: dri-devel@lists.freedesktop.org Subject: re: drm/amdgpu: implement cgs gpu memory callbacks
Hello Chunming Zhou,
The patch 57ff96cf471a: "drm/amdgpu: implement cgs gpu memory callbacks" from Apr 24, 2015, leads to the following static checker warning:
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:274 amdgpu_cgs_gmap_gpu_mem() warn: should 'obj->placements[0]->fpfn << 12' be a 64 bit type?
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:275 amdgpu_cgs_gmap_gpu_mem() warn: should 'obj->placements[0]->lpfn << 12' be a 64 bit type?
drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c 265 static int amdgpu_cgs_gmap_gpu_mem(void *cgs_device, cgs_handle_t handle, 266 uint64_t *mcaddr) 267 { 268 int r; 269 u64 min_offset, max_offset; 270 struct amdgpu_bo *obj = (struct amdgpu_bo *)handle; 271 272 WARN_ON_ONCE(obj->placement.num_placement > 1); 273 274 min_offset = obj->placements[0].fpfn << PAGE_SHIFT; 275 max_offset = obj->placements[0].lpfn << PAGE_SHIFT;
Both of these.
276 277 r = amdgpu_bo_reserve(obj, false); 278 if (unlikely(r != 0)) 279 return r; 280 r = amdgpu_bo_pin_restricted(obj, AMDGPU_GEM_DOMAIN_GTT, 281 min_offset, max_offset, mcaddr); 282 amdgpu_bo_unreserve(obj); 283 return r; 284 }
There are actually a few of these warnings which were less clear whether the warning was correct or not so I didn't send them.
drivers/gpu/drm/amd/amdgpu/cz_smc.c:463 cz_smu_populate_single_firmware_entry() warn: should '((header->jt_offset)) << 2' be a 64 bit type? drivers/gpu/drm/amd/amdgpu/fiji_smc.c:404 fiji_smu_populate_single_firmware_entry() warn: should '((header->jt_offset)) << 2' be a 64 bit type? drivers/gpu/drm/amd/amdgpu/amdgpu_cgs.c:724 amdgpu_cgs_get_firmware_info() warn: should '((header->jt_offset)) << 2' be a 64 bit type? drivers/gpu/drm/amd/amdgpu/tonga_smc.c:406 tonga_smu_populate_single_firmware_entry() warn: should '((header-
jt_offset)) << 2' be a 64 bit type?
drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c:615 amdgpu_gem_op_ioctl() warn: should 'robj->tbo.mem.page_alignment << 12' be a 64 bit type?
regards, dan carpenter
On Mon, Aug 24, 2015 at 07:09:15AM +0000, Zhou, David(ChunMing) wrote:
Hi Dan, Thanks for figuring out that.
274 min_offset = obj->placements[0].fpfn << PAGE_SHIFT; 275 max_offset = obj->placements[0].lpfn << PAGE_SHIFT;
Maybe should be: min_offset = obj->placements[0].fpfn; min_offset <<= PAGE_SHIFT; max_offset = obj->placements[0].lpfn; max_offset <<= PAGE_SHIFT;
It's probably just simpler to be:
min_offset = (u64)obj->placements[0].fpfn << PAGE_SHIFT; max_offset = (u64)obj->placements[0].lpfn << PAGE_SHIFT;
But the larger questions aer why is min_offset a u64, and can the shift actually wrap? I'm just looking at static checker warnings, and I'm not very familiar with this code so I don't know the answers.
regards, dan carpenter
Inline...[DZ]
-----Original Message----- From: Dan Carpenter [mailto:dan.carpenter@oracle.com] Sent: Tuesday, August 25, 2015 3:51 AM To: Zhou, David(ChunMing) Cc: dri-devel@lists.freedesktop.org Subject: Re: drm/amdgpu: implement cgs gpu memory callbacks
On Mon, Aug 24, 2015 at 07:09:15AM +0000, Zhou, David(ChunMing) wrote:
Hi Dan, Thanks for figuring out that.
274 min_offset = obj->placements[0].fpfn << PAGE_SHIFT; 275 max_offset = obj->placements[0].lpfn << PAGE_SHIFT;
Maybe should be: min_offset = obj->placements[0].fpfn; min_offset <<= PAGE_SHIFT; max_offset = obj->placements[0].lpfn; max_offset <<= PAGE_SHIFT;
It's probably just simpler to be:
min_offset = (u64)obj->placements[0].fpfn << PAGE_SHIFT; max_offset = (u64)obj->placements[0].lpfn << PAGE_SHIFT;
But the larger questions aer why is min_offset a u64,
[DZ] max/min_offset is memory size.
and can the shift actually wrap?
[DZ] of course, adding shift wrap is better. fpfn/lpfn is page number, so <<PAGE_SHIFT is to convert to memory size.
Regards, David Zhou
I'm just looking at static checker warnings, and I'm not very familiar with this code so I don't know the answers.
regards, dan carpenter
On Tue, Aug 25, 2015 at 02:07:21AM +0000, Zhou, David(ChunMing) wrote:
and can the shift actually wrap?
[DZ] of course, adding shift wrap is better. fpfn/lpfn is page number, so <<PAGE_SHIFT is to convert to memory size.
By "Shift wrap" I meant how you shift beyond the end of a 32bit number and it truncates, or if it's signed then it can wrap around (it's undefined actually, I probably should use a different word?).
int main(void) { unsigned long long a = 0xf0000000U << 12; unsigned long long b = 0xf0000000ULL << 12;
printf("%llx %llx\n", a, b);
return 0; }
regards, dan carpenter
-----Original Message----- From: Dan Carpenter [mailto:dan.carpenter@oracle.com] Sent: Tuesday, August 25, 2015 1:51 PM To: Zhou, David(ChunMing) Cc: dri-devel@lists.freedesktop.org Subject: Re: drm/amdgpu: implement cgs gpu memory callbacks
On Tue, Aug 25, 2015 at 02:07:21AM +0000, Zhou, David(ChunMing) wrote:
and can the shift actually wrap?
[DZ] of course, adding shift wrap is better. fpfn/lpfn is page number, so
<<PAGE_SHIFT is to convert to memory size.
By "Shift wrap" I meant how you shift beyond the end of a 32bit number and it truncates, or if it's signed then it can wrap around (it's undefined actually, I probably should use a different word?).
int main(void) { unsigned long long a = 0xf0000000U << 12; unsigned long long b = 0xf0000000ULL << 12;
printf("%llx %llx\n", a, b);
return 0; }
[DZ] oh, I understand your mean, with previous PAGE_SHIFT, I think it should be like 'b' in your example without truncates.
Thanks, David Zhou
regards, dan carpenter
dri-devel@lists.freedesktop.org
-
Dan Carpenter -
Zhou, David(ChunMing)