Sorry, more bike-shedding.
Am 2020-08-17 um 7:58 p.m. schrieb Mukul Joshi:
> Add __user annotation to fix related sparse warning while reading
> SDMA counters from userland.
>
> Reported-by: kernel test robot <lkp@xxxxxxxxx>
> Signed-off-by: Mukul Joshi <mukul.joshi@xxxxxxx>
> ---
> drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 9 +++------
> 1 file changed, 3 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> index e0e60b0d0669..e2894967c372 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -157,19 +157,16 @@ int read_sdma_queue_counter(uint64_t q_rptr, uint64_t *val)
> {
> int ret;
> uint64_t tmp = 0;
> + uint64_t __user *sdma_usage_cntr;
>
> if (!val)
> return -EINVAL;
Maybe this check isn't needed. Both callers pass in pointers to local
variables. If a caller gets that wrong, how likely are they going to
handle the error code correctly?
> /*
> * SDMA activity counter is stored at queue's RPTR + 0x8 location.
> */
> - if (!access_ok((const void __user *)(q_rptr +
> - sizeof(uint64_t)), sizeof(uint64_t))) {
> - pr_err("Can't access sdma queue activity counter\n");
> - return -EFAULT;
> - }
> + sdma_usage_cntr = (uint64_t __user *)q_rptr + 1;
>
> - ret = get_user(tmp, (uint64_t *)(q_rptr + sizeof(uint64_t)));
> + ret = get_user(tmp, sdma_usage_cntr);
Maybe you don't need sdma_usage_cntr. Just inline the pointer
arithmetic. And I'm not sure why you need tmp either. Is it in case the
read gets only one dword and fails on the second one? The callers will
ignore the value, if you return an error, so I don't think it matters.
So this whole function would become very simple:
return get_user(*val, (uint64_t __user *)q_rptr + 1);
Now it could probably be a static inline function in a
kfd_device_queue_manager.h.
If that is the only use of q_rptr in the function, why not make the
parameter type uint64_t __user *, so you don't even need the type cast
in here? You can also change the type in struct temp_sdma_queue_list to
match.
While you're at it, you could also change the types of read_ptr and
write_ptr in struct queue_properties, because uint32_t * is really not
the correct type. It's a pointer to the wrong size and the wrong address
space. Though that change may have a few more ripple effects.
Regards,
Felix
> if (!ret) {
> *val = tmp;
> }
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
