Re: [PATCH v2 i-g-t 2/2] lib/igt_kmod: Add IGT_HOOK_POST_GPUKMOD_UNBIND

[Gustavo Sousa <gustavo.sousa@intel.com>]

Quoting Peter Senna Tschudin (2025-07-13 08:23:54-03:00)
>Adds an igt_hook after the unbind operation of the GPU kmod intended as
>a strategice entry point for kmemleak scans.
>
>Can be used with --hook  'post-gpukmod-unbind:...'
>
>Cc: Gustavo Souza <gustavo.souza@linux.intel.com>

This email address also does not exist (I think). :-)

Also,

s/Souza/Sousa/

To be fair, we have both names in Brazil. Mine is Sousa.

>Signed-off-by: Peter Senna Tschudin <peter.senna@linux.intel.com>
>---
>v2:
> - Added code ducumentation for new hook
> - Added env var IGT_HOOK_KMOD_UNBIND_MODULE_NAME and extended help text
>
> lib/igt_hook.c | 10 ++++++++++
> lib/igt_hook.h |  3 +++
> lib/igt_kmod.c |  8 ++++++++
> 3 files changed, 21 insertions(+)
>
>diff --git a/lib/igt_hook.c b/lib/igt_hook.c
>index 8932d118d..a5a55a659 100644
>--- a/lib/igt_hook.c
>+++ b/lib/igt_hook.c
>@@ -74,6 +74,8 @@ static const char *igt_hook_evt_type_to_name(enum igt_hook_evt_type evt_type)
>                 return "post-subtest";
>         case IGT_HOOK_POST_TEST:
>                 return "post-test";
>+        case IGT_HOOK_POST_GPUKMOD_UNBIND:
>+                return "post-gpukmod-unbind";
>         case IGT_HOOK_NUM_EVENTS:
>                 break;
>         /* No "default:" case, to force a warning from -Wswitch in case we miss
>@@ -335,6 +337,7 @@ static void igt_hook_update_env_vars(struct igt_hook *igt_hook, struct igt_hook_
>         setenv("IGT_HOOK_TEST", igt_hook->test_name, 1);
>         setenv("IGT_HOOK_SUBTEST", igt_hook->subtest_name, 1);
>         setenv("IGT_HOOK_DYN_SUBTEST", igt_hook->dyn_subtest_name, 1);
>+        setenv("IGT_HOOK_KMOD_UNBIND_MODULE_NAME", evt->target_name ?: "", 1);

I think it only makes sense to set this when evt->evt_type is
IGT_HOOK_POST_GPUKMOD_UNBIND. So, we could probably do something like:

    setenv("IGT_HOOK_KMOD_UNBIND_MODULE_NAME",
           evt->evt_type == IGT_HOOK_POST_GPUKMOD_UNBIND ? evt->target_name : "", 1);


>         setenv("IGT_HOOK_RESULT", evt->result ?: "", 1);
> }
> 
>@@ -470,6 +473,9 @@ below:\n\
>                 case IGT_HOOK_POST_TEST:
>                         desc = "Occurs after a test case has finished.";
>                         break;
>+                case IGT_HOOK_POST_GPUKMOD_UNBIND:
>+                        desc = "Occurs after the GPU kernel module is unbound from the device.";

It appears igt_kmod_unbind() is also called for "snd_hda_intel", meaning
the that hook could be potentially called for a non-gpu module. So the
hook type and description lacks a bit of precision.

I think we should either condition the call igt_hook_event_notify() for
GPU-only modules or make the hook type use a more generic name and
description (e.g. IGT_HOOK_POST_KMOD_UNBIND).

--
Gustavo Sousa

>+                        break;
>                 default:
>                         desc = "MISSING DESCRIPTION";
>                 }
>@@ -504,6 +510,10 @@ available to the command:\n\
>   values are: SUCCESS, SKIP or FAIL. This is only applicable on \"post-*\"\n\
>   events and will be the empty string for other types of events.\n\
> \n\
>+  IGT_HOOK_KMOD_UNBIND_MODULE_NAME\n\
>+  Name of the GPU kernel module that was unbound from the device. This is only\n\
>+  applicable on the `IGT_HOOK_POST_GPUKMOD_UNBIND` event and will be the empty\n\
>+  string for other types of events.\n\
> \n\
> Note that %s can be passed multiple times. Each descriptor is evaluated in turn\n\
> when matching events and running hook commands.\n\
>diff --git a/lib/igt_hook.h b/lib/igt_hook.h
>index e9f97b79b..d80a012f4 100644
>--- a/lib/igt_hook.h
>+++ b/lib/igt_hook.h
>@@ -26,6 +26,8 @@ struct igt_hook;
>  * @IGT_HOOK_POST_SUBTEST: Occurs after the execution of a subtest..
>  * @IGT_HOOK_POST_TEST: Occurs after a test case (executable) is finished with
>  * the test code.
>+ * @IGT_HOOK_POST_GPUKMOD_UNBIND: Occurs after the GPU kernel module is
>+ * unbound from the device.
>  * @IGT_HOOK_NUM_EVENTS: This is not really an event and represents the number
>  * of possible events tracked by igt_hook.
>  *
>@@ -39,6 +41,7 @@ enum igt_hook_evt_type {
>         IGT_HOOK_POST_DYN_SUBTEST,
>         IGT_HOOK_POST_SUBTEST,
>         IGT_HOOK_POST_TEST,
>+        IGT_HOOK_POST_GPUKMOD_UNBIND,
>         IGT_HOOK_NUM_EVENTS /* This must always be the last one. */
> };
> 
>diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
>index 6d7f5705c..9b95a2288 100644
>--- a/lib/igt_kmod.c
>+++ b/lib/igt_kmod.c
>@@ -41,6 +41,7 @@
> #include "igt_aux.h"
> #include "igt_core.h"
> #include "igt_debugfs.h"
>+#include "igt_hook.h"
> #include "igt_kmod.h"
> #include "igt_ktap.h"
> #include "igt_sysfs.h"
>@@ -604,6 +605,7 @@ int __igt_intel_driver_unload(char **who, const char *driver)
>  */
> int igt_kmod_unbind(const char *mod_name, const char *pci_device)
> {
>+        struct igt_hook *igt_hook = NULL;
>         char path[PATH_MAX];
>         struct dirent *de;
>         int dirlen;
>@@ -634,6 +636,12 @@ int igt_kmod_unbind(const char *mod_name, const char *pci_device)
> 
>         closedir(dir);
> 
>+        igt_hook = igt_core_get_igt_hook();
>+        igt_hook_event_notify(igt_hook, &(struct igt_hook_evt){
>+                .evt_type = IGT_HOOK_POST_GPUKMOD_UNBIND,
>+                .target_name = mod_name,
>+        });
>+
>         return 0;
> }
> 
>-- 
>2.43.0
>