| Message ID | 1463757605-27042-1-git-send-email-marius.c.vlad@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On pe, 2016-05-20 at 18:20 +0300, Marius Vlad wrote: > Either we return $IGT_EXIT_FAILURE or remove it entirely (like in > this > patch). If rmmod returns non-zero (i.e., Module: i915 is still in > use), reload > will bail with $IGT_EXIT_SKIP, making the check with lsmod useless. > Also use the return value in the fault-injection loop. > > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> > --- > tests/drv_module_reload_basic | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > diff --git a/tests/drv_module_reload_basic > b/tests/drv_module_reload_basic > index 3bba796..3a8df33 100755 > --- a/tests/drv_module_reload_basic > +++ b/tests/drv_module_reload_basic > @@ -30,7 +30,7 @@ function reload() { > > #ignore errors in ips - gen5 only > rmmod intel_ips &> /dev/null > - rmmod i915 || return $IGT_EXIT_SKIP > + rmmod i915 Not sure what was the reason to bail out here, continuing seems like the correct thing to do. > #ignore errors in intel-gtt, often built-in > rmmod intel-gtt &> /dev/null > # drm may be used by other devices (nouveau, radeon, udl, > etc) > @@ -76,7 +76,7 @@ finish_load || exit $? > > # Repeat the module reload trying to to generate faults > for i in $(seq 1 4); do > - reload inject_load_failure=$i > + reload inject_load_failure=$i || exit $? The idea was to keep the system in a working state even in case of failure here, so I'd still attempt a normal reload before exiting with failure. --Imre
On Fri, May 20, 2016 at 07:00:18PM +0300, Imre Deak wrote: > On pe, 2016-05-20 at 18:20 +0300, Marius Vlad wrote: > > Either we return $IGT_EXIT_FAILURE or remove it entirely (like in > > this > > patch). If rmmod returns non-zero (i.e., Module: i915 is still in > > use), reload > > will bail with $IGT_EXIT_SKIP, making the check with lsmod useless. > > Also use the return value in the fault-injection loop. > > > > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> > > --- > > tests/drv_module_reload_basic | 4 ++-- > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > diff --git a/tests/drv_module_reload_basic > > b/tests/drv_module_reload_basic > > index 3bba796..3a8df33 100755 > > --- a/tests/drv_module_reload_basic > > +++ b/tests/drv_module_reload_basic > > @@ -30,7 +30,7 @@ function reload() { > > > > #ignore errors in ips - gen5 only > > rmmod intel_ips &> /dev/null > > - rmmod i915 || return $IGT_EXIT_SKIP > > + rmmod i915 > > Not sure what was the reason to bail out here, continuing seems like > the correct thing to do. If we can't unload, we can perform the modprobe testing. The system is not in a state suitable for testing so skip or fail. If we are certain that the rmmod failure is a bug, fail, if it merely something like the system doesn't support module unloading, skip. Continuing on after failure to unload is not a good idea. -Chris
On pe, 2016-05-20 at 17:23 +0100, Chris Wilson wrote: > On Fri, May 20, 2016 at 07:00:18PM +0300, Imre Deak wrote: > > On pe, 2016-05-20 at 18:20 +0300, Marius Vlad wrote: > > > Either we return $IGT_EXIT_FAILURE or remove it entirely (like in > > > this > > > patch). If rmmod returns non-zero (i.e., Module: i915 is still in > > > use), reload > > > will bail with $IGT_EXIT_SKIP, making the check with lsmod > > > useless. > > > Also use the return value in the fault-injection loop. > > > > > > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> > > > --- > > > tests/drv_module_reload_basic | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/tests/drv_module_reload_basic > > > b/tests/drv_module_reload_basic > > > index 3bba796..3a8df33 100755 > > > --- a/tests/drv_module_reload_basic > > > +++ b/tests/drv_module_reload_basic > > > @@ -30,7 +30,7 @@ function reload() { > > > > > > #ignore errors in ips - gen5 only > > > rmmod intel_ips &> /dev/null > > > - rmmod i915 || return $IGT_EXIT_SKIP > > > + rmmod i915 > > > > Not sure what was the reason to bail out here, continuing seems > > like > > the correct thing to do. > > If we can't unload, we can perform the modprobe testing. The system > is > not in a state suitable for testing so skip or fail. If we are > certain > that the rmmod failure is a bug, fail, if it merely something like > the > system doesn't support module unloading, skip. > > Continuing on after failure to unload is not a good idea. I meant continuing here and depending on the lsmod check later to exit. > -Chris >
On Fri, May 20, 2016 at 05:23:56PM +0100, Chris Wilson wrote: > On Fri, May 20, 2016 at 07:00:18PM +0300, Imre Deak wrote: > > On pe, 2016-05-20 at 18:20 +0300, Marius Vlad wrote: > > > Either we return $IGT_EXIT_FAILURE or remove it entirely (like in > > > this > > > patch). If rmmod returns non-zero (i.e., Module: i915 is still in > > > use), reload > > > will bail with $IGT_EXIT_SKIP, making the check with lsmod useless. > > > Also use the return value in the fault-injection loop. > > > > > > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> > > > --- > > > tests/drv_module_reload_basic | 4 ++-- > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > diff --git a/tests/drv_module_reload_basic > > > b/tests/drv_module_reload_basic > > > index 3bba796..3a8df33 100755 > > > --- a/tests/drv_module_reload_basic > > > +++ b/tests/drv_module_reload_basic > > > @@ -30,7 +30,7 @@ function reload() { > > > > > > #ignore errors in ips - gen5 only > > > rmmod intel_ips &> /dev/null > > > - rmmod i915 || return $IGT_EXIT_SKIP > > > + rmmod i915 > > > > Not sure what was the reason to bail out here, continuing seems like > > the correct thing to do. > > If we can't unload, we can perform the modprobe testing. The system is > not in a state suitable for testing so skip or fail. If we are certain > that the rmmod failure is a bug, fail, if it merely something like the > system doesn't support module unloading, skip. I've seen this couple of times in the CI...and went mostly mostly unnoticed, wanted to make it hard-fail so it easy to spot when it happens again. Although it might be cases where the module is built-in this is not the case in CI. > > Continuing on after failure to unload is not a good idea. > -Chris > > -- > Chris Wilson, Intel Open Source Technology Centre
On Mon, May 23, 2016 at 01:43:42PM +0300, Marius Vlad wrote: > On Fri, May 20, 2016 at 05:23:56PM +0100, Chris Wilson wrote: > > On Fri, May 20, 2016 at 07:00:18PM +0300, Imre Deak wrote: > > > On pe, 2016-05-20 at 18:20 +0300, Marius Vlad wrote: > > > > Either we return $IGT_EXIT_FAILURE or remove it entirely (like in > > > > this > > > > patch). If rmmod returns non-zero (i.e., Module: i915 is still in > > > > use), reload > > > > will bail with $IGT_EXIT_SKIP, making the check with lsmod useless. > > > > Also use the return value in the fault-injection loop. > > > > > > > > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> > > > > --- > > > > tests/drv_module_reload_basic | 4 ++-- > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > diff --git a/tests/drv_module_reload_basic > > > > b/tests/drv_module_reload_basic > > > > index 3bba796..3a8df33 100755 > > > > --- a/tests/drv_module_reload_basic > > > > +++ b/tests/drv_module_reload_basic > > > > @@ -30,7 +30,7 @@ function reload() { > > > > > > > > #ignore errors in ips - gen5 only > > > > rmmod intel_ips &> /dev/null > > > > - rmmod i915 || return $IGT_EXIT_SKIP > > > > + rmmod i915 > > > > > > Not sure what was the reason to bail out here, continuing seems like > > > the correct thing to do. > > > > If we can't unload, we can perform the modprobe testing. The system is > > not in a state suitable for testing so skip or fail. If we are certain > > that the rmmod failure is a bug, fail, if it merely something like the > > system doesn't support module unloading, skip. > I've seen this couple of times in the CI...and went mostly mostly > unnoticed, wanted to make it hard-fail so it easy to spot when it happens > again. Although it might be cases where the module is built-in this is > not the case in CI. The || SKIP was added in commit f14d56c42d9e43df2790465aba6a2ea2593418fc Author: Chris Wilson <chris@chris-wilson.co.uk> Date: Fri Mar 11 21:25:48 2016 +0000 igt/drv_module_reload_basic: Rinse and repeat with addition module parameters and apparently not intentionally. Please cite that commit using Fixes: and ack on your patch from my side. -Daniel > > > > > Continuing on after failure to unload is not a good idea. > > -Chris > > > > -- > > Chris Wilson, Intel Open Source Technology Centre
On Tue, May 24, 2016 at 09:55:26AM +0200, Daniel Vetter wrote: > On Mon, May 23, 2016 at 01:43:42PM +0300, Marius Vlad wrote: > > On Fri, May 20, 2016 at 05:23:56PM +0100, Chris Wilson wrote: > > > On Fri, May 20, 2016 at 07:00:18PM +0300, Imre Deak wrote: > > > > On pe, 2016-05-20 at 18:20 +0300, Marius Vlad wrote: > > > > > Either we return $IGT_EXIT_FAILURE or remove it entirely (like in > > > > > this > > > > > patch). If rmmod returns non-zero (i.e., Module: i915 is still in > > > > > use), reload > > > > > will bail with $IGT_EXIT_SKIP, making the check with lsmod useless. > > > > > Also use the return value in the fault-injection loop. > > > > > > > > > > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> > > > > > --- > > > > > tests/drv_module_reload_basic | 4 ++-- > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > diff --git a/tests/drv_module_reload_basic > > > > > b/tests/drv_module_reload_basic > > > > > index 3bba796..3a8df33 100755 > > > > > --- a/tests/drv_module_reload_basic > > > > > +++ b/tests/drv_module_reload_basic > > > > > @@ -30,7 +30,7 @@ function reload() { > > > > > > > > > > #ignore errors in ips - gen5 only > > > > > rmmod intel_ips &> /dev/null > > > > > - rmmod i915 || return $IGT_EXIT_SKIP > > > > > + rmmod i915 > > > > > > > > Not sure what was the reason to bail out here, continuing seems like > > > > the correct thing to do. > > > > > > If we can't unload, we can perform the modprobe testing. The system is > > > not in a state suitable for testing so skip or fail. If we are certain > > > that the rmmod failure is a bug, fail, if it merely something like the > > > system doesn't support module unloading, skip. > > I've seen this couple of times in the CI...and went mostly mostly > > unnoticed, wanted to make it hard-fail so it easy to spot when it happens > > again. Although it might be cases where the module is built-in this is > > not the case in CI. > > The || SKIP was added in > > commit f14d56c42d9e43df2790465aba6a2ea2593418fc > Author: Chris Wilson <chris@chris-wilson.co.uk> > Date: Fri Mar 11 21:25:48 2016 +0000 > > igt/drv_module_reload_basic: Rinse and repeat with addition module parameters > > and apparently not intentionally. It was, because the test wasn't behaving properly on my setup where I had disabled module unloading. If we can't unload the module, we have to SKIP the test. -Chris
On Tue, May 24, 2016 at 09:03:19AM +0100, Chris Wilson wrote: > On Tue, May 24, 2016 at 09:55:26AM +0200, Daniel Vetter wrote: > > On Mon, May 23, 2016 at 01:43:42PM +0300, Marius Vlad wrote: > > > On Fri, May 20, 2016 at 05:23:56PM +0100, Chris Wilson wrote: > > > > On Fri, May 20, 2016 at 07:00:18PM +0300, Imre Deak wrote: > > > > > On pe, 2016-05-20 at 18:20 +0300, Marius Vlad wrote: > > > > > > Either we return $IGT_EXIT_FAILURE or remove it entirely (like in > > > > > > this > > > > > > patch). If rmmod returns non-zero (i.e., Module: i915 is still in > > > > > > use), reload > > > > > > will bail with $IGT_EXIT_SKIP, making the check with lsmod useless. > > > > > > Also use the return value in the fault-injection loop. > > > > > > > > > > > > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> > > > > > > --- > > > > > > tests/drv_module_reload_basic | 4 ++-- > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/tests/drv_module_reload_basic > > > > > > b/tests/drv_module_reload_basic > > > > > > index 3bba796..3a8df33 100755 > > > > > > --- a/tests/drv_module_reload_basic > > > > > > +++ b/tests/drv_module_reload_basic > > > > > > @@ -30,7 +30,7 @@ function reload() { > > > > > > > > > > > > #ignore errors in ips - gen5 only > > > > > > rmmod intel_ips &> /dev/null > > > > > > - rmmod i915 || return $IGT_EXIT_SKIP > > > > > > + rmmod i915 > > > > > > > > > > Not sure what was the reason to bail out here, continuing seems like > > > > > the correct thing to do. > > > > > > > > If we can't unload, we can perform the modprobe testing. The system is > > > > not in a state suitable for testing so skip or fail. If we are certain > > > > that the rmmod failure is a bug, fail, if it merely something like the > > > > system doesn't support module unloading, skip. > > > I've seen this couple of times in the CI...and went mostly mostly > > > unnoticed, wanted to make it hard-fail so it easy to spot when it happens > > > again. Although it might be cases where the module is built-in this is > > > not the case in CI. > > > > The || SKIP was added in > > > > commit f14d56c42d9e43df2790465aba6a2ea2593418fc > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > Date: Fri Mar 11 21:25:48 2016 +0000 > > > > igt/drv_module_reload_basic: Rinse and repeat with addition module parameters > > > > and apparently not intentionally. > > It was, because the test wasn't behaving properly on my setup where I > had disabled module unloading. If we can't unload the module, we have to > SKIP the test. The problem is that we actually have the test split up incorrectly. Instead of doing a preparation phase; for_each_subtest load; check; unload check; restoration we have for_each_subtest; unload load check and so we are doingthe tail of the first test in the second. -Chris
On Tue, May 24, 2016 at 09:03:19AM +0100, Chris Wilson wrote: > On Tue, May 24, 2016 at 09:55:26AM +0200, Daniel Vetter wrote: > > On Mon, May 23, 2016 at 01:43:42PM +0300, Marius Vlad wrote: > > > On Fri, May 20, 2016 at 05:23:56PM +0100, Chris Wilson wrote: > > > > On Fri, May 20, 2016 at 07:00:18PM +0300, Imre Deak wrote: > > > > > On pe, 2016-05-20 at 18:20 +0300, Marius Vlad wrote: > > > > > > Either we return $IGT_EXIT_FAILURE or remove it entirely (like in > > > > > > this > > > > > > patch). If rmmod returns non-zero (i.e., Module: i915 is still in > > > > > > use), reload > > > > > > will bail with $IGT_EXIT_SKIP, making the check with lsmod useless. > > > > > > Also use the return value in the fault-injection loop. > > > > > > > > > > > > Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> > > > > > > --- > > > > > > tests/drv_module_reload_basic | 4 ++-- > > > > > > 1 file changed, 2 insertions(+), 2 deletions(-) > > > > > > > > > > > > diff --git a/tests/drv_module_reload_basic > > > > > > b/tests/drv_module_reload_basic > > > > > > index 3bba796..3a8df33 100755 > > > > > > --- a/tests/drv_module_reload_basic > > > > > > +++ b/tests/drv_module_reload_basic > > > > > > @@ -30,7 +30,7 @@ function reload() { > > > > > > > > > > > > #ignore errors in ips - gen5 only > > > > > > rmmod intel_ips &> /dev/null > > > > > > - rmmod i915 || return $IGT_EXIT_SKIP > > > > > > + rmmod i915 > > > > > > > > > > Not sure what was the reason to bail out here, continuing seems like > > > > > the correct thing to do. > > > > > > > > If we can't unload, we can perform the modprobe testing. The system is > > > > not in a state suitable for testing so skip or fail. If we are certain > > > > that the rmmod failure is a bug, fail, if it merely something like the > > > > system doesn't support module unloading, skip. > > > I've seen this couple of times in the CI...and went mostly mostly > > > unnoticed, wanted to make it hard-fail so it easy to spot when it happens > > > again. Although it might be cases where the module is built-in this is > > > not the case in CI. > > > > The || SKIP was added in > > > > commit f14d56c42d9e43df2790465aba6a2ea2593418fc > > Author: Chris Wilson <chris@chris-wilson.co.uk> > > Date: Fri Mar 11 21:25:48 2016 +0000 > > > > igt/drv_module_reload_basic: Rinse and repeat with addition module parameters > > > > and apparently not intentionally. > > It was, because the test wasn't behaving properly on my setup where I > had disabled module unloading. If we can't unload the module, we have to > SKIP the test. I think a better check would be to lsmod | grep i915 and bail if that's false. Yeah that'll still fall over if you have unloading disabled, but either don't do that, or have some other check instead of rmmod i915 to assess whether unloading works. And please don't hide such changes in another commit without even mentioning. i-g-t isn't free-for-all anymore, and if too much stuff slips through then we'd have to lock down commit process a lot. I kinda don't want that, since I still see plenty of benefits in making it simple to create more tests. -Daniel
diff --git a/tests/drv_module_reload_basic b/tests/drv_module_reload_basic index 3bba796..3a8df33 100755 --- a/tests/drv_module_reload_basic +++ b/tests/drv_module_reload_basic @@ -30,7 +30,7 @@ function reload() { #ignore errors in ips - gen5 only rmmod intel_ips &> /dev/null - rmmod i915 || return $IGT_EXIT_SKIP + rmmod i915 #ignore errors in intel-gtt, often built-in rmmod intel-gtt &> /dev/null # drm may be used by other devices (nouveau, radeon, udl, etc) @@ -76,7 +76,7 @@ finish_load || exit $? # Repeat the module reload trying to to generate faults for i in $(seq 1 4); do - reload inject_load_failure=$i + reload inject_load_failure=$i || exit $? done reload || exit $?
Either we return $IGT_EXIT_FAILURE or remove it entirely (like in this patch). If rmmod returns non-zero (i.e., Module: i915 is still in use), reload will bail with $IGT_EXIT_SKIP, making the check with lsmod useless. Also use the return value in the fault-injection loop. Signed-off-by: Marius Vlad <marius.c.vlad@intel.com> --- tests/drv_module_reload_basic | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)