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

[Russell, Kent]

Correct (coming from the guy who did all of the checkpatch cleanup for KFD). For multi-line comments, /* Can be on its own, or on the same line as the comment. */ has to be on its own. https://github.com/torvalds/linux/blob/master/scripts/checkpatch.pl#L3042

 Kent

-----Original Message-----
From: amd-gfx [mailto:amd-gfx-bounces at lists.freedesktop.org] On Behalf Of Felix Kuehling
Sent: Monday, September 18, 2017 11:22 AM
To: Oded Gabbay
Cc: Zhao, Yong; amd-gfx list
Subject: Re: [PATCH 09/11] drm/amdkfd: Fix kernel-queue wrapping bugs

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

_______________________________________________
amd-gfx mailing list
amd-gfx at lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx