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

[Kieran Bingham]

Hi Laurent,

On 17/02/2020 23:24, Laurent Pinchart wrote:
> 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.

Ohh, yes that's a nice solution for it!

It's quick too - 5 minute implementation posted :-)

--
Kieran


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