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

[Felix Kuehling]

On 2017-09-17 08:03 AM, Oded Gabbay wrote:
> 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 /*

That's a new rule to me, and it seems a bit arbitrary. checkpatch.pl
doesn't complain about this. checkpatch.pl does complain about the
closing */ not being on it's own line, but checkpatch has always been OK
with multiline comments starting on the same line as the opening /*.

There are also plenty of examples of multiline comments starting on the
same line as /*. For example run this grep on the include/linux:

    grep -A3 '/\*[^*]\+[^/]$' *.h

Regards,
  Felix

>
>> +        * 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