[libcamera-devel,v2,2/2] libcamera: pipeline_handler: Make lock() and unlock() thread-safe

15221 diff mbox series

The PipelineHandler lock() and unlock() functions are documented as
thread-safe, but they're not. Fix them using a mutex.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
---
Changes since v1:

- Add thread safety annotation
- Comment that lockOwner_ does not indicate ownership of lock_
---
 include/libcamera/internal/pipeline_handler.h | 4 +++-
 src/libcamera/pipeline_handler.cpp            | 5 +++++
 2 files changed, 8 insertions(+), 1 deletion(-)

Hi Laurent,

On Mon, Dec 27, 2021 at 01:12:55AM +0200, Laurent Pinchart wrote:
> The PipelineHandler lock() and unlock() functions are documented as
> thread-safe, but they're not. Fix them using a mutex.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> ---
> Changes since v1:
>
> - Add thread safety annotation
> - Comment that lockOwner_ does not indicate ownership of lock_
> ---
>  include/libcamera/internal/pipeline_handler.h | 4 +++-
>  src/libcamera/pipeline_handler.cpp            | 5 +++++
>  2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index a7331cedfb16..e5b8ffb4db3d 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -14,6 +14,7 @@
>  #include <sys/types.h>
>  #include <vector>
>
> +#include <libcamera/base/mutex.h>
>  #include <libcamera/base/object.h>
>
>  #include <libcamera/controls.h>
> @@ -88,7 +89,8 @@ private:
>
>  	const char *name_;
>
> -	bool lockOwner_;
> +	Mutex lock_;
> +	bool lockOwner_ LIBCAMERA_TSA_GUARDED_BY(lock_); /* *Not* ownership of lock_ */

Took me a while to find out what the comment means..

Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>

Thanks
   j
>
>  	friend class PipelineHandlerFactory;
>  };
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index a9241ac62979..03e4b9e66aa6 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -11,6 +11,7 @@
>  #include <sys/sysmacros.h>
>
>  #include <libcamera/base/log.h>
> +#include <libcamera/base/mutex.h>
>  #include <libcamera/base/utils.h>
>
>  #include <libcamera/camera.h>
> @@ -155,6 +156,8 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,
>   */
>  bool PipelineHandler::lock()
>  {
> +	MutexLocker locker(lock_);
> +
>  	/* Do not allow nested locking in the same libcamera instance. */
>  	if (lockOwner_)
>  		return false;
> @@ -183,6 +186,8 @@ bool PipelineHandler::lock()
>   */
>  void PipelineHandler::unlock()
>  {
> +	MutexLocker locker(lock_);
> +
>  	if (!lockOwner_)
>  		return;
>
> --
> Regards,
>
> Laurent Pinchart
>

Hi Jacopo,

On Mon, Dec 27, 2021 at 09:56:46AM +0100, Jacopo Mondi wrote:
> On Mon, Dec 27, 2021 at 01:12:55AM +0200, Laurent Pinchart wrote:
> > The PipelineHandler lock() and unlock() functions are documented as
> > thread-safe, but they're not. Fix them using a mutex.
> >
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>
> > ---
> > Changes since v1:
> >
> > - Add thread safety annotation
> > - Comment that lockOwner_ does not indicate ownership of lock_
> > ---
> >  include/libcamera/internal/pipeline_handler.h | 4 +++-
> >  src/libcamera/pipeline_handler.cpp            | 5 +++++
> >  2 files changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> > index a7331cedfb16..e5b8ffb4db3d 100644
> > --- a/include/libcamera/internal/pipeline_handler.h
> > +++ b/include/libcamera/internal/pipeline_handler.h
> > @@ -14,6 +14,7 @@
> >  #include <sys/types.h>
> >  #include <vector>
> >
> > +#include <libcamera/base/mutex.h>
> >  #include <libcamera/base/object.h>
> >
> >  #include <libcamera/controls.h>
> > @@ -88,7 +89,8 @@ private:
> >
> >  	const char *name_;
> >
> > -	bool lockOwner_;
> > +	Mutex lock_;
> > +	bool lockOwner_ LIBCAMERA_TSA_GUARDED_BY(lock_); /* *Not* ownership of lock_ */
> 
> Took me a while to find out what the comment means..

I'm sure it could be improved, but as this will be reworked soon, I
decided not to spend too much time on it.

> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
> 
> >  	friend class PipelineHandlerFactory;
> >  };
> > diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> > index a9241ac62979..03e4b9e66aa6 100644
> > --- a/src/libcamera/pipeline_handler.cpp
> > +++ b/src/libcamera/pipeline_handler.cpp
> > @@ -11,6 +11,7 @@
> >  #include <sys/sysmacros.h>
> >
> >  #include <libcamera/base/log.h>
> > +#include <libcamera/base/mutex.h>
> >  #include <libcamera/base/utils.h>
> >
> >  #include <libcamera/camera.h>
> > @@ -155,6 +156,8 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,
> >   */
> >  bool PipelineHandler::lock()
> >  {
> > +	MutexLocker locker(lock_);
> > +
> >  	/* Do not allow nested locking in the same libcamera instance. */
> >  	if (lockOwner_)
> >  		return false;
> > @@ -183,6 +186,8 @@ bool PipelineHandler::lock()
> >   */
> >  void PipelineHandler::unlock()
> >  {
> > +	MutexLocker locker(lock_);
> > +
> >  	if (!lockOwner_)
> >  		return;
> >

Hi,

Thank you for the patch

On 12/27/21 4:42 AM, Laurent Pinchart wrote:
> The PipelineHandler lock() and unlock() functions are documented as
> thread-safe, but they're not. Fix them using a mutex.
>
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Reviewed-by: Kieran Bingham <kieran.bingham@ideasonboard.com>


Reviewed-by: Umang Jain <umang.jain@ideasonboard.com>

> ---
> Changes since v1:
>
> - Add thread safety annotation
> - Comment that lockOwner_ does not indicate ownership of lock_
> ---
>   include/libcamera/internal/pipeline_handler.h | 4 +++-
>   src/libcamera/pipeline_handler.cpp            | 5 +++++
>   2 files changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/include/libcamera/internal/pipeline_handler.h b/include/libcamera/internal/pipeline_handler.h
> index a7331cedfb16..e5b8ffb4db3d 100644
> --- a/include/libcamera/internal/pipeline_handler.h
> +++ b/include/libcamera/internal/pipeline_handler.h
> @@ -14,6 +14,7 @@
>   #include <sys/types.h>
>   #include <vector>
>   
> +#include <libcamera/base/mutex.h>
>   #include <libcamera/base/object.h>
>   
>   #include <libcamera/controls.h>
> @@ -88,7 +89,8 @@ private:
>   
>   	const char *name_;
>   
> -	bool lockOwner_;
> +	Mutex lock_;
> +	bool lockOwner_ LIBCAMERA_TSA_GUARDED_BY(lock_); /* *Not* ownership of lock_ */
>   
>   	friend class PipelineHandlerFactory;
>   };
> diff --git a/src/libcamera/pipeline_handler.cpp b/src/libcamera/pipeline_handler.cpp
> index a9241ac62979..03e4b9e66aa6 100644
> --- a/src/libcamera/pipeline_handler.cpp
> +++ b/src/libcamera/pipeline_handler.cpp
> @@ -11,6 +11,7 @@
>   #include <sys/sysmacros.h>
>   
>   #include <libcamera/base/log.h>
> +#include <libcamera/base/mutex.h>
>   #include <libcamera/base/utils.h>
>   
>   #include <libcamera/camera.h>
> @@ -155,6 +156,8 @@ MediaDevice *PipelineHandler::acquireMediaDevice(DeviceEnumerator *enumerator,
>    */
>   bool PipelineHandler::lock()
>   {
> +	MutexLocker locker(lock_);
> +
>   	/* Do not allow nested locking in the same libcamera instance. */
>   	if (lockOwner_)
>   		return false;
> @@ -183,6 +186,8 @@ bool PipelineHandler::lock()
>    */
>   void PipelineHandler::unlock()
>   {
> +	MutexLocker locker(lock_);
> +
>   	if (!lockOwner_)
>   		return;
>