Re: [PATCH] drm/radeon: cleanup coding style a bit
Applied. Thanks! Alex On Sun, Apr 26, 2020 at 9:18 AM Christian König wrote: > > Am 26.04.20 um 15:12 schrieb Bernard Zhao: > > Maybe no need to check ws before kmalloc, kmalloc will check > > itself, kmalloc`s logic is if ptr is NULL, kmalloc will just > > return > > > > Signed-off-by: Bernard Zhao > > Reviewed-by: Christian König > > I'm wondering why the automated scripts haven't found that one before. > > > --- > > drivers/gpu/drm/radeon/atom.c | 3 +-- > > 1 file changed, 1 insertion(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c > > index 2c27627b6659..f15b20da5315 100644 > > --- a/drivers/gpu/drm/radeon/atom.c > > +++ b/drivers/gpu/drm/radeon/atom.c > > @@ -1211,8 +1211,7 @@ static int atom_execute_table_locked(struct > > atom_context *ctx, int index, uint32 > > SDEBUG("<<\n"); > > > > free: > > - if (ws) > > - kfree(ectx.ws); > > + kfree(ectx.ws); > > return ret; > > } > > > > ___ > dri-devel mailing list > dri-de...@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/radeon: cleanup coding style a bit
Am 30.04.20 um 13:00 schrieb Bernard: 发件人:Joe Perches 发送日期:2020-04-27 01:53:06 收件人:"Christian König" ,Bernard Zhao ,Alex Deucher ,"David (ChunMing) Zhou" ,David Airlie ,Daniel Vetter ,amd-gfx@lists.freedesktop.org,dri-de...@lists.freedesktop.org,linux-ker...@vger.kernel.org 抄送人:opensource.ker...@vivo.com 主题:Re: [PATCH] drm/radeon: cleanup coding style a bit>On Sun, 2020-04-26 at 15:18 +0200, Christian König wrote: Am 26.04.20 um 15:12 schrieb Bernard Zhao: Maybe no need to check ws before kmalloc, kmalloc will check itself, kmalloc`s logic is if ptr is NULL, kmalloc will just return Signed-off-by: Bernard Zhao Reviewed-by: Christian König I'm wondering why the automated scripts haven't found that one before. because this pattern is if (foo) kfree(bar); and the pattern looked for is: if (foo) kfree(foo); diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c [] @@ -1211,8 +1211,7 @@ static int atom_execute_table_locked(struct atom_context *ctx, int index, uint32 SDEBUG("<<\n"); free: - if (ws) - kfree(ectx.ws); + kfree(ectx.ws); return ret; } I'm wondering if this removal is correct as the function is named _locked and it may be recursive or called under some external lock. Hi I am a little confused about this. I understand that the caller guarantees the lock protection that we will not release the wrong pointer. And the NULL check is the same with the first check in kfree? Maybe we do not need check twich. I don't understand the comment either. When you look at the function you see that code is freeing up the temporary allocated buffer which is to large for the stack. In other words we kcalloc() this buffer a few lines above and free it here again. So I think the patch is perfectly valid. What we could do is to update the coci pattern to catch this as well, but this case is so rare that it is probably not worth it. Regards, Christian. Regards, Bernard ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re:Re: [PATCH] drm/radeon: cleanup coding style a bit
发件人:Joe Perches 发送日期:2020-04-27 01:53:06 收件人:"Christian König" ,Bernard Zhao ,Alex Deucher ,"David (ChunMing) Zhou" ,David Airlie ,Daniel Vetter ,amd-gfx@lists.freedesktop.org,dri-de...@lists.freedesktop.org,linux-ker...@vger.kernel.org 抄送人:opensource.ker...@vivo.com 主题:Re: [PATCH] drm/radeon: cleanup coding style a bit>On Sun, 2020-04-26 at 15:18 +0200, Christian König wrote: >> Am 26.04.20 um 15:12 schrieb Bernard Zhao: >> > Maybe no need to check ws before kmalloc, kmalloc will check >> > itself, kmalloc`s logic is if ptr is NULL, kmalloc will just >> > return >> > >> > Signed-off-by: Bernard Zhao >> >> Reviewed-by: Christian König >> >> I'm wondering why the automated scripts haven't found that one before. > >because this pattern is > > if (foo) > kfree(bar); > >and the pattern looked for is: > > if (foo) > kfree(foo); > >> > diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c >[] >> > @@ -1211,8 +1211,7 @@ static int atom_execute_table_locked(struct >> > atom_context *ctx, int index, uint32 >> >SDEBUG("<<\n"); >> > >> > free: >> > - if (ws) >> > - kfree(ectx.ws); >> > + kfree(ectx.ws); >> >return ret; >> > } > >I'm wondering if this removal is correct as the function >is named _locked and it may be recursive or called under >some external lock. > Hi I am a little confused about this. I understand that the caller guarantees the lock protection that we will not release the wrong pointer. And the NULL check is the same with the first check in kfree? Maybe we do not need check twich. Regards, Bernard ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/radeon: cleanup coding style a bit
On Sun, 2020-04-26 at 15:18 +0200, Christian König wrote: > Am 26.04.20 um 15:12 schrieb Bernard Zhao: > > Maybe no need to check ws before kmalloc, kmalloc will check > > itself, kmalloc`s logic is if ptr is NULL, kmalloc will just > > return > > > > Signed-off-by: Bernard Zhao > > Reviewed-by: Christian König > > I'm wondering why the automated scripts haven't found that one before. because this pattern is if (foo) kfree(bar); and the pattern looked for is: if (foo) kfree(foo); > > diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c [] > > @@ -1211,8 +1211,7 @@ static int atom_execute_table_locked(struct > > atom_context *ctx, int index, uint32 > > SDEBUG("<<\n"); > > > > free: > > - if (ws) > > - kfree(ectx.ws); > > + kfree(ectx.ws); > > return ret; > > } I'm wondering if this removal is correct as the function is named _locked and it may be recursive or called under some external lock. ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
[PATCH] drm/radeon: cleanup coding style a bit
Maybe no need to check ws before kmalloc, kmalloc will check itself, kmalloc`s logic is if ptr is NULL, kmalloc will just return Signed-off-by: Bernard Zhao --- drivers/gpu/drm/radeon/atom.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c index 2c27627b6659..f15b20da5315 100644 --- a/drivers/gpu/drm/radeon/atom.c +++ b/drivers/gpu/drm/radeon/atom.c @@ -1211,8 +1211,7 @@ static int atom_execute_table_locked(struct atom_context *ctx, int index, uint32 SDEBUG("<<\n"); free: - if (ws) - kfree(ectx.ws); + kfree(ectx.ws); return ret; } -- 2.26.2 ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Re: [PATCH] drm/radeon: cleanup coding style a bit
btw: the debugging macros in atom.c are not good. It could be something like the below as the output logging is at best poorly formatted due to the many individual printks without KERN_ that are emitted on separate lines. #define ATOM_DEBUG should probably be commented out. The debugging macros and #include file should be better formatted. The no_printk macro is useful to verify formats and arguments when not debugging and removing the ATOM_DEBUG from atom-names.h does not cause the unused char *arrays to be added to the object file as the compiler elides unused arrays. --- drivers/gpu/drm/radeon/atom-names.h | 266 +++- drivers/gpu/drm/radeon/atom.c | 23 ++-- 2 files changed, 218 insertions(+), 71 deletions(-) diff --git a/drivers/gpu/drm/radeon/atom-names.h b/drivers/gpu/drm/radeon/atom-names.h index 6f907a5..055775 100644 --- a/drivers/gpu/drm/radeon/atom-names.h +++ b/drivers/gpu/drm/radeon/atom-names.h @@ -27,74 +27,218 @@ #include "atom.h" -#ifdef ATOM_DEBUG - #define ATOM_OP_NAMES_CNT 123 -static char *atom_op_names[ATOM_OP_NAMES_CNT] = { -"RESERVED", "MOVE_REG", "MOVE_PS", "MOVE_WS", "MOVE_FB", "MOVE_PLL", -"MOVE_MC", "AND_REG", "AND_PS", "AND_WS", "AND_FB", "AND_PLL", "AND_MC", -"OR_REG", "OR_PS", "OR_WS", "OR_FB", "OR_PLL", "OR_MC", "SHIFT_LEFT_REG", -"SHIFT_LEFT_PS", "SHIFT_LEFT_WS", "SHIFT_LEFT_FB", "SHIFT_LEFT_PLL", -"SHIFT_LEFT_MC", "SHIFT_RIGHT_REG", "SHIFT_RIGHT_PS", "SHIFT_RIGHT_WS", -"SHIFT_RIGHT_FB", "SHIFT_RIGHT_PLL", "SHIFT_RIGHT_MC", "MUL_REG", -"MUL_PS", "MUL_WS", "MUL_FB", "MUL_PLL", "MUL_MC", "DIV_REG", "DIV_PS", -"DIV_WS", "DIV_FB", "DIV_PLL", "DIV_MC", "ADD_REG", "ADD_PS", "ADD_WS", -"ADD_FB", "ADD_PLL", "ADD_MC", "SUB_REG", "SUB_PS", "SUB_WS", "SUB_FB", -"SUB_PLL", "SUB_MC", "SET_ATI_PORT", "SET_PCI_PORT", "SET_SYS_IO_PORT", -"SET_REG_BLOCK", "SET_FB_BASE", "COMPARE_REG", "COMPARE_PS", -"COMPARE_WS", "COMPARE_FB", "COMPARE_PLL", "COMPARE_MC", "SWITCH", -"JUMP", "JUMP_EQUAL", "JUMP_BELOW", "JUMP_ABOVE", "JUMP_BELOW_OR_EQUAL", -"JUMP_ABOVE_OR_EQUAL", "JUMP_NOT_EQUAL", "TEST_REG", "TEST_PS", "TEST_WS", -"TEST_FB", "TEST_PLL", "TEST_MC", "DELAY_MILLISEC", "DELAY_MICROSEC", -"CALL_TABLE", "REPEAT", "CLEAR_REG", "CLEAR_PS", "CLEAR_WS", "CLEAR_FB", -"CLEAR_PLL", "CLEAR_MC", "NOP", "EOT", "MASK_REG", "MASK_PS", "MASK_WS", -"MASK_FB", "MASK_PLL", "MASK_MC", "POST_CARD", "BEEP", "SAVE_REG", -"RESTORE_REG", "SET_DATA_BLOCK", "XOR_REG", "XOR_PS", "XOR_WS", "XOR_FB", -"XOR_PLL", "XOR_MC", "SHL_REG", "SHL_PS", "SHL_WS", "SHL_FB", "SHL_PLL", -"SHL_MC", "SHR_REG", "SHR_PS", "SHR_WS", "SHR_FB", "SHR_PLL", "SHR_MC", -"DEBUG", "CTB_DS", +static const char * const atom_op_names[ATOM_OP_NAMES_CNT] = { + "RESERVED", + "MOVE_REG", + "MOVE_PS", + "MOVE_WS", + "MOVE_FB", + "MOVE_PLL", + "MOVE_MC", + "AND_REG", + "AND_PS", + "AND_WS", + "AND_FB", + "AND_PLL", + "AND_MC", + "OR_REG", + "OR_PS", + "OR_WS", + "OR_FB", + "OR_PLL", + "OR_MC", + "SHIFT_LEFT_REG", + "SHIFT_LEFT_PS", + "SHIFT_LEFT_WS", + "SHIFT_LEFT_FB", + "SHIFT_LEFT_PLL", + "SHIFT_LEFT_MC", + "SHIFT_RIGHT_REG", + "SHIFT_RIGHT_PS", + "SHIFT_RIGHT_WS", + "SHIFT_RIGHT_FB", + "SHIFT_RIGHT_PLL", + "SHIFT_RIGHT_MC", + "MUL_REG", + "MUL_PS", + "MUL_WS", + "MUL_FB", + "MUL_PLL", + "MUL_MC", + "DIV_REG", + "DIV_PS", + "DIV_WS", + "DIV_FB", + "DIV_PLL", + "DIV_MC", + "ADD_REG", + "ADD_PS", + "ADD_WS", + "ADD_FB", + "ADD_PLL", + "ADD_MC", + "SUB_REG", + "SUB_PS", + "SUB_WS", + "SUB_FB", + "SUB_PLL", + "SUB_MC", + "SET_ATI_PORT", + "SET_PCI_PORT", + "SET_SYS_IO_PORT", + "SET_REG_BLOCK", + "SET_FB_BASE", + "COMPARE_REG", + "COMPARE_PS", + "COMPARE_WS", + "COMPARE_FB", + "COMPARE_PLL", + "COMPARE_MC", + "SWITCH", + "JUMP", + "JUMP_EQUAL", + "JUMP_BELOW", + "JUMP_ABOVE", + "JUMP_BELOW_OR_EQUAL", + "JUMP_ABOVE_OR_EQUAL", + "JUMP_NOT_EQUAL", + "TEST_REG", + "TEST_PS", + "TEST_WS", + "TEST_FB", + "TEST_PLL", + "TEST_MC", + "DELAY_MILLISEC", + "DELAY_MICROSEC", + "CALL_TABLE", + "REPEAT", + "CLEAR_REG", + "CLEAR_PS", + "CLEAR_WS", + "CLEAR_FB", + "CLEAR_PLL", + "CLEAR_MC", + "NOP", + "EOT", + "MASK_REG", + "MASK_PS", + "MASK_WS", + "MASK_FB", + "MASK_PLL", + "MASK_MC", + "POST_CARD", + "BEEP", + "SAVE_REG", + "RESTORE_REG", + "SET_DATA_BLOCK", + "XOR_REG", + "XOR_PS", + "XOR_WS", + "XOR_FB", + "XOR_PLL", + "XOR_MC", + "SHL_REG", + "SHL_PS", +
Re: [PATCH] drm/radeon: cleanup coding style a bit
Am 26.04.20 um 15:12 schrieb Bernard Zhao: Maybe no need to check ws before kmalloc, kmalloc will check itself, kmalloc`s logic is if ptr is NULL, kmalloc will just return Signed-off-by: Bernard Zhao Reviewed-by: Christian König I'm wondering why the automated scripts haven't found that one before. --- drivers/gpu/drm/radeon/atom.c | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c index 2c27627b6659..f15b20da5315 100644 --- a/drivers/gpu/drm/radeon/atom.c +++ b/drivers/gpu/drm/radeon/atom.c @@ -1211,8 +1211,7 @@ static int atom_execute_table_locked(struct atom_context *ctx, int index, uint32 SDEBUG("<<\n"); free: - if (ws) - kfree(ectx.ws); + kfree(ectx.ws); return ret; } ___ amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx