Re: [RFC PATCH 1/2] bits: introduce ffs_val()

[Thomas Zimmermann]

Hi Petr

Am 09.01.26 um 17:37 schrieb Petr Tesarik:
> Introduce a macro that can efficiently extract the least significant
> non-zero bit from a value.
>
> Interestingly, this bit-twiddling trick is open-coded in some places, but
> it also appears to be little known, leading to various inefficient
> implementations in other places. Let's make it part of the standard bitops
> arsenal.
>
> Define the macro in a separate header file included from <linux/bitops.h>,
> to allow using it in very low-level header files that may not want to
> include all of <linux/bitops.h>.
>
> Signed-off-by: Petr Tesarik <ptesarik@xxxxxxxx>
> ---
>   MAINTAINERS             |  1 +
>   include/linux/bitops.h  |  1 +
>   include/linux/ffs_val.h | 21 +++++++++++++++++++++
>   3 files changed, 23 insertions(+)
>   create mode 100644 include/linux/ffs_val.h
>
> diff --git a/MAINTAINERS b/MAINTAINERS
> index a0dd762f5648b..8f15c76a67ea2 100644
> --- a/MAINTAINERS
> +++ b/MAINTAINERS
> @@ -4466,6 +4466,7 @@ F:	arch/*/lib/bitops.c
>   F:	include/asm-generic/bitops
>   F:	include/asm-generic/bitops.h
>   F:	include/linux/bitops.h
> +F:	include/linux/ffs_val.h
>   F:	lib/hweight.c
>   F:	lib/test_bitops.c
>   F:	tools/*/bitops*
> diff --git a/include/linux/bitops.h b/include/linux/bitops.h
> index ea7898cc59039..209f0c3e07b9e 100644
> --- a/include/linux/bitops.h
> +++ b/include/linux/bitops.h
> @@ -4,6 +4,7 @@
>   
>   #include <asm/types.h>
>   #include <linux/bits.h>
> +#include <linux/ffs_val.h>
>   #include <linux/typecheck.h>
>   
>   #include <uapi/linux/kernel.h>
> diff --git a/include/linux/ffs_val.h b/include/linux/ffs_val.h
> new file mode 100644
> index 0000000000000..193ec86d2b53b
> --- /dev/null
> +++ b/include/linux/ffs_val.h
> @@ -0,0 +1,21 @@
> +/* SPDX-License-Identifier: GPL-2.0 */
> +#ifndef _ASM_LINUX_FFS_VAL_H_
> +#define _ASM_LINUX_FFS_VAL_H_
> +
> +/**
> + * ffs_val - find the value of the first set bit
> + * @x: the value to search
> + *
> + * Unlike ffs(), which returns a bit position, ffs_val() returns the bit
> + * value itself.

This sentence was confusing me at first, because the individual bit's 
value is always '1'. Maybe say something more descriptive, such as 
'ffs_val returns the value resulting from that bit's position.'

> + *
> + * Returns:
> + * least significant non-zero bit, 0 if all bits are zero

Same here.

> + */
> +#define ffs_val(x)			\
> +({					\
> +	const typeof(x) val__ = (x);	\
> +	val__ & -val__;			\

Is this construct supposed to work with signed integers and/or negative 
numbers? I assume that two's complement can be expected nowadays, but 
for LONG_MIN it returns zero AFAICT. The documentation should mention 
these cases.

Best regards
Thomas

> +})
> +
> +#endif /* _ASM_LINUX_FFS_VAL_H_ */

--
--
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstr. 146, 90461 Nürnberg, Germany, www.suse.com
GF: Jochen Jaser, Andrew McDonald, Werner Knoblich, (HRB 36809, AG Nürnberg)