Simplify regulator locking by removing locking around locking.
rdev->ref check when unlocking is moved inside the critical section.

This patch depends on commit 12235da8c80a ("kernel/locking: Add context
to ww_mutex_trylock()").

Note: return -EALREADY is removed as no caller depends on it and in that
case the lock count is incremented anyway.

Reviewed-by: Douglas Anderson <dianders@chromium.org>
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/regulator/core.c | 23 ++++++-----------------
 1 file changed, 6 insertions(+), 17 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 921c7039baa3..f18e7cb88a0d 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);
@@ -141,25 +140,18 @@ static bool regulator_ops_is_valid(struct regulator_dev *rdev, int ops)
 static inline int regulator_lock_nested(struct regulator_dev *rdev,
 					struct ww_acquire_ctx *ww_ctx)
 {
-	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);
-		ret = ww_mutex_lock(&rdev->mutex, ww_ctx);
+	    READ_ONCE(rdev->mutex_owner) != current) {
+		int 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 +178,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 Wed, Aug 30, 2023 at 10:35 AM 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 commit 12235da8c80a ("kernel/locking: Add context
> to ww_mutex_trylock()").
>
> Note: return -EALREADY is removed as no caller depends on it and in that
> case the lock count is incremented anyway.
>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  drivers/regulator/core.c | 23 ++++++-----------------
>  1 file changed, 6 insertions(+), 17 deletions(-)

Note that I didn't actually provide a Reviewed-by on this patch in v1.
I was hoping for something in the commit message that explained why
commit 12235da8c80a ("kernel/locking: Add context to
ww_mutex_trylock()") meant that we didn't need the extra lock. You
responded to the v1, but didn't add anything to the commit message
about it.

Looking at your response to v1, I'm not sure it helps enlighten me on
why adding the context removed the need for the extra lock. Can you
add more words? Pretend I don't know anything about ww_mutex, which is
not far from the truth since every time I look at ww_mutex I have to
re-learn how it works. :-P Specifically, what would actually have been
broken without the extra lock but before the context was added?


-Doug

On Wed, Aug 30, 2023 at 01:54:21PM -0700, Doug Anderson wrote:
> On Wed, Aug 30, 2023 at 10:35 AM 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 commit 12235da8c80a ("kernel/locking: Add context
> > to ww_mutex_trylock()").
> >
> > Note: return -EALREADY is removed as no caller depends on it and in that
> > case the lock count is incremented anyway.
> >
> > Reviewed-by: Douglas Anderson <dianders@chromium.org>
> > Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> > ---
> >  drivers/regulator/core.c | 23 ++++++-----------------
> >  1 file changed, 6 insertions(+), 17 deletions(-)
> 
> Note that I didn't actually provide a Reviewed-by on this patch in v1.
> I was hoping for something in the commit message that explained why
> commit 12235da8c80a ("kernel/locking: Add context to
> ww_mutex_trylock()") meant that we didn't need the extra lock. You
> responded to the v1, but didn't add anything to the commit message
> about it.
> 
> Looking at your response to v1, I'm not sure it helps enlighten me on
> why adding the context removed the need for the extra lock. Can you
> add more words? Pretend I don't know anything about ww_mutex, which is
> not far from the truth since every time I look at ww_mutex I have to
> re-learn how it works. :-P Specifically, what would actually have been

Thanks for all your (and Stephen's) questions and comments! I had a bit
more of thinking and reading time about the W/W mutex and how it works.
It turns out I can remove some noise from this commit.

The commit 12235da8c80a dependency is due to text changes: the original
code would need a bit of reordering, all not much different than the
two previous patches.

If ww_mutex_lock() was able to return -EALREADY with NULL ww_ctx,
regulator_lock_nested() could be made even simpler.

Best Regards,
Michał Mirosław

Quoting Michał Mirosław (2023-08-30 10:35:31)
> Simplify regulator locking by removing locking around locking.

Maybe this should say "Simplify regulator_lock_nested() by removing the
`regulator_nesting_mutex` now that rdev is locked whenever rdev->ref_cnt or
rdev->owner are modified"?

> rdev->ref check when unlocking is moved inside the critical section.

rdev->ref_cnt?

>
> This patch depends on commit 12235da8c80a ("kernel/locking: Add context
> to ww_mutex_trylock()").
>
> Note: return -EALREADY is removed as no caller depends on it and in that
> case the lock count is incremented anyway.

Where is -EALREADY removed in this patch? Perhaps "removed" should be
"ignored"?

Note: A return value of -EALREADY from ww_mutex_lock() in
regulator_lock_nested() is ignored as no caller depends on it.

>
> Reviewed-by: Douglas Anderson <dianders@chromium.org>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>

On Wed, Aug 30, 2023 at 01:36:38PM -0700, Stephen Boyd wrote:
> Quoting Michał Mirosław (2023-08-30 10:35:31)
> > Simplify regulator locking by removing locking around locking.
> 
> Maybe this should say "Simplify regulator_lock_nested() by removing the
> `regulator_nesting_mutex` now that rdev is locked whenever rdev->ref_cnt or
> rdev->owner are modified"?

I'll rework the message. Thanks for the hints!

[...]
> > Note: return -EALREADY is removed as no caller depends on it and in that
> > case the lock count is incremented anyway.
> 
> Where is -EALREADY removed in this patch? Perhaps "removed" should be
> "ignored"?
> 
> Note: A return value of -EALREADY from ww_mutex_lock() in
> regulator_lock_nested() is ignored as no caller depends on it.

I can actually remove this altogether: ww_mutex_lock() won't ever
return -EALREADY as it's called only if (rdev->mutex_owner != current).

Best Regards.
Michał Mirosław