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()").

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 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;
 
-	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);
 }
 
 /**
-- 
2.39.2

Hi,

On Sat, Aug 19, 2023 at 3:46 PM Michał Mirosław <mirq-linux@rere.qmqm.pl> 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@rere.qmqm.pl>
> ---
>  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.


> -       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?

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? ;-)

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@rere.qmqm.pl> 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@rere.qmqm.pl>
> > ---
> >  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