[libcamera-devel] [PATCH v2] libcamera: device_enumerator: Don't stop if one device fails

[Kieran Bingham]

Hi Laurent,

On 17/02/2020 11:03, Laurent Pinchart wrote:
> If one device fails to enumerate, which isn't supposed to happen under
> normal conditions, both the sysfs and the udev enumerators stop
> enumeration of further devices. This potentially prevents working
> devices from being detected and handled. Fix it by skipping the faulty
> device.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart at ideasonboard.com>

The addition of the prints for /which/ device fails don't help my use
case as I fail before getting to those prints
(DeviceEnumerator::createDevice)

We can't add any further introspection of the MediaDevice in
::createDevice anyway, so that can't be extended to say more information
about the device.

MediaDevice::populate() however is painful, as it discards the actual
return value of the MEDIA_IOC_DEVICE_INFO, and
MEDIA_IOC_G_TOPOLOGY,ioctls in event of those failing and instead
returns -EINVAL ...

But that is not a topic for /this/ patch.


Reviewed-by: Kieran Bingham <kieran.bingham at ideasonboard.com>
(and still Tested-by: KB too if you desire)


> ---
> Changes since v1:
> 
> - Add more warning messages to DeviceEnumeratorUdev
> - Use media->deviceNode() and print the driver name where appropriate
> ---
>  src/libcamera/device_enumerator_sysfs.cpp | 16 +++++++--------
>  src/libcamera/device_enumerator_udev.cpp  | 25 ++++++++++++++++-------
>  2 files changed, 26 insertions(+), 15 deletions(-)
> 
> diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp
> index ad26affb38a3..197ca235879b 100644
> --- a/src/libcamera/device_enumerator_sysfs.cpp
> +++ b/src/libcamera/device_enumerator_sysfs.cpp
> @@ -33,7 +33,6 @@ int DeviceEnumeratorSysfs::enumerate()
>  {
>  	struct dirent *ent;
>  	DIR *dir;
> -	int ret = 0;
>  
>  	static const char * const sysfs_dirs[] = {
>  		"/sys/subsystem/media/devices",
> @@ -74,14 +73,15 @@ int DeviceEnumeratorSysfs::enumerate()
>  		}
>  
>  		std::shared_ptr<MediaDevice> media = createDevice(devnode);
> -		if (!media) {
> -			ret = -ENODEV;
> -			break;
> -		}
> +		if (!media)
> +			continue;
>  
>  		if (populateMediaDevice(media) < 0) {
> -			ret = -ENODEV;
> -			break;
> +			LOG(DeviceEnumerator, Warning)
> +				<< "Failed to populate media device "
> +				<< media->deviceNode()
> +				<< " (" << media->driver() << "), skipping";
> +			continue;
>  		}
>  
>  		addDevice(media);
> @@ -89,7 +89,7 @@ int DeviceEnumeratorSysfs::enumerate()
>  
>  	closedir(dir);
>  
> -	return ret;
> +	return 0;
>  }
>  
>  int DeviceEnumeratorSysfs::populateMediaDevice(const std::shared_ptr<MediaDevice> &media)
> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp
> index b2c5fd221dcd..87638c59761c 100644
> --- a/src/libcamera/device_enumerator_udev.cpp
> +++ b/src/libcamera/device_enumerator_udev.cpp
> @@ -82,8 +82,15 @@ int DeviceEnumeratorUdev::addUdevDevice(struct udev_device *dev)
>  			return -ENODEV;
>  

Interestingly - actually in my testing of the failures, I don't get to
the code below.

The failure occurs during media->populate() which is called from
createDevice, which has just returned -ENODEV above here.


And while it is valid to call media->driver() below - it's not valid
here, as the created device didn't get created :-D



>  		int ret = populateMediaDevice(media);
> -		if (ret == 0)
> -			addDevice(media);
> +		if (ret < 0) {
> +			LOG(DeviceEnumerator, Warning)
> +				<< "Failed to populate media device "
> +				<< media->deviceNode()
> +				<< " (" << media->driver() << "), skipping";
> +			return ret;



> +		}
> +
> +		addDevice(media);
>  		return 0;
>  	}
>  
> @@ -141,14 +148,18 @@ int DeviceEnumeratorUdev::enumerate()
>  		devnode = udev_device_get_devnode(dev);
>  		if (!devnode) {
>  			udev_device_unref(dev);
> -			ret = -ENODEV;
> -			goto done;
> +			LOG(DeviceEnumerator, Warning)
> +				<< "Failed to get device node for '"
> +				<< syspath << "', skipping";
> +			continue;
>  		}
>  
> -		ret = addUdevDevice(dev);
> +		if (addUdevDevice(dev) < 0)
> +			LOG(DeviceEnumerator, Warning)
> +				<< "Failed to add device for '"
> +				<< syspath << "', skipping";
> +
>  		udev_device_unref(dev);
> -		if (ret < 0)
> -			break;
>  	}
>  
>  done:
> 

-- 
Regards
--
Kieran