[PATCH i-g-t v4 3/4] lib/drmtest: invalidate cached fds after unbind

[Lucas De Marchi]

On Tue, Nov 26, 2024 at 07:10:22PM +0100, Kamil Konieczny wrote:
>After driver unbind or reload cached drm file descriptors could
>become stale, so there is no point in preventing calling
>drm_open_driver() again. This should prevent errors like:
>
>(xe_wedged:3680) drmtest-DEBUG: Condition stat(path, &new) != 0 occurred
>	in function _is_already_opened, file ../lib/drmtest.c:407
>(xe_wedged:3680) drmtest-WARNING: card maching filter 0 is already opened
>
>when test tries to open again driver and make checks if
>it is functional.
>
>Cc: Janusz Krzysztofik <janusz.krzysztofik at linux.intel.com>
>Signed-off-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
>---
> lib/drmtest.c          | 18 ++++++++++++++++++
> lib/drmtest.h          |  2 ++
> lib/igt_sysfs.c        |  1 +
> tests/core_hotunplug.c |  1 +
> 4 files changed, 22 insertions(+)
>
>diff --git a/lib/drmtest.c b/lib/drmtest.c
>index 2dd4540b8..2919a4891 100644
>--- a/lib/drmtest.c
>+++ b/lib/drmtest.c
>@@ -726,6 +726,24 @@ static void cancel_work_at_exit_render(int sig)
> 	at_exit_drm_render_fd = -1;
> }
>
>+/**
>+ * __drm_invalidate_opened_fds:
>+ *
>+ * Invalidate drmtest internal fd caches.
>+ */
>+void __drm_invalidate_opened_fds(void)
>+{
>+	_opened_fds_count = 0;
>+	if (at_exit_drm_fd >= 0)
>+		close(at_exit_drm_fd);
>+
>+	at_exit_drm_fd = -1;
>+	if (at_exit_drm_render_fd >= 0)
>+		close(at_exit_drm_render_fd);
>+
>+	at_exit_drm_render_fd = -1;

humn... this only cleans up the _at_exit fds, leaving the fd/stat intact
in the array. The next drm_open_driver() will still complain that the fd
is already opened.

>+}
>+
> static const char *chipset_to_vendor_str(int chipset)
> {
> 	return chipset == DRIVER_XE ? chipset_to_str(DRIVER_INTEL) : chipset_to_str(chipset);
>diff --git a/lib/drmtest.h b/lib/drmtest.h
>index 27e5a18e2..fb37a1070 100644
>--- a/lib/drmtest.h
>+++ b/lib/drmtest.h
>@@ -129,6 +129,8 @@ int drm_reopen_driver(int fd);
> int drm_prepare_filtered_multigpu(int chipset);
> int drm_open_filtered_card(int idx);
>
>+void __drm_invalidate_opened_fds(void);
>+
> void igt_require_amdgpu(int fd);
> void igt_require_intel(int fd);
> void igt_require_i915(int fd);
>diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
>index 00d5822fd..e903a9857 100644
>--- a/lib/igt_sysfs.c
>+++ b/lib/igt_sysfs.c
>@@ -1448,6 +1448,7 @@ int xe_sysfs_driver_do(int xe_device, char pci_slot[], enum xe_sysfs_driver_acti
>
> 	sysfs = open("/sys/bus/pci/drivers/xe", O_DIRECTORY);
> 	igt_assert(sysfs);
>+	__drm_invalidate_opened_fds();

this doesn't seem to be placed correctly. It shouldn't be done here for
all the operations below and doing so means the at_exit_ logic stops
working.

why don't we pair it with drm_close_driver() and change other places to
make sure they call drm_close_driver() rather than plain close(), like I
did in my patch series?

Also, another possible issue is that __drm_open_device() doesn't use
O_CLOEXEC, so if the test exec it will leak the fd and that will change
the logic of what will be the next card# on a rebind.

Lucas De Marchi