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

[Laurent Pinchart]

Hi Kieran,

On Mon, Feb 17, 2020 at 11:56:42AM +0000, Kieran Bingham wrote:
> 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.

Yes, if we fail to retrieve information from the device, then we can
hardly print it :-)

> 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.

Agreed.

It could also be useful to make MediaDevice inherit from Loggable.

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

Absolutely, thank you.

> > ---
> > 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,

Laurent Pinchart