[PATCH i-g-t] igt/pm_rpm: Close local fd before trying to unload module

[PATCH i-g-t] igt/pm_rpm: Close local fd before trying to unload module

[Chris Wilson]

Fixes: d8e78990aa2b ("igt/pm_rpm: Test reaquisition of runtime-pm after module reload")
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 tests/pm_rpm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
index 65489bcdb..a4f9f783e 100644
--- a/tests/pm_rpm.c
+++ b/tests/pm_rpm.c
@@ -2034,6 +2034,8 @@ int main(int argc, char *argv[])
 		teardown_environment();
 
 	igt_subtest("module-reload") {
+		teardown_environment();
+
 		igt_debug("Reload w/o display\n");
 		igt_i915_driver_unload();
 		igt_assert_eq(igt_i915_driver_load("disable_display=1"), 0);
-- 
2.18.0

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

Re: [igt-dev] [PATCH i-g-t] igt/pm_rpm: Close local fd before trying to unload module

[Imre Deak]

On Wed, Aug 15, 2018 at 10:25:11AM +0100, Chris Wilson wrote:
> Fixes: d8e78990aa2b ("igt/pm_rpm: Test reaquisition of runtime-pm after module reload")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  tests/pm_rpm.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
> index 65489bcdb..a4f9f783e 100644
> --- a/tests/pm_rpm.c
> +++ b/tests/pm_rpm.c
> @@ -2034,6 +2034,8 @@ int main(int argc, char *argv[])
>  		teardown_environment();
>  
>  	igt_subtest("module-reload") {
> +		teardown_environment();
> +

Why isn't the teardown_environment(); under igt_fixture enough?

>  		igt_debug("Reload w/o display\n");
>  		igt_i915_driver_unload();
>  		igt_assert_eq(igt_i915_driver_load("disable_display=1"), 0);
> -- 
> 2.18.0
> 
> _______________________________________________
> igt-dev mailing list
> igt-dev@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/igt-dev
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [PATCH i-g-t] igt/pm_rpm: Close local fd before trying to unload module

[Chris Wilson]

Quoting Imre Deak (2018-08-15 12:01:31)
> On Wed, Aug 15, 2018 at 10:25:11AM +0100, Chris Wilson wrote:
> > Fixes: d8e78990aa2b ("igt/pm_rpm: Test reaquisition of runtime-pm after module reload")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  tests/pm_rpm.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
> > index 65489bcdb..a4f9f783e 100644
> > --- a/tests/pm_rpm.c
> > +++ b/tests/pm_rpm.c
> > @@ -2034,6 +2034,8 @@ int main(int argc, char *argv[])
> >               teardown_environment();
> >  
> >       igt_subtest("module-reload") {
> > +             teardown_environment();
> > +
> 
> Why isn't the teardown_environment(); under igt_fixture enough?

Hmm, looks like I thought about it earlier. I guess we leak an fd...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [PATCH i-g-t] igt/pm_rpm: Close local fd before trying to unload module

[Antonio Argenziano]

On 15/08/18 02:25, Chris Wilson wrote:
> Fixes: d8e78990aa2b ("igt/pm_rpm: Test reaquisition of runtime-pm after module reload")
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>   tests/pm_rpm.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
> index 65489bcdb..a4f9f783e 100644
> --- a/tests/pm_rpm.c
> +++ b/tests/pm_rpm.c
> @@ -2034,6 +2034,8 @@ int main(int argc, char *argv[])
>   		teardown_environment();
>   
>   	igt_subtest("module-reload") {
> +		teardown_environment();

There is already a fixture with a call to 'teardown_environment()' 
surrounding this test, is it missing a couple of subtests groups?

Antonio

> +
>   		igt_debug("Reload w/o display\n");
>   		igt_i915_driver_unload();
>   		igt_assert_eq(igt_i915_driver_load("disable_display=1"), 0);
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [PATCH i-g-t] igt/pm_rpm: Close local fd before trying to unload module

[Chris Wilson]

Quoting Antonio Argenziano (2018-08-17 18:29:09)
> 
> 
> On 15/08/18 02:25, Chris Wilson wrote:
> > Fixes: d8e78990aa2b ("igt/pm_rpm: Test reaquisition of runtime-pm after module reload")
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >   tests/pm_rpm.c | 2 ++
> >   1 file changed, 2 insertions(+)
> > 
> > diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
> > index 65489bcdb..a4f9f783e 100644
> > --- a/tests/pm_rpm.c
> > +++ b/tests/pm_rpm.c
> > @@ -2034,6 +2034,8 @@ int main(int argc, char *argv[])
> >               teardown_environment();
> >   
> >       igt_subtest("module-reload") {
> > +             teardown_environment();
> 
> There is already a fixture with a call to 'teardown_environment()' 
> surrounding this test, is it missing a couple of subtests groups?

It was intended for sequential execution (and I had forgotten that I had
purposely placed it after the teardown_environment -- the confusion was
caused by teardown_environment being incomplete). Adding a subtest group
for the preceding bunch would have the debatable consequence of module-
reload reporting a FAIL if something else caused a wakeref leak. My
opinion is to prefer SKIP for external artefacts so that it is clear when
a test failed of its own accord (and so be useful for debugging). It just
so happens that this test was to reproduce the trigger for the external
failures elsewhere and so detect the wakeref leak.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [igt-dev] [PATCH i-g-t] igt/pm_rpm: Close local fd before trying to unload module

[Antonio Argenziano]

On 17/08/18 10:49, Chris Wilson wrote:
> Quoting Antonio Argenziano (2018-08-17 18:29:09)
>>
>>
>> On 15/08/18 02:25, Chris Wilson wrote:
>>> Fixes: d8e78990aa2b ("igt/pm_rpm: Test reaquisition of runtime-pm after module reload")
>>> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
>>> ---
>>>    tests/pm_rpm.c | 2 ++
>>>    1 file changed, 2 insertions(+)
>>>
>>> diff --git a/tests/pm_rpm.c b/tests/pm_rpm.c
>>> index 65489bcdb..a4f9f783e 100644
>>> --- a/tests/pm_rpm.c
>>> +++ b/tests/pm_rpm.c
>>> @@ -2034,6 +2034,8 @@ int main(int argc, char *argv[])
>>>                teardown_environment();
>>>    
>>>        igt_subtest("module-reload") {
>>> +             teardown_environment();
>>
>> There is already a fixture with a call to 'teardown_environment()'
>> surrounding this test, is it missing a couple of subtests groups?
> 
> It was intended for sequential execution (and I had forgotten that I had
> purposely placed it after the teardown_environment -- the confusion was
> caused by teardown_environment being incomplete). Adding a subtest group
> for the preceding bunch would have the debatable consequence of module-
> reload reporting a FAIL if something else caused a wakeref leak. My
> opinion is to prefer SKIP for external artefacts so that it is clear when
> a test failed of its own accord (and so be useful for debugging). It just
> so happens that this test was to reproduce the trigger for the external
> failures elsewhere and so detect the wakeref leak.

Agreed. Maybe a comment would help, your choice.

Reviewed-by: Antonio Argenziano <antonio.argenziano@intel.com>

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