| Message ID | 20200214123715.13026-1-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, On 14/02/2020 12:37, 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. Thank you, This does indeed allow the system to continue and function with the available cameras. Interestingly, in the particular case that I was investigating - it wasn't the newly added device that failed ... it was the existing camera interface. That can be investigated separately. -- Kieran > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > --- > src/libcamera/device_enumerator_sysfs.cpp | 15 +++++++-------- > src/libcamera/device_enumerator_udev.cpp | 8 +++++--- > 2 files changed, 12 insertions(+), 11 deletions(-) > > diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp > index ad26affb38a3..bde736226f95 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,14 @@ 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 /dev/media" > + << idx << ", skipping"; > + continue; > } > > addDevice(media); > @@ -89,7 +88,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..3837aaf97e6a 100644 > --- a/src/libcamera/device_enumerator_udev.cpp > +++ b/src/libcamera/device_enumerator_udev.cpp > @@ -145,10 +145,12 @@ int DeviceEnumeratorUdev::enumerate() > goto done; > } > > - 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: >
On 17/02/2020 08:23, Kieran Bingham wrote: > Hi Laurent, > > On 14/02/2020 12:37, 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. > > Thank you, > > This does indeed allow the system to continue and function with the > available cameras. > > Interestingly, in the particular case that I was investigating - it > wasn't the newly added device that failed ... it was the existing camera > interface. > > That can be investigated separately. > > -- > Kieran > > > >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Thanks, indeed this now continues to function and produces an appropriate message for the failing device components: > [2:20:54.425174891] [3209] ERR MediaDevice media_device.cpp:242 Failed to enumerate topology: Bad address > [2:20:54.425281647] [3209] INFO DeviceEnumerator device_enumerator.cpp:217 Unable to populate media device /dev/media0 (Invalid argument), skipping > [2:20:54.425393663] [3209] WARN DeviceEnumerator device_enumerator_udev.cpp:149 Failed to add device for '/sys/devices/platform/soc/fe801000.csi/media0', skipping > Using camera VF0520 Live! Cam Sync: VF0520 L Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> And with one small comment considered below: Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> --- >> src/libcamera/device_enumerator_sysfs.cpp | 15 +++++++-------- >> src/libcamera/device_enumerator_udev.cpp | 8 +++++--- >> 2 files changed, 12 insertions(+), 11 deletions(-) >> >> diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp >> index ad26affb38a3..bde736226f95 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,14 @@ 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 /dev/media" >> + << idx << ", skipping"; Shouldn't this use something like media->deviceNode() ? (Perhaps even printing media->driver() and media->model for guidance too?) >> + continue; >> } >> >> addDevice(media); >> @@ -89,7 +88,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..3837aaf97e6a 100644 >> --- a/src/libcamera/device_enumerator_udev.cpp >> +++ b/src/libcamera/device_enumerator_udev.cpp >> @@ -145,10 +145,12 @@ int DeviceEnumeratorUdev::enumerate() >> goto done; >> } >> >> - 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 10:41:23AM +0000, Kieran Bingham wrote: > On 17/02/2020 08:23, Kieran Bingham wrote: > > On 14/02/2020 12:37, 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. > > > > Thank you, > > > > This does indeed allow the system to continue and function with the > > available cameras. > > > > Interestingly, in the particular case that I was investigating - it > > wasn't the newly added device that failed ... it was the existing camera > > interface. > > > > That can be investigated separately. > > > >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Thanks, indeed this now continues to function and produces an > appropriate message for the failing device components: > > > [2:20:54.425174891] [3209] ERR MediaDevice media_device.cpp:242 Failed to enumerate topology: Bad address > > [2:20:54.425281647] [3209] INFO DeviceEnumerator device_enumerator.cpp:217 Unable to populate media device /dev/media0 (Invalid argument), skipping > > [2:20:54.425393663] [3209] WARN DeviceEnumerator device_enumerator_udev.cpp:149 Failed to add device for '/sys/devices/platform/soc/fe801000.csi/media0', skipping > > Using camera VF0520 Live! Cam Sync: VF0520 L > > Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > And with one small comment considered below: > > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > > >> --- > >> src/libcamera/device_enumerator_sysfs.cpp | 15 +++++++-------- > >> src/libcamera/device_enumerator_udev.cpp | 8 +++++--- > >> 2 files changed, 12 insertions(+), 11 deletions(-) > >> > >> diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp > >> index ad26affb38a3..bde736226f95 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,14 @@ 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 /dev/media" > >> + << idx << ", skipping"; > > Shouldn't this use something like media->deviceNode() ? > > (Perhaps even printing media->driver() and media->model for guidance too?) That's a good idea, I'll submit a v2 for you to test :-) > >> + continue; > >> } > >> > >> addDevice(media); > >> @@ -89,7 +88,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..3837aaf97e6a 100644 > >> --- a/src/libcamera/device_enumerator_udev.cpp > >> +++ b/src/libcamera/device_enumerator_udev.cpp > >> @@ -145,10 +145,12 @@ int DeviceEnumeratorUdev::enumerate() > >> goto done; > >> } > >> > >> - 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 10:43, Laurent Pinchart wrote: > Hi Kieran, > > On Mon, Feb 17, 2020 at 10:41:23AM +0000, Kieran Bingham wrote: >> On 17/02/2020 08:23, Kieran Bingham wrote: >>> On 14/02/2020 12:37, 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. >>> >>> Thank you, >>> >>> This does indeed allow the system to continue and function with the >>> available cameras. >>> >>> Interestingly, in the particular case that I was investigating - it >>> wasn't the newly added device that failed ... it was the existing camera >>> interface. >>> >>> That can be investigated separately. >>> >>>> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> >> >> Thanks, indeed this now continues to function and produces an >> appropriate message for the failing device components: >> >>> [2:20:54.425174891] [3209] ERR MediaDevice media_device.cpp:242 Failed to enumerate topology: Bad address >>> [2:20:54.425281647] [3209] INFO DeviceEnumerator device_enumerator.cpp:217 Unable to populate media device /dev/media0 (Invalid argument), skipping >>> [2:20:54.425393663] [3209] WARN DeviceEnumerator device_enumerator_udev.cpp:149 Failed to add device for '/sys/devices/platform/soc/fe801000.csi/media0', skipping >>> Using camera VF0520 Live! Cam Sync: VF0520 L >> >> Tested-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >> And with one small comment considered below: >> >> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> >> >>>> --- >>>> src/libcamera/device_enumerator_sysfs.cpp | 15 +++++++-------- >>>> src/libcamera/device_enumerator_udev.cpp | 8 +++++--- >>>> 2 files changed, 12 insertions(+), 11 deletions(-) >>>> >>>> diff --git a/src/libcamera/device_enumerator_sysfs.cpp b/src/libcamera/device_enumerator_sysfs.cpp >>>> index ad26affb38a3..bde736226f95 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,14 @@ 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 /dev/media" >>>> + << idx << ", skipping"; >> >> Shouldn't this use something like media->deviceNode() ? >> >> (Perhaps even printing media->driver() and media->model for guidance too?) > > That's a good idea, I'll submit a v2 for you to test :-) There's also the local variable available which was used to construct the MediaDevice; - devnode; But getting the strings from the MediaDevice and being more verbose about the failure seems a bit more beneficial all the same. -- Kieran > >>>> + continue; >>>> } >>>> >>>> addDevice(media); >>>> @@ -89,7 +88,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..3837aaf97e6a 100644 >>>> --- a/src/libcamera/device_enumerator_udev.cpp >>>> +++ b/src/libcamera/device_enumerator_udev.cpp >>>> @@ -145,10 +145,12 @@ int DeviceEnumeratorUdev::enumerate() >>>> goto done; >>>> } >>>> >>>> - 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..bde736226f95 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,14 @@ 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 /dev/media" + << idx << ", skipping"; + continue; } addDevice(media); @@ -89,7 +88,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..3837aaf97e6a 100644 --- a/src/libcamera/device_enumerator_udev.cpp +++ b/src/libcamera/device_enumerator_udev.cpp @@ -145,10 +145,12 @@ int DeviceEnumeratorUdev::enumerate() goto done; } - 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> --- src/libcamera/device_enumerator_sysfs.cpp | 15 +++++++-------- src/libcamera/device_enumerator_udev.cpp | 8 +++++--- 2 files changed, 12 insertions(+), 11 deletions(-)