Initializing global variable to 0 or NULL is not necessary and should be avoided. Issue reported by checkpatch script as: ERROR: do not initialise globals to 0 (or NULL).
Signed-off-by: Deepak R Varma mh12gx2825@gmail.com --- drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 46 ++++++++++++------------- drivers/gpu/drm/amd/amdgpu/atom.c | 4 +-- 2 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 8ab6126ff70c..6de94c46bc91 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -94,16 +94,16 @@ #define KMS_DRIVER_MINOR 40 #define KMS_DRIVER_PATCHLEVEL 0
-int amdgpu_vram_limit = 0; -int amdgpu_vis_vram_limit = 0; +int amdgpu_vram_limit; +int amdgpu_vis_vram_limit; int amdgpu_gart_size = -1; /* auto */ int amdgpu_gtt_size = -1; /* auto */ int amdgpu_moverate = -1; /* auto */ -int amdgpu_benchmarking = 0; -int amdgpu_testing = 0; +int amdgpu_benchmarking; +int amdgpu_testing; int amdgpu_audio = -1; -int amdgpu_disp_priority = 0; -int amdgpu_hw_i2c = 0; +int amdgpu_disp_priority; +int amdgpu_hw_i2c; int amdgpu_pcie_gen2 = -1; int amdgpu_msi = -1; char amdgpu_lockup_timeout[AMDGPU_MAX_TIMEOUT_PARAM_LENGTH]; @@ -113,43 +113,43 @@ int amdgpu_aspm = -1; int amdgpu_runtime_pm = -1; uint amdgpu_ip_block_mask = 0xffffffff; int amdgpu_bapm = -1; -int amdgpu_deep_color = 0; +int amdgpu_deep_color; int amdgpu_vm_size = -1; int amdgpu_vm_fragment_size = -1; int amdgpu_vm_block_size = -1; -int amdgpu_vm_fault_stop = 0; -int amdgpu_vm_debug = 0; +int amdgpu_vm_fault_stop; +int amdgpu_vm_debug; int amdgpu_vm_update_mode = -1; -int amdgpu_exp_hw_support = 0; +int amdgpu_exp_hw_support; int amdgpu_dc = -1; int amdgpu_sched_jobs = 32; int amdgpu_sched_hw_submission = 2; -uint amdgpu_pcie_gen_cap = 0; -uint amdgpu_pcie_lane_cap = 0; +uint amdgpu_pcie_gen_cap; +uint amdgpu_pcie_lane_cap; uint amdgpu_cg_mask = 0xffffffff; uint amdgpu_pg_mask = 0xffffffff; uint amdgpu_sdma_phase_quantum = 32; -char *amdgpu_disable_cu = NULL; -char *amdgpu_virtual_display = NULL; +char *amdgpu_disable_cu; +char *amdgpu_virtual_display; /* OverDrive(bit 14) disabled by default*/ uint amdgpu_pp_feature_mask = 0xffffbfff; -uint amdgpu_force_long_training = 0; -int amdgpu_job_hang_limit = 0; +uint amdgpu_force_long_training; +int amdgpu_job_hang_limit; int amdgpu_lbpw = -1; int amdgpu_compute_multipipe = -1; int amdgpu_gpu_recovery = -1; /* auto */ -int amdgpu_emu_mode = 0; -uint amdgpu_smu_memory_pool_size = 0; +int amdgpu_emu_mode; +uint amdgpu_smu_memory_pool_size; /* FBC (bit 0) disabled by default*/ -uint amdgpu_dc_feature_mask = 0; -uint amdgpu_dc_debug_mask = 0; +uint amdgpu_dc_feature_mask; +uint amdgpu_dc_debug_mask; int amdgpu_async_gfx_ring = 1; -int amdgpu_mcbp = 0; +int amdgpu_mcbp; int amdgpu_discovery = -1; -int amdgpu_mes = 0; +int amdgpu_mes; int amdgpu_noretry = -1; int amdgpu_force_asic_type = -1; -int amdgpu_tmz = 0; +int amdgpu_tmz; int amdgpu_reset_method = -1; /* auto */ int amdgpu_num_kcq = -1;
diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c b/drivers/gpu/drm/amd/amdgpu/atom.c index 696e97ab77eb..46c00ee580b1 100644 --- a/drivers/gpu/drm/amd/amdgpu/atom.c +++ b/drivers/gpu/drm/amd/amdgpu/atom.c @@ -66,7 +66,7 @@ typedef struct { bool abort; } atom_exec_context;
-int amdgpu_atom_debug = 0; +int amdgpu_atom_debug; static int amdgpu_atom_execute_table_locked(struct atom_context *ctx, int index, uint32_t * params); int amdgpu_atom_execute_table(struct atom_context *ctx, int index, uint32_t * params);
@@ -88,7 +88,7 @@ static int atom_dst_to_src[8][4] = { }; static int atom_def_dst[8] = { 0, 0, 1, 2, 0, 1, 2, 3 };
-static int debug_depth = 0; +static int debug_depth; #ifdef ATOM_DEBUG static void debug_print_spaces(int n) {
On Mon, Nov 2, 2020 at 1:42 PM Deepak R Varma mh12gx2825@gmail.com wrote:
Initializing global variable to 0 or NULL is not necessary and should be avoided. Issue reported by checkpatch script as: ERROR: do not initialise globals to 0 (or NULL).
I agree that this is technically correct, but a lot of people don't seem to know that so we get a lot of comments about this code for the variables that are not explicitly set. Seems less confusing to initialize them even if it not necessary. I don't have a particularly strong opinion on it however.
Alex
Signed-off-by: Deepak R Varma mh12gx2825@gmail.com
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 46 ++++++++++++------------- drivers/gpu/drm/amd/amdgpu/atom.c | 4 +-- 2 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 8ab6126ff70c..6de94c46bc91 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -94,16 +94,16 @@ #define KMS_DRIVER_MINOR 40 #define KMS_DRIVER_PATCHLEVEL 0
-int amdgpu_vram_limit = 0; -int amdgpu_vis_vram_limit = 0; +int amdgpu_vram_limit; +int amdgpu_vis_vram_limit; int amdgpu_gart_size = -1; /* auto */ int amdgpu_gtt_size = -1; /* auto */ int amdgpu_moverate = -1; /* auto */ -int amdgpu_benchmarking = 0; -int amdgpu_testing = 0; +int amdgpu_benchmarking; +int amdgpu_testing; int amdgpu_audio = -1; -int amdgpu_disp_priority = 0; -int amdgpu_hw_i2c = 0; +int amdgpu_disp_priority; +int amdgpu_hw_i2c; int amdgpu_pcie_gen2 = -1; int amdgpu_msi = -1; char amdgpu_lockup_timeout[AMDGPU_MAX_TIMEOUT_PARAM_LENGTH]; @@ -113,43 +113,43 @@ int amdgpu_aspm = -1; int amdgpu_runtime_pm = -1; uint amdgpu_ip_block_mask = 0xffffffff; int amdgpu_bapm = -1; -int amdgpu_deep_color = 0; +int amdgpu_deep_color; int amdgpu_vm_size = -1; int amdgpu_vm_fragment_size = -1; int amdgpu_vm_block_size = -1; -int amdgpu_vm_fault_stop = 0; -int amdgpu_vm_debug = 0; +int amdgpu_vm_fault_stop; +int amdgpu_vm_debug; int amdgpu_vm_update_mode = -1; -int amdgpu_exp_hw_support = 0; +int amdgpu_exp_hw_support; int amdgpu_dc = -1; int amdgpu_sched_jobs = 32; int amdgpu_sched_hw_submission = 2; -uint amdgpu_pcie_gen_cap = 0; -uint amdgpu_pcie_lane_cap = 0; +uint amdgpu_pcie_gen_cap; +uint amdgpu_pcie_lane_cap; uint amdgpu_cg_mask = 0xffffffff; uint amdgpu_pg_mask = 0xffffffff; uint amdgpu_sdma_phase_quantum = 32; -char *amdgpu_disable_cu = NULL; -char *amdgpu_virtual_display = NULL; +char *amdgpu_disable_cu; +char *amdgpu_virtual_display; /* OverDrive(bit 14) disabled by default*/ uint amdgpu_pp_feature_mask = 0xffffbfff; -uint amdgpu_force_long_training = 0; -int amdgpu_job_hang_limit = 0; +uint amdgpu_force_long_training; +int amdgpu_job_hang_limit; int amdgpu_lbpw = -1; int amdgpu_compute_multipipe = -1; int amdgpu_gpu_recovery = -1; /* auto */ -int amdgpu_emu_mode = 0; -uint amdgpu_smu_memory_pool_size = 0; +int amdgpu_emu_mode; +uint amdgpu_smu_memory_pool_size; /* FBC (bit 0) disabled by default*/ -uint amdgpu_dc_feature_mask = 0; -uint amdgpu_dc_debug_mask = 0; +uint amdgpu_dc_feature_mask; +uint amdgpu_dc_debug_mask; int amdgpu_async_gfx_ring = 1; -int amdgpu_mcbp = 0; +int amdgpu_mcbp; int amdgpu_discovery = -1; -int amdgpu_mes = 0; +int amdgpu_mes; int amdgpu_noretry = -1; int amdgpu_force_asic_type = -1; -int amdgpu_tmz = 0; +int amdgpu_tmz; int amdgpu_reset_method = -1; /* auto */ int amdgpu_num_kcq = -1;
diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c b/drivers/gpu/drm/amd/amdgpu/atom.c index 696e97ab77eb..46c00ee580b1 100644 --- a/drivers/gpu/drm/amd/amdgpu/atom.c +++ b/drivers/gpu/drm/amd/amdgpu/atom.c @@ -66,7 +66,7 @@ typedef struct { bool abort; } atom_exec_context;
-int amdgpu_atom_debug = 0; +int amdgpu_atom_debug; static int amdgpu_atom_execute_table_locked(struct atom_context *ctx, int index, uint32_t * params); int amdgpu_atom_execute_table(struct atom_context *ctx, int index, uint32_t * params);
@@ -88,7 +88,7 @@ static int atom_dst_to_src[8][4] = { }; static int atom_def_dst[8] = { 0, 0, 1, 2, 0, 1, 2, 3 };
-static int debug_depth = 0; +static int debug_depth; #ifdef ATOM_DEBUG static void debug_print_spaces(int n) { -- 2.25.1
amd-gfx mailing list amd-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/amd-gfx
Am 02.11.20 um 20:43 schrieb Alex Deucher:
On Mon, Nov 2, 2020 at 1:42 PM Deepak R Varma mh12gx2825@gmail.com wrote:
Initializing global variable to 0 or NULL is not necessary and should be avoided. Issue reported by checkpatch script as: ERROR: do not initialise globals to 0 (or NULL).
I agree that this is technically correct, but a lot of people don't seem to know that so we get a lot of comments about this code for the variables that are not explicitly set. Seems less confusing to initialize them even if it not necessary. I don't have a particularly strong opinion on it however.
Agree with Alex.
Especially for the module parameters we should have a explicit init value for documentation purposes, even when it is 0.
Christian.
Alex
Signed-off-by: Deepak R Varma mh12gx2825@gmail.com
drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 46 ++++++++++++------------- drivers/gpu/drm/amd/amdgpu/atom.c | 4 +-- 2 files changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c index 8ab6126ff70c..6de94c46bc91 100644 --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c @@ -94,16 +94,16 @@ #define KMS_DRIVER_MINOR 40 #define KMS_DRIVER_PATCHLEVEL 0
-int amdgpu_vram_limit = 0; -int amdgpu_vis_vram_limit = 0; +int amdgpu_vram_limit; +int amdgpu_vis_vram_limit; int amdgpu_gart_size = -1; /* auto */ int amdgpu_gtt_size = -1; /* auto */ int amdgpu_moverate = -1; /* auto */ -int amdgpu_benchmarking = 0; -int amdgpu_testing = 0; +int amdgpu_benchmarking; +int amdgpu_testing; int amdgpu_audio = -1; -int amdgpu_disp_priority = 0; -int amdgpu_hw_i2c = 0; +int amdgpu_disp_priority; +int amdgpu_hw_i2c; int amdgpu_pcie_gen2 = -1; int amdgpu_msi = -1; char amdgpu_lockup_timeout[AMDGPU_MAX_TIMEOUT_PARAM_LENGTH]; @@ -113,43 +113,43 @@ int amdgpu_aspm = -1; int amdgpu_runtime_pm = -1; uint amdgpu_ip_block_mask = 0xffffffff; int amdgpu_bapm = -1; -int amdgpu_deep_color = 0; +int amdgpu_deep_color; int amdgpu_vm_size = -1; int amdgpu_vm_fragment_size = -1; int amdgpu_vm_block_size = -1; -int amdgpu_vm_fault_stop = 0; -int amdgpu_vm_debug = 0; +int amdgpu_vm_fault_stop; +int amdgpu_vm_debug; int amdgpu_vm_update_mode = -1; -int amdgpu_exp_hw_support = 0; +int amdgpu_exp_hw_support; int amdgpu_dc = -1; int amdgpu_sched_jobs = 32; int amdgpu_sched_hw_submission = 2; -uint amdgpu_pcie_gen_cap = 0; -uint amdgpu_pcie_lane_cap = 0; +uint amdgpu_pcie_gen_cap; +uint amdgpu_pcie_lane_cap; uint amdgpu_cg_mask = 0xffffffff; uint amdgpu_pg_mask = 0xffffffff; uint amdgpu_sdma_phase_quantum = 32; -char *amdgpu_disable_cu = NULL; -char *amdgpu_virtual_display = NULL; +char *amdgpu_disable_cu; +char *amdgpu_virtual_display; /* OverDrive(bit 14) disabled by default*/ uint amdgpu_pp_feature_mask = 0xffffbfff; -uint amdgpu_force_long_training = 0; -int amdgpu_job_hang_limit = 0; +uint amdgpu_force_long_training; +int amdgpu_job_hang_limit; int amdgpu_lbpw = -1; int amdgpu_compute_multipipe = -1; int amdgpu_gpu_recovery = -1; /* auto */ -int amdgpu_emu_mode = 0; -uint amdgpu_smu_memory_pool_size = 0; +int amdgpu_emu_mode; +uint amdgpu_smu_memory_pool_size; /* FBC (bit 0) disabled by default*/ -uint amdgpu_dc_feature_mask = 0; -uint amdgpu_dc_debug_mask = 0; +uint amdgpu_dc_feature_mask; +uint amdgpu_dc_debug_mask; int amdgpu_async_gfx_ring = 1; -int amdgpu_mcbp = 0; +int amdgpu_mcbp; int amdgpu_discovery = -1; -int amdgpu_mes = 0; +int amdgpu_mes; int amdgpu_noretry = -1; int amdgpu_force_asic_type = -1; -int amdgpu_tmz = 0; +int amdgpu_tmz; int amdgpu_reset_method = -1; /* auto */ int amdgpu_num_kcq = -1;
diff --git a/drivers/gpu/drm/amd/amdgpu/atom.c b/drivers/gpu/drm/amd/amdgpu/atom.c index 696e97ab77eb..46c00ee580b1 100644 --- a/drivers/gpu/drm/amd/amdgpu/atom.c +++ b/drivers/gpu/drm/amd/amdgpu/atom.c @@ -66,7 +66,7 @@ typedef struct { bool abort; } atom_exec_context;
-int amdgpu_atom_debug = 0; +int amdgpu_atom_debug; static int amdgpu_atom_execute_table_locked(struct atom_context *ctx, int index, uint32_t * params); int amdgpu_atom_execute_table(struct atom_context *ctx, int index, uint32_t * params);
@@ -88,7 +88,7 @@ static int atom_dst_to_src[8][4] = { }; static int atom_def_dst[8] = { 0, 0, 1, 2, 0, 1, 2, 3 };
-static int debug_depth = 0; +static int debug_depth; #ifdef ATOM_DEBUG static void debug_print_spaces(int n) { -- 2.25.1
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
On Mon, Nov 02, 2020 at 09:06:21PM +0100, Christian König wrote:
Am 02.11.20 um 20:43 schrieb Alex Deucher:
On Mon, Nov 2, 2020 at 1:42 PM Deepak R Varma mh12gx2825@gmail.com wrote:
Initializing global variable to 0 or NULL is not necessary and should be avoided. Issue reported by checkpatch script as: ERROR: do not initialise globals to 0 (or NULL).
I agree that this is technically correct, but a lot of people don't seem to know that so we get a lot of comments about this code for the variables that are not explicitly set. Seems less confusing to initialize them even if it not necessary. I don't have a particularly strong opinion on it however.
Agree with Alex.
Especially for the module parameters we should have a explicit init value for documentation purposes, even when it is 0.
Why is this one tiny driver somehow special compared to the entire rest of the kernel? (hint, it isn't...)
Please follow the normal coding style rules, there's no reason to ignore them unless you like to constantly reject patches like this that get sent to you.
thnaks,
greg k-h
Am 03.11.20 um 07:53 schrieb Greg KH:
On Mon, Nov 02, 2020 at 09:06:21PM +0100, Christian König wrote:
Am 02.11.20 um 20:43 schrieb Alex Deucher:
On Mon, Nov 2, 2020 at 1:42 PM Deepak R Varma mh12gx2825@gmail.com wrote:
Initializing global variable to 0 or NULL is not necessary and should be avoided. Issue reported by checkpatch script as: ERROR: do not initialise globals to 0 (or NULL).
I agree that this is technically correct, but a lot of people don't seem to know that so we get a lot of comments about this code for the variables that are not explicitly set. Seems less confusing to initialize them even if it not necessary. I don't have a particularly strong opinion on it however.
Agree with Alex.
Especially for the module parameters we should have a explicit init value for documentation purposes, even when it is 0.
Why is this one tiny driver somehow special compared to the entire rest of the kernel? (hint, it isn't...)
And it certainly shouldn't :)
Please follow the normal coding style rules, there's no reason to ignore them unless you like to constantly reject patches like this that get sent to you.
Yeah, that's a rather good point.
Not a particular strong opinion on this either, but when something global is set to 0 people usually do this to emphases that it is important that it is zero.
Regards, Christian.
thnaks,
greg k-h
On Mon, Nov 02, 2020 at 09:48:25PM +0100, Christian König wrote:
Am 03.11.20 um 07:53 schrieb Greg KH:
On Mon, Nov 02, 2020 at 09:06:21PM +0100, Christian König wrote:
Am 02.11.20 um 20:43 schrieb Alex Deucher:
On Mon, Nov 2, 2020 at 1:42 PM Deepak R Varma mh12gx2825@gmail.com wrote:
Initializing global variable to 0 or NULL is not necessary and should be avoided. Issue reported by checkpatch script as: ERROR: do not initialise globals to 0 (or NULL).
I agree that this is technically correct, but a lot of people don't seem to know that so we get a lot of comments about this code for the variables that are not explicitly set. Seems less confusing to initialize them even if it not necessary. I don't have a particularly strong opinion on it however.
Agree with Alex.
Especially for the module parameters we should have a explicit init value for documentation purposes, even when it is 0.
Why is this one tiny driver somehow special compared to the entire rest of the kernel? (hint, it isn't...)
And it certainly shouldn't :)
Please follow the normal coding style rules, there's no reason to ignore them unless you like to constantly reject patches like this that get sent to you.
Yeah, that's a rather good point.
Not a particular strong opinion on this either, but when something global is set to 0 people usually do this to emphases that it is important that it is zero.
Again, no, that's not what we have been doing in the kernel for the past 20+ years. If you do not set it to anything, we all know it is important for it to be set to 0. Otherwise we would explicitly set it to something else. And if we don't care, then that too doesn't matter so we let it be 0 by not initializing it, it doesn't matter.
I think this very change is what started the whole "kernel janitor" movement all those years ago, because it was easily proven that this simple change saved both time and memory.
This shouldn't even be an argument we are having anymore...
thanks,
greg k-h
Am 03.11.20 um 08:53 schrieb Greg KH:
On Mon, Nov 02, 2020 at 09:48:25PM +0100, Christian König wrote:
Am 03.11.20 um 07:53 schrieb Greg KH:
On Mon, Nov 02, 2020 at 09:06:21PM +0100, Christian König wrote:
Am 02.11.20 um 20:43 schrieb Alex Deucher:
On Mon, Nov 2, 2020 at 1:42 PM Deepak R Varma mh12gx2825@gmail.com wrote:
Initializing global variable to 0 or NULL is not necessary and should be avoided. Issue reported by checkpatch script as: ERROR: do not initialise globals to 0 (or NULL).
I agree that this is technically correct, but a lot of people don't seem to know that so we get a lot of comments about this code for the variables that are not explicitly set. Seems less confusing to initialize them even if it not necessary. I don't have a particularly strong opinion on it however.
Agree with Alex.
Especially for the module parameters we should have a explicit init value for documentation purposes, even when it is 0.
Why is this one tiny driver somehow special compared to the entire rest of the kernel? (hint, it isn't...)
And it certainly shouldn't :)
Please follow the normal coding style rules, there's no reason to ignore them unless you like to constantly reject patches like this that get sent to you.
Yeah, that's a rather good point.
Not a particular strong opinion on this either, but when something global is set to 0 people usually do this to emphases that it is important that it is zero.
Again, no, that's not what we have been doing in the kernel for the past 20+ years. If you do not set it to anything, we all know it is important for it to be set to 0. Otherwise we would explicitly set it to something else. And if we don't care, then that too doesn't matter so we let it be 0 by not initializing it, it doesn't matter.
I think this very change is what started the whole "kernel janitor" movement all those years ago, because it was easily proven that this simple change saved both time and memory.
Ok, well that is even better because it is a technical argument.
You have convinced me, the patch is Reviewed-by: Christian König christian.koenig@amd.com.
Regards, Christian.
This shouldn't even be an argument we are having anymore...
thanks,
greg k-h
[AMD Public Use]
-----Original Message----- From: Greg KH gregkh@linuxfoundation.org Sent: Tuesday, November 3, 2020 1:53 AM To: Koenig, Christian Christian.Koenig@amd.com Cc: Alex Deucher alexdeucher@gmail.com; Deepak R Varma mh12gx2825@gmail.com; David Airlie airlied@linux.ie; LKML <linux- kernel@vger.kernel.org>; Maling list - DRI developers <dri- devel@lists.freedesktop.org>; Melissa Wen melissa.srw@gmail.com; amd-gfx list amd-gfx@lists.freedesktop.org; Daniel Vetter daniel@ffwll.ch; Daniel Vetter daniel.vetter@ffwll.ch; Deucher, Alexander Alexander.Deucher@amd.com Subject: Re: [PATCH] drm/amdgpu: do not initialise global variables to 0 or NULL
On Mon, Nov 02, 2020 at 09:06:21PM +0100, Christian König wrote:
Am 02.11.20 um 20:43 schrieb Alex Deucher:
On Mon, Nov 2, 2020 at 1:42 PM Deepak R Varma
mh12gx2825@gmail.com wrote:
Initializing global variable to 0 or NULL is not necessary and should be avoided. Issue reported by checkpatch script as: ERROR: do not initialise globals to 0 (or NULL).
I agree that this is technically correct, but a lot of people don't seem to know that so we get a lot of comments about this code for the variables that are not explicitly set. Seems less confusing to initialize them even if it not necessary. I don't have a particularly strong opinion on it however.
Agree with Alex.
Especially for the module parameters we should have a explicit init value for documentation purposes, even when it is 0.
Why is this one tiny driver somehow special compared to the entire rest of the kernel? (hint, it isn't...)
Please follow the normal coding style rules, there's no reason to ignore them unless you like to constantly reject patches like this that get sent to you.
I'll apply the patch, but as a data point, this is the first time I've gotten a patch to make this change. I get several bug reports or patches every year to explicitly set values to global variables because users assume they are not initialized. So it will always be a trade off as to which patches you want to NACK.
Alex
On Tue, Nov 03, 2020 at 02:50:40PM +0000, Deucher, Alexander wrote:
[AMD Public Use]
-----Original Message----- From: Greg KH gregkh@linuxfoundation.org Sent: Tuesday, November 3, 2020 1:53 AM To: Koenig, Christian Christian.Koenig@amd.com Cc: Alex Deucher alexdeucher@gmail.com; Deepak R Varma mh12gx2825@gmail.com; David Airlie airlied@linux.ie; LKML <linux- kernel@vger.kernel.org>; Maling list - DRI developers <dri- devel@lists.freedesktop.org>; Melissa Wen melissa.srw@gmail.com; amd-gfx list amd-gfx@lists.freedesktop.org; Daniel Vetter daniel@ffwll.ch; Daniel Vetter daniel.vetter@ffwll.ch; Deucher, Alexander Alexander.Deucher@amd.com Subject: Re: [PATCH] drm/amdgpu: do not initialise global variables to 0 or NULL
On Mon, Nov 02, 2020 at 09:06:21PM +0100, Christian König wrote:
Am 02.11.20 um 20:43 schrieb Alex Deucher:
On Mon, Nov 2, 2020 at 1:42 PM Deepak R Varma
mh12gx2825@gmail.com wrote:
Initializing global variable to 0 or NULL is not necessary and should be avoided. Issue reported by checkpatch script as: ERROR: do not initialise globals to 0 (or NULL).
I agree that this is technically correct, but a lot of people don't seem to know that so we get a lot of comments about this code for the variables that are not explicitly set. Seems less confusing to initialize them even if it not necessary. I don't have a particularly strong opinion on it however.
Agree with Alex.
Especially for the module parameters we should have a explicit init value for documentation purposes, even when it is 0.
Why is this one tiny driver somehow special compared to the entire rest of the kernel? (hint, it isn't...)
Please follow the normal coding style rules, there's no reason to ignore them unless you like to constantly reject patches like this that get sent to you.
I'll apply the patch, but as a data point, this is the first time I've gotten a patch to make this change. I get several bug reports or patches every year to explicitly set values to global variables because users assume they are not initialized. So it will always be a trade off as to which patches you want to NACK.
Are you all not running your patches through checkpatch.pl to tell you simple things like this? If no, I suggest you start doing that :)
thanks,
greg k-h
On Mon, Nov 02, 2020 at 02:43:45PM -0500, Alex Deucher wrote:
On Mon, Nov 2, 2020 at 1:42 PM Deepak R Varma mh12gx2825@gmail.com wrote:
Initializing global variable to 0 or NULL is not necessary and should be avoided. Issue reported by checkpatch script as: ERROR: do not initialise globals to 0 (or NULL).
I agree that this is technically correct, but a lot of people don't seem to know that so we get a lot of comments about this code for the variables that are not explicitly set. Seems less confusing to initialize them even if it not necessary. I don't have a particularly strong opinion on it however.
The kernel coding style is to do it this way. You even save space and time by doing it as well :)
thanks,
greg k-h
From: Greg KH
Sent: 02 November 2020 20:11
On Mon, Nov 02, 2020 at 02:43:45PM -0500, Alex Deucher wrote:
On Mon, Nov 2, 2020 at 1:42 PM Deepak R Varma mh12gx2825@gmail.com wrote:
Initializing global variable to 0 or NULL is not necessary and should be avoided. Issue reported by checkpatch script as: ERROR: do not initialise globals to 0 (or NULL).
I agree that this is technically correct, but a lot of people don't seem to know that so we get a lot of comments about this code for the variables that are not explicitly set. Seems less confusing to initialize them even if it not necessary. I don't have a particularly strong opinion on it however.
The kernel coding style is to do it this way. You even save space and time by doing it as well :)
Uninitialised globals end up as 'named common' (variables that are their own code section - from FORTRAN) until the final link puts them into the .bss. Globals initialised to 0 go into the .bss of the object file being created.
So both end up in the final .bss.
If the code goes into a module you aren't allowed 'common' data in a module to every global must be initialised.
I'm surprised checkpatch complains.
David
- Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK Registration No: 1397386 (Wales)
dri-devel@lists.freedesktop.org
-
Alex Deucher -
Christian König -
Christian König -
David Laight -
Deepak R Varma -
Deucher, Alexander -
Greg KH