Re: [PATCH v2] bitfield: Add FIELD_MODIFY() helper

[Yury Norov]

On Thu, Apr 10, 2025 at 09:10:48PM +0800, Luo Jie wrote:
> Add a helper for replacing the contents of bitfield in memory
> with the specified value.
> 
> Even though a helper xxx_replace_bits() is available, it is not
> well documented, and only reports errors at the run time, which
> will not be helpful to catch possible overflow errors due to
> incorrect parameter types used.
> 
> Add the helper FIELD_MODIFY() to the FIELD_XXX family of bitfield
> macros. It is functionally similar as xxx_replace_bits(), and in
> addition adds the compile time type checking.
> 
> FIELD_MODIFY(&reg, REG_FIELD_C, c) is the wrapper of the code below.
> reg &= ~REG_FIELD_C;
> reg |= FIELD_PREP(REG_FIELD_C, c);
> 
> Signed-off-by: Luo Jie <quic_luoj@xxxxxxxxxxx>
> ---
> 
> The new added macro FIELD_MODIFY() is expected to be used by the
> following Ethernet PPE driver as link.
> https://lore.kernel.org/linux-arm-msm/20250209-qcom_ipq_ppe-v3-0-453ea18d3271@xxxxxxxxxxx/
> 
> Changes in v2:
> - Update the documented example for FIELD_MODIFY().
> - Improve the commit message to describe the need for the change.
> 
>  include/linux/bitfield.h | 23 ++++++++++++++++++++---
>  1 file changed, 20 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 63928f173223..421c7701a18d 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -7,8 +7,9 @@
>  #ifndef _LINUX_BITFIELD_H
>  #define _LINUX_BITFIELD_H
>  
> -#include <linux/build_bug.h>
>  #include <asm/byteorder.h>
> +#include <linux/build_bug.h>
> +#include <linux/typecheck.h>

Don't change the headers order: linux first, asm next.

>  
>  /*
>   * Bitfield access macros
> @@ -38,8 +39,7 @@
>   *	  FIELD_PREP(REG_FIELD_D, 0x40);
>   *
>   * Modify:
> - *  reg &= ~REG_FIELD_C;
> - *  reg |= FIELD_PREP(REG_FIELD_C, c);
> + *  FIELD_MODIFY(REG_FIELD_C, &reg, c);
>   */
>  
>  #define __bf_shf(x) (__builtin_ffsll(x) - 1)
> @@ -156,6 +156,23 @@
>  		(typeof(_mask))(((_reg) & (_mask)) >> __bf_shf(_mask));	\
>  	})
>  
> +/**
> + * FIELD_MODIFY() - modify a bitfield element
> + * @_mask: shifted mask defining the field's length and position
> + * @_reg_p: pointer to the memory that should be updated
> + * @_val: value to store in the bitfield
> + *
> + * FIELD_MODIFY() modifies the set of bits in @_reg_p specified by @_mask,
> + * by replacing them with the bitfield value passed in as @_val.
> + */

Please inspect the codebase and convert existing opencoded FIELD_MODIFY()s.
I don't ask you to convert every driver out there, but core kernel files
should be clear.

The first good candidate for you is __tlbi_level() in arm64. You may want
to use Coccinelle to automate the search.

Thanks,
Yury

> +#define FIELD_MODIFY(_mask, _reg_p, _val)				\
> +	({								\
> +		typecheck_pointer(_reg_p);				\
> +		__BF_FIELD_CHECK(_mask, *(_reg_p), _val, "FIELD_MODIFY: "); \
> +		*(_reg_p) &= ~(_mask);					\
> +		*(_reg_p) |= ((_val) << __bf_shf(_mask)) & (_mask);	\
> +	})
> +
>  extern void __compiletime_error("value doesn't fit into mask")
>  __field_overflow(void);
>  extern void __compiletime_error("bad bitfield mask")
> -- 
> 2.34.1