Merge changes from topic "getSysfsRootPath" into main

prabirmsp · Android (Google) Code Review · commit 723823999c27 · 2025-03-21T11:20:36.000-07:00
* changes:
  EventHub: Re-open Devices serially when AssociatedDevice changes
  InputReader: Add getter API for the sysfs node path of an InputDevice
diff --git a/services/inputflinger/include/InputReaderBase.h b/services/inputflinger/include/InputReaderBase.h
@@ -443,6 +443,9 @@ class InputReaderInterface {
     /* Get the Bluetooth address of an input device, if known. */
     virtual std::optional<std::string> getBluetoothAddress(int32_t deviceId) const = 0;
 
+    /* Gets the sysfs root path for this device. Returns an empty path if there is none. */
+    virtual std::filesystem::path getSysfsRootPath(int32_t deviceId) const = 0;
+
     /* Sysfs node change reported. Recreate device if required to incorporate the new sysfs nodes */
     virtual void sysfsNodeChanged(const std::string& sysfsNodePath) = 0;
 
diff --git a/services/inputflinger/reader/EventHub.cpp b/services/inputflinger/reader/EventHub.cpp
@@ -246,7 +246,7 @@ static nsecs_t processEventTimestamp(const struct input_event& event) {
 /**
  * Returns the sysfs root path of the input device.
  */
-static std::optional<std::filesystem::path> getSysfsRootPath(const char* devicePath) {
+static std::optional<std::filesystem::path> getSysfsRootForEvdevDevicePath(const char* devicePath) {
     std::error_code errorCode;
 
     // Stat the device path to get the major and minor number of the character file
@@ -1619,7 +1619,7 @@ void EventHub::assignDescriptorLocked(InputDeviceIdentifier& identifier) {
 std::shared_ptr<const EventHub::AssociatedDevice> EventHub::obtainAssociatedDeviceLocked(
         const std::filesystem::path& devicePath, const std::shared_ptr<PropertyMap>& config) const {
     const std::optional<std::filesystem::path> sysfsRootPathOpt =
-            getSysfsRootPath(devicePath.c_str());
+            getSysfsRootForEvdevDevicePath(devicePath.c_str());
     if (!sysfsRootPathOpt) {
         return nullptr;
     }
@@ -1899,58 +1899,87 @@ std::vector<RawEvent> EventHub::getEvents(int timeoutMillis) {
 
         handleSysfsNodeChangeNotificationsLocked();
 
-        // Report any devices that had last been added/removed.
-        for (auto it = mClosingDevices.begin(); it != mClosingDevices.end();) {
-            std::unique_ptr<Device> device = std::move(*it);
-            ALOGV("Reporting device closed: id=%d, name=%s\n", device->id, device->path.c_str());
-            const int32_t deviceId = (device->id == mBuiltInKeyboardId)
-                    ? ReservedInputDeviceId::BUILT_IN_KEYBOARD_ID
-                    : device->id;
-            events.push_back({
-                    .when = now,
-                    .deviceId = deviceId,
-                    .type = DEVICE_REMOVED,
-            });
-            it = mClosingDevices.erase(it);
-            if (events.size() == EVENT_BUFFER_SIZE) {
-                break;
+        // Use a do-while loop to ensure that we drain the closing and opening devices loop
+        // at least once, even if there are no devices to re-open.
+        do {
+            if (!mDeviceIdsToReopen.empty()) {
+                // If there are devices that need to be re-opened, ensure that we re-open them
+                // one at a time to send the DEVICE_REMOVED and DEVICE_ADDED notifications for
+                // each before moving on to the next. This is to avoid notifying all device
+                // removals and additions in one batch, which could cause additional unnecessary
+                // device added/removed notifications for merged InputDevices from InputReader.
+                const int32_t deviceId = mDeviceIdsToReopen.back();
+                mDeviceIdsToReopen.erase(mDeviceIdsToReopen.end() - 1);
+                if (auto it = mDevices.find(deviceId); it != mDevices.end()) {
+                    ALOGI("Reopening input device: id=%d, name=%s", it->second->id,
+                          it->second->identifier.name.c_str());
+                    const auto path = it->second->path;
+                    closeDeviceLocked(*it->second);
+                    openDeviceLocked(path);
+                }
             }
-        }
 
-        if (mNeedToScanDevices) {
-            mNeedToScanDevices = false;
-            scanDevicesLocked();
-        }
-
-        while (!mOpeningDevices.empty()) {
-            std::unique_ptr<Device> device = std::move(*mOpeningDevices.rbegin());
-            mOpeningDevices.pop_back();
-            ALOGV("Reporting device opened: id=%d, name=%s\n", device->id, device->path.c_str());
-            const int32_t deviceId = device->id == mBuiltInKeyboardId ? 0 : device->id;
-            events.push_back({
-                    .when = now,
-                    .deviceId = deviceId,
-                    .type = DEVICE_ADDED,
-            });
-
-            // Try to find a matching video device by comparing device names
-            for (auto it = mUnattachedVideoDevices.begin(); it != mUnattachedVideoDevices.end();
-                 it++) {
-                std::unique_ptr<TouchVideoDevice>& videoDevice = *it;
-                if (tryAddVideoDeviceLocked(*device, videoDevice)) {
-                    // videoDevice was transferred to 'device'
-                    it = mUnattachedVideoDevices.erase(it);
+            // Report any devices that had last been added/removed.
+            for (auto it = mClosingDevices.begin(); it != mClosingDevices.end();) {
+                std::unique_ptr<Device> device = std::move(*it);
+                ALOGV("Reporting device closed: id=%d, name=%s\n", device->id,
+                      device->path.c_str());
+                const int32_t deviceId = (device->id == mBuiltInKeyboardId)
+                        ? ReservedInputDeviceId::BUILT_IN_KEYBOARD_ID
+                        : device->id;
+                events.push_back({
+                        .when = now,
+                        .deviceId = deviceId,
+                        .type = DEVICE_REMOVED,
+                });
+                it = mClosingDevices.erase(it);
+                if (events.size() == EVENT_BUFFER_SIZE) {
                     break;
                 }
             }
 
-            auto [dev_it, inserted] = mDevices.insert_or_assign(device->id, std::move(device));
-            if (!inserted) {
-                ALOGW("Device id %d exists, replaced.", device->id);
+            if (mNeedToScanDevices) {
+                mNeedToScanDevices = false;
+                scanDevicesLocked();
             }
-            if (events.size() == EVENT_BUFFER_SIZE) {
-                break;
+
+            while (!mOpeningDevices.empty()) {
+                std::unique_ptr<Device> device = std::move(*mOpeningDevices.rbegin());
+                mOpeningDevices.pop_back();
+                ALOGV("Reporting device opened: id=%d, name=%s\n", device->id,
+                      device->path.c_str());
+                const int32_t deviceId = device->id == mBuiltInKeyboardId ? 0 : device->id;
+                events.push_back({
+                        .when = now,
+                        .deviceId = deviceId,
+                        .type = DEVICE_ADDED,
+                });
+
+                // Try to find a matching video device by comparing device names
+                for (auto it = mUnattachedVideoDevices.begin(); it != mUnattachedVideoDevices.end();
+                     it++) {
+                    std::unique_ptr<TouchVideoDevice>& videoDevice = *it;
+                    if (tryAddVideoDeviceLocked(*device, videoDevice)) {
+                        // videoDevice was transferred to 'device'
+                        it = mUnattachedVideoDevices.erase(it);
+                        break;
+                    }
+                }
+
+                auto [dev_it, inserted] = mDevices.insert_or_assign(device->id, std::move(device));
+                if (!inserted) {
+                    ALOGW("Device id %d exists, replaced.", device->id);
+                }
+                if (events.size() == EVENT_BUFFER_SIZE) {
+                    break;
+                }
             }
+
+            // Perform this loop of re-opening devices so that we re-open one device at a time.
+        } while (!mDeviceIdsToReopen.empty());
+
+        if (events.size() == EVENT_BUFFER_SIZE) {
+            break;
         }
 
         // Grab the next input event.
@@ -2666,6 +2695,18 @@ status_t EventHub::disableDevice(int32_t deviceId) {
     return device->disable();
 }
 
+std::filesystem::path EventHub::getSysfsRootPath(int32_t deviceId) const {
+    std::scoped_lock _l(mLock);
+    Device* device = getDeviceLocked(deviceId);
+    if (device == nullptr) {
+        ALOGE("Invalid device id=%" PRId32 " provided to %s", deviceId, __func__);
+        return {};
+    }
+
+    return device->associatedDevice ? device->associatedDevice->sysfsRootPath
+                                    : std::filesystem::path{};
+}
+
 // TODO(b/274755573): Shift to uevent handling on native side and remove this method
 // Currently using Java UEventObserver to trigger this which uses UEvent infrastructure that uses a
 // NETLINK socket to observe UEvents. We can create similar infrastructure on Eventhub side to
@@ -2688,8 +2729,10 @@ void EventHub::handleSysfsNodeChangeNotificationsLocked() {
 
     // Testing whether a sysfs node changed involves several syscalls, so use a cache to avoid
     // testing the same node multiple times.
+    // TODO(b/281822656): Notify InputReader separately when an AssociatedDevice changes,
+    //  instead of needing to re-open all of Devices that are associated with it.
     std::map<std::shared_ptr<const AssociatedDevice>, bool /*changed*/> testedDevices;
-    auto isAssociatedDeviceChanged = [&testedDevices, &changedNodes](const Device& dev) {
+    auto shouldReopenDevice = [&testedDevices, &changedNodes](const Device& dev) {
         if (!dev.associatedDevice) {
             return false;
         }
@@ -2710,31 +2753,33 @@ void EventHub::handleSysfsNodeChangeNotificationsLocked() {
         auto reloadedDevice = AssociatedDevice(dev.associatedDevice->sysfsRootPath,
                                                dev.associatedDevice->baseDevConfig);
         const bool changed = *dev.associatedDevice != reloadedDevice;
+        if (changed) {
+            ALOGI("sysfsNodeChanged: Identified change in sysfs nodes for device: %s",
+                  dev.identifier.name.c_str());
+        }
         testedDevices.emplace(dev.associatedDevice, changed);
         return changed;
     };
 
-    std::set<Device*> devicesToReopen;
-
-    // Check in opening devices.
+    // Check in opening devices. These can be re-opened directly because we have not yet notified
+    // the Reader about these devices.
     for (const auto& dev : mOpeningDevices) {
-        if (isAssociatedDeviceChanged(*dev)) {
-            devicesToReopen.emplace(dev.get());
+        if (shouldReopenDevice(*dev)) {
+            ALOGI("Reopening input device from mOpeningDevices: id=%d, name=%s", dev->id,
+                  dev->identifier.name.c_str());
+            const auto path = dev->path;
+            closeDeviceLocked(*dev); // The Device object is deleted by this function.
+            openDeviceLocked(path);
         }
     }
 
-    // Check in already added devices.
+    // Check in already added devices. Add them to the re-opening list so they can be
+    // re-opened serially.
     for (const auto& [id, dev] : mDevices) {
-        if (isAssociatedDeviceChanged(*dev)) {
-            devicesToReopen.emplace(dev.get());
+        if (shouldReopenDevice(*dev)) {
+            mDeviceIdsToReopen.emplace_back(dev->id);
         }
     }
-
-    for (auto* device : devicesToReopen) {
-        const auto path = device->path;
-        closeDeviceLocked(*device); // The Device object is deleted by this function.
-        openDeviceLocked(path);
-    }
 }
 
 void EventHub::createVirtualKeyboardLocked() {
diff --git a/services/inputflinger/reader/InputDevice.cpp b/services/inputflinger/reader/InputDevice.cpp
@@ -136,6 +136,8 @@ void InputDevice::dump(std::string& dump, const std::string& eventHubDevStr) {
     } else {
         dump += "<none>\n";
     }
+    dump += StringPrintf(INDENT2 "SysfsRootPath:     %s\n",
+                         mSysfsRootPath.empty() ? "<none>" : mSysfsRootPath.c_str());
     dump += StringPrintf(INDENT2 "HasMic:     %s\n", toString(mHasMic));
     dump += StringPrintf(INDENT2 "Sources: %s\n",
                          inputEventSourceToString(deviceInfo.getSources()).c_str());
@@ -195,6 +197,10 @@ void InputDevice::addEmptyEventHubDevice(int32_t eventHubId) {
     DevicePair& devicePair = mDevices[eventHubId];
     devicePair.second = createMappers(*devicePair.first, readerConfig);
 
+    if (mSysfsRootPath.empty()) {
+        mSysfsRootPath = devicePair.first->getSysfsRootPath();
+    }
+
     // Must change generation to flag this device as changed
     bumpGeneration();
     return out;
diff --git a/services/inputflinger/reader/InputReader.cpp b/services/inputflinger/reader/InputReader.cpp
@@ -917,6 +917,16 @@ bool InputReader::canDispatchToDisplay(int32_t deviceId, ui::LogicalDisplayId di
     return *associatedDisplayId == displayId;
 }
 
+std::filesystem::path InputReader::getSysfsRootPath(int32_t deviceId) const {
+    std::scoped_lock _l(mLock);
+
+    const InputDevice* device = findInputDeviceLocked(deviceId);
+    if (!device) {
+        return {};
+    }
+    return device->getSysfsRootPath();
+}
+
 void InputReader::sysfsNodeChanged(const std::string& sysfsNodePath) {
     mEventHub->sysfsNodeChanged(sysfsNodePath);
     mEventHub->wake();
diff --git a/services/inputflinger/reader/include/EventHub.h b/services/inputflinger/reader/include/EventHub.h
@@ -399,6 +399,9 @@ class EventHubInterface {
     /* Disable an input device. Closes file descriptor to that device. */
     virtual status_t disableDevice(int32_t deviceId) = 0;
 
+    /* Gets the sysfs root path for this device. Returns an empty path if there is none. */
+    virtual std::filesystem::path getSysfsRootPath(int32_t deviceId) const = 0;
+
     /* Sysfs node changed. Reopen the Eventhub device if any new Peripheral like Light, Battery,
      * etc. is detected. */
     virtual void sysfsNodeChanged(const std::string& sysfsNodePath) = 0;
@@ -614,6 +617,8 @@ class EventHub : public EventHubInterface {
 
     status_t disableDevice(int32_t deviceId) override final;
 
+    std::filesystem::path getSysfsRootPath(int32_t deviceId) const override final;
+
     void sysfsNodeChanged(const std::string& sysfsNodePath) override final;
 
     bool setKernelWakeEnabled(int32_t deviceId, bool enabled) override final;
@@ -810,6 +815,7 @@ class EventHub : public EventHubInterface {
     bool mNeedToReopenDevices;
     bool mNeedToScanDevices;
     std::vector<std::string> mExcludedDevices;
+    std::vector<int32_t> mDeviceIdsToReopen;
 
     int mEpollFd;
     int mINotifyFd;
diff --git a/services/inputflinger/reader/include/InputDevice.h b/services/inputflinger/reader/include/InputDevice.h
@@ -81,6 +81,8 @@ class InputDevice {
 
     inline virtual KeyboardType getKeyboardType() const { return mKeyboardType; }
 
+    inline std::filesystem::path getSysfsRootPath() const { return mSysfsRootPath; }
+
     bool isEnabled();
 
     void dump(std::string& dump, const std::string& eventHubDevStr);
@@ -209,6 +211,7 @@ class InputDevice {
     bool mHasMic;
     bool mDropUntilNextSync;
     std::optional<bool> mShouldSmoothScroll;
+    std::filesystem::path mSysfsRootPath;
 
     typedef int32_t (InputMapper::*GetStateFunc)(uint32_t sourceMask, int32_t code);
     int32_t getState(uint32_t sourceMask, int32_t code, GetStateFunc getStateFunc);
@@ -471,6 +474,9 @@ class InputDeviceContext {
     inline void setKeyboardType(KeyboardType keyboardType) {
         return mDevice.setKeyboardType(keyboardType);
     }
+    inline std::filesystem::path getSysfsRootPath() const {
+        return mEventHub->getSysfsRootPath(mId);
+    }
     inline bool setKernelWakeEnabled(bool enabled) {
         return mEventHub->setKernelWakeEnabled(mId, enabled);
     }
diff --git a/services/inputflinger/reader/include/InputReader.h b/services/inputflinger/reader/include/InputReader.h
@@ -118,6 +118,8 @@ class InputReader : public InputReaderInterface {
 
     std::optional<std::string> getBluetoothAddress(int32_t deviceId) const override;
 
+    std::filesystem::path getSysfsRootPath(int32_t deviceId) const override;
+
     void sysfsNodeChanged(const std::string& sysfsNodePath) override;
 
     DeviceId getLastUsedInputDeviceId() override;
diff --git a/services/inputflinger/tests/FakeEventHub.cpp b/services/inputflinger/tests/FakeEventHub.cpp
@@ -627,6 +627,14 @@ void FakeEventHub::setSysfsRootPath(int32_t deviceId, std::string sysfsRootPath)
     device->sysfsRootPath = sysfsRootPath;
 }
 
+std::filesystem::path FakeEventHub::getSysfsRootPath(int32_t deviceId) const {
+    Device* device = getDevice(deviceId);
+    if (device == nullptr) {
+        return {};
+    }
+    return device->sysfsRootPath;
+}
+
 void FakeEventHub::sysfsNodeChanged(const std::string& sysfsNodePath) {
     int32_t foundDeviceId = -1;
     Device* foundDevice = nullptr;
diff --git a/services/inputflinger/tests/FakeEventHub.h b/services/inputflinger/tests/FakeEventHub.h
@@ -222,6 +222,7 @@ class FakeEventHub : public EventHubInterface {
     std::optional<int32_t> getLightBrightness(int32_t deviceId, int32_t lightId) const override;
     std::optional<std::unordered_map<LightColor, int32_t>> getLightIntensities(
             int32_t deviceId, int32_t lightId) const override;
+    std::filesystem::path getSysfsRootPath(int32_t deviceId) const override;
     void sysfsNodeChanged(const std::string& sysfsNodePath) override;
     void dump(std::string&) const override {}
     void monitor() const override {}
diff --git a/services/inputflinger/tests/InputReader_test.cpp b/services/inputflinger/tests/InputReader_test.cpp
@@ -611,8 +611,10 @@ class InputReaderTest : public testing::Test {
     }
 
     void addDevice(int32_t eventHubId, const std::string& name,
-                   ftl::Flags<InputDeviceClass> classes, const PropertyMap* configuration) {
+                   ftl::Flags<InputDeviceClass> classes, const PropertyMap* configuration,
+                   std::string sysfsRootPath = "") {
         mFakeEventHub->addDevice(eventHubId, name, classes);
+        mFakeEventHub->setSysfsRootPath(eventHubId, sysfsRootPath);
 
         if (configuration) {
             mFakeEventHub->addConfigurationMap(eventHubId, configuration);
@@ -664,6 +666,18 @@ TEST_F(InputReaderTest, PolicyGetInputDevices) {
     ASSERT_EQ(0U, inputDevices[0].getMotionRanges().size());
 }
 
+TEST_F(InputReaderTest, GetSysfsRootPath) {
+    constexpr std::string SYSFS_ROOT = "xyz";
+    ASSERT_NO_FATAL_FAILURE(
+            addDevice(1, "keyboard", InputDeviceClass::KEYBOARD, nullptr, SYSFS_ROOT));
+
+    // Should also have received a notification describing the new input device.
+    ASSERT_EQ(1U, mFakePolicy->getInputDevices().size());
+    InputDeviceInfo inputDevice = mFakePolicy->getInputDevices()[0];
+
+    ASSERT_EQ(SYSFS_ROOT, mReader->getSysfsRootPath(inputDevice.getId()).string());
+}
+
 TEST_F(InputReaderTest, InputDeviceRecreatedOnSysfsNodeChanged) {
     ASSERT_NO_FATAL_FAILURE(addDevice(1, "keyboard", InputDeviceClass::KEYBOARD, nullptr));
     mFakeEventHub->setSysfsRootPath(1, "xyz");
diff --git a/services/inputflinger/tests/InterfaceMocks.h b/services/inputflinger/tests/InterfaceMocks.h
diff --git a/services/inputflinger/tests/fuzzers/InputReaderFuzzer.cpp b/services/inputflinger/tests/fuzzers/InputReaderFuzzer.cpp
diff --git a/services/inputflinger/tests/fuzzers/MapperHelpers.h b/services/inputflinger/tests/fuzzers/MapperHelpers.h
