[Intel-gfx] [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 at 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 at 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