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;
From:   Waiman Long <llong@redhat.com>
Subject: Re: [RFC PATCH v1 2/3] locking/lockdep: Unify the return values of
 check_wait_context()
To:     Xiongwei Song <sxwjean@me.com>, peterz@infradead.org,
        mingo@redhat.com, will@kernel.org, boqun.feng@gmail.com
Cc:     linux-kernel@vger.kernel.org, Xiongwei Song <sxwjean@gmail.com>
References: <20210711141430.896595-1-sxwjean@me.com>
 <20210711141430.896595-2-sxwjean@me.com>
Message-ID: <0e48f59d-a8fd-936c-c57f-976632f9cead@redhat.com>
Date:   Sun, 11 Jul 2021 11:19:40 -0400
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:78.0) Gecko/20100101
 Thunderbird/78.9.0
MIME-Version: 1.0
In-Reply-To: <20210711141430.896595-2-sxwjean@me.com>
Content-Type: text/plain; charset=utf-8; format=flowed
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk

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"?
> 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. For clarity, 
maybe you should state the meaning of the return value in the comment 
above the function.

Cheers,
Longman

check_wait_context