[libcamera-devel,v5,1/6] libcamera: CameraManager: Drop the vector of created PipelineHandlers

4064 diff mbox series

The pipes_ vector was initially used to store pipeline handlers
instances with the CameraManager when it cannot be referenced from
anywhere else. It was used to retrieve cameras and deleting pipeline
handlers when stopping the camera manager.

In f3695e9b09ce ("libcamera: camera_manager: Register cameras with the
camera manager"), cameras started to get registered directly with camera
manager and in 5b02e03199b7 ("libcamera: camera: Associate cameras with
their pipeline handler") pipeline handler started to get stored in a
std::shared_ptr<> with each camera starting to hold a strong reference
to its associated pipeline-handler. At this point, both the camera
manager and the camera held a strong reference to the pipeline handler.

Since the additional reference held by the camera manager gets released
only on cleanup(), this lurking reference held on pipeline handler, did
not allow it to get destroyed the even when cameras instances have been
destroyed. This situation of having a pipeline handler instance around
without having a camera, may lead to problems (one of them explained
below) especially when the camera manager is still running.

It was noticed that, there was a dangling driver directory issue (tested
for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on 'unbind' → 'bind'
operation while the CameraManager is running. The directories were still
kept around even after 'unbind' because of the lurking reference of
pipeline handler holding onto them. That reference would clear if and
only if the CameraManager is stopped and then only directories were
getting removed in the above stated path.

Rather than writing a fix to release the pipeline handlers' reference
from camera manager on camera disconnection, it is decided to eliminate
the pipes_ vector from CameraManager moving forwards. There is no
point in holding a reference to it from camera manager's point-of-view
at this stage. It also helps us to fix the issue as explained above.

Now that the pipeline handler instances are referenced via cameras
only, it can happen that the destruction of last camera instance may
result into destruction of pipeline handler itself. Such a possibility
exists PipelineHandler::disconnect(), where the pipeline handler itself
can get destroyed while removing the camera. This is acceptable as long
as we make sure that there is no access of pipeline handler's members
later on in the code-path. Address this situation and also add a
detailed comment about it.

Signed-off-by: Umang Jain <email@uajain.com>
---
 src/libcamera/camera_manager.cpp   |  8 ++------
 src/libcamera/pipeline_handler.cpp | 18 +++++++++++++++---
 2 files changed, 17 insertions(+), 9 deletions(-)

Hi Umang,

Thank you for the patch.

On Tue, Jun 16, 2020 at 07:45:33PM +0000, Umang Jain wrote:
> The pipes_ vector was initially used to store pipeline handlers
> instances with the CameraManager when it cannot be referenced from
> anywhere else. It was used to retrieve cameras and deleting pipeline
> handlers when stopping the camera manager.
> 
> In f3695e9b09ce ("libcamera: camera_manager: Register cameras with the
> camera manager"), cameras started to get registered directly with camera
> manager and in 5b02e03199b7 ("libcamera: camera: Associate cameras with
> their pipeline handler") pipeline handler started to get stored in a

s/handler/handlers/

> std::shared_ptr<> with each camera starting to hold a strong reference
> to its associated pipeline-handler. At this point, both the camera

s/pipeline-handler/pipeline handler/

> manager and the camera held a strong reference to the pipeline handler.
> 
> Since the additional reference held by the camera manager gets released
> only on cleanup(), this lurking reference held on pipeline handler, did

s/handler,/handler/

> not allow it to get destroyed the even when cameras instances have been

s/the //

> destroyed. This situation of having a pipeline handler instance around
> without having a camera, may lead to problems (one of them explained

s/camera,/camera/

> below) especially when the camera manager is still running.
> 
> It was noticed that, there was a dangling driver directory issue (tested
> for UVC camera - in /sys/bus/usb/drivers/uvcvideo) on 'unbind' → 'bind'
> operation while the CameraManager is running. The directories were still
> kept around even after 'unbind' because of the lurking reference of
> pipeline handler holding onto them. That reference would clear if and
> only if the CameraManager is stopped and then only directories were
> getting removed in the above stated path.
> 
> Rather than writing a fix to release the pipeline handlers' reference
> from camera manager on camera disconnection, it is decided to eliminate
> the pipes_ vector from CameraManager moving forwards. There is no
> point in holding a reference to it from camera manager's point-of-view
> at this stage. It also helps us to fix the issue as explained above.
> 
> Now that the pipeline handler instances are referenced via cameras
> only, it can happen that the destruction of last camera instance may

s/of last/of the last/

> result into destruction of pipeline handler itself. Such a possibility

s/into/in/
s/pipeline/the pipeline/

> exists PipelineHandler::disconnect(), where the pipeline handler itself

s/exists/exists in/

> can get destroyed while removing the camera. This is acceptable as long
> as we make sure that there is no access of pipeline handler's members

s/of/to the/

> later on in the code-path. Address this situation and also add a

s/code-path/code path/

> detailed comment about it.
> 
> Signed-off-by: Umang Jain <email@uajain.com>

A commit message longer than the commit itself is often the sign a
problem well understood :-)

> ---
>  src/libcamera/camera_manager.cpp   |  8 ++------
>  src/libcamera/pipeline_handler.cpp | 18 +++++++++++++++---
>  2 files changed, 17 insertions(+), 9 deletions(-)
> 
> diff --git a/src/libcamera/camera_manager.cpp b/src/libcamera/camera_manager.cpp
> index 576856a..dbdc78e 100644
> --- a/src/libcamera/camera_manager.cpp
> +++ b/src/libcamera/camera_manager.cpp
> @@ -63,7 +63,6 @@ private:
>  	bool initialized_;
>  	int status_;
>  
> -	std::vector<std::shared_ptr<PipelineHandler>> pipes_;
>  	std::unique_ptr<DeviceEnumerator> enumerator_;
>  
>  	IPAManager ipaManager_;
> @@ -144,7 +143,6 @@ int CameraManager::Private::init()
>  			LOG(Camera, Debug)
>  				<< "Pipeline handler \"" << factory->name()
>  				<< "\" matched";
> -			pipes_.push_back(std::move(pipe));
>  		}
>  	}
>  
> @@ -158,11 +156,9 @@ void CameraManager::Private::cleanup()
>  	/* TODO: unregister hot-plug callback here */
>  
>  	/*
> -	 * Release all references to cameras and pipeline handlers to ensure
> -	 * they all get destroyed before the device enumerator deletes the
> -	 * media devices.
> +	 * Release all references to cameras to ensure they all get destroyed
> +	 * before the device enumerator deletes the media devices.
>  	 */
> -	pipes_.clear();
>  	cameras_.clear();
>  
>  	enumerator_.reset(nullptr);
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index a0f6b0f..c457be9 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -559,7 +559,21 @@ void PipelineHandler::mediaDeviceDisconnected(MediaDevice *media)
>   */
>  void PipelineHandler::disconnect()
>  {
> -	for (std::weak_ptr<Camera> ptr : cameras_) {
> +	/*
> +	 * Each camera holds a reference to its associated pipeline handler
> +	 * instance. Hence, when the last camera is dropped, the pipeline
> +	 * handler will get destroyed by the last manager_->removeCamera(camera)
> +	 * call in the loop below.
> +	 *
> +	 * This is acceptable as long as we make sure that the code path does not
> +	 * access any member of the (already destroyed)pipeline handler instance

s/pipeline/ pipeline/

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> +	 * afterwards. Therefore, we move the cameras_ vector to a local temporary
> +	 * container to avoid accessing freed memory later i.e. to explicitly run
> +	 * cameras_.clear().
> +	 */
> +	std::vector<std::weak_ptr<Camera>> cameras{ std::move(cameras_) };
> +
> +	for (std::weak_ptr<Camera> ptr : cameras) {
>  		std::shared_ptr<Camera> camera = ptr.lock();
>  		if (!camera)
>  			continue;
> @@ -567,8 +581,6 @@ void PipelineHandler::disconnect()
>  		camera->disconnect();
>  		manager_->removeCamera(camera.get());
>  	}
> -
> -	cameras_.clear();
>  }
>  
>  /**