[PATCH i-g-t] lib/igt_kmod: Rewrite xe unload logic

[Cavitt, Jonathan]

-----Original Message-----
From: igt-dev <igt-dev-bounces at lists.freedesktop.org> On Behalf Of Lucas De Marchi
Sent: Friday, September 13, 2024 3:14 PM
To: igt-dev at lists.freedesktop.org
Cc: Vivi, Rodrigo <rodrigo.vivi at intel.com>; Bommu, Krishnaiah <krishnaiah.bommu at intel.com>; Ceraolo Spurio, Daniele <daniele.ceraolospurio at intel.com>; De Marchi, Lucas <lucas.demarchi at intel.com>
Subject: [PATCH i-g-t] lib/igt_kmod: Rewrite xe unload logic
> 
> Stop trying to unload possibly-dependent modules. Loading a module and
> probing a module are 2 different things, although the Linux kernel will
> by default probe devices when a module is loaded (for pci, configurable
> in /sys/bus/pci/drivers_autoprobe).
> 
> For the unloading part we can simplify: instead of trying to unload
> the modules directly (which may complain that module is already in use)
> we just unplug the HW first.  For each possible device bound.
> 
> This will be come part of libkmod in future, but we don't need to wait:

s/be come/become

> we can implement it in igt too as it's straightforward.
> 
> For now this only switches the unload path for xe, but if it works well,
> we can switch others in future too.
> 
> Closes: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/2362
> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>

Some minor nits below, but otherwise:
Reviewed-by: Jonathan Cavitt <jonathan.cavitt at intel.com>

> ---
> 
> This works for me for the test above that was failing, but not thouroughly
> tested with the rest of the tests that may trigger module load/unload.
> Tested on BMG and DG2. YMMV :)
> 
>  lib/igt_kmod.c | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  lib/igt_kmod.h |  5 +----
>  2 files changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 464c0dcf4..1868fdbf7 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -697,6 +697,53 @@ igt_intel_driver_unload(const char *driver)
>  	return 0;
>  }
>  
> +static int unbind_xe(void)

Nit: It might be better to rename this function to something like xe_unbind
or xe_unbind_helper instead.

> +{
> +	struct dirent *de;
> +	DIR *dir;
> +
> +	dir = opendir("/sys/module/xe/drivers/pci:xe/");
> +
> +	/* Module may be loaded, but without any device bound */
> +	if (!dir)
> +		return 0;
> +
> +	while ((de = readdir(dir))) {
> +		int devfd;
> +		bool ret;
> +
> +		if (de->d_type != DT_LNK || !isdigit(de->d_name[0]))
> +			continue;
> +
> +		devfd = openat(dirfd(dir), de->d_name, O_RDONLY | O_CLOEXEC);
> +		igt_assert(devfd >= 0);
> +
> +		ret = igt_sysfs_set(devfd, "power/control", "auto");
> +		igt_assert(ret);
> +
> +		ret = igt_sysfs_set(devfd, "driver/unbind", de->d_name);
> +		igt_assert(ret);
> +
> +		close(devfd);
> +		errno = 0;
> +	}
> +
> +	igt_assert_eq(errno, 0);
> +
> +        return 0;
> +}
> +
> +int igt_xe_driver_unload(void)
> +{
> +	unbind_xe();
> +
> +	igt_kmod_unload("xe");
> +	if (igt_kmod_is_loaded("xe"))
> +		return IGT_EXIT_FAILURE;

Nit: This is a pretty significant departure from the current implementation
that uses igt_intel_driver_unload.  Specifically, it seems to carry a lot less
"baggage", which I think is a good thing.  But we should also be careful
nothing important went missing from the old implementation, just in case.
-Jonathan Cavitt

> +
> +	return IGT_EXIT_SUCCESS;
> +}
> +
>  /**
>   * igt_amdgpu_driver_load:
>   * @opts: options to pass to amdgpu driver
> diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
> index efb46da12..ee1719a8f 100644
> --- a/lib/igt_kmod.h
> +++ b/lib/igt_kmod.h
> @@ -63,10 +63,7 @@ static inline int igt_xe_driver_load(const char *opts)
>  }
>  
>  
> -static inline int igt_xe_driver_unload(void)
> -{
> -	return igt_intel_driver_unload("xe");
> -}
> +int igt_xe_driver_unload(void);
>  
>  int igt_amdgpu_driver_load(const char *opts);
>  int igt_amdgpu_driver_unload(void);
> -- 
> 2.43.0
> 
>