Re: [PATCH 3/6] regulator: core: simplify nested locking

[Michał Mirosław]

On Mon, Aug 21, 2023 at 09:50:32AM -0700, Doug Anderson wrote:
> Hi,
> 
> On Sat, Aug 19, 2023 at 3:46 PM Michał Mirosław <mirq-linux@xxxxxxxxxxxx> wrote:
> >
> > Simplify regulator locking by removing locking around locking.
> > rdev->ref check when unlocking is moved inside the critical section.
> >
> > This patch depends on 12235da8c80a ("kernel/locking: Add context to
> > ww_mutex_trylock()").
> 
> nit: when I run checkpatch, it always wants me to put the word
> "commit" before the git hash when I refer to a commit. ;-)

> > Signed-off-by: Michał Mirosław <mirq-linux@xxxxxxxxxxxx>
> > ---
> >  drivers/regulator/core.c | 19 +++++--------------
> >  1 file changed, 5 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> > index 921c7039baa3..87e54b776a0f 100644
> > --- a/drivers/regulator/core.c
> > +++ b/drivers/regulator/core.c
> > @@ -34,7 +34,6 @@
> >  #include "internal.h"
> >
> >  static DEFINE_WW_CLASS(regulator_ww_class);
> > -static DEFINE_MUTEX(regulator_nesting_mutex);
> >  static DEFINE_MUTEX(regulator_list_mutex);
> >  static LIST_HEAD(regulator_map_list);
> >  static LIST_HEAD(regulator_ena_gpio_list);
> > @@ -143,23 +142,18 @@ static inline int regulator_lock_nested(struct regulator_dev *rdev,
> >  {
> >         int ret = 0;
> 
> nit: remove initialization of "ret" to 0 since changing "return ret"
> to "return 0" below. Those changes belong in one of the previous
> patch, too.

Will do.

> > -       mutex_lock(&regulator_nesting_mutex);
> > -
> >         if (!ww_mutex_trylock(&rdev->mutex, ww_ctx) &&
> > -           rdev->mutex_owner != current) {
> > -               mutex_unlock(&regulator_nesting_mutex);
> > +           READ_ONCE(rdev->mutex_owner) != current) {
> >                 ret = ww_mutex_lock(&rdev->mutex, ww_ctx);
> > +
> >                 if (ret == -EDEADLK)
> >                         return ret;
> > -               mutex_lock(&regulator_nesting_mutex);
> >         }
> >
> >         rdev->ref_cnt++;
> >         rdev->mutex_owner = current;
> >
> > -       mutex_unlock(&regulator_nesting_mutex);
> > -
> > -       return ret;
> > +       return 0;
> >  }
> >
> >  /**
> > @@ -186,16 +180,13 @@ static void regulator_lock(struct regulator_dev *rdev)
> >   */
> >  static void regulator_unlock(struct regulator_dev *rdev)
> >  {
> > -       mutex_lock(&regulator_nesting_mutex);
> > +       if (WARN_ON_ONCE(rdev->ref_cnt <= 0))
> > +               return;
> >
> >         if (--rdev->ref_cnt == 0) {
> >                 rdev->mutex_owner = NULL;
> >                 ww_mutex_unlock(&rdev->mutex);
> >         }
> > -
> > -       WARN_ON_ONCE(rdev->ref_cnt < 0);
> > -
> > -       mutex_unlock(&regulator_nesting_mutex);
> 
> I guess the "fix" you talked about in the cover letter is moving the
> WARN_ON up? That could be done in patch #1 and marked as a "Fix",
> right?

The fix is patch #5. (And has a Fixes line to mark it. :-)

Moving WARN_ON up is needed to keep it from touching ref_cnt outside of
the locked code (the regulator_nesting_mutex was protecting it before).

> I'm not 100% sure why we needed the "regulator_nesting_mutex" to begin
> with. I'm also not 100% sure why we depend on commit 12235da8c80a
> ("kernel/locking: Add context to ww_mutex_trylock()"). Could you add
> something to the commit message to make this more obvious so I don't
> need to work so hard to figure it out? ;-)

Commit 12235da8c80a is needed as it enables ww_mutex_trylock() to
correcly account the lock in ww_ctx if it was able to grab it.

Best Regards
Michał Mirosław