| Message ID | 20170830135609.9010-1-paul.kocialkowski@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Wed, Aug 30, 2017 at 04:56:09PM +0300, Paul Kocialkowski wrote: > This removes the igt_require condition on the sysfs open call used to > write the suspend/resume delay so that it is allowed to fail. Intsead, > the code that depends on it is put in a conditional block. > > This allows running test binaries as a non-privileged user for e.g. > listing the available tests with the SuspendResumeDelay parameter set > in igtrc configuration. Sysfs access would otherwise cause it to fail. > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com> Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> and pushed thanks for fixing it on a short notice!
On Wed, Aug 30, 2017 at 09:15:03PM +0300, Arkadiusz Hiler wrote: > On Wed, Aug 30, 2017 at 04:56:09PM +0300, Paul Kocialkowski wrote: > > This removes the igt_require condition on the sysfs open call used to > > write the suspend/resume delay so that it is allowed to fail. Intsead, > > the code that depends on it is put in a conditional block. > > > > This allows running test binaries as a non-privileged user for e.g. > > listing the available tests with the SuspendResumeDelay parameter set > > in igtrc configuration. Sysfs access would otherwise cause it to fail. > > > > Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com> > Reviewed-by: Arkadiusz Hiler <arkadiusz.hiler@intel.com> This is the wrong fix. When enumerating tests you're not supposed to touch anything hw or system related at all. Allocating a bit of memory and stuff like that is ok, but not anything with effects. The correct way to handle this is by wrapping this into an igt_fixture block. Hitting an igt_require/assert outside of an igt_fixture or igt_subtest should result in a assert. You still have igt_skip and igt_require in there, so not fixed properly yet. Aside: We need to come up with a way to have functions shared between parts of the library, without documenting and exporting them to tests. -Daniel
diff --git a/lib/igt_aux.c b/lib/igt_aux.c index f428f159..d808fe3e 100644 --- a/lib/igt_aux.c +++ b/lib/igt_aux.c @@ -883,19 +883,21 @@ void igt_set_autoresume_delay(int delay_secs) igt_skip_on_simulation(); - igt_require((delay_fd = open("/sys/module/suspend/parameters/pm_test_delay", - O_RDWR)) >= 0); + delay_fd = open("/sys/module/suspend/parameters/pm_test_delay", O_RDWR); + + if (delay_fd >= 0) { + if (!original_autoresume_delay) { + igt_require(read(delay_fd, delay_str, + sizeof(delay_str))); + original_autoresume_delay = atoi(delay_str); + igt_install_exit_handler(igt_restore_autoresume_delay); + } - if (!original_autoresume_delay) { - igt_require(read(delay_fd, delay_str, sizeof(delay_str))); - original_autoresume_delay = atoi(delay_str); - igt_install_exit_handler(igt_restore_autoresume_delay); - } + snprintf(delay_str, sizeof(delay_str), "%d", delay_secs); + igt_require(write(delay_fd, delay_str, strlen(delay_str))); - snprintf(delay_str, sizeof(delay_str), "%d", delay_secs); - igt_require(write(delay_fd, delay_str, strlen(delay_str))); - - close(delay_fd); + close(delay_fd); + } autoresume_delay = delay_secs; }
This removes the igt_require condition on the sysfs open call used to write the suspend/resume delay so that it is allowed to fail. Intsead, the code that depends on it is put in a conditional block. This allows running test binaries as a non-privileged user for e.g. listing the available tests with the SuspendResumeDelay parameter set in igtrc configuration. Sysfs access would otherwise cause it to fail. Signed-off-by: Paul Kocialkowski <paul.kocialkowski@linux.intel.com> --- lib/igt_aux.c | 24 +++++++++++++----------- 1 file changed, 13 insertions(+), 11 deletions(-)