| Message ID | 20180606174214.17969-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Jun 06, 2018 at 06:42:14PM +0100, Chris Wilson wrote: > The current method of checking for a failed module load is flawed, as we > only report the error on probing it is not being reported back by > modprobe. So we have to dig inside the module_parameters while the > module is still loaded to discover the error. > > v2: Expect i915.inject_load_failure to be zero on success > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> > --- > tests/drv_module_reload.c | 45 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 40 insertions(+), 5 deletions(-) > > diff --git a/tests/drv_module_reload.c b/tests/drv_module_reload.c > index 092982960..e18aaea5e 100644 > --- a/tests/drv_module_reload.c > +++ b/tests/drv_module_reload.c > @@ -234,6 +234,38 @@ reload(const char *opts_i915) > return err; > } > > +static int open_parameters(const char *module_name) > +{ > + char path[256]; > + > + snprintf(path, sizeof(path), "/sys/module/%s/parameters", module_name); > + return open(path, O_RDONLY); > +} > + > +static int > +inject_fault(const char *module_name, const char *opt, int fault) > +{ > + char buf[1024]; > + int dir; > + > + igt_assert(fault > 0); > + snprintf(buf, sizeof(buf), "%s=%d", opt, fault); > + > + if (igt_kmod_load(module_name, buf)) { > + igt_warn("Failed to load module '%s' with options '%s'\n", > + module_name, buf); > + return 1; > + } > + > + dir = open_parameters(module_name); > + igt_sysfs_scanf(dir, opt, "%d", &fault); Just a note for later: in case we switch to async probing we'll have to wait somehow until the probe completed. One way would be to disable async probing for this test, not sure if that could hide some problem though. > + close(dir); > + > + igt_kmod_unload(module_name, 0); > + > + return fault; > +} > + > static void > gem_sanitycheck(void) > { > @@ -323,12 +355,15 @@ igt_main > igt_assert_eq(reload("disable_display=1"), 0); > > igt_subtest("basic-reload-inject") { > - char buf[64]; > int i = 0; > - do { > - snprintf(buf, sizeof(buf), > - "inject_load_failure=%d", ++i); > - } while (reload(buf)); > + > + igt_i915_driver_unload(); > + > + while (inject_fault("i915", "inject_load_failure", ++i) == 0) > + ; > + > + /* We expect to hit at least one fault! */ > + igt_assert(i > 1); > } > > igt_fixture { > -- > 2.17.1 >
Quoting Imre Deak (2018-06-06 19:00:52) > On Wed, Jun 06, 2018 at 06:42:14PM +0100, Chris Wilson wrote: > > The current method of checking for a failed module load is flawed, as we > > only report the error on probing it is not being reported back by > > modprobe. So we have to dig inside the module_parameters while the > > module is still loaded to discover the error. > > > > v2: Expect i915.inject_load_failure to be zero on success > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Michał Winiarski <michal.winiarski@intel.com> > > Cc: Imre Deak <imre.deak@intel.com> > > Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> > > --- > > tests/drv_module_reload.c | 45 ++++++++++++++++++++++++++++++++++----- > > 1 file changed, 40 insertions(+), 5 deletions(-) > > > > diff --git a/tests/drv_module_reload.c b/tests/drv_module_reload.c > > index 092982960..e18aaea5e 100644 > > --- a/tests/drv_module_reload.c > > +++ b/tests/drv_module_reload.c > > @@ -234,6 +234,38 @@ reload(const char *opts_i915) > > return err; > > } > > > > +static int open_parameters(const char *module_name) > > +{ > > + char path[256]; > > + > > + snprintf(path, sizeof(path), "/sys/module/%s/parameters", module_name); > > + return open(path, O_RDONLY); > > +} > > + > > +static int > > +inject_fault(const char *module_name, const char *opt, int fault) > > +{ > > + char buf[1024]; > > + int dir; > > + > > + igt_assert(fault > 0); > > + snprintf(buf, sizeof(buf), "%s=%d", opt, fault); > > + > > + if (igt_kmod_load(module_name, buf)) { > > + igt_warn("Failed to load module '%s' with options '%s'\n", > > + module_name, buf); > > + return 1; > > + } > > + > > + dir = open_parameters(module_name); > > + igt_sysfs_scanf(dir, opt, "%d", &fault); > > Just a note for later: in case we switch to async probing we'll have to > wait somehow until the probe completed. One way would be to disable > async probing for this test, not sure if that could hide some problem > though. modprobe already waits for async probes (via async_synchronize_full() before it returns to userspace). Afaict, that async flag only has any relevance for builtins. If you want to parallel module loading, you need to fork. -Chris
On Wed, Jun 06, 2018 at 07:04:47PM +0100, Chris Wilson wrote: > Quoting Imre Deak (2018-06-06 19:00:52) > > On Wed, Jun 06, 2018 at 06:42:14PM +0100, Chris Wilson wrote: > > > The current method of checking for a failed module load is flawed, as we > > > only report the error on probing it is not being reported back by > > > modprobe. So we have to dig inside the module_parameters while the > > > module is still loaded to discover the error. > > > > > > v2: Expect i915.inject_load_failure to be zero on success > > > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Michał Winiarski <michal.winiarski@intel.com> > > > Cc: Imre Deak <imre.deak@intel.com> > > > Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> > > > --- > > > tests/drv_module_reload.c | 45 ++++++++++++++++++++++++++++++++++----- > > > 1 file changed, 40 insertions(+), 5 deletions(-) > > > > > > diff --git a/tests/drv_module_reload.c b/tests/drv_module_reload.c > > > index 092982960..e18aaea5e 100644 > > > --- a/tests/drv_module_reload.c > > > +++ b/tests/drv_module_reload.c > > > @@ -234,6 +234,38 @@ reload(const char *opts_i915) > > > return err; > > > } > > > > > > +static int open_parameters(const char *module_name) > > > +{ > > > + char path[256]; > > > + > > > + snprintf(path, sizeof(path), "/sys/module/%s/parameters", module_name); > > > + return open(path, O_RDONLY); > > > +} > > > + > > > +static int > > > +inject_fault(const char *module_name, const char *opt, int fault) > > > +{ > > > + char buf[1024]; > > > + int dir; > > > + > > > + igt_assert(fault > 0); > > > + snprintf(buf, sizeof(buf), "%s=%d", opt, fault); > > > + > > > + if (igt_kmod_load(module_name, buf)) { > > > + igt_warn("Failed to load module '%s' with options '%s'\n", > > > + module_name, buf); > > > + return 1; > > > + } > > > + > > > + dir = open_parameters(module_name); > > > + igt_sysfs_scanf(dir, opt, "%d", &fault); > > > > Just a note for later: in case we switch to async probing we'll have to > > wait somehow until the probe completed. One way would be to disable > > async probing for this test, not sure if that could hide some problem > > though. > > modprobe already waits for async probes (via async_synchronize_full() > before it returns to userspace). Afaict, that async flag only has any > relevance for builtins. If you want to parallel module loading, you need > to fork. Hm, right. Looking at that now, there's also the async_probe module param, but we only need to care about the driver flag. So looks ok then. > -Chris
On 06/06/18 10:42, Chris Wilson wrote: > The current method of checking for a failed module load is flawed, as we > only report the error on probing it is not being reported back by > modprobe. So we have to dig inside the module_parameters while the > module is still loaded to discover the error. > > v2: Expect i915.inject_load_failure to be zero on success > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Michał Winiarski <michal.winiarski@intel.com> > Cc: Imre Deak <imre.deak@intel.com> > Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> > --- > tests/drv_module_reload.c | 45 ++++++++++++++++++++++++++++++++++----- > 1 file changed, 40 insertions(+), 5 deletions(-) > > diff --git a/tests/drv_module_reload.c b/tests/drv_module_reload.c > index 092982960..e18aaea5e 100644 > --- a/tests/drv_module_reload.c > +++ b/tests/drv_module_reload.c > @@ -234,6 +234,38 @@ reload(const char *opts_i915) > return err; > } > > static void > gem_sanitycheck(void) > { > @@ -323,12 +355,15 @@ igt_main > igt_assert_eq(reload("disable_display=1"), 0); > > igt_subtest("basic-reload-inject") { > - char buf[64]; > int i = 0; > - do { > - snprintf(buf, sizeof(buf), > - "inject_load_failure=%d", ++i); > - } while (reload(buf)); > + > + igt_i915_driver_unload(); > + > + while (inject_fault("i915", "inject_load_failure", ++i) == 0) > + ; > + > + /* We expect to hit at least one fault! */ > + igt_assert(i > 1); I think Michal's patch adds the number of available checkpoints in a debugfs, should we trust the driver and assert on: amount of checkpoints hit != available checkpoints? Or maybe just spew out a warning. Thanks, Antonio > } > > igt_fixture { >
Quoting Antonio Argenziano (2018-06-06 21:48:22) > > > On 06/06/18 10:42, Chris Wilson wrote: > > The current method of checking for a failed module load is flawed, as we > > only report the error on probing it is not being reported back by > > modprobe. So we have to dig inside the module_parameters while the > > module is still loaded to discover the error. > > > > v2: Expect i915.inject_load_failure to be zero on success > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Michał Winiarski <michal.winiarski@intel.com> > > Cc: Imre Deak <imre.deak@intel.com> > > Reviewed-by: Michał Winiarski <michal.winiarski@intel.com> > > --- > > tests/drv_module_reload.c | 45 ++++++++++++++++++++++++++++++++++----- > > 1 file changed, 40 insertions(+), 5 deletions(-) > > > > diff --git a/tests/drv_module_reload.c b/tests/drv_module_reload.c > > index 092982960..e18aaea5e 100644 > > --- a/tests/drv_module_reload.c > > +++ b/tests/drv_module_reload.c > > @@ -234,6 +234,38 @@ reload(const char *opts_i915) > > return err; > > } > > > > > static void > > gem_sanitycheck(void) > > { > > @@ -323,12 +355,15 @@ igt_main > > igt_assert_eq(reload("disable_display=1"), 0); > > > > igt_subtest("basic-reload-inject") { > > - char buf[64]; > > int i = 0; > > - do { > > - snprintf(buf, sizeof(buf), > > - "inject_load_failure=%d", ++i); > > - } while (reload(buf)); > > + > > + igt_i915_driver_unload(); > > + > > + while (inject_fault("i915", "inject_load_failure", ++i) == 0) > > + ; > > + > > + /* We expect to hit at least one fault! */ > > + igt_assert(i > 1); > > I think Michal's patch adds the number of available checkpoints in a > debugfs, should we trust the driver and assert on: amount of checkpoints > hit != available checkpoints? Or maybe just spew out a warning. This loop hits all the fault points you can hit. There is nothing more the driver nor igt can do. The only assertion we have there is to basically catch the case where the protocol fails, or there are no fault points built into the driver. That is trusting the driver less than expecting it to report the exact number of reachable fault points. -Chris
diff --git a/tests/drv_module_reload.c b/tests/drv_module_reload.c index 092982960..e18aaea5e 100644 --- a/tests/drv_module_reload.c +++ b/tests/drv_module_reload.c @@ -234,6 +234,38 @@ reload(const char *opts_i915) return err; } +static int open_parameters(const char *module_name) +{ + char path[256]; + + snprintf(path, sizeof(path), "/sys/module/%s/parameters", module_name); + return open(path, O_RDONLY); +} + +static int +inject_fault(const char *module_name, const char *opt, int fault) +{ + char buf[1024]; + int dir; + + igt_assert(fault > 0); + snprintf(buf, sizeof(buf), "%s=%d", opt, fault); + + if (igt_kmod_load(module_name, buf)) { + igt_warn("Failed to load module '%s' with options '%s'\n", + module_name, buf); + return 1; + } + + dir = open_parameters(module_name); + igt_sysfs_scanf(dir, opt, "%d", &fault); + close(dir); + + igt_kmod_unload(module_name, 0); + + return fault; +} + static void gem_sanitycheck(void) { @@ -323,12 +355,15 @@ igt_main igt_assert_eq(reload("disable_display=1"), 0); igt_subtest("basic-reload-inject") { - char buf[64]; int i = 0; - do { - snprintf(buf, sizeof(buf), - "inject_load_failure=%d", ++i); - } while (reload(buf)); + + igt_i915_driver_unload(); + + while (inject_fault("i915", "inject_load_failure", ++i) == 0) + ; + + /* We expect to hit at least one fault! */ + igt_assert(i > 1); } igt_fixture {