[PATCH] drm/i915: Add fault injection support

[PATCH] drm/i915: Add fault injection support

[Imre Deak]

Add support for forcing an error at selected places in the driver. As an
example add 3 options to fail during driver loading.

Requested by 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    | 9 +++++++++
 drivers/gpu/drm/i915/i915_drv.h    | 4 ++++
 drivers/gpu/drm/i915/i915_params.c | 6 ++++++
 drivers/gpu/drm/i915/i915_params.h | 1 +
 4 files changed, 20 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
index b1dbf1b..d12ae0b 100644
--- a/drivers/gpu/drm/i915/i915_dma.c
+++ b/drivers/gpu/drm/i915/i915_dma.c
@@ -950,6 +950,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_fault & I915_FAULT_INIT_EARLY)
+		return -ENODEV;
+
 	dev_priv->dev = dev;
 
 	/* Setup the write-once "constant" device info */
@@ -1064,6 +1067,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_fault & I915_FAULT_INIT_MMIO)
+		return -ENODEV;
+
 	if (i915_get_bridge_dev(dev))
 		return -EIO;
 
@@ -1107,6 +1113,9 @@ static int i915_driver_init_hw(struct drm_i915_private *dev_priv)
 	uint32_t aperture_size;
 	int ret;
 
+	if (i915.inject_fault & I915_FAULT_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 f62b6d1..5122ed9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -98,6 +98,10 @@
 #define I915_STATE_WARN_ON(x)						\
 	I915_STATE_WARN((x), "%s", "WARN_ON(" __stringify(x) ")")
 
+#define I915_FAULT_INIT_EARLY	0x0001
+#define I915_FAULT_INIT_MMIO	0x0002
+#define I915_FAULT_INIT_HW	0x0004
+
 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..21f8dd3 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_fault = 0,
 };
 
 module_param_named(modeset, i915.modeset, int, 0400);
@@ -201,3 +202,8 @@ 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_fault, i915.inject_fault, uint, 0600);
+MODULE_PARM_DESC(inject_fault,
+	"Force an error at selected points "
+	"(0:disabled, 0x1:INIT_EARLY, 0x2:INIT_MMIO, 0x4:INIT_HW)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index bd5026b..2989a48 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_fault;
 };
 
 extern struct i915_params i915 __read_mostly;
-- 
2.5.0

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

✗ Fi.CI.BAT: failure for drm/i915: Add fault injection support

[Patchwork]

== Series Details ==

Series: drm/i915: Add fault injection support
URL   : https://patchwork.freedesktop.org/series/4385/
State : failure

== Summary ==

Series 4385v1 drm/i915: Add fault injection support
2016-03-11T22:53:14.152284 http://patchwork.freedesktop.org/api/1.0/series/4385/revisions/1/mbox/
Applying: drm/i915: Add fault injection support
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_dma.c
M	drivers/gpu/drm/i915/i915_drv.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/i915_drv.h
Auto-merging drivers/gpu/drm/i915/i915_dma.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/i915_dma.c
Patch failed at 0001 drm/i915: Add fault injection support

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

[PATCH v2] drm/i915: Add fault injection support

[Imre Deak]

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

CC: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 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 b1dbf1b..65be671 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_fault(I915_FAULT_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_fault(I915_FAULT_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_fault(I915_FAULT_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_fault(I915_FAULT_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 f62b6d1..33bcc6e 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_FAULT_INIT_EARLY	1
+#define I915_FAULT_INIT_MMIO	2
+#define I915_FAULT_INIT_HW	4
+#define I915_FAULT_INIT_MODESET	8
+
+static inline bool i915_inject_fault(unsigned int fault_mask)
+{
+	if (i915.inject_fault & fault_mask) {
+		DRM_DEBUG_DRIVER("Injecting fault %08x\n", fault_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..d96c2c0 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_fault = 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_fault, i915.inject_fault, uint, 0600);
+MODULE_PARM_DESC(inject_fault,
+	"Force an error at selected points (0:disabled, 1:INIT_EARLY, 2:INIT_MMIO, 4:INIT_HW)");
diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index bd5026b..2989a48 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_fault;
 };
 
 extern struct i915_params i915 __read_mostly;
-- 
2.5.0

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

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

[Imre Deak]

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;
 
@@ -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);
+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;
 };
 
 extern struct i915_params i915 __read_mostly;
-- 
2.5.0

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

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

[Joonas Lahtinen]

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.

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.

> +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

Regards, Joonas

>  };
>  
>  extern struct i915_params i915 __read_mostly;
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation

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

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

[Chris Wilson]

On Tue, Mar 15, 2016 at 10:34:02AM +0200, Joonas Lahtinen wrote:
> On ma, 2016-03-14 at 16:59 +0200, Imre Deak wrote:
> > +module_param_named(inject_load_failure, i915.inject_load_failure,
> > uint, 0600);
> 
> This most definitely should be module_param_named_unsafe.
> 
> 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.

That's not a bad idea. We can get rid of most of the dangerous params
this way. On the other hand, we need to keep the debug/quirk params
around so that we can test options in the wild (bug reporters).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Daniel Vetter]

On Tue, Mar 15, 2016 at 09:28:34AM +0000, Chris Wilson wrote:
> On Tue, Mar 15, 2016 at 10:34:02AM +0200, Joonas Lahtinen wrote:
> > On ma, 2016-03-14 at 16:59 +0200, Imre Deak wrote:
> > > +module_param_named(inject_load_failure, i915.inject_load_failure,
> > > uint, 0600);
> > 
> > This most definitely should be module_param_named_unsafe.
> > 
> > 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.
> 
> That's not a bad idea. We can get rid of most of the dangerous params
> this way. On the other hand, we need to keep the debug/quirk params
> around so that we can test options in the wild (bug reporters).

Yeah, most of the _unsafe stuff is _really_ useful for quick bug triage.
Definitely don't want to disable those by default. Maybe an
i915.fry_my_kernel option? But even that is too confusing I think for
testers.
-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

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

[Imre Deak]

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

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

[Daniel Vetter]

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

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

[Imre Deak]

On ti, 2016-03-15 at 09:56 +0100, Daniel Vetter wrote:
> 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.

I'm not sure if you want to check all failure paths, I think for that
the existing failslab etc. mechanisms are better suited. This new
option would be used at relatively few well defined places. The option
is a mask since Chris wanted the possibility to mix failures (which
makes sense when injecting a non-fatal failure somewhere). If he
doesn't insist on that possibility I can convert the mask option to a
counter based one identifying a single failure spot instead as you
suggest. Chris?

> 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;

The above will not work when we want to propagate the error and I
haven't found a nice way to do that with such a helper taking the
function call as argument. I could move the check to the call site
instead of having it inside the called function, if that's what you'd
prefer:

if ((ret = i915_load_failure(...)) < 0 ||
    (ret = i915_driver_init_mmio(...)) < 0)
	return ret;

Although I'm not sure if this makes things clearer. Other suggestions are
welcome.

> 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.

Yes, I could do this provided Chris doesn't oppose the idea of the
counter based approach.

> 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.

I can switch to using a counter instead of the mask and if there is a
good suggestion about the macro I can use that instead.

--Imre

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

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

[Chris Wilson]

On Tue, Mar 15, 2016 at 04:01:14PM +0200, Imre Deak wrote:
> I'm not sure if you want to check all failure paths, I think for that
> the existing failslab etc. mechanisms are better suited. This new
> option would be used at relatively few well defined places. The option
> is a mask since Chris wanted the possibility to mix failures (which
> makes sense when injecting a non-fatal failure somewhere). If he
> doesn't insist on that possibility I can convert the mask option to a
> counter based one identifying a single failure spot instead as you
> suggest. Chris?

We can extend the counter mechanism by having multiple counters behind
i915.inject_load_failure (i.e. =gem:4,driver:10,modeset:1)
so extensibility for more testing seems fine.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Joonas Lahtinen]

On ti, 2016-03-15 at 14:14 +0000, Chris Wilson wrote:
> On Tue, Mar 15, 2016 at 04:01:14PM +0200, Imre Deak wrote:
> > 
> > I'm not sure if you want to check all failure paths, I think for that
> > the existing failslab etc. mechanisms are better suited. This new
> > option would be used at relatively few well defined places. The option
> > is a mask since Chris wanted the possibility to mix failures (which
> > makes sense when injecting a non-fatal failure somewhere). If he
> > doesn't insist on that possibility I can convert the mask option to a
> > counter based one identifying a single failure spot instead as you
> > suggest. Chris?
> We can extend the counter mechanism by having multiple counters behind
> i915.inject_load_failure (i.e. =gem:4,driver:10,modeset:1)

Now that there's a series to split down the init functions nicely, one
could use the function names directly. By stripping parts of it if
needed to shorten them.

Regards, Joonas

> so extensibility for more testing seems fine.
> -Chris
> 
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Chris Wilson]

On Wed, Mar 16, 2016 at 11:18:04AM +0200, Joonas Lahtinen wrote:
> On ti, 2016-03-15 at 14:14 +0000, Chris Wilson wrote:
> > On Tue, Mar 15, 2016 at 04:01:14PM +0200, Imre Deak wrote:
> > > 
> > > I'm not sure if you want to check all failure paths, I think for that
> > > the existing failslab etc. mechanisms are better suited. This new
> > > option would be used at relatively few well defined places. The option
> > > is a mask since Chris wanted the possibility to mix failures (which
> > > makes sense when injecting a non-fatal failure somewhere). If he
> > > doesn't insist on that possibility I can convert the mask option to a
> > > counter based one identifying a single failure spot instead as you
> > > suggest. Chris?
> > We can extend the counter mechanism by having multiple counters behind
> > i915.inject_load_failure (i.e. =gem:4,driver:10,modeset:1)
> 
> Now that there's a series to split down the init functions nicely, one
> could use the function names directly. By stripping parts of it if
> needed to shorten them.

That's nice. Advantage for using counters is that we can write a test to
iterate over the first thousand and have it run against future
faultpointers. Advantage for using names is that it is readable and
easily extensible.

What we could do for testing is record the available fault injection
points for debugfs and so have the tests automatically adjust (given a
working start point). Overkill?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

On Wed, 2016-03-16 at 09:24 +0000, Chris Wilson wrote:
> On Wed, Mar 16, 2016 at 11:18:04AM +0200, Joonas Lahtinen wrote:
> > On ti, 2016-03-15 at 14:14 +0000, Chris Wilson wrote:
> > > On Tue, Mar 15, 2016 at 04:01:14PM +0200, Imre Deak wrote:
> > > > 
> > > > I'm not sure if you want to check all failure paths, I think
> > > > for that
> > > > the existing failslab etc. mechanisms are better suited. This
> > > > new
> > > > option would be used at relatively few well defined places. The
> > > > option
> > > > is a mask since Chris wanted the possibility to mix failures
> > > > (which
> > > > makes sense when injecting a non-fatal failure somewhere). If
> > > > he
> > > > doesn't insist on that possibility I can convert the mask
> > > > option to a
> > > > counter based one identifying a single failure spot instead as
> > > > you
> > > > suggest. Chris?
> > > We can extend the counter mechanism by having multiple counters
> > > behind
> > > i915.inject_load_failure (i.e. =gem:4,driver:10,modeset:1)
> > 
> > Now that there's a series to split down the init functions nicely,
> > one
> > could use the function names directly. By stripping parts of it if
> > needed to shorten them.
> 
> That's nice. Advantage for using counters is that we can write a test
> to
> iterate over the first thousand and have it run against future
> faultpointers. Advantage for using names is that it is readable and
> easily extensible.
> 
> What we could do for testing is record the available fault injection
> points for debugfs and so have the tests automatically adjust (given
> a
> working start point). Overkill?

I'd prefer if we could fine grain things once we have more failure
points. Atm the ones we defined cause driver init to fail, so separate 
counters wouldn't bring us much. I suggest that we keep the module
option as a simple integer for now and extend it later to support the
above tagged counter mechanism and debugfs interface. Here is what I
planned to submit to the list:

https://github.com/ideak/linux/commits/driver_init_refactor

Comments?

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

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

[Chris Wilson]

On Wed, Mar 16, 2016 at 11:43:14AM +0200, Imre Deak wrote:
> On Wed, 2016-03-16 at 09:24 +0000, Chris Wilson wrote:
> > On Wed, Mar 16, 2016 at 11:18:04AM +0200, Joonas Lahtinen wrote:
> > > On ti, 2016-03-15 at 14:14 +0000, Chris Wilson wrote:
> > > > On Tue, Mar 15, 2016 at 04:01:14PM +0200, Imre Deak wrote:
> > > > > 
> > > > > I'm not sure if you want to check all failure paths, I think
> > > > > for that
> > > > > the existing failslab etc. mechanisms are better suited. This
> > > > > new
> > > > > option would be used at relatively few well defined places. The
> > > > > option
> > > > > is a mask since Chris wanted the possibility to mix failures
> > > > > (which
> > > > > makes sense when injecting a non-fatal failure somewhere). If
> > > > > he
> > > > > doesn't insist on that possibility I can convert the mask
> > > > > option to a
> > > > > counter based one identifying a single failure spot instead as
> > > > > you
> > > > > suggest. Chris?
> > > > We can extend the counter mechanism by having multiple counters
> > > > behind
> > > > i915.inject_load_failure (i.e. =gem:4,driver:10,modeset:1)
> > > 
> > > Now that there's a series to split down the init functions nicely,
> > > one
> > > could use the function names directly. By stripping parts of it if
> > > needed to shorten them.
> > 
> > That's nice. Advantage for using counters is that we can write a test
> > to
> > iterate over the first thousand and have it run against future
> > faultpointers. Advantage for using names is that it is readable and
> > easily extensible.
> > 
> > What we could do for testing is record the available fault injection
> > points for debugfs and so have the tests automatically adjust (given
> > a
> > working start point). Overkill?
> 
> I'd prefer if we could fine grain things once we have more failure
> points. Atm the ones we defined cause driver init to fail, so separate 
> counters wouldn't bring us much. I suggest that we keep the module
> option as a simple integer for now and extend it later to support the
> above tagged counter mechanism and debugfs interface. Here is what I
> planned to submit to the list:
> 
> https://github.com/ideak/linux/commits/driver_init_refactor

Move the inline into __i915_inject_load_failure, and yes I am happy with
the simple first step.  Please use __func__ or __FUNCTION__ though as
that is more descriptive than file, especially when they move.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[Imre Deak]

On Wed, 2016-03-16 at 10:04 +0000, Chris Wilson wrote:
> On Wed, Mar 16, 2016 at 11:43:14AM +0200, Imre Deak wrote:
> > On Wed, 2016-03-16 at 09:24 +0000, Chris Wilson wrote:
> > > On Wed, Mar 16, 2016 at 11:18:04AM +0200, Joonas Lahtinen wrote:
> > > > On ti, 2016-03-15 at 14:14 +0000, Chris Wilson wrote:
> > > > > On Tue, Mar 15, 2016 at 04:01:14PM +0200, Imre Deak wrote:
> > > > > > 
> > > > > > I'm not sure if you want to check all failure paths, I
> > > > > > think
> > > > > > for that
> > > > > > the existing failslab etc. mechanisms are better suited.
> > > > > > This
> > > > > > new
> > > > > > option would be used at relatively few well defined places.
> > > > > > The
> > > > > > option
> > > > > > is a mask since Chris wanted the possibility to mix
> > > > > > failures
> > > > > > (which
> > > > > > makes sense when injecting a non-fatal failure somewhere).
> > > > > > If
> > > > > > he
> > > > > > doesn't insist on that possibility I can convert the mask
> > > > > > option to a
> > > > > > counter based one identifying a single failure spot instead
> > > > > > as
> > > > > > you
> > > > > > suggest. Chris?
> > > > > We can extend the counter mechanism by having multiple
> > > > > counters
> > > > > behind
> > > > > i915.inject_load_failure (i.e. =gem:4,driver:10,modeset:1)
> > > > 
> > > > Now that there's a series to split down the init functions
> > > > nicely,
> > > > one
> > > > could use the function names directly. By stripping parts of it
> > > > if
> > > > needed to shorten them.
> > > 
> > > That's nice. Advantage for using counters is that we can write a
> > > test
> > > to
> > > iterate over the first thousand and have it run against future
> > > faultpointers. Advantage for using names is that it is readable
> > > and
> > > easily extensible.
> > > 
> > > What we could do for testing is record the available fault
> > > injection
> > > points for debugfs and so have the tests automatically adjust
> > > (given
> > > a
> > > working start point). Overkill?
> > 
> > I'd prefer if we could fine grain things once we have more failure
> > points. Atm the ones we defined cause driver init to fail, so
> > separate 
> > counters wouldn't bring us much. I suggest that we keep the module
> > option as a simple integer for now and extend it later to support
> > the
> > above tagged counter mechanism and debugfs interface. Here is what
> > I
> > planned to submit to the list:
> > 
> > https://github.com/ideak/linux/commits/driver_init_refactor
> 
> Move the inline into __i915_inject_load_failure

hm, I have the macro in the header file to optimize for the
failure_count=0 case. Or did you mean to
bring __i915_inject_load_failure to the header file too/merge it into
the macro?

> , and yes I am happy with
> the simple first step.  Please use __func__ or __FUNCTION__ though as
> that is more descriptive than file, especially when they move.

Ok.

--Imre

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

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

[Chris Wilson]

On Wed, Mar 16, 2016 at 12:17:51PM +0200, Imre Deak wrote:
> On Wed, 2016-03-16 at 10:04 +0000, Chris Wilson wrote:
> > On Wed, Mar 16, 2016 at 11:43:14AM +0200, Imre Deak wrote:
> > > On Wed, 2016-03-16 at 09:24 +0000, Chris Wilson wrote:
> > > > On Wed, Mar 16, 2016 at 11:18:04AM +0200, Joonas Lahtinen wrote:
> > > > > On ti, 2016-03-15 at 14:14 +0000, Chris Wilson wrote:
> > > > > > On Tue, Mar 15, 2016 at 04:01:14PM +0200, Imre Deak wrote:
> > > > > > > 
> > > > > > > I'm not sure if you want to check all failure paths, I
> > > > > > > think
> > > > > > > for that
> > > > > > > the existing failslab etc. mechanisms are better suited.
> > > > > > > This
> > > > > > > new
> > > > > > > option would be used at relatively few well defined places.
> > > > > > > The
> > > > > > > option
> > > > > > > is a mask since Chris wanted the possibility to mix
> > > > > > > failures
> > > > > > > (which
> > > > > > > makes sense when injecting a non-fatal failure somewhere).
> > > > > > > If
> > > > > > > he
> > > > > > > doesn't insist on that possibility I can convert the mask
> > > > > > > option to a
> > > > > > > counter based one identifying a single failure spot instead
> > > > > > > as
> > > > > > > you
> > > > > > > suggest. Chris?
> > > > > > We can extend the counter mechanism by having multiple
> > > > > > counters
> > > > > > behind
> > > > > > i915.inject_load_failure (i.e. =gem:4,driver:10,modeset:1)
> > > > > 
> > > > > Now that there's a series to split down the init functions
> > > > > nicely,
> > > > > one
> > > > > could use the function names directly. By stripping parts of it
> > > > > if
> > > > > needed to shorten them.
> > > > 
> > > > That's nice. Advantage for using counters is that we can write a
> > > > test
> > > > to
> > > > iterate over the first thousand and have it run against future
> > > > faultpointers. Advantage for using names is that it is readable
> > > > and
> > > > easily extensible.
> > > > 
> > > > What we could do for testing is record the available fault
> > > > injection
> > > > points for debugfs and so have the tests automatically adjust
> > > > (given
> > > > a
> > > > working start point). Overkill?
> > > 
> > > I'd prefer if we could fine grain things once we have more failure
> > > points. Atm the ones we defined cause driver init to fail, so
> > > separate 
> > > counters wouldn't bring us much. I suggest that we keep the module
> > > option as a simple integer for now and extend it later to support
> > > the
> > > above tagged counter mechanism and debugfs interface. Here is what
> > > I
> > > planned to submit to the list:
> > > 
> > > https://github.com/ideak/linux/commits/driver_init_refactor
> > 
> > Move the inline into __i915_inject_load_failure
> 
> hm, I have the macro in the header file to optimize for the
> failure_count=0 case. Or did you mean to
> bring __i915_inject_load_failure to the header file too/merge it into
> the macro?

Just the extra inlined conditional need not be in the header.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for drm/i915: Add fault injection support (rev2)

[Patchwork]

== Series Details ==

Series: drm/i915: Add fault injection support (rev2)
URL   : https://patchwork.freedesktop.org/series/4385/
State : failure

== Summary ==

Series 4385v2 drm/i915: Add fault injection support
2016-03-14T08:58:17.234391 http://patchwork.freedesktop.org/api/1.0/series/4385/revisions/2/mbox/
Applying: drm/i915: Add fault injection support
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_dma.c
M	drivers/gpu/drm/i915/i915_drv.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/i915_drv.h
Auto-merging drivers/gpu/drm/i915/i915_dma.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/i915_dma.c
Patch failed at 0001 drm/i915: Add fault injection support

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

✗ Fi.CI.BAT: failure for drm/i915: Add fault injection support (rev3)

[Patchwork]

== Series Details ==

Series: drm/i915: Add fault injection support (rev3)
URL   : https://patchwork.freedesktop.org/series/4385/
State : failure

== Summary ==

Series 4385v3 drm/i915: Add fault injection support
2016-03-14T14:37:24.542491 http://patchwork.freedesktop.org/api/1.0/series/4385/revisions/3/mbox/
Applying: drm/i915: Add fault injection support
Using index info to reconstruct a base tree...
M	drivers/gpu/drm/i915/i915_dma.c
M	drivers/gpu/drm/i915/i915_drv.h
Falling back to patching base and 3-way merge...
Auto-merging drivers/gpu/drm/i915/i915_drv.h
Auto-merging drivers/gpu/drm/i915/i915_dma.c
CONFLICT (content): Merge conflict in drivers/gpu/drm/i915/i915_dma.c
Patch failed at 0001 drm/i915: Add fault injection support

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