[PATCH i-g-t] tests/drv_module_reload_basic: Don't use rmmod exit code when reloading the module.

[PATCH i-g-t] tests/drv_module_reload_basic: Don't use rmmod exit code when reloading the module.

[Marius Vlad]

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
 	#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 $?
-- 
2.5.0

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

Re: [PATCH i-g-t] tests/drv_module_reload_basic: Don't use rmmod exit code when reloading the module.

[Imre Deak]

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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] tests/drv_module_reload_basic: Don't use rmmod exit code when reloading the module.

[Chris Wilson]

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

-- 
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 i-g-t] tests/drv_module_reload_basic: Don't use rmmod exit code when reloading the module.

[Imre Deak]

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
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH i-g-t] tests/drv_module_reload_basic: Don't use rmmod exit code when reloading the module.

[Marius Vlad]

[-- Attachment #1.1: Type: text/plain, Size: 1862 bytes --]

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

[-- Attachment #1.2: Digital signature --]
[-- Type: application/pgp-signature, Size: 473 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH i-g-t] tests/drv_module_reload_basic: Don't use rmmod exit code when reloading the module.

[Daniel Vetter]

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



-- 
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 i-g-t] tests/drv_module_reload_basic: Don't use rmmod exit code when reloading the module.

[Chris Wilson]

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

-- 
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 i-g-t] tests/drv_module_reload_basic: Don't use rmmod exit code when reloading the module.

[Chris Wilson]

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

-- 
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 i-g-t] tests/drv_module_reload_basic: Don't use rmmod exit code when reloading the module.

[Daniel Vetter]

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