Fix Apps stopping & starting lvgl

Modules/app-esp32-module/source/module.cpp

@@ -47,7 +47,7 @@ void module_refs_end() {
 
 void module_refs_release(ModuleRefs* refs) {
     for (auto* module : refs->modules) {
-        module_put(module);
+        module_put_code(module);
     }
     refs->modules.clear();
 }
@@ -57,7 +57,9 @@ extern "C" {
 // Overrides elf_loader's default KConfig-based symbol resolver with one that looks up symbols
 // across every started kernel module's own symbol table instead. This module's own symbols come
 // first, as they replace libc functions for apps (see app_symbols.cpp).
-// While an ELF is relocated, the modules it resolves symbols from are referenced, so they can't stop while it runs.
+// While an ELF is relocated, the modules it resolves symbols from are referenced, so that their
+// code stays mapped while it runs. It's a code reference, so it doesn't stop an app from stopping
+// such a module on purpose (e.g. to take over the display) and starting it again afterwards.
 uintptr_t app_esp32_symbol_resolver(const char* symbolName) {
     uintptr_t symbol_address;
     if (module_resolve_symbol(&app_esp32_module, symbolName, &symbol_address)) {
@@ -69,13 +71,13 @@ uintptr_t app_esp32_symbol_resolver(const char* symbolName) {
     }
 
     Module* owner = nullptr;
-    if (!module_resolve_symbol_global_get(symbolName, &symbol_address, &owner)) {
+    if (!module_resolve_symbol_global_get_code(symbolName, &symbol_address, &owner)) {
         return 0;
     }
     // One reference per module is enough
     for (auto* module : current_refs->modules) {
         if (module == owner) {
-            module_put(owner);
+            module_put_code(owner);
             return symbol_address;
         }
     }

Modules/app-esp32-module/source/module_refs.h

@@ -5,7 +5,7 @@
 
 #include <vector>
 
-/** The kernel modules whose symbols an ELF resolved while it was relocated. Each one is held with module_get(). */
+/** The kernel modules whose symbols an ELF resolved while it was relocated. Each one is held with a code reference (module_get_code()). */
 struct ModuleRefs {
     std::vector<Module*> modules;
 };

TactilityKernel/include/tactility/module.h

@@ -103,7 +103,7 @@ error_t module_add(struct Module* module);
  * @brief Remove a module from the system.
  * @param module module to remove
  * @retval ERROR_NONE if successful
- * @retval ERROR_RESOURCE_BUSY the module is referenced (see module_get()). It stays added.
+ * @retval ERROR_RESOURCE_BUSY the module is referenced (see module_get() and module_get_code()). It stays added.
  */
 error_t module_remove(struct Module* module);
 
@@ -118,7 +118,8 @@ error_t module_start(struct Module* module);
  * @brief Stop the module.
  * @param module module
  * @retval ERROR_NONE if successful or if the module wasn't started
- * @retval ERROR_RESOURCE_BUSY the module is referenced (see module_get()) or a driver of it is bound to a device. The module keeps running.
+ * @retval ERROR_RESOURCE_BUSY the module is referenced (see module_get(), or module_get_code() for a
+ * module that came from a package) or a driver of it is bound to a device. The module keeps running.
  * @return otherwise the result of the module's stop function
  */
 error_t module_stop(struct Module* module);
@@ -187,6 +188,26 @@ error_t module_get(struct Module* module);
  */
 void module_put(struct Module* module);
 
+/**
+ * @brief Take a reference on a module for code that was relocated against its symbols.
+ * @details Unlike module_get(), this reference doesn't prevent module_stop(): relocated code only
+ * needs the module's code and data to stay mapped, which only module_remove() would break. It does
+ * prevent module_remove() for as long as the reference is held.
+ * This is what an app's ELF gets for every module it resolves symbols from, so that the app can
+ * still stop a module it was linked against (e.g. to take exclusive control of a device) and start
+ * it again afterwards. Every successful call must be paired with one module_put_code().
+ * @param module module to reference
+ * @retval ERROR_NONE the module is referenced
+ * @retval ERROR_INVALID_STATE the module isn't added, so its symbols can't be resolved
+ */
+error_t module_get_code(struct Module* module);
+
+/**
+ * @brief Release a reference taken with a successful module_get_code() or module_resolve_symbol_global_get_code().
+ * @param module module that was referenced
+ */
+void module_put_code(struct Module* module);
+
 /**
  * @brief Resolve a symbol from the module.
  * @details The module must be added with module_add() for symbol resolution to succeed. It doesn't have to be started.
@@ -216,6 +237,19 @@ bool module_resolve_symbol_global(const char* symbol_name, uintptr_t* symbol_add
  */
 bool module_resolve_symbol_global_get(const char* symbol_name, uintptr_t* symbol_address, struct Module** out_module);
 
+/**
+ * @brief Same as module_resolve_symbol_global(), but also takes a code reference on the module
+ * that has the symbol (see module_get_code()).
+ * @details Use this when the resolved symbol ends up in code that outlives the lookup (e.g. an ELF
+ * relocation), where the module's code must stay mapped but where the module itself may still be
+ * stopped and started again.
+ * @param symbol_name name of the symbol to resolve
+ * @param symbol_address pointer to store the address of the resolved symbol
+ * @param out_module pointer to store the module that has the symbol. Call module_put_code() on it.
+ * @return true if the symbol was found. The module is referenced then.
+ */
+bool module_resolve_symbol_global_get_code(const char* symbol_name, uintptr_t* symbol_address, struct Module** out_module);
+
 #ifdef __cplusplus
 }
 #endif

TactilityKernel/source/module.cpp

@@ -17,6 +17,9 @@ struct ModuleInternal {
     bool external = false;
     // All three are guarded by the ledger mutex
     int use_count = 0;
+    // References from relocated code (see module_get_code()): they keep the module mapped, but
+    // they don't keep it running
+    int code_count = 0;
     bool stopping = false;
     // In the ledger. Its symbols can be resolved from then on, also while it hasn't started yet.
     bool added = false;
@@ -76,7 +79,7 @@ error_t module_add(Module* module) {
 error_t module_remove(Module* module) {
     mutex_lock(&ledger.mutex);
     // Checked under the same lock as the removal, so no reference can be taken in between
-    if (module->internal != nullptr && module->internal->use_count != 0) {
+    if (module->internal != nullptr && (module->internal->use_count != 0 || module->internal->code_count != 0)) {
         mutex_unlock(&ledger.mutex);
         return ERROR_RESOURCE_BUSY;
     }
@@ -140,6 +143,18 @@ static void end_stopping(ModuleInternal* internal) {
     mutex_unlock(&ledger.mutex);
 }
 
+/**
+ * A usage reference always blocks stopping. A code reference (from relocated code) doesn't: an app
+ * stops a firmware module it was linked against on purpose, to take over the device that module
+ * drives, and starts it again when it's done.
+ * Modules from packages are the exception: the user can stop those from Kernel settings while an
+ * app is running, so there the references from that app must keep it running.
+ * Caller must hold the ledger mutex.
+ */
+static bool is_stop_blocked(const ModuleInternal* internal) {
+    return internal->use_count != 0 || (internal->external && internal->code_count != 0);
+}
+
 error_t module_stop(Module* module) {
     LOG_I(TAG, "stop %s", module->name);
 
@@ -148,7 +163,7 @@ error_t module_stop(Module* module) {
 
     // A module that hasn't started can be referenced too, through its symbols
     mutex_lock(&ledger.mutex);
-    const bool referenced = internal->use_count != 0;
+    const bool referenced = is_stop_blocked(internal);
     mutex_unlock(&ledger.mutex);
     if (referenced) {
         LOG_W(TAG, "%s can't stop: it is in use", module->name);
@@ -159,7 +174,7 @@ error_t module_stop(Module* module) {
 
     // Checking and refusing new references is one atomic step, so a module_get() can't slip in afterwards
     mutex_lock(&ledger.mutex);
-    if (internal->use_count != 0) {
+    if (is_stop_blocked(internal)) {
         mutex_unlock(&ledger.mutex);
         LOG_W(TAG, "%s can't stop: it is in use", module->name);
         return ERROR_RESOURCE_BUSY;
@@ -266,6 +281,27 @@ void module_put(Module* module) {
     mutex_unlock(&ledger.mutex);
 }
 
+error_t module_get_code(Module* module) {
+    auto* internal = module->internal;
+    if (internal == nullptr) { return ERROR_INVALID_STATE; }
+
+    mutex_lock(&ledger.mutex);
+    // Also allowed while the module isn't started: relocated code needs the symbols, not the state
+    const bool available = internal->added && !internal->stopping;
+    if (available) {
+        internal->code_count++;
+    }
+    mutex_unlock(&ledger.mutex);
+    return available ? ERROR_NONE : ERROR_INVALID_STATE;
+}
+
+void module_put_code(Module* module) {
+    mutex_lock(&ledger.mutex);
+    check(module->internal != nullptr && module->internal->code_count > 0);
+    module->internal->code_count--;
+    mutex_unlock(&ledger.mutex);
+}
+
 bool module_resolve_symbol(Module* module, const char* symbol_name, uintptr_t* symbol_address) {
     if (module->internal == nullptr || !module->internal->added) return false;
     auto* symbol_ptr = module->symbols;
@@ -280,33 +316,46 @@ bool module_resolve_symbol(Module* module, const char* symbol_name, uintptr_t* s
     return false;
 }
 
-bool module_resolve_symbol_global(const char* symbol_name, uintptr_t* symbol_address) {
-    mutex_lock(&ledger.mutex);
-    for (auto* module : ledger.modules) {
-        if (module_resolve_symbol(module, symbol_name, symbol_address)) {
-            mutex_unlock(&ledger.mutex);
-            return true;
-        }
-    }
-    mutex_unlock(&ledger.mutex);
-    return false;
-}
+enum class RefKind { NONE, USE, CODE };
 
-bool module_resolve_symbol_global_get(const char* symbol_name, uintptr_t* symbol_address, Module** out_module) {
+static bool resolve_symbol_global(const char* symbol_name, uintptr_t* symbol_address, Module** out_module, RefKind ref_kind) {
     mutex_lock(&ledger.mutex);
     for (auto* module : ledger.modules) {
         auto* internal = module->internal;
-        if (internal == nullptr || internal->stopping)
+        if (internal == nullptr) {
             continue;
+        }
+        // A module that's being stopped may still be resolvable, but no new reference may be taken
+        if (ref_kind != RefKind::NONE && internal->stopping) {
+            continue;
+        }
         if (module_resolve_symbol(module, symbol_name, symbol_address)) {
-            internal->use_count++;
+            if (ref_kind == RefKind::USE) {
+                internal->use_count++;
+            } else if (ref_kind == RefKind::CODE) {
+                internal->code_count++;
+            }
             mutex_unlock(&ledger.mutex);
-            *out_module = module;
+            if (out_module != nullptr) {
+                *out_module = module;
+            }
             return true;
         }
     }
     mutex_unlock(&ledger.mutex);
     return false;
 }
 
+bool module_resolve_symbol_global(const char* symbol_name, uintptr_t* symbol_address) {
+    return resolve_symbol_global(symbol_name, symbol_address, nullptr, RefKind::NONE);
+}
+
+bool module_resolve_symbol_global_get(const char* symbol_name, uintptr_t* symbol_address, Module** out_module) {
+    return resolve_symbol_global(symbol_name, symbol_address, out_module, RefKind::USE);
+}
+
+bool module_resolve_symbol_global_get_code(const char* symbol_name, uintptr_t* symbol_address, Module** out_module) {
+    return resolve_symbol_global(symbol_name, symbol_address, out_module, RefKind::CODE);
+}
+
 }

TactilityKernel/source/symbols.c

@@ -620,8 +620,11 @@ const struct ModuleSymbol KERNEL_SYMBOLS[] = {
     DEFINE_MODULE_SYMBOL(module_resolve_symbol),
     DEFINE_MODULE_SYMBOL(module_resolve_symbol_global),
     DEFINE_MODULE_SYMBOL(module_resolve_symbol_global_get),
+    DEFINE_MODULE_SYMBOL(module_resolve_symbol_global_get_code),
     DEFINE_MODULE_SYMBOL(module_get),
     DEFINE_MODULE_SYMBOL(module_put),
+    DEFINE_MODULE_SYMBOL(module_get_code),
+    DEFINE_MODULE_SYMBOL(module_put_code),
     // bundle
     DEFINE_MODULE_SYMBOL(bundle_alloc),
     DEFINE_MODULE_SYMBOL(bundle_clone),

TactilityKernel/tests/source/module_test.cpp

@@ -229,6 +229,73 @@ TEST_CASE("module_stop is refused while the module is referenced") {
     CHECK_EQ(module_destruct(&module), ERROR_NONE);
 }
 
+TEST_CASE("A code reference keeps a module mapped without preventing a stop") {
+    static const struct ModuleSymbol symbols[] = {
+        DEFINE_MODULE_SYMBOL(symbol_test_function),
+        MODULE_SYMBOL_TERMINATOR
+    };
+    start_called = false;
+    stop_called = false;
+    test_start_result = ERROR_NONE;
+    test_stop_result = ERROR_NONE;
+    struct Module module = {
+        .name = "code_referenced_test",
+        .start = test_start,
+        .stop = test_stop,
+        .symbols = symbols,
+        .internal = nullptr
+    };
+
+    REQUIRE_EQ(module_construct_add_start(&module), ERROR_NONE);
+
+    uintptr_t address = 0;
+    struct Module* owner = nullptr;
+    REQUIRE(module_resolve_symbol_global_get_code("symbol_test_function", &address, &owner));
+    CHECK_EQ(owner, &module);
+
+    // Relocated code only needs the symbols, so it doesn't keep the module running
+    CHECK_EQ(module_stop(&module), ERROR_NONE);
+    CHECK_EQ(stop_called, true);
+
+    // It does keep the module from being removed or destructed
+    CHECK_EQ(module_remove(&module), ERROR_RESOURCE_BUSY);
+    CHECK_EQ(module_ensure_destructed(&module), ERROR_RESOURCE_BUSY);
+
+    // And it can be started again while the code reference is held
+    CHECK_EQ(module_start(&module), ERROR_NONE);
+    CHECK_EQ(module_is_started(&module), true);
+
+    module_put_code(owner);
+    CHECK_EQ(module_ensure_destructed(&module), ERROR_NONE);
+}
+
+TEST_CASE("A code reference prevents stopping a module from a package") {
+    stop_called = false;
+    test_start_result = ERROR_NONE;
+    test_stop_result = ERROR_NONE;
+    struct Module module = {
+        .name = "external_code_referenced_test",
+        .start = test_start,
+        .stop = test_stop,
+        .symbols = nullptr,
+        .internal = nullptr
+    };
+
+    REQUIRE_EQ(module_construct_add_start(&module), ERROR_NONE);
+    REQUIRE_EQ(module_set_external(&module, true), ERROR_NONE);
+    REQUIRE_EQ(module_get_code(&module), ERROR_NONE);
+
+    // The user can stop package modules from Kernel settings, so an app's use of one must protect it
+    CHECK_EQ(module_stop(&module), ERROR_RESOURCE_BUSY);
+    CHECK_EQ(stop_called, false);
+
+    module_put_code(&module);
+    CHECK_EQ(module_stop(&module), ERROR_NONE);
+    CHECK_EQ(stop_called, true);
+    CHECK_EQ(module_remove(&module), ERROR_NONE);
+    CHECK_EQ(module_destruct(&module), ERROR_NONE);
+}
+
 TEST_CASE("module_remove is refused while the module is referenced") {
     static const struct ModuleSymbol symbols[] = {
         DEFINE_MODULE_SYMBOL(symbol_test_function),