Hi Emil,
Sorry for late reply. I was so busy last week and this week.
Your logic looks correct, smarter and shorter. I feel relatively more
difficult to understand the logic, with two "!" and one "||" in the same
if statement.
Thanks for the advice always.
On 2017-07-20 02:29 PM, Emil Velikov wrote:
> Hi Alex,
>
> On 20 July 2017 at 03:46, Alex Xie <AlexBin.Xie at amd.com> wrote:
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_bo_list.c
>> @@ -198,12 +198,18 @@ amdgpu_bo_list_get(struct amdgpu_fpriv *fpriv, int id)
>> result = idr_find(&fpriv->bo_list_handles, id);
>>
>> if (result) {
>> - if (kref_get_unless_zero(&result->refcount))
>> + if (kref_get_unless_zero(&result->refcount)) {
>> + rcu_read_unlock();
>> mutex_lock(&result->lock);
>> - else
>> + }
>> + else {
>> + rcu_read_unlock();
>> result = NULL;
>> + }
>> + }
>> + else {
>> + rcu_read_unlock();
>> }
>> - rcu_read_unlock();
>>
>> return result;
> A drive-by suggestion - feel free to ignore.
>
> The "return early" approach seems great IMHO. The code will be
> shorter, indentation - less, no {}/else to track plus overall it seems
> clearer.
> Namely:
>
> result = idr_find(&fpriv->bo_list_handles, id);
>
> if (!result || !kref_get_unless_zero(&result->refcount)) {
> rcu_read_unlock();
> return NULL;
> }
>
> rcu_read_unlock();
> mutex_lock(&result->lock);
> return result;
> }
>
> HTH
> Emil
