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

[Imre Deak <imre.deak@intel.com>]

On ti, 2016-03-15 at 10:34 +0200, Joonas Lahtinen wrote:
> On ma, 2016-03-14 at 16:59 +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
> > .h
> > tml]
> > 
> >  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;
> >  
> > @@ -1107,6 +1116,9 @@ static int i915_driver_init_hw(struct
> > drm_i915_private *dev_priv)
> >  	uint32_t aperture_size;
> >  	int ret;
> >  
> > +	if (i915_inject_load_failure(I915_FAIL_INIT_HW))
> > +		return -ENODEV;
> > +
> >  	intel_device_info_runtime_init(dev);
> >  
> >  	ret = i915_gem_gtt_init(dev);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> > b/drivers/gpu/drm/i915/i915_drv.h
> > index 25274e1..e2d21d5 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -98,6 +98,22 @@
> >  #define I915_STATE_WARN_ON(x)					
> > 	\
> >  	I915_STATE_WARN((x), "%s", "WARN_ON(" __stringify(x) ")")
> >  
> > +#define I915_FAIL_INIT_EARLY	BIT(0)
> > +#define I915_FAIL_INIT_MMIO	BIT(1)
> > +#define I915_FAIL_INIT_HW	BIT(2)
> > +#define I915_FAIL_INIT_MODESET	BIT(3)
> > +
> > +static inline bool i915_inject_load_failure(unsigned int
> > fail_mask)
> > +{
> > +	if (i915.inject_load_failure & fail_mask) {
> > +		DRM_DEBUG_DRIVER("Injecting failure %08x\n",
> > fail_mask);
> > +
> > +		return true;
> > +	}
> > +
> > +	return false;
> > +}
> > +
> >  static inline const char *yesno(bool v)
> >  {
> >  	return v ? "yes" : "no";
> > diff --git a/drivers/gpu/drm/i915/i915_params.c
> > b/drivers/gpu/drm/i915/i915_params.c
> > index 278c9c4..4faeeed 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -56,6 +56,7 @@ struct i915_params i915 __read_mostly = {
> >  	.edp_vswing = 0,
> >  	.enable_guc_submission = false,
> >  	.guc_log_level = -1,
> > +	.inject_load_failure = 0,
> >  };
> >  
> >  module_param_named(modeset, i915.modeset, int, 0400);
> > @@ -201,3 +202,7 @@ MODULE_PARM_DESC(enable_guc_submission, "Enable
> > GuC submission (default:false)")
> >  module_param_named(guc_log_level, i915.guc_log_level, int, 0400);
> >  MODULE_PARM_DESC(guc_log_level,
> >  	"GuC firmware logging level (-1:disabled (default), 0-
> > 3:enabled)");
> > +
> > +module_param_named(inject_load_failure, i915.inject_load_failure,
> > uint, 0600);
> 
> This most definitely should be module_param_named_unsafe.

Ok, will change that.

> 
> I think I'd also hope to see some kind of compile time option to
> enable
> this. I don't think average user wants this by default.

I agree with Chris and Daniel, so I'll leave this out for now. It can
be added later in any case. 

> 
> > +MODULE_PARM_DESC(inject_load_failure,
> > +	"Force an error at selected points (0:disabled,
> > 0x1:INIT_EARLY, 0x2:INIT_MMIO, 0x4:INIT_HW, 0x8:INIT_MODESET)");
> > diff --git a/drivers/gpu/drm/i915/i915_params.h
> > b/drivers/gpu/drm/i915/i915_params.h
> > index bd5026b..b691026 100644
> > --- a/drivers/gpu/drm/i915/i915_params.h
> > +++ b/drivers/gpu/drm/i915/i915_params.h
> > @@ -59,6 +59,7 @@ struct i915_params {
> >  	bool enable_guc_submission;
> >  	bool verbose_state_checks;
> >  	bool nuclear_pageflip;
> > +	unsigned int inject_load_failure;
> 
> Duh, add it above the bools, this struct was cleaned once already :P

Ah, missed the code comment about this. Will move it thanks.

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