Re: [PATCH i-g-t] tests/intel/xe_oa: Call get_stream_status only on EIO

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Thu, 17 Apr 2025 12:00:39 -0700, Umesh Nerlige Ramappa wrote:
>
> On Wed, Apr 16, 2025 at 04:16:40PM -0700, Dixit, Ashutosh wrote:
> > On Wed, 16 Apr 2025 16:12:02 -0700, Umesh Nerlige Ramappa wrote:
> >>
> >> Turns out that get_stream_status resets errno and anything that checks
> >> errno afterwards fails. This resulted in a bunch of test failures. Fix
> >> it by calling get_stream_status only if errno is EIO and also save and
> >> restore errno within get_stream_status.
> >>
> >> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/issues/4804
> >> Signed-off-by: Umesh Nerlige Ramappa <umesh.nerlige.ramappa@intel.com>
> >> ---
> >>  tests/intel/xe_oa.c | 20 ++++++++++++++------
> >>  1 file changed, 14 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/tests/intel/xe_oa.c b/tests/intel/xe_oa.c
> >> index 1fc8bfaafee2..360dd8ba00d9 100644
> >> --- a/tests/intel/xe_oa.c
> >> +++ b/tests/intel/xe_oa.c
> >> @@ -380,9 +380,11 @@ static void set_fd_flags(int fd, int flags)
> >>  static u32 get_stream_status(int fd)
> >>  {
> >>	struct drm_xe_oa_stream_status status;
> >> +	int _e = errno;
> >>
> >>	do_ioctl(fd, DRM_XE_OBSERVATION_IOCTL_STATUS, &status);
> >>	igt_debug("oa status %llx\n", status.oa_status);
> >> +	errno = _e;
> >>
> >>	return status.oa_status;
> >>  }
> >> @@ -2294,7 +2296,8 @@ static void test_polling_small_buf(void)
> >>	errno = 0;
> >>	ret = read(stream_fd, buf, sizeof(buf));
> >>	igt_assert_eq(ret, -1);
> >> -	get_stream_status(stream_fd);
> >> +	if (errno == EIO)
> >> +		get_stream_status(stream_fd);
> >
> > Hi Umesh, if we are saving/restoring errno in get_stream_status(), do we
> > still need to check for EIO? Or you just want to check for EIO anyhow?
>
> I wanted to make sure of both actually. Ideally, we get the stream status
> only on EIO. In future if anyone does not follow that and just calls the
> get_stream_status, then preserve the errno to avoid strange issues. Maybe
> should update the commit then?

By "commit" you mean this patch? Yes I think the original intent of
'db9eca9df70f ("tests/intel/xe_oa: Get stream status on read error")' was
to get the stream status unconditionally, so maybe stick to that?

Thanks.
--
Ashutosh