[libcamera-devel,v3,2/4] libcamera: ipa: raspberrypi: ALSC: Improve locking in a few places

9126 diff mbox series

Fix up a few locations where we call notify_one() with the lock
held. In particular, restartAsync does not need to be called with the
lock held for its entire duration.

Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 src/ipa/raspberrypi/controller/rpi/alsc.cpp | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

Hi David,

Thank you for the patch.

On Sat, 1 Aug 2020 at 09:02, David Plowman
<david.plowman@raspberrypi.com> wrote:
>
> Fix up a few locations where we call notify_one() with the lock
> held. In particular, restartAsync does not need to be called with the
> lock held for its entire duration.
>
> Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Reviewed-by: Naushir Patuck <naush@raspberrypi.com>


> ---
>  src/ipa/raspberrypi/controller/rpi/alsc.cpp | 10 ++++++----
>  1 file changed, 6 insertions(+), 4 deletions(-)
>
> diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> index 76e2f04..12359dc 100644
> --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> @@ -32,8 +32,8 @@ Alsc::~Alsc()
>         {
>                 std::lock_guard<std::mutex> lock(mutex_);
>                 async_abort_ = true;
> -               async_signal_.notify_one();
>         }
> +       async_signal_.notify_one();
>         async_thread_.join();
>  }
>
> @@ -268,8 +268,11 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)
>         frame_phase_ = 0;
>         // copy the camera mode so it won't change during the calculations
>         async_camera_mode_ = camera_mode_;
> -       async_start_ = true;
>         async_started_ = true;
> +       {
> +               std::lock_guard<std::mutex> lock(mutex_);
> +               async_start_ = true;
> +       }
>         async_signal_.notify_one();
>  }
>
> @@ -315,7 +318,6 @@ void Alsc::Process(StatisticsPtr &stats, Metadata *image_metadata)
>         RPI_LOG("Alsc: frame_phase " << frame_phase_);
>         if (frame_phase_ >= (int)config_.frame_period ||
>             frame_count2_ < (int)config_.startup_frames) {
> -               std::unique_lock<std::mutex> lock(mutex_);
>                 if (async_started_ == false) {
>                         RPI_LOG("ALSC thread starting");
>                         restartAsync(stats, image_metadata);
> @@ -339,8 +341,8 @@ void Alsc::asyncFunc()
>                 {
>                         std::lock_guard<std::mutex> lock(mutex_);
>                         async_finished_ = true;
> -                       sync_signal_.notify_one();
>                 }
> +               sync_signal_.notify_one();
>         }
>  }
>
> --
> 2.20.1
>
> _______________________________________________
> libcamera-devel mailing list
> libcamera-devel@lists.libcamera.org
> https://lists.libcamera.org/listinfo/libcamera-devel

Hi David,

I believe this patch has introduced the following coverity warning. If
you supply a patch to fix this issue, please add the following tag:

Reported-by: Coverity CID=298183

Alternatively, if you believe it's a false positive, let me know and
I'll close it.

** CID 298183:  Concurrent data access violations  (MISSING_LOCK)
/home/linuxembedded/iob/libcamera/libcamera-daily/src/ipa/raspberrypi/controller/rpi/alsc.cpp:
369 in RPi::Alsc::asyncFunc()()

Hi Kieran

Yes, I agree, it looks like a false positive to me. According to my
(limited!) understanding of C++, the lock is taken straight back as we
come out of the wait, so async_start_ is written to with the mutex
held.

Thanks!
David

On Thu, 6 Aug 2020 at 11:06, Kieran Bingham
<kieran.bingham@ideasonboard.com> wrote:
>
> Hi David,
>
> I believe this patch has introduced the following coverity warning. If
> you supply a patch to fix this issue, please add the following tag:
>
> Reported-by: Coverity CID=298183
>
> Alternatively, if you believe it's a false positive, let me know and
> I'll close it.
>
> ** CID 298183:  Concurrent data access violations  (MISSING_LOCK)
> /home/linuxembedded/iob/libcamera/libcamera-daily/src/ipa/raspberrypi/controller/rpi/alsc.cpp:
> 369 in RPi::Alsc::asyncFunc()()
>
>
> ________________________________________________________________________________________________________
> *** CID 298183:  Concurrent data access violations  (MISSING_LOCK)
> /home/linuxembedded/iob/libcamera/libcamera-daily/src/ipa/raspberrypi/controller/rpi/alsc.cpp:
> 369 in RPi::Alsc::asyncFunc()()
> 363             while (true) {
> 364                     {
> 365                             std::unique_lock<std::mutex> lock(mutex_);
> 366                             async_signal_.wait(lock, [&] {
> 367                                     return async_start_ || async_abort_;
> 368                             });
> >>>     CID 298183:  Concurrent data access violations  (MISSING_LOCK)
> >>>     Accessing "this->async_start_" without holding lock
> "RPi::Alsc.mutex_". Elsewhere, "_ZN3RPi4AlscE.async_start_" is accessed
> with "RPi::Alsc.mutex_" held 1 out of 2 times (1 of these accesses
> strongly imply that it is necessary).
> 369                             async_start_ = false;
> 370                             if (async_abort_)
> 371                                     break;
> 372                     }
> 373                     doAlsc();
> 374                     {
>
>
>
> Briefly looking, I think that the call to async_signal_.wait(lock...)
> means that the lock is held for the remainder of the scope, so in fact
> this is likely a false positive. Is that interpretation accurate?
>
>
> --
> Regards
>
> Kieran
>
>
> On 01/08/2020 09:01, David Plowman wrote:
> > Fix up a few locations where we call notify_one() with the lock
> > held. In particular, restartAsync does not need to be called with the
> > lock held for its entire duration.
> >
> > Signed-off-by: David Plowman <david.plowman@raspberrypi.com>
> > Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> >  src/ipa/raspberrypi/controller/rpi/alsc.cpp | 10 ++++++----
> >  1 file changed, 6 insertions(+), 4 deletions(-)
> >
> > diff --git a/src/ipa/raspberrypi/controller/rpi/alsc.cpp b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> > index 76e2f04..12359dc 100644
> > --- a/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> > +++ b/src/ipa/raspberrypi/controller/rpi/alsc.cpp
> > @@ -32,8 +32,8 @@ Alsc::~Alsc()
> >       {
> >               std::lock_guard<std::mutex> lock(mutex_);
> >               async_abort_ = true;
> > -             async_signal_.notify_one();
> >       }
> > +     async_signal_.notify_one();
> >       async_thread_.join();
> >  }
> >
> > @@ -268,8 +268,11 @@ void Alsc::restartAsync(StatisticsPtr &stats, Metadata *image_metadata)
> >       frame_phase_ = 0;
> >       // copy the camera mode so it won't change during the calculations
> >       async_camera_mode_ = camera_mode_;
> > -     async_start_ = true;
> >       async_started_ = true;
> > +     {
> > +             std::lock_guard<std::mutex> lock(mutex_);
> > +             async_start_ = true;
> > +     }
> >       async_signal_.notify_one();
> >  }
> >
> > @@ -315,7 +318,6 @@ void Alsc::Process(StatisticsPtr &stats, Metadata *image_metadata)
> >       RPI_LOG("Alsc: frame_phase " << frame_phase_);
> >       if (frame_phase_ >= (int)config_.frame_period ||
> >           frame_count2_ < (int)config_.startup_frames) {
> > -             std::unique_lock<std::mutex> lock(mutex_);
> >               if (async_started_ == false) {
> >                       RPI_LOG("ALSC thread starting");
> >                       restartAsync(stats, image_metadata);
> > @@ -339,8 +341,8 @@ void Alsc::asyncFunc()
> >               {
> >                       std::lock_guard<std::mutex> lock(mutex_);
> >                       async_finished_ = true;
> > -                     sync_signal_.notify_one();
> >               }
> > +             sync_signal_.notify_one();
> >       }
> >  }
> >
> >
>
> --
> Regards
> --
> Kieran