From e136587f7390ecbb19780a2e4ac700c18327c58d Mon Sep 17 00:00:00 2001 From: Prabir Pradhan Date: Thu, 6 Feb 2025 20:29:48 +0000 Subject: EventHub: Refactor AssociatedDevice creation logic Refactor the AssociatedDevice creation logic to make it easier to implement upcoming changes to AssociatedDevice initialization. In particular, we want to make it simple to differentiate the case where an existing AssociatedDevice needs to be updated from the case where a new AssociatedDevice needs to be created. Bug: 245989146 Bug: 357090960 Test: Presubmit Flag: EXEMPT refactor Change-Id: I8d86f56f3c00f9ae13232a60e338ff7ec246ce3b --- services/inputflinger/reader/EventHub.cpp | 63 +++++++++++++------------ services/inputflinger/reader/include/EventHub.h | 1 + 2 files changed, 33 insertions(+), 31 deletions(-) diff --git a/services/inputflinger/reader/EventHub.cpp b/services/inputflinger/reader/EventHub.cpp index 013ef862ad..cc105bdf24 100644 --- a/services/inputflinger/reader/EventHub.cpp +++ b/services/inputflinger/reader/EventHub.cpp @@ -1620,41 +1620,47 @@ std::shared_ptr EventHub::obtainAssociatedDevi const auto& path = *sysfsRootPathOpt; - std::shared_ptr associatedDevice = std::make_shared( - AssociatedDevice{.sysfsRootPath = path, - .batteryInfos = readBatteryConfiguration(path), - .lightInfos = readLightsConfiguration(path), - .layoutInfo = readLayoutConfiguration(path)}); - - bool associatedDeviceChanged = false; + std::shared_ptr associatedDevice; for (const auto& [id, dev] : mDevices) { - if (dev->associatedDevice && dev->associatedDevice->sysfsRootPath == path) { - if (*associatedDevice != *dev->associatedDevice) { - associatedDeviceChanged = true; - dev->associatedDevice = associatedDevice; - } + if (!dev->associatedDevice || dev->associatedDevice->sysfsRootPath != path) { + continue; + } + if (!associatedDevice) { + // Found matching associated device for the first time. associatedDevice = dev->associatedDevice; + // Reload this associated device if needed. + const auto reloadedDevice = AssociatedDevice(path); + if (reloadedDevice != *dev->associatedDevice) { + ALOGI("The AssociatedDevice changed for path '%s'. Using new AssociatedDevice: %s", + path.c_str(), associatedDevice->dump().c_str()); + associatedDevice = std::make_shared(std::move(reloadedDevice)); + } } + // Update the associatedDevice. + dev->associatedDevice = associatedDevice; + } + + if (!associatedDevice) { + // No existing associated device found for this path, so create a new one. + associatedDevice = std::make_shared(path); } - ALOGI_IF(associatedDeviceChanged, - "The AssociatedDevice changed for path '%s'. Using new AssociatedDevice: %s", - path.c_str(), associatedDevice->dump().c_str()); return associatedDevice; } +EventHub::AssociatedDevice::AssociatedDevice(const std::filesystem::path& sysfsRootPath) + : sysfsRootPath(sysfsRootPath), + batteryInfos(readBatteryConfiguration(sysfsRootPath)), + lightInfos(readLightsConfiguration(sysfsRootPath)), + layoutInfo(readLayoutConfiguration(sysfsRootPath)) {} + bool EventHub::AssociatedDevice::isChanged() const { - std::unordered_map newBatteryInfos = - readBatteryConfiguration(sysfsRootPath); - std::unordered_map newLightInfos = - readLightsConfiguration(sysfsRootPath); - std::optional newLayoutInfo = readLayoutConfiguration(sysfsRootPath); - - if (newBatteryInfos == batteryInfos && newLightInfos == lightInfos && - newLayoutInfo == layoutInfo) { - return false; - } - return true; + return AssociatedDevice(sysfsRootPath) != *this; +} + +std::string EventHub::AssociatedDevice::dump() const { + return StringPrintf("path=%s, numBatteries=%zu, numLight=%zu", sysfsRootPath.c_str(), + batteryInfos.size(), lightInfos.size()); } void EventHub::vibrate(int32_t deviceId, const VibrationElement& element) { @@ -2972,9 +2978,4 @@ void EventHub::monitor() const { std::unique_lock lock(mLock); } -std::string EventHub::AssociatedDevice::dump() const { - return StringPrintf("path=%s, numBatteries=%zu, numLight=%zu", sysfsRootPath.c_str(), - batteryInfos.size(), lightInfos.size()); -} - } // namespace android diff --git a/services/inputflinger/reader/include/EventHub.h b/services/inputflinger/reader/include/EventHub.h index 5839b4c41c..8109dae715 100644 --- a/services/inputflinger/reader/include/EventHub.h +++ b/services/inputflinger/reader/include/EventHub.h @@ -619,6 +619,7 @@ public: private: // Holds information about the sysfs device associated with the Device. struct AssociatedDevice { + AssociatedDevice(const std::filesystem::path& sysfsRootPath); // The sysfs root path of the misc device. std::filesystem::path sysfsRootPath; std::unordered_map batteryInfos; -- cgit v1.2.3-59-g8ed1b From 7fb7187dabe8c92b064221785ee198cb616ecef5 Mon Sep 17 00:00:00 2001 From: Prabir Pradhan Date: Thu, 6 Feb 2025 20:41:29 +0000 Subject: EventHub: Reimplement sysfsNodeChanged There were several bugs in the old implementation of sysfsNodeChanged. There are unfortunately no existing unit tests for EventHub, so the old code and the new code are not tested exhaustively. Issues addressed: - The old code would concurrently modify mOpeningDevices while iterating through it, making the code difficult to reason about, particularly since calling openDeviceLocked() from the iteration would add an additional opening device. - The old code was improperly erasing items from mOpeningDevices. vector::erase() returns the next iterator position after the removal, and when progressing to the next for loop iteration, the iterator is incremented again, leading to a missed element. - Each call to isChanged() involves several syscalls, and the old code would perform the check multiple times on the same AssociatedDevice object. Note that for each changed sysfs node, the sysfs node is reloaded yet again for each openDeviceLocked() call, which this CL does not address. Bug: 245989146 Test: atest SonyDualshock4BluetoothTest --iterations Test: Presubmit Flag: EXEMPT refactor/bug fix Change-Id: Iaec9079d8c888310f6d8e496b9dd9df231aff228 --- services/inputflinger/reader/EventHub.cpp | 69 ++++++++++++++++--------- services/inputflinger/reader/include/EventHub.h | 1 - 2 files changed, 44 insertions(+), 26 deletions(-) diff --git a/services/inputflinger/reader/EventHub.cpp b/services/inputflinger/reader/EventHub.cpp index cc105bdf24..3c8b6f54c1 100644 --- a/services/inputflinger/reader/EventHub.cpp +++ b/services/inputflinger/reader/EventHub.cpp @@ -1654,10 +1654,6 @@ EventHub::AssociatedDevice::AssociatedDevice(const std::filesystem::path& sysfsR lightInfos(readLightsConfiguration(sysfsRootPath)), layoutInfo(readLayoutConfiguration(sysfsRootPath)) {} -bool EventHub::AssociatedDevice::isChanged() const { - return AssociatedDevice(sysfsRootPath) != *this; -} - std::string EventHub::AssociatedDevice::dump() const { return StringPrintf("path=%s, numBatteries=%zu, numLight=%zu", sysfsRootPath.c_str(), batteryInfos.size(), lightInfos.size()); @@ -2652,33 +2648,56 @@ status_t EventHub::disableDevice(int32_t deviceId) { void EventHub::sysfsNodeChanged(const std::string& sysfsNodePath) { std::scoped_lock _l(mLock); - // Check in opening devices - for (auto it = mOpeningDevices.begin(); it != mOpeningDevices.end(); it++) { - std::unique_ptr& device = *it; - if (device->associatedDevice && - sysfsNodePath.find(device->associatedDevice->sysfsRootPath.string()) != - std::string::npos && - device->associatedDevice->isChanged()) { - it = mOpeningDevices.erase(it); - openDeviceLocked(device->path); + // Testing whether a sysfs node changed involves several syscalls, so use a cache to avoid + // testing the same node multiple times. + std::map, bool /*changed*/> testedDevices; + auto isAssociatedDeviceChanged = [&testedDevices, &sysfsNodePath](const Device& dev) { + if (!dev.associatedDevice) { + return false; } - } + if (auto testedIt = testedDevices.find(dev.associatedDevice); + testedIt != testedDevices.end()) { + return testedIt->second; + } + // Cache miss + if (sysfsNodePath.find(dev.associatedDevice->sysfsRootPath.string()) == std::string::npos) { + testedDevices.emplace(dev.associatedDevice, false); + return false; + } + auto reloadedDevice = AssociatedDevice(dev.associatedDevice->sysfsRootPath); + const bool changed = *dev.associatedDevice != reloadedDevice; + testedDevices.emplace(dev.associatedDevice, changed); + return changed; + }; - // Check in already added device - std::vector devicesToReopen; - for (const auto& [id, device] : mDevices) { - if (device->associatedDevice && - sysfsNodePath.find(device->associatedDevice->sysfsRootPath.string()) != - std::string::npos && - device->associatedDevice->isChanged()) { - devicesToReopen.push_back(device.get()); + std::set devicesToClose; + std::set devicesToOpen; + + // Check in opening devices. If its associated device changed, + // the device should be removed from mOpeningDevices and needs to be opened again. + std::erase_if(mOpeningDevices, [&](const auto& dev) { + if (isAssociatedDeviceChanged(*dev)) { + devicesToOpen.emplace(dev->path); + return true; + } + return false; + }); + + // Check in already added device. If its associated device changed, + // the device needs to be re-opened. + for (const auto& [id, dev] : mDevices) { + if (isAssociatedDeviceChanged(*dev)) { + devicesToOpen.emplace(dev->path); + devicesToClose.emplace(dev.get()); } } - for (const auto& device : devicesToReopen) { + + for (auto* device : devicesToClose) { closeDeviceLocked(*device); - openDeviceLocked(device->path); } - devicesToReopen.clear(); + for (const auto& path : devicesToOpen) { + openDeviceLocked(path); + } } void EventHub::createVirtualKeyboardLocked() { diff --git a/services/inputflinger/reader/include/EventHub.h b/services/inputflinger/reader/include/EventHub.h index 8109dae715..31ac63f31e 100644 --- a/services/inputflinger/reader/include/EventHub.h +++ b/services/inputflinger/reader/include/EventHub.h @@ -626,7 +626,6 @@ private: std::unordered_map lightInfos; std::optional layoutInfo; - bool isChanged() const; bool operator==(const AssociatedDevice&) const = default; bool operator!=(const AssociatedDevice&) const = default; std::string dump() const; -- cgit v1.2.3-59-g8ed1b