| Message ID | 20171208162717.34318-1-antonio.argenziano@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Quoting Antonio Argenziano (2017-12-08 16:27:17) > The test expected IOCTL 'I915_GET_RESET_STATS' would return an error > when not root. That is no longer true in the driver and therefore > the test was incorrectly failing. > > Cc: Michel Thierry <michel.thierry@intel.com> > Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> > Signed-off-by: Antonio Argenziano <antonio.argenziano@intel.com> > --- > tests/gem_reset_stats.c | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/tests/gem_reset_stats.c b/tests/gem_reset_stats.c > index edc40767..83c91f0f 100644 > --- a/tests/gem_reset_stats.c > +++ b/tests/gem_reset_stats.c > @@ -605,10 +605,7 @@ static void test_reset_count(const struct intel_execution_engine *e, > > c2 = get_reset_count(fd, ctx); > > - if (ctx == 0) > - igt_assert(c2 == -EPERM); > - else > - igt_assert(c2 == 0); > + igt_assert(c2 == 0); > } > > igt_waitchildren(); > @@ -619,6 +616,11 @@ static void test_reset_count(const struct intel_execution_engine *e, > close(fd); > } > > +static int __get_reset_stats(int fd, struct local_drm_i915_reset_stats *rs) > +{ > + return drmIoctl(fd, GET_RESET_STATS_IOCTL, &rs); > +} > + > static int _test_params(int fd, int ctx, uint32_t flags, uint32_t pad) > { > struct local_drm_i915_reset_stats rs; > @@ -644,10 +646,16 @@ static void _check_param_ctx(const int fd, const int ctx, const cap_t cap) > const uint32_t bad = rand() + 1; > > if (ctx == 0) { > - if (cap == root) > igt_assert_eq(_test_params(fd, ctx, 0, 0), 0); Spurious indenting leftover. > - else > - igt_assert_eq(_test_params(fd, ctx, 0, 0), -EPERM); > + if (cap != root) { So what are you expecting to happen if you do happen to be rot? Is this test redundant, which is why you skipped it? -Chris
On 08/12/17 08:46, Chris Wilson wrote: > Quoting Antonio Argenziano (2017-12-08 16:27:17) >> The test expected IOCTL 'I915_GET_RESET_STATS' would return an error >> when not root. That is no longer true in the driver and therefore >> the test was incorrectly failing. >> >> Cc: Michel Thierry <michel.thierry@intel.com> >> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >> Signed-off-by: Antonio Argenziano <antonio.argenziano@intel.com> >> --- >> tests/gem_reset_stats.c | 22 +++++++++++++++------- >> 1 file changed, 15 insertions(+), 7 deletions(-) >> >> diff --git a/tests/gem_reset_stats.c b/tests/gem_reset_stats.c >> index edc40767..83c91f0f 100644 >> --- a/tests/gem_reset_stats.c >> +++ b/tests/gem_reset_stats.c >> @@ -605,10 +605,7 @@ static void test_reset_count(const struct intel_execution_engine *e, >> >> c2 = get_reset_count(fd, ctx); >> >> - if (ctx == 0) >> - igt_assert(c2 == -EPERM); >> - else >> - igt_assert(c2 == 0); >> + igt_assert(c2 == 0); >> } >> >> igt_waitchildren(); >> @@ -619,6 +616,11 @@ static void test_reset_count(const struct intel_execution_engine *e, >> close(fd); >> } >> >> +static int __get_reset_stats(int fd, struct local_drm_i915_reset_stats *rs) >> +{ >> + return drmIoctl(fd, GET_RESET_STATS_IOCTL, &rs); >> +} >> + >> static int _test_params(int fd, int ctx, uint32_t flags, uint32_t pad) >> { >> struct local_drm_i915_reset_stats rs; >> @@ -644,10 +646,16 @@ static void _check_param_ctx(const int fd, const int ctx, const cap_t cap) >> const uint32_t bad = rand() + 1; >> >> if (ctx == 0) { >> - if (cap == root) >> igt_assert_eq(_test_params(fd, ctx, 0, 0), 0); > > Spurious indenting leftover. > >> - else >> - igt_assert_eq(_test_params(fd, ctx, 0, 0), -EPERM); >> + if (cap != root) { > > So what are you expecting to happen if you do happen to be rot? Is this > test redundant, which is why you skipped it? Yes, I think it is redundant because the only expectation for root is for the IOCTL to be successful as it is for non root users (that is why I left the first assert to be run unconditionally), and, even if root is supposed to get the correct reset_count value, unless I am missing something, that test is not in the scope of this subtest. -Antonio > -Chris >
On 08/12/17 09:07, Antonio Argenziano wrote: > > > On 08/12/17 08:46, Chris Wilson wrote: >> Quoting Antonio Argenziano (2017-12-08 16:27:17) >>> The test expected IOCTL 'I915_GET_RESET_STATS' would return an error >>> when not root. That is no longer true in the driver and therefore I would add the commit that changed the behaviour, ...when not root. This is no longer true in the driver since commit 4c9c0d09741d ("drm/i915: Fix retrieval of hangcheck stats") and therefore... >>> the test was incorrectly failing. >>> >>> Cc: Michel Thierry <michel.thierry@intel.com> >>> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >>> Signed-off-by: Antonio Argenziano <antonio.argenziano@intel.com> >>> --- >>> tests/gem_reset_stats.c | 22 +++++++++++++++------- >>> 1 file changed, 15 insertions(+), 7 deletions(-) >>> >>> diff --git a/tests/gem_reset_stats.c b/tests/gem_reset_stats.c >>> index edc40767..83c91f0f 100644 >>> --- a/tests/gem_reset_stats.c >>> +++ b/tests/gem_reset_stats.c >>> @@ -605,10 +605,7 @@ static void test_reset_count(const struct intel_execution_engine *e, >>> >>> c2 = get_reset_count(fd, ctx); >>> >>> - if (ctx == 0) >>> - igt_assert(c2 == -EPERM); >>> - else >>> - igt_assert(c2 == 0); >>> + igt_assert(c2 == 0); >>> } >>> >>> igt_waitchildren(); >>> @@ -619,6 +616,11 @@ static void test_reset_count(const struct intel_execution_engine *e, >>> close(fd); >>> } >>> >>> +static int __get_reset_stats(int fd, struct local_drm_i915_reset_stats *rs) >>> +{ >>> + return drmIoctl(fd, GET_RESET_STATS_IOCTL, &rs); >>> +} >>> + >>> static int _test_params(int fd, int ctx, uint32_t flags, uint32_t pad) >>> { >>> struct local_drm_i915_reset_stats rs; >>> @@ -644,10 +646,16 @@ static void _check_param_ctx(const int fd, const int ctx, const cap_t cap) >>> const uint32_t bad = rand() + 1; >>> >>> if (ctx == 0) { >>> - if (cap == root) >>> igt_assert_eq(_test_params(fd, ctx, 0, 0), 0); >> >> Spurious indenting leftover. >> >>> - else >>> - igt_assert_eq(_test_params(fd, ctx, 0, 0), -EPERM); >>> + if (cap != root) { >> >> So what are you expecting to happen if you do happen to be rot? Is this >> test redundant, which is why you skipped it? > > Yes, I think it is redundant because the only expectation for root is > for the IOCTL to be successful as it is for non root users (that is why > I left the first assert to be run unconditionally), and, even if root is > supposed to get the correct reset_count value, unless I am missing > something, that test is not in the scope of this subtest. > > -Antonio > >> -Chris >> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx >
On 08/12/17 09:07, Antonio Argenziano wrote: > > > On 08/12/17 08:46, Chris Wilson wrote: >> Quoting Antonio Argenziano (2017-12-08 16:27:17) >>> The test expected IOCTL 'I915_GET_RESET_STATS' would return an error >>> when not root. That is no longer true in the driver and therefore >>> the test was incorrectly failing. >>> >>> Cc: Michel Thierry <michel.thierry@intel.com> >>> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> >>> Signed-off-by: Antonio Argenziano <antonio.argenziano@intel.com> >>> --- >>> tests/gem_reset_stats.c | 22 +++++++++++++++------- >>> 1 file changed, 15 insertions(+), 7 deletions(-) >>> >>> diff --git a/tests/gem_reset_stats.c b/tests/gem_reset_stats.c >>> index edc40767..83c91f0f 100644 >>> --- a/tests/gem_reset_stats.c >>> +++ b/tests/gem_reset_stats.c >>> @@ -605,10 +605,7 @@ static void test_reset_count(const struct >>> intel_execution_engine *e, >>> c2 = get_reset_count(fd, ctx); >>> - if (ctx == 0) >>> - igt_assert(c2 == -EPERM); >>> - else >>> - igt_assert(c2 == 0); >>> + igt_assert(c2 == 0); >>> } >>> igt_waitchildren(); >>> @@ -619,6 +616,11 @@ static void test_reset_count(const struct >>> intel_execution_engine *e, >>> close(fd); >>> } >>> +static int __get_reset_stats(int fd, struct >>> local_drm_i915_reset_stats *rs) >>> +{ >>> + return drmIoctl(fd, GET_RESET_STATS_IOCTL, &rs); >>> +} >>> + >>> static int _test_params(int fd, int ctx, uint32_t flags, uint32_t pad) >>> { >>> struct local_drm_i915_reset_stats rs; >>> @@ -644,10 +646,16 @@ static void _check_param_ctx(const int fd, >>> const int ctx, const cap_t cap) >>> const uint32_t bad = rand() + 1; >>> if (ctx == 0) { >>> - if (cap == root) >>> igt_assert_eq(_test_params(fd, ctx, 0, 0), 0); >> >> Spurious indenting leftover. >> >>> - else >>> - igt_assert_eq(_test_params(fd, ctx, 0, 0), >>> -EPERM); >>> + if (cap != root) { >> >> So what are you expecting to happen if you do happen to be rot? Is this >> test redundant, which is why you skipped it? > > Yes, I think it is redundant because the only expectation for root is > for the IOCTL to be successful as it is for non root users (that is why > I left the first assert to be run unconditionally), and, even if root is > supposed to get the correct reset_count value, unless I am missing After looking at this again I disagree with myself :). I (now) think that if the interface doesn't allow a non privileged user to access some information it should return an error (EPERM) instead of returning a returning a fixed value after what looks like a a successful IOCTL, it might be incorrectly interpreted as the real thing while it should have been discarded. What do you think? BTW, why are non-root users not allowed to read reset_stats? > something, that test is not in the scope of this subtest. > > -Antonio > >> -Chris >> > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/tests/gem_reset_stats.c b/tests/gem_reset_stats.c index edc40767..83c91f0f 100644 --- a/tests/gem_reset_stats.c +++ b/tests/gem_reset_stats.c @@ -605,10 +605,7 @@ static void test_reset_count(const struct intel_execution_engine *e, c2 = get_reset_count(fd, ctx); - if (ctx == 0) - igt_assert(c2 == -EPERM); - else - igt_assert(c2 == 0); + igt_assert(c2 == 0); } igt_waitchildren(); @@ -619,6 +616,11 @@ static void test_reset_count(const struct intel_execution_engine *e, close(fd); } +static int __get_reset_stats(int fd, struct local_drm_i915_reset_stats *rs) +{ + return drmIoctl(fd, GET_RESET_STATS_IOCTL, &rs); +} + static int _test_params(int fd, int ctx, uint32_t flags, uint32_t pad) { struct local_drm_i915_reset_stats rs; @@ -644,10 +646,16 @@ static void _check_param_ctx(const int fd, const int ctx, const cap_t cap) const uint32_t bad = rand() + 1; if (ctx == 0) { - if (cap == root) igt_assert_eq(_test_params(fd, ctx, 0, 0), 0); - else - igt_assert_eq(_test_params(fd, ctx, 0, 0), -EPERM); + if (cap != root) { + struct local_drm_i915_reset_stats rs; + rs.ctx_id = ctx; + rs.reset_count = rand(); + rs.batch_active = rand(); + rs.batch_pending = rand(); + igt_assert(__get_reset_stats(fd, &rs)); + igt_assert(rs.reset_count != 0); + } } igt_assert_eq(_test_params(fd, ctx, 0, bad), -EINVAL);
The test expected IOCTL 'I915_GET_RESET_STATS' would return an error when not root. That is no longer true in the driver and therefore the test was incorrectly failing. Cc: Michel Thierry <michel.thierry@intel.com> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com> Signed-off-by: Antonio Argenziano <antonio.argenziano@intel.com> --- tests/gem_reset_stats.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-)