Re: [Intel-gfx] [PATCH 0197/1285] Replace numeric parameter like 0444 with macro
On Tue, Aug 02, 2016 at 02:37:37PM +0300, Ville Syrjälä wrote:
> On Tue, Aug 02, 2016 at 06:48:47PM +0800, Baole Ni wrote:
> > 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
> > Signed-off-by: Baole Ni
> > ---
> > drivers/gpu/drm/i915/i915_params.c | 64
> > +++---
> > 1 file changed, 32 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_params.c
> > b/drivers/gpu/drm/i915/i915_params.c
> > index 1779f02..7184e06 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -60,22 +60,22 @@ struct i915_params i915 __read_mostly = {
> > .inject_load_failure = 0,
> > };
> >
> > -module_param_named(modeset, i915.modeset, int, 0400);
> > +module_param_named(modeset, i915.modeset, int, S_IRUSR);
>
> At least I can't read those macros. Octal is much clearer IMO.
It's also easier to grep for, say, 644, rather than formulating a
regex with all possible ordering permutations of these macros.
Best regards,
Lukas
___
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 0197/1285] Replace numeric parameter like 0444 with macro
On Tue, Aug 02, 2016 at 02:37:37PM +0300, Ville Syrjälä wrote:
> On Tue, Aug 02, 2016 at 06:48:47PM +0800, Baole Ni wrote:
> > 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
> > Signed-off-by: Baole Ni
> > ---
> > drivers/gpu/drm/i915/i915_params.c | 64
> > +++---
> > 1 file changed, 32 insertions(+), 32 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_params.c
> > b/drivers/gpu/drm/i915/i915_params.c
> > index 1779f02..7184e06 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -60,22 +60,22 @@ struct i915_params i915 __read_mostly = {
> > .inject_load_failure = 0,
> > };
> >
> > -module_param_named(modeset, i915.modeset, int, 0400);
> > +module_param_named(modeset, i915.modeset, int, S_IRUSR);
>
> At least I can't read those macros. Octal is much clearer IMO.
Yeah, not really sold on this either.
-Daniel
>
> > MODULE_PARM_DESC(modeset,
> > "Use kernel modesetting [KMS] (0=disable, "
> > "1=on, -1=force vga console preference [default])");
> >
> > -module_param_named_unsafe(panel_ignore_lid, i915.panel_ignore_lid, int,
> > 0600);
> > +module_param_named_unsafe(panel_ignore_lid, i915.panel_ignore_lid, int,
> > S_IRUSR | S_IWUSR);
> > MODULE_PARM_DESC(panel_ignore_lid,
> > "Override lid status (0=autodetect, 1=autodetect disabled [default], "
> > "-1=force lid closed, -2=force lid open)");
> >
> > -module_param_named_unsafe(semaphores, i915.semaphores, int, 0400);
> > +module_param_named_unsafe(semaphores, i915.semaphores, int, S_IRUSR);
> > MODULE_PARM_DESC(semaphores,
> > "Use semaphores for inter-ring sync "
> > "(default: -1 (use per-chip defaults))");
> >
> > -module_param_named_unsafe(enable_rc6, i915.enable_rc6, int, 0400);
> > +module_param_named_unsafe(enable_rc6, i915.enable_rc6, int, S_IRUSR);
> > MODULE_PARM_DESC(enable_rc6,
> > "Enable power-saving render C-state 6. "
> > "Different stages can be selected via bitmask values "
> > @@ -83,82 +83,82 @@ MODULE_PARM_DESC(enable_rc6,
> > "For example, 3 would enable rc6 and deep rc6, and 7 would enable
> > everything. "
> > "default: -1 (use per-chip default)");
> >
> > -module_param_named_unsafe(enable_dc, i915.enable_dc, int, 0400);
> > +module_param_named_unsafe(enable_dc, i915.enable_dc, int, S_IRUSR);
> > MODULE_PARM_DESC(enable_dc,
> > "Enable power-saving display C-states. "
> > "(-1=auto [default]; 0=disable; 1=up to DC5; 2=up to DC6)");
> >
> > -module_param_named_unsafe(enable_fbc, i915.enable_fbc, int, 0600);
> > +module_param_named_unsafe(enable_fbc, i915.enable_fbc, int, S_IRUSR |
> > S_IWUSR);
> > MODULE_PARM_DESC(enable_fbc,
> > "Enable frame buffer compression for power savings "
> > "(default: -1 (use per-chip default))");
> >
> > -module_param_named_unsafe(lvds_channel_mode, i915.lvds_channel_mode, int,
> > 0400);
> > +module_param_named_unsafe(lvds_channel_mode, i915.lvds_channel_mode, int,
> > S_IRUSR);
> > MODULE_PARM_DESC(lvds_channel_mode,
> > "Specify LVDS channel mode "
> > "(0=probe BIOS [default], 1=single-channel, 2=dual-channel)");
> >
> > -module_param_named_unsafe(lvds_use_ssc, i915.panel_use_ssc, int, 0600);
> > +module_param_named_unsafe(lvds_use_ssc, i915.panel_use_ssc, int, S_IRUSR |
> > S_IWUSR);
> > MODULE_PARM_DESC(lvds_use_ssc,
> > "Use Spread Spectrum Clock with panels [LVDS/eDP] "
> > "(default: auto from VBT)");
> >
> > -module_param_named_unsafe(vbt_sdvo_panel_type, i915.vbt_sdvo_panel_type,
> > int, 0400);
> > +module_param_named_unsafe(vbt_sdvo_panel_type, i915.vbt_sdvo_panel_type,
> > int, S_IRUSR);
> > MODULE_PARM_DESC(vbt_sdvo_panel_type,
> > "Override/Ignore selection of SDVO panel mode in the VBT "
> > "(-2=ignore, -1=auto [default], index in VBT BIOS table)");
> >
> > -module_param_named_unsafe(reset, i915.reset, bool, 0600);
> > +module_param_named_unsafe(reset, i915.reset, bool, S_IRUSR | S_IWUSR);
> > MODULE_PARM_DESC(reset, "Attempt GPU resets (default: true)");
> >
> > -module_param_named_unsafe(enable_hangcheck, i915.enable_hangcheck, bool,
> > 0644);
> > +module_param_named_unsafe(enable_hangcheck, i915.enable_hangcheck, bool,
> > S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
> > MODULE_PARM_DESC(enable_hangcheck,
> > "Periodically check GPU activity for detecting hangs. "
> > "WARNING: Disabling this can cause system wide hangs. "
> > "(default: true)");
> >
> > -module_param_named_unsafe(enable_ppgtt, i915.enable_ppgtt, int, 0400);
> > +module_param_named_unsafe(enabl
Re: [Intel-gfx] [PATCH 0197/1285] Replace numeric parameter like 0444 with macro
On Tue, 02 Aug 2016, Ville Syrjälä wrote:
> On Tue, Aug 02, 2016 at 06:48:47PM +0800, Baole Ni wrote:
>> 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
>> Signed-off-by: Baole Ni
>> ---
>> drivers/gpu/drm/i915/i915_params.c | 64
>> +++---
>> 1 file changed, 32 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_params.c
>> b/drivers/gpu/drm/i915/i915_params.c
>> index 1779f02..7184e06 100644
>> --- a/drivers/gpu/drm/i915/i915_params.c
>> +++ b/drivers/gpu/drm/i915/i915_params.c
>> @@ -60,22 +60,22 @@ struct i915_params i915 __read_mostly = {
>> .inject_load_failure = 0,
>> };
>>
>> -module_param_named(modeset, i915.modeset, int, 0400);
>> +module_param_named(modeset, i915.modeset, int, S_IRUSR);
>
> At least I can't read those macros. Octal is much clearer IMO.
Besides I think we should consider making most if not all of these
parameters group/other readable. Which would then potentially become
S_IWUSR | S_IRUSR | S_IRGRP | S_IROTH instead of just 0644.
BR,
Jani.
>
>> MODULE_PARM_DESC(modeset,
>> "Use kernel modesetting [KMS] (0=disable, "
>> "1=on, -1=force vga console preference [default])");
>>
>> -module_param_named_unsafe(panel_ignore_lid, i915.panel_ignore_lid, int,
>> 0600);
>> +module_param_named_unsafe(panel_ignore_lid, i915.panel_ignore_lid, int,
>> S_IRUSR | S_IWUSR);
>> MODULE_PARM_DESC(panel_ignore_lid,
>> "Override lid status (0=autodetect, 1=autodetect disabled [default], "
>> "-1=force lid closed, -2=force lid open)");
>>
>> -module_param_named_unsafe(semaphores, i915.semaphores, int, 0400);
>> +module_param_named_unsafe(semaphores, i915.semaphores, int, S_IRUSR);
>> MODULE_PARM_DESC(semaphores,
>> "Use semaphores for inter-ring sync "
>> "(default: -1 (use per-chip defaults))");
>>
>> -module_param_named_unsafe(enable_rc6, i915.enable_rc6, int, 0400);
>> +module_param_named_unsafe(enable_rc6, i915.enable_rc6, int, S_IRUSR);
>> MODULE_PARM_DESC(enable_rc6,
>> "Enable power-saving render C-state 6. "
>> "Different stages can be selected via bitmask values "
>> @@ -83,82 +83,82 @@ MODULE_PARM_DESC(enable_rc6,
>> "For example, 3 would enable rc6 and deep rc6, and 7 would enable
>> everything. "
>> "default: -1 (use per-chip default)");
>>
>> -module_param_named_unsafe(enable_dc, i915.enable_dc, int, 0400);
>> +module_param_named_unsafe(enable_dc, i915.enable_dc, int, S_IRUSR);
>> MODULE_PARM_DESC(enable_dc,
>> "Enable power-saving display C-states. "
>> "(-1=auto [default]; 0=disable; 1=up to DC5; 2=up to DC6)");
>>
>> -module_param_named_unsafe(enable_fbc, i915.enable_fbc, int, 0600);
>> +module_param_named_unsafe(enable_fbc, i915.enable_fbc, int, S_IRUSR |
>> S_IWUSR);
>> MODULE_PARM_DESC(enable_fbc,
>> "Enable frame buffer compression for power savings "
>> "(default: -1 (use per-chip default))");
>>
>> -module_param_named_unsafe(lvds_channel_mode, i915.lvds_channel_mode, int,
>> 0400);
>> +module_param_named_unsafe(lvds_channel_mode, i915.lvds_channel_mode, int,
>> S_IRUSR);
>> MODULE_PARM_DESC(lvds_channel_mode,
>> "Specify LVDS channel mode "
>> "(0=probe BIOS [default], 1=single-channel, 2=dual-channel)");
>>
>> -module_param_named_unsafe(lvds_use_ssc, i915.panel_use_ssc, int, 0600);
>> +module_param_named_unsafe(lvds_use_ssc, i915.panel_use_ssc, int, S_IRUSR |
>> S_IWUSR);
>> MODULE_PARM_DESC(lvds_use_ssc,
>> "Use Spread Spectrum Clock with panels [LVDS/eDP] "
>> "(default: auto from VBT)");
>>
>> -module_param_named_unsafe(vbt_sdvo_panel_type, i915.vbt_sdvo_panel_type,
>> int, 0400);
>> +module_param_named_unsafe(vbt_sdvo_panel_type, i915.vbt_sdvo_panel_type,
>> int, S_IRUSR);
>> MODULE_PARM_DESC(vbt_sdvo_panel_type,
>> "Override/Ignore selection of SDVO panel mode in the VBT "
>> "(-2=ignore, -1=auto [default], index in VBT BIOS table)");
>>
>> -module_param_named_unsafe(reset, i915.reset, bool, 0600);
>> +module_param_named_unsafe(reset, i915.reset, bool, S_IRUSR | S_IWUSR);
>> MODULE_PARM_DESC(reset, "Attempt GPU resets (default: true)");
>>
>> -module_param_named_unsafe(enable_hangcheck, i915.enable_hangcheck, bool,
>> 0644);
>> +module_param_named_unsafe(enable_hangcheck, i915.enable_hangcheck, bool,
>> S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
>> MODULE_PARM_DESC(enable_hangcheck,
>> "Periodically check GPU activity for detecting hangs. "
>> "WARNING: Disabling this can cause system wide hangs. "
>> "(default: true)");
>>
>> -module_param_named_unsafe(enable_ppgtt, i915.enable_
Re: [Intel-gfx] [PATCH 0197/1285] Replace numeric parameter like 0444 with macro
On Tue, Aug 02, 2016 at 06:48:47PM +0800, Baole Ni wrote:
> 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
> Signed-off-by: Baole Ni
> ---
> drivers/gpu/drm/i915/i915_params.c | 64
> +++---
> 1 file changed, 32 insertions(+), 32 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_params.c
> b/drivers/gpu/drm/i915/i915_params.c
> index 1779f02..7184e06 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -60,22 +60,22 @@ struct i915_params i915 __read_mostly = {
> .inject_load_failure = 0,
> };
>
> -module_param_named(modeset, i915.modeset, int, 0400);
> +module_param_named(modeset, i915.modeset, int, S_IRUSR);
At least I can't read those macros. Octal is much clearer IMO.
> MODULE_PARM_DESC(modeset,
> "Use kernel modesetting [KMS] (0=disable, "
> "1=on, -1=force vga console preference [default])");
>
> -module_param_named_unsafe(panel_ignore_lid, i915.panel_ignore_lid, int,
> 0600);
> +module_param_named_unsafe(panel_ignore_lid, i915.panel_ignore_lid, int,
> S_IRUSR | S_IWUSR);
> MODULE_PARM_DESC(panel_ignore_lid,
> "Override lid status (0=autodetect, 1=autodetect disabled [default], "
> "-1=force lid closed, -2=force lid open)");
>
> -module_param_named_unsafe(semaphores, i915.semaphores, int, 0400);
> +module_param_named_unsafe(semaphores, i915.semaphores, int, S_IRUSR);
> MODULE_PARM_DESC(semaphores,
> "Use semaphores for inter-ring sync "
> "(default: -1 (use per-chip defaults))");
>
> -module_param_named_unsafe(enable_rc6, i915.enable_rc6, int, 0400);
> +module_param_named_unsafe(enable_rc6, i915.enable_rc6, int, S_IRUSR);
> MODULE_PARM_DESC(enable_rc6,
> "Enable power-saving render C-state 6. "
> "Different stages can be selected via bitmask values "
> @@ -83,82 +83,82 @@ MODULE_PARM_DESC(enable_rc6,
> "For example, 3 would enable rc6 and deep rc6, and 7 would enable
> everything. "
> "default: -1 (use per-chip default)");
>
> -module_param_named_unsafe(enable_dc, i915.enable_dc, int, 0400);
> +module_param_named_unsafe(enable_dc, i915.enable_dc, int, S_IRUSR);
> MODULE_PARM_DESC(enable_dc,
> "Enable power-saving display C-states. "
> "(-1=auto [default]; 0=disable; 1=up to DC5; 2=up to DC6)");
>
> -module_param_named_unsafe(enable_fbc, i915.enable_fbc, int, 0600);
> +module_param_named_unsafe(enable_fbc, i915.enable_fbc, int, S_IRUSR |
> S_IWUSR);
> MODULE_PARM_DESC(enable_fbc,
> "Enable frame buffer compression for power savings "
> "(default: -1 (use per-chip default))");
>
> -module_param_named_unsafe(lvds_channel_mode, i915.lvds_channel_mode, int,
> 0400);
> +module_param_named_unsafe(lvds_channel_mode, i915.lvds_channel_mode, int,
> S_IRUSR);
> MODULE_PARM_DESC(lvds_channel_mode,
>"Specify LVDS channel mode "
>"(0=probe BIOS [default], 1=single-channel, 2=dual-channel)");
>
> -module_param_named_unsafe(lvds_use_ssc, i915.panel_use_ssc, int, 0600);
> +module_param_named_unsafe(lvds_use_ssc, i915.panel_use_ssc, int, S_IRUSR |
> S_IWUSR);
> MODULE_PARM_DESC(lvds_use_ssc,
> "Use Spread Spectrum Clock with panels [LVDS/eDP] "
> "(default: auto from VBT)");
>
> -module_param_named_unsafe(vbt_sdvo_panel_type, i915.vbt_sdvo_panel_type,
> int, 0400);
> +module_param_named_unsafe(vbt_sdvo_panel_type, i915.vbt_sdvo_panel_type,
> int, S_IRUSR);
> MODULE_PARM_DESC(vbt_sdvo_panel_type,
> "Override/Ignore selection of SDVO panel mode in the VBT "
> "(-2=ignore, -1=auto [default], index in VBT BIOS table)");
>
> -module_param_named_unsafe(reset, i915.reset, bool, 0600);
> +module_param_named_unsafe(reset, i915.reset, bool, S_IRUSR | S_IWUSR);
> MODULE_PARM_DESC(reset, "Attempt GPU resets (default: true)");
>
> -module_param_named_unsafe(enable_hangcheck, i915.enable_hangcheck, bool,
> 0644);
> +module_param_named_unsafe(enable_hangcheck, i915.enable_hangcheck, bool,
> S_IRUSR | S_IWUSR | S_IRGRP | S_IROTH);
> MODULE_PARM_DESC(enable_hangcheck,
> "Periodically check GPU activity for detecting hangs. "
> "WARNING: Disabling this can cause system wide hangs. "
> "(default: true)");
>
> -module_param_named_unsafe(enable_ppgtt, i915.enable_ppgtt, int, 0400);
> +module_param_named_unsafe(enable_ppgtt, i915.enable_ppgtt, int, S_IRUSR);
> MODULE_PARM_DESC(enable_ppgtt,
> "Override PPGTT usage. "
> "(-1=auto [default], 0=disabled, 1=aliasing, 2=full, 3=full with
> extended address space)");
>
> -module_param_named_unsafe(enable_execlists, i915.enable_execlist
