[RFC,v1,3/3] libcamera: device_enumerator_udev: Handle duplicate devices
diff mbox series

Message ID 20251202135817.1607250-3-barnabas.pocze@ideasonboard.com
State New
Headers show
Series
  • [RFC,v1,1/3] libcamera: device_enumerator_udev: Add `override`
Related show

Commit Message

Barnabás Pőcze Dec. 2, 2025, 1:58 p.m. UTC
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 <barnabas.pocze@ideasonboard.com>
---
 .../internal/device_enumerator_udev.h         |  3 ++
 src/libcamera/device_enumerator_udev.cpp      | 38 ++++++++++++++-----
 2 files changed, 32 insertions(+), 9 deletions(-)

Comments

Kieran Bingham Dec. 2, 2025, 2:09 p.m. UTC | #1
Quoting Barnabás Pőcze (2025-12-02 13:58:17)
> 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
> 

This seems like such a small race window - it's surprising if that's
what caused the reported bug. But if it's a window it can happen right
...

But the patch looks reasonable to me:

Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>

> 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 <barnabas.pocze@ideasonboard.com>
> ---
>  .../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 <map>
>  #include <memory>
>  #include <set>
> +#include <unordered_set>
>  #include <string>
>  #include <sys/types.h>
>  
> @@ -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<dev_t> orphans_;
> +       std::unordered_set<dev_t> devices_;
>         std::list<MediaDeviceDeps> pending_;
>         std::map<dev_t, MediaDeviceDeps *> 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<MediaDevice> 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);
>  }
> -- 
> 2.52.0
>
Barnabás Pőcze Dec. 2, 2025, 2:15 p.m. UTC | #2
2025. 12. 02. 15:09 keltezéssel, Kieran Bingham írta:
> Quoting Barnabás Pőcze (2025-12-02 13:58:17)
>> 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
>>
> 
> This seems like such a small race window - it's surprising if that's
> what caused the reported bug. But if it's a window it can happen right

The reporter said

   libcamera in wireplumber crashed when I updated my system and Wireplumber restarted, here are logs:

So I imagine, due to the system update, there was some (a lot?) udev activity in
parallel with wireplumber restarting, which made the bug visible.


> ...
> 
> But the patch looks reasonable to me:
> 
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> 
>> 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 <barnabas.pocze@ideasonboard.com>
>> ---
>>   .../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 <map>
>>   #include <memory>
>>   #include <set>
>> +#include <unordered_set>
>>   #include <string>
>>   #include <sys/types.h>
>>   
>> @@ -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<dev_t> orphans_;
>> +       std::unordered_set<dev_t> devices_;
>>          std::list<MediaDeviceDeps> pending_;
>>          std::map<dev_t, MediaDeviceDeps *> 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<MediaDevice> 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);
>>   }
>> -- 
>> 2.52.0
>>
Laurent Pinchart Dec. 2, 2025, 3:43 p.m. UTC | #3
Hi Barnabás,

Thank you for addressing this long-standing issue.

On Tue, Dec 02, 2025 at 02:58:17PM +0100, Barnabás Pőcze wrote:
> 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).

A race between initial enumerate and hotplug events is unavoidable with
our code structure. As this is an issue that I would expect to affect
lots of applications using udev, I wonder if there's a recommended way
in the udev API to handle this. Are all applications expected to filter
out duplicates manually ?

> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/293
> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
> ---
>  .../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 <map>
>  #include <memory>
>  #include <set>
> +#include <unordered_set>
>  #include <string>
>  #include <sys/types.h>
>  
> @@ -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<dev_t> orphans_;
> +	std::unordered_set<dev_t> devices_;
>  	std::list<MediaDeviceDeps> pending_;
>  	std::map<dev_t, MediaDeviceDeps *> 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<MediaDevice> 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);
>  }
Barnabás Pőcze Dec. 2, 2025, 5:09 p.m. UTC | #4
2025. 12. 02. 16:43 keltezéssel, Laurent Pinchart írta:
> Hi Barnabás,
> 
> Thank you for addressing this long-standing issue.
> 
> On Tue, Dec 02, 2025 at 02:58:17PM +0100, Barnabás Pőcze wrote:
>> 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).
> 
> A race between initial enumerate and hotplug events is unavoidable with
> our code structure. As this is an issue that I would expect to affect
> lots of applications using udev, I wonder if there's a recommended way
> in the udev API to handle this. Are all applications expected to filter
> out duplicates manually ?

My conclusion is "yes(?)". The udev enumerator and monitor are largely
separate things, they do refer to a common `struct udev` context, but
at least in the systemd implementation, that only stores a "user data"
`void *`. So I don't think we can expect udev to mediate this. I also
cannot see any way to "connect" the enumerator and the monitor.

I have looked at multiple users on debian code search, and while it wasn't
too conclusive, I have seen deduplication code for sure. E.g. libinput
explicitly mentions this very race condition:

	/* There is a race at startup: a device added between setting
	 * up the udev monitor and enumerating all current devices may show
	 * up in both lists. Filter those out.
	 */
	if (filter_duplicates(seat, udev_device))
		return 0;

   -- https://sources.debian.org/src/libinput/1.30.0-1/src/udev-seat.c?hl=57#L103


Regards,
Barnabás Pőcze

> 
>> Closes: https://gitlab.freedesktop.org/camera/libcamera/-/issues/293
>> Signed-off-by: Barnabás Pőcze <barnabas.pocze@ideasonboard.com>
>> ---
>>   .../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 <map>
>>   #include <memory>
>>   #include <set>
>> +#include <unordered_set>
>>   #include <string>
>>   #include <sys/types.h>
>>   
>> @@ -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<dev_t> orphans_;
>> +	std::unordered_set<dev_t> devices_;
>>   	std::list<MediaDeviceDeps> pending_;
>>   	std::map<dev_t, MediaDeviceDeps *> 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<MediaDevice> 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);
>>   }
>

Patch
diff mbox series

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 <map>
 #include <memory>
 #include <set>
+#include <unordered_set>
 #include <string>
 #include <sys/types.h>
 
@@ -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<dev_t> orphans_;
+	std::unordered_set<dev_t> devices_;
 	std::list<MediaDeviceDeps> pending_;
 	std::map<dev_t, MediaDeviceDeps *> 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<MediaDevice> 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);
 }