| Message ID | 20200217110324.7431-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 68daa9302f90833ce345cb33ffcf075f23cbfc9a |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your work. On 2020-02-17 13:03:24 +0200, 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@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > 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; > > 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 > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
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@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@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: >
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@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@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:
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@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@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: >
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; 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:
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@ideasonboard.com> --- 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(-)