| Message ID | 20200320210435.8876-2-laurent.pinchart@ideasonboard.com |
|---|---|
| State | Accepted |
| Commit | 3f97be923c11df4324f20a94591dc5562a05c7ba |
| Headers | show |
| Series |
|
| Related | show |
Hi Laurent, Thanks for your patch. On 2020-03-20 23:04:35 +0200, Laurent Pinchart wrote: > Commit 68daa9302f90 ("libcamera: device_enumerator: Don't stop if one > device fails") introduced a bug in device enumeration. Previously, a > media device would only be added when all its dependencies were present, > as indicated by populateMediaDevice() returning 0. The commit changed > the logic to propagate the populateMediaDevice() return code up, but > mistakenly added the media device to the enumerator even when > dependencies were missing. This causes media devices enumerated with > missing dependencies to be added twice, once when enumerating the media > device itself, and a second time when all the dependencies are found. > > Fix it by deferring addition of the media device when dependencies are > missing, and add debug messages to track this process. > > Fixes: 68daa9302f90 ("libcamera: device_enumerator: Don't stop if one device fails") > Reported-by: Andrey Konovalov <andrey.konovalov@linaro.org> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> > --- > src/libcamera/device_enumerator_udev.cpp | 10 ++++++++++ > 1 file changed, 10 insertions(+) > > diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp > index 87638c59761c..e55350544feb 100644 > --- a/src/libcamera/device_enumerator_udev.cpp > +++ b/src/libcamera/device_enumerator_udev.cpp > @@ -90,6 +90,13 @@ int DeviceEnumeratorUdev::addUdevDevice(struct udev_device *dev) > return ret; > } > > + if (ret) { > + LOG(DeviceEnumerator, Debug) > + << "Defer media device " << media->deviceNode() > + << " due to " << ret << " missing dependencies"; > + return 0; > + } > + > addDevice(media); > return 0; > } > @@ -313,6 +320,9 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) > deps->deps_.erase(devnum); > > if (deps->deps_.empty()) { > + LOG(DeviceEnumerator, Debug) > + << "All dependencies for media device " > + << deps->media_->deviceNode() << " found"; > addDevice(deps->media_); > pending_.remove(*deps); > } > -- > Regards, > > Laurent Pinchart > > _______________________________________________ > libcamera-devel mailing list > libcamera-devel@lists.libcamera.org > https://lists.libcamera.org/listinfo/libcamera-devel
Hi Laurent, On 23/03/2020 11:05, Niklas Söderlund wrote: > Hi Laurent, > > Thanks for your patch. > > On 2020-03-20 23:04:35 +0200, Laurent Pinchart wrote: >> Commit 68daa9302f90 ("libcamera: device_enumerator: Don't stop if one >> device fails") introduced a bug in device enumeration. Previously, a >> media device would only be added when all its dependencies were present, >> as indicated by populateMediaDevice() returning 0. The commit changed >> the logic to propagate the populateMediaDevice() return code up, but >> mistakenly added the media device to the enumerator even when >> dependencies were missing. This causes media devices enumerated with >> missing dependencies to be added twice, once when enumerating the media >> device itself, and a second time when all the dependencies are found. >> >> Fix it by deferring addition of the media device when dependencies are >> missing, and add debug messages to track this process. >> >> Fixes: 68daa9302f90 ("libcamera: device_enumerator: Don't stop if one device fails") >> Reported-by: Andrey Konovalov <andrey.konovalov@linaro.org> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> > > Reviewed-by: Niklas Söderlund <niklas.soderlund@ragnatech.se> Sure, looks ok. Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com> > >> --- >> src/libcamera/device_enumerator_udev.cpp | 10 ++++++++++ >> 1 file changed, 10 insertions(+) >> >> diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp >> index 87638c59761c..e55350544feb 100644 >> --- a/src/libcamera/device_enumerator_udev.cpp >> +++ b/src/libcamera/device_enumerator_udev.cpp >> @@ -90,6 +90,13 @@ int DeviceEnumeratorUdev::addUdevDevice(struct udev_device *dev) >> return ret; >> } >> >> + if (ret) { >> + LOG(DeviceEnumerator, Debug) >> + << "Defer media device " << media->deviceNode() >> + << " due to " << ret << " missing dependencies"; >> + return 0; >> + } >> + >> addDevice(media); >> return 0; >> } >> @@ -313,6 +320,9 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) >> deps->deps_.erase(devnum); >> >> if (deps->deps_.empty()) { >> + LOG(DeviceEnumerator, Debug) >> + << "All dependencies for media device " >> + << deps->media_->deviceNode() << " found"; >> addDevice(deps->media_); >> pending_.remove(*deps); >> } >> -- >> Regards, >> >> Laurent Pinchart >> >> _______________________________________________ >> libcamera-devel mailing list >> libcamera-devel@lists.libcamera.org >> https://lists.libcamera.org/listinfo/libcamera-devel >
diff --git a/src/libcamera/device_enumerator_udev.cpp b/src/libcamera/device_enumerator_udev.cpp index 87638c59761c..e55350544feb 100644 --- a/src/libcamera/device_enumerator_udev.cpp +++ b/src/libcamera/device_enumerator_udev.cpp @@ -90,6 +90,13 @@ int DeviceEnumeratorUdev::addUdevDevice(struct udev_device *dev) return ret; } + if (ret) { + LOG(DeviceEnumerator, Debug) + << "Defer media device " << media->deviceNode() + << " due to " << ret << " missing dependencies"; + return 0; + } + addDevice(media); return 0; } @@ -313,6 +320,9 @@ int DeviceEnumeratorUdev::addV4L2Device(dev_t devnum) deps->deps_.erase(devnum); if (deps->deps_.empty()) { + LOG(DeviceEnumerator, Debug) + << "All dependencies for media device " + << deps->media_->deviceNode() << " found"; addDevice(deps->media_); pending_.remove(*deps); }
Commit 68daa9302f90 ("libcamera: device_enumerator: Don't stop if one device fails") introduced a bug in device enumeration. Previously, a media device would only be added when all its dependencies were present, as indicated by populateMediaDevice() returning 0. The commit changed the logic to propagate the populateMediaDevice() return code up, but mistakenly added the media device to the enumerator even when dependencies were missing. This causes media devices enumerated with missing dependencies to be added twice, once when enumerating the media device itself, and a second time when all the dependencies are found. Fix it by deferring addition of the media device when dependencies are missing, and add debug messages to track this process. Fixes: 68daa9302f90 ("libcamera: device_enumerator: Don't stop if one device fails") Reported-by: Andrey Konovalov <andrey.konovalov@linaro.org> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com> --- src/libcamera/device_enumerator_udev.cpp | 10 ++++++++++ 1 file changed, 10 insertions(+)