[PATCH] bitops: Add a comment explaining the double underscore macros

[PATCH] bitops: Add a comment explaining the double underscore macros

[Dan Carpenter]

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@linaro.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

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

[Yury Norov]

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@linaro.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

[PATCH] bitops: Add a comment explaining the double underscore macros

[Dan Carpenter]

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@linaro.org>
---
v2: re-word the comment, put it right next to the macros and add a blank
    line in front of the test_bit() macros so it's not mixed in with the
    non-atomic macros

 include/linux/bitops.h | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/include/linux/bitops.h b/include/linux/bitops.h
index 46d4bdc634c0..ba35bbf07798 100644
--- a/include/linux/bitops.h
+++ b/include/linux/bitops.h
@@ -47,12 +47,17 @@ extern unsigned long __sw_hweight64(__u64 w);
 	  __builtin_constant_p(*(const unsigned long *)(addr))) ?	\
 	 const##op(nr, addr) : op(nr, addr))
 
+/*
+ * The following macros are non-atomic versions of their non-underscored
+ * counterparts.
+ */
 #define __set_bit(nr, addr)		bitop(___set_bit, nr, addr)
 #define __clear_bit(nr, addr)		bitop(___clear_bit, nr, addr)
 #define __change_bit(nr, addr)		bitop(___change_bit, nr, addr)
 #define __test_and_set_bit(nr, addr)	bitop(___test_and_set_bit, nr, addr)
 #define __test_and_clear_bit(nr, addr)	bitop(___test_and_clear_bit, nr, addr)
 #define __test_and_change_bit(nr, addr)	bitop(___test_and_change_bit, nr, addr)
+
 #define test_bit(nr, addr)		bitop(_test_bit, nr, addr)
 #define test_bit_acquire(nr, addr)	bitop(_test_bit_acquire, nr, addr)
 
-- 
2.39.2

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

[Linus Walleij]

On Tue, Jun 11, 2024 at 2:38 PM Dan Carpenter <dan.carpenter@linaro.org> 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@linaro.org>
> ---
> v2: re-word the comment, put it right next to the macros and add a blank
>     line in front of the test_bit() macros so it's not mixed in with the
>     non-atomic macros

Thanks Dan! This makes the kernel a better place.
Reviewed-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

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

[Yury Norov]

On Tue, Jun 11, 2024 at 03:38:12PM +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@linaro.org>
> ---
> v2: re-word the comment, put it right next to the macros and add a blank
>     line in front of the test_bit() macros so it's not mixed in with the
>     non-atomic macros
> 
>  include/linux/bitops.h | 5 +++++
>  1 file changed, 5 insertions(+)
> 
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index 46d4bdc634c0..ba35bbf07798 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -47,12 +47,17 @@ extern unsigned long __sw_hweight64(__u64 w);
>  	  __builtin_constant_p(*(const unsigned long *)(addr))) ?	\
>  	 const##op(nr, addr) : op(nr, addr))
>  
> +/*
> + * The following macros are non-atomic versions of their non-underscored
> + * counterparts.
> + */
>  #define __set_bit(nr, addr)		bitop(___set_bit, nr, addr)
>  #define __clear_bit(nr, addr)		bitop(___clear_bit, nr, addr)
>  #define __change_bit(nr, addr)		bitop(___change_bit, nr, addr)
>  #define __test_and_set_bit(nr, addr)	bitop(___test_and_set_bit, nr, addr)
>  #define __test_and_clear_bit(nr, addr)	bitop(___test_and_clear_bit, nr, addr)
>  #define __test_and_change_bit(nr, addr)	bitop(___test_and_change_bit, nr, addr)
> +
>  #define test_bit(nr, addr)		bitop(_test_bit, nr, addr)
>  #define test_bit_acquire(nr, addr)	bitop(_test_bit_acquire, nr, addr)
>  
> -- 
> 2.39.2

Applied in bitmap-for-next. For the next time please make the subject
prefix [PATCH v2], then [PATCH v3], and so on. The motivation is to
avoid sending emails with identical subjects as some (not mine) email
clients consider one as a reply to another.

Thanks,
Yury

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

[Dan Carpenter]

On Tue, Jun 11, 2024 at 01:42:09PM -0700, Yury Norov wrote:
> 
> Applied in bitmap-for-next. For the next time please make the subject
> prefix [PATCH v2], then [PATCH v3], and so on. The motivation is to
> avoid sending emails with identical subjects as some (not mine) email
> clients consider one as a reply to another.

Oops.  Sorry.  I meant to do it that way, but I messed up.  Thanks!

regards,
dan carpenter