[libcamera-devel,v2] libcamera: device_enumerator: fix udev media graph loading dependency

1909 diff mbox series

When a MediaDevice is enumerated and populated by the
DeviceEnumeratorUdev, there is a possibility that the member device
nodes of the media graph would not be ready (either not created, or
without proper permissions set by udev yet) The MediaDevice is still
passed up to the pipeline handler, where an attempt to access the device
nodes will fail in EPERM. This whole issue is especially likely to
happen when libcamera is run at system init time.

To fix this, we first split DeviceEnumerator::addDevice() into three
methods:
- createDevice() to simply create the MediaDevice
- populateMediaDevice() to populate the MediaDevice
- addDevice() to pass the MediaDevice up to the pipeline handler

DeviceEnumeratorSysfs calls these methods in succession, similar to what
it did before when they were all together as addDevice().

DeviceEnumeratorUdev additionally keeps a map of MediaDevices to a list
of pending device nodes (plus some other auxillary maps), and a simple
list of orphan device nodes. If a v4l device node is ready and there
does not exist any MediaDevice node for it, then it goes to the orphan
list, otherwise it is initialized and removed from the pending list of
the corresponding MediaDevice in the dependency map. When a MediaDevice
is populated via DeviceEnumeratorUdev::populateMediaDevice(), it first
checks the orphan list to see if the device nodes it needs are there,
otherwise it tries to initialize the device nodes and if it fails, then
it adds the device nodes it wants to its list in the dependency map.

This allows MediaDevice instances to be created and initialized properly
with udev when v4l device nodes in the media graph may not be ready when
the MediaDevice is populated.

Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>

---
Changes in v2:
- add documentation
- better name for the maps
- other minor changes

todos:
- do we need a better name for populateMediaDevice() ?

---
 src/libcamera/device_enumerator.cpp           |  52 +++----
 src/libcamera/device_enumerator_sysfs.cpp     |  34 ++++-
 src/libcamera/device_enumerator_udev.cpp      | 140 +++++++++++++++++-
 src/libcamera/include/device_enumerator.h     |   3 +-
 .../include/device_enumerator_sysfs.h         |   5 +-
 .../include/device_enumerator_udev.h          |  16 ++
 6 files changed, 218 insertions(+), 32 deletions(-)

Hi Paul,

Thank you for the patch.

On Mon, Sep 02, 2019 at 12:17:13AM -0400, Paul Elder wrote:
> When a MediaDevice is enumerated and populated by the
> DeviceEnumeratorUdev, there is a possibility that the member device
> nodes of the media graph would not be ready (either not created, or
> without proper permissions set by udev yet) The MediaDevice is still

s/)/)./

> passed up to the pipeline handler, where an attempt to access the device
> nodes will fail in EPERM. This whole issue is especially likely to
> happen when libcamera is run at system init time.
> 
> To fix this, we first split DeviceEnumerator::addDevice() into three
> methods:
> - createDevice() to simply create the MediaDevice
> - populateMediaDevice() to populate the MediaDevice
> - addDevice() to pass the MediaDevice up to the pipeline handler
> 
> DeviceEnumeratorSysfs calls these methods in succession, similar to what
> it did before when they were all together as addDevice().
> 
> DeviceEnumeratorUdev additionally keeps a map of MediaDevices to a list
> of pending device nodes (plus some other auxillary maps), and a simple
> list of orphan device nodes. If a v4l device node is ready and there
> does not exist any MediaDevice node for it, then it goes to the orphan
> list, otherwise it is initialized and removed from the pending list of
> the corresponding MediaDevice in the dependency map. When a MediaDevice
> is populated via DeviceEnumeratorUdev::populateMediaDevice(), it first
> checks the orphan list to see if the device nodes it needs are there,
> otherwise it tries to initialize the device nodes and if it fails, then
> it adds the device nodes it wants to its list in the dependency map.
> 
> This allows MediaDevice instances to be created and initialized properly
> with udev when v4l device nodes in the media graph may not be ready when
> the MediaDevice is populated.
> 
> Signed-off-by: Paul Elder <paul.elder@ideasonboard.com>
> 
> ---
> Changes in v2:
> - add documentation
> - better name for the maps
> - other minor changes
> 
> todos:
> - do we need a better name for populateMediaDevice() ?
> 
> ---
>  src/libcamera/device_enumerator.cpp           |  52 +++----
>  src/libcamera/device_enumerator_sysfs.cpp     |  34 ++++-
>  src/libcamera/device_enumerator_udev.cpp      | 140 +++++++++++++++++-
>  src/libcamera/include/device_enumerator.h     |   3 +-
>  .../include/device_enumerator_sysfs.h         |   5 +-
>  .../include/device_enumerator_udev.h          |  16 ++
>  6 files changed, 218 insertions(+), 32 deletions(-)
> 
> diff --git a/src/libcamera/device_enumerator.cpp b/src/libcamera/device_enumerator.cpp
> index 60c918f..0290832 100644
> --- a/src/libcamera/device_enumerator.cpp
> +++ b/src/libcamera/device_enumerator.cpp
> @@ -195,16 +195,19 @@ DeviceEnumerator::~DeviceEnumerator()
>   */
>  
>  /**
> - * \brief Add a media device to the enumerator
> - * \param[in] deviceNode path to the media device to add
> + * \brief Create a media device instance
> + * \param[in] deviceNode path to the media device to create
>   *
> - * Create a media device for the \a deviceNode, open it, populate its media graph,
> - * and look up device nodes associated with all entities. Store the media device
> - * in the internal list for later matching with pipeline handlers.
> + * Create a media device for the \a deviceNode, open it, and populate its
> + * media graph. populateMediaDevice() should be called after this to look
> + * up device nodes associated with all entities and to initialize them.
> + * addDevice() shall be called with the media device returned from this
> + * method, after populateMediaDevice() and after all devices nodes in the
> + * media graph have been confirmed to be initialized.

I wouldn't mention populateMediaDevice() here as those methods are only
implemented in the derived classes. How about

 * Create a media device for the \a deviceNode, open it, and populate its
 * media graph. The device enumerator shall then populate the media device by
 * associating device nodes with entities using MediaEntity::setDeviceNode().
 * This process is specific to each device enumerator, and the device enumerator
 * shall ensure that device nodes are ready to be used (for instance, if
 * applicable, by waiting for device nodes to be created and access permissions
 * to be set by the system). Once done, it shall add the media device to the
 * system with addDevice().

>   *
> - * \return 0 on success or a negative error code otherwise
> + * \return Created media device instance on success, or nullptr otherwise
>   */
> -int DeviceEnumerator::addDevice(const std::string &deviceNode)
> +std::shared_ptr<MediaDevice> DeviceEnumerator::createDevice(const std::string &deviceNode)
>  {
>  	std::shared_ptr<MediaDevice> media = std::make_shared<MediaDevice>(deviceNode);
>  
> @@ -213,34 +216,31 @@ int DeviceEnumerator::addDevice(const std::string &deviceNode)
>  		LOG(DeviceEnumerator, Info)
>  			<< "Unable to populate media device " << deviceNode
>  			<< " (" << strerror(-ret) << "), skipping";
> -		return ret;
> +		return nullptr;
>  	}
>  
>  	LOG(DeviceEnumerator, Debug)
>  		<< "New media device \"" << media->driver()
>  		<< "\" created from " << deviceNode;
>  
> -	/* Associate entities to device node paths. */
> -	for (MediaEntity *entity : media->entities()) {
> -		if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0)
> -			continue;
> -
> -		std::string deviceNode = lookupDeviceNode(entity->deviceMajor(),
> -							  entity->deviceMinor());
> -		if (deviceNode.empty())
> -			return -EINVAL;
> -
> -		ret = entity->setDeviceNode(deviceNode);
> -		if (ret)
> -			return ret;
> -	}
> +	return media;
> +}
>  
> +/**
> + * \brief Add a media device to the enumerator
> + * \param[in] media media device instance to add
> + *
> + * Store the media device in the internal list for later matching with
> + * pipeline handlers. \a media shall be created with createDevice() first.
> + * This method shall be called after all members of the members of the
> + * media graph have been confirmed to be initialized.

Did you mean "all members of the entities of the media graph" ?

> + */
> +void DeviceEnumerator::addDevice(std::shared_ptr<MediaDevice> media)
> +{

You can pass a const reference to the shared pointer instead of a copy.

>  	LOG(DeviceEnumerator, Debug)
> -		<< "Added device " << deviceNode << ": " << media->driver();
> -
> -	devices_.push_back(std::move(media));
> +		<< "Added device " << media->deviceNode() << ": " << media->driver();
>  
> -	return 0;
> +	devices_.push_back(media);
>  }
>  
>  /**
> diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp
> index f3054d5..98299d7 100644
> --- a/src/libcamera/device_enumerator_sysfs.cpp
> +++ b/src/libcamera/device_enumerator_sysfs.cpp
> @@ -32,6 +32,7 @@ int DeviceEnumeratorSysfs::enumerate()
>  {
>  	struct dirent *ent;
>  	DIR *dir;
> +	int ret = 0;
>  
>  	static const char * const sysfs_dirs[] = {
>  		"/sys/subsystem/media/devices",
> @@ -71,11 +72,42 @@ int DeviceEnumeratorSysfs::enumerate()
>  			continue;
>  		}
>  
> -		addDevice(devnode);
> +		std::shared_ptr<MediaDevice> media = createDevice(devnode);
> +		if (!media) {
> +			ret = -ENODEV;
> +			break;
> +		}
> +
> +		if (populateMediaDevice(media) < 0) {
> +			ret = -ENODEV;
> +			break;
> +		}
> +
> +		addDevice(media);
>  	}
>  
>  	closedir(dir);
>  
> +	return ret;
> +}
> +
> +int DeviceEnumeratorSysfs::populateMediaDevice(std::shared_ptr<MediaDevice> media)

You can pass a const reference to the shared pointer instead of a copy
(same for the udev enumerator).

> +{
> +	/* Associate entities to device node paths. */
> +	for (MediaEntity *entity : media->entities()) {
> +		if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0)
> +			continue;
> +
> +		std::string deviceNode = lookupDeviceNode(entity->deviceMajor(),
> +							  entity->deviceMinor());
> +		if (deviceNode.empty())
> +			return -EINVAL;
> +
> +		int ret = entity->setDeviceNode(deviceNode);
> +		if (ret)
> +			return ret;
> +	}
> +
>  	return 0;
>  }
>  
> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> index 86f6ca1..c89f152 100644
> --- a/src/libcamera/device_enumerator_udev.cpp
> +++ b/src/libcamera/device_enumerator_udev.cpp
> @@ -7,6 +7,10 @@
>  
>  #include "device_enumerator_udev.h"
>  
> +#include <algorithm>
> +#include <list>
> +#include <map>
> +
>  #include <fcntl.h>
>  #include <libudev.h>
>  #include <string.h>
> @@ -17,6 +21,7 @@
>  #include <libcamera/event_notifier.h>
>  
>  #include "log.h"
> +#include "media_device.h"
>  
>  namespace libcamera {
>  
> @@ -57,9 +62,40 @@ int DeviceEnumeratorUdev::init()
>  	if (ret < 0)
>  		return ret;
>  
> +	ret = udev_monitor_filter_add_match_subsystem_devtype(monitor_, "video4linux",
> +							      nullptr);
> +	if (ret < 0)
> +		return ret;
> +
>  	return 0;
>  }
>  
> +int DeviceEnumeratorUdev::addUdevDevice(struct udev_device *dev)
> +{
> +	const char *subsystem = udev_device_get_subsystem(dev);
> +	if (!subsystem)
> +		return -ENODEV;
> +
> +	if (!strcmp(subsystem, "media")) {
> +		std::shared_ptr<MediaDevice> media =
> +			createDevice(udev_device_get_devnode(dev));
> +		if (!media)
> +			return -ENODEV;
> +
> +		int ret = populateMediaDevice(media);
> +		if (ret == 0)
> +			addDevice(media);
> +		return 0;
> +	}
> +
> +	if (!strcmp(subsystem, "video4linux")) {
> +		addV4L2Device(udev_device_get_devnum(dev));
> +		return 0;
> +	}
> +
> +	return -ENODEV;
> +}
> +
>  int DeviceEnumeratorUdev::enumerate()
>  {
>  	struct udev_enumerate *udev_enum = nullptr;
> @@ -74,6 +110,14 @@ int DeviceEnumeratorUdev::enumerate()
>  	if (ret < 0)
>  		goto done;
>  
> +	ret = udev_enumerate_add_match_subsystem(udev_enum, "video4linux");
> +	if (ret < 0)
> +		goto done;
> +
> +	ret = udev_enumerate_add_match_is_initialized(udev_enum);
> +	if (ret < 0)
> +		goto done;
> +
>  	ret = udev_enumerate_scan_devices(udev_enum);
>  	if (ret < 0)
>  		goto done;
> @@ -102,10 +146,16 @@ int DeviceEnumeratorUdev::enumerate()
>  			goto done;
>  		}
>  
> -		addDevice(devnode);
> +		if (addUdevDevice(dev) < 0) {
> +			udev_device_unref(dev);
> +			ret = -ENODEV;
> +			goto done;
> +		}
>  
>  		udev_device_unref(dev);
> +		continue;

This line isn't needed anymore.

How about

	ret = addUdevDevice(dev);
	udev_device_unref(dev);
	if (ret < 0)
		break;

to simplify the code and propagate the error ?

>  	}
> +
>  done:
>  	udev_enumerate_unref(udev_enum);
>  	if (ret < 0)
> @@ -122,6 +172,48 @@ done:
>  	return 0;
>  }
>  
> +int DeviceEnumeratorUdev::populateMediaDevice(std::shared_ptr<MediaDevice> media)
> +{
> +	unsigned int pendingNodes = 0;
> +	int ret;
> +
> +	/* Associate entities to device node paths. */
> +	for (MediaEntity *entity : media->entities()) {
> +		if (entity->deviceMajor() == 0 && entity->deviceMinor() == 0)
> +			continue;
> +
> +		std::string deviceNode = lookupDeviceNode(entity->deviceMajor(),
> +							  entity->deviceMinor());
> +		dev_t devnum = makedev(entity->deviceMajor(),
> +				       entity->deviceMinor());
> +
> +		/* Take device from orphan list first, if it is in the list. */
> +		if (std::find(orphans_.begin(), orphans_.end(), devnum) != orphans_.end()) {
> +			if (deviceNode.empty())
> +				return -EINVAL;
> +
> +			ret = entity->setDeviceNode(deviceNode);
> +			if (ret)
> +				return ret;
> +
> +			orphans_.remove(devnum);
> +			continue;
> +		}
> +
> +		if (::access(deviceNode.c_str(), R_OK | W_OK) < 0) {
> +			deps_[media].push_back(devnum);
> +			devnumToDevice_[devnum] = media;
> +			devnumToEntity_[devnum] = entity;
> +			pendingNodes++;
> +			continue;
> +		}
> +
> +		entity->setDeviceNode(deviceNode);

What exactly would go wrong if you removed the access() check and the
entity->setDeviceNode() ? I agree that a device node may be ready here,
but if it's not in the orphans list it will get there later, and will be
added to the media device at that time. Trying to add it early here just
in case is an optimisation that I believe makes more harm than good, as
it opens the door to more race conditions. For instance, maybe udev
first sets permissions and then renames the device node, and we could
run just between those two operations.

Furthermore, when DeviceEnumeratorUdev::addV4L2Device() is later called,
the device will not be in devnumToEntity_, so it will be added to
orphans_, even though it will already be in use by the media device. I
don't think that's right.

As there's no hurry to make media devices available, I would simplify
the code and stay on the safe side.

> +	}
> +
> +	return pendingNodes;
> +}
> +
>  std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor)
>  {
>  	struct udev_device *device;
> @@ -143,6 +235,46 @@ std::string DeviceEnumeratorUdev::lookupDeviceNode(int major, int minor)
>  	return deviceNode;
>  }
>  
> +/**
> + * \brief Add a V4L2 device to the media device that needs it

s/that needs it/that it belongs to/ ?

> + * \param[in] devnum major:minor number of V4L2 device to add, as a dev_t
> + *
> + * Add V4L2 device identified by \a devnum to the MediaDevice that needs it,

Same here.

> + * if such a MediaDevice has been created. Otherwise add the V4L2 device to
> + * the orphan list. If the V4L2 device is added to a MediaDevice, and it is
> + * the last V4L2 device that the MediaDevice needs, then the MediaDevice
> + * will be sent up to the pipeline handler.

There's one intermediate step, so I would say something along the lines
of "then the MediaDevice is added to the DeviceEnumerator, where it is
available for pipeline handlers."

> + *
> + * \return 0 on success or a negative error code otherwise
> + */
> +int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum)
> +{
> +	MediaEntity *entity = devnumToEntity_[devnum];
> +	if (!entity) {
> +		orphans_.push_back(devnum);
> +		return 0;
> +	}
> +
> +	std::string deviceNode = lookupDeviceNode(entity->deviceMajor(),
> +						  entity->deviceMinor());
> +	if (deviceNode.empty())
> +		return -EINVAL;
> +
> +	int ret = entity->setDeviceNode(deviceNode);
> +	if (ret)
> +		return ret;
> +
> +	std::shared_ptr<MediaDevice> media = devnumToDevice_[devnum];

I'm really tempted to use normal pointer internally, and create the
shared pointer in addDevice(). If you do so, the above line could be
replaced with

	MediaDevice *media = entity->device();

and the devnumToDevice_ map could be removed, which I think would
simplify the code. You didn't seem to like this when I proposed it in
the review of v1, do you think it's a bad idea ? Or would you prefer
fixing it on top ? (I may submit a patch to do so :-))

> +	deps_[media].remove(devnum);
> +	devnumToDevice_.erase(devnum);
> +	devnumToEntity_.erase(devnum);
> +
> +	if (deps_[media].empty())
> +		addDevice(media);

You should also deps_.erase(media).

> +
> +	return 0;
> +}
> +
>  void DeviceEnumeratorUdev::udevNotify(EventNotifier *notifier)
>  {
>  	struct udev_device *dev = udev_monitor_receive_device(monitor_);
> @@ -153,9 +285,11 @@ void DeviceEnumeratorUdev::udevNotify(EventNotifier *notifier)
>  		<< action << " device " << udev_device_get_devnode(dev);
>  
>  	if (action == "add") {
> -		addDevice(deviceNode);
> +		addUdevDevice(dev);
>  	} else if (action == "remove") {
> -		removeDevice(deviceNode);
> +		const char *subsystem = udev_device_get_subsystem(dev);
> +		if (subsystem && !strcmp(subsystem, "media"))
> +			removeDevice(deviceNode);

We could create a removeUdevDevice() method for symmetry, but I don't
think that's required yet.

>  	}
>  
>  	udev_device_unref(dev);
> diff --git a/src/libcamera/include/device_enumerator.h b/src/libcamera/include/device_enumerator.h
> index 02aec3b..b7d1b0a 100644
> --- a/src/libcamera/include/device_enumerator.h
> +++ b/src/libcamera/include/device_enumerator.h
> @@ -44,7 +44,8 @@ public:
>  	std::shared_ptr<MediaDevice> search(const DeviceMatch &dm);
>  
>  protected:
> -	int addDevice(const std::string &deviceNode);
> +	std::shared_ptr<MediaDevice> createDevice(const std::string &deviceNode);
> +	void addDevice(std::shared_ptr<MediaDevice> media);
>  	void removeDevice(const std::string &deviceNode);
>  
>  private:
> diff --git a/src/libcamera/include/device_enumerator_sysfs.h b/src/libcamera/include/device_enumerator_sysfs.h
> index 8d3adc9..cc33f75 100644
> --- a/src/libcamera/include/device_enumerator_sysfs.h
> +++ b/src/libcamera/include/device_enumerator_sysfs.h
> @@ -9,6 +9,8 @@
>  
>  #include <string>

You should #include <memory> for std::shared_ptr.

>  
> +#include "media_device.h"
> +

You can just forward-declare class MediaDevice.

>  #include "device_enumerator.h"
>  
>  namespace libcamera {
> @@ -20,7 +22,8 @@ public:
>  	int enumerate();
>  
>  private:
> -	std::string lookupDeviceNode(int major, int minor);
> +	int populateMediaDevice(std::shared_ptr<MediaDevice> media);
> +	std::string lookupDeviceNode(int major, int minor) final;
>  };
>  
>  } /* namespace libcamera */
> diff --git a/src/libcamera/include/device_enumerator_udev.h b/src/libcamera/include/device_enumerator_udev.h
> index 80f9372..3f8af52 100644
> --- a/src/libcamera/include/device_enumerator_udev.h
> +++ b/src/libcamera/include/device_enumerator_udev.h
> @@ -7,8 +7,13 @@
>  #ifndef __LIBCAMERA_DEVICE_ENUMERATOR_UDEV_H__
>  #define __LIBCAMERA_DEVICE_ENUMERATOR_UDEV_H__
>  
> +#include <list>
> +#include <map>
>  #include <string>

You need #include <memory> here too.

>  
> +#include <libudev.h>

You can just forward-declare struct udev_device.

> +#include <sys/types.h>
> +

You can mix the C++ and C headers.

>  #include "device_enumerator.h"
>  
>  struct udev;
> @@ -17,6 +22,8 @@ struct udev_monitor;
>  namespace libcamera {
>  
>  class EventNotifier;
> +class MediaDevice;
> +class MediaEntity;
>  
>  class DeviceEnumeratorUdev : public DeviceEnumerator
>  {
> @@ -32,8 +39,17 @@ private:
>  	struct udev_monitor *monitor_;
>  	EventNotifier *notifier_;
>  
> +	std::map<std::shared_ptr<MediaDevice>, std::list<dev_t>> deps_;
> +	std::map<dev_t, std::shared_ptr<MediaDevice>> devnumToDevice_;
> +	std::map<dev_t, MediaEntity *> devnumToEntity_;
> +
> +	std::list<dev_t> orphans_;
> +
> +	int addUdevDevice(struct udev_device *dev);
> +	int populateMediaDevice(std::shared_ptr<MediaDevice> media);
>  	std::string lookupDeviceNode(int major, int minor) final;
>  
> +	int addV4L2Device(dev_t devnum);
>  	void udevNotify(EventNotifier *notifier);
>  };
>