From patchwork Tue Dec 2 13:58:17 2025 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 8bit X-Patchwork-Submitter: =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= X-Patchwork-Id: 25336 Return-Path: X-Original-To: parsemail@patchwork.libcamera.org Delivered-To: parsemail@patchwork.libcamera.org Received: from lancelot.ideasonboard.com (lancelot.ideasonboard.com [92.243.16.209]) by patchwork.libcamera.org (Postfix) with ESMTPS id 1C460C326B for ; Tue, 2 Dec 2025 13:58:28 +0000 (UTC) Received: from lancelot.ideasonboard.com (localhost [IPv6:::1]) by lancelot.ideasonboard.com (Postfix) with ESMTP id 83E2E60C8A; Tue, 2 Dec 2025 14:58:26 +0100 (CET) Authentication-Results: lancelot.ideasonboard.com; dkim=pass (1024-bit key; unprotected) header.d=ideasonboard.com header.i=@ideasonboard.com header.b="GjsheDcT"; dkim-atps=neutral Received: from perceval.ideasonboard.com (perceval.ideasonboard.com [IPv6:2001:4b98:dc2:55:216:3eff:fef7:d647]) by lancelot.ideasonboard.com (Postfix) with ESMTPS id F2A0B60C85 for ; Tue, 2 Dec 2025 14:58:21 +0100 (CET) Received: from pb-laptop.local (185.182.214.104.nat.pool.zt.hu [185.182.214.104]) by perceval.ideasonboard.com (Postfix) with ESMTPSA id DBDE53E6 for ; Tue, 2 Dec 2025 14:56:07 +0100 (CET) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=ideasonboard.com; s=mail; t=1764683767; bh=jPB0+IUU8YzDDMW47FMXdlxIgmHC2QlYdky5+D22dpw=; h=From:To:Subject:Date:In-Reply-To:References:From; b=GjsheDcTFxe6jVp2MImDh8kdpWsnDBFHlc2Ges3AMChkSf/VVvOZ7sUm9pd0zuViY JUi05xvcBPE4p1XlB3GIuzSD9cT/GfXYiil2Y8gLS0CNZz6HfF+EvZnqS4dvPMcKv5 55Ag7pXWJLdqUHmozKHWSaJj+/DnjWxG/siM3ACE= From: =?utf-8?q?Barnab=C3=A1s_P=C5=91cze?= To: libcamera-devel@lists.libcamera.org Subject: [RFC PATCH v1 3/3] libcamera: device_enumerator_udev: Handle duplicate devices Date: Tue, 2 Dec 2025 14:58:17 +0100 Message-ID: <20251202135817.1607250-3-barnabas.pocze@ideasonboard.com> X-Mailer: git-send-email 2.52.0 In-Reply-To: <20251202135817.1607250-1-barnabas.pocze@ideasonboard.com> References: <20251202135817.1607250-1-barnabas.pocze@ideasonboard.com> MIME-Version: 1.0 X-BeenThere: libcamera-devel@lists.libcamera.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: libcamera-devel-bounces@lists.libcamera.org Sender: "libcamera-devel" It is possible that same device is processed multiple times, leading to multiple `MediaDevice`s being instantiated, mostly likely leading to a fatal error: Trying to register a camera with a duplicated ID xyzw... This can most easily happen as a result of multiple `enumerate()` calls, however, currently `enumerate()` is only ever called once, so that won't trigger the issue. Nonetheless, it is still possible to trigger this issue due to the fact that there is a time window after the `udev_monitor` has been created in `init()` and the first (and only) enumeration done in `enumerate()`. If e.g. a UVC camera is connected in this time frame, then it is possible that it will be processed both by the `udevNotify()` and the initial `enumerate()`, leading to the fatal error. This can be reproduced as follows: 1. $ gdb --args cam -m 2. (gdb) break libcamera::DeviceEnumeratorUdev::enumerate 3. (gdb) run 4. when the breakpoint is hit, connect a usb camera 5. (gdb) continue 6. observe fatal error To address this, keep track of the devnums of all devices reported by udev, and reject devices with already known devnums. This ensures that any number of `enumerate()` calls and hotplug events will work correctly (assuming that udev reports "add" / "remove" events correctly). Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/293 Signed-off-by: Barnabás Pőcze Reviewed-by: Kieran Bingham --- .../internal/device_enumerator_udev.h | 3 ++ src/libcamera/device_enumerator_udev.cpp | 38 ++++++++++++++----- 2 files changed, 32 insertions(+), 9 deletions(-) diff --git a/include/libcamera/internal/device_enumerator_udev.h b/include/libcamera/internal/device_enumerator_udev.h index c2f7154b6b..3de7c37104 100644 --- a/include/libcamera/internal/device_enumerator_udev.h +++ b/include/libcamera/internal/device_enumerator_udev.h @@ -11,6 +11,7 @@ #include #include #include +#include #include #include @@ -59,6 +60,7 @@ private: LIBCAMERA_DISABLE_COPY_AND_MOVE(DeviceEnumeratorUdev) int addUdevDevice(struct udev_device *dev); + void removeUdevDevice(struct udev_device *dev); int populateMediaDevice(MediaDevice *media, DependencyMap *deps); std::string lookupDeviceNode(dev_t devnum); @@ -70,6 +72,7 @@ private: EventNotifier *notifier_; std::set orphans_; + std::unordered_set devices_; std::list pending_; std::map devMap_; }; diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp index 4e20a3cc0c..406e59b360 100644 --- a/src/libcamera/device_enumerator_udev.cpp +++ b/src/libcamera/device_enumerator_udev.cpp @@ -76,6 +76,21 @@ int DeviceEnumeratorUdev::addUdevDevice(struct udev_device *dev) if (!subsystem) return -ENODEV; + /* + * Record that udev reported the given devnum. And reject if it has already + * been seen (e.g. multiple `enumerate()` calls, device added between udev + * monitor creation in `init()` and `enumerate()`). This record is kept even + * if later in this function an error is encountered. Only a "remove" event from + * udev should erase it from `devices_`. + */ + const dev_t devnum = udev_device_get_devnum(dev); + if (devnum == makedev(0, 0)) + return -ENODEV; + + const auto [it, inserted] = devices_.insert(devnum); + if (!inserted) + return -EEXIST; + if (!strcmp(subsystem, "media")) { std::unique_ptr media = createDevice(udev_device_get_devnode(dev)); @@ -111,13 +126,22 @@ int DeviceEnumeratorUdev::addUdevDevice(struct udev_device *dev) } if (!strcmp(subsystem, "video4linux")) { - addV4L2Device(udev_device_get_devnum(dev)); + addV4L2Device(devnum); return 0; } return -ENODEV; } +void DeviceEnumeratorUdev::removeUdevDevice(struct udev_device *dev) +{ + const char *subsystem = udev_device_get_subsystem(dev); + if (subsystem && !strcmp(subsystem, "media")) + removeDevice(udev_device_get_devnode(dev)); + + devices_.erase(udev_device_get_devnum(dev)); +} + int DeviceEnumeratorUdev::enumerate() { struct udev_enumerate *udev_enum = nullptr; @@ -341,18 +365,14 @@ void DeviceEnumeratorUdev::udevNotify() } std::string_view action(udev_device_get_action(dev)); - std::string_view deviceNode(udev_device_get_devnode(dev)); LOG(DeviceEnumerator, Debug) - << action << " device " << deviceNode; + << action << " device " << udev_device_get_devnode(dev); - if (action == "add") { + if (action == "add") addUdevDevice(dev); - } else if (action == "remove") { - const char *subsystem = udev_device_get_subsystem(dev); - if (subsystem && !strcmp(subsystem, "media")) - removeDevice(std::string(deviceNode)); - } + else if (action == "remove") + removeUdevDevice(dev); udev_device_unref(dev); }