Skip to content

Commit 0e41b19

Browse files
arkqandy31415
andauthored
[Linux] Run only one GLib main event loop (#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 <andy314@gmail.com>
1 parent 48860e2 commit 0e41b19

16 files changed

+329
-510
lines changed

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

+1-2
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,6 @@
22
#include <lib/support/CHIPMem.h>
33
#include <platform/CHIPDeviceLayer.h>
44
#include <platform/Linux/bluez/AdapterIterator.h>
5-
#include <platform/Linux/bluez/MainLoop.h>
65
#include <platform/internal/BLEManager.h>
76

87
using namespace chip::DeviceLayer::Internal;
@@ -58,7 +57,7 @@ extern "C" void * pychip_ble_adapter_list_get_raw_adapter(void * adapterIterator
5857

5958
namespace {
6059

61-
// To avoid pythoon compatibility issues on inc/dec references,
60+
// To avoid python compatibility issues on inc/dec references,
6261
// code assumes an abstract type and leaves it up to python to keep track of
6362
// reference counts
6463
struct PyObject;

src/platform/BUILD.gn

-2
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,6 @@ if (chip_device_platform != "none" && chip_device_platform != "external") {
9191
header = "CHIPDeviceBuildConfig.h"
9292
header_dir = "platform"
9393

94-
chip_with_gio = chip_enable_wifi
9594
chip_device_config_enable_wpa =
9695
chip_enable_wifi && chip_device_platform != "darwin"
9796
chip_stack_lock_tracking_log = chip_stack_lock_tracking != "none"
@@ -100,7 +99,6 @@ if (chip_device_platform != "none" && chip_device_platform != "external") {
10099
"CHIP_DEVICE_CONFIG_ENABLE_WPA=${chip_device_config_enable_wpa}",
101100
"CHIP_ENABLE_OPENTHREAD=${chip_enable_openthread}",
102101
"CHIP_DEVICE_CONFIG_THREAD_FTD=${chip_device_config_thread_ftd}",
103-
"CHIP_WITH_GIO=${chip_with_gio}",
104102
"CHIP_STACK_LOCK_TRACKING_ENABLED=${chip_stack_lock_tracking_log}",
105103
"CHIP_STACK_LOCK_TRACKING_ERROR_FATAL=${chip_stack_lock_tracking_fatal}",
106104
"CHIP_ENABLE_ADDITIONAL_DATA_ADVERTISING=${chip_enable_additional_data_advertising}",

src/platform/Linux/BLEManagerImpl.cpp

+31-5
Original file line numberDiff line numberDiff line change
@@ -97,6 +97,9 @@ void BLEManagerImpl::_Shutdown()
9797
{
9898
// ensure scan resources are cleared (e.g. timeout timers)
9999
mDeviceScanner.reset();
100+
// Release BLE connection resources (unregister from BlueZ).
101+
ShutdownBluezBleLayer(mpEndpoint);
102+
mFlags.Clear(Flags::kBluezBLELayerInitialized);
100103
}
101104

102105
CHIP_ERROR BLEManagerImpl::_SetAdvertisingEnabled(bool val)
@@ -348,7 +351,10 @@ bool BLEManagerImpl::SubscribeCharacteristic(BLE_CONNECTION_OBJECT conId, const
348351
VerifyOrExit(Ble::UUIDsMatch(charId, &ChipUUID_CHIPoBLEChar_TX),
349352
ChipLogError(DeviceLayer, "SubscribeCharacteristic() called with invalid characteristic ID"));
350353

351-
result = BluezSubscribeCharacteristic(conId);
354+
VerifyOrExit(BluezSubscribeCharacteristic(conId) == CHIP_NO_ERROR,
355+
ChipLogError(DeviceLayer, "BluezSubscribeCharacteristic() failed"));
356+
result = true;
357+
352358
exit:
353359
return result;
354360
}
@@ -362,21 +368,38 @@ bool BLEManagerImpl::UnsubscribeCharacteristic(BLE_CONNECTION_OBJECT conId, cons
362368
VerifyOrExit(Ble::UUIDsMatch(charId, &ChipUUID_CHIPoBLEChar_TX),
363369
ChipLogError(DeviceLayer, "UnsubscribeCharacteristic() called with invalid characteristic ID"));
364370

365-
result = BluezUnsubscribeCharacteristic(conId);
371+
VerifyOrExit(BluezUnsubscribeCharacteristic(conId) == CHIP_NO_ERROR,
372+
ChipLogError(DeviceLayer, "BluezUnsubscribeCharacteristic() failed"));
373+
result = true;
374+
366375
exit:
367376
return result;
368377
}
369378

370379
bool BLEManagerImpl::CloseConnection(BLE_CONNECTION_OBJECT conId)
371380
{
381+
bool result = false;
382+
372383
ChipLogProgress(DeviceLayer, "Closing BLE GATT connection (con %p)", conId);
373-
return CloseBluezConnection(conId);
384+
385+
VerifyOrExit(CloseBluezConnection(conId) == CHIP_NO_ERROR, ChipLogError(DeviceLayer, "CloseBluezConnection() failed"));
386+
result = true;
387+
388+
exit:
389+
return result;
374390
}
375391

376392
bool BLEManagerImpl::SendIndication(BLE_CONNECTION_OBJECT conId, const ChipBleUUID * svcId, const Ble::ChipBleUUID * charId,
377393
chip::System::PacketBufferHandle pBuf)
378394
{
379-
return SendBluezIndication(conId, std::move(pBuf));
395+
bool result = false;
396+
397+
VerifyOrExit(SendBluezIndication(conId, std::move(pBuf)) == CHIP_NO_ERROR,
398+
ChipLogError(DeviceLayer, "SendBluezIndication() failed"));
399+
result = true;
400+
401+
exit:
402+
return result;
380403
}
381404

382405
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
389412
VerifyOrExit(Ble::UUIDsMatch(charId, &ChipUUID_CHIPoBLEChar_RX),
390413
ChipLogError(DeviceLayer, "SendWriteRequest() called with invalid characteristic ID"));
391414

392-
result = BluezSendWriteRequest(conId, std::move(pBuf));
415+
VerifyOrExit(BluezSendWriteRequest(conId, std::move(pBuf)) == CHIP_NO_ERROR,
416+
ChipLogError(DeviceLayer, "BluezSendWriteRequest() failed"));
417+
result = true;
418+
393419
exit:
394420
return result;
395421
}

src/platform/Linux/BUILD.gn

-2
Original file line numberDiff line numberDiff line change
@@ -91,8 +91,6 @@ static_library("Linux") {
9191
"bluez/ChipDeviceScanner.h",
9292
"bluez/Helper.cpp",
9393
"bluez/Helper.h",
94-
"bluez/MainLoop.cpp",
95-
"bluez/MainLoop.h",
9694
"bluez/Types.h",
9795
]
9896
}

src/platform/Linux/CHIPDevicePlatformConfig.h

+8
Original file line numberDiff line numberDiff line change
@@ -41,6 +41,14 @@
4141
#define CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE 0
4242
#endif
4343

44+
// Start GLib main event loop if BLE or WiFi is enabled. This is needed to handle
45+
// D-Bus communication with BlueZ or wpa_supplicant.
46+
#if CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE || CHIP_DEVICE_CONFIG_ENABLE_WIFI
47+
#define CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP 1
48+
#else
49+
#define CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP 0
50+
#endif
51+
4452
// ========== Platform-specific Configuration =========
4553

4654
// These are configuration options that are unique to Linux platforms.

src/platform/Linux/PlatformManagerImpl.cpp

+134-51
Original file line numberDiff line numberDiff line change
@@ -24,6 +24,17 @@
2424

2525
#include <platform/internal/CHIPDeviceLayerInternal.h>
2626

27+
#include <arpa/inet.h>
28+
#include <dirent.h>
29+
#include <errno.h>
30+
#include <linux/netlink.h>
31+
#include <linux/rtnetlink.h>
32+
#include <net/if.h>
33+
#include <netinet/in.h>
34+
#include <unistd.h>
35+
36+
#include <mutex>
37+
2738
#include <app-common/zap-generated/enums.h>
2839
#include <app-common/zap-generated/ids/Events.h>
2940
#include <lib/support/CHIPMem.h>
@@ -35,22 +46,6 @@
3546
#include <platform/PlatformManager.h>
3647
#include <platform/internal/GenericPlatformManagerImpl_POSIX.ipp>
3748

38-
#include <thread>
39-
40-
#include <arpa/inet.h>
41-
#include <dirent.h>
42-
#include <errno.h>
43-
#include <linux/netlink.h>
44-
#include <linux/rtnetlink.h>
45-
#include <net/if.h>
46-
#include <netinet/in.h>
47-
#include <unistd.h>
48-
49-
#if __GLIBC__ == 2 && __GLIBC_MINOR__ < 30
50-
#include <sys/syscall.h>
51-
#define gettid() syscall(SYS_gettid)
52-
#endif
53-
5449
using namespace ::chip::app::Clusters;
5550

5651
namespace chip {
@@ -60,41 +55,32 @@ PlatformManagerImpl PlatformManagerImpl::sInstance;
6055

6156
namespace {
6257

63-
#if CHIP_WITH_GIO
64-
void GDBus_Thread()
58+
#if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP
59+
void * GLibMainLoopThread(void * loop)
6560
{
66-
GMainLoop * loop = g_main_loop_new(nullptr, false);
67-
68-
g_main_loop_run(loop);
69-
g_main_loop_unref(loop);
61+
g_main_loop_run(static_cast<GMainLoop *>(loop));
62+
return nullptr;
7063
}
7164
#endif
72-
} // namespace
7365

7466
#if CHIP_DEVICE_CONFIG_ENABLE_WIFI
75-
void PlatformManagerImpl::WiFIIPChangeListener()
67+
68+
gboolean WiFiIPChangeListener(GIOChannel * ch, GIOCondition /* condition */, void * /* userData */)
7669
{
77-
int sock;
78-
if ((sock = socket(PF_NETLINK, SOCK_RAW, NETLINK_ROUTE)) == -1)
79-
{
80-
ChipLogError(DeviceLayer, "Failed to init netlink socket for ip addresses.");
81-
return;
82-
}
8370

84-
struct sockaddr_nl addr;
85-
memset(&addr, 0, sizeof(addr));
86-
addr.nl_family = AF_NETLINK;
87-
addr.nl_groups = RTMGRP_IPV4_IFADDR;
71+
char buffer[4096];
72+
auto * header = reinterpret_cast<struct nlmsghdr *>(buffer);
73+
ssize_t len;
8874

89-
if (bind(sock, (struct sockaddr *) &addr, sizeof(addr)) == -1)
75+
if ((len = recv(g_io_channel_unix_get_fd(ch), buffer, sizeof(buffer), 0)) == -1)
9076
{
91-
ChipLogError(DeviceLayer, "Failed to bind netlink socket for ip addresses.");
92-
return;
77+
if (errno == EINTR || errno == EAGAIN)
78+
return G_SOURCE_CONTINUE;
79+
ChipLogError(DeviceLayer, "Error reading from netlink socket: %d", errno);
80+
return G_SOURCE_CONTINUE;
9381
}
9482

95-
ssize_t len;
96-
char buffer[4096];
97-
for (struct nlmsghdr * header = reinterpret_cast<struct nlmsghdr *>(buffer); (len = recv(sock, header, sizeof(buffer), 0)) > 0;)
83+
if (len > 0)
9884
{
9985
for (struct nlmsghdr * messageHeader = header;
10086
(NLMSG_OK(messageHeader, static_cast<uint32_t>(len))) && (messageHeader->nlmsg_type != NLMSG_DONE);
@@ -154,23 +140,70 @@ void PlatformManagerImpl::WiFIIPChangeListener()
154140
}
155141
}
156142
}
143+
else
144+
{
145+
ChipLogError(DeviceLayer, "EOF on netlink socket");
146+
return G_SOURCE_REMOVE;
147+
}
148+
149+
return G_SOURCE_CONTINUE;
157150
}
151+
152+
// The temporary hack for getting IP address change on linux for network provisioning in the rendezvous session.
153+
// This should be removed or find a better place once we deprecate the rendezvous session.
154+
CHIP_ERROR RunWiFiIPChangeListener()
155+
{
156+
int sock;
157+
if ((sock = socket(PF_NETLINK, SOCK_RAW, NETLINK_ROUTE)) == -1)
158+
{
159+
ChipLogError(DeviceLayer, "Failed to init netlink socket for IP addresses: %d", errno);
160+
return CHIP_ERROR_INTERNAL;
161+
}
162+
163+
struct sockaddr_nl addr;
164+
memset(&addr, 0, sizeof(addr));
165+
addr.nl_family = AF_NETLINK;
166+
addr.nl_groups = RTMGRP_IPV4_IFADDR;
167+
168+
if (bind(sock, (struct sockaddr *) &addr, sizeof(addr)) == -1)
169+
{
170+
ChipLogError(DeviceLayer, "Failed to bind netlink socket for IP addresses: %d", errno);
171+
close(sock);
172+
return CHIP_ERROR_INTERNAL;
173+
}
174+
175+
GIOChannel * ch = g_io_channel_unix_new(sock);
176+
g_io_add_watch_full(ch, G_PRIORITY_DEFAULT, G_IO_IN, WiFiIPChangeListener, nullptr, nullptr);
177+
178+
g_io_channel_set_close_on_unref(ch, TRUE);
179+
g_io_channel_set_encoding(ch, nullptr, nullptr);
180+
g_io_channel_unref(ch);
181+
182+
return CHIP_NO_ERROR;
183+
}
184+
158185
#endif // #if CHIP_DEVICE_CONFIG_ENABLE_WIFI
159186

187+
} // namespace
188+
160189
CHIP_ERROR PlatformManagerImpl::_InitChipStack()
161190
{
162-
#if CHIP_WITH_GIO
163-
GError * error = nullptr;
191+
#if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP
164192

165-
this->mpGDBusConnection = UniqueGDBusConnection(g_bus_get_sync(G_BUS_TYPE_SYSTEM, nullptr, &error));
193+
mGLibMainLoop = g_main_loop_new(nullptr, FALSE);
194+
mGLibMainLoopThread = g_thread_new("gmain-matter", GLibMainLoopThread, mGLibMainLoop);
195+
196+
{
197+
std::unique_lock<std::mutex> lock(mGLibMainLoopCallbackIndirectionMutex);
198+
CallbackIndirection startedInd([](void *) { return G_SOURCE_REMOVE; }, nullptr);
199+
g_idle_add(G_SOURCE_FUNC(&CallbackIndirection::Callback), &startedInd);
200+
startedInd.Wait(lock);
201+
}
166202

167-
std::thread gdbusThread(GDBus_Thread);
168-
gdbusThread.detach();
169203
#endif
170204

171205
#if CHIP_DEVICE_CONFIG_ENABLE_WIFI
172-
std::thread wifiIPThread(WiFIIPChangeListener);
173-
wifiIPThread.detach();
206+
ReturnErrorOnFailure(RunWiFiIPChangeListener());
174207
#endif
175208

176209
// Initialize the configuration system.
@@ -212,14 +245,64 @@ void PlatformManagerImpl::_Shutdown()
212245
}
213246

214247
Internal::GenericPlatformManagerImpl_POSIX<PlatformManagerImpl>::_Shutdown();
248+
249+
#if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP
250+
g_main_loop_quit(mGLibMainLoop);
251+
g_main_loop_unref(mGLibMainLoop);
252+
g_thread_join(mGLibMainLoopThread);
253+
#endif
215254
}
216255

217-
#if CHIP_WITH_GIO
218-
GDBusConnection * PlatformManagerImpl::GetGDBusConnection()
256+
#if CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP
257+
258+
void PlatformManagerImpl::CallbackIndirection::Wait(std::unique_lock<std::mutex> & lock)
219259
{
220-
return this->mpGDBusConnection.get();
260+
mDoneCond.wait(lock, [this]() { return mDone; });
221261
}
222-
#endif
262+
263+
gboolean PlatformManagerImpl::CallbackIndirection::Callback(CallbackIndirection * self)
264+
{
265+
// We can not access "self" before acquiring the lock, because TSAN will complain that
266+
// there is a race condition between the thread that created the object and the thread
267+
// that is executing the callback.
268+
std::unique_lock<std::mutex> lock(PlatformMgrImpl().mGLibMainLoopCallbackIndirectionMutex);
269+
270+
auto callback = self->mCallback;
271+
auto userData = self->mUserData;
272+
273+
lock.unlock();
274+
auto result = callback(userData);
275+
lock.lock();
276+
277+
self->mDone = true;
278+
self->mDoneCond.notify_all();
279+
280+
return result;
281+
}
282+
283+
#if CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE
284+
CHIP_ERROR PlatformManagerImpl::RunOnGLibMainLoopThread(GSourceFunc callback, void * userData, bool wait)
285+
{
286+
287+
GMainContext * context = g_main_loop_get_context(mGLibMainLoop);
288+
VerifyOrReturnError(context != nullptr,
289+
(ChipLogDetail(DeviceLayer, "Failed to get GLib main loop context"), CHIP_ERROR_INTERNAL));
290+
291+
if (wait)
292+
{
293+
std::unique_lock<std::mutex> lock(mGLibMainLoopCallbackIndirectionMutex);
294+
CallbackIndirection indirection(callback, userData);
295+
g_main_context_invoke(context, G_SOURCE_FUNC(&CallbackIndirection::Callback), &indirection);
296+
indirection.Wait(lock);
297+
return CHIP_NO_ERROR;
298+
}
299+
300+
g_main_context_invoke(context, callback, userData);
301+
return CHIP_NO_ERROR;
302+
}
303+
#endif // CHIP_DEVICE_CONFIG_ENABLE_CHIPOBLE
304+
305+
#endif // CHIP_DEVICE_CONFIG_WITH_GLIB_MAIN_LOOP
223306

224307
} // namespace DeviceLayer
225308
} // namespace chip

0 commit comments

Comments
 (0)