RE: [PATCH 0179/1285] Replace numeric parameter like 0444 with macro

[Deucher, Alexander]

> -----Original Message-----
> From: Baole Ni [mailto:baolex.ni@xxxxxxxxx]
> Sent: Tuesday, August 02, 2016 6:47 AM
> To: Deucher, Alexander; Koenig, Christian; airlied@xxxxxxxx;
> tony.luck@xxxxxxxxx; matt@xxxxxxxxxxxxxxxxxxx; m.chehab@xxxxxxxxxxx;
> lho@xxxxxxx; dougthompson@xxxxxxxxxxxx; bp@xxxxxxxxx
> Cc: dri-devel@xxxxxxxxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx; Zhou,
> Jammy; Zhou, David(ChunMing); Cui, Flora; chuansheng.liu@xxxxxxxxx;
> baolex.ni@xxxxxxxxx
> Subject: [PATCH 0179/1285] Replace numeric parameter like 0444 with macro
> 
> I find that the developers often just specified the numeric value
> when calling a macro which is defined with a parameter for access
> permission.
> As we know, these numeric value for access permission have had the
> corresponding macro,
> and that using macro can improve the robustness and readability of the code,
> thus, I suggest replacing the numeric parameter with the macro.
> 
> Signed-off-by: Chuansheng Liu <chuansheng.liu@xxxxxxxxx>
> Signed-off-by: Baole Ni <baolex.ni@xxxxxxxxx>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c | 54 ++++++++++++++++----
> -------------
>  1 file changed, 27 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> index f888c01..f0c9c29 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_drv.c
> @@ -86,87 +86,87 @@ unsigned amdgpu_pcie_gen_cap = 0;
>  unsigned amdgpu_pcie_lane_cap = 0;
> 
>  MODULE_PARM_DESC(vramlimit, "Restrict VRAM for testing, in
> megabytes");
> -module_param_named(vramlimit, amdgpu_vram_limit, int, 0600);
> +module_param_named(vramlimit, amdgpu_vram_limit, int, S_IRUSR |
> S_IWUSR);
> 

FWIW, I find it much easier to read and understand the numeric representation.

Alex

>  MODULE_PARM_DESC(gartsize, "Size of PCIE/IGP gart to setup in
> megabytes (32, 64, etc., -1 = auto)");
> -module_param_named(gartsize, amdgpu_gart_size, int, 0600);
> +module_param_named(gartsize, amdgpu_gart_size, int, S_IRUSR |
> S_IWUSR);
> 
>  MODULE_PARM_DESC(benchmark, "Run benchmark");
> -module_param_named(benchmark, amdgpu_benchmarking, int, 0444);
> +module_param_named(benchmark, amdgpu_benchmarking, int, S_IRUSR
> | S_IRGRP | S_IROTH);
> 
>  MODULE_PARM_DESC(test, "Run tests");
> -module_param_named(test, amdgpu_testing, int, 0444);
> +module_param_named(test, amdgpu_testing, int, S_IRUSR | S_IRGRP |
> S_IROTH);
> 
>  MODULE_PARM_DESC(audio, "Audio enable (-1 = auto, 0 = disable, 1 =
> enable)");
> -module_param_named(audio, amdgpu_audio, int, 0444);
> +module_param_named(audio, amdgpu_audio, int, S_IRUSR | S_IRGRP |
> S_IROTH);
> 
>  MODULE_PARM_DESC(disp_priority, "Display Priority (0 = auto, 1 = normal, 2
> = high)");
> -module_param_named(disp_priority, amdgpu_disp_priority, int, 0444);
> +module_param_named(disp_priority, amdgpu_disp_priority, int, S_IRUSR |
> S_IRGRP | S_IROTH);
> 
>  MODULE_PARM_DESC(hw_i2c, "hw i2c engine enable (0 = disable)");
> -module_param_named(hw_i2c, amdgpu_hw_i2c, int, 0444);
> +module_param_named(hw_i2c, amdgpu_hw_i2c, int, S_IRUSR | S_IRGRP |
> S_IROTH);
> 
>  MODULE_PARM_DESC(pcie_gen2, "PCIE Gen2 mode (-1 = auto, 0 = disable,
> 1 = enable)");
> -module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, 0444);
> +module_param_named(pcie_gen2, amdgpu_pcie_gen2, int, S_IRUSR |
> S_IRGRP | S_IROTH);
> 
>  MODULE_PARM_DESC(msi, "MSI support (1 = enable, 0 = disable, -1 =
> auto)");
> -module_param_named(msi, amdgpu_msi, int, 0444);
> +module_param_named(msi, amdgpu_msi, int, S_IRUSR | S_IRGRP |
> S_IROTH);
> 
>  MODULE_PARM_DESC(lockup_timeout, "GPU lockup timeout in ms (default
> 0 = disable)");
> -module_param_named(lockup_timeout, amdgpu_lockup_timeout, int,
> 0444);
> +module_param_named(lockup_timeout, amdgpu_lockup_timeout, int,
> S_IRUSR | S_IRGRP | S_IROTH);
> 
>  MODULE_PARM_DESC(dpm, "DPM support (1 = enable, 0 = disable, -1 =
> auto)");
> -module_param_named(dpm, amdgpu_dpm, int, 0444);
> +module_param_named(dpm, amdgpu_dpm, int, S_IRUSR | S_IRGRP |
> S_IROTH);
> 
>  MODULE_PARM_DESC(smc_load_fw, "SMC firmware loading(1 = enable, 0 =
> disable)");
> -module_param_named(smc_load_fw, amdgpu_smc_load_fw, int, 0444);
> +module_param_named(smc_load_fw, amdgpu_smc_load_fw, int, S_IRUSR
> | S_IRGRP | S_IROTH);
> 
>  MODULE_PARM_DESC(aspm, "ASPM support (1 = enable, 0 = disable, -1 =
> auto)");
> -module_param_named(aspm, amdgpu_aspm, int, 0444);
> +module_param_named(aspm, amdgpu_aspm, int, S_IRUSR | S_IRGRP |
> S_IROTH);
> 
>  MODULE_PARM_DESC(runpm, "PX runtime pm (1 = force enable, 0 =
> disable, -1 = PX only default)");
> -module_param_named(runpm, amdgpu_runtime_pm, int, 0444);
> +module_param_named(runpm, amdgpu_runtime_pm, int, S_IRUSR |
> S_IRGRP | S_IROTH);
> 
>  MODULE_PARM_DESC(ip_block_mask, "IP Block Mask (all blocks enabled
> (default))");
> -module_param_named(ip_block_mask, amdgpu_ip_block_mask, uint,
> 0444);
> +module_param_named(ip_block_mask, amdgpu_ip_block_mask, uint,
> S_IRUSR | S_IRGRP | S_IROTH);
> 
>  MODULE_PARM_DESC(bapm, "BAPM support (1 = enable, 0 = disable, -1 =
> auto)");
> -module_param_named(bapm, amdgpu_bapm, int, 0444);
> +module_param_named(bapm, amdgpu_bapm, int, S_IRUSR | S_IRGRP |
> S_IROTH);
> 
>  MODULE_PARM_DESC(deep_color, "Deep Color support (1 = enable, 0 =
> disable (default))");
> -module_param_named(deep_color, amdgpu_deep_color, int, 0444);
> +module_param_named(deep_color, amdgpu_deep_color, int, S_IRUSR |
> S_IRGRP | S_IROTH);
> 
>  MODULE_PARM_DESC(vm_size, "VM address space size in gigabytes
> (default 64GB)");
> -module_param_named(vm_size, amdgpu_vm_size, int, 0444);
> +module_param_named(vm_size, amdgpu_vm_size, int, S_IRUSR |
> S_IRGRP | S_IROTH);
> 
>  MODULE_PARM_DESC(vm_block_size, "VM page table size in bits (default
> depending on vm_size)");
> -module_param_named(vm_block_size, amdgpu_vm_block_size, int, 0444);
> +module_param_named(vm_block_size, amdgpu_vm_block_size, int,
> S_IRUSR | S_IRGRP | S_IROTH);
> 
>  MODULE_PARM_DESC(vm_fault_stop, "Stop on VM fault (0 = never
> (default), 1 = print first, 2 = always)");
> -module_param_named(vm_fault_stop, amdgpu_vm_fault_stop, int, 0444);
> +module_param_named(vm_fault_stop, amdgpu_vm_fault_stop, int,
> S_IRUSR | S_IRGRP | S_IROTH);
> 
>  MODULE_PARM_DESC(vm_debug, "Debug VM handling (0 = disabled
> (default), 1 = enabled)");
> -module_param_named(vm_debug, amdgpu_vm_debug, int, 0644);
> +module_param_named(vm_debug, amdgpu_vm_debug, int, S_IRUSR |
> S_IWUSR | S_IRGRP | S_IROTH);
> 
>  MODULE_PARM_DESC(exp_hw_support, "experimental hw support (1 =
> enable, 0 = disable (default))");
> -module_param_named(exp_hw_support, amdgpu_exp_hw_support, int,
> 0444);
> +module_param_named(exp_hw_support, amdgpu_exp_hw_support, int,
> S_IRUSR | S_IRGRP | S_IROTH);
> 
>  MODULE_PARM_DESC(sched_jobs, "the max number of jobs supported in
> the sw queue (default 32)");
> -module_param_named(sched_jobs, amdgpu_sched_jobs, int, 0444);
> +module_param_named(sched_jobs, amdgpu_sched_jobs, int, S_IRUSR |
> S_IRGRP | S_IROTH);
> 
>  MODULE_PARM_DESC(sched_hw_submission, "the max number of HW
> submissions (default 2)");
> -module_param_named(sched_hw_submission,
> amdgpu_sched_hw_submission, int, 0444);
> +module_param_named(sched_hw_submission,
> amdgpu_sched_hw_submission, int, S_IRUSR | S_IRGRP | S_IROTH);
> 
>  #ifdef CONFIG_DRM_AMD_POWERPLAY
>  MODULE_PARM_DESC(powerplay, "Powerplay component (1 = enable, 0 =
> disable, -1 = auto (default))");
> -module_param_named(powerplay, amdgpu_powerplay, int, 0444);
> +module_param_named(powerplay, amdgpu_powerplay, int, S_IRUSR |
> S_IRGRP | S_IROTH);
>  #endif
> 
>  MODULE_PARM_DESC(pcie_gen_cap, "PCIE Gen Caps (0: autodetect
> (default))");
> -module_param_named(pcie_gen_cap, amdgpu_pcie_gen_cap, uint, 0444);
> +module_param_named(pcie_gen_cap, amdgpu_pcie_gen_cap, uint,
> S_IRUSR | S_IRGRP | S_IROTH);
> 
>  MODULE_PARM_DESC(pcie_lane_cap, "PCIE Lane Caps (0: autodetect
> (default))");
> -module_param_named(pcie_lane_cap, amdgpu_pcie_lane_cap, uint,
> 0444);
> +module_param_named(pcie_lane_cap, amdgpu_pcie_lane_cap, uint,
> S_IRUSR | S_IRGRP | S_IROTH);
> 
>  static const struct pci_device_id pciidlist[] = {
>  #ifdef CONFIG_DRM_AMDGPU_CIK
> --
> 2.9.2