[libcamera-devel] [PATCH 02/10] ipa: add missing thread-safety annotations

[Jacopo Mondi]

Hi Nicholas

On Thu, Oct 27, 2022 at 05:04:31PM -0500, Nicholas Roth wrote:
> > How come this gets inserted here by git when the commit author and the
> sender have the same identity ?
>
> I'm really not sure. Maybe because I specify "--from" in git-send-email? Is
> this not a standard thing to do?
> git send-email --compose --from=nicholas at rothemail.net --reply-to=
> libcamera-devel at lists.libcamera.org --to=libcamera-devel at lists.libcamera.org
> --subject="libcamera Android Enhancements" --thread --no-chain-reply-to
> --annotate -v2 main

Oh, so many options :)

I'm not sure if --from is the culprit, it might be,  but be aware the
sender can be picked up automatically by git global config variables
and you most probably don't need this option.

I would discourage --reply-to, it mangles the "Reply-To" field of your
patches, something that might even break someone's scripts. Just
spcify the mailing list in the receiver or cc list and people will
reply to it.

--thread should be on by default if I get it right, same for
--no-chain-reply-to

Try to drop the --from and --reply-to options and see how it goes


>
> > nit: double empty line
> If you're referring to the commit message, the line below the error is for
> the carat that Clang uses to indicate the position of the error. Happy to
> not skip a line before "Signed-off-by" if it would help.

Ah sorry, missed it

>
> > this seems correct to me, even more so if it silences a compilation error.
> Do I need an LGTM from anyone else? What would the next step be here?

The policy is usually to get at least two reviewed-by tags. Once the
series is fully reviewed it will be picked up, run through a few
compiler matrices and if it touches the Android HAL through CTS, then
merged.

The process is still a bit fuzzy, we don't have per-component
maintainers nor someone designed specifically for the
'collect-test-merge' part, but it's usually Laurent and Kieran taking
care of that.

>
> On Thu, Oct 27, 2022 at 11:08 AM Jacopo Mondi <jacopo at jmondi.org> wrote:
>
> > Hi Nicholas
> >
> > On Thu, Oct 27, 2022 at 12:55:07AM -0500, Nicholas Roth via
> > libcamera-devel wrote:
> > > From: Nicholas Roth <nicholas at rothemail.net>
> >
> > How come this gets inserted here by git when the commit author and the
> > sender have the same identity ?
> >
> > >
> > > The raspberrypi IPA is missing thread-safety annotations, which breaks
> > > the build.
> > >
> > > Add required thread-safety annotations.
> > >
> > > ../src/ipa/raspberrypi/controller/metadata.h:108:31: error: mutex
> > >   'mutex_' is still held at the end of function
> > [-Werror,-Wthread-safety-analysis]
> > >         void lock() { mutex_.lock(); }
> > >                                      ^
> > > ../src/ipa/raspberrypi/controller/metadata.h:108:23: note: mutex
> > >   acquired here
> > >         void lock() { mutex_.lock(); }
> > >                              ^
> > > ../src/ipa/raspberrypi/controller/metadata.h:109:25: error: releasing
> > >   mutex 'mutex_' that was not held [-Werror,-Wthread-safety-analysis]
> > >         void unlock() { mutex_.unlock(); }
> > >                                ^
> >
> > nit: double empty line
> >
> > >
> > > Signed-off-by: Nicholas Roth <nicholas at rothemail.net>
> >
> > I've not followed much the thread annotation thing, but this seems
> > correct to me, even more so if it silences a compilation error.
> >
> > Reviewed-by: Jacopo Mondi <jacopo at jmondi.org>
> >
> > > ---
> > >  src/ipa/raspberrypi/controller/metadata.h | 8 +++++---
> > >  1 file changed, 5 insertions(+), 3 deletions(-)
> > >
> > > diff --git a/src/ipa/raspberrypi/controller/metadata.h
> > b/src/ipa/raspberrypi/controller/metadata.h
> > > index 0f7ebfaf..870b6e26 100644
> > > --- a/src/ipa/raspberrypi/controller/metadata.h
> > > +++ b/src/ipa/raspberrypi/controller/metadata.h
> > > @@ -13,9 +13,11 @@
> > >  #include <mutex>
> > >  #include <string>
> > >
> > > +#include <libcamera/base/thread_annotations.h>
> > > +
> > >  namespace RPiController {
> > >
> > > -class Metadata
> > > +class LIBCAMERA_TSA_CAPABILITY("mutex") Metadata
> > >  {
> > >  public:
> > >       Metadata() = default;
> > > @@ -103,8 +105,8 @@ public:
> > >        * locks with the standard lock classes.
> > >        * e.g. std::lock_guard<RPiController::Metadata> lock(metadata)
> > >        */
> > > -     void lock() { mutex_.lock(); }
> > > -     void unlock() { mutex_.unlock(); }
> > > +     void lock() LIBCAMERA_TSA_ACQUIRE() { mutex_.lock(); }
> > > +     void unlock() LIBCAMERA_TSA_RELEASE() { mutex_.unlock(); }
> > >
> > >  private:
> > >       mutable std::mutex mutex_;
> > > --
> > > 2.34.1
> > >
> >