[Linux] Run only one GLib main event loop (project-chip#23320)

* Unregister from BlueZ when shutting down BLE manager

* Return CHIP_ERROR from all BlueZ helper functions

* Remove not used class method

* Remove D-Bus connection handler from PlatformManager

The D-Bus connection handler stored in the PlatformManager is not
used anywhere. It's not required to pre-connect to D-Bus, because
the connection (for given bus type) returned by the g_bus_get_sync()
function is shared with other callers.

* Run only one GLib main event loop per Matter SDK

It is not necessary to start new GLib main event loop for every new
D-Bus communication with external service. Single main event loop can
handle all such communication.

* Run WiFi IP address change listener in GLib main loop

* Properly release cancellable object on cleanup

* Explicitly wait for glib main thread to exit

* Workaround for TSAN false positive reports with glib

TSAN thinks that memory accessed before the call to g_source_attach()
(which is internally used by e.g. g_main_context_invoke() and
g_idle_add()) needs to be guarded by a mutex, otherwise that's a race
condition between the thread that is creating shared data (the current
thread) and the glib main event loop thread. In fact such race condition
does not occur because g_source_attach() acts here as pthread_create() -
code is not executed on the other thread before the g_source_attach() is
called.

Co-authored-by: Andrei Litvin <[email protected]>

src/controller/python/chip/ble/LinuxImpl.cpp

@@ -2,7 +2,6 @@
 #include <lib/support/CHIPMem.h>
 #include <platform/CHIPDeviceLayer.h>
 #include <platform/Linux/bluez/AdapterIterator.h>
-#include <platform/Linux/bluez/MainLoop.h>
 #include <platform/internal/BLEManager.h>
 
 using namespace chip::DeviceLayer::Internal;
@@ -58,7 +57,7 @@ extern "C" void * pychip_ble_adapter_list_get_raw_adapter(void * adapterIterator
 
 namespace {
 
-// To avoid pythoon compatibility issues on inc/dec references,
+// To avoid python compatibility issues on inc/dec references,
 // code assumes an abstract type and leaves it up to python to keep track of
 // reference counts
 struct PyObject;

src/platform/BUILD.gn

@@ -91,7 +91,6 @@ if (chip_device_platform != "none" && chip_device_platform != "external") {
     header = "CHIPDeviceBuildConfig.h"
     header_dir = "platform"
 
-    chip_with_gio = chip_enable_wifi
     chip_device_config_enable_wpa =
         chip_enable_wifi && chip_device_platform != "darwin"
     chip_stack_lock_tracking_log = chip_stack_lock_tracking != "none"
@@ -100,7 +99,6 @@ if (chip_device_platform != "none" && chip_device_platform != "external") {
       "CHIP_DEVICE_CONFIG_ENABLE_WPA=${chip_device_config_enable_wpa}",
       "CHIP_ENABLE_OPENTHREAD=${chip_enable_openthread}",
       "CHIP_DEVICE_CONFIG_THREAD_FTD=${chip_device_config_thread_ftd}",
-      "CHIP_WITH_GIO=${chip_with_gio}",
       "CHIP_STACK_LOCK_TRACKING_ENABLED=${chip_stack_lock_tracking_log}",
       "CHIP_STACK_LOCK_TRACKING_ERROR_FATAL=${chip_stack_lock_tracking_fatal}",
       "CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING=${chip_enable_additional_data_advertising}",

src/platform/Linux/BLEManagerImpl.cpp

@@ -97,6 +97,9 @@ void BLEManagerImpl::_Shutdown()
 {
     // ensure scan resources are cleared (e.g. timeout timers)
     mDeviceScanner.reset();
+    // Release BLE connection resources (unregister from BlueZ).
+    ShutdownBluezBleLayer(mpEndpoint);
+    mFlags.Clear(Flags::kBluezBLELayerInitialized);
 }
 
 CHIP_ERROR BLEManagerImpl::_SetAdvertisingEnabled(bool val)
@@ -348,7 +351,10 @@ bool BLEManagerImpl::SubscribeCharacteristic(BLE_CONNECTION_OBJECT conId, const
     VerifyOrExit(Ble::UUIDsMatch(charId, &ChipUUID_CHIPoBLEChar_TX),
                  ChipLogError(DeviceLayer, "SubscribeCharacteristic() called with invalid characteristic ID"));
 
-    result = BluezSubscribeCharacteristic(conId);
+    VerifyOrExit(BluezSubscribeCharacteristic(conId) == CHIP_NO_ERROR,
+                 ChipLogError(DeviceLayer, "BluezSubscribeCharacteristic() failed"));
+    result = true;
+
 exit:
     return result;
 }
@@ -362,21 +368,38 @@ bool BLEManagerImpl::UnsubscribeCharacteristic(BLE_CONNECTION_OBJECT conId, cons
     VerifyOrExit(Ble::UUIDsMatch(charId, &ChipUUID_CHIPoBLEChar_TX),
                  ChipLogError(DeviceLayer, "UnsubscribeCharacteristic() called with invalid characteristic ID"));
 
-    result = BluezUnsubscribeCharacteristic(conId);
+    VerifyOrExit(BluezUnsubscribeCharacteristic(conId) == CHIP_NO_ERROR,
+                 ChipLogError(DeviceLayer, "BluezUnsubscribeCharacteristic() failed"));
+    result = true;
+
 exit:
     return result;
 }
 
 bool BLEManagerImpl::CloseConnection(BLE_CONNECTION_OBJECT conId)
 {
+    bool result = false;
+
     ChipLogProgress(DeviceLayer, "Closing BLE GATT connection (con %p)", conId);
-    return CloseBluezConnection(conId);
+
+    VerifyOrExit(CloseBluezConnection(conId) == CHIP_NO_ERROR, ChipLogError(DeviceLayer, "CloseBluezConnection() failed"));
+    result = true;
+
+exit:
+    return result;
 }
 
 bool BLEManagerImpl::SendIndication(BLE_CONNECTION_OBJECT conId, const ChipBleUUID * svcId, const Ble::ChipBleUUID * charId,
                                     chip::System::PacketBufferHandle pBuf)
 {
-    return SendBluezIndication(conId, std::move(pBuf));
+    bool result = false;
+
+    VerifyOrExit(SendBluezIndication(conId, std::move(pBuf)) == CHIP_NO_ERROR,
+                 ChipLogError(DeviceLayer, "SendBluezIndication() failed"));
+    result = true;
+
+exit:
+    return result;
 }
 
 bool BLEManagerImpl::SendWriteRequest(BLE_CONNECTION_OBJECT conId, const Ble::ChipBleUUID * svcId, const Ble::ChipBleUUID * charId,
@@ -389,7 +412,10 @@ bool BLEManagerImpl::SendWriteRequest(BLE_CONNECTION_OBJECT conId, const Ble::Ch
     VerifyOrExit(Ble::UUIDsMatch(charId, &ChipUUID_CHIPoBLEChar_RX),
                  ChipLogError(DeviceLayer, "SendWriteRequest() called with invalid characteristic ID"));
 
-    result = BluezSendWriteRequest(conId, std::move(pBuf));
+    VerifyOrExit(BluezSendWriteRequest(conId, std::move(pBuf)) == CHIP_NO_ERROR,
+                 ChipLogError(DeviceLayer, "BluezSendWriteRequest() failed"));
+    result = true;
+
 exit:
     return result;
 }

src/platform/Linux/BUILD.gn

@@ -91,8 +91,6 @@ static_library("Linux") {
       "bluez/ChipDeviceScanner.h",
       "bluez/Helper.cpp",
       "bluez/Helper.h",
-      "bluez/MainLoop.cpp",
-      "bluez/MainLoop.h",
       "bluez/Types.h",
     ]
   }

src/platform/Linux/CHIPDevicePlatformConfig.h

@@ -41,6 +41,14 @@
 #define CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE 0
 #endif
 
+// Start GLib main event loop if BLE or WiFi is enabled. This is needed to handle
+// D-Bus communication with BlueZ or wpa_supplicant.
+#if CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE || CHIP_DEVICE_CONFIG_ENABLE_WIFI
+#define CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP 1
+#else
+#define CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP 0
+#endif
+
 // ========== Platform-specific Configuration =========
 
 // These are configuration options that are unique to Linux platforms.

src/platform/Linux/PlatformManagerImpl.cpp

@@ -24,6 +24,17 @@
 
 #include <platform/internal/CHIPDeviceLayerInternal.h>
 
+#include <arpa/inet.h>
+#include <dirent.h>
+#include <errno.h>
+#include <linux/netlink.h>
+#include <linux/rtnetlink.h>
+#include <net/if.h>
+#include <netinet/in.h>
+#include <unistd.h>
+
+#include <mutex>
+
 #include <app-common/zap-generated/enums.h>
 #include <app-common/zap-generated/ids/Events.h>
 #include <lib/support/CHIPMem.h>
@@ -35,22 +46,6 @@
 #include <platform/PlatformManager.h>
 #include <platform/internal/GenericPlatformManagerImpl_POSIX.ipp>
 
-#include <thread>
-
-#include <arpa/inet.h>
-#include <dirent.h>
-#include <errno.h>
-#include <linux/netlink.h>
-#include <linux/rtnetlink.h>
-#include <net/if.h>
-#include <netinet/in.h>
-#include <unistd.h>
-
-#if __GLIBC__ == 2 && __GLIBC_MINOR__ < 30
-#include <sys/syscall.h>
-#define gettid() syscall(SYS_gettid)
-#endif
-
 using namespace ::chip::app::Clusters;
 
 namespace chip {
@@ -60,41 +55,32 @@ PlatformManagerImpl PlatformManagerImpl::sInstance;
 
 namespace {
 
-#if CHIP_WITH_GIO
-void GDBus_Thread()
+#if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP
+void * GLibMainLoopThread(void * loop)
 {
-    GMainLoop * loop = g_main_loop_new(nullptr, false);
-
-    g_main_loop_run(loop);
-    g_main_loop_unref(loop);
+    g_main_loop_run(static_cast<GMainLoop *>(loop));
+    return nullptr;
 }
 #endif
-} // namespace
 
 #if CHIP_DEVICE_CONFIG_ENABLE_WIFI
-void PlatformManagerImpl::WiFIIPChangeListener()
+
+gboolean WiFiIPChangeListener(GIOChannel * ch, GIOCondition /* condition */, void * /* userData */)
 {
-    int sock;
-    if ((sock = socket(PF_NETLINK, SOCK_RAW, NETLINK_ROUTE)) == -1)
-    {
-        ChipLogError(DeviceLayer, "Failed to init netlink socket for ip addresses.");
-        return;
-    }
 
-    struct sockaddr_nl addr;
-    memset(&addr, 0, sizeof(addr));
-    addr.nl_family = AF_NETLINK;
-    addr.nl_groups = RTMGRP_IPV4_IFADDR;
+    char buffer[4096];
+    auto * header = reinterpret_cast<struct nlmsghdr *>(buffer);
+    ssize_t len;
 
-    if (bind(sock, (struct sockaddr *) &addr, sizeof(addr)) == -1)
+    if ((len = recv(g_io_channel_unix_get_fd(ch), buffer, sizeof(buffer), 0)) == -1)
     {
-        ChipLogError(DeviceLayer, "Failed to bind netlink socket for ip addresses.");
-        return;
+        if (errno == EINTR || errno == EAGAIN)
+            return G_SOURCE_CONTINUE;
+        ChipLogError(DeviceLayer, "Error reading from netlink socket: %d", errno);
+        return G_SOURCE_CONTINUE;
     }
 
-    ssize_t len;
-    char buffer[4096];
-    for (struct nlmsghdr * header = reinterpret_cast<struct nlmsghdr *>(buffer); (len = recv(sock, header, sizeof(buffer), 0)) > 0;)
+    if (len > 0)
     {
         for (struct nlmsghdr * messageHeader = header;
              (NLMSG_OK(messageHeader, static_cast<uint32_t>(len))) && (messageHeader->nlmsg_type != NLMSG_DONE);
@@ -154,23 +140,70 @@ void PlatformManagerImpl::WiFIIPChangeListener()
             }
         }
     }
+    else
+    {
+        ChipLogError(DeviceLayer, "EOF on netlink socket");
+        return G_SOURCE_REMOVE;
+    }
+
+    return G_SOURCE_CONTINUE;
 }
+
+// The temporary hack for getting IP address change on linux for network provisioning in the rendezvous session.
+// This should be removed or find a better place once we deprecate the rendezvous session.
+CHIP_ERROR RunWiFiIPChangeListener()
+{
+    int sock;
+    if ((sock = socket(PF_NETLINK, SOCK_RAW, NETLINK_ROUTE)) == -1)
+    {
+        ChipLogError(DeviceLayer, "Failed to init netlink socket for IP addresses: %d", errno);
+        return CHIP_ERROR_INTERNAL;
+    }
+
+    struct sockaddr_nl addr;
+    memset(&addr, 0, sizeof(addr));
+    addr.nl_family = AF_NETLINK;
+    addr.nl_groups = RTMGRP_IPV4_IFADDR;
+
+    if (bind(sock, (struct sockaddr *) &addr, sizeof(addr)) == -1)
+    {
+        ChipLogError(DeviceLayer, "Failed to bind netlink socket for IP addresses: %d", errno);
+        close(sock);
+        return CHIP_ERROR_INTERNAL;
+    }
+
+    GIOChannel * ch = g_io_channel_unix_new(sock);
+    g_io_add_watch_full(ch, G_PRIORITY_DEFAULT, G_IO_IN, WiFiIPChangeListener, nullptr, nullptr);
+
+    g_io_channel_set_close_on_unref(ch, TRUE);
+    g_io_channel_set_encoding(ch, nullptr, nullptr);
+    g_io_channel_unref(ch);
+
+    return CHIP_NO_ERROR;
+}
+
 #endif // #if CHIP_DEVICE_CONFIG_ENABLE_WIFI
 
+} // namespace
+
 CHIP_ERROR PlatformManagerImpl::_InitChipStack()
 {
-#if CHIP_WITH_GIO
-    GError * error = nullptr;
+#if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP
 
-    this->mpGDBusConnection = UniqueGDBusConnection(g_bus_get_sync(G_BUS_TYPE_SYSTEM, nullptr, &error));
+    mGLibMainLoop       = g_main_loop_new(nullptr, FALSE);
+    mGLibMainLoopThread = g_thread_new("gmain-matter", GLibMainLoopThread, mGLibMainLoop);
+
+    {
+        std::unique_lock<std::mutex> lock(mGLibMainLoopCallbackIndirectionMutex);
+        CallbackIndirection startedInd([](void *) { return G_SOURCE_REMOVE; }, nullptr);
+        g_idle_add(G_SOURCE_FUNC(&CallbackIndirection::Callback), &startedInd);
+        startedInd.Wait(lock);
+    }
 
-    std::thread gdbusThread(GDBus_Thread);
-    gdbusThread.detach();
 #endif
 
 #if CHIP_DEVICE_CONFIG_ENABLE_WIFI
-    std::thread wifiIPThread(WiFIIPChangeListener);
-    wifiIPThread.detach();
+    ReturnErrorOnFailure(RunWiFiIPChangeListener());
 #endif
 
     // Initialize the configuration system.
@@ -212,14 +245,64 @@ void PlatformManagerImpl::_Shutdown()
     }
 
     Internal::GenericPlatformManagerImpl_POSIX<PlatformManagerImpl>::_Shutdown();
+
+#if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP
+    g_main_loop_quit(mGLibMainLoop);
+    g_main_loop_unref(mGLibMainLoop);
+    g_thread_join(mGLibMainLoopThread);
+#endif
 }
 
-#if CHIP_WITH_GIO
-GDBusConnection * PlatformManagerImpl::GetGDBusConnection()
+#if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP
+
+void PlatformManagerImpl::CallbackIndirection::Wait(std::unique_lock<std::mutex> & lock)
 {
-    return this->mpGDBusConnection.get();
+    mDoneCond.wait(lock, [this]() { return mDone; });
 }
-#endif
+
+gboolean PlatformManagerImpl::CallbackIndirection::Callback(CallbackIndirection * self)
+{
+    // We can not access "self" before acquiring the lock, because TSAN will complain that
+    // there is a race condition between the thread that created the object and the thread
+    // that is executing the callback.
+    std::unique_lock<std::mutex> lock(PlatformMgrImpl().mGLibMainLoopCallbackIndirectionMutex);
+
+    auto callback = self->mCallback;
+    auto userData = self->mUserData;
+
+    lock.unlock();
+    auto result = callback(userData);
+    lock.lock();
+
+    self->mDone = true;
+    self->mDoneCond.notify_all();
+
+    return result;
+}
+
+#if CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE
+CHIP_ERROR PlatformManagerImpl::RunOnGLibMainLoopThread(GSourceFunc callback, void * userData, bool wait)
+{
+
+    GMainContext * context = g_main_loop_get_context(mGLibMainLoop);
+    VerifyOrReturnError(context != nullptr,
+                        (ChipLogDetail(DeviceLayer, "Failed to get GLib main loop context"), CHIP_ERROR_INTERNAL));
+
+    if (wait)
+    {
+        std::unique_lock<std::mutex> lock(mGLibMainLoopCallbackIndirectionMutex);
+        CallbackIndirection indirection(callback, userData);
+        g_main_context_invoke(context, G_SOURCE_FUNC(&CallbackIndirection::Callback), &indirection);
+        indirection.Wait(lock);
+        return CHIP_NO_ERROR;
+    }
+
+    g_main_context_invoke(context, callback, userData);
+    return CHIP_NO_ERROR;
+}
+#endif // CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE
+
+#endif // CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP
 
 } // namespace DeviceLayer
 } // namespace chip