Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
MIME-Version: 1.0
References: <20210711141430.896595-1-sxwjean@me.com> <20210711141430.896595-2-sxwjean@me.com>
 <0e48f59d-a8fd-936c-c57f-976632f9cead@redhat.com>
In-Reply-To: <0e48f59d-a8fd-936c-c57f-976632f9cead@redhat.com>
From:   Xiongwei Song <sxwjean@gmail.com>
Date:   Mon, 12 Jul 2021 15:48:42 +0800
Message-ID: <CAEVVKH8q2Dyhv2B7DBXmncnaEUBgFy9UgmDwGepWjuA9SdWjwQ@mail.gmail.com>
Subject: Re: [RFC PATCH v1 2/3] locking/lockdep: Unify the return values of check_wait_context()
To:     Waiman Long <llong@redhat.com>
Cc:     Xiongwei Song <sxwjean@me.com>, peterz@infradead.org,
        mingo@redhat.com, will@kernel.org,
        Boqun Feng <boqun.feng@gmail.com>,
        Linux Kernel Mailing List <linux-kernel@vger.kernel.org>
Content-Type: text/plain; charset="UTF-8"
Precedence: bulk

On Sun, Jul 11, 2021 at 11:19 PM Waiman Long <llong@redhat.com> wrote:
>
> On 7/11/21 10:14 AM, Xiongwei Song wrote:
> > From: Xiongwei Song <sxwjean@gmail.com>
> >
> > Unity the return values of check_wait_context() as check_prev_add(),
> "Unify"?
Sorry. Will improve the description.

> > check_irq_usage(), etc. 1 means no bug, 0 means there is a bug.
> >
> > The return values of print_lock_invalid_wait_context() are unnecessary,
> > remove them.
> >
> > Signed-off-by: Xiongwei Song <sxwjean@gmail.com>
> > ---
> >   kernel/locking/lockdep.c | 20 ++++++++++----------
> >   1 file changed, 10 insertions(+), 10 deletions(-)
> >
> > diff --git a/kernel/locking/lockdep.c b/kernel/locking/lockdep.c
> > index bf1c00c881e4..8b50da42f2c6 100644
> > --- a/kernel/locking/lockdep.c
> > +++ b/kernel/locking/lockdep.c
> > @@ -4635,16 +4635,16 @@ static inline short task_wait_context(struct task_struct *curr)
> >       return LD_WAIT_MAX;
> >   }
> >
> > -static int
> > +static void
> >   print_lock_invalid_wait_context(struct task_struct *curr,
> >                               struct held_lock *hlock)
> >   {
> >       short curr_inner;
> >
> >       if (!debug_locks_off())
> > -             return 0;
> > +             return;
> >       if (debug_locks_silent)
> > -             return 0;
> > +             return;
> >
> >       pr_warn("\n");
> >       pr_warn("=============================\n");
> > @@ -4664,8 +4664,6 @@ print_lock_invalid_wait_context(struct task_struct *curr,
> >
> >       pr_warn("stack backtrace:\n");
> >       dump_stack();
> > -
> > -     return 0;
> >   }
> >
> >   /*
> > @@ -4691,7 +4689,7 @@ static int check_wait_context(struct task_struct *curr, struct held_lock *next)
> >       int depth;
> >
> >       if (!next_inner || next->trylock)
> > -             return 0;
> > +             return 1;
> >
> >       if (!next_outer)
> >               next_outer = next_inner;
> > @@ -4723,10 +4721,12 @@ static int check_wait_context(struct task_struct *curr, struct held_lock *next)
> >               }
> >       }
> >
> > -     if (next_outer > curr_inner)
> > -             return print_lock_invalid_wait_context(curr, next);
> > +     if (next_outer > curr_inner) {
> > +             print_lock_invalid_wait_context(curr, next);
> > +             return 0;
> > +     }
> >
> > -     return 0;
> > +     return 1;
> >   }
> >
> >   #else /* CONFIG_PROVE_LOCKING */
> > @@ -4962,7 +4962,7 @@ static int __lock_acquire(struct lockdep_map *lock, unsigned int subclass,
> >   #endif
> >       hlock->pin_count = pin_count;
> >
> > -     if (check_wait_context(curr, hlock))
> > +     if (!check_wait_context(curr, hlock))
> >               return 0;
> >
> >       /* Initialize the lock usage bit */
>
> There is also another check_wait_context() in the "#else
> CONFIG_PROVE_LOCKING" path that needs to be kept in sync.
Oops, my fault.

For clarity,
> maybe you should state the meaning of the return value in the comment
> above the function.
Good point. Thanks.

Regards,
Xiongwei
>
> Cheers,
> Longman
>
> check_wait_context
>