Though to be fair, we should probably consolidate the comment style so that it's actually consistent through the KFD. Kent -----Original Message----- From: amd-gfx [mailto:amd-gfx-bounces@xxxxxxxxxxxxxxxxxxxxx] On Behalf Of Russell, Kent Sent: Monday, September 18, 2017 11:28 AM To: Kuehling, Felix; Oded Gabbay Cc: Zhao, Yong; amd-gfx list Subject: RE: [PATCH 09/11] drm/amdkfd: Fix kernel-queue wrapping bugs 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@xxxxxxxxxxxxxxxxxxxxx] 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 _______________________________________________ amd-gfx mailing list amd-gfx at lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
