* [PATCH] drm/radeon: cleanup coding style a bit
@ 2020-04-26 13:12 Bernard Zhao
2020-04-26 13:18 ` Christian König
0 siblings, 1 reply; 7+ messages in thread
From: Bernard Zhao @ 2020-04-26 13:12 UTC (permalink / raw)
To: Alex Deucher, Christian König, David (ChunMing) Zhou,
David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel
Cc: opensource.kernel, 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 <bernard@vivo.com>
---
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/radeon: cleanup coding style a bit
2020-04-26 13:12 [PATCH] drm/radeon: cleanup coding style a bit Bernard Zhao
@ 2020-04-26 13:18 ` Christian König
2020-04-26 17:53 ` Joe Perches
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Christian König @ 2020-04-26 13:18 UTC (permalink / raw)
To: Bernard Zhao, Alex Deucher, David (ChunMing) Zhou, David Airlie,
Daniel Vetter, amd-gfx, dri-devel, linux-kernel
Cc: opensource.kernel
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 <bernard@vivo.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
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;
> }
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/radeon: cleanup coding style a bit
2020-04-26 13:18 ` Christian König
@ 2020-04-26 17:53 ` Joe Perches
2020-04-30 11:00 ` Bernard
2020-04-26 18:06 ` Joe Perches
2020-04-30 14:53 ` Alex Deucher
2 siblings, 1 reply; 7+ messages in thread
From: Joe Perches @ 2020-04-26 17:53 UTC (permalink / raw)
To: Christian König, Bernard Zhao, Alex Deucher,
David (ChunMing) Zhou, David Airlie, Daniel Vetter, amd-gfx,
dri-devel, linux-kernel
Cc: opensource.kernel
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 <bernard@vivo.com>
>
> Reviewed-by: Christian König <christian.koenig@amd.com>
>
> 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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/radeon: cleanup coding style a bit
2020-04-26 13:18 ` Christian König
2020-04-26 17:53 ` Joe Perches
@ 2020-04-26 18:06 ` Joe Perches
2020-04-30 14:53 ` Alex Deucher
2 siblings, 0 replies; 7+ messages in thread
From: Joe Perches @ 2020-04-26 18:06 UTC (permalink / raw)
To: Christian König, Bernard Zhao, Alex Deucher,
David (ChunMing) Zhou, David Airlie, Daniel Vetter, amd-gfx,
dri-devel, linux-kernel
Cc: opensource.kernel
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_<LEVEL> 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",
+ "SHL_WS",
+ "SHL_FB",
+ "SHL_PLL",
+ "SHL_MC",
+ "SHR_REG",
+ "SHR_PS",
+ "SHR_WS",
+ "SHR_FB",
+ "SHR_PLL",
+ "SHR_MC",
+ "DEBUG",
+ "CTB_DS",
};
#define ATOM_TABLE_NAMES_CNT 74
-static char *atom_table_names[ATOM_TABLE_NAMES_CNT] = {
-"ASIC_Init", "GetDisplaySurfaceSize", "ASIC_RegistersInit",
-"VRAM_BlockVenderDetection", "SetClocksRatio", "MemoryControllerInit",
-"GPIO_PinInit", "MemoryParamAdjust", "DVOEncoderControl",
-"GPIOPinControl", "SetEngineClock", "SetMemoryClock", "SetPixelClock",
-"DynamicClockGating", "ResetMemoryDLL", "ResetMemoryDevice",
-"MemoryPLLInit", "EnableMemorySelfRefresh", "AdjustMemoryController",
-"EnableASIC_StaticPwrMgt", "ASIC_StaticPwrMgtStatusChange",
-"DAC_LoadDetection", "TMDS2EncoderControl", "LCD1OutputControl",
-"DAC1EncoderControl", "DAC2EncoderControl", "DVOOutputControl",
-"CV1OutputControl", "SetCRTC_DPM_State", "TVEncoderControl",
-"TMDS1EncoderControl", "LVDSEncoderControl", "TV1OutputControl",
-"EnableScaler", "BlankCRTC", "EnableCRTC", "GetPixelClock",
-"EnableVGA_Render", "EnableVGA_Access", "SetCRTC_Timing",
-"SetCRTC_OverScan", "SetCRTC_Replication", "SelectCRTC_Source",
-"EnableGraphSurfaces", "UpdateCRTC_DoubleBufferRegisters",
-"LUT_AutoFill", "EnableHW_IconCursor", "GetMemoryClock",
-"GetEngineClock", "SetCRTC_UsingDTDTiming", "TVBootUpStdPinDetection",
-"DFP2OutputControl", "VRAM_BlockDetectionByStrap", "MemoryCleanUp",
-"ReadEDIDFromHWAssistedI2C", "WriteOneByteToHWAssistedI2C",
-"ReadHWAssistedI2CStatus", "SpeedFanControl", "PowerConnectorDetection",
-"MC_Synchronization", "ComputeMemoryEnginePLL", "MemoryRefreshConversion",
-"VRAM_GetCurrentInfoBlock", "DynamicMemorySettings", "MemoryTraining",
-"EnableLVDS_SS", "DFP1OutputControl", "SetVoltage", "CRT1OutputControl",
-"CRT2OutputControl", "SetupHWAssistedI2CStatus", "ClockSource",
-"MemoryDeviceInit", "EnableYUV",
+static const char * const atom_table_names[ATOM_TABLE_NAMES_CNT] = {
+ "ASIC_Init",
+ "GetDisplaySurfaceSize",
+ "ASIC_RegistersInit",
+ "VRAM_BlockVenderDetection",
+ "SetClocksRatio",
+ "MemoryControllerInit",
+ "GPIO_PinInit",
+ "MemoryParamAdjust",
+ "DVOEncoderControl",
+ "GPIOPinControl",
+ "SetEngineClock",
+ "SetMemoryClock",
+ "SetPixelClock",
+ "DynamicClockGating",
+ "ResetMemoryDLL",
+ "ResetMemoryDevice",
+ "MemoryPLLInit",
+ "EnableMemorySelfRefresh",
+ "AdjustMemoryController",
+ "EnableASIC_StaticPwrMgt",
+ "ASIC_StaticPwrMgtStatusChange",
+ "DAC_LoadDetection",
+ "TMDS2EncoderControl",
+ "LCD1OutputControl",
+ "DAC1EncoderControl",
+ "DAC2EncoderControl",
+ "DVOOutputControl",
+ "CV1OutputControl",
+ "SetCRTC_DPM_State",
+ "TVEncoderControl",
+ "TMDS1EncoderControl",
+ "LVDSEncoderControl",
+ "TV1OutputControl",
+ "EnableScaler",
+ "BlankCRTC",
+ "EnableCRTC",
+ "GetPixelClock",
+ "EnableVGA_Render",
+ "EnableVGA_Access",
+ "SetCRTC_Timing",
+ "SetCRTC_OverScan",
+ "SetCRTC_Replication",
+ "SelectCRTC_Source",
+ "EnableGraphSurfaces",
+ "UpdateCRTC_DoubleBufferRegisters",
+ "LUT_AutoFill",
+ "EnableHW_IconCursor",
+ "GetMemoryClock",
+ "GetEngineClock",
+ "SetCRTC_UsingDTDTiming",
+ "TVBootUpStdPinDetection",
+ "DFP2OutputControl",
+ "VRAM_BlockDetectionByStrap",
+ "MemoryCleanUp",
+ "ReadEDIDFromHWAssistedI2C",
+ "WriteOneByteToHWAssistedI2C",
+ "ReadHWAssistedI2CStatus",
+ "SpeedFanControl",
+ "PowerConnectorDetection",
+ "MC_Synchronization",
+ "ComputeMemoryEnginePLL",
+ "MemoryRefreshConversion",
+ "VRAM_GetCurrentInfoBlock",
+ "DynamicMemorySettings",
+ "MemoryTraining",
+ "EnableLVDS_SS",
+ "DFP1OutputControl",
+ "SetVoltage",
+ "CRT1OutputControl",
+ "CRT2OutputControl",
+ "SetupHWAssistedI2CStatus",
+ "ClockSource",
+ "MemoryDeviceInit",
+ "EnableYUV",
};
#define ATOM_IO_NAMES_CNT 5
-static char *atom_io_names[ATOM_IO_NAMES_CNT] = {
-"MM", "PLL", "MC", "PCIE", "PCIE PORT",
+static const char * const atom_io_names[ATOM_IO_NAMES_CNT] = {
+ "MM",
+ "PLL",
+ "MC",
+ "PCIE",
+ "PCIE PORT",
};
-#else
-
-#define ATOM_OP_NAMES_CNT 0
-#define ATOM_TABLE_NAMES_CNT 0
-#define ATOM_IO_NAMES_CNT 0
-
-#endif
-
#endif
diff --git a/drivers/gpu/drm/radeon/atom.c b/drivers/gpu/drm/radeon/atom.c
index 2c2762..892eeb 100644
--- a/drivers/gpu/drm/radeon/atom.c
+++ b/drivers/gpu/drm/radeon/atom.c
@@ -91,17 +91,20 @@ static int atom_def_dst[8] = { 0, 0, 1, 2, 0, 1, 2, 3 };
static int debug_depth = 0;
#ifdef ATOM_DEBUG
-static void debug_print_spaces(int n)
-{
- while (n--)
- printk(" ");
-}
-
-#define DEBUG(...) do if (atom_debug) { printk(KERN_DEBUG __VA_ARGS__); } while (0)
-#define SDEBUG(...) do if (atom_debug) { printk(KERN_DEBUG); debug_print_spaces(debug_depth); printk(__VA_ARGS__); } while (0)
+#define DEBUG(fmt, ...) \
+do { \
+ if (atom_debug) \
+ printk(KERN_DEBUG fmt, ##__VA_ARGS__); \
+} while (0)
+#define SDEBUG(fmt, ...) \
+do { \
+ if (atom_debug) \
+ printk(KERN_DEBUG "%*s" fmt, \
+ debug_depth * 3, "", ##__VA_ARGS__); \
+} while (0)
#else
-#define DEBUG(...) do { } while (0)
-#define SDEBUG(...) do { } while (0)
+#define DEBUG(fmt, ...) no_printk(fmt, ##__VA_ARGS__)
+#define SDEBUG(fmt, ...) no_printk(fmt, ##__VA_ARGS__)
#endif
static uint32_t atom_iio_execute(struct atom_context *ctx, int base,
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re:Re: [PATCH] drm/radeon: cleanup coding style a bit
2020-04-26 17:53 ` Joe Perches
@ 2020-04-30 11:00 ` Bernard
2020-04-30 13:32 ` Christian König
0 siblings, 1 reply; 7+ messages in thread
From: Bernard @ 2020-04-30 11:00 UTC (permalink / raw)
To: Joe Perches
Cc: Christian König, Alex Deucher, David (ChunMing) Zhou,
David Airlie, Daniel Vetter, amd-gfx, dri-devel, linux-kernel,
opensource.kernel
发件人：Joe Perches <joe@perches.com>
发送日期：2020-04-27 01:53:06
收件人："Christian König" <christian.koenig@amd.com>,Bernard Zhao <bernard@vivo.com>,Alex Deucher <alexander.deucher@amd.com>,"David (ChunMing) Zhou" <David1.Zhou@amd.com>,David Airlie <airlied@linux.ie>,Daniel Vetter <daniel@ffwll.ch>,amd-gfx@lists.freedesktop.org,dri-devel@lists.freedesktop.org,linux-kernel@vger.kernel.org
抄送人：opensource.kernel@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 <bernard@vivo.com>
>>
>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>
>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/radeon: cleanup coding style a bit
2020-04-30 11:00 ` Bernard
@ 2020-04-30 13:32 ` Christian König
0 siblings, 0 replies; 7+ messages in thread
From: Christian König @ 2020-04-30 13:32 UTC (permalink / raw)
To: Bernard, Joe Perches
Cc: David (ChunMing) Zhou, opensource.kernel, David Airlie,
linux-kernel, amd-gfx, dri-devel, Daniel Vetter, Alex Deucher,
Christian König
Am 30.04.20 um 13:00 schrieb Bernard:
>
> 发件人：Joe Perches <joe@perches.com>
> 发送日期：2020-04-27 01:53:06
> 收件人："Christian König" <christian.koenig@amd.com>,Bernard Zhao <bernard@vivo.com>,Alex Deucher <alexander.deucher@amd.com>,"David (ChunMing) Zhou" <David1.Zhou@amd.com>,David Airlie <airlied@linux.ie>,Daniel Vetter <daniel@ffwll.ch>,amd-gfx@lists.freedesktop.org,dri-devel@lists.freedesktop.org,linux-kernel@vger.kernel.org
> 抄送人：opensource.kernel@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 <bernard@vivo.com>
>>> Reviewed-by: Christian König <christian.koenig@amd.com>
>>>
>>> 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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/radeon: cleanup coding style a bit
2020-04-26 13:18 ` Christian König
2020-04-26 17:53 ` Joe Perches
2020-04-26 18:06 ` Joe Perches
@ 2020-04-30 14:53 ` Alex Deucher
2 siblings, 0 replies; 7+ messages in thread
From: Alex Deucher @ 2020-04-30 14:53 UTC (permalink / raw)
To: Christian König
Cc: Bernard Zhao, Alex Deucher, David (ChunMing) Zhou, David Airlie,
Daniel Vetter, amd-gfx list, Maling list - DRI developers, LKML,
opensource.kernel
Applied. Thanks!
Alex
On Sun, Apr 26, 2020 at 9:18 AM Christian König
<christian.koenig@amd.com> 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 <bernard@vivo.com>
>
> Reviewed-by: Christian König <christian.koenig@amd.com>
>
> 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-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-04-30 14:53 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-04-26 13:12 [PATCH] drm/radeon: cleanup coding style a bit Bernard Zhao
2020-04-26 13:18 ` Christian König
2020-04-26 17:53 ` Joe Perches
2020-04-30 11:00 ` Bernard
2020-04-30 13:32 ` Christian König
2020-04-26 18:06 ` Joe Perches
2020-04-30 14:53 ` Alex Deucher
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).