From: Felix Kuehling <felix.kuehling@xxxxxxx> Date: 2020-04-21 12:24:19 To: 1587180037-113840-1-git-send-email-bernard@xxxxxxxx,Alex Deucher <alexander.deucher@xxxxxxx>,"Christian König" <christian.koenig@xxxxxxx>,"David (ChunMing) Zhou" <David1.Zhou@xxxxxxx>,David Airlie <airlied@xxxxxxxx>,Daniel Vetter <daniel@xxxxxxxx>,amd-gfx@xxxxxxxxxxxxxxxxxxxxx,dri-devel@xxxxxxxxxxxxxxxxxxxxx,linux-kernel@xxxxxxxxxxxxxxx Cc: opensource.kernel@xxxxxxxx,Bernard Zhao <bernard@xxxxxxxx> Subject: Re: [PATCH V2] amdgpu: remove unnecessary condition check>Hi Bernard, > >Please see comments inline. > >Am 2020-04-20 um 10:41 p.m. schrieb Bernard Zhao: >> There is no need to if check again, maybe we could merge >> into the above else branch. >> >> Signed-off-by: Bernard Zhao <bernard@xxxxxxxx> >> >> --- >> Changes since V1: >> *commit message improve >> *code style refactoring >> >> Link for V1: >> * https://lore.kernel.org/patchwork/patch/1226587/ >> --- >> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 16 +++++++++------- >> 1 file changed, 9 insertions(+), 7 deletions(-) >> >> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c >> index 9dff792c9290..a64eeb07bec4 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c >> @@ -660,13 +660,15 @@ static int reserve_bo_and_vm(struct kgd_mem *mem, >> >> ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list, >> false, &ctx->duplicates); >> - if (!ret) >> - ctx->reserved = true; >> - else { >> + >> + if (ret) { >> pr_err("Failed to reserve buffers in ttm\n"); >> kfree(ctx->vm_pd); >> ctx->vm_pd = NULL; >> } >> + else { >> + ctx->reserved = true; >> + } > >Here you're just reversing the if and else branches. This change looks >completely superfluous to me. > >You're also breaking coding style conventions. The "else" should be on >the same line as the closing brace "}". I'm pretty sure checkpatch.pl >will complain about this. > In this file, only these two functions are <if (! Condition)... else .... > format. So in V2, after improve the commit info, I refer to the following code style suggestions and modify it to <if (condition)...else... > format https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=90280eaa88ac1a9140dc759941123530d5545bb6#n191 (refer from Markus Elfring`s suggestion). >> >> return ret; >> } >> @@ -733,15 +735,15 @@ static int reserve_bo_and_cond_vms(struct kgd_mem *mem, >> >> ret = ttm_eu_reserve_buffers(&ctx->ticket, &ctx->list, >> false, &ctx->duplicates); >> - if (!ret) >> - ctx->reserved = true; >> - else >> - pr_err("Failed to reserve buffers in ttm.\n"); >> >> if (ret) { >> + pr_err("Failed to reserve buffers in ttm.\n"); >> kfree(ctx->vm_pd); >> ctx->vm_pd = NULL; >> } >> + else { >> + ctx->reserved = true; >> + } > >Same as above regarding coding style. > >To minimize unnecessary code changes, you can merge the "if (ret) ..." >code into the else-branch of the previous if. > >Regards, > Felix > > >> >> return ret; >> } _______________________________________________ amd-gfx mailing list amd-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/amd-gfx
