Re: [PATCH v3] drm/i915: Add fault injection support

[Daniel Vetter <daniel@ffwll.ch>]

On Mon, Mar 14, 2016 at 04:59:20PM +0200, Imre Deak wrote:
> Add support for forcing an error at selected places in the driver. As an
> example add 4 options to fail during driver loading.
> 
> Requested by Chris.
> 
> v2:
> - Add fault point for modeset initialization
> - Print debug message when injecting an error
> v3:
> - Rename inject_fault to inject_load_failure, rename the related macros
>   and helper accordingly (Chris)
> 
> CC: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
> 
> [This depends on
>  https://lists.freedesktop.org/archives/intel-gfx/2016-March/089597.html]
> 
>  drivers/gpu/drm/i915/i915_dma.c    | 12 ++++++++++++
>  drivers/gpu/drm/i915/i915_drv.h    | 16 ++++++++++++++++
>  drivers/gpu/drm/i915/i915_params.c |  5 +++++
>  drivers/gpu/drm/i915/i915_params.h |  1 +
>  4 files changed, 34 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
> index a5121cd..7490307 100644
> --- a/drivers/gpu/drm/i915/i915_dma.c
> +++ b/drivers/gpu/drm/i915/i915_dma.c
> @@ -370,6 +370,9 @@ static int i915_load_modeset_init(struct drm_device *dev)
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int ret;
>  
> +	if (i915_inject_load_failure(I915_FAIL_INIT_MODESET))
> +		return -ENODEV;
> +
>  	ret = intel_bios_init(dev_priv);
>  	if (ret)
>  		DRM_INFO("failed to find VBIOS tables\n");
> @@ -950,6 +953,9 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
>  	struct intel_device_info *device_info;
>  	int ret = 0;
>  
> +	if (i915_inject_load_failure(I915_FAIL_INIT_EARLY))
> +		return -ENODEV;
> +
>  	dev_priv->dev = dev;
>  
>  	/* Setup the write-once "constant" device info */
> @@ -1064,6 +1070,9 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv)
>  	struct drm_device *dev = dev_priv->dev;
>  	int ret;
>  
> +	if (i915_inject_load_failure(I915_FAIL_INIT_MMIO))
> +		return -ENODEV;
> +
>  	if (i915_get_bridge_dev(dev))
>  		return -EIO;

Ok, thought a bit more about this, and if we really want to check all the
load failure paths then this will become extremely verbose. Since we'd
need to have a bitflag for every return -EFAIL. So maybe another look at
lib/fault-inject.c is warranted, plus then some macro magic that we could
to wrap _all_ the checks in our load code like this:

 	if (i915_load_failure(i915_get_bridge_dev(dev)))
 		return -EIO;

i915_load_failure ofc needs to fail if it's argument is false, but it
could also increment a counter every time it's called and then use that
counter to inquire the fault injection framework with
should_fail(i915_load_failures, counter). Abusing a counter for the size
would allow us to easily restrict fault injection to a certain range of
faults. We could also expose the final value of that counter in debugfs,
so that the igt can tune itself.

Anyway, this is kinda a big plan, but the part I think we should ponder is
how to make the fault injection less noise and intrusive. A macro to wrap
existing checks seems like a good idea.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx