[PATCH i-g-t] lib/igt_kmod: Unload/Reload the mei Modules Before Unloading/Reloading the i915/xe Driver

[Lucas De Marchi]

On Thu, Aug 15, 2024 at 06:34:23PM GMT, Bommu Krishnaiah wrote:
>To ensure stability and proper module management, this update introduces
>changes to the handling of MEI modules when loading or unloading
>the Intel i915 or Xe drivers.
>
>Key Changes:
>- Unload Order: The `mei_gsc_proxy` module is now unloaded before `mei_gsc`,
>preventing potential failures during the unloading process.
>- Platform-Specific Handling: On platforms where the MEI hardware is integrated
>with the graphics device (e.g., DG2/BMG), the MEI modules depend on the i915/Xe driver.
>Conversely, on newer platforms like CLS, where MEI hardware is separate,
>this dependency does not exist. This update ensures that MEI modules are
>unloaded/reloaded in the correct order based on platform-specific dependencies.
>
>These changes address the need for a more robust handling of MEI modules across
>different hardware platforms, ensuring that the i915/Xe driver can be
>cleanly unloaded and reloaded without issues.
>
>Signed-off-by: Bommu Krishnaiah <krishnaiah.bommu at intel.com>
>Cc: Kamil Konieczny <kamil.konieczny at linux.intel.com>
>Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
>---
> lib/igt_kmod.c | 38 ++++++++++++++++++++++++++++++++++++++
> 1 file changed, 38 insertions(+)
>
>diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
>index 464c0dcf4..6ee2b7afe 100644
>--- a/lib/igt_kmod.c
>+++ b/lib/igt_kmod.c
>@@ -480,9 +480,29 @@ igt_intel_driver_load(const char *opts, const char *driver)
> {
> 	int ret;
>
>+	const char *mei[] = {
>+		/* mei_gsc uses an i915 aux dev and the other mei mods depend on it */
>+		"mei_pxp",
>+		"mei_hdcp",
>+		"mei_gsc_proxy",
>+		"mei_gsc",
>+		NULL,

all this manual approach we have for module unloading should be gone
IMO. Replace that with 1) unbind the driver; 2) unload the module.
There's no need afaict to keep tracking what the module uses.

as for loading, why exactly do we have to do the kernels/depmod
manually?  If we have to do anything manually like that, it means
real users won't have that and we are actually missing the proper
integration. I typed something on the different types of
dependencies (harddep, softdep, weakdep):
https://politreco.com/2024/08/linux-module-dependencies/

Lucas De Marchi

>+	};
>+
> 	if (opts)
> 		igt_info("Reloading %s with %s\n\n", driver, opts);
>
>+	for (const char **m = mei; *m; m++) {
>+		if (igt_kmod_is_loaded(*m))
>+			continue;
>+
>+		ret = igt_kmod_load(*m, NULL);
>+		if (ret) {
>+			igt_debug("Could not load %s\n", *m);
>+			return ret;
>+		}
>+	}
>+
> 	ret = igt_kmod_load(driver, opts);
> 	if (ret) {
> 		igt_debug("Could not load %s\n", driver);
>@@ -620,9 +640,14 @@ int __igt_intel_driver_unload(char **who, const char *driver)
> 	const char *aux[] = {
> 		/* gen5: ips uses symbol_get() so only a soft module dependency */
> 		"intel_ips",
>+		NULL,
>+	};
>+
>+	const char *mei[] = {
> 		/* mei_gsc uses an i915 aux dev and the other mei mods depend on it */
> 		"mei_pxp",
> 		"mei_hdcp",
>+		"mei_gsc_proxy",
> 		"mei_gsc",
> 		NULL,
> 	};
>@@ -647,6 +672,19 @@ int __igt_intel_driver_unload(char **who, const char *driver)
> 		}
> 	}
>
>+	for (const char **m = mei; *m; m++) {
>+		if (!igt_kmod_is_loaded(*m))
>+			continue;
>+
>+		ret = igt_kmod_unload(*m);
>+		if (ret) {
>+			if (who)
>+				*who = strdup_realloc(*who, *m);
>+
>+			return ret;
>+		}
>+	}
>+
> 	if (igt_kmod_is_loaded(driver)) {
> 		ret = igt_kmod_unload(driver);
> 		if (ret) {
>-- 
>2.25.1
>