ControllerInterface: fix UpdateReferences() deadlock

Removed useless locks to DeviceContainer::m_devices_mutex, as they were all already protected by m_devices_population_mutex.
We have no interest in blocking other threads that were potentially reading devices at the same time so this seems fine.
This simplifies the code, and I've adjusted a few comments which mentioned possible deadlock that should now be totally gone.

The deadlock could have happen if a thread directly called EmulatedController::UpdateReferences(), while another another thread also reached EmulatedController::UpdateReferences() within a call to ControllerInterface::UpdateDevices(), as the mentioned function locked both the DeviceContainer::m_devices_mutex and s_get_state_mutex at the same time.

The deadlock was frequent on game emulation startup on Android, due to the UpdateReferences() call in InputConfig::LoadConfig() and the UI thread triggering calls to ControllerInterface::UpdateDevices().
It could also have happened on Desktop if a user pressed "Refresh Devices" manually in the UI while the input config was loading.

Also brought some UpdateReferences() comments and thread safety fixes from https://github.com/dolphin-emu/dolphin/pull/9489

1 files changed, 17 insertions, 34 deletions

diff --git a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp
index 6a9cfc25fd..aaaa3971e6 100644
--- a/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp
+++ b/Source/Core/InputCommon/ControllerInterface/ControllerInterface.cpp
@@ -54,8 +54,6 @@ void ControllerInterface::Initialize(const WindowSystemInfo& wsi)
 
   m_populating_devices_counter = 1;
 
-  m_devices_mutex.lock();
-
 #ifdef CIFACE_USE_WIN32
   ciface::Win32::Init(wsi.render_window);
 #endif
@@ -87,8 +85,9 @@ void ControllerInterface::Initialize(const WindowSystemInfo& wsi)
 
   RefreshDevices();
 
+  // Devices writes are already protected by m_devices_population_mutex but this won't hurt
+  m_devices_mutex.lock();
   const bool devices_empty = m_devices.empty();
-
   m_devices_mutex.unlock();
 
   if (m_populating_devices_counter.fetch_sub(1) == 1 && !devices_empty)
@@ -121,17 +120,17 @@ void ControllerInterface::RefreshDevices(RefreshReason reason)
     std::lock_guard lk_pre_population(m_pre_population_mutex);
     // This is needed to stop its threads before locking our mutexes, to avoid deadlocks
     // (in case it tried to add a device after we had locked m_devices_population_mutex).
-    // There doesn't seem to be an easy to way to repopulate OSX devices without restarting
-    // its hotplug thread. This will not release its devices, that's still done below.
+    // There doesn't seem to be an easy to way to repopulate OSX devices without restarting its
+    // hotplug thread. This should not remove its devices, and if it did, calls should be ignored.
     ciface::OSX::DeInit();
   }
 #endif
 
-  // This lock has two main functions:
-  // -Avoid a deadlock between m_devices_mutex and ControllerEmu::s_state_mutex when
-  // InvokeDevicesChangedCallbacks() is called concurrently by two different threads.
-  // -Avoid devices being destroyed while others of the same type are being created.
-  // This wasn't thread safe in multiple device sources.
+  // We lock m_devices_population_mutex here to make everything simpler.
+  // Multiple devices classes have their own "hotplug" thread, and can add/remove devices at any
+  // time, while actual writes to "m_devices" are safe, the order in which they happen is not. That
+  // means a thread could be adding devices while we are removing them, or removing them as we are
+  // populating them (causing missing or duplicate devices).
   std::lock_guard lk_population(m_devices_population_mutex);
 
 #if defined(CIFACE_USE_WIN32) && !defined(CIFACE_USE_XLIB) && !defined(CIFACE_USE_OSX)
@@ -141,12 +140,9 @@ void ControllerInterface::RefreshDevices(RefreshReason reason)
   {
     m_populating_devices_counter.fetch_add(1);
 
-    {
-      std::lock_guard lk(m_devices_mutex);
-      // No need to do anything else in this case.
-      // Only (Win32) DInput needs the window handle to be updated.
-      ciface::Win32::ChangeWindow(m_wsi.render_window);
-    }
+    // No need to do anything else in this case.
+    // Only (Win32) DInput needs the window handle to be updated.
+    ciface::Win32::ChangeWindow(m_wsi.render_window);
 
     if (m_populating_devices_counter.fetch_sub(1) == 1)
       InvokeDevicesChangedCallbacks();
@@ -156,13 +152,6 @@ void ControllerInterface::RefreshDevices(RefreshReason reason)
 
   m_populating_devices_counter.fetch_add(1);
 
-  // We lock m_devices_mutex here to make everything simpler.
-  // Multiple devices classes have their own "hotplug" thread, and can add/remove devices at any
-  // time, while actual writes to "m_devices" are safe, the order in which they happen is not. That
-  // means a thread could be adding devices while we are removing them, or removing them as we are
-  // populating them (causing missing or duplicate devices).
-  m_devices_mutex.lock();
-
   // Make sure shared_ptr<Device> objects are released before repopulating.
   ClearDevices();
 
@@ -206,8 +195,6 @@ void ControllerInterface::RefreshDevices(RefreshReason reason)
 
   WiimoteReal::PopulateDevices();
 
-  m_devices_mutex.unlock();
-
   if (m_populating_devices_counter.fetch_sub(1) == 1)
     InvokeDevicesChangedCallbacks();
 }
@@ -221,11 +208,7 @@ void ControllerInterface::PlatformPopulateDevices(std::function<void()> callback
 
   m_populating_devices_counter.fetch_add(1);
 
-  {
-    std::lock_guard lk(m_devices_mutex);
-
-    callback();
-  }
+  callback();
 
   if (m_populating_devices_counter.fetch_sub(1) == 1)
     InvokeDevicesChangedCallbacks();
@@ -270,8 +253,8 @@ void ControllerInterface::Shutdown()
 
   // Make sure no devices had been added within Shutdown() in the time
   // between checking they checked atomic m_is_init bool and we changed it.
-  // We couldn't have locked m_devices_mutex nor m_devices_population_mutex for the whole Shutdown()
-  // as they could cause deadlocks. Note that this is still not 100% safe as some backends are
+  // We couldn't have locked m_devices_population_mutex for the whole Shutdown()
+  // as it could cause deadlocks. Note that this is still not 100% safe as some backends are
   // shut down in other places, possibly adding devices after we have shut down, but the chances of
   // that happening are basically zero.
   ClearDevices();
@@ -294,8 +277,8 @@ void ControllerInterface::ClearDevices()
         o->SetState(0);
     }
 
-    // Devices will still be alive after this: there are shared ptrs around the code holding them,
-    // but InvokeDevicesChangedCallbacks() will clean all of them.
+    // Devices could still be alive after this as there might be shared ptrs around holding them.
+    // The InvokeDevicesChangedCallbacks() underneath should always clean all of them (it needs to).
     m_devices.clear();
   }