Re: [PATCH i-g-t] lib/igt_core: Refactor libpciaccess init/cleanup wrappers

["Bernatowicz, Marcin" <marcin.bernatowicz@linux.intel.com>]

On 8/29/2024 9:58 AM, Marcin Bernatowicz wrote:
> Enable reinitialization of the libpciaccess global state, necessary
> to correctly handle dynamic add/remove of PCI devices, such as the
> creation/removal of Virtual Functions (VFs). Update
> igt_pci_system_cleanup() to conditionally call pci_system_cleanup()
> based on the initialization state. Introduce igt_pci_system_reinit()
> for explicit reinitialization of the libpciaccess global state,
> particularly useful after PCI device changes, to be used in
> subsequent patches.
> 
> Signed-off-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> Cc: Adam Miszczak <adam.miszczak@linux.intel.com>
> Cc: Lukasz Laguna <lukasz.laguna@intel.com>
> Cc: Jakub Kolakowski <jakub1.kolakowski@intel.com>
> Cc: Janusz Krzysztofik <janusz.krzysztofik@linux.intel.com>
> Cc: Chris Wilson <chris.p.wilson@intel.com>
> ---
>   lib/igt_core.c | 42 ++++++++++++++++++++++++++++++++++++------
>   lib/igt_core.h | 36 ++++++++++++++++++++++++------------
>   2 files changed, 60 insertions(+), 18 deletions(-)
> 
> diff --git a/lib/igt_core.c b/lib/igt_core.c
> index 3ff3e0392..4710899bf 100644
> --- a/lib/igt_core.c
> +++ b/lib/igt_core.c
> @@ -3414,21 +3414,51 @@ void igt_srandom(void)
>   }
>   
>   /* IGT wrappers around libpciaccess init/cleanup functions */
> +static int pci_system_initialized;
> +static pthread_mutex_t pci_system_mutex = PTHREAD_MUTEX_INITIALIZER;
> +
> +void igt_pci_system_cleanup(void)
> +{
> +	pthread_mutex_lock(&pci_system_mutex);
> +	if (pci_system_initialized) {
> +		pci_system_cleanup();
> +		pci_system_initialized = 0;
> +	}
> +	pthread_mutex_unlock(&pci_system_mutex);
> +}
>   
>   static void pci_system_exit_handler(int sig)
>   {
> -	pci_system_cleanup();
> +	igt_pci_system_cleanup();
>   }
>   
> -static void __pci_system_init(void)
> +int igt_pci_system_init(void)
>   {
> -	if (!igt_warn_on_f(pci_system_init(), "Could not initialize libpciaccess global data\n"))
> +	int ret = 0;
> +	bool install_handler = false;
> +
> +	pthread_mutex_lock(&pci_system_mutex);
> +	if (!pci_system_initialized) {
> +		ret = igt_warn_on_f(pci_system_init(),
> +				    "Could not initialize libpciaccess global data\n");
> +		if (ret) {
> +			pci_system_initialized = 0;
> +		} else {
> +			pci_system_initialized = 1;
> +			install_handler = true;
> +		}

To be simplified with:

if (!pci_system_initialized) {
     ret = igt_warn_on_f(pci_system_init(), "Could not initialize...\n");
     if (!ret) {
         pci_system_initialized = 1;
         install_handler = true;
     }
}

> +	}
> +	pthread_mutex_unlock(&pci_system_mutex);
> +
> +	if (install_handler)
>   		igt_install_exit_handler(pci_system_exit_handler);
> +
> +	return ret;
>   }
>   
> -int igt_pci_system_init(void)
> +int igt_pci_system_reinit(void)
>   {
> -	static pthread_once_t once_control = PTHREAD_ONCE_INIT;
> +	igt_pci_system_cleanup();
>   
> -	return pthread_once(&once_control, __pci_system_init);
> +	return igt_pci_system_init();
>   }
> diff --git a/lib/igt_core.h b/lib/igt_core.h
> index 06c5314bf..0d7e948a8 100644
> --- a/lib/igt_core.h
> +++ b/lib/igt_core.h
> @@ -1518,28 +1518,40 @@ void igt_kmsg(const char *format, ...);
>    * igt_pci_system_init:
>    * IGT wrapper around pci_system_init()
>    *
> - * Runs pci_system_init() and installs pci_system_cleanup() as IGT exit handler when
> - * called first per thread, subsequent calls are noop.  Tests should use this wrapper
> + * Runs pci_system_init() and installs igt_pci_system_cleanup() as IGT exit handler when
> + * called first per thread, subsequent calls are noop. Tests should use this wrapper
>    * instead of pci_system_init() to avoid memory leaking which happens each time a call
>    * to pci_system_init() is repeated not preceded by pci_system_cleanup() (may easily
>    * happen in consequence of long jumps performed by IGT flow control functions).
>    *
> - * Return value: equal return value of pthread_once() (return value of pci_system_init()
> - *		 can't be passed through pthread_once())
> + * Return:
> + * Return value of pci_system_init() or 0 if pci system is already initialized.
>    */
>   int igt_pci_system_init(void);
>   
> +/**
> + * igt_pci_system_reinit:
> + * Reinitialize libpciaccess global data.
> + *
> + * Executes igt_pci_system_cleanup() and igt_pci_system_init() to refresh
> + * the PCI system state, typically needed after PCI devices are added or
> + * removed.
> + *
> + * Note: All previously obtained handles (pci_dev, mmio) become invalid
> + * after this call. Do not use old handles post-reinitialization.
> + *
> + * Return: Outcome of igt_pci_system_init().
> + */
> +int igt_pci_system_reinit(void);
> +
>   /**
>    * igt_pci_system_cleanup():
> - * IGT replacement for pci_system_cleanup()
> + * IGT wrapper around pci_system_cleanup()
>    *
> - * For use in IGT library and tests to avoid destroying libpciaccess global data.
> - * Direct calls to pci_system_cleanup() should be either dropped or replaced with this
> - * wrapper (for code clarity), otherwise subsequent access to libpciaccess global data
> - * may be lost unless preceded by direct call to pci_system_init() (not recommended).
> + * Runs pci_system_cleanup() if igt_pci_system_init() was successfully called
> + * before. This allows to refresh the libpciaccess global data when followed
> + * by igt_pci_system_init(), see igt_pci_system_reinit().
>    */
> -static inline void igt_pci_system_cleanup(void)
> -{
> -}
> +void igt_pci_system_cleanup(void);
>   
>   #endif /* IGT_CORE_H */