Merge pull request #380 from PixlOne/fix-wakeup-memleak

src/logid/Device.cpp

@@ -184,14 +184,15 @@ void Device::sleep() {
 
 void Device::wakeup() {
     std::lock_guard<std::mutex> lock(_state_lock);
-    logPrintf(INFO, "%s:%d woke up.", _path.c_str(), _index);
 
     reconfigure();
 
     if (!_awake) {
         _awake = true;
         _ipc_interface->notifyStatus();
     }
+
+    logPrintf(INFO, "%s:%d woke up.", _path.c_str(), _index);
 }
 
 void Device::reconfigure() {

src/logid/backend/EventHandlerList.h

@@ -24,21 +24,64 @@
 #include <mutex>
 #include <shared_mutex>
 #include <list>
+#include <atomic>
 
 template <class T>
 class EventHandlerLock;
 
 template <class T>
-struct EventHandlerList {
-    typedef std::list<typename T::EventHandler> list_t;
-    typedef typename list_t::const_iterator iterator_t;
+class EventHandlerList {
+public:
+    typedef std::list<std::pair<typename T::EventHandler, std::atomic_bool>> list_t;
+    typedef typename list_t::iterator iterator_t;
+private:
+    list_t list;
+    std::shared_mutex mutex;
+    std::shared_mutex add_mutex;
+
+    void cleanup() {
+        std::unique_lock lock(mutex, std::try_to_lock);
+        if (lock.owns_lock()) {
+            std::list<iterator_t> to_remove;
+            for (auto it = list.begin(); it != list.end(); ++it) {
+                if (!it->second)
+                    to_remove.push_back(it);
+            }
 
-    std::list<typename T::EventHandler> list;
-    mutable std::shared_mutex mutex;
+            for(auto& it : to_remove)
+                list.erase(it);
+        }
+    }
+public:
+    iterator_t add(typename T::EventHandler handler) {
+        std::unique_lock add_lock(add_mutex);
+        list.emplace_front(std::move(handler), true);
+        return list.begin();
+    }
 
     void remove(iterator_t iterator) {
-        std::unique_lock lock(mutex);
-        list.erase(iterator);
+        std::unique_lock lock(mutex, std::try_to_lock);
+        if (lock.owns_lock()) {
+            std::unique_lock add_lock(add_mutex);
+            list.erase(iterator);
+        } else {
+            iterator->second = false;
+        }
+    }
+
+    template <typename Arg>
+    void run_all(Arg arg) {
+        cleanup();
+        std::shared_lock lock(mutex);
+        std::shared_lock add_lock(add_mutex);
+        for (auto& handler : list) {
+            add_lock.unlock();
+            if (handler.second) {
+                if (handler.first.condition(arg))
+                    handler.first.callback(arg);
+            }
+            add_lock.lock();
+        }
     }
 };
 

src/logid/backend/hidpp/Device.cpp

@@ -114,35 +114,8 @@ void Device::_setupReportsAndInit() {
     /* hid_logitech_dj creates virtual /dev/hidraw nodes for receiver
      * devices. We should ignore these devices.
      */
-    if (_index == hidpp::DefaultDevice) {
-        _raw_handler = _raw_device->addEventHandler(
-                {[](const std::vector<uint8_t>& report) -> bool {
-                    return (report[Offset::Type] == Report::Type::Short ||
-                            report[Offset::Type] == Report::Type::Long);
-                },
-                 [self_weak = _self](const std::vector<uint8_t>& report) -> void {
-                     Report _report(report);
-                     if(auto self = self_weak.lock())
-                        self->handleEvent(_report);
-                 }});
-
-        try {
-            auto rsp = sendReport({ReportType::Short, _index,
-                                   hidpp20::FeatureID::ROOT, hidpp20::Root::Ping,
-                                   hidpp::softwareID});
-            if (rsp.deviceIndex() != _index)
-                throw InvalidDevice(InvalidDevice::VirtualNode);
-        } catch (hidpp10::Error& e) {
-            if (e.deviceIndex() != _index)
-                throw InvalidDevice(InvalidDevice::VirtualNode);
-        } catch (hidpp20::Error& e) {
-            /* This shouldn't happen, the device must not be ready */
-            if (e.deviceIndex() != _index)
-                throw InvalidDevice(InvalidDevice::VirtualNode);
-            else
-                throw DeviceNotReady();
-        }
-    }
+    if (_raw_device->isSubDevice())
+        throw InvalidDevice(InvalidDevice::VirtualNode);
 
     _raw_handler = _raw_device->addEventHandler(
             {[index = _index](
@@ -214,25 +187,21 @@ void Device::_init() {
 }
 
 EventHandlerLock<Device> Device::addEventHandler(EventHandler handler) {
-    std::unique_lock lock(_event_handlers->mutex);
-    _event_handlers->list.emplace_front(std::move(handler));
-    return {_event_handlers, _event_handlers->list.cbegin()};
+    return {_event_handlers, _event_handlers->add(std::move(handler))};
 }
 
 void Device::handleEvent(Report& report) {
     if (responseReport(report))
         return;
 
-    std::shared_lock lock(_event_handlers->mutex);
-    for (auto& handler: _event_handlers->list)
-        if (handler.condition(report))
-            handler.callback(report);
+    _event_handlers->run_all(report);
 }
 
 Report Device::sendReport(const Report& report) {
     /* Must complete transaction before next send */
     std::lock_guard send_lock(_send_mutex);
     _sent_sub_id = report.subId();
+    _sent_address = report.address();
     std::unique_lock lock(_response_mutex);
     _sendReport(report);
 
@@ -249,6 +218,7 @@ Report Device::sendReport(const Report& report) {
     Response response = _response.value();
     _response.reset();
     _sent_sub_id.reset();
+    _sent_address.reset();
 
     if (std::holds_alternative<Report>(response)) {
         return std::get<Report>(response);
@@ -268,20 +238,23 @@ bool Device::responseReport(const Report& report) {
     std::lock_guard lock(_response_mutex);
     Response response = report;
     uint8_t sub_id;
+    uint8_t address;
 
     Report::Hidpp10Error hidpp10_error{};
     Report::Hidpp20Error hidpp20_error{};
     if (report.isError10(hidpp10_error)) {
         sub_id = hidpp10_error.sub_id;
+        address = hidpp10_error.address;
         response = hidpp10_error;
     } else if (report.isError20(hidpp20_error)) {
         sub_id = hidpp20_error.feature_index;
-        response = hidpp20_error;
+        address = (hidpp20_error.function << 4) | (hidpp20_error.software_id & 0x0f);
     } else {
         sub_id = report.subId();
+        address = report.address();
     }
 
-    if (sub_id == _sent_sub_id) {
+    if (sub_id == _sent_sub_id && address == _sent_address) {
         _response = response;
         _response_cv.notify_all();
         return true;

src/logid/backend/hidpp/Device.h

@@ -158,6 +158,7 @@ namespace logid::backend::hidpp {
 
         std::optional<Response> _response;
         std::optional<uint8_t> _sent_sub_id{};
+        std::optional<uint8_t> _sent_address{};
 
         std::shared_ptr<EventHandlerList<Device>> _event_handlers;
 

src/logid/backend/hidpp/defs.h

@@ -42,7 +42,7 @@ namespace logid::backend::hidpp {
 
     static constexpr uint8_t softwareID = 2;
     /* For sending reports with no response, use a different SW ID */
-    static constexpr uint8_t noAckSoftwareID = 1;
+    static constexpr uint8_t noAckSoftwareID = 3;
 
     static constexpr std::size_t ShortParamLength = 3;
     static constexpr std::size_t LongParamLength = 16;

src/logid/backend/hidpp10/Device.cpp

@@ -24,6 +24,23 @@
 using namespace logid::backend;
 using namespace logid::backend::hidpp10;
 
+hidpp::Report setupRegReport(hidpp::DeviceIndex index,
+                             uint8_t sub_id, uint8_t address,
+                             const std::vector<uint8_t>& params) {
+    hidpp::Report::Type type = params.size() <= hidpp::ShortParamLength ?
+                               hidpp::Report::Type::Short : hidpp::Report::Type::Long;
+
+    if (sub_id == SetRegisterLong) {
+        // When setting a long register, the report must be long.
+        type = hidpp::Report::Type::Long;
+    }
+
+    hidpp::Report request(type, index, sub_id, address);
+    std::copy(params.begin(), params.end(), request.paramBegin());
+
+    return request;
+}
+
 Device::Device(const std::string& path, hidpp::DeviceIndex index,
                const std::shared_ptr<raw::DeviceMonitor>& monitor, double timeout) :
         hidpp::Device(path, index, monitor, timeout) {
@@ -124,22 +141,24 @@ std::vector<uint8_t> Device::setRegister(uint8_t address,
     return accessRegister(sub_id, address, params);
 }
 
-std::vector<uint8_t> Device::accessRegister(uint8_t sub_id, uint8_t address,
-                                            const std::vector<uint8_t>& params) {
-    hidpp::Report::Type type = params.size() <= hidpp::ShortParamLength ?
-                               hidpp::Report::Type::Short : hidpp::Report::Type::Long;
+void Device::setRegisterNoResponse(uint8_t address,
+                                   const std::vector<uint8_t>& params,
+                                   hidpp::Report::Type type) {
+    assert(params.size() <= hidpp::LongParamLength);
 
-    if (sub_id == SetRegisterLong) {
-        // When setting a long register, the report must be long.
-        type = hidpp::Report::Type::Long;
-    }
+    uint8_t sub_id = type == hidpp::Report::Type::Short ?
+                     SetRegisterShort : SetRegisterLong;
 
-    hidpp::Report request(type, deviceIndex(), sub_id, address);
-    std::copy(params.begin(), params.end(), request.paramBegin());
+    return accessRegisterNoResponse(sub_id, address, params);
+}
 
-    auto response = sendReport(request);
+std::vector<uint8_t> Device::accessRegister(uint8_t sub_id, uint8_t address,
+                                            const std::vector<uint8_t>& params) {
+    auto response = sendReport(setupRegReport(deviceIndex(), sub_id, address, params));
     return {response.paramBegin(), response.paramEnd()};
 }
 
-
-
+void Device::accessRegisterNoResponse(uint8_t sub_id, uint8_t address,
+                                      const std::vector<uint8_t>& params) {
+    sendReportNoACK(setupRegReport(deviceIndex(), sub_id, address, params));
+}

src/logid/backend/hidpp10/Device.h

@@ -39,6 +39,9 @@ namespace logid::backend::hidpp10 {
                                          const std::vector<uint8_t>& params,
                                          hidpp::Report::Type type);
 
+        void setRegisterNoResponse(uint8_t address, const std::vector<uint8_t>& params,
+                                   hidpp::Report::Type type);
+
     protected:
         Device(const std::string& path, hidpp::DeviceIndex index,
                const std::shared_ptr<raw::DeviceMonitor>& monitor, double timeout);
@@ -53,18 +56,24 @@ namespace logid::backend::hidpp10 {
 
     private:
         typedef std::variant<hidpp::Report, hidpp::Report::Hidpp10Error> Response;
+
         struct ResponseSlot {
             std::optional<Response> response;
             std::optional<uint8_t> sub_id;
+
             void reset();
         };
+
         std::array<ResponseSlot, SubIDCount> _responses;
 
         std::vector<uint8_t> accessRegister(
                 uint8_t sub_id, uint8_t address, const std::vector<uint8_t>& params);
 
+        void accessRegisterNoResponse(
+                uint8_t sub_id, uint8_t address, const std::vector<uint8_t>& params);
+
     protected:
-        template <typename T, typename... Args>
+        template<typename T, typename... Args>
         static std::shared_ptr<T> makeDerived(Args... args) {
             auto device = hidpp::Device::makeDerived<T>(std::forward<Args>(args)...);
 
@@ -73,8 +82,9 @@ namespace logid::backend::hidpp10 {
 
             return device;
         }
+
     public:
-        template <typename... Args>
+        template<typename... Args>
         static std::shared_ptr<Device> make(Args... args) {
             return makeDerived<Device>(std::forward<Args>(args)...);
         }

src/logid/backend/hidpp10/Receiver.cpp

@@ -68,7 +68,7 @@ void Receiver::setNotifications(NotificationFlags flags) {
 }
 
 void Receiver::enumerate() {
-    setRegister(ConnectionState, {2}, hidpp::ReportType::Short);
+    setRegisterNoResponse(ConnectionState, {2}, hidpp::ReportType::Short);
 }
 
 ///TODO: Investigate usage

src/logid/backend/hidpp10/ReceiverMonitor.cpp

@@ -158,28 +158,31 @@ void ReceiverMonitor::enumerate() {
 }
 
 void ReceiverMonitor::waitForDevice(hidpp::DeviceIndex index) {
-    auto handler_id = std::make_shared<EventHandlerLock<raw::RawDevice>>();
-
-    *handler_id = _receiver->rawDevice()->addEventHandler(
-            {[index](const std::vector<uint8_t>& report) -> bool {
-                return report[Offset::DeviceIndex] == index;
-            },
-             [self_weak = _self, index, handler_id](
-                     [[maybe_unused]] const std::vector<uint8_t>& report) {
-                 hidpp::DeviceConnectionEvent event{};
-                 event.withPayload = false;
-                 event.linkEstablished = true;
-                 event.index = index;
-                 event.fromTimeoutCheck = true;
-
-                 run_task([self_weak, event, handler_id]() {
-                     *handler_id = {};
-                     if (auto self = self_weak.lock()) {
-                         self->_addHandler(event);
-                     }
-                 });
-             }
-            });
+    const std::lock_guard lock(_wait_mutex);
+    if (!_waiters.count(index)) {
+        _waiters.emplace(index, _receiver->rawDevice()->addEventHandler(
+                {[index](const std::vector<uint8_t>& report) -> bool {
+                    /* Connection events should be handled by connect_ev_handler */
+                    auto sub_id = report[Offset::SubID];
+                    return report[Offset::DeviceIndex] == index &&
+                           sub_id != Receiver::DeviceConnection &&
+                           sub_id != Receiver::DeviceDisconnection;
+                },
+                 [self_weak = _self, index](
+                         [[maybe_unused]] const std::vector<uint8_t>& report) {
+                     hidpp::DeviceConnectionEvent event{};
+                     event.withPayload = false;
+                     event.linkEstablished = true;
+                     event.index = index;
+                     event.fromTimeoutCheck = true;
+
+                     run_task([self_weak, event]() {
+                         if (auto self = self_weak.lock())
+                             self->_addHandler(event);
+                     });
+                 }
+                }));
+    }
 }
 
 std::shared_ptr<Receiver> ReceiverMonitor::receiver() const {
@@ -217,6 +220,8 @@ void ReceiverMonitor::_addHandler(const hidpp::DeviceConnectionEvent& event, int
     auto device_path = _receiver->devicePath();
     try {
         addDevice(event);
+        const std::lock_guard lock(_wait_mutex);
+        _waiters.erase(event.index);
     } catch (DeviceNotReady& e) {
         if (tries == max_tries) {
             logPrintf(WARN, "Failed to add device %s:%d after %d tries."

src/logid/backend/hidpp10/ReceiverMonitor.h

@@ -101,6 +101,9 @@ namespace logid::backend::hidpp10 {
 
         std::weak_ptr<ReceiverMonitor> _self;
 
+        std::mutex _wait_mutex;
+        std::map<hidpp::DeviceIndex, EventHandlerLock<raw::RawDevice>> _waiters;
+
     public:
         template<typename T, typename... Args>
         static std::shared_ptr<T> make(Args... args) {