Re: [Intel-gfx] [PATCH 3/4] drm/i915/params: prevent changing module params runtime

[Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>]

Reviewed-by: Juha-Pekka Heikkila <juhapekka.heikkila@gmail.com>

On 14.5.2020 18.40, Jani Nikula wrote:
> Only support runtime changes through the debugfs.
> 
> i915.verbose_state_checks remains an exception, and is not exposed via
> debugfs.
> 
> This depends on IGT having been updated to use the debugfs for modifying
> the parameters.
> 
> Cc: Juha-Pekka Heikkilä <juha-pekka.heikkila@intel.com>
> Cc: Venkata Sandeep Dhanalakota <venkata.s.dhanalakota@intel.com>
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_params.c | 38 +++++++++++++++++++-----------
>   1 file changed, 24 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> index a3dde770226d..ace44ad7e6df 100644
> --- a/drivers/gpu/drm/i915/i915_params.c
> +++ b/drivers/gpu/drm/i915/i915_params.c
> @@ -40,6 +40,15 @@ struct i915_params i915_modparams __read_mostly = {
>   #undef MEMBER
>   };
>   
> +/*
> + * Note: As a rule, keep module parameter sysfs permissions read-only
> + * 0400. Runtime changes are only supported through i915 debugfs.
> + *
> + * For any exceptions requiring write access and runtime changes through module
> + * parameter sysfs, prevent debugfs file creation by setting the parameter's
> + * debugfs mode to 0.
> + */
> +
>   i915_param_named(modeset, int, 0400,
>   	"Use kernel modesetting [KMS] (0=disable, "
>   	"1=on, -1=force vga console preference [default])");
> @@ -49,7 +58,7 @@ i915_param_named_unsafe(enable_dc, int, 0400,
>   	"(-1=auto [default]; 0=disable; 1=up to DC5; 2=up to DC6; "
>   	"3=up to DC5 with DC3CO; 4=up to DC6 with DC3CO)");
>   
> -i915_param_named_unsafe(enable_fbc, int, 0600,
> +i915_param_named_unsafe(enable_fbc, int, 0400,
>   	"Enable frame buffer compression for power savings "
>   	"(default: -1 (use per-chip default))");
>   
> @@ -57,7 +66,7 @@ i915_param_named_unsafe(lvds_channel_mode, int, 0400,
>   	 "Specify LVDS channel mode "
>   	 "(0=probe BIOS [default], 1=single-channel, 2=dual-channel)");
>   
> -i915_param_named_unsafe(panel_use_ssc, int, 0600,
> +i915_param_named_unsafe(panel_use_ssc, int, 0400,
>   	"Use Spread Spectrum Clock with panels [LVDS/eDP] "
>   	"(default: auto from VBT)");
>   
> @@ -65,25 +74,25 @@ i915_param_named_unsafe(vbt_sdvo_panel_type, int, 0400,
>   	"Override/Ignore selection of SDVO panel mode in the VBT "
>   	"(-2=ignore, -1=auto [default], index in VBT BIOS table)");
>   
> -i915_param_named_unsafe(reset, int, 0600,
> +i915_param_named_unsafe(reset, int, 0400,
>   	"Attempt GPU resets (0=disabled, 1=full gpu reset, 2=engine reset [default])");
>   
>   i915_param_named_unsafe(vbt_firmware, charp, 0400,
>   	"Load VBT from specified file under /lib/firmware");
>   
>   #if IS_ENABLED(CONFIG_DRM_I915_CAPTURE_ERROR)
> -i915_param_named(error_capture, bool, 0600,
> +i915_param_named(error_capture, bool, 0400,
>   	"Record the GPU state following a hang. "
>   	"This information in /sys/class/drm/card<N>/error is vital for "
>   	"triaging and debugging hangs.");
>   #endif
>   
> -i915_param_named_unsafe(enable_hangcheck, bool, 0600,
> +i915_param_named_unsafe(enable_hangcheck, bool, 0400,
>   	"Periodically check GPU activity for detecting hangs. "
>   	"WARNING: Disabling this can cause system wide hangs. "
>   	"(default: true)");
>   
> -i915_param_named_unsafe(enable_psr, int, 0600,
> +i915_param_named_unsafe(enable_psr, int, 0400,
>   	"Enable PSR "
>   	"(0=disabled, 1=enabled) "
>   	"Default: -1 (use per-chip default)");
> @@ -96,22 +105,22 @@ i915_param_named_unsafe(disable_power_well, int, 0400,
>   	"Disable display power wells when possible "
>   	"(-1=auto [default], 0=power wells always on, 1=power wells disabled when possible)");
>   
> -i915_param_named_unsafe(enable_ips, int, 0600, "Enable IPS (default: true)");
> +i915_param_named_unsafe(enable_ips, int, 0400, "Enable IPS (default: true)");
>   
> -i915_param_named(fastboot, int, 0600,
> +i915_param_named(fastboot, int, 0400,
>   	"Try to skip unnecessary mode sets at boot time "
>   	"(0=disabled, 1=enabled) "
>   	"Default: -1 (use per-chip default)");
>   
> -i915_param_named_unsafe(load_detect_test, bool, 0600,
> +i915_param_named_unsafe(load_detect_test, bool, 0400,
>   	"Force-enable the VGA load detect code for testing (default:false). "
>   	"For developers only.");
>   
> -i915_param_named_unsafe(force_reset_modeset_test, bool, 0600,
> +i915_param_named_unsafe(force_reset_modeset_test, bool, 0400,
>   	"Force a modeset during gpu reset for testing (default:false). "
>   	"For developers only.");
>   
> -i915_param_named_unsafe(invert_brightness, int, 0600,
> +i915_param_named_unsafe(invert_brightness, int, 0400,
>   	"Invert backlight brightness "
>   	"(-1 force normal, 0 machine defaults, 1 force inversion), please "
>   	"report PCI device ID, subsystem vendor and subsystem device ID "
> @@ -121,10 +130,11 @@ i915_param_named_unsafe(invert_brightness, int, 0600,
>   i915_param_named(disable_display, bool, 0400,
>   	"Disable display (default: false)");
>   
> -i915_param_named(mmio_debug, int, 0600,
> +i915_param_named(mmio_debug, int, 0400,
>   	"Enable the MMIO debug code for the first N failures (default: off). "
>   	"This may negatively affect performance.");
>   
> +/* Special case writable file */
>   i915_param_named(verbose_state_checks, bool, 0600,
>   	"Enable verbose logs (ie. WARN_ON()) in case of unexpected hw state conditions.");
>   
> @@ -155,7 +165,7 @@ i915_param_named_unsafe(huc_firmware_path, charp, 0400,
>   i915_param_named_unsafe(dmc_firmware_path, charp, 0400,
>   	"DMC firmware path to use instead of the default one");
>   
> -i915_param_named_unsafe(enable_dp_mst, bool, 0600,
> +i915_param_named_unsafe(enable_dp_mst, bool, 0400,
>   	"Enable multi-stream transport (MST) for new DisplayPort sinks. (default: true)");
>   
>   #if IS_ENABLED(CONFIG_DRM_I915_DEBUG)
> @@ -163,7 +173,7 @@ i915_param_named_unsafe(inject_probe_failure, uint, 0400,
>   	"Force an error after a number of failure check points (0:disabled (default), N:force failure at the Nth failure check point)");
>   #endif
>   
> -i915_param_named(enable_dpcd_backlight, int, 0600,
> +i915_param_named(enable_dpcd_backlight, int, 0400,
>   	"Enable support for DPCD backlight control"
>   	"(-1=use per-VBT LFP backlight type setting [default], 0=disabled, 1=enabled)");
>   
> 

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx