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

["Gupta, Anshuman" <anshuman.gupta@intel.com>]

> -----Original Message-----
> From: Dixit, Ashutosh <ashutosh.dixit@intel.com>
> Sent: Saturday, February 12, 2022 9:51 AM
> To: Gupta, Anshuman <anshuman.gupta@intel.com>
> Cc: igt-dev@lists.freedesktop.org; Wilson, Chris P <chris.p.wilson@intel.com>;
> Nilawar, Badal <badal.nilawar@intel.com>; janusz.krzysztofik@linux.intel.com
> Subject: Re: [igt-dev] [PATCH i-g-t 1/2] lib: Add silent __igt_i915_driver_unload
> 
> 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?
Actually I missed this earlier while sending v2, perf_pmu test expect i915 module to be busy in
certain cases and returning IGT_EXIT_FAILIURE will fail the perf_pmu test.
https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_6626/shard-tglb8/igt@perf_pmu@module-unload.html
So in case of *who is NULL , __igt_i915_driver_unload() should return IGT_EXIT_SKIP,
whenever igt module fails to unload.
Thanks,
Anshuman Gupta.
> 
> 
> > +}
> > +
> > +/**
> > + * 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
> >