drivers/misc/habanalabs: atomic_t api usage inconsistencies

drivers/misc/habanalabs: atomic_t api usage inconsistencies

[Shuah Khan]

All,

While I was looking at the atomic_t api usages for an unrelated issue,
I noticed free_slots_cnt in struct hl_cq incerment/decrement/reads are
not consistent.

atomic_inc() and atomic_set() are used, however instead of atomic_read()
the value is referenced directly in
drivers/misc/habanalabs/common/hw_queue.c

hl_queue_add_ptr()
atomic_t *free_slots = &hdev->completion_queue[q->cq_id].free_slots_cnt;

hl_hw_queue_schedule_cs()

atomic_t *free_slots = &hdev->completion_queue[i].free_slots_cnt;

Any reason why this is necessary. I don't know that this is causing
any problems, it is just odd that access is inconsistent.

thanks,
-- Shuah

Re: drivers/misc/habanalabs: atomic_t api usage inconsistencies

[Oded Gabbay]

On Tue, Sep 22, 2020 at 1:08 AM Shuah Khan <skhan@linuxfoundation.org> wrote:
>
> All,
>
> While I was looking at the atomic_t api usages for an unrelated issue,
> I noticed free_slots_cnt in struct hl_cq incerment/decrement/reads are
> not consistent.
>
> atomic_inc() and atomic_set() are used, however instead of atomic_read()
> the value is referenced directly in
> drivers/misc/habanalabs/common/hw_queue.c
>
> hl_queue_add_ptr()
> atomic_t *free_slots = &hdev->completion_queue[q->cq_id].free_slots_cnt;
>
> hl_hw_queue_schedule_cs()
>
> atomic_t *free_slots = &hdev->completion_queue[i].free_slots_cnt;
>
> Any reason why this is necessary. I don't know that this is causing
> any problems, it is just odd that access is inconsistent.
>
> thanks,
> -- Shuah

Hi Shuah,
Thanks for taking notice of this issue :)
We will take a deeper look and fix the inconsistencies, although I
must say that we didn't notice any impact of this issue.
Thanks again.
Oded

Re: drivers/misc/habanalabs: atomic_t api usage inconsistencies

[Shuah Khan]

On 9/23/20 2:41 AM, Oded Gabbay wrote:
> On Tue, Sep 22, 2020 at 1:08 AM Shuah Khan <skhan@linuxfoundation.org> wrote:
>>
>> All,
>>
>> While I was looking at the atomic_t api usages for an unrelated issue,
>> I noticed free_slots_cnt in struct hl_cq incerment/decrement/reads are
>> not consistent.
>>
>> atomic_inc() and atomic_set() are used, however instead of atomic_read()
>> the value is referenced directly in
>> drivers/misc/habanalabs/common/hw_queue.c
>>
>> hl_queue_add_ptr()
>> atomic_t *free_slots = &hdev->completion_queue[q->cq_id].free_slots_cnt;
>>
>> hl_hw_queue_schedule_cs()
>>
>> atomic_t *free_slots = &hdev->completion_queue[i].free_slots_cnt;
>>
>> Any reason why this is necessary. I don't know that this is causing
>> any problems, it is just odd that access is inconsistent.
>>
>> thanks,
>> -- Shuah
> 
> Hi Shuah,
> Thanks for taking notice of this issue :)
> We will take a deeper look and fix the inconsistencies, although I
> must say that we didn't notice any impact of this issue.

If you haven't noticed any problems, it could be that this variable
doesn't need to atomic or you haven't run into it yet?

thanks,
-- Shuah