From: "Christian König" <[email protected]> Date: 2020-04-21 16:06:03 To: [email protected],Felix Kuehling <[email protected]>,Alex Deucher <[email protected]>,"David (ChunMing) Zhou" <[email protected]>,David Airlie <[email protected]>,Daniel Vetter <[email protected]>,[email protected],[email protected],[email protected] Cc: [email protected],Bernard Zhao <[email protected]> Subject: Re: [PATCH V3] amdgpu: remove unnecessary condition check>Am 21.04.20 um 10:03 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 <[email protected]> >> >> Changes since V1: >> *commit message improve >> *code style refactoring >> >> Changes since V2: >> *code style adjust >> >> Link for V1: >> *https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flore.kernel.org%2Fpatchwork%2Fpatch%2F1226587%2F&data=02%7C01%7Cchristian.koenig%40amd.com%7C0b8fffafb715474289b208d7e5ca7f6c%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637230530201280350&sdata=Sewv5ESX%2B0B4DbFbE03uM5sifrEcmJllC8pt7J42I7M%3D&reserved=0 >> --- >> .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 18 +++++++----------- >> 1 file changed, 7 insertions(+), 11 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..5424bd921a7b 100644 >> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c >> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c >> @@ -660,13 +660,12 @@ 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 { >> - 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; > >That is still not correct coding style. In general when one branch of an >if/else uses {} the other one should use it as well. > >But I agree with Felix that this change looks rather superfluous to me >as well. > >Regards, >Christian.
About the code style, you are right, I checked the refers: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/coding-style.rst?id=90280eaa88ac1a9140dc759941123530d5545bb6#n191 The if and else should use the same style. But i have to say there are so many code not follow the kernel code-style in amdgpu module. And also the ./scripts/checkpatch.pl did not throw any warning or error. If this change looks rather superfluous to all of you, should i change to the V1 change? After all i don`t think there is any necessary to check "ret" again, merge the <else and if (ret)> maybe better. Original code: static int reserve_bo_and_cond_vms(struct kgd_mem *mem,..... if (!ret) ctx->reserved = true; else pr_err("Failed to reserve buffers in ttm.\n"); if (ret) { kfree(ctx->vm_pd); ctx->vm_pd = NULL; } BR//bernard >> >> return ret; >> } >> @@ -733,15 +732,12 @@ 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; >> >> return ret; >> } > _______________________________________________ dri-devel mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/dri-devel
