From cf60a9a7f77f78d4cd9de08cdbe93e0236794a5b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?L=C3=A9o=20Lam?= Date: Tue, 25 Jun 2019 17:31:35 +0200 Subject: [PATCH] Use separate libusb contexts to avoid thread safety issues Unfortunately, it appears that using libusb's synchronous transfer API from several threads causes nasty race conditions in event handling and can lead to deadlocks, despite the fact that libusb's synchronous API is documented to be perfectly fine to use from several threads (only the manual polling functionality is supposed to require special precautions). Since usbdk was the only real reason for using a single libusb context and since usbdk (currently) has so many issues with Dolphin, I think dropping support for it in order to fix other backends is acceptable. --- Source/Core/Core/IOS/USB/Bluetooth/BTReal.cpp | 6 ++---- Source/Core/Core/IOS/USB/Bluetooth/BTReal.h | 2 ++ Source/Core/Core/IOS/USB/Host.cpp | 6 ++---- Source/Core/Core/IOS/USB/Host.h | 2 ++ Source/Core/Core/LibusbUtils.cpp | 6 ------ Source/Core/Core/LibusbUtils.h | 5 ----- Source/Core/InputCommon/GCAdapter.cpp | 13 +++++++------ Source/Core/UICommon/USBUtils.cpp | 2 +- 8 files changed, 16 insertions(+), 26 deletions(-) diff --git a/Source/Core/Core/IOS/USB/Bluetooth/BTReal.cpp b/Source/Core/Core/IOS/USB/Bluetooth/BTReal.cpp index 7ccd5c4fac7..b4322681d76 100644 --- a/Source/Core/Core/IOS/USB/Bluetooth/BTReal.cpp +++ b/Source/Core/Core/IOS/USB/Bluetooth/BTReal.cpp @@ -28,7 +28,6 @@ #include "Core/Core.h" #include "Core/HW/Memmap.h" #include "Core/IOS/Device.h" -#include "Core/LibusbUtils.h" #include "VideoCommon/OnScreenDisplay.h" namespace IOS::HLE::Device @@ -75,11 +74,10 @@ BluetoothReal::~BluetoothReal() IPCCommandResult BluetoothReal::Open(const OpenRequest& request) { - auto& context = LibusbUtils::GetContext(); - if (!context.IsValid()) + if (!m_context.IsValid()) return GetDefaultReply(IPC_EACCES); - context.GetDeviceList([this](libusb_device* device) { + m_context.GetDeviceList([this](libusb_device* device) { libusb_device_descriptor device_descriptor; libusb_get_device_descriptor(device, &device_descriptor); auto config_descriptor = LibusbUtils::MakeConfigDescriptor(device); diff --git a/Source/Core/Core/IOS/USB/Bluetooth/BTReal.h b/Source/Core/Core/IOS/USB/Bluetooth/BTReal.h index 5070cbee75f..a1986638ea0 100644 --- a/Source/Core/Core/IOS/USB/Bluetooth/BTReal.h +++ b/Source/Core/Core/IOS/USB/Bluetooth/BTReal.h @@ -19,6 +19,7 @@ #include "Core/IOS/USB/Bluetooth/BTBase.h" #include "Core/IOS/USB/Bluetooth/hci.h" #include "Core/IOS/USB/USBV0.h" +#include "Core/LibusbUtils.h" class PointerWrap; struct libusb_device; @@ -70,6 +71,7 @@ private: std::atomic m_sync_button_state{SyncButtonState::Unpressed}; Common::Timer m_sync_button_held_timer; + LibusbUtils::Context m_context; libusb_device* m_device = nullptr; libusb_device_handle* m_handle = nullptr; diff --git a/Source/Core/Core/IOS/USB/Host.cpp b/Source/Core/Core/IOS/USB/Host.cpp index 15308d2b4ec..710d26919c1 100644 --- a/Source/Core/Core/IOS/USB/Host.cpp +++ b/Source/Core/Core/IOS/USB/Host.cpp @@ -24,7 +24,6 @@ #include "Core/Core.h" #include "Core/IOS/USB/Common.h" #include "Core/IOS/USB/LibusbDevice.h" -#include "Core/LibusbUtils.h" namespace IOS::HLE::Device { @@ -121,10 +120,9 @@ bool USBHost::AddNewDevices(std::set& new_devices, DeviceChangeHooks& hooks if (SConfig::GetInstance().m_usb_passthrough_devices.empty()) return true; - auto& context = LibusbUtils::GetContext(); - if (context.IsValid()) + if (m_context.IsValid()) { - context.GetDeviceList([&](libusb_device* device) { + m_context.GetDeviceList([&](libusb_device* device) { libusb_device_descriptor descriptor; libusb_get_device_descriptor(device, &descriptor); const std::pair vid_pid = {descriptor.idVendor, descriptor.idProduct}; diff --git a/Source/Core/Core/IOS/USB/Host.h b/Source/Core/Core/IOS/USB/Host.h index 40e42193d27..d1d08640976 100644 --- a/Source/Core/Core/IOS/USB/Host.h +++ b/Source/Core/Core/IOS/USB/Host.h @@ -20,6 +20,7 @@ #include "Core/IOS/Device.h" #include "Core/IOS/IOS.h" #include "Core/IOS/USB/Common.h" +#include "Core/LibusbUtils.h" class PointerWrap; @@ -71,5 +72,6 @@ private: std::thread m_scan_thread; Common::Event m_first_scan_complete_event; bool m_has_initialised = false; + LibusbUtils::Context m_context; }; } // namespace IOS::HLE::Device diff --git a/Source/Core/Core/LibusbUtils.cpp b/Source/Core/Core/LibusbUtils.cpp index 2ee7838c334..a4083e2d06d 100644 --- a/Source/Core/Core/LibusbUtils.cpp +++ b/Source/Core/Core/LibusbUtils.cpp @@ -109,12 +109,6 @@ bool Context::GetDeviceList(GetDeviceListCallback callback) return m_impl->GetDeviceList(std::move(callback)); } -Context& GetContext() -{ - static Context s_context; - return s_context; -} - ConfigDescriptor MakeConfigDescriptor(libusb_device* device, u8 config_num) { #if defined(__LIBUSB__) diff --git a/Source/Core/Core/LibusbUtils.h b/Source/Core/Core/LibusbUtils.h index ded3bd9940f..487ba990602 100644 --- a/Source/Core/Core/LibusbUtils.h +++ b/Source/Core/Core/LibusbUtils.h @@ -38,11 +38,6 @@ private: std::unique_ptr m_impl; }; -// Use this to get a libusb context. Do *not* use any other context -// because some libusb backends such as UsbDk only work properly with a single context. -// Additionally, device lists must be retrieved using GetDeviceList for thread safety reasons. -Context& GetContext(); - using ConfigDescriptor = UniquePtr; ConfigDescriptor MakeConfigDescriptor(libusb_device* device, u8 config_num = 0); } // namespace LibusbUtils diff --git a/Source/Core/InputCommon/GCAdapter.cpp b/Source/Core/InputCommon/GCAdapter.cpp index 493d8003f2e..f54f85393c0 100644 --- a/Source/Core/InputCommon/GCAdapter.cpp +++ b/Source/Core/InputCommon/GCAdapter.cpp @@ -72,6 +72,8 @@ static bool s_libusb_hotplug_enabled = false; static libusb_hotplug_callback_handle s_hotplug_handle; #endif +static LibusbUtils::Context s_libusb_context; + static u8 s_endpoint_in = 0; static u8 s_endpoint_out = 0; @@ -147,7 +149,6 @@ static int HotplugCallback(libusb_context* ctx, libusb_device* dev, libusb_hotpl static void ScanThreadFunc() { - auto& context = LibusbUtils::GetContext(); Common::SetCurrentThreadName("GC Adapter Scanning Thread"); NOTICE_LOG(SERIALINTERFACE, "GC Adapter scanning thread started"); @@ -158,7 +159,7 @@ static void ScanThreadFunc() if (s_libusb_hotplug_enabled) { if (libusb_hotplug_register_callback( - context, + s_libusb_context, (libusb_hotplug_event)(LIBUSB_HOTPLUG_EVENT_DEVICE_ARRIVED | LIBUSB_HOTPLUG_EVENT_DEVICE_LEFT), LIBUSB_HOTPLUG_ENUMERATE, 0x057e, 0x0337, LIBUSB_HOTPLUG_MATCH_ANY, HotplugCallback, @@ -212,7 +213,7 @@ void StartScanThread() { if (s_adapter_detect_thread_running.IsSet()) return; - if (!LibusbUtils::GetContext().IsValid()) + if (!s_libusb_context.IsValid()) return; s_adapter_detect_thread_running.Set(true); s_adapter_detect_thread = std::thread(ScanThreadFunc); @@ -240,7 +241,7 @@ static void Setup() s_controller_rumble[i] = 0; } - LibusbUtils::GetContext().GetDeviceList([](libusb_device* device) { + s_libusb_context.GetDeviceList([](libusb_device* device) { if (CheckDeviceAccess(device)) { // Only connect to a single adapter in case the user has multiple connected @@ -364,8 +365,8 @@ void Shutdown() { StopScanThread(); #if defined(LIBUSB_API_VERSION) && LIBUSB_API_VERSION >= 0x01000102 - if (LibusbUtils::GetContext().IsValid() && s_libusb_hotplug_enabled) - libusb_hotplug_deregister_callback(LibusbUtils::GetContext(), s_hotplug_handle); + if (s_libusb_context.IsValid() && s_libusb_hotplug_enabled) + libusb_hotplug_deregister_callback(s_libusb_context, s_hotplug_handle); #endif Reset(); diff --git a/Source/Core/UICommon/USBUtils.cpp b/Source/Core/UICommon/USBUtils.cpp index 42cf815a125..fe7ad513442 100644 --- a/Source/Core/UICommon/USBUtils.cpp +++ b/Source/Core/UICommon/USBUtils.cpp @@ -35,7 +35,7 @@ std::map, std::string> GetInsertedDevices() std::map, std::string> devices; #ifdef __LIBUSB__ - auto& context = LibusbUtils::GetContext(); + LibusbUtils::Context context; if (!context.IsValid()) return devices;