* [igt-dev] [RFC, i-g-t, v2] tests/device_reset: Test device sysfs reset
@ 2020-06-29 9:23 Marcin Bernatowicz
2020-06-29 10:02 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/device_reset: Test device sysfs reset (rev2) Patchwork
2020-07-01 8:08 ` [igt-dev] [RFC, i-g-t, v2] tests/device_reset: Test device sysfs reset Janusz Krzysztofik
0 siblings, 2 replies; 4+ messages in thread
From: Marcin Bernatowicz @ 2020-06-29 9:23 UTC (permalink / raw)
To: igt-dev; +Cc: michal.winiarski, rodrigo.vivi, marcin.bernatowicz
Device reset is initiated by writing "1" to reset sysfs file,
which should initiate device FLR if supported by device.
Test scenarios:
1. unbind driver from device, initiate sysfs reset, rebind driver to
device
2. device reset with bound driver
v2: removed unbind-rebind (duplicates core_hotunplug@unbind-rebind)
added healthcheck to each test (Janusz)
Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@intel.com>
---
tests/device_reset.c | 263 +++++++++++++++++++++++++++++++++++++++++++
tests/meson.build | 1 +
2 files changed, 264 insertions(+)
create mode 100644 tests/device_reset.c
diff --git a/tests/device_reset.c b/tests/device_reset.c
new file mode 100644
index 000000000..0e1e9e3aa
--- /dev/null
+++ b/tests/device_reset.c
@@ -0,0 +1,263 @@
+// SPDX-License-Identifier: MIT
+/*
+ * Copyright(c) 2020 Intel Corporation. All rights reserved.
+ */
+
+#include <fcntl.h>
+#include <sys/stat.h>
+#include "igt.h"
+#include "igt_device_scan.h"
+#include "igt_sysfs.h"
+
+IGT_TEST_DESCRIPTION("Examine behavior of a driver on device sysfs reset");
+
+
+#define DEV_PATH_LEN 80
+#define DEV_BUS_ADDR_LEN 13 /* addr has form 0000:00:00.0 */
+
+/**
+ * Helper structure containing sysfs file descriptors
+ * and path related to tested device
+ */
+struct sysfs_fds {
+ struct {
+ int dev;
+ int dev_dir;
+ int drv_dir;
+ } fds;
+ char dev_bus_addr[DEV_BUS_ADDR_LEN];
+};
+
+static int __open_sysfs_dir(int fd, const char* path)
+{
+ int sysfs;
+
+ sysfs = igt_sysfs_open(fd);
+ if (sysfs < 0) {
+ return -1;
+ }
+
+ return openat(sysfs, path, O_DIRECTORY);
+}
+
+static int open_device_sysfs_dir(int fd)
+{
+ return __open_sysfs_dir(fd, "device");
+}
+
+static int open_driver_sysfs_dir(int fd)
+{
+ return __open_sysfs_dir(fd, "device/driver");
+}
+
+/**
+ * device_sysfs_path:
+ * @fd: opened device file descriptor
+ * @path: buffer to store sysfs path to device directory
+ *
+ * Returns:
+ * On successfull path resolution sysfs path to device directory,
+ * NULL otherwise
+ */
+static char *device_sysfs_path(int fd, char *path)
+{
+ char sysfs[DEV_PATH_LEN];
+
+ if (!igt_sysfs_path(fd, sysfs, sizeof(sysfs)))
+ return NULL;
+
+ if (DEV_PATH_LEN <= (strlen(sysfs) + strlen("/device")))
+ return NULL;
+
+ strcat(sysfs, "/device");
+
+ return realpath(sysfs, path);
+}
+
+static void init_sysfs_fds(struct sysfs_fds *sysfs)
+{
+ char dev_path[PATH_MAX];
+ char *addr_pos;
+
+ igt_debug("open device\n");
+ /**
+ * As subtests must be able to close examined devices
+ * completely, don't use drm_open_driver() as it keeps
+ * a device file descriptor open for exit handler use.
+ */
+ sysfs->fds.dev = __drm_open_driver(DRIVER_ANY);
+ igt_assert_fd(sysfs->fds.dev);
+
+ igt_assert(device_sysfs_path(sysfs->fds.dev, dev_path));
+ addr_pos = strrchr(dev_path, '/');
+ snprintf(sysfs->dev_bus_addr, sizeof(sysfs->dev_bus_addr),
+ "%s", (addr_pos ? addr_pos + 1 : ""));
+ igt_assert(sysfs->dev_bus_addr[0]);
+
+ sysfs->fds.dev_dir = open_device_sysfs_dir(sysfs->fds.dev);
+ igt_assert_fd(sysfs->fds.dev_dir);
+
+ sysfs->fds.drv_dir = open_driver_sysfs_dir(sysfs->fds.dev);
+ igt_assert_fd(sysfs->fds.drv_dir);
+}
+
+static int close_if_opened(int *fd)
+{
+ int rc = 0;
+ if (fd && *fd != -1) {
+ rc = close(*fd);
+ *fd = -1;
+ }
+ return rc;
+}
+
+static void cleanup_sysfs_fds(struct sysfs_fds *sysfs)
+{
+ close_if_opened(&sysfs->fds.dev);
+ close_if_opened(&sysfs->fds.dev_dir);
+ close_if_opened(&sysfs->fds.drv_dir);
+ sysfs->dev_bus_addr[0] = '\0';
+}
+
+/**
+ * is_sysfs_reset_supported:
+ * @fd: opened device file descriptor
+ *
+ * Check if device supports reset based on sysfs file presence.
+ *
+ * Returns:
+ * True if device supports reset, false otherwise.
+ */
+static bool is_sysfs_reset_supported(int fd)
+{
+ struct stat st;
+ int rc;
+ int sysfs;
+ int reset_fd = -1;
+
+ sysfs = igt_sysfs_open(fd);
+
+ if (sysfs >= 0) {
+ reset_fd = openat(sysfs, "device/reset", O_WRONLY);
+ close(sysfs);
+ }
+
+ if (reset_fd < 0)
+ return false;
+
+ rc = fstat(reset_fd, &st);
+ close(reset_fd);
+
+ if (rc || !S_ISREG(st.st_mode))
+ return false;
+
+ return true;
+}
+
+/* Unbind the driver from the device */
+static void driver_unbind(struct sysfs_fds *sysfs)
+{
+ igt_debug("unbind the driver from the device\n");
+ igt_assert(igt_sysfs_set(sysfs->fds.drv_dir, "unbind",
+ sysfs->dev_bus_addr));
+}
+
+/* Re-bind the driver to the device */
+static void driver_bind(struct sysfs_fds *sysfs)
+{
+ igt_debug("rebind the driver to the device\n");
+ igt_abort_on_f(!igt_sysfs_set(sysfs->fds.drv_dir, "bind",
+ sysfs->dev_bus_addr), "driver rebind failed");
+}
+
+/* Initiate device reset */
+static void initiate_device_reset(struct sysfs_fds *sysfs)
+{
+ igt_debug("reset device\n");
+ igt_assert(igt_sysfs_set(sysfs->fds.dev_dir, "reset", "1"));
+}
+
+/**
+ * healthcheck:
+ * @sysfs: structure with device descriptor, if descriptor equals -1
+ * the device is reopened
+ */
+static void healthcheck(struct sysfs_fds *sysfs)
+{
+ if (sysfs->fds.dev == -1) {
+ /* refresh device list */
+ igt_devices_scan(true);
+ igt_debug("reopen the device\n");
+ sysfs->fds.dev = __drm_open_driver(DRIVER_ANY);
+ }
+ igt_assert_fd(sysfs->fds.dev);
+
+ if (is_i915_device(sysfs->fds.dev))
+ gem_test_engine(sysfs->fds.dev, ALL_ENGINES);
+}
+
+/**
+ * set_device_filter:
+ *
+ * Sets device filter to ensure subtests always reopen the same device
+ *
+ * @dev_path: path to device under tests
+ */
+static void set_device_filter(const char* dev_path)
+{
+ char filter[strlen("sys:") + strlen(dev_path) + 1];
+
+ snprintf(filter, sizeof(filter), "sys:%s", dev_path);
+ igt_device_filter_free_all();
+ igt_assert_eq(igt_device_filter_add(filter), 1);
+}
+
+static void unbind_reset_rebind(struct sysfs_fds *sysfs)
+{
+ igt_debug("close the device\n");
+ close_if_opened(&sysfs->fds.dev);
+
+ driver_unbind(sysfs);
+
+ initiate_device_reset(sysfs);
+
+ driver_bind(sysfs);
+}
+
+igt_main
+{
+ struct sysfs_fds sysfs;
+
+ igt_fixture {
+ char dev_path[PATH_MAX];
+
+ igt_debug("opening device\n");
+ init_sysfs_fds(&sysfs);
+
+ /* Make sure subtests always reopen the same device */
+ igt_assert(device_sysfs_path(sysfs.fds.dev, dev_path));
+ set_device_filter(dev_path);
+
+ igt_skip_on(!is_sysfs_reset_supported(sysfs.fds.dev));
+
+ igt_set_timeout(60, "device reset tests timed out after 60s");
+ }
+
+ igt_describe("Unbinds driver from device, initiates reset"
+ " then rebinds driver to device");
+ igt_subtest("unbind-reset-rebind") {
+ unbind_reset_rebind(&sysfs);
+ healthcheck(&sysfs);
+ }
+
+ igt_describe("Resets device with bound driver");
+ igt_subtest("reset-bound") {
+ initiate_device_reset(&sysfs);
+ healthcheck(&sysfs);
+ }
+
+ igt_fixture {
+ igt_reset_timeout();
+ cleanup_sysfs_fds(&sysfs);
+ }
+}
diff --git a/tests/meson.build b/tests/meson.build
index 28091794f..b0acdf7c0 100644
--- a/tests/meson.build
+++ b/tests/meson.build
@@ -8,6 +8,7 @@ test_progs = [
'core_setmaster_vs_auth',
'debugfs_test',
'dmabuf',
+ 'device_reset',
'drm_import_export',
'drm_mm',
'drm_read',
--
2.25.1
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply related [flat|nested] 4+ messages in thread
* [igt-dev] ✓ Fi.CI.BAT: success for tests/device_reset: Test device sysfs reset (rev2)
2020-06-29 9:23 [igt-dev] [RFC, i-g-t, v2] tests/device_reset: Test device sysfs reset Marcin Bernatowicz
@ 2020-06-29 10:02 ` Patchwork
2020-07-01 8:08 ` [igt-dev] [RFC, i-g-t, v2] tests/device_reset: Test device sysfs reset Janusz Krzysztofik
1 sibling, 0 replies; 4+ messages in thread
From: Patchwork @ 2020-06-29 10:02 UTC (permalink / raw)
To: Marcin Bernatowicz; +Cc: igt-dev
== Series Details ==
Series: tests/device_reset: Test device sysfs reset (rev2)
URL : https://patchwork.freedesktop.org/series/78849/
State : success
== Summary ==
CI Bug Log - changes from CI_DRM_8671 -> IGTPW_4709
====================================================
Summary
-------
**SUCCESS**
No regressions found.
External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/index.html
Known issues
------------
Here are the changes found in IGTPW_4709 that come from known issues:
### IGT changes ###
#### Issues hit ####
* igt@i915_module_load@reload:
- fi-byt-j1900: [PASS][1] -> [DMESG-WARN][2] ([i915#1982])
[1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8671/fi-byt-j1900/igt@i915_module_load@reload.html
[2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/fi-byt-j1900/igt@i915_module_load@reload.html
- fi-tgl-u2: [PASS][3] -> [DMESG-WARN][4] ([i915#402])
[3]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8671/fi-tgl-u2/igt@i915_module_load@reload.html
[4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/fi-tgl-u2/igt@i915_module_load@reload.html
* igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic:
- fi-bsw-kefka: [PASS][5] -> [DMESG-WARN][6] ([i915#1982])
[5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8671/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
[6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/fi-bsw-kefka/igt@kms_cursor_legacy@basic-busy-flip-before-cursor-atomic.html
* igt@kms_flip@basic-flip-vs-wf_vblank@b-edp1:
- fi-icl-u2: [PASS][7] -> [DMESG-WARN][8] ([i915#1982])
[7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8671/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@b-edp1.html
[8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/fi-icl-u2/igt@kms_flip@basic-flip-vs-wf_vblank@b-edp1.html
* igt@prime_vgem@basic-write:
- fi-tgl-y: [PASS][9] -> [DMESG-WARN][10] ([i915#402]) +1 similar issue
[9]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8671/fi-tgl-y/igt@prime_vgem@basic-write.html
[10]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/fi-tgl-y/igt@prime_vgem@basic-write.html
#### Possible fixes ####
* igt@gem_render_linear_blits@basic:
- fi-tgl-y: [DMESG-WARN][11] ([i915#402]) -> [PASS][12]
[11]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8671/fi-tgl-y/igt@gem_render_linear_blits@basic.html
[12]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/fi-tgl-y/igt@gem_render_linear_blits@basic.html
* igt@i915_pm_rpm@module-reload:
- fi-glk-dsi: [DMESG-WARN][13] ([i915#1982]) -> [PASS][14]
[13]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8671/fi-glk-dsi/igt@i915_pm_rpm@module-reload.html
[14]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/fi-glk-dsi/igt@i915_pm_rpm@module-reload.html
* igt@i915_selftest@live@blt:
- fi-bsw-kefka: [INCOMPLETE][15] ([i915#392]) -> [PASS][16]
[15]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8671/fi-bsw-kefka/igt@i915_selftest@live@blt.html
[16]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/fi-bsw-kefka/igt@i915_selftest@live@blt.html
* igt@kms_cursor_legacy@basic-flip-after-cursor-legacy:
- fi-icl-u2: [DMESG-WARN][17] ([i915#1982]) -> [PASS][18]
[17]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8671/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
[18]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/fi-icl-u2/igt@kms_cursor_legacy@basic-flip-after-cursor-legacy.html
* igt@kms_flip@basic-flip-vs-wf_vblank@a-dsi1:
- {fi-tgl-dsi}: [DMESG-WARN][19] ([i915#1982]) -> [PASS][20]
[19]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8671/fi-tgl-dsi/igt@kms_flip@basic-flip-vs-wf_vblank@a-dsi1.html
[20]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/fi-tgl-dsi/igt@kms_flip@basic-flip-vs-wf_vblank@a-dsi1.html
* igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence:
- fi-tgl-u2: [DMESG-WARN][21] ([i915#402]) -> [PASS][22]
[21]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8671/fi-tgl-u2/igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence.html
[22]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/fi-tgl-u2/igt@kms_pipe_crc_basic@read-crc-pipe-a-frame-sequence.html
#### Warnings ####
* igt@i915_pm_rpm@module-reload:
- fi-kbl-x1275: [SKIP][23] ([fdo#109271]) -> [DMESG-FAIL][24] ([i915#62])
[23]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8671/fi-kbl-x1275/igt@i915_pm_rpm@module-reload.html
[24]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/fi-kbl-x1275/igt@i915_pm_rpm@module-reload.html
* igt@kms_flip@basic-flip-vs-wf_vblank@a-dp1:
- fi-kbl-x1275: [DMESG-WARN][25] ([i915#62] / [i915#92] / [i915#95]) -> [DMESG-WARN][26] ([i915#62] / [i915#92]) +2 similar issues
[25]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8671/fi-kbl-x1275/igt@kms_flip@basic-flip-vs-wf_vblank@a-dp1.html
[26]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/fi-kbl-x1275/igt@kms_flip@basic-flip-vs-wf_vblank@a-dp1.html
* igt@kms_force_connector_basic@force-edid:
- fi-kbl-x1275: [DMESG-WARN][27] ([i915#62] / [i915#92]) -> [DMESG-WARN][28] ([i915#62] / [i915#92] / [i915#95]) +3 similar issues
[27]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_8671/fi-kbl-x1275/igt@kms_force_connector_basic@force-edid.html
[28]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/fi-kbl-x1275/igt@kms_force_connector_basic@force-edid.html
{name}: This element is suppressed. This means it is ignored when computing
the status of the difference (SUCCESS, WARNING, or FAILURE).
[fdo#109271]: https://bugs.freedesktop.org/show_bug.cgi?id=109271
[i915#1982]: https://gitlab.freedesktop.org/drm/intel/issues/1982
[i915#392]: https://gitlab.freedesktop.org/drm/intel/issues/392
[i915#402]: https://gitlab.freedesktop.org/drm/intel/issues/402
[i915#62]: https://gitlab.freedesktop.org/drm/intel/issues/62
[i915#92]: https://gitlab.freedesktop.org/drm/intel/issues/92
[i915#95]: https://gitlab.freedesktop.org/drm/intel/issues/95
Participating hosts (46 -> 39)
------------------------------
Missing (7): fi-ilk-m540 fi-hsw-4200u fi-byt-squawks fi-bsw-cyan fi-ctg-p8600 fi-byt-clapper fi-bdw-samus
Build changes
-------------
* CI: CI-20190529 -> None
* IGT: IGT_5718 -> IGTPW_4709
CI-20190529: 20190529
CI_DRM_8671: d5765fff817aa868cbbb86ff3d3ef58d7e27251d @ git://anongit.freedesktop.org/gfx-ci/linux
IGTPW_4709: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/index.html
IGT_5718: af1ef32bfae90bcdbaf1b5d84c61ff4e04368505 @ git://anongit.freedesktop.org/xorg/app/intel-gpu-tools
== Testlist changes ==
+igt@device_reset@reset-bound
+igt@device_reset@unbind-reset-rebind
== Logs ==
For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_4709/index.html
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [igt-dev] [RFC, i-g-t, v2] tests/device_reset: Test device sysfs reset
2020-06-29 9:23 [igt-dev] [RFC, i-g-t, v2] tests/device_reset: Test device sysfs reset Marcin Bernatowicz
2020-06-29 10:02 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/device_reset: Test device sysfs reset (rev2) Patchwork
@ 2020-07-01 8:08 ` Janusz Krzysztofik
2020-07-03 9:05 ` Bernatowicz, Marcin
1 sibling, 1 reply; 4+ messages in thread
From: Janusz Krzysztofik @ 2020-07-01 8:08 UTC (permalink / raw)
To: Marcin Bernatowicz, igt-dev; +Cc: michal.winiarski, rodrigo.vivi
Hi Marcin,
On Mon, 2020-06-29 at 11:23 +0200, Marcin Bernatowicz wrote:
> Device reset is initiated by writing "1" to reset sysfs file,
> which should initiate device FLR if supported by device.
>
> Test scenarios:
> 1. unbind driver from device, initiate sysfs reset, rebind driver to
> device
> 2. device reset with bound driver
>
> v2: removed unbind-rebind (duplicates core_hotunplug@unbind-rebind)
> added healthcheck to each test (Janusz)
>
> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@intel.com>
> ---
> tests/device_reset.c | 263 +++++++++++++++++++++++++++++++++++++++++++
> tests/meson.build | 1 +
> 2 files changed, 264 insertions(+)
> create mode 100644 tests/device_reset.c
>
> diff --git a/tests/device_reset.c b/tests/device_reset.c
> new file mode 100644
> index 000000000..0e1e9e3aa
> --- /dev/null
> +++ b/tests/device_reset.c
> @@ -0,0 +1,263 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2020 Intel Corporation. All rights reserved.
> + */
> +
> +#include <fcntl.h>
> +#include <sys/stat.h>
> +#include "igt.h"
> +#include "igt_device_scan.h"
> +#include "igt_sysfs.h"
> +
> +IGT_TEST_DESCRIPTION("Examine behavior of a driver on device sysfs reset");
> +
> +
> +#define DEV_PATH_LEN 80
> +#define DEV_BUS_ADDR_LEN 13 /* addr has form 0000:00:00.0 */
> +
> +/**
> + * Helper structure containing sysfs file descriptors
> + * and path related to tested device
> + */
> +struct sysfs_fds {
> + struct {
> + int dev;
NIT: In your code, sysfs.fds.dev doesn't correspond to a sysfs node,
only to a device special file under /dev. Then, 'sysfs_fds' as the
name of the structure type and 'sysfs' as the name of its instance may
be misleading in regard to the fds.dev member.
> + int dev_dir;
> + int drv_dir;
> + } fds;
> + char dev_bus_addr[DEV_BUS_ADDR_LEN];
> +};
> +
> +static int __open_sysfs_dir(int fd, const char* path)
> +{
> + int sysfs;
> +
> + sysfs = igt_sysfs_open(fd);
> + if (sysfs < 0) {
> + return -1;
> + }
> +
> + return openat(sysfs, path, O_DIRECTORY);
Can we take care of closing the 'sysfs' fd before returning?
> +}
> +
> +static int open_device_sysfs_dir(int fd)
> +{
> + return __open_sysfs_dir(fd, "device");
> +}
> +
> +static int open_driver_sysfs_dir(int fd)
> +{
> + return __open_sysfs_dir(fd, "device/driver");
> +}
> +
> +/**
> + * device_sysfs_path:
> + * @fd: opened device file descriptor
> + * @path: buffer to store sysfs path to device directory
> + *
> + * Returns:
> + * On successfull path resolution sysfs path to device directory,
> + * NULL otherwise
> + */
> +static char *device_sysfs_path(int fd, char *path)
> +{
> + char sysfs[DEV_PATH_LEN];
> +
> + if (!igt_sysfs_path(fd, sysfs, sizeof(sysfs)))
> + return NULL;
> +
> + if (DEV_PATH_LEN <= (strlen(sysfs) + strlen("/device")))
> + return NULL;
> +
> + strcat(sysfs, "/device");
> +
> + return realpath(sysfs, path);
> +}
> +
> +static void init_sysfs_fds(struct sysfs_fds *sysfs)
> +{
> + char dev_path[PATH_MAX];
> + char *addr_pos;
> +
> + igt_debug("open device\n");
> + /**
> + * As subtests must be able to close examined devices
> + * completely, don't use drm_open_driver() as it keeps
> + * a device file descriptor open for exit handler use.
> + */
> + sysfs->fds.dev = __drm_open_driver(DRIVER_ANY);
> + igt_assert_fd(sysfs->fds.dev);
> +
> + igt_assert(device_sysfs_path(sysfs->fds.dev, dev_path));
> + addr_pos = strrchr(dev_path, '/');
> + snprintf(sysfs->dev_bus_addr, sizeof(sysfs->dev_bus_addr),
> + "%s", (addr_pos ? addr_pos + 1 : ""));
NIT: Couldn't we assert non-NULL addr_pos before using it instead of
using this conditional expression?
> + igt_assert(sysfs->dev_bus_addr[0]);
NIT: Couldn't we assert correctness of return value of snprintf()
instead?
> +
> + sysfs->fds.dev_dir = open_device_sysfs_dir(sysfs->fds.dev);
> + igt_assert_fd(sysfs->fds.dev_dir);
> +
> + sysfs->fds.drv_dir = open_driver_sysfs_dir(sysfs->fds.dev);
> + igt_assert_fd(sysfs->fds.drv_dir);
> +}
> +
> +static int close_if_opened(int *fd)
> +{
> + int rc = 0;
Could we have declarations separated here from the code with a blank
line?
> + if (fd && *fd != -1) {
> + rc = close(*fd);
> + *fd = -1;
> + }
> + return rc;
> +}
> +
> +static void cleanup_sysfs_fds(struct sysfs_fds *sysfs)
> +{
> + close_if_opened(&sysfs->fds.dev);
> + close_if_opened(&sysfs->fds.dev_dir);
> + close_if_opened(&sysfs->fds.drv_dir);
NIT: There is an igt_ignore_warn() helper which you may want to use for
preventively suppressing potential compiler warnings on ignored return
values of non-void functions.
> + sysfs->dev_bus_addr[0] = '\0';
Is cleanup of dev_bus_addr content really needed?
> +}
> +
> +/**
> + * is_sysfs_reset_supported:
> + * @fd: opened device file descriptor
> + *
> + * Check if device supports reset based on sysfs file presence.
> + *
> + * Returns:
> + * True if device supports reset, false otherwise.
> + */
> +static bool is_sysfs_reset_supported(int fd)
> +{
> + struct stat st;
> + int rc;
> + int sysfs;
> + int reset_fd = -1;
> +
> + sysfs = igt_sysfs_open(fd);
> +
> + if (sysfs >= 0) {
> + reset_fd = openat(sysfs, "device/reset", O_WRONLY);
> + close(sysfs);
> + }
> +
> + if (reset_fd < 0)
> + return false;
> +
> + rc = fstat(reset_fd, &st);
> + close(reset_fd);
> +
> + if (rc || !S_ISREG(st.st_mode))
> + return false;
> +
> + return true;
> +}
> +
> +/* Unbind the driver from the device */
> +static void driver_unbind(struct sysfs_fds *sysfs)
> +{
> + igt_debug("unbind the driver from the device\n");
> + igt_assert(igt_sysfs_set(sysfs->fds.drv_dir, "unbind",
> + sysfs->dev_bus_addr));
> +}
> +
> +/* Re-bind the driver to the device */
> +static void driver_bind(struct sysfs_fds *sysfs)
> +{
> + igt_debug("rebind the driver to the device\n");
> + igt_abort_on_f(!igt_sysfs_set(sysfs->fds.drv_dir, "bind",
> + sysfs->dev_bus_addr), "driver rebind failed");
> +}
> +
> +/* Initiate device reset */
> +static void initiate_device_reset(struct sysfs_fds *sysfs)
> +{
> + igt_debug("reset device\n");
> + igt_assert(igt_sysfs_set(sysfs->fds.dev_dir, "reset", "1"));
> +}
> +
> +/**
> + * healthcheck:
> + * @sysfs: structure with device descriptor, if descriptor equals -1
> + * the device is reopened
> + */
> +static void healthcheck(struct sysfs_fds *sysfs)
> +{
> + if (sysfs->fds.dev == -1) {
> + /* refresh device list */
> + igt_devices_scan(true);
> + igt_debug("reopen the device\n");
> + sysfs->fds.dev = __drm_open_driver(DRIVER_ANY);
> + }
> + igt_assert_fd(sysfs->fds.dev);
> +
> + if (is_i915_device(sysfs->fds.dev))
> + gem_test_engine(sysfs->fds.dev, ALL_ENGINES);
> +}
> +
> +/**
> + * set_device_filter:
> + *
> + * Sets device filter to ensure subtests always reopen the same device
> + *
> + * @dev_path: path to device under tests
> + */
> +static void set_device_filter(const char* dev_path)
> +{
> + char filter[strlen("sys:") + strlen(dev_path) + 1];
Can we avoiding variable-length arrays?
Other than that, LGTM.
Thanks,
Janusz
> +
> + snprintf(filter, sizeof(filter), "sys:%s", dev_path);
> + igt_device_filter_free_all();
> + igt_assert_eq(igt_device_filter_add(filter), 1);
> +}
> +
> +static void unbind_reset_rebind(struct sysfs_fds *sysfs)
> +{
> + igt_debug("close the device\n");
> + close_if_opened(&sysfs->fds.dev);
> +
> + driver_unbind(sysfs);
> +
> + initiate_device_reset(sysfs);
> +
> + driver_bind(sysfs);
> +}
> +
> +igt_main
> +{
> + struct sysfs_fds sysfs;
> +
> + igt_fixture {
> + char dev_path[PATH_MAX];
> +
> + igt_debug("opening device\n");
> + init_sysfs_fds(&sysfs);
> +
> + /* Make sure subtests always reopen the same device */
> + igt_assert(device_sysfs_path(sysfs.fds.dev, dev_path));
> + set_device_filter(dev_path);
> +
> + igt_skip_on(!is_sysfs_reset_supported(sysfs.fds.dev));
> +
> + igt_set_timeout(60, "device reset tests timed out after 60s");
> + }
> +
> + igt_describe("Unbinds driver from device, initiates reset"
> + " then rebinds driver to device");
> + igt_subtest("unbind-reset-rebind") {
> + unbind_reset_rebind(&sysfs);
> + healthcheck(&sysfs);
> + }
> +
> + igt_describe("Resets device with bound driver");
> + igt_subtest("reset-bound") {
> + initiate_device_reset(&sysfs);
> + healthcheck(&sysfs);
> + }
> +
> + igt_fixture {
> + igt_reset_timeout();
> + cleanup_sysfs_fds(&sysfs);
> + }
> +}
> diff --git a/tests/meson.build b/tests/meson.build
> index 28091794f..b0acdf7c0 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -8,6 +8,7 @@ test_progs = [
> 'core_setmaster_vs_auth',
> 'debugfs_test',
> 'dmabuf',
> + 'device_reset',
> 'drm_import_export',
> 'drm_mm',
> 'drm_read',
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 4+ messages in thread
* Re: [igt-dev] [RFC, i-g-t, v2] tests/device_reset: Test device sysfs reset
2020-07-01 8:08 ` [igt-dev] [RFC, i-g-t, v2] tests/device_reset: Test device sysfs reset Janusz Krzysztofik
@ 2020-07-03 9:05 ` Bernatowicz, Marcin
0 siblings, 0 replies; 4+ messages in thread
From: Bernatowicz, Marcin @ 2020-07-03 9:05 UTC (permalink / raw)
To: igt-dev@lists.freedesktop.org, janusz.krzysztofik@linux.intel.com
Cc: Winiarski, Michal, Vivi, Rodrigo
On Wed, 2020-07-01 at 10:08 +0200, Janusz Krzysztofik wrote:
> Hi Marcin,
>
> On Mon, 2020-06-29 at 11:23 +0200, Marcin Bernatowicz wrote:
> > Device reset is initiated by writing "1" to reset sysfs file,
> > which should initiate device FLR if supported by device.
> >
> > Test scenarios:
> > 1. unbind driver from device, initiate sysfs reset, rebind driver
> > to
> > device
> > 2. device reset with bound driver
> >
> > v2: removed unbind-rebind (duplicates core_hotunplug@unbind-rebind)
> > added healthcheck to each test (Janusz)
> >
> > Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@intel.com>
> > ---
> > tests/device_reset.c | 263
> > +++++++++++++++++++++++++++++++++++++++++++
> > tests/meson.build | 1 +
> > 2 files changed, 264 insertions(+)
> > create mode 100644 tests/device_reset.c
> >
> > diff --git a/tests/device_reset.c b/tests/device_reset.c
> > new file mode 100644
> > index 000000000..0e1e9e3aa
> > --- /dev/null
> > +++ b/tests/device_reset.c
> > @@ -0,0 +1,263 @@
> > +// SPDX-License-Identifier: MIT
> > +/*
> > + * Copyright(c) 2020 Intel Corporation. All rights reserved.
> > + */
> > +
> > +#include <fcntl.h>
> > +#include <sys/stat.h>
> > +#include "igt.h"
> > +#include "igt_device_scan.h"
> > +#include "igt_sysfs.h"
> > +
> > +IGT_TEST_DESCRIPTION("Examine behavior of a driver on device sysfs
> > reset");
> > +
> > +
> > +#define DEV_PATH_LEN 80
> > +#define DEV_BUS_ADDR_LEN 13 /* addr has form 0000:00:00.0 */
> > +
> > +/**
> > + * Helper structure containing sysfs file descriptors
> > + * and path related to tested device
> > + */
> > +struct sysfs_fds {
> > + struct {
> > + int dev;
>
> NIT: In your code, sysfs.fds.dev doesn't correspond to a sysfs node,
> only to a device special file under /dev. Then, 'sysfs_fds' as the
> name of the structure type and 'sysfs' as the name of its instance
> may
> be misleading in regard to the fds.dev member.
renamed to device_fds
>
> > + int dev_dir;
> > + int drv_dir;
> > + } fds;
> > + char dev_bus_addr[DEV_BUS_ADDR_LEN];
> > +};
> > +
> > +static int __open_sysfs_dir(int fd, const char* path)
> > +{
> > + int sysfs;
> > +
> > + sysfs = igt_sysfs_open(fd);
> > + if (sysfs < 0) {
> > + return -1;
> > + }
> > +
> > + return openat(sysfs, path, O_DIRECTORY);
>
> Can we take care of closing the 'sysfs' fd before returning?
thanks, corrected in rev3
>
> > +}
> > +
> > +static int open_device_sysfs_dir(int fd)
> > +{
> > + return __open_sysfs_dir(fd, "device");
> > +}
> > +
> > +static int open_driver_sysfs_dir(int fd)
> > +{
> > + return __open_sysfs_dir(fd, "device/driver");
> > +}
> > +
> > +/**
> > + * device_sysfs_path:
> > + * @fd: opened device file descriptor
> > + * @path: buffer to store sysfs path to device directory
> > + *
> > + * Returns:
> > + * On successfull path resolution sysfs path to device directory,
> > + * NULL otherwise
> > + */
> > +static char *device_sysfs_path(int fd, char *path)
> > +{
> > + char sysfs[DEV_PATH_LEN];
> > +
> > + if (!igt_sysfs_path(fd, sysfs, sizeof(sysfs)))
> > + return NULL;
> > +
> > + if (DEV_PATH_LEN <= (strlen(sysfs) + strlen("/device")))
> > + return NULL;
> > +
> > + strcat(sysfs, "/device");
> > +
> > + return realpath(sysfs, path);
> > +}
> > +
> > +static void init_sysfs_fds(struct sysfs_fds *sysfs)
> > +{
> > + char dev_path[PATH_MAX];
> > + char *addr_pos;
> > +
> > + igt_debug("open device\n");
> > + /**
> > + * As subtests must be able to close examined devices
> > + * completely, don't use drm_open_driver() as it keeps
> > + * a device file descriptor open for exit handler use.
> > + */
> > + sysfs->fds.dev = __drm_open_driver(DRIVER_ANY);
> > + igt_assert_fd(sysfs->fds.dev);
> > +
> > + igt_assert(device_sysfs_path(sysfs->fds.dev, dev_path));
> > + addr_pos = strrchr(dev_path, '/');
> > + snprintf(sysfs->dev_bus_addr, sizeof(sysfs->dev_bus_addr),
> > + "%s", (addr_pos ? addr_pos + 1 : ""));
>
> NIT: Couldn't we assert non-NULL addr_pos before using it instead of
> using this conditional expression?
> > + igt_assert(sysfs->dev_bus_addr[0]);
>
> NIT: Couldn't we assert correctness of return value of snprintf()
> instead?
> > +
> > + sysfs->fds.dev_dir = open_device_sysfs_dir(sysfs->fds.dev);
> > + igt_assert_fd(sysfs->fds.dev_dir);
> > +
> > + sysfs->fds.drv_dir = open_driver_sysfs_dir(sysfs->fds.dev);
> > + igt_assert_fd(sysfs->fds.drv_dir);
> > +}
> > +
> > +static int close_if_opened(int *fd)
> > +{
> > + int rc = 0;
>
> Could we have declarations separated here from the code with a blank
> line?
>
> > + if (fd && *fd != -1) {
> > + rc = close(*fd);
> > + *fd = -1;
> > + }
> > + return rc;
> > +}
> > +
> > +static void cleanup_sysfs_fds(struct sysfs_fds *sysfs)
> > +{
> > + close_if_opened(&sysfs->fds.dev);
> > + close_if_opened(&sysfs->fds.dev_dir);
> > + close_if_opened(&sysfs->fds.drv_dir);
>
> NIT: There is an igt_ignore_warn() helper which you may want to use
> for
> preventively suppressing potential compiler warnings on ignored
> return
> values of non-void functions.
>
> > + sysfs->dev_bus_addr[0] = '\0';
>
> Is cleanup of dev_bus_addr content really needed?
>
> > +}
> > +
> > +/**
> > + * is_sysfs_reset_supported:
> > + * @fd: opened device file descriptor
> > + *
> > + * Check if device supports reset based on sysfs file presence.
> > + *
> > + * Returns:
> > + * True if device supports reset, false otherwise.
> > + */
> > +static bool is_sysfs_reset_supported(int fd)
> > +{
> > + struct stat st;
> > + int rc;
> > + int sysfs;
> > + int reset_fd = -1;
> > +
> > + sysfs = igt_sysfs_open(fd);
> > +
> > + if (sysfs >= 0) {
> > + reset_fd = openat(sysfs, "device/reset", O_WRONLY);
> > + close(sysfs);
> > + }
> > +
> > + if (reset_fd < 0)
> > + return false;
> > +
> > + rc = fstat(reset_fd, &st);
> > + close(reset_fd);
> > +
> > + if (rc || !S_ISREG(st.st_mode))
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > +/* Unbind the driver from the device */
> > +static void driver_unbind(struct sysfs_fds *sysfs)
> > +{
> > + igt_debug("unbind the driver from the device\n");
> > + igt_assert(igt_sysfs_set(sysfs->fds.drv_dir, "unbind",
> > + sysfs->dev_bus_addr));
> > +}
> > +
> > +/* Re-bind the driver to the device */
> > +static void driver_bind(struct sysfs_fds *sysfs)
> > +{
> > + igt_debug("rebind the driver to the device\n");
> > + igt_abort_on_f(!igt_sysfs_set(sysfs->fds.drv_dir, "bind",
> > + sysfs->dev_bus_addr), "driver rebind failed");
> > +}
> > +
> > +/* Initiate device reset */
> > +static void initiate_device_reset(struct sysfs_fds *sysfs)
> > +{
> > + igt_debug("reset device\n");
> > + igt_assert(igt_sysfs_set(sysfs->fds.dev_dir, "reset", "1"));
> > +}
> > +
> > +/**
> > + * healthcheck:
> > + * @sysfs: structure with device descriptor, if descriptor equals
> > -1
> > + * the device is reopened
> > + */
> > +static void healthcheck(struct sysfs_fds *sysfs)
> > +{
> > + if (sysfs->fds.dev == -1) {
> > + /* refresh device list */
> > + igt_devices_scan(true);
> > + igt_debug("reopen the device\n");
> > + sysfs->fds.dev = __drm_open_driver(DRIVER_ANY);
> > + }
> > + igt_assert_fd(sysfs->fds.dev);
> > +
> > + if (is_i915_device(sysfs->fds.dev))
> > + gem_test_engine(sysfs->fds.dev, ALL_ENGINES);
> > +}
> > +
> > +/**
> > + * set_device_filter:
> > + *
> > + * Sets device filter to ensure subtests always reopen the same
> > device
> > + *
> > + * @dev_path: path to device under tests
> > + */
> > +static void set_device_filter(const char* dev_path)
> > +{
> > + char filter[strlen("sys:") + strlen(dev_path) + 1];
>
> Can we avoiding variable-length arrays?
>
> Other than that, LGTM.
>
> Thanks,
> Janusz
>
I've corrected the issues and send updated version
Thanks,
Marcin
>
> > +
> > + snprintf(filter, sizeof(filter), "sys:%s", dev_path);
> > + igt_device_filter_free_all();
> > + igt_assert_eq(igt_device_filter_add(filter), 1);
> > +}
> > +
> > +static void unbind_reset_rebind(struct sysfs_fds *sysfs)
> > +{
> > + igt_debug("close the device\n");
> > + close_if_opened(&sysfs->fds.dev);
> > +
> > + driver_unbind(sysfs);
> > +
> > + initiate_device_reset(sysfs);
> > +
> > + driver_bind(sysfs);
> > +}
> > +
> > +igt_main
> > +{
> > + struct sysfs_fds sysfs;
> > +
> > + igt_fixture {
> > + char dev_path[PATH_MAX];
> > +
> > + igt_debug("opening device\n");
> > + init_sysfs_fds(&sysfs);
> > +
> > + /* Make sure subtests always reopen the same device */
> > + igt_assert(device_sysfs_path(sysfs.fds.dev, dev_path));
> > + set_device_filter(dev_path);
> > +
> > + igt_skip_on(!is_sysfs_reset_supported(sysfs.fds.dev));
> > +
> > + igt_set_timeout(60, "device reset tests timed out after
> > 60s");
> > + }
> > +
> > + igt_describe("Unbinds driver from device, initiates reset"
> > + " then rebinds driver to device");
> > + igt_subtest("unbind-reset-rebind") {
> > + unbind_reset_rebind(&sysfs);
> > + healthcheck(&sysfs);
> > + }
> > +
> > + igt_describe("Resets device with bound driver");
> > + igt_subtest("reset-bound") {
> > + initiate_device_reset(&sysfs);
> > + healthcheck(&sysfs);
> > + }
> > +
> > + igt_fixture {
> > + igt_reset_timeout();
> > + cleanup_sysfs_fds(&sysfs);
> > + }
> > +}
> > diff --git a/tests/meson.build b/tests/meson.build
> > index 28091794f..b0acdf7c0 100644
> > --- a/tests/meson.build
> > +++ b/tests/meson.build
> > @@ -8,6 +8,7 @@ test_progs = [
> > 'core_setmaster_vs_auth',
> > 'debugfs_test',
> > 'dmabuf',
> > + 'device_reset',
> > 'drm_import_export',
> > 'drm_mm',
> > 'drm_read',
>
>
---------------------------------------------------------------------
Intel Technology Poland sp. z o.o.
ul. Sowackiego 173 | 80-298 Gdask | Sd Rejonowy Gdask Pnoc | VII Wydzia Gospodarczy Krajowego Rejestru Sdowego - KRS 101882 | NIP 957-07-52-316 | Kapita zakadowy 200.000 PLN.
Ta wiadomo wraz z zacznikami jest przeznaczona dla okrelonego adresata i moe zawiera informacje poufne. W razie przypadkowego otrzymania tej wiadomoci, prosimy o powiadomienie nadawcy oraz trwae jej usunicie; jakiekolwiek przegldanie lub rozpowszechnianie jest zabronione.
This e-mail and any attachments may contain confidential material for the sole use of the intended recipient(s). If you are not the intended recipient, please contact the sender and delete all copies; any review or distribution by others is strictly prohibited.
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev
^ permalink raw reply [flat|nested] 4+ messages in thread
end of thread, other threads:[~2020-07-03 9:05 UTC | newest]
Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2020-06-29 9:23 [igt-dev] [RFC, i-g-t, v2] tests/device_reset: Test device sysfs reset Marcin Bernatowicz
2020-06-29 10:02 ` [igt-dev] ✓ Fi.CI.BAT: success for tests/device_reset: Test device sysfs reset (rev2) Patchwork
2020-07-01 8:08 ` [igt-dev] [RFC, i-g-t, v2] tests/device_reset: Test device sysfs reset Janusz Krzysztofik
2020-07-03 9:05 ` Bernatowicz, Marcin
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox