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>
---
 drivers/regulator/core.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
index de434d550937..d8c2277cea36 100644
--- a/drivers/regulator/core.c
+++ b/drivers/regulator/core.c
@@ -197,9 +197,9 @@ static void regulator_unlock(struct regulator_dev *rdev)
  *
  * Locks both rdevs using the regulator_ww_class.
  */
-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;
@@ -208,10 +208,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)
+		return 0;
+	if (WARN_ON(ret != -EDEADLOCK)) {
+		regulator_unlock(rdev1);
 		goto exit;
 	}
 
@@ -225,15 +228,15 @@ static void regulator_lock_two(struct regulator_dev *rdev1,
 		contended->mutex_owner = current;
 		swap(held, contended);
 		ret = regulator_lock_nested(contended, ww_ctx);
-
-		if (ret != -EDEADLOCK) {
-			WARN_ON(ret);
+		if (!ret)
+			return 0;
+		if (WARN_ON(ret != -EDEADLOCK))
 			break;
-		}
 	}
 
 exit:
 	ww_acquire_done(ww_ctx);
+	return ret;
 }
 
 /**
@@ -2101,7 +2104,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

Hi,

On Sat, Aug 19, 2023 at 3:46 PM 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>
> ---
>  drivers/regulator/core.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/regulator/core.c b/drivers/regulator/core.c
> index de434d550937..d8c2277cea36 100644
> --- a/drivers/regulator/core.c
> +++ b/drivers/regulator/core.c
> @@ -197,9 +197,9 @@ static void regulator_unlock(struct regulator_dev *rdev)
>   *
>   * Locks both rdevs using the regulator_ww_class.
>   */
> -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)

Please update the kerneldoc to talk about the return value.


>  {
>         struct regulator_dev *held, *contended;
>         int ret;
> @@ -208,10 +208,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)
> +               return 0;

Aren't you missing the "ww_acquire_done()" in this case? I know it's
optional, but nice to call it anyway...


> +       if (WARN_ON(ret != -EDEADLOCK)) {
> +               regulator_unlock(rdev1);
>                 goto exit;
>         }
>
> @@ -225,15 +228,15 @@ static void regulator_lock_two(struct regulator_dev *rdev1,
>                 contended->mutex_owner = current;
>                 swap(held, contended);
>                 ret = regulator_lock_nested(contended, ww_ctx);
> -
> -               if (ret != -EDEADLOCK) {
> -                       WARN_ON(ret);
> +               if (!ret)
> +                       return 0;

Aren't you missing the "ww_acquire_done()" in this case?


Overall, I guess I'm OK with this. I agree that we'd be in pretty bad
shape if we exited the routine and left one of the mutexes acquired,
which the old code could have done. However, at the same time these
error cases really shouldn't happen anyway. In that sense, I guess
they really should be BUG_ON(), but IIRC that's discouraged. Thus, I
think your patch is the right direction.


-Doug