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

[Lucas De Marchi]

On Fri, Sep 13, 2024 at 05:47:47PM GMT, Cavitt, Jonathan wrote:
>-----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.

yeah, maybe... but if this does work, then we can probably convert the
rest and then this would be just unbind_xe(const char *module) or so

>
>> +{
>> +	struct dirent *de;
>> +	DIR *dir;
>> +
>> +	dir = opendir("/sys/module/xe/drivers/pci:xe/");

in `kmod unbind` I actually start from /sys/module/<module>/drivers/
because there I have to take care of several types of buses. Looking
again in igt, I guess I could type something common that would take care
of snd_hda_intel. Or maybe we don't even need that anymore after making
sure we go through the unbind path rather than simply rmmod.

I'm also not sure why we are removing the drm module in some places.
It seems unneeded and we keep adding more complexity because of these
additional stuff we do.... that whole pipewire / pulse thing can
actually be gone.

>> +
>> +	/* 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


honestly I wasn't expecting this would even pass BAT. It did, which was
a nice surprise.

thanks
Lucas De Marchi

>
>> +
>> +	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
>>
>>