mpsl: pm: Fix MPSL Pl uninit -NRF_ETIMEDOUT

There was a lock if a MPSL PM was uninitialized by
mpsl_lib_uninit() from hci_driver.c hci_driver_close().
The hci_driver_close() locks multi-threading by call
MULTITHREADING_LOCK_ACQUIRE(). The same mutex is acquired
by MPSL mpsl_low_prio_work_handler. The MPSL PM used
an uninit work item that was scheduled to MPSL low prio work
queue. The work was never executed during the MPSL PM uninit,
even though the uninit was waiting on a semaphore.
That was a lock that caused -NRF_ETIMEDOUT error to be
returned.

The mpsl_pm_uninit function can't be called without earlier
MULTITHREADING_LOCK_ACQUIRE() call. That is a prerequisite
to be fulfilled by a user.
On the other hand, the use of the lock allows to stop
using a work item during uninitialization of the MPSL PM.
The is not needed because if there is any work item in
the queue, then it will be handled after uninit is done.
It will have no effect on PM subsystem. These items will
be just removed from the MPSL work queue.

Signed-off-by: Piotr Pryga <[email protected]>

Author: ppryga-nordic

subsys/mpsl/pm/Kconfig

@@ -15,13 +15,6 @@ config MPSL_USE_ZEPHYR_PM
 
 if MPSL_USE_ZEPHYR_PM
 
-config MPSL_PM_UNINIT_WORK_WAIT_TIME_MS
-	int "Timeout value the MPSL PM uninit procedure will wait for work queue to complete, in milliseconds"
-	default 100
-	help
-	  The option specifies a timeout value in milliseconds, the MPLS PM uninit procedure
-	  will wait for MPSL work queue to complete handling of scheduled uninit work item.
-
 config MPSL_PM_NO_RADIO_EVENT_PERIOD_LATENCY_US
 	int "Latency value the MPSL PM allows in periods outside of radio events, in microseconds"
 	default 499999

subsys/mpsl/pm/mpsl_pm_utils.c

@@ -20,19 +20,9 @@
 LOG_MODULE_REGISTER(mpsl_pm_utils, CONFIG_MPSL_LOG_LEVEL);
 
 #define NO_RADIO_EVENT_PERIOD_LATENCY_US CONFIG_MPSL_PM_NO_RADIO_EVENT_PERIOD_LATENCY_US
-#define UNINIT_WORK_WAIT_TIMEOUT_MS	 K_MSEC(CONFIG_MPSL_PM_UNINIT_WORK_WAIT_TIME_MS)
-
-/* All MPSL PM event and latency actions are triggered inside the library.
- * The uninitialization may be started outside of the library thgough public API.
- * To avoid interference with other MPSL work items we need dedicated work
- * item for uninitialization purpose.
- */
-static void m_pm_uninit_work_handler(struct k_work *work);
-static K_WORK_DELAYABLE_DEFINE(m_pm_uninit_work, m_pm_uninit_work_handler);
 
 enum MPLS_PM_STATE {
 	MPSL_PM_UNINITIALIZED,
-	MPSL_PM_UNINITIALIZING,
 	MPSL_PM_INITIALIZED
 };
 
@@ -43,7 +33,6 @@ static struct pm_policy_latency_request m_latency_req;
 static struct pm_policy_event m_evt;
 
 static atomic_t m_pm_state = (atomic_val_t)MPSL_PM_UNINITIALIZED;
-struct k_sem m_uninit_wait_sem;
 
 #if defined(CONFIG_MPSL_PM_USE_MRAM_LATENCY_SERVICE)
 #define LOW_LATENCY_ATOMIC_BITS_NUM 2
@@ -214,36 +203,13 @@ static void m_mram_low_latency_release(void)
 }
 #endif /* CONFIG_MPSL_PM_USE_MRAM_LATENCY_SERVICE */
 
-static void m_pm_uninit_work_handler(struct k_work *work)
-{
-	ARG_UNUSED(work);
-
-	mpsl_pm_utils_work_handler();
-}
-
 void mpsl_pm_utils_work_handler(void)
 {
 	enum MPLS_PM_STATE pm_state = (enum MPLS_PM_STATE)atomic_get(&m_pm_state);
 
 	if (pm_state == MPSL_PM_INITIALIZED) {
 		m_register_event();
 		m_register_latency();
-	} else if (pm_state == MPSL_PM_UNINITIALIZING) {
-
-		/* The uninitialization is handled by all MPSL work items as well as by dedicated
-		 * uninit work item. In case regular MPSL work item cleans MPSL PM the uninit
-		 * work queue will not do anything.
-		 */
-		pm_policy_latency_request_remove(&m_latency_req);
-		pm_policy_event_unregister(&m_evt);
-
-		/* The MPSL PM status is updated here to make the code unit testable.
-		 * There is no work queue when running UTs, so the mpsl_pm_utils_work_handler()
-		 * is manualy executed after mpsl_pm_utils_uninit() returns.
-		 */
-		atomic_set(&m_pm_state, (atomic_val_t)MPSL_PM_UNINITIALIZED);
-
-		k_sem_give(&m_uninit_wait_sem);
 	}
 }
 
@@ -271,31 +237,20 @@ int32_t mpsl_pm_utils_init(void)
 
 int32_t mpsl_pm_utils_uninit(void)
 {
-	int err;
-
 	if (atomic_get(&m_pm_state) != (atomic_val_t)MPSL_PM_INITIALIZED) {
 		return -NRF_EPERM;
 	}
 
 	mpsl_pm_uninit();
-	atomic_set(&m_pm_state, (atomic_val_t)MPSL_PM_UNINITIALIZING);
-	/* In case there is any pended MPSL work item that was not handled, a dedicated
-	 * work item is used to remove PM policy event and unregister latency request.
-	 */
-	(void)k_sem_init(&m_uninit_wait_sem, 0, 1);
-
-	mpsl_work_reschedule(&m_pm_uninit_work, K_NO_WAIT);
 
-	/* Wait for completion of the uninit work item to make sure user can re-initialize
-	 * MPSL PM after return.
+	/* The mpsl_pm_utils_uninit() must be called with MULTITHREADING_LOCK_ACQUIRE to make sure
+	 * there is no mpsl_low_prio_work running. That could lead to a race condition.
+	 * Call to MULTITHREADING_LOCK_ACQUIRE() is responsibility of the function caller.
 	 */
-	err = k_sem_take(&m_uninit_wait_sem, UNINIT_WORK_WAIT_TIMEOUT_MS);
-	if (err == -EAGAIN) {
-		return -NRF_ETIMEDOUT;
-	} else if (err != 0) {
-		__ASSERT(false, "MPSL PM uninit failed to complete: %d", err);
-		return -NRF_EFAULT;
-	}
+	pm_policy_latency_request_remove(&m_latency_req);
+	pm_policy_event_unregister(&m_evt);
+
+	atomic_set(&m_pm_state, (atomic_val_t)MPSL_PM_UNINITIALIZED);
 
 	return 0;
 }

subsys/mpsl/pm/mpsl_pm_utils.h

@@ -24,6 +24,9 @@ int32_t mpsl_pm_utils_init(void);
  *
  * This routine uninitializes MPSL PM (via `mpsl_pm_uninit`).
  *
+ * @pre The function requires the multithreading lock (@see MULTITHREADING_LOCK_ACQUIRE())
+ *      to be acquired before.
+ *
  * @retval 0 MPSL PM was uninitialized successfully.
  * @retval -NRF_EPERM MPSL was not initialized before the call.
  * @retval -NRF_ETIMEDOUT MPSL PM uninitialization timed out while waiting for completion.

tests/subsys/mpsl/pm/CMakeLists.txt

@@ -13,7 +13,6 @@ project(pm_test)
 test_runner_generate(pm_test.c)
 
 # Create mocks for pm module.
-cmock_handle(${CMAKE_CURRENT_SOURCE_DIR}/kernel_minimal_mock.h)
 cmock_handle(${ZEPHYR_BASE}/include/zephyr/pm/policy.h)
 cmock_handle(${ZEPHYR_NRFXLIB_MODULE_DIR}/mpsl/include/mpsl_pm.h)
 cmock_handle(${ZEPHYR_NRFXLIB_MODULE_DIR}/mpsl/include/mpsl_pm_config.h)
@@ -39,6 +38,5 @@ set_property(SOURCE ${ZEPHYR_NRF_MODULE_DIR}/subsys/mpsl/pm/mpsl_pm_utils.c
 target_compile_options(app PRIVATE
     -DCONFIG_PM=y
     -DCONFIG_MPSL_USE_ZEPHYR_PM=y
-    -DCONFIG_MPSL_PM_UNINIT_WORK_WAIT_TIME_MS=100
     -DCONFIG_MPSL_PM_NO_RADIO_EVENT_PERIOD_LATENCY_US=499999
 )

tests/subsys/mpsl/pm/kernel_minimal_mock.h

@@ -11,7 +11,6 @@
 #include <zephyr/kernel.h>
 
 #include "cmock_policy.h"
-#include "cmock_kernel_minimal_mock.h"
 #include "cmock_mpsl_pm.h"
 #include "cmock_mpsl_pm_config.h"
 #include "cmock_mpsl_work.h"
@@ -20,7 +19,6 @@
 #include "nrf_errno.h"
 
 #define NO_RADIO_EVENT_PERIOD_LATENCY_US CONFIG_MPSL_PM_NO_RADIO_EVENT_PERIOD_LATENCY_US
-#define UNINIT_WORK_WAIT_TIMEOUT_MS	 K_MSEC(CONFIG_MPSL_PM_UNINIT_WORK_WAIT_TIME_MS)
 /* The unity_main is not declared in any header file. It is only defined in the generated test
  * runner because of ncs' unity configuration. It is therefore declared here to avoid a compiler
  * warning.
@@ -55,52 +53,17 @@ typedef struct {
 	mpsl_pm_low_latency_state_t low_latency_state_next;
 } test_vector_latency_t;
 
-/* Init and uninit tests */
-void start_uninit_module(void)
+/* Manual teardown tested module, there is no way to overload it for given test case. */
+void uninit_tested_module(void)
 {
-	__cmock_mpsl_work_reschedule_Expect(0, (k_timeout_t){0});
-	__cmock_mpsl_work_reschedule_IgnoreArg_dwork();
 	__cmock_mpsl_pm_uninit_Expect();
 
-	__cmock_z_impl_k_sem_init_ExpectAndReturn(NULL, 0, 1, 0);
-	__cmock_z_impl_k_sem_init_IgnoreArg_sem();
-
-	/* The k_sem_take will return immediately because there is no real work queue in the test.
-	 * To make the proper uninitialization we must call mpsl_pm_utils_work_handler() after that.
-	 *
-	 * NOTE: There is an assumption the remaining part of unitialization is done by
-	 *       mpsl_pm_utils_work_handler(). The mpsl_pm_utils_uninit() just waits for it to end.
-	 */
-	__cmock_z_impl_k_sem_take_ExpectAndReturn(NULL, UNINIT_WORK_WAIT_TIMEOUT_MS, 0);
-	__cmock_z_impl_k_sem_take_IgnoreArg_sem();
-
-	TEST_ASSERT_EQUAL(0, mpsl_pm_utils_uninit());
-}
-
-void commit_uninit_module(void)
-{
 	__cmock_pm_policy_latency_request_remove_Expect(0);
 	__cmock_pm_policy_latency_request_remove_IgnoreArg_req();
 	__cmock_pm_policy_event_unregister_Expect(0);
 	__cmock_pm_policy_event_unregister_IgnoreArg_evt();
 
-	/* There is no real work queue so the mpsl_pm_utils_uninit() is not waiting for this
-	 * semaphore to be signaled. We must call this function to make unintialization to complete.
-	 *
-	 * NOTE: There is an assumption the remaining part of unitialization is done by
-	 *       mpsl_pm_utils_work_handler(). The mpsl_pm_utils_uninit() just waits for it to end.
-	 */
-	__cmock_z_impl_k_sem_give_Expect(NULL);
-	__cmock_z_impl_k_sem_give_IgnoreArg_sem();
-
-	mpsl_pm_utils_work_handler();
-}
-
-/* Manual teardown tested module, there is no way to overload it for given test case. */
-void uninit_tested_module(void)
-{
-	start_uninit_module();
-	commit_uninit_module();
+	TEST_ASSERT_EQUAL(0, mpsl_pm_utils_uninit());
 }
 
 void init_expect_prepare(void)
@@ -136,68 +99,11 @@ void test_init_when_already_initialized(void)
 	uninit_tested_module();
 }
 
-void test_init_when_uninit_started(void)
-{
-	init_expect_prepare();
-
-	TEST_ASSERT_EQUAL(0, mpsl_pm_utils_init());
-
-	start_uninit_module();
-
-	TEST_ASSERT_EQUAL(-NRF_EPERM, mpsl_pm_utils_init());
-
-	commit_uninit_module();
-}
-
 void test_uninit_when_not_initialized(void)
 {
 	TEST_ASSERT_EQUAL(-NRF_EPERM, mpsl_pm_utils_uninit());
 }
 
-void test_uninit_wait_timeout(void)
-{
-	init_expect_prepare();
-
-	TEST_ASSERT_EQUAL(0, mpsl_pm_utils_init());
-
-	__cmock_mpsl_work_reschedule_Expect(0, (k_timeout_t){0});
-	__cmock_mpsl_work_reschedule_IgnoreArg_dwork();
-	__cmock_mpsl_pm_uninit_Expect();
-
-	__cmock_z_impl_k_sem_init_ExpectAndReturn(NULL, 0, 1, 0);
-	__cmock_z_impl_k_sem_init_IgnoreArg_sem();
-
-	__cmock_z_impl_k_sem_take_ExpectAndReturn(NULL, UNINIT_WORK_WAIT_TIMEOUT_MS, -EAGAIN);
-	__cmock_z_impl_k_sem_take_IgnoreArg_sem();
-
-	TEST_ASSERT_EQUAL(-NRF_ETIMEDOUT, mpsl_pm_utils_uninit());
-
-	/* Just to cleanup after the test case */
-	commit_uninit_module();
-}
-
-void test_uninit_wait_fatal_err(void)
-{
-	init_expect_prepare();
-
-	TEST_ASSERT_EQUAL(0, mpsl_pm_utils_init());
-
-	__cmock_mpsl_work_reschedule_Expect(0, (k_timeout_t){0});
-	__cmock_mpsl_work_reschedule_IgnoreArg_dwork();
-	__cmock_mpsl_pm_uninit_Expect();
-
-	__cmock_z_impl_k_sem_init_ExpectAndReturn(NULL, 0, 1, 0);
-	__cmock_z_impl_k_sem_init_IgnoreArg_sem();
-
-	__cmock_z_impl_k_sem_take_ExpectAndReturn(NULL, UNINIT_WORK_WAIT_TIMEOUT_MS, -EBUSY);
-	__cmock_z_impl_k_sem_take_IgnoreArg_sem();
-
-	TEST_ASSERT_EQUAL(-NRF_EFAULT, mpsl_pm_utils_uninit());
-
-	/* Just to cleanup after the test case */
-	commit_uninit_module();
-}
-
 /* Latency and event handling tests */
 
 void run_test(test_vector_event_t *p_test_vectors, int num_test_vctr)