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....@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
_______________________________________________ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
