Re: [igt-dev] [PATCH i-g-t 1/2] lib: Add silent __igt_i915_driver_unload

["Dixit, Ashutosh" <ashutosh.dixit@intel.com>]

On Mon, 25 Oct 2021 02:23:04 -0700, Anshuman Gupta wrote:
>
> Add silent __igt_i915_driver_unload(), which will not
> print any warning in case of passed argument is NULL.

I think we need to add a "why" to this commit message and that is "so that
we can test scenarios when module unload is expected to fail".

>
> Cc: Chris Wilson <chris.p.wilson@intel.com>
> Signed-off-by: Anshuman Gupta <anshuman.gupta@intel.com>
> ---
>  lib/igt_kmod.c | 86 ++++++++++++++++++++++++++++++--------------------
>  lib/igt_kmod.h |  1 +
>  2 files changed, 52 insertions(+), 35 deletions(-)
>
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 10e983844..ce9bfc07a 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -369,56 +369,72 @@ igt_i915_driver_load(const char *opts)
>	return IGT_EXIT_SUCCESS;
>  }
>
> -/**
> - * igt_i915_driver_unload:
> - *
> - * Unloads the i915 driver and its dependencies.
> - *
> - */
> -int
> -igt_i915_driver_unload(void)
> +int __igt_i915_driver_unload(const char **whom)

I prefer s/whom/who/ as in "Who failed to unload?".

>  {
> +	const char *sound[] = {
> +		"snd_hda_intel",
> +		"snd_hdmi_lpe_audio",
> +		NULL,
> +	};
> +
> +	const char *aux[] = {
> +		/* gen5: ips uses symbol_get() so only a soft module dependency */
> +		"intel_ips",
> +		NULL,
> +	};
> +
>	/* unbind vt */
>	bind_fbcon(false);
>
> -	if (igt_kmod_is_loaded("snd_hda_intel")) {
> -		igt_terminate_process(SIGTERM, "alsactl");
> -
> -		/* unbind snd_hda_intel */
> -		kick_snd_hda_intel();
> -
> -		if (igt_kmod_unload("snd_hda_intel", 0)) {
> -			igt_warn("Could not unload snd_hda_intel\n");
> -			igt_kmod_list_loaded();
> -			igt_lsof("/dev/snd");
> -			return IGT_EXIT_FAILURE;
> +	for (const char **m = sound; *m; m++) {
> +		if (igt_kmod_is_loaded(*m)) {
> +			igt_terminate_process(SIGTERM, "alsactl");
> +			kick_snd_hda_intel();
> +			if (igt_kmod_unload(*m, 0)) {
> +				if (whom)
> +					*whom = *m;

This seems like a bug. The sound and aux arrays above should be static
otherwise these strings go out of scope after the function exits.

> +				return IGT_EXIT_FAILURE;
> +			}
>		}
>	}
>
> -	if (igt_kmod_is_loaded("snd_hdmi_lpe_audio")) {
> -		igt_terminate_process(SIGTERM, "alsactl");
> -
> -		if (igt_kmod_unload("snd_hdmi_lpe_audio", 0)) {
> -			igt_warn("Could not unload snd_hdmi_lpe_audio\n");
> -			igt_kmod_list_loaded();
> -			igt_lsof("/dev/snd");
> -			return IGT_EXIT_FAILURE;
> -		}
> +	for (const char **m = aux; *m; m++) {
> +		if (igt_kmod_is_loaded(*m))
> +			igt_kmod_unload(*m, 0);

This issue exists in previous code too but why not return an error here
(and also populate 'whom' or 'who')?

>	}
>
> -	/* gen5: ips uses symbol_get() so only a soft module dependency */
> -	if (igt_kmod_is_loaded("intel_ips"))
> -		igt_kmod_unload("intel_ips", 0);
> -
>	if (igt_kmod_is_loaded("i915")) {
>		if (igt_kmod_unload("i915", 0)) {
> -			igt_warn("Could not unload i915\n");
> -			igt_kmod_list_loaded();
> -			igt_lsof("/dev/dri");
> +			if (whom)
> +				*whom = "i915";
>			return IGT_EXIT_SKIP;
>		}
>	}
>
> +	return IGT_EXIT_SUCCESS;

The issue exists in previous code but to me it seems highly unusual that we
are using process exit codes as return values from these functions
(both load and unload).

Note that after we have introduced and populated 'whom' or 'who', we just
return the value retured from igt_kmod_unload() from here (and similarly in
igt_i915_driver_load()) and let the caller figure out who failed to unload
from 'whom' or 'who'. That is we don't need separate return values for
other drivers vs. i915. Thoughts?


> +}
> +
> +/**
> + * igt_i915_driver_unload:
> + *
> + * Unloads the i915 driver and its dependencies.
> + *
> + */
> +int
> +igt_i915_driver_unload(void)
> +{
> +	const char *whom;
> +	int ret;
> +
> +	ret = __igt_i915_driver_unload(&whom);
> +	if (ret) {
> +		igt_warn("Could not unload %s\n", whom);
> +		igt_kmod_list_loaded();
> +		igt_lsof("/dev/dri");
> +		igt_lsof("/dev/snd");
> +		return ret;
> +	}
> +
>	if (igt_kmod_is_loaded("intel-gtt"))
>		igt_kmod_unload("intel-gtt", 0);
>
> diff --git a/lib/igt_kmod.h b/lib/igt_kmod.h
> index 04c87516f..0898122b3 100644
> --- a/lib/igt_kmod.h
> +++ b/lib/igt_kmod.h
> @@ -38,6 +38,7 @@ int igt_kmod_unload(const char *mod_name, unsigned int flags);
>
>  int igt_i915_driver_load(const char *opts);
>  int igt_i915_driver_unload(void);
> +int __igt_i915_driver_unload(const char **whom);
>
>  int igt_amdgpu_driver_load(const char *opts);
>  int igt_amdgpu_driver_unload(void);
> --
> 2.26.2
>