linux-kernel - Re: [PATCH] bitops: Add a comment explaining the double underscore macros

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] [day] [month] [year] [list]

Message-ID: <Zmb1ebn3HWPZc6Bo@yury-ThinkPad>
Date: Mon, 10 Jun 2024 05:45:45 -0700
From: Yury Norov <yury.norov@...il.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Rasmus Villemoes <linux@...musvillemoes.dk>,
	linux-kernel@...r.kernel.org, kernel-janitors@...r.kernel.org,
	Linus Walleij <linus.walleij@...aro.org>
Subject: Re: [PATCH] bitops: Add a comment explaining the double underscore
 macros

On Mon, Jun 10, 2024 at 12:18:03PM +0300, Dan Carpenter wrote:
> Linus Walleij pointed out that a new comer might be confused about the
> difference between set_bit() and __set_bit().  Add a comment explaining
> the difference.
> 
> Link: https://lore.kernel.org/all/CACRpkdZFPG_YLici-BmYfk9HZ36f4WavCN3JNotkk8cPgCODCg@mail.gmail.com/
> Signed-off-by: Dan Carpenter <dan.carpenter@...aro.org>
> ---
>  include/linux/bitops.h | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index 46d4bdc634c0..b35a5c3783f6 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -29,6 +29,9 @@ extern unsigned long __sw_hweight64(__u64 w);
>  #include <asm-generic/bitops/generic-non-atomic.h>
>  
>  /*
> + * These double underscore __set_bit(), __clear_bit() macros are non-atomic
> + * versions of set_bit(), clear_bit() and so on.
> + *
>   * Many architecture-specific non-atomic bitops contain inline asm code and due
>   * to that the compiler can't optimize them to compile-time expressions or
>   * constants. In contrary, generic_*() helpers are defined in pure C and
> -- 
> 2.39.2

If you find the underscored notation confusing (everyone does),
and want to add an extra notice, I'm OK with that. But...

The comment you're prepending relates to the bitop() macro, not
those individual bit ops. bitop() is used to generate test_bit()
as well, with is not split to atomic/non-atomic.

Can you put your note on top of the actual macro declarations, one
starting with '#define __set_bit()'. Can you also please use a more
neutral language. Maybe something like this?

        The following macros are non-atomic versions of their
        non-underscored counterparts.

Now that you explicitly mention non-atomic macros, and for test_bit()
and test_bit_aquire() there's no such separation, can you add a blank
line to split them out and make it clear that the comment is not
related to the test_bit() guys.

Thanks,
Yury