[PATCH v2 1/7] regulator/core: regulator_lock_nested: remove `lock`

[PATCH v2 1/7] regulator/core: regulator_lock_nested: remove `lock`

[Michał Mirosław]

`lock` is only false when the `rdev` is already locked and the owner is
`current`. In this case `ret` is always zero. By removing `lock`, we
thus remove `mutex_owner` write avoidance, but make the code flow more
understandable.

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

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index d8e1caaf207e..215b721e5cd4 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -141,27 +141,19 @@ 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)
 {
-	bool lock = false;
 	int ret = 0;
 
 	mutex_lock(&regulator_nesting_mutex);
 
 	if (!ww_mutex_trylock(&rdev->mutex, ww_ctx)) {
-		if (rdev->mutex_owner == current)
-			rdev->ref_cnt++;
-		else
-			lock = true;
-
-		if (lock) {
+		if (rdev->mutex_owner != current) {
 			mutex_unlock(&regulator_nesting_mutex);
 			ret = ww_mutex_lock(&rdev->mutex, ww_ctx);
 			mutex_lock(&regulator_nesting_mutex);
 		}
-	} else {
-		lock = true;
 	}
 
-	if (lock && ret != -EDEADLK) {
+	if (ret != -EDEADLK) {
 		rdev->ref_cnt++;
 		rdev->mutex_owner = current;
 	}
-- 
2.39.2

[PATCH v2 0/7] regulator: core: locking cleanups, part 1/inf

[Michał Mirosław]

This series aims to reduce regulator locking code complexity. There is
a consmetic fix (patch #6) included (applies to unexpected/unlikely
errors only). v2 adds a regulator_lock_contended() function to make the
ww_mutex locking sequence easier to understand.

Michał Mirosław (7):
  regulator/core: regulator_lock_nested: remove `lock`
  regulator/core: regulator_lock_nested: skip extra lock for -EDEADLK
  regulator/core: regulator_lock_nested: simplify nested locking
  regulator/core: regulator_resolve_supply: remove gotos
  regulator/core: regulator_lock_contended: wrap ww_mutex lock sequence
    restart
  regulator/core: regulator_lock_two: propagate error up
  regulator/core: regulator_lock_two: remove duplicate locking code

 drivers/regulator/core.c | 133 +++++++++++++++++----------------------
 1 file changed, 59 insertions(+), 74 deletions(-)

-- 
2.39.2

[PATCH v2 2/7] regulator/core: regulator_lock_nested: skip extra lock for -EDEADLK

[Michał Mirosław]

When ww_mutex_lock() returns -EDEADLK the nesting mutex-protected
section becomes a no-op. Return early and avoid the extra lock.

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

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 215b721e5cd4..921c7039baa3 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -145,18 +145,17 @@ static inline int regulator_lock_nested(struct regulator_dev *rdev,
 
 	mutex_lock(&regulator_nesting_mutex);
 
-	if (!ww_mutex_trylock(&rdev->mutex, ww_ctx)) {
-		if (rdev->mutex_owner != current) {
-			mutex_unlock(&regulator_nesting_mutex);
-			ret = ww_mutex_lock(&rdev->mutex, ww_ctx);
-			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);
+		if (ret == -EDEADLK)
+			return ret;
+		mutex_lock(&regulator_nesting_mutex);
 	}
 
-	if (ret != -EDEADLK) {
-		rdev->ref_cnt++;
-		rdev->mutex_owner = current;
-	}
+	rdev->ref_cnt++;
+	rdev->mutex_owner = current;
 
 	mutex_unlock(&regulator_nesting_mutex);
 
-- 
2.39.2

[PATCH v2 3/7] regulator/core: regulator_lock_nested: simplify nested locking

[Michał Mirosław]

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

[PATCH v2 5/7] regulator/core: regulator_lock_contended: wrap ww_mutex lock sequence restart

[Michał Mirosław]

Wrap locking a regulator after a failed ww_mutex locking sequence with a
new function.  This is to deduplicate occurrences of the pattern and make
it self-documenting.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/regulator/core.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index e89c12d27a9d..7201927c5d5b 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -154,6 +154,22 @@ static inline int regulator_lock_nested(struct regulator_dev *rdev,
 	return 0;
 }
 
+/**
+ * regulator_lock_contended - retry locking a regulator
+ * @rdev:		regulator source
+ * @ww_ctx:		w/w mutex acquire context
+ *
+ * Locks a regulator after a failed locking sequence (aborted
+ * with -EDEADLK).
+ */
+static inline void regulator_lock_contended(struct regulator_dev *rdev,
+					    struct ww_acquire_ctx *ww_ctx)
+{
+	ww_mutex_lock_slow(&rdev->mutex, ww_ctx);
+	rdev->ref_cnt++;
+	rdev->mutex_owner = current;
+}
+
 /**
  * regulator_lock - lock a single regulator
  * @rdev:		regulator source
@@ -218,9 +234,7 @@ static void regulator_lock_two(struct regulator_dev *rdev1,
 	while (true) {
 		regulator_unlock(held);
 
-		ww_mutex_lock_slow(&contended->mutex, ww_ctx);
-		contended->ref_cnt++;
-		contended->mutex_owner = current;
+		regulator_lock_contended(contended, ww_ctx);
 		swap(held, contended);
 		ret = regulator_lock_nested(contended, ww_ctx);
 
@@ -376,10 +390,8 @@ static void regulator_lock_dependent(struct regulator_dev *rdev,
 
 	do {
 		if (new_contended_rdev) {
-			ww_mutex_lock_slow(&new_contended_rdev->mutex, ww_ctx);
+			regulator_lock_contended(new_contended_rdev, ww_ctx);
 			old_contended_rdev = new_contended_rdev;
-			old_contended_rdev->ref_cnt++;
-			old_contended_rdev->mutex_owner = current;
 		}
 
 		err = regulator_lock_recursive(rdev,
@@ -6085,10 +6097,8 @@ static void regulator_summary_lock(struct ww_acquire_ctx *ww_ctx)
 
 	do {
 		if (new_contended_rdev) {
-			ww_mutex_lock_slow(&new_contended_rdev->mutex, ww_ctx);
+			regulator_lock_contended(new_contended_rdev, ww_ctx);
 			old_contended_rdev = new_contended_rdev;
-			old_contended_rdev->ref_cnt++;
-			old_contended_rdev->mutex_owner = current;
 		}
 
 		err = regulator_summary_lock_all(ww_ctx,
-- 
2.39.2

[PATCH v2 4/7] regulator/core: regulator_resolve_supply: remove gotos

[Michał Mirosław]

Since 14a71d509ac8 ("Fix lockdep warning resolving supplies") the `out`
label is just `return ret;`. Inline it for easier reading.

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

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index f18e7cb88a0d..e89c12d27a9d 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -2039,7 +2039,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	struct regulator_dev *r;
 	struct device *dev = rdev->dev.parent;
 	struct ww_acquire_ctx ww_ctx;
-	int ret = 0;
+	int ret;
 
 	/* No supply to resolve? */
 	if (!rdev->supply_name)
@@ -2055,7 +2055,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 
 		/* Did the lookup explicitly defer for us? */
 		if (ret == -EPROBE_DEFER)
-			goto out;
+			return ret;
 
 		if (have_full_constraints()) {
 			r = dummy_regulator_rdev;
@@ -2063,18 +2063,15 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 		} else {
 			dev_err(dev, "Failed to resolve %s-supply for %s\n",
 				rdev->supply_name, rdev->desc->name);
-			ret = -EPROBE_DEFER;
-			goto out;
+			return -EPROBE_DEFER;
 		}
 	}
 
 	if (r == rdev) {
 		dev_err(dev, "Supply for %s (%s) resolved to itself\n",
 			rdev->desc->name, rdev->supply_name);
-		if (!have_full_constraints()) {
-			ret = -EINVAL;
-			goto out;
-		}
+		if (!have_full_constraints())
+			return -EINVAL;
 		r = dummy_regulator_rdev;
 		get_device(&r->dev);
 	}
@@ -2088,8 +2085,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	if (r->dev.parent && r->dev.parent != rdev->dev.parent) {
 		if (!device_is_bound(r->dev.parent)) {
 			put_device(&r->dev);
-			ret = -EPROBE_DEFER;
-			goto out;
+			return -EPROBE_DEFER;
 		}
 	}
 
@@ -2097,7 +2093,7 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	ret = regulator_resolve_supply(r);
 	if (ret < 0) {
 		put_device(&r->dev);
-		goto out;
+		return ret;
 	}
 
 	/*
@@ -2111,14 +2107,14 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	if (rdev->supply) {
 		regulator_unlock_two(rdev, r, &ww_ctx);
 		put_device(&r->dev);
-		goto out;
+		return 0;
 	}
 
 	ret = set_supply(rdev, r);
 	if (ret < 0) {
 		regulator_unlock_two(rdev, r, &ww_ctx);
 		put_device(&r->dev);
-		goto out;
+		return ret;
 	}
 
 	regulator_unlock_two(rdev, r, &ww_ctx);
@@ -2133,12 +2129,11 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 		if (ret < 0) {
 			_regulator_put(rdev->supply);
 			rdev->supply = NULL;
-			goto out;
+			return ret;
 		}
 	}
 
-out:
-	return ret;
+	return 0;
 }
 
 /* Internal regulator request function */
-- 
2.39.2

[PATCH v2 6/7] regulator/core: regulator_lock_two: propagate error up

[Michał Mirosław]

Fix up error paths from regulator_lock_two(): although it should not
fail, returning with half-locked state after issuing a WARN() asks
for even more trouble.

Fixes: cba6cfdc7c3f ("regulator: core: Avoid lockdep reports when resolving supplies")
Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
v2:
  - updated kerneldoc
  - call ww_acquire_done() on all exits
---
 drivers/regulator/core.c | 28 +++++++++++++++++++---------
 1 file changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 7201927c5d5b..3f9621621da9 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -209,11 +209,12 @@ static void regulator_unlock(struct regulator_dev *rdev)
  * @rdev2:		second regulator
  * @ww_ctx:		w/w mutex acquire context
  *
- * Locks both rdevs using the regulator_ww_class.
+ * Locks both rdevs using the regulator_ww_class. Returns error if an
+ * unexpected error has been detected during a locking sequence.
  */
-static void regulator_lock_two(struct regulator_dev *rdev1,
-			       struct regulator_dev *rdev2,
-			       struct ww_acquire_ctx *ww_ctx)
+static int regulator_lock_two(struct regulator_dev *rdev1,
+			      struct regulator_dev *rdev2,
+			      struct ww_acquire_ctx *ww_ctx)
 {
 	struct regulator_dev *held, *contended;
 	int ret;
@@ -222,10 +223,13 @@ static void regulator_lock_two(struct regulator_dev *rdev1,
 
 	/* Try to just grab both of them */
 	ret = regulator_lock_nested(rdev1, ww_ctx);
-	WARN_ON(ret);
+	if (WARN_ON(ret))
+		goto exit;
 	ret = regulator_lock_nested(rdev2, ww_ctx);
-	if (ret != -EDEADLOCK) {
-		WARN_ON(ret);
+	if (!ret)
+		goto exit;
+	if (WARN_ON(ret != -EDEADLOCK)) {
+		regulator_unlock(rdev1);
 		goto exit;
 	}
 
@@ -239,13 +243,15 @@ static void regulator_lock_two(struct regulator_dev *rdev1,
 		ret = regulator_lock_nested(contended, ww_ctx);
 
 		if (ret != -EDEADLOCK) {
-			WARN_ON(ret);
+			if (WARN_ON(ret))
+				regulator_unlock(held);
 			break;
 		}
 	}
 
 exit:
 	ww_acquire_done(ww_ctx);
+	return ret;
 }
 
 /**
@@ -2113,7 +2119,11 @@ static int regulator_resolve_supply(struct regulator_dev *rdev)
 	 * between rdev->supply null check and setting rdev->supply in
 	 * set_supply() from concurrent tasks.
 	 */
-	regulator_lock_two(rdev, r, &ww_ctx);
+	ret = regulator_lock_two(rdev, r, &ww_ctx);
+	if (ret < 0) {
+		put_device(&r->dev);
+		return ret;
+	}
 
 	/* Supply just resolved by a concurrent task? */
 	if (rdev->supply) {
-- 
2.39.2

[PATCH v2 7/7] regulator/core: regulator_lock_two: remove duplicate locking code

[Michał Mirosław]

Make regulator_lock_two() shorter by observing that we have only two
locks and when swapped earlier the retry code becomes identical to the
normal (optimistic) path.

Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
---
 drivers/regulator/core.c | 30 +++++++++-------------------
 1 file changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index 3f9621621da9..36cec3fbcdea 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -216,37 +216,29 @@ static int regulator_lock_two(struct regulator_dev *rdev1,
 			      struct regulator_dev *rdev2,
 			      struct ww_acquire_ctx *ww_ctx)
 {
-	struct regulator_dev *held, *contended;
 	int ret;
 
 	ww_acquire_init(ww_ctx, &regulator_ww_class);
 
-	/* Try to just grab both of them */
 	ret = regulator_lock_nested(rdev1, ww_ctx);
 	if (WARN_ON(ret))
 		goto exit;
-	ret = regulator_lock_nested(rdev2, ww_ctx);
-	if (!ret)
-		goto exit;
-	if (WARN_ON(ret != -EDEADLOCK)) {
-		regulator_unlock(rdev1);
-		goto exit;
-	}
 
-	held = rdev1;
-	contended = rdev2;
 	while (true) {
-		regulator_unlock(held);
+		ret = regulator_lock_nested(rdev2, ww_ctx);
+		if (!ret)
+			break;
 
-		regulator_lock_contended(contended, ww_ctx);
-		swap(held, contended);
-		ret = regulator_lock_nested(contended, ww_ctx);
+		regulator_unlock(rdev1);
 
-		if (ret != -EDEADLOCK) {
-			if (WARN_ON(ret))
-				regulator_unlock(held);
+		if (WARN_ON(ret != -EDEADLOCK))
 			break;
-		}
+
+		/* Swap the locking order and retry. */
+
+		swap(rdev1, rdev2);
+
+		regulator_lock_contended(rdev1, ww_ctx);
 	}
 
 exit:
-- 
2.39.2

Re: [PATCH v2 5/7] regulator/core: regulator_lock_contended: wrap ww_mutex lock sequence restart

[Doug Anderson]

Hi,

On Wed, Aug 30, 2023 at 10:35 AM Michał Mirosław
<mirq-linux@rere.qmqm.pl> wrote:
>
> Wrap locking a regulator after a failed ww_mutex locking sequence with a
> new function.  This is to deduplicate occurrences of the pattern and make
> it self-documenting.
>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  drivers/regulator/core.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index e89c12d27a9d..7201927c5d5b 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -154,6 +154,22 @@ static inline int regulator_lock_nested(struct regulator_dev *rdev,
>         return 0;
>  }
>
> +/**
> + * regulator_lock_contended - retry locking a regulator
> + * @rdev:              regulator source
> + * @ww_ctx:            w/w mutex acquire context
> + *
> + * Locks a regulator after a failed locking sequence (aborted
> + * with -EDEADLK).
> + */
> +static inline void regulator_lock_contended(struct regulator_dev *rdev,
> +                                           struct ww_acquire_ctx *ww_ctx)

nit: IMO "inline" should be reserved for places where it would be a
serious problem if the function wasn't inlined. For cases like this,
let the compiler do its job and decide whether we'll be better off
with the code inlined or not.

In any case:

Reviewed-by: Douglas Anderson <dianders@chromium.org>

Re: [PATCH v2 6/7] regulator/core: regulator_lock_two: propagate error up

[Doug Anderson]

Hi,

On Wed, Aug 30, 2023 at 10:35 AM Michał Mirosław
<mirq-linux@rere.qmqm.pl> wrote:
>
> Fix up error paths from regulator_lock_two(): although it should not
> fail, returning with half-locked state after issuing a WARN() asks
> for even more trouble.
>
> Fixes: cba6cfdc7c3f ("regulator: core: Avoid lockdep reports when resolving supplies")
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
> v2:
>   - updated kerneldoc
>   - call ww_acquire_done() on all exits
> ---
>  drivers/regulator/core.c | 28 +++++++++++++++++++---------
>  1 file changed, 19 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index 7201927c5d5b..3f9621621da9 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -209,11 +209,12 @@ static void regulator_unlock(struct regulator_dev *rdev)
>   * @rdev2:             second regulator
>   * @ww_ctx:            w/w mutex acquire context
>   *
> - * Locks both rdevs using the regulator_ww_class.
> + * Locks both rdevs using the regulator_ww_class. Returns error if an
> + * unexpected error has been detected during a locking sequence.

I don't believe this is the correct way to document return values in
kernel-doc. See:

Documentation/doc-guide/kernel-doc.rst

Specifically if you run:

scripts/kernel-doc -v drivers/regulator/core.c

You can see that the description of the return doesn't show up in the
proper place.

With that fixed, feel free to add my Reviewed-by tag.

-Doug

Re: [PATCH v2 7/7] regulator/core: regulator_lock_two: remove duplicate locking code

[Doug Anderson]

Hi,

On Wed, Aug 30, 2023 at 10:35 AM Michał Mirosław
<mirq-linux@rere.qmqm.pl> wrote:
>
> Make regulator_lock_two() shorter by observing that we have only two
> locks and when swapped earlier the retry code becomes identical to the
> normal (optimistic) path.
>
> Signed-off-by: Michał Mirosław <mirq-linux@rere.qmqm.pl>
> ---
>  drivers/regulator/core.c | 30 +++++++++-------------------
>  1 file changed, 11 insertions(+), 19 deletions(-)

FWIW, I'm leaving this one to Stephen to review since he had strong
opinions on some of the style issues here.

-Doug

Re: [PATCH v2 3/7] regulator/core: regulator_lock_nested: simplify nested locking

[Stephen Boyd]

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>

Re: [PATCH v2 3/7] regulator/core: regulator_lock_nested: simplify nested locking

[Doug Anderson]

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

Re: [PATCH v2 3/7] regulator/core: regulator_lock_nested: simplify nested locking

[Michał Mirosław]

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

Re: [PATCH v2 3/7] regulator/core: regulator_lock_nested: simplify nested locking

[Michał Mirosław]

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