linux-kernel - Re: [PATCH 3/6] regulator: core: simplify nested locking

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAD=FV=VHvi-2GPxXsDaiPKzgJHqBpX1a6+0CUwp1y8vfr8-Xtg@mail.gmail.com>
Date:   Mon, 21 Aug 2023 09:50:32 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     Michał Mirosław <mirq-linux@...e.qmqm.pl>
Cc:     Liam Girdwood <lgirdwood@...il.com>,
        Mark Brown <broonie@...nel.org>,
        Dmitry Osipenko <digetx@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 3/6] regulator: core: simplify nested locking

Hi,

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