linux-kernel - Re: [PATCH v2 3/7] bitfield: introduce HI16

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <6139278.alqRGMn8q6@diego>
Date: Mon, 02 Jun 2025 21:01:35 +0200
From: Heiko Stübner <heiko@...ech.de>
To: Linus Walleij <linus.walleij@...aro.org>, Rob Herring <robh@...nel.org>,
 Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
 Uwe Kleine-König <ukleinek@...nel.org>,
 William Breathitt Gray <wbg@...nel.org>,
 Sebastian Reichel <sebastian.reichel@...labora.com>,
 Kever Yang <kever.yang@...k-chips.com>, Yury Norov <yury.norov@...il.com>,
 Rasmus Villemoes <linux@...musvillemoes.dk>,
 Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
 Dave Ertman <david.m.ertman@...el.com>, Ira Weiny <ira.weiny@...el.com>,
 Leon Romanovsky <leon@...nel.org>, Lee Jones <lee@...nel.org>,
 linux-gpio@...r.kernel.org, devicetree@...r.kernel.org,
 linux-arm-kernel@...ts.infradead.org, linux-rockchip@...ts.infradead.org,
 linux-kernel@...r.kernel.org, linux-pwm@...r.kernel.org,
 linux-iio@...r.kernel.org, kernel@...labora.com,
 Jonas Karlman <jonas@...boo.se>,
 Detlev Casanova <detlev.casanova@...labora.com>,
 Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
Subject: Re: [PATCH v2 3/7] bitfield: introduce HI16_WE bitfield prep macros

Am Montag, 2. Juni 2025, 18:19:14 Mitteleuropäische Sommerzeit schrieb Nicolas Frattaroli:
> Hardware of various vendors, but very notably Rockchip, often uses
> 32-bit registers where the upper 16-bit half of the register is a
> write-enable mask for the lower half.
> 
> This type of hardware setup allows for more granular concurrent register
> write access.
> 
> Over the years, many drivers have hand-rolled their own version of this
> macro, usually without any checks, often called something like
> HIWORD_UPDATE or FIELD_PREP_HIWORD, commonly with slightly different
> semantics between them.
> 
> Clearly there is a demand for such a macro, and thus the demand should
> be satisfied in a common header file.
> 
> Add two macros: FIELD_PREP_HI16_WE, and FIELD_PREP_HI16_WE_CONST. The
> latter is a version that can be used in initializers, like
> FIELD_PREP_CONST. The macro names are chosen to explicitly reference the
> assumed half-register width, and its function, while not clashing with
> any potential other macros that drivers may already have implemented
> themselves.
> 
> Future drivers should use these macros instead of handrolling their own,
> and old drivers can be ported to the new macros as time and opportunity
> allows.
> 
> Signed-off-by: Nicolas Frattaroli <nicolas.frattaroli@...labora.com>
> ---
>  include/linux/bitfield.h | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 6d9a53db54b66c0833973c880444bd289d9667b1..2b3e7cb90ccb5d48f510104f61443b06748bb7eb 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -8,6 +8,7 @@
>  #define _LINUX_BITFIELD_H
>  
>  #include <linux/build_bug.h>
> +#include <linux/limits.h>
>  #include <linux/typecheck.h>
>  #include <asm/byteorder.h>
>  
> @@ -142,6 +143,52 @@
>  		(((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))	\
>  	)
>  
> +/**
> + * FIELD_PREP_HI16_WE() - prepare a bitfield element with a write-enable mask
> + * @_mask: shifted mask defining the field's length and position
> + * @_val:  value to put in the field
> + *
> + * FIELD_PREP_HI16_WE() masks and shifts up the value, as well as bitwise ORs
> + * the result with the mask shifted up by 16.
> + *
> + * This is useful for a common design of hardware registers where the upper
> + * 16-bit half of a 32-bit register is used as a write-enable mask. In such a
> + * register, a bit in the lower half is only updated if the corresponding bit
> + * in the upper half is high.
> + */
> +#define FIELD_PREP_HI16_WE(_mask, _val)					\
> +	({								\
> +		__BF_FIELD_CHECK(_mask, ((u16) 0U), _val,		\
> +				 "FIELD_PREP_HI16_WE: ");		\
> +		((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask) |	\

gcc is quite adamant about suggesting more parentheses here:

../include/linux/bitfield.h:163:70: warning: suggest parentheses around arithmetic in operand of ‘|’ [-Wparentheses]
  163 |                 ((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask) |  \
      |                                                                      ^
../include/soc/rockchip/mfpwm.h:225:41: note: in expansion of macro ‘FIELD_PREP_HI16_WE’
  225 | #define PWMV4_MODE(v)                   FIELD_PREP_HI16_WE(PWMV4_MODE_MASK, (v))
      |                                         ^~~~~~~~~~~~~~~~~~
../include/soc/rockchip/mfpwm.h:230:34: note: in expansion of macro ‘PWMV4_MODE’
  230 | #define PWMV4_CTRL_CONT_FLAGS   (PWMV4_MODE(PWMV4_MODE_CONT) | \
      |                                  ^~~~~~~~~~
../drivers/pwm/pwm-rockchip-v4.c:237:57: note: in expansion of macro ‘PWMV4_CTRL_CONT_FLAGS’
  237 |         mfpwm_reg_write(pc->pwmf->base, PWMV4_REG_CTRL, PWMV4_CTRL_CONT_FLAGS);
      |                                                         ^~~~~~~~~~~~~~~~~~~~~


Heiko