[igt-dev] [PATCH i-g-t] lib/drmtest: Change do_ioctl_err() macro to function

[igt-dev] [PATCH i-g-t] lib/drmtest: Change do_ioctl_err() macro to function

[Zbigniew Kempczyński]

Chris enlightened me after I did some review do_ioctl_err() macro
is a little bit problematic when it catches unexpected error (debug
output isn't much descriptive). As we plan to use it widely in xe
uapi verification lets make error callstack more readable.

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
---
 lib/drmtest.c | 12 ++++++++++++
 lib/drmtest.h | 10 +++-------
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index c91a914257..759e889874 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -681,3 +681,15 @@ void igt_require_xe(int fd)
 {
 	igt_require(is_xe_device(fd));
 }
+
+void do_ioctl_err(int fd, unsigned long ioc, void *ioc_data, int err)
+{
+	int ret;
+
+	ret = igt_ioctl(fd, ioc, ioc_data);
+	igt_assert_f(ret == -1,
+		     "ioctl should fail with -1 but returned %d\n", ret);
+	igt_assert_f(errno == err,
+		     "errno fail - returned: %d, expected: %d\n", errno, err);
+	errno = 0;
+}
diff --git a/lib/drmtest.h b/lib/drmtest.h
index 3c88b85c6f..792996dd28 100644
--- a/lib/drmtest.h
+++ b/lib/drmtest.h
@@ -153,13 +153,9 @@ bool is_intel_device(int fd);
  * @ioc_data: data pointer for the ioctl operation
  * @err: value to expect in errno
  *
- * This macro wraps drmIoctl() and uses igt_assert to check that it fails,
- * returning a particular value in errno.
+ * This function wraps drmIoctl() and uses igt_assert to check that it fails,
+ * verifying expected errno in @err.
  */
-#define do_ioctl_err(fd, ioc, ioc_data, err) do { \
-	igt_assert_eq(igt_ioctl((fd), (ioc), (ioc_data)), -1); \
-	igt_assert_eq(errno, err); \
-	errno = 0; \
-} while (0)
+void do_ioctl_err(int fd, unsigned long ioc, void *ioc_data, int err);
 
 #endif /* DRMTEST_H */
-- 
2.34.1

[igt-dev] ✗ Fi.CI.BAT: failure for lib/drmtest: Change do_ioctl_err() macro to function

[Patchwork]

[-- Attachment #1: Type: text/plain, Size: 5430 bytes --]

== Series Details ==

Series: lib/drmtest: Change do_ioctl_err() macro to function
URL   : https://patchwork.freedesktop.org/series/117106/
State : failure

== Summary ==

CI Bug Log - changes from IGT_7276 -> IGTPW_8888
====================================================

Summary
-------

  **FAILURE**

  Serious unknown changes coming with IGTPW_8888 absolutely need to be
  verified manually.
  
  If you think the reported changes have nothing to do with the changes
  introduced in IGTPW_8888, please notify your bug team to allow them
  to document this new failure mode, which will reduce false positives in CI.

  External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8888/index.html

Participating hosts (38 -> 37)
------------------------------

  Missing    (1): fi-snb-2520m 

Possible new issues
-------------------

  Here are the unknown changes that may have been introduced in IGTPW_8888:

### IGT changes ###

#### Possible regressions ####

  * igt@i915_selftest@live@hangcheck:
    - fi-hsw-4770:        [PASS][1] -> [DMESG-WARN][2]
   [1]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7276/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html
   [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8888/fi-hsw-4770/igt@i915_selftest@live@hangcheck.html

  
Known issues
------------

  Here are the changes found in IGTPW_8888 that come from known issues:

### IGT changes ###

#### Issues hit ####

  * igt@i915_selftest@live@gt_heartbeat:
    - fi-glk-j4005:       [PASS][3] -> [DMESG-FAIL][4] ([i915#5334])
   [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7276/fi-glk-j4005/igt@i915_selftest@live@gt_heartbeat.html
   [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8888/fi-glk-j4005/igt@i915_selftest@live@gt_heartbeat.html

  * igt@i915_selftest@live@migrate:
    - bat-atsm-1:         [PASS][5] -> [DMESG-FAIL][6] ([i915#7699] / [i915#7913])
   [5]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7276/bat-atsm-1/igt@i915_selftest@live@migrate.html
   [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8888/bat-atsm-1/igt@i915_selftest@live@migrate.html

  * igt@i915_selftest@live@reset:
    - bat-rpls-1:         [PASS][7] -> [ABORT][8] ([i915#4983] / [i915#7461] / [i915#8384])
   [7]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7276/bat-rpls-1/igt@i915_selftest@live@reset.html
   [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8888/bat-rpls-1/igt@i915_selftest@live@reset.html

  * igt@i915_suspend@basic-s3-without-i915:
    - fi-tgl-1115g4:      NOTRUN -> [INCOMPLETE][9] ([i915#7443] / [i915#8102])
   [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8888/fi-tgl-1115g4/igt@i915_suspend@basic-s3-without-i915.html

  * igt@kms_chamelium_hpd@common-hpd-after-suspend:
    - bat-dg2-11:         NOTRUN -> [SKIP][10] ([i915#7828])
   [10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8888/bat-dg2-11/igt@kms_chamelium_hpd@common-hpd-after-suspend.html

  
#### Possible fixes ####

  * igt@i915_selftest@live@hangcheck:
    - bat-dg2-11:         [ABORT][11] ([i915#7913] / [i915#7979]) -> [PASS][12]
   [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7276/bat-dg2-11/igt@i915_selftest@live@hangcheck.html
   [12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8888/bat-dg2-11/igt@i915_selftest@live@hangcheck.html

  * igt@i915_selftest@live@migrate:
    - bat-dg2-11:         [DMESG-WARN][13] ([i915#7699]) -> [PASS][14]
   [13]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7276/bat-dg2-11/igt@i915_selftest@live@migrate.html
   [14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8888/bat-dg2-11/igt@i915_selftest@live@migrate.html

  * igt@i915_suspend@basic-s2idle-without-i915:
    - fi-tgl-1115g4:      [ABORT][15] ([i915#8213]) -> [PASS][16]
   [15]: https://intel-gfx-ci.01.org/tree/drm-tip/IGT_7276/fi-tgl-1115g4/igt@i915_suspend@basic-s2idle-without-i915.html
   [16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8888/fi-tgl-1115g4/igt@i915_suspend@basic-s2idle-without-i915.html

  
  {name}: This element is suppressed. This means it is ignored when computing
          the status of the difference (SUCCESS, WARNING, or FAILURE).

  [i915#4983]: https://gitlab.freedesktop.org/drm/intel/issues/4983
  [i915#5334]: https://gitlab.freedesktop.org/drm/intel/issues/5334
  [i915#7443]: https://gitlab.freedesktop.org/drm/intel/issues/7443
  [i915#7461]: https://gitlab.freedesktop.org/drm/intel/issues/7461
  [i915#7699]: https://gitlab.freedesktop.org/drm/intel/issues/7699
  [i915#7828]: https://gitlab.freedesktop.org/drm/intel/issues/7828
  [i915#7913]: https://gitlab.freedesktop.org/drm/intel/issues/7913
  [i915#7920]: https://gitlab.freedesktop.org/drm/intel/issues/7920
  [i915#7979]: https://gitlab.freedesktop.org/drm/intel/issues/7979
  [i915#8102]: https://gitlab.freedesktop.org/drm/intel/issues/8102
  [i915#8213]: https://gitlab.freedesktop.org/drm/intel/issues/8213
  [i915#8384]: https://gitlab.freedesktop.org/drm/intel/issues/8384


Build changes
-------------

  * CI: CI-20190529 -> None
  * IGT: IGT_7276 -> IGTPW_8888

  CI-20190529: 20190529
  CI_DRM_13073: d4602c57ac02d66526e4785b80c2e01dea122f33 @ git://anongit.freedesktop.org/gfx-ci/linux
  IGTPW_8888: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8888/index.html
  IGT_7276: f99f30adaf65f0889c3d06858103e52419761ea6 @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_8888/index.html

[-- Attachment #2: Type: text/html, Size: 6322 bytes --]

[igt-dev] [PATCH i-g-t] lib/drmtest: Change do_ioctl_err() macro to function

[Zbigniew Kempczyński]

Chris enlightened me after I did some review do_ioctl_err() macro
is a little bit problematic when it catches unexpected error (debug
output isn't much descriptive). As we plan to use it widely in xe
uapi verification lets make error callstack more readable.

Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
---
 lib/drmtest.c | 12 ++++++++++++
 lib/drmtest.h | 10 +++-------
 2 files changed, 15 insertions(+), 7 deletions(-)

diff --git a/lib/drmtest.c b/lib/drmtest.c
index c91a914257..759e889874 100644
--- a/lib/drmtest.c
+++ b/lib/drmtest.c
@@ -681,3 +681,15 @@ void igt_require_xe(int fd)
 {
 	igt_require(is_xe_device(fd));
 }
+
+void do_ioctl_err(int fd, unsigned long ioc, void *ioc_data, int err)
+{
+	int ret;
+
+	ret = igt_ioctl(fd, ioc, ioc_data);
+	igt_assert_f(ret == -1,
+		     "ioctl should fail with -1 but returned %d\n", ret);
+	igt_assert_f(errno == err,
+		     "errno fail - returned: %d, expected: %d\n", errno, err);
+	errno = 0;
+}
diff --git a/lib/drmtest.h b/lib/drmtest.h
index 3c88b85c6f..792996dd28 100644
--- a/lib/drmtest.h
+++ b/lib/drmtest.h
@@ -153,13 +153,9 @@ bool is_intel_device(int fd);
  * @ioc_data: data pointer for the ioctl operation
  * @err: value to expect in errno
  *
- * This macro wraps drmIoctl() and uses igt_assert to check that it fails,
- * returning a particular value in errno.
+ * This function wraps drmIoctl() and uses igt_assert to check that it fails,
+ * verifying expected errno in @err.
  */
-#define do_ioctl_err(fd, ioc, ioc_data, err) do { \
-	igt_assert_eq(igt_ioctl((fd), (ioc), (ioc_data)), -1); \
-	igt_assert_eq(errno, err); \
-	errno = 0; \
-} while (0)
+void do_ioctl_err(int fd, unsigned long ioc, void *ioc_data, int err);
 
 #endif /* DRMTEST_H */
-- 
2.34.1

Re: [igt-dev] [PATCH i-g-t] lib/drmtest: Change do_ioctl_err() macro to function

[Kamil Konieczny]

Hi Zbigniew,

On 2023-05-09 at 08:39:21 +0200, Zbigniew Kempczyński wrote:
> Chris enlightened me after I did some review do_ioctl_err() macro
> is a little bit problematic when it catches unexpected error (debug
> output isn't much descriptive). As we plan to use it widely in xe
> uapi verification lets make error callstack more readable.

It is up to you if you would like to have a macro or a function,
you can improve macro to have more descriptive prints.

> 
> Signed-off-by: Zbigniew Kempczyński <zbigniew.kempczynski@intel.com>
> ---
>  lib/drmtest.c | 12 ++++++++++++
>  lib/drmtest.h | 10 +++-------
>  2 files changed, 15 insertions(+), 7 deletions(-)
> 
> diff --git a/lib/drmtest.c b/lib/drmtest.c
> index c91a914257..759e889874 100644
> --- a/lib/drmtest.c
> +++ b/lib/drmtest.c
> @@ -681,3 +681,15 @@ void igt_require_xe(int fd)
>  {
>  	igt_require(is_xe_device(fd));
>  }
> +

Please write here description.

> +void do_ioctl_err(int fd, unsigned long ioc, void *ioc_data, int err)
> +{
> +	int ret;
> +
> +	ret = igt_ioctl(fd, ioc, ioc_data);
> +	igt_assert_f(ret == -1,
> +		     "ioctl should fail with -1 but returned %d\n", ret);
> +	igt_assert_f(errno == err,
> +		     "errno fail - returned: %d, expected: %d\n", errno, err);
--------------------- ^ --- ^
Maybe "ioctl mismatch error:" ?

> +	errno = 0;
> +}
> diff --git a/lib/drmtest.h b/lib/drmtest.h
> index 3c88b85c6f..792996dd28 100644
> --- a/lib/drmtest.h
> +++ b/lib/drmtest.h
> @@ -153,13 +153,9 @@ bool is_intel_device(int fd);
>   * @ioc_data: data pointer for the ioctl operation
>   * @err: value to expect in errno
>   *
> - * This macro wraps drmIoctl() and uses igt_assert to check that it fails,
> - * returning a particular value in errno.
> + * This function wraps drmIoctl() and uses igt_assert to check that it fails,
> + * verifying expected errno in @err.
>   */

If this is a function then descritpion should be moved to .c file

> -#define do_ioctl_err(fd, ioc, ioc_data, err) do { \
> -	igt_assert_eq(igt_ioctl((fd), (ioc), (ioc_data)), -1); \

You can improve macro with temporary var for ret and change
asserts into igt_assert_f.
Second idea would be to have

bool __do_ioctl_err

call it from macro and fail if not true.

Regards,
Kamil

> -	igt_assert_eq(errno, err); \
> -	errno = 0; \
> -} while (0)
> +void do_ioctl_err(int fd, unsigned long ioc, void *ioc_data, int err);
>  
>  #endif /* DRMTEST_H */
> -- 
> 2.34.1
>