Re: [PATCH 5/6] regulator: core: propagate error out ouf regulator_lock_two()

[Doug Anderson]

Hi,

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