[igt-dev] [PATCH i-g-t] lib/igt_kmod: Allow some leeway in igt_kmod_unload_r

[Kamil Konieczny]

Hi Jonathan,

On 2023-01-09 at 11:25:36 -0800, Jonathan Cavitt wrote:
> kmod_module_remove_module occasionally returns EAGAIN for mei_gsc
> in the setup of some gem_lmem_swapping subtests.  Just because
> EAGAIN is returned doesn't mean the module is lost and unable to
> be unloaded.  Try again some number of times (currently 10) before
> giving up.
> 
> Retries will occur for -EBUSY and -EAGAIN, as these imply the
> system is waiting for the target module can simply be waited for.
> All other errors exit immediately, but as the context for each
> error informs their relative severity, no warnings will be issued.
> 
> References: VLK-30287, VLK-39629
- ^ --------- ^
Please remove any refs not reachable from internet.

> 
> Signed-off-by: Jonathan Cavitt <jonathan.cavitt at intel.com>
> CC: Stuart Summers <stuart.summers at intel.com>
> CC: Sandeep Kumar Parupalli <sandeep.kumar.parupalli at intel.com>
> CC: Chris Wilson <chris.p.wilson at linux.intel.com>
> Reviewed-by: Chris Wilson <chris.p.wilson at linux.intel.com>
> ---
>  lib/igt_kmod.c | 41 ++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/lib/igt_kmod.c b/lib/igt_kmod.c
> index 17090110c..c840f9a17 100644
> --- a/lib/igt_kmod.c
> +++ b/lib/igt_kmod.c
> @@ -253,8 +253,11 @@ out:
>  
>  static int igt_kmod_unload_r(struct kmod_module *kmod, unsigned int flags)
>  {
> +#define MAX_TRIES	20
> +#define SLEEP_DURATION	500000
>  	struct kmod_list *holders, *pos;
> -	int err = 0;
> +	int err, tries;
> +	const char *mod_name = kmod_module_get_name(kmod);
>  
>  	holders = kmod_module_get_holders(kmod);
>  	kmod_list_foreach(pos, holders) {
> @@ -269,7 +272,6 @@ static int igt_kmod_unload_r(struct kmod_module *kmod, unsigned int flags)
>  		return err;
>  
>  	if (igt_kmod_is_loading(kmod)) {
> -		const char *mod_name = kmod_module_get_name(kmod);

Please describe this change, it looks unrelated.

>  		igt_debug("%s still initializing\n", mod_name);
>  		err = igt_wait(!igt_kmod_is_loading(kmod), 10000, 100);
>  		if (err < 0) {
> @@ -279,7 +281,40 @@ static int igt_kmod_unload_r(struct kmod_module *kmod, unsigned int flags)
>  		}
>  	}
>  
> -	return kmod_module_remove_module(kmod, flags);
> +	for (tries = 0; tries < MAX_TRIES; tries++) {
> +		bool loop = false;
> +
> +		err = kmod_module_remove_module(kmod, flags);
> +
> +		/* Only loop in the following cases */
> +		switch (err) {
> +		case -EBUSY:
> +		case -EAGAIN:
> +			/* Waiting for module to be available */
> +			loop = true;
> +			break;
> +		default:
> +			break;
> +                }
> +		if (!loop)
> +			break;

May you turn switch into if() ? Now it looks convoluted.

> +
> +		igt_debug("Module %s failed to unload with err: %d on attempt: %i\n",
> +			  mod_name, err, tries + 1);
> +
> +		usleep(SLEEP_DURATION);
> +	}
> +
> +	if (err == -ENOENT)
> +		igt_debug("Module %s could not be found.  Skipping.\n", mod_name);
------------------------------------------------------- ^^^^^^^^^^
Please remove /  Skipping./, you print debugs and you will not
decide what caller will do.

One more error would be if driver is compiled in, so it
just can not be removed (will it be -ENOTSUP ?).

> +	else if (err)
> +		igt_info("Module %s failed to unload with err: %d after ~%.1fms\n",
--------------- ^
Please make them all into igt_info or igt_debug.

> +			 mod_name, err, SLEEP_DURATION*tries/1000.);
> +	else if (tries)
> +		igt_info("Module %s unload took ~%.1fms over %i attempts\n",
--------------- ^
Same here, please be consistent.

Regards,
Kamil

> +			 mod_name, SLEEP_DURATION*tries/1000., tries + 1);
> +
> +	return err;
>  }
>  
>  /**
> -- 
> 2.25.1
>