[PATCH 09/11] drm/amdkfd: Fix kernel-queue wrapping bugs

[Oded Gabbay]

On Sat, Sep 16, 2017 at 2:43 AM, Felix Kuehling <Felix.Kuehling at amd.com> wrote:
> From: Yong Zhao <yong.zhao at amd.com>
>
> Avoid intermediate negative numbers when doing calculations with a mix
> of signed and unsigned variables where implicit conversions can lead
> to unexpected results.
>
> When kernel queue buffer wraps around to 0, we need to check that rptr
> won't be overwritten by the new packet.
>
> Signed-off-by: Yong Zhao <yong.zhao at amd.com>
> Signed-off-by: Felix Kuehling <Felix.Kuehling at amd.com>
> ---
>  drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c | 18 +++++++++++++++---
>  1 file changed, 15 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> index 9ebb4c1..1c66334 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_kernel_queue.c
> @@ -210,6 +210,11 @@ static int acquire_packet_buffer(struct kernel_queue *kq,
>         uint32_t wptr, rptr;
>         unsigned int *queue_address;
>
> +       /* When rptr == wptr, the buffer is empty.
Start comment text in a new line. First line should be just /*

> +        * When rptr == wptr + 1, the buffer is full.
> +        * It is always rptr that advances to the position of wptr, rather than
> +        * the opposite. So we can only use up to queue_size_dwords - 1 dwords.
> +        */
>         rptr = *kq->rptr_kernel;
>         wptr = *kq->wptr_kernel;
>         queue_address = (unsigned int *)kq->pq_kernel_addr;
> @@ -219,11 +224,10 @@ static int acquire_packet_buffer(struct kernel_queue *kq,
>         pr_debug("wptr: %d\n", wptr);
>         pr_debug("queue_address 0x%p\n", queue_address);
>
> -       available_size = (rptr - 1 - wptr + queue_size_dwords) %
> +       available_size = (rptr + queue_size_dwords - 1 - wptr) %
>                                                         queue_size_dwords;
>
> -       if (packet_size_in_dwords >= queue_size_dwords ||
> -                       packet_size_in_dwords >= available_size) {
> +       if (packet_size_in_dwords > available_size) {
>                 /*
>                  * make sure calling functions know
>                  * acquire_packet_buffer() failed
> @@ -233,6 +237,14 @@ static int acquire_packet_buffer(struct kernel_queue *kq,
>         }
>
>         if (wptr + packet_size_in_dwords >= queue_size_dwords) {
> +               /* make sure after rolling back to position 0, there is
> +                * still enough space.
> +                */
> +               if (packet_size_in_dwords >= rptr) {
> +                       *buffer_ptr = NULL;
> +                       return -ENOMEM;
> +               }

I don't think the condition is correct.
Suppose, queue_size_dwords == 100, wptr == rptr == 50 (queue is empty)
and we have a new packet with size of 70.
Now, wptr + size is 120, which is >= 100
However, 70 >= rptr (50) which will give us -ENOMEM, but this is not
correct condition, because the packet *does* have enough room in the
queue.

I think the condition should be:
if (packet_size_in_dwords - (queue_size_dwords - wptr) >= rptr)
but please check this.

> +               /* fill nops, roll back and start at position 0 */
>                 while (wptr > 0) {
>                         queue_address[wptr] = kq->nop_packet;
>                         wptr = (wptr + 1) % queue_size_dwords;
> --
> 2.7.4
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx