[PATCH v2 0/3] Non-const bitfield helpers

[PATCH v2 0/3] Non-const bitfield helpers

[Geert Uytterhoeven]

	Hi all,

This is an updated subset of a patch series I sent more than 3 years
ago[1].

<linux/bitfield.h> contains various helpers for accessing bitfields, as
typically used in hardware registers for memory-mapped I/O blocks.
These helpers ensure type safety, and deduce automatically shift values
from mask values, avoiding mistakes due to inconsistent shifts and
masks, and leading to a reduction in source code size.

The existing FIELD_{GET,PREP}() macros are limited to compile-time
constants.  However, it is very common to prepare or extract bitfield
elements where the bitfield mask is not a compile-time constant.
To avoid this limitation, the AT91 clock driver introduced its own
field_{prep,get}() macros.  Hence my v1 series aimed to make them
available for general use, and convert several drivers to the existing
FIELD_{GET,PREP}() and the new field_{get,prep}() helpers.

Due to some pushback (mostly centered around using the typed
{u*,be*,le*,...}_get_bits() macros instead, which of course would
require making them work with non-constant masks first, too), this
series was never applied, and became buried deep in my TODO haystack...
However, several people still liked the idea: since v1, multiple copies
of the field_{prep,get}() macros appeared upstream, and one more is
queued for v6.15.

Hence I think it's time to revive and consolidate...

Changes compared to v1:
  - Cast val resp. reg to the mask type,
  - Fix 64-bit use on 32-bit architectures,
  - Convert new upstream users:
      - drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c
      - drivers/gpio/gpio-aspeed.c
      - drivers/iio/temperature/mlx90614.c
      - drivers/pinctrl/nuvoton/pinctrl-ma35.c
      - sound/usb/mixer_quirks.c
  - Convert new user queued in renesas-devel for v6.15:
      - drivers/soc/renesas/rz-sysc.c
  - Drop the last 14 RFC patches.
    They can be updated/resubmitted/applied later.

I can take all three patches through the Renesas tree, and provide an
immutable branch with the first patch for ther interested parties.

Thanks for your comments!

[1] "[PATCH 00/17] Non-const bitfield helper conversions"
    https://lore.kernel.org/all/cover.1637592133.git.geert+renesas@glider.be

Geert Uytterhoeven (3):
  bitfield: Add non-constant field_{prep,get}() helpers
  clk: renesas: Use bitfield helpers
  soc: renesas: Use bitfield helpers

 drivers/clk/at91/clk-peripheral.c             |  1 +
 drivers/clk/at91/pmc.h                        |  3 --
 drivers/clk/renesas/clk-div6.c                |  6 ++--
 drivers/clk/renesas/rcar-gen3-cpg.c           | 15 +++-----
 drivers/clk/renesas/rcar-gen4-cpg.c           |  9 ++---
 .../qat/qat_common/adf_gen4_pm_debugfs.c      |  8 +----
 drivers/gpio/gpio-aspeed.c                    |  5 +--
 drivers/iio/temperature/mlx90614.c            |  5 +--
 drivers/pinctrl/nuvoton/pinctrl-ma35.c        |  4 ---
 drivers/soc/renesas/renesas-soc.c             |  4 +--
 drivers/soc/renesas/rz-sysc.c                 |  3 +-
 include/linux/bitfield.h                      | 34 +++++++++++++++++++
 sound/usb/mixer_quirks.c                      |  4 ---
 13 files changed, 52 insertions(+), 49 deletions(-)

-- 
2.43.0

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
							    -- Linus Torvalds

[PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Geert Uytterhoeven]

The existing FIELD_{GET,PREP}() macros are limited to compile-time
constants.  However, it is very common to prepare or extract bitfield
elements where the bitfield mask is not a compile-time constant.

To avoid this limitation, the AT91 clock driver and several other
drivers already have their own non-const field_{prep,get}() macros.
Make them available for general use by consolidating them in
<linux/bitfield.h>, and improve them slightly:
  1. Avoid evaluating macro parameters more than once,
  2. Replace "ffs() - 1" by "__ffs()",
  3. Support 64-bit use on 32-bit architectures.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Cast val resp. reg to the mask type,
  - Fix 64-bit use on 32-bit architectures,
  - Convert new upstream users:
      - drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c
      - drivers/gpio/gpio-aspeed.c
      - drivers/iio/temperature/mlx90614.c
      - drivers/pinctrl/nuvoton/pinctrl-ma35.c
      - sound/usb/mixer_quirks.c
  - Convert new user queued in renesas-devel for v6.15:
      - drivers/soc/renesas/rz-sysc.c
---
 drivers/clk/at91/clk-peripheral.c             |  1 +
 drivers/clk/at91/pmc.h                        |  3 --
 .../qat/qat_common/adf_gen4_pm_debugfs.c      |  8 +----
 drivers/gpio/gpio-aspeed.c                    |  5 +--
 drivers/iio/temperature/mlx90614.c            |  5 +--
 drivers/pinctrl/nuvoton/pinctrl-ma35.c        |  4 ---
 drivers/soc/renesas/rz-sysc.c                 |  3 +-
 include/linux/bitfield.h                      | 34 +++++++++++++++++++
 sound/usb/mixer_quirks.c                      |  4 ---
 9 files changed, 39 insertions(+), 28 deletions(-)

diff --git a/drivers/clk/at91/clk-peripheral.c b/drivers/clk/at91/clk-peripheral.c
index c173a44c800aa8cc..60208bdc3fe4797e 100644
--- a/drivers/clk/at91/clk-peripheral.c
+++ b/drivers/clk/at91/clk-peripheral.c
@@ -3,6 +3,7 @@
  *  Copyright (C) 2013 Boris BREZILLON <b.brezillon@overkiz.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/bitops.h>
 #include <linux/clk-provider.h>
 #include <linux/clkdev.h>
diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h
index 4fb29ca111f7d427..3838e4f7df2d4a70 100644
--- a/drivers/clk/at91/pmc.h
+++ b/drivers/clk/at91/pmc.h
@@ -116,9 +116,6 @@ struct at91_clk_pms {
 	unsigned int parent;
 };
 
-#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
-#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask))
-
 #define ndck(a, s) (a[s - 1].id + 1)
 #define nck(a) (a[ARRAY_SIZE(a) - 1].id + 1)
 
diff --git a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c
index 2e4095c4c12c94f9..ebaa59e934178309 100644
--- a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c
+++ b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c
@@ -1,5 +1,6 @@
 // SPDX-License-Identifier: GPL-2.0-only
 /* Copyright(c) 2023 Intel Corporation */
+#include <linux/bitfield.h>
 #include <linux/dma-mapping.h>
 #include <linux/kernel.h>
 #include <linux/string_helpers.h>
@@ -11,13 +12,6 @@
 #include "adf_gen4_pm.h"
 #include "icp_qat_fw_init_admin.h"
 
-/*
- * This is needed because a variable is used to index the mask at
- * pm_scnprint_table(), making it not compile time constant, so the compile
- * asserts from FIELD_GET() or u32_get_bits() won't be fulfilled.
- */
-#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
-
 #define PM_INFO_MEMBER_OFF(member)	\
 	(offsetof(struct icp_qat_fw_init_admin_pm_info, member) / sizeof(u32))
 
diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
index 40c1bd80f8b0434d..b45e4dd8d8e4f00a 100644
--- a/drivers/gpio/gpio-aspeed.c
+++ b/drivers/gpio/gpio-aspeed.c
@@ -5,6 +5,7 @@
  * Joel Stanley <joel@jms.id.au>
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk.h>
 #include <linux/gpio/aspeed.h>
 #include <linux/gpio/driver.h>
@@ -30,10 +31,6 @@
 #include <linux/gpio/consumer.h>
 #include "gpiolib.h"
 
-/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */
-#define field_get(_mask, _reg)	(((_reg) & (_mask)) >> (ffs(_mask) - 1))
-#define field_prep(_mask, _val)	(((_val) << (ffs(_mask) - 1)) & (_mask))
-
 #define GPIO_G7_IRQ_STS_BASE 0x100
 #define GPIO_G7_IRQ_STS_OFFSET(x) (GPIO_G7_IRQ_STS_BASE + (x) * 0x4)
 #define GPIO_G7_CTRL_REG_BASE 0x180
diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
index 740018d4b3dfb35e..c58dc59d4f570831 100644
--- a/drivers/iio/temperature/mlx90614.c
+++ b/drivers/iio/temperature/mlx90614.c
@@ -22,6 +22,7 @@
  * the "wakeup" GPIO is not given, power management will be disabled.
  */
 
+#include <linux/bitfield.h>
 #include <linux/delay.h>
 #include <linux/err.h>
 #include <linux/gpio/consumer.h>
@@ -68,10 +69,6 @@
 #define MLX90614_CONST_SCALE 20 /* Scale in milliKelvin (0.02 * 1000) */
 #define MLX90614_CONST_FIR 0x7 /* Fixed value for FIR part of low pass filter */
 
-/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */
-#define field_get(_mask, _reg)	(((_reg) & (_mask)) >> (ffs(_mask) - 1))
-#define field_prep(_mask, _val)	(((_val) << (ffs(_mask) - 1)) & (_mask))
-
 struct mlx_chip_info {
 	/* EEPROM offsets with 16-bit data, MSB first */
 	/* emissivity correction coefficient */
diff --git a/drivers/pinctrl/nuvoton/pinctrl-ma35.c b/drivers/pinctrl/nuvoton/pinctrl-ma35.c
index 59c4e7c6cddea127..3ba28faa8e1418a9 100644
--- a/drivers/pinctrl/nuvoton/pinctrl-ma35.c
+++ b/drivers/pinctrl/nuvoton/pinctrl-ma35.c
@@ -81,10 +81,6 @@
 #define MVOLT_1800			0
 #define MVOLT_3300			1
 
-/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */
-#define field_get(_mask, _reg)	(((_reg) & (_mask)) >> (ffs(_mask) - 1))
-#define field_prep(_mask, _val)	(((_val) << (ffs(_mask) - 1)) & (_mask))
-
 static const char * const gpio_group_name[] = {
 	"gpioa", "gpiob", "gpioc", "gpiod", "gpioe", "gpiof", "gpiog",
 	"gpioh", "gpioi", "gpioj", "gpiok", "gpiol", "gpiom", "gpion",
diff --git a/drivers/soc/renesas/rz-sysc.c b/drivers/soc/renesas/rz-sysc.c
index 1c98da37b7d18745..917a029d849585cd 100644
--- a/drivers/soc/renesas/rz-sysc.c
+++ b/drivers/soc/renesas/rz-sysc.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2024 Renesas Electronics Corp.
  */
 
+#include <linux/bitfield.h>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
@@ -12,8 +13,6 @@
 
 #include "rz-sysc.h"
 
-#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
-
 /**
  * struct rz_sysc - RZ SYSC private data structure
  * @base: SYSC base address
diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 63928f1732230700..c62324a9fcc81241 100644
--- a/include/linux/bitfield.h
+++ b/include/linux/bitfield.h
@@ -203,4 +203,38 @@ __MAKE_OP(64)
 #undef __MAKE_OP
 #undef ____MAKE_OP
 
+/**
+ * field_prep() - prepare a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_val:  value to put in the field
+ *
+ * field_prep() masks and shifts up the value.  The result should be
+ * combined with other fields of the bitfield using logical OR.
+ * Unlike FIELD_PREP(), @_mask is not limited to a compile-time constant.
+ */
+#define field_prep(_mask, _val)						\
+	({								\
+		typeof(_mask) __mask = (_mask);				\
+		unsigned int __shift = sizeof(_mask) <= 4 ?		\
+				       __ffs(__mask) : __ffs64(__mask);	\
+		(((typeof(_mask))(_val) << __shift) & (__mask));	\
+	})
+
+/**
+ * field_get() - extract a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_reg:  value of entire bitfield
+ *
+ * field_get() extracts the field specified by @_mask from the
+ * bitfield passed in as @_reg by masking and shifting it down.
+ * Unlike FIELD_GET(), @_mask is not limited to a compile-time constant.
+ */
+#define field_get(_mask, _reg)						\
+	({								\
+		typeof(_mask) __mask = _mask;				\
+		unsigned int __shift = sizeof(_mask) <= 4 ?		\
+				       __ffs(__mask) : __ffs64(__mask);	\
+		(typeof(_mask))(((_reg) & (__mask)) >> __shift);	\
+	})
+
 #endif
diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
index 23fcd680167d0298..00ab811e4b11a573 100644
--- a/sound/usb/mixer_quirks.c
+++ b/sound/usb/mixer_quirks.c
@@ -3109,10 +3109,6 @@ static int snd_bbfpro_controls_create(struct usb_mixer_interface *mixer)
 #define RME_DIGIFACE_REGISTER(reg, mask) (((reg) << 16) | (mask))
 #define RME_DIGIFACE_INVERT BIT(31)
 
-/* Nonconst helpers */
-#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
-#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask))
-
 static int snd_rme_digiface_write_reg(struct snd_kcontrol *kcontrol, int item, u16 mask, u16 val)
 {
 	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kcontrol);
-- 
2.43.0

[PATCH v2 2/3] clk: renesas: Use bitfield helpers

[Geert Uytterhoeven]

Use the FIELD_{GET,PREP}() and field_{get,prep}() helpers for const
respective non-const bitfields, instead of open-coding the same
operations.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Rebase on top of commit 470e3f0d0b1529ab ("clk: renesas: rcar-gen4:
    Introduce R-Car Gen4 CPG driver").
---
 drivers/clk/renesas/clk-div6.c      |  6 +++---
 drivers/clk/renesas/rcar-gen3-cpg.c | 15 +++++----------
 drivers/clk/renesas/rcar-gen4-cpg.c |  9 +++------
 3 files changed, 11 insertions(+), 19 deletions(-)

diff --git a/drivers/clk/renesas/clk-div6.c b/drivers/clk/renesas/clk-div6.c
index 3abd6e5400aded6a..f7b827b5e9b2dd32 100644
--- a/drivers/clk/renesas/clk-div6.c
+++ b/drivers/clk/renesas/clk-div6.c
@@ -7,6 +7,7 @@
  * Contact: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
  */
 
+#include <linux/bitfield.h>
 #include <linux/clk-provider.h>
 #include <linux/init.h>
 #include <linux/io.h>
@@ -171,8 +172,7 @@ static u8 cpg_div6_clock_get_parent(struct clk_hw *hw)
 	if (clock->src_mask == 0)
 		return 0;
 
-	hw_index = (readl(clock->reg) & clock->src_mask) >>
-		   __ffs(clock->src_mask);
+	hw_index = field_get(clock->src_mask, readl(clock->reg));
 	for (i = 0; i < clk_hw_get_num_parents(hw); i++) {
 		if (clock->parents[i] == hw_index)
 			return i;
@@ -191,7 +191,7 @@ static int cpg_div6_clock_set_parent(struct clk_hw *hw, u8 index)
 	if (index >= clk_hw_get_num_parents(hw))
 		return -EINVAL;
 
-	src = clock->parents[index] << __ffs(clock->src_mask);
+	src = field_prep(clock->src_mask, clock->parents[index]);
 	writel((readl(clock->reg) & ~clock->src_mask) | src, clock->reg);
 	return 0;
 }
diff --git a/drivers/clk/renesas/rcar-gen3-cpg.c b/drivers/clk/renesas/rcar-gen3-cpg.c
index 027100e84ee4c429..ca8f6a68771628fb 100644
--- a/drivers/clk/renesas/rcar-gen3-cpg.c
+++ b/drivers/clk/renesas/rcar-gen3-cpg.c
@@ -54,10 +54,8 @@ static unsigned long cpg_pll_clk_recalc_rate(struct clk_hw *hw,
 {
 	struct cpg_pll_clk *pll_clk = to_pll_clk(hw);
 	unsigned int mult;
-	u32 val;
 
-	val = readl(pll_clk->pllcr_reg) & CPG_PLLnCR_STC_MASK;
-	mult = (val >> __ffs(CPG_PLLnCR_STC_MASK)) + 1;
+	mult = FIELD_GET(CPG_PLLnCR_STC_MASK, readl(pll_clk->pllcr_reg)) + 1;
 
 	return parent_rate * mult * pll_clk->fixed_mult;
 }
@@ -94,7 +92,7 @@ static int cpg_pll_clk_set_rate(struct clk_hw *hw, unsigned long rate,
 
 	val = readl(pll_clk->pllcr_reg);
 	val &= ~CPG_PLLnCR_STC_MASK;
-	val |= (mult - 1) << __ffs(CPG_PLLnCR_STC_MASK);
+	val |= FIELD_PREP(CPG_PLLnCR_STC_MASK, mult - 1);
 	writel(val, pll_clk->pllcr_reg);
 
 	for (i = 1000; i; i--) {
@@ -176,11 +174,7 @@ static unsigned long cpg_z_clk_recalc_rate(struct clk_hw *hw,
 					   unsigned long parent_rate)
 {
 	struct cpg_z_clk *zclk = to_z_clk(hw);
-	unsigned int mult;
-	u32 val;
-
-	val = readl(zclk->reg) & zclk->mask;
-	mult = 32 - (val >> __ffs(zclk->mask));
+	unsigned int mult = 32 - field_get(zclk->mask, readl(zclk->reg));
 
 	return DIV_ROUND_CLOSEST_ULL((u64)parent_rate * mult,
 				     32 * zclk->fixed_div);
@@ -231,7 +225,8 @@ static int cpg_z_clk_set_rate(struct clk_hw *hw, unsigned long rate,
 	if (readl(zclk->kick_reg) & CPG_FRQCRB_KICK)
 		return -EBUSY;
 
-	cpg_reg_modify(zclk->reg, zclk->mask, (32 - mult) << __ffs(zclk->mask));
+	cpg_reg_modify(zclk->reg, zclk->mask,
+		       field_prep(zclk->mask, 32 - mult));
 
 	/*
 	 * Set KICK bit in FRQCRB to update hardware setting and wait for
diff --git a/drivers/clk/renesas/rcar-gen4-cpg.c b/drivers/clk/renesas/rcar-gen4-cpg.c
index 31aa790fd003d45e..a63114a1d431968f 100644
--- a/drivers/clk/renesas/rcar-gen4-cpg.c
+++ b/drivers/clk/renesas/rcar-gen4-cpg.c
@@ -279,11 +279,7 @@ static unsigned long cpg_z_clk_recalc_rate(struct clk_hw *hw,
 					   unsigned long parent_rate)
 {
 	struct cpg_z_clk *zclk = to_z_clk(hw);
-	unsigned int mult;
-	u32 val;
-
-	val = readl(zclk->reg) & zclk->mask;
-	mult = 32 - (val >> __ffs(zclk->mask));
+	unsigned int mult = 32 - field_get(zclk->mask, readl(zclk->reg));
 
 	return DIV_ROUND_CLOSEST_ULL((u64)parent_rate * mult,
 				     32 * zclk->fixed_div);
@@ -334,7 +330,8 @@ static int cpg_z_clk_set_rate(struct clk_hw *hw, unsigned long rate,
 	if (readl(zclk->kick_reg) & CPG_FRQCRB_KICK)
 		return -EBUSY;
 
-	cpg_reg_modify(zclk->reg, zclk->mask, (32 - mult) << __ffs(zclk->mask));
+	cpg_reg_modify(zclk->reg, zclk->mask,
+		       field_prep(zclk->mask, 32 - mult));
 
 	/*
 	 * Set KICK bit in FRQCRB to update hardware setting and wait for
-- 
2.43.0

[PATCH v2 3/3] soc: renesas: Use bitfield helpers

[Geert Uytterhoeven]

Use the field_get() helper, instead of open-coding the same operation.

Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
---
v2:
  - Drop RFC, as a dependency was applied.
---
 drivers/soc/renesas/renesas-soc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/soc/renesas/renesas-soc.c b/drivers/soc/renesas/renesas-soc.c
index df2b38417b8042fc..8030b9a62ec46668 100644
--- a/drivers/soc/renesas/renesas-soc.c
+++ b/drivers/soc/renesas/renesas-soc.c
@@ -5,6 +5,7 @@
  * Copyright (C) 2014-2016 Glider bvba
  */
 
+#include <linux/bitfield.h>
 #include <linux/io.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
@@ -512,8 +513,7 @@ static int __init renesas_soc_init(void)
 							   eshi, eslo);
 		}
 
-		if (soc->id &&
-		    ((product & id->mask) >> __ffs(id->mask)) != soc->id) {
+		if (soc->id && field_get(id->mask, product) != soc->id) {
 			pr_warn("SoC mismatch (product = 0x%x)\n", product);
 			ret = -ENODEV;
 			goto free_soc_dev_attr;
-- 
2.43.0

Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Alexandre Belloni]

On 31/01/2025 14:46:51+0100, Geert Uytterhoeven wrote:
> The existing FIELD_{GET,PREP}() macros are limited to compile-time
> constants.  However, it is very common to prepare or extract bitfield
> elements where the bitfield mask is not a compile-time constant.
> 
> To avoid this limitation, the AT91 clock driver and several other
> drivers already have their own non-const field_{prep,get}() macros.
> Make them available for general use by consolidating them in
> <linux/bitfield.h>, and improve them slightly:
>   1. Avoid evaluating macro parameters more than once,
>   2. Replace "ffs() - 1" by "__ffs()",
>   3. Support 64-bit use on 32-bit architectures.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
Acked-by: Alexandre Belloni <alexandre.belloni@bootlin.com>

> ---
> v2:
>   - Cast val resp. reg to the mask type,
>   - Fix 64-bit use on 32-bit architectures,
>   - Convert new upstream users:
>       - drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c
>       - drivers/gpio/gpio-aspeed.c
>       - drivers/iio/temperature/mlx90614.c
>       - drivers/pinctrl/nuvoton/pinctrl-ma35.c
>       - sound/usb/mixer_quirks.c
>   - Convert new user queued in renesas-devel for v6.15:
>       - drivers/soc/renesas/rz-sysc.c
> ---
>  drivers/clk/at91/clk-peripheral.c             |  1 +
>  drivers/clk/at91/pmc.h                        |  3 --
>  .../qat/qat_common/adf_gen4_pm_debugfs.c      |  8 +----
>  drivers/gpio/gpio-aspeed.c                    |  5 +--
>  drivers/iio/temperature/mlx90614.c            |  5 +--
>  drivers/pinctrl/nuvoton/pinctrl-ma35.c        |  4 ---
>  drivers/soc/renesas/rz-sysc.c                 |  3 +-
>  include/linux/bitfield.h                      | 34 +++++++++++++++++++
>  sound/usb/mixer_quirks.c                      |  4 ---
>  9 files changed, 39 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/clk/at91/clk-peripheral.c b/drivers/clk/at91/clk-peripheral.c
> index c173a44c800aa8cc..60208bdc3fe4797e 100644
> --- a/drivers/clk/at91/clk-peripheral.c
> +++ b/drivers/clk/at91/clk-peripheral.c
> @@ -3,6 +3,7 @@
>   *  Copyright (C) 2013 Boris BREZILLON <b.brezillon@overkiz.com>
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/bitops.h>
>  #include <linux/clk-provider.h>
>  #include <linux/clkdev.h>
> diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h
> index 4fb29ca111f7d427..3838e4f7df2d4a70 100644
> --- a/drivers/clk/at91/pmc.h
> +++ b/drivers/clk/at91/pmc.h
> @@ -116,9 +116,6 @@ struct at91_clk_pms {
>  	unsigned int parent;
>  };
>  
> -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
> -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask))
> -
>  #define ndck(a, s) (a[s - 1].id + 1)
>  #define nck(a) (a[ARRAY_SIZE(a) - 1].id + 1)
>  
> diff --git a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c
> index 2e4095c4c12c94f9..ebaa59e934178309 100644
> --- a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c
> +++ b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2023 Intel Corporation */
> +#include <linux/bitfield.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/kernel.h>
>  #include <linux/string_helpers.h>
> @@ -11,13 +12,6 @@
>  #include "adf_gen4_pm.h"
>  #include "icp_qat_fw_init_admin.h"
>  
> -/*
> - * This is needed because a variable is used to index the mask at
> - * pm_scnprint_table(), making it not compile time constant, so the compile
> - * asserts from FIELD_GET() or u32_get_bits() won't be fulfilled.
> - */
> -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
> -
>  #define PM_INFO_MEMBER_OFF(member)	\
>  	(offsetof(struct icp_qat_fw_init_admin_pm_info, member) / sizeof(u32))
>  
> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> index 40c1bd80f8b0434d..b45e4dd8d8e4f00a 100644
> --- a/drivers/gpio/gpio-aspeed.c
> +++ b/drivers/gpio/gpio-aspeed.c
> @@ -5,6 +5,7 @@
>   * Joel Stanley <joel@jms.id.au>
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/gpio/aspeed.h>
>  #include <linux/gpio/driver.h>
> @@ -30,10 +31,6 @@
>  #include <linux/gpio/consumer.h>
>  #include "gpiolib.h"
>  
> -/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */
> -#define field_get(_mask, _reg)	(((_reg) & (_mask)) >> (ffs(_mask) - 1))
> -#define field_prep(_mask, _val)	(((_val) << (ffs(_mask) - 1)) & (_mask))
> -
>  #define GPIO_G7_IRQ_STS_BASE 0x100
>  #define GPIO_G7_IRQ_STS_OFFSET(x) (GPIO_G7_IRQ_STS_BASE + (x) * 0x4)
>  #define GPIO_G7_CTRL_REG_BASE 0x180
> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
> index 740018d4b3dfb35e..c58dc59d4f570831 100644
> --- a/drivers/iio/temperature/mlx90614.c
> +++ b/drivers/iio/temperature/mlx90614.c
> @@ -22,6 +22,7 @@
>   * the "wakeup" GPIO is not given, power management will be disabled.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/gpio/consumer.h>
> @@ -68,10 +69,6 @@
>  #define MLX90614_CONST_SCALE 20 /* Scale in milliKelvin (0.02 * 1000) */
>  #define MLX90614_CONST_FIR 0x7 /* Fixed value for FIR part of low pass filter */
>  
> -/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */
> -#define field_get(_mask, _reg)	(((_reg) & (_mask)) >> (ffs(_mask) - 1))
> -#define field_prep(_mask, _val)	(((_val) << (ffs(_mask) - 1)) & (_mask))
> -
>  struct mlx_chip_info {
>  	/* EEPROM offsets with 16-bit data, MSB first */
>  	/* emissivity correction coefficient */
> diff --git a/drivers/pinctrl/nuvoton/pinctrl-ma35.c b/drivers/pinctrl/nuvoton/pinctrl-ma35.c
> index 59c4e7c6cddea127..3ba28faa8e1418a9 100644
> --- a/drivers/pinctrl/nuvoton/pinctrl-ma35.c
> +++ b/drivers/pinctrl/nuvoton/pinctrl-ma35.c
> @@ -81,10 +81,6 @@
>  #define MVOLT_1800			0
>  #define MVOLT_3300			1
>  
> -/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */
> -#define field_get(_mask, _reg)	(((_reg) & (_mask)) >> (ffs(_mask) - 1))
> -#define field_prep(_mask, _val)	(((_val) << (ffs(_mask) - 1)) & (_mask))
> -
>  static const char * const gpio_group_name[] = {
>  	"gpioa", "gpiob", "gpioc", "gpiod", "gpioe", "gpiof", "gpiog",
>  	"gpioh", "gpioi", "gpioj", "gpiok", "gpiol", "gpiom", "gpion",
> diff --git a/drivers/soc/renesas/rz-sysc.c b/drivers/soc/renesas/rz-sysc.c
> index 1c98da37b7d18745..917a029d849585cd 100644
> --- a/drivers/soc/renesas/rz-sysc.c
> +++ b/drivers/soc/renesas/rz-sysc.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2024 Renesas Electronics Corp.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> @@ -12,8 +13,6 @@
>  
>  #include "rz-sysc.h"
>  
> -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
> -
>  /**
>   * struct rz_sysc - RZ SYSC private data structure
>   * @base: SYSC base address
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 63928f1732230700..c62324a9fcc81241 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -203,4 +203,38 @@ __MAKE_OP(64)
>  #undef __MAKE_OP
>  #undef ____MAKE_OP
>  
> +/**
> + * field_prep() - prepare a bitfield element
> + * @_mask: shifted mask defining the field's length and position
> + * @_val:  value to put in the field
> + *
> + * field_prep() masks and shifts up the value.  The result should be
> + * combined with other fields of the bitfield using logical OR.
> + * Unlike FIELD_PREP(), @_mask is not limited to a compile-time constant.
> + */
> +#define field_prep(_mask, _val)						\
> +	({								\
> +		typeof(_mask) __mask = (_mask);				\
> +		unsigned int __shift = sizeof(_mask) <= 4 ?		\
> +				       __ffs(__mask) : __ffs64(__mask);	\
> +		(((typeof(_mask))(_val) << __shift) & (__mask));	\
> +	})
> +
> +/**
> + * field_get() - extract a bitfield element
> + * @_mask: shifted mask defining the field's length and position
> + * @_reg:  value of entire bitfield
> + *
> + * field_get() extracts the field specified by @_mask from the
> + * bitfield passed in as @_reg by masking and shifting it down.
> + * Unlike FIELD_GET(), @_mask is not limited to a compile-time constant.
> + */
> +#define field_get(_mask, _reg)						\
> +	({								\
> +		typeof(_mask) __mask = _mask;				\
> +		unsigned int __shift = sizeof(_mask) <= 4 ?		\
> +				       __ffs(__mask) : __ffs64(__mask);	\
> +		(typeof(_mask))(((_reg) & (__mask)) >> __shift);	\
> +	})
> +
>  #endif
> diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
> index 23fcd680167d0298..00ab811e4b11a573 100644
> --- a/sound/usb/mixer_quirks.c
> +++ b/sound/usb/mixer_quirks.c
> @@ -3109,10 +3109,6 @@ static int snd_bbfpro_controls_create(struct usb_mixer_interface *mixer)
>  #define RME_DIGIFACE_REGISTER(reg, mask) (((reg) << 16) | (mask))
>  #define RME_DIGIFACE_INVERT BIT(31)
>  
> -/* Nonconst helpers */
> -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
> -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask))
> -
>  static int snd_rme_digiface_write_reg(struct snd_kcontrol *kcontrol, int item, u16 mask, u16 val)
>  {
>  	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kcontrol);
> -- 
> 2.43.0
> 

-- 
Alexandre Belloni, co-owner and COO, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Jonathan Cameron]

On Fri, 31 Jan 2025 14:46:51 +0100
Geert Uytterhoeven <geert+renesas@glider.be> wrote:

> The existing FIELD_{GET,PREP}() macros are limited to compile-time
> constants.  However, it is very common to prepare or extract bitfield
> elements where the bitfield mask is not a compile-time constant.
> 
> To avoid this limitation, the AT91 clock driver and several other
> drivers already have their own non-const field_{prep,get}() macros.
> Make them available for general use by consolidating them in
> <linux/bitfield.h>, and improve them slightly:
>   1. Avoid evaluating macro parameters more than once,
>   2. Replace "ffs() - 1" by "__ffs()",
>   3. Support 64-bit use on 32-bit architectures.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>
For the IIO one.

Acked-by: Jonathan Cameron <Jonathan.Cameron@huawei.com>

> ---
> v2:
>   - Cast val resp. reg to the mask type,
>   - Fix 64-bit use on 32-bit architectures,
>   - Convert new upstream users:
>       - drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c
>       - drivers/gpio/gpio-aspeed.c
>       - drivers/iio/temperature/mlx90614.c
>       - drivers/pinctrl/nuvoton/pinctrl-ma35.c
>       - sound/usb/mixer_quirks.c
>   - Convert new user queued in renesas-devel for v6.15:
>       - drivers/soc/renesas/rz-sysc.c
> ---
>  drivers/clk/at91/clk-peripheral.c             |  1 +
>  drivers/clk/at91/pmc.h                        |  3 --
>  .../qat/qat_common/adf_gen4_pm_debugfs.c      |  8 +----
>  drivers/gpio/gpio-aspeed.c                    |  5 +--
>  drivers/iio/temperature/mlx90614.c            |  5 +--
>  drivers/pinctrl/nuvoton/pinctrl-ma35.c        |  4 ---
>  drivers/soc/renesas/rz-sysc.c                 |  3 +-
>  include/linux/bitfield.h                      | 34 +++++++++++++++++++
>  sound/usb/mixer_quirks.c                      |  4 ---
>  9 files changed, 39 insertions(+), 28 deletions(-)
> 
> diff --git a/drivers/clk/at91/clk-peripheral.c b/drivers/clk/at91/clk-peripheral.c
> index c173a44c800aa8cc..60208bdc3fe4797e 100644
> --- a/drivers/clk/at91/clk-peripheral.c
> +++ b/drivers/clk/at91/clk-peripheral.c
> @@ -3,6 +3,7 @@
>   *  Copyright (C) 2013 Boris BREZILLON <b.brezillon@overkiz.com>
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/bitops.h>
>  #include <linux/clk-provider.h>
>  #include <linux/clkdev.h>
> diff --git a/drivers/clk/at91/pmc.h b/drivers/clk/at91/pmc.h
> index 4fb29ca111f7d427..3838e4f7df2d4a70 100644
> --- a/drivers/clk/at91/pmc.h
> +++ b/drivers/clk/at91/pmc.h
> @@ -116,9 +116,6 @@ struct at91_clk_pms {
>  	unsigned int parent;
>  };
>  
> -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
> -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask))
> -
>  #define ndck(a, s) (a[s - 1].id + 1)
>  #define nck(a) (a[ARRAY_SIZE(a) - 1].id + 1)
>  
> diff --git a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c
> index 2e4095c4c12c94f9..ebaa59e934178309 100644
> --- a/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c
> +++ b/drivers/crypto/intel/qat/qat_common/adf_gen4_pm_debugfs.c
> @@ -1,5 +1,6 @@
>  // SPDX-License-Identifier: GPL-2.0-only
>  /* Copyright(c) 2023 Intel Corporation */
> +#include <linux/bitfield.h>
>  #include <linux/dma-mapping.h>
>  #include <linux/kernel.h>
>  #include <linux/string_helpers.h>
> @@ -11,13 +12,6 @@
>  #include "adf_gen4_pm.h"
>  #include "icp_qat_fw_init_admin.h"
>  
> -/*
> - * This is needed because a variable is used to index the mask at
> - * pm_scnprint_table(), making it not compile time constant, so the compile
> - * asserts from FIELD_GET() or u32_get_bits() won't be fulfilled.
> - */
> -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
> -
>  #define PM_INFO_MEMBER_OFF(member)	\
>  	(offsetof(struct icp_qat_fw_init_admin_pm_info, member) / sizeof(u32))
>  
> diff --git a/drivers/gpio/gpio-aspeed.c b/drivers/gpio/gpio-aspeed.c
> index 40c1bd80f8b0434d..b45e4dd8d8e4f00a 100644
> --- a/drivers/gpio/gpio-aspeed.c
> +++ b/drivers/gpio/gpio-aspeed.c
> @@ -5,6 +5,7 @@
>   * Joel Stanley <joel@jms.id.au>
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/clk.h>
>  #include <linux/gpio/aspeed.h>
>  #include <linux/gpio/driver.h>
> @@ -30,10 +31,6 @@
>  #include <linux/gpio/consumer.h>
>  #include "gpiolib.h"
>  
> -/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */
> -#define field_get(_mask, _reg)	(((_reg) & (_mask)) >> (ffs(_mask) - 1))
> -#define field_prep(_mask, _val)	(((_val) << (ffs(_mask) - 1)) & (_mask))
> -
>  #define GPIO_G7_IRQ_STS_BASE 0x100
>  #define GPIO_G7_IRQ_STS_OFFSET(x) (GPIO_G7_IRQ_STS_BASE + (x) * 0x4)
>  #define GPIO_G7_CTRL_REG_BASE 0x180
> diff --git a/drivers/iio/temperature/mlx90614.c b/drivers/iio/temperature/mlx90614.c
> index 740018d4b3dfb35e..c58dc59d4f570831 100644
> --- a/drivers/iio/temperature/mlx90614.c
> +++ b/drivers/iio/temperature/mlx90614.c
> @@ -22,6 +22,7 @@
>   * the "wakeup" GPIO is not given, power management will be disabled.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/delay.h>
>  #include <linux/err.h>
>  #include <linux/gpio/consumer.h>
> @@ -68,10 +69,6 @@
>  #define MLX90614_CONST_SCALE 20 /* Scale in milliKelvin (0.02 * 1000) */
>  #define MLX90614_CONST_FIR 0x7 /* Fixed value for FIR part of low pass filter */
>  
> -/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */
> -#define field_get(_mask, _reg)	(((_reg) & (_mask)) >> (ffs(_mask) - 1))
> -#define field_prep(_mask, _val)	(((_val) << (ffs(_mask) - 1)) & (_mask))
> -
>  struct mlx_chip_info {
>  	/* EEPROM offsets with 16-bit data, MSB first */
>  	/* emissivity correction coefficient */
> diff --git a/drivers/pinctrl/nuvoton/pinctrl-ma35.c b/drivers/pinctrl/nuvoton/pinctrl-ma35.c
> index 59c4e7c6cddea127..3ba28faa8e1418a9 100644
> --- a/drivers/pinctrl/nuvoton/pinctrl-ma35.c
> +++ b/drivers/pinctrl/nuvoton/pinctrl-ma35.c
> @@ -81,10 +81,6 @@
>  #define MVOLT_1800			0
>  #define MVOLT_3300			1
>  
> -/* Non-constant mask variant of FIELD_GET() and FIELD_PREP() */
> -#define field_get(_mask, _reg)	(((_reg) & (_mask)) >> (ffs(_mask) - 1))
> -#define field_prep(_mask, _val)	(((_val) << (ffs(_mask) - 1)) & (_mask))
> -
>  static const char * const gpio_group_name[] = {
>  	"gpioa", "gpiob", "gpioc", "gpiod", "gpioe", "gpiof", "gpiog",
>  	"gpioh", "gpioi", "gpioj", "gpiok", "gpiol", "gpiom", "gpion",
> diff --git a/drivers/soc/renesas/rz-sysc.c b/drivers/soc/renesas/rz-sysc.c
> index 1c98da37b7d18745..917a029d849585cd 100644
> --- a/drivers/soc/renesas/rz-sysc.c
> +++ b/drivers/soc/renesas/rz-sysc.c
> @@ -5,6 +5,7 @@
>   * Copyright (C) 2024 Renesas Electronics Corp.
>   */
>  
> +#include <linux/bitfield.h>
>  #include <linux/io.h>
>  #include <linux/of.h>
>  #include <linux/platform_device.h>
> @@ -12,8 +13,6 @@
>  
>  #include "rz-sysc.h"
>  
> -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
> -
>  /**
>   * struct rz_sysc - RZ SYSC private data structure
>   * @base: SYSC base address
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 63928f1732230700..c62324a9fcc81241 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -203,4 +203,38 @@ __MAKE_OP(64)
>  #undef __MAKE_OP
>  #undef ____MAKE_OP
>  
> +/**
> + * field_prep() - prepare a bitfield element
> + * @_mask: shifted mask defining the field's length and position
> + * @_val:  value to put in the field
> + *
> + * field_prep() masks and shifts up the value.  The result should be
> + * combined with other fields of the bitfield using logical OR.
> + * Unlike FIELD_PREP(), @_mask is not limited to a compile-time constant.
> + */
> +#define field_prep(_mask, _val)						\
> +	({								\
> +		typeof(_mask) __mask = (_mask);				\
> +		unsigned int __shift = sizeof(_mask) <= 4 ?		\
> +				       __ffs(__mask) : __ffs64(__mask);	\
> +		(((typeof(_mask))(_val) << __shift) & (__mask));	\
> +	})
> +
> +/**
> + * field_get() - extract a bitfield element
> + * @_mask: shifted mask defining the field's length and position
> + * @_reg:  value of entire bitfield
> + *
> + * field_get() extracts the field specified by @_mask from the
> + * bitfield passed in as @_reg by masking and shifting it down.
> + * Unlike FIELD_GET(), @_mask is not limited to a compile-time constant.
> + */
> +#define field_get(_mask, _reg)						\
> +	({								\
> +		typeof(_mask) __mask = _mask;				\
> +		unsigned int __shift = sizeof(_mask) <= 4 ?		\
> +				       __ffs(__mask) : __ffs64(__mask);	\
> +		(typeof(_mask))(((_reg) & (__mask)) >> __shift);	\
> +	})
> +
>  #endif
> diff --git a/sound/usb/mixer_quirks.c b/sound/usb/mixer_quirks.c
> index 23fcd680167d0298..00ab811e4b11a573 100644
> --- a/sound/usb/mixer_quirks.c
> +++ b/sound/usb/mixer_quirks.c
> @@ -3109,10 +3109,6 @@ static int snd_bbfpro_controls_create(struct usb_mixer_interface *mixer)
>  #define RME_DIGIFACE_REGISTER(reg, mask) (((reg) << 16) | (mask))
>  #define RME_DIGIFACE_INVERT BIT(31)
>  
> -/* Nonconst helpers */
> -#define field_get(_mask, _reg) (((_reg) & (_mask)) >> (ffs(_mask) - 1))
> -#define field_prep(_mask, _val) (((_val) << (ffs(_mask) - 1)) & (_mask))
> -
>  static int snd_rme_digiface_write_reg(struct snd_kcontrol *kcontrol, int item, u16 mask, u16 val)
>  {
>  	struct usb_mixer_elem_list *list = snd_kcontrol_chip(kcontrol);

Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[David Laight]

On Fri, 31 Jan 2025 14:46:51 +0100
Geert Uytterhoeven <geert+renesas@glider.be> wrote:

> The existing FIELD_{GET,PREP}() macros are limited to compile-time
> constants.  However, it is very common to prepare or extract bitfield
> elements where the bitfield mask is not a compile-time constant.
> 
> To avoid this limitation, the AT91 clock driver and several other
> drivers already have their own non-const field_{prep,get}() macros.
> Make them available for general use by consolidating them in
> <linux/bitfield.h>, and improve them slightly:
>   1. Avoid evaluating macro parameters more than once,
>   2. Replace "ffs() - 1" by "__ffs()",
>   3. Support 64-bit use on 32-bit architectures.
...
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 63928f1732230700..c62324a9fcc81241 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -203,4 +203,38 @@ __MAKE_OP(64)
>  #undef __MAKE_OP
>  #undef ____MAKE_OP
>  
> +/**
> + * field_prep() - prepare a bitfield element
> + * @_mask: shifted mask defining the field's length and position
> + * @_val:  value to put in the field
> + *
> + * field_prep() masks and shifts up the value.  The result should be
> + * combined with other fields of the bitfield using logical OR.
> + * Unlike FIELD_PREP(), @_mask is not limited to a compile-time constant.
> + */
> +#define field_prep(_mask, _val)						\

You don't need an _ prefix on the 'parameters' - it doesn't gain anything.

> +	({								\
> +		typeof(_mask) __mask = (_mask);				\

Use: __auto_type __mask = (_mask);

> +		unsigned int __shift = sizeof(_mask) <= 4 ?		\
> +				       __ffs(__mask) : __ffs64(__mask);	\
> +		(((typeof(_mask))(_val) << __shift) & (__mask));	\

There are a lot of () in that line, perhaps:

		__auto_type(__mask) = (_mask);
		typeof (__mask) __val = (_val);
		unsigned int __shift = ...;

		(__val << __shift) & __mask;

Note the typeof (__mask) - avoids line-length 'bloat' when the arguments are non-trivial.

	David

Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Vincent Mailhol]

On 31/01/2025 at 22:46, Geert Uytterhoeven wrote:
> The existing FIELD_{GET,PREP}() macros are limited to compile-time
> constants.  However, it is very common to prepare or extract bitfield
> elements where the bitfield mask is not a compile-time constant.

Why is it that the existing FIELD_{GET,PREP}() macros must be limited to
compile time constants? Instead of creating another variant for
non-constant bitfields, wouldn't it be better to make the existing macro
accept both?

As far as I can see, only __BUILD_BUG_ON_NOT_POWER_OF_2()  and
__BF_FIELD_CHECK() need to be adjusted. I am thinking of this:

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
index 63928f173223..c6bedab862d1 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/compiler.h>
 #include <asm/byteorder.h>

 /*
@@ -62,15 +63,13 @@

 #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx)                      \
        ({                                                              \
-               BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),          \
-                                _pfx "mask is not constant");          \
-               BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero");    \
-               BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?           \
-                                ~((_mask) >> __bf_shf(_mask)) &        \
-                                       (0 + (_val)) : 0,               \
+               BUILD_BUG_ON_MSG(statically_true((_mask) == 0),         \
+                                _pfx "mask is zero");                  \
+               BUILD_BUG_ON_MSG(statically_true(~((_mask) >>
__bf_shf(_mask)) & \
+                                                (0 + (_val))),         \
                                 _pfx "value too large for the field"); \
-               BUILD_BUG_ON_MSG(__bf_cast_unsigned(_mask, _mask) >     \
-                                __bf_cast_unsigned(_reg, ~0ull),       \
+
BUILD_BUG_ON_MSG(statically_true(__bf_cast_unsigned(_mask, _mask) > \
+
__bf_cast_unsigned(_reg, ~0ull)), \
                                 _pfx "type of reg too small for mask"); \
                __BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +                 \
                                              (1ULL << __bf_shf(_mask))); \
diff --git a/include/linux/build_bug.h b/include/linux/build_bug.h
index 3aa3640f8c18..3b8055ebb55f 100644
--- a/include/linux/build_bug.h
+++ b/include/linux/build_bug.h
@@ -18,9 +18,9 @@

 /* Force a compilation error if a constant expression is not a power of
2 */
 #define __BUILD_BUG_ON_NOT_POWER_OF_2(n)       \
-       BUILD_BUG_ON(((n) & ((n) - 1)) != 0)
+       BUILD_BUG_ON(statically_true((n) & ((n) - 1)))
 #define BUILD_BUG_ON_NOT_POWER_OF_2(n)                 \
-       BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
+       BUILD_BUG_ON(statically_true(!(n) || ((n) & ((n) - 1))))

 /*
  * BUILD_BUG_ON_INVALID() permits the compiler to check the validity of the


> To avoid this limitation, the AT91 clock driver and several other
> drivers already have their own non-const field_{prep,get}() macros.
> Make them available for general use by consolidating them in
> <linux/bitfield.h>, and improve them slightly:
>   1. Avoid evaluating macro parameters more than once,
>   2. Replace "ffs() - 1" by "__ffs()",
>   3. Support 64-bit use on 32-bit architectures.
> 
> Signed-off-by: Geert Uytterhoeven <geert+renesas@glider.be>

(...)


Yours sincerely,
Vincent Mailhol

Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Yury Norov]

On Sun, Feb 02, 2025 at 05:26:04PM +0900, Vincent Mailhol wrote:
> On 31/01/2025 at 22:46, Geert Uytterhoeven wrote:
> > The existing FIELD_{GET,PREP}() macros are limited to compile-time
> > constants.  However, it is very common to prepare or extract bitfield
> > elements where the bitfield mask is not a compile-time constant.
> 
> Why is it that the existing FIELD_{GET,PREP}() macros must be limited to
> compile time constants?

I guess, for historical reasons?

> Instead of creating another variant for
> non-constant bitfields, wouldn't it be better to make the existing macro
> accept both?

Yes, it would definitely be better IMO.

> As far as I can see, only __BUILD_BUG_ON_NOT_POWER_OF_2()  and
> __BF_FIELD_CHECK() need to be adjusted. I am thinking of this:
> 
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 63928f173223..c6bedab862d1 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/compiler.h>
>  #include <asm/byteorder.h>
> 
>  /*
> @@ -62,15 +63,13 @@
> 
>  #define __BF_FIELD_CHECK(_mask, _reg, _val, _pfx)                      \
>         ({                                                              \
> -               BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask),          \
> -                                _pfx "mask is not constant");          \
> -               BUILD_BUG_ON_MSG((_mask) == 0, _pfx "mask is zero");    \
> -               BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?           \
> -                                ~((_mask) >> __bf_shf(_mask)) &        \
> -                                       (0 + (_val)) : 0,               \
> +               BUILD_BUG_ON_MSG(statically_true((_mask) == 0),         \
> +                                _pfx "mask is zero");                  \
> +               BUILD_BUG_ON_MSG(statically_true(~((_mask) >>

This should be a const_true(), because statically_true() may be OK
with something like:
        ((runtime_var << 1) & 1 == 0)

I think it's your own patch that adds const_true(): 4f3d1be4c2f8a :)

Thanks,
Yury

Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Johannes Berg]

On Sun, 2025-02-02 at 12:53 -0500, Yury Norov wrote:
> 
> > Instead of creating another variant for
> > non-constant bitfields, wouldn't it be better to make the existing macro
> > accept both?
> 
> Yes, it would definitely be better IMO.

On the flip side, there have been discussions in the past (though I
think not all, if any, on the list(s)) about the argument order. Since
the value is typically not a constant, requiring the mask to be a
constant has ensured that the argument order isn't as easily mixed up as
otherwise.

With a non-constant mask there can also be no validation that the mask
is contiguous etc.

Now that doesn't imply a strong objection - personally I've come to
prefer the lower-case typed versions anyway - but something to keep in
mind when doing this.

However, the suggested change to BUILD_BUG_ON_NOT_POWER_OF_2 almost
certainly shouldn't be done for the same reason - not compiling for non-
constant values is [IMHO] part of the API contract for that macro. This
can be important for the same reasons.

(Obviously, doing that change now doesn't invalidate existing code, but
it does remove checks that may have been intended to be present in the
code.)

johannes

Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Vincent Mailhol]

On 03/02/2025 at 16:44, Johannes Berg wrote:
> On Sun, 2025-02-02 at 12:53 -0500, Yury Norov wrote:
>>
>>> Instead of creating another variant for
>>> non-constant bitfields, wouldn't it be better to make the existing macro
>>> accept both?
>>
>> Yes, it would definitely be better IMO.
> 
> On the flip side, there have been discussions in the past (though I
> think not all, if any, on the list(s)) about the argument order. Since
> the value is typically not a constant, requiring the mask to be a
> constant has ensured that the argument order isn't as easily mixed up as
> otherwise.

If this is a concern, then it can be checked with:

  BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask) &&
                   __builtin_constant_p(_val),
                   _pfx "mask is not constant");

It means that we forbid FIELD_PREP(non_const_mask, const_val) but allow
any other combination.

> With a non-constant mask there can also be no validation that the mask
> is contiguous etc.
> 
> Now that doesn't imply a strong objection - personally I've come to
> prefer the lower-case typed versions anyway - but something to keep in
> mind when doing this.
> 
> However, the suggested change to BUILD_BUG_ON_NOT_POWER_OF_2 almost
> certainly shouldn't be done for the same reason - not compiling for non-
> constant values is [IMHO] part of the API contract for that macro. This
> can be important for the same reasons.

Your point is fair enough. But I do not see this as a killer argument.
We can instead just add below helper:

  BUILD_BUG_ON_STATICALLY_NOT_POWER_OF_2()

But, for the same reason why I would rather not have both the
FIELD_{PREP,GET}() and the field_{prep,get}(), I would also rather not
have a BUILD_BUG_ON_NOT_POWER_OF_2() and a
BUILD_BUG_ON_STATICALLY_NOT_POWER_OF_2().

If your concern is the wording of the contract, the description can just
be updated.


Yours sincerely,
Vincent Mailhol

Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Geert Uytterhoeven]

Hi Vincent,

On Mon, 3 Feb 2025 at 14:37, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
> On 03/02/2025 at 16:44, Johannes Berg wrote:
> > On Sun, 2025-02-02 at 12:53 -0500, Yury Norov wrote:
> >>> Instead of creating another variant for
> >>> non-constant bitfields, wouldn't it be better to make the existing macro
> >>> accept both?
> >>
> >> Yes, it would definitely be better IMO.
> >
> > On the flip side, there have been discussions in the past (though I
> > think not all, if any, on the list(s)) about the argument order. Since
> > the value is typically not a constant, requiring the mask to be a
> > constant has ensured that the argument order isn't as easily mixed up as
> > otherwise.
>
> If this is a concern, then it can be checked with:
>
>   BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask) &&
>                    __builtin_constant_p(_val),
>                    _pfx "mask is not constant");
>
> It means that we forbid FIELD_PREP(non_const_mask, const_val) but allow
> any other combination.

Even that case looks valid to me. Actually there is already such a user
in drivers/iio/temperature/mlx90614.c:

    ret |= field_prep(chip_info->fir_config_mask, MLX90614_CONST_FIR);

So if you want enhanced safety, having both the safer/const upper-case
variants and the less-safe/non-const lower-case variants makes sense.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Johannes Berg]

On Mon, 2025-02-03 at 22:36 +0900, Vincent Mailhol wrote:
> > On the flip side, there have been discussions in the past (though I
> > think not all, if any, on the list(s)) about the argument order. Since
> > the value is typically not a constant, requiring the mask to be a
> > constant has ensured that the argument order isn't as easily mixed up as
> > otherwise.
> 
> If this is a concern, then it can be checked with:
> 
>   BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask) &&
>                    __builtin_constant_p(_val),
>                    _pfx "mask is not constant");
> 
> It means that we forbid FIELD_PREP(non_const_mask, const_val) but allow
> any other combination.

There almost certainly will be users who want both to be non-constant
though, and anyway I don't understand how that helps - if you want to
write the value 0x7 to the (variable) mask 0xF then this won't catch
anything?

> > However, the suggested change to BUILD_BUG_ON_NOT_POWER_OF_2 almost
> > certainly shouldn't be done for the same reason - not compiling for non-
> > constant values is [IMHO] part of the API contract for that macro. This
> > can be important for the same reasons.
> 
> Your point is fair enough. But I do not see this as a killer argument.
> We can instead just add below helper:
> 
>   BUILD_BUG_ON_STATICALLY_NOT_POWER_OF_2()
> 
> But, for the same reason why I would rather not have both the
> FIELD_{PREP,GET}() and the field_{prep,get}(), I would also rather not
> have a BUILD_BUG_ON_NOT_POWER_OF_2() and a
> BUILD_BUG_ON_STATICALLY_NOT_POWER_OF_2().
> 
> If your concern is the wording of the contract, the description can just
> be updated.

No, I just think in both cases it's really bad form to silently update
the contract removing negative assertions that other people may have
been relying on. Not because these trigger today, of course, but because
they may not have added additional checks, or similar.

So arguably then you should have BUILD_BUG_ON_CONST_NOT_POWER_OF_2() or
so instead, so that all existing users are unaffected by the updates,
and similarly that's an argument for leaving FIELD_* versions intact. Or
I guess one could change all existing users to new ones accordingly, say
FIELD_*_CONST_MASK(), but that's pretty annoying too.

johannes

Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Vincent Mailhol]

On 03/02/2025 at 22:59, Geert Uytterhoeven wrote:
> Hi Vincent,
> 
> On Mon, 3 Feb 2025 at 14:37, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
>> On 03/02/2025 at 16:44, Johannes Berg wrote:
>>> On Sun, 2025-02-02 at 12:53 -0500, Yury Norov wrote:
>>>>> Instead of creating another variant for
>>>>> non-constant bitfields, wouldn't it be better to make the existing macro
>>>>> accept both?
>>>>
>>>> Yes, it would definitely be better IMO.
>>>
>>> On the flip side, there have been discussions in the past (though I
>>> think not all, if any, on the list(s)) about the argument order. Since
>>> the value is typically not a constant, requiring the mask to be a
>>> constant has ensured that the argument order isn't as easily mixed up as
>>> otherwise.
>>
>> If this is a concern, then it can be checked with:
>>
>>   BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask) &&
>>                    __builtin_constant_p(_val),
>>                    _pfx "mask is not constant");
>>
>> It means that we forbid FIELD_PREP(non_const_mask, const_val) but allow
>> any other combination.
> 
> Even that case looks valid to me. Actually there is already such a user
> in drivers/iio/temperature/mlx90614.c:
> 
>     ret |= field_prep(chip_info->fir_config_mask, MLX90614_CONST_FIR);
> 
> So if you want enhanced safety, having both the safer/const upper-case
> variants and the less-safe/non-const lower-case variants makes sense.

So, we are scared of people calling FIELD_PREP() with the arguments in
the wrong order:

  FIELD_PREP(val, mask)

thus adding the check that mask must be a compile time constant.

But if we introduce a second function, don't we introduce the risk of
having people use the lower case variant instead of the upper case variant?

  field_prep(incorrect_const_mask, val)

I am not sure to follow the logic of why having two functions is the
safer choice. Whatever the solution you propose, there will be a way to
misuse it. Let me ask, what is the most likely to happen:

  1. wrong parameter order
  2. wrong function name

?

If you have the conviction that people more often do mistake 1. then I
am fine with your solution. Otherwise, if 1. and 2. have an equally
likelihood, then I would argue to go with the simplicity of the single
function.


Yours sincerely,
Vincent Mailhol

Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Yury Norov]

On Tue, Feb 04, 2025 at 12:41:55AM +0900, Vincent Mailhol wrote:
> On 03/02/2025 at 22:59, Geert Uytterhoeven wrote:
> > Hi Vincent,
> > 
> > On Mon, 3 Feb 2025 at 14:37, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
> >> On 03/02/2025 at 16:44, Johannes Berg wrote:
> >>> On Sun, 2025-02-02 at 12:53 -0500, Yury Norov wrote:
> >>>>> Instead of creating another variant for
> >>>>> non-constant bitfields, wouldn't it be better to make the existing macro
> >>>>> accept both?
> >>>>
> >>>> Yes, it would definitely be better IMO.
> >>>
> >>> On the flip side, there have been discussions in the past (though I
> >>> think not all, if any, on the list(s)) about the argument order. Since
> >>> the value is typically not a constant, requiring the mask to be a
> >>> constant has ensured that the argument order isn't as easily mixed up as
> >>> otherwise.
> >>
> >> If this is a concern, then it can be checked with:
> >>
> >>   BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask) &&
> >>                    __builtin_constant_p(_val),
> >>                    _pfx "mask is not constant");
> >>
> >> It means that we forbid FIELD_PREP(non_const_mask, const_val) but allow
> >> any other combination.
> > 
> > Even that case looks valid to me. Actually there is already such a user
> > in drivers/iio/temperature/mlx90614.c:
> > 
> >     ret |= field_prep(chip_info->fir_config_mask, MLX90614_CONST_FIR);
> > 
> > So if you want enhanced safety, having both the safer/const upper-case
> > variants and the less-safe/non-const lower-case variants makes sense.

I agree with that. I just don't want the same shift-and operation to be
opencoded again and again.

What I actually meant is that I'm OK with whatever number of field_prep()
macro flavors, if we make sure that they don't duplicate each other. So
for me, something like this would be the best solution:

 #define field_prep(mask, val) \
       (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))

 #define FIELD_PREP(mask, val)                                         \
         (                                                             \
                 FIELD_PREP_INPUT_CHECK(_mask, _val,);                 \
                 field_prep(mask, val);                                \
         )
 
#define FIELD_PREP_CONST(_mask, _val)                                  \
        (                                                              \
                FIELD_PREP_CONST_INPUT_CHECK(mask, val);
                FIELD_PREP(mask, val); // or field_prep()
        )

We have a similar macro GENMASK() in linux/bits.h. It is implemented
like this:

 #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
 #define GENMASK(h, l) \
         (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))

And it works just well. Can we end up with a similar approach here?

> So, we are scared of people calling FIELD_PREP() with the arguments in
> the wrong order:
>
>   FIELD_PREP(val, mask)
> 
> thus adding the check that mask must be a compile time constant.

Don't be scared. Kernel coding implies that people get used to read
function declarations and comments on top of them before using
something.

Thansk,
Yury

Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Jakub Kicinski]

On Sun, 2 Feb 2025 17:26:04 +0900 Vincent Mailhol wrote:
> On 31/01/2025 at 22:46, Geert Uytterhoeven wrote:
> > The existing FIELD_{GET,PREP}() macros are limited to compile-time
> > constants.  However, it is very common to prepare or extract bitfield
> > elements where the bitfield mask is not a compile-time constant.  
> 
> Why is it that the existing FIELD_{GET,PREP}() macros must be limited to
> compile time constants?

Hard no, some high performance networking drivers use this on 
the fastpath. We want to make sure that the compiler doesn't
do anything stupid, and decomposes the masks at build time.

The macros work just fine for a *lot* of code:

$ git grep -E 'FIELD_(PREP|GET)\(' | wc -l
22407

BTW aren't u32_get_bits(), u32_replace_bits() etc. not what 
you need in the first place? I think people don't know about
those, with all due respect the way they are coded up looks 
like an IOCCC submission..

Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Geert Uytterhoeven]

Hi David,

On Fri, 31 Jan 2025 at 20:03, David Laight <david.laight.linux@gmail.com> wrote:
> On Fri, 31 Jan 2025 14:46:51 +0100
> Geert Uytterhoeven <geert+renesas@glider.be> wrote:
> > The existing FIELD_{GET,PREP}() macros are limited to compile-time
> > constants.  However, it is very common to prepare or extract bitfield
> > elements where the bitfield mask is not a compile-time constant.
> >
> > To avoid this limitation, the AT91 clock driver and several other
> > drivers already have their own non-const field_{prep,get}() macros.
> > Make them available for general use by consolidating them in
> > <linux/bitfield.h>, and improve them slightly:
> >   1. Avoid evaluating macro parameters more than once,
> >   2. Replace "ffs() - 1" by "__ffs()",
> >   3. Support 64-bit use on 32-bit architectures.
> ...
> > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> > index 63928f1732230700..c62324a9fcc81241 100644
> > --- a/include/linux/bitfield.h
> > +++ b/include/linux/bitfield.h
> > @@ -203,4 +203,38 @@ __MAKE_OP(64)
> >  #undef __MAKE_OP
> >  #undef ____MAKE_OP
> >
> > +/**
> > + * field_prep() - prepare a bitfield element
> > + * @_mask: shifted mask defining the field's length and position
> > + * @_val:  value to put in the field
> > + *
> > + * field_prep() masks and shifts up the value.  The result should be
> > + * combined with other fields of the bitfield using logical OR.
> > + * Unlike FIELD_PREP(), @_mask is not limited to a compile-time constant.
> > + */
> > +#define field_prep(_mask, _val)                                              \
>
> You don't need an _ prefix on the 'parameters' - it doesn't gain anything.

I just followed the style of all other macros in this file.
I can add a new patch converting the existing macros, though...

>
> > +     ({                                                              \
> > +             typeof(_mask) __mask = (_mask);                         \
>
> Use: __auto_type __mask = (_mask);

Likewise ;-)

> > +             unsigned int __shift = sizeof(_mask) <= 4 ?             \
> > +                                    __ffs(__mask) : __ffs64(__mask); \
> > +             (((typeof(_mask))(_val) << __shift) & (__mask));        \
>
> There are a lot of () in that line, perhaps:
>
>                 __auto_type(__mask) = (_mask);
>                 typeof (__mask) __val = (_val);
>                 unsigned int __shift = ...;
>
>                 (__val << __shift) & __mask;
>
> Note the typeof (__mask) - avoids line-length 'bloat' when the arguments are non-trivial.

OK, thanks!

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Geert Uytterhoeven]

Hi Jakub,

On Tue, 4 Feb 2025 at 16:30, Jakub Kicinski <kuba@kernel.org> wrote:
> On Sun, 2 Feb 2025 17:26:04 +0900 Vincent Mailhol wrote:
> > On 31/01/2025 at 22:46, Geert Uytterhoeven wrote:
> > > The existing FIELD_{GET,PREP}() macros are limited to compile-time
> > > constants.  However, it is very common to prepare or extract bitfield
> > > elements where the bitfield mask is not a compile-time constant.
> >
> > Why is it that the existing FIELD_{GET,PREP}() macros must be limited to
> > compile time constants?
>
> Hard no, some high performance networking drivers use this on
> the fastpath. We want to make sure that the compiler doesn't
> do anything stupid, and decomposes the masks at build time.
>
> The macros work just fine for a *lot* of code:
>
> $ git grep -E 'FIELD_(PREP|GET)\(' | wc -l
> 22407

Indeed.

> BTW aren't u32_get_bits(), u32_replace_bits() etc. not what
> you need in the first place? I think people don't know about
> those, with all due respect the way they are coded up looks
> like an IOCCC submission..

These support only compile-time constants, too.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Geert Uytterhoeven]

Hu Yury,

On Mon, 3 Feb 2025 at 17:48, Yury Norov <yury.norov@gmail.com> wrote:
> On Tue, Feb 04, 2025 at 12:41:55AM +0900, Vincent Mailhol wrote:
> > On 03/02/2025 at 22:59, Geert Uytterhoeven wrote:
> > > On Mon, 3 Feb 2025 at 14:37, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
> > >> On 03/02/2025 at 16:44, Johannes Berg wrote:
> > >>> On Sun, 2025-02-02 at 12:53 -0500, Yury Norov wrote:
> > >>>>> Instead of creating another variant for
> > >>>>> non-constant bitfields, wouldn't it be better to make the existing macro
> > >>>>> accept both?
> > >>>>
> > >>>> Yes, it would definitely be better IMO.
> > >>>
> > >>> On the flip side, there have been discussions in the past (though I
> > >>> think not all, if any, on the list(s)) about the argument order. Since
> > >>> the value is typically not a constant, requiring the mask to be a
> > >>> constant has ensured that the argument order isn't as easily mixed up as
> > >>> otherwise.
> > >>
> > >> If this is a concern, then it can be checked with:
> > >>
> > >>   BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask) &&
> > >>                    __builtin_constant_p(_val),
> > >>                    _pfx "mask is not constant");
> > >>
> > >> It means that we forbid FIELD_PREP(non_const_mask, const_val) but allow
> > >> any other combination.
> > >
> > > Even that case looks valid to me. Actually there is already such a user
> > > in drivers/iio/temperature/mlx90614.c:
> > >
> > >     ret |= field_prep(chip_info->fir_config_mask, MLX90614_CONST_FIR);
> > >
> > > So if you want enhanced safety, having both the safer/const upper-case
> > > variants and the less-safe/non-const lower-case variants makes sense.
>
> I agree with that. I just don't want the same shift-and operation to be
> opencoded again and again.
>
> What I actually meant is that I'm OK with whatever number of field_prep()
> macro flavors, if we make sure that they don't duplicate each other. So
> for me, something like this would be the best solution:
>
>  #define field_prep(mask, val) \
>        (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))
>
>  #define FIELD_PREP(mask, val)                                         \
>          (                                                             \
>                  FIELD_PREP_INPUT_CHECK(_mask, _val,);                 \
>                  field_prep(mask, val);                                \
>          )
>
> #define FIELD_PREP_CONST(_mask, _val)                                  \
>         (                                                              \
>                 FIELD_PREP_CONST_INPUT_CHECK(mask, val);
>                 FIELD_PREP(mask, val); // or field_prep()
>         )
>
> We have a similar macro GENMASK() in linux/bits.h. It is implemented
> like this:
>
>  #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
>  #define GENMASK(h, l) \
>          (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
>
> And it works just well. Can we end up with a similar approach here?

Note that there already exists a FIELD_PREP_CONST() macro, which is
intended for struct member initialization.

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH treewide v2 1/3] bitfield: Add non-constant field_{prep,get}() helpers

[Yury Norov]

On Fri, Feb 14, 2025 at 12:03:16PM +0100, Geert Uytterhoeven wrote:
> Hu Yury,
> 
> On Mon, 3 Feb 2025 at 17:48, Yury Norov <yury.norov@gmail.com> wrote:
> > On Tue, Feb 04, 2025 at 12:41:55AM +0900, Vincent Mailhol wrote:
> > > On 03/02/2025 at 22:59, Geert Uytterhoeven wrote:
> > > > On Mon, 3 Feb 2025 at 14:37, Vincent Mailhol <mailhol.vincent@wanadoo.fr> wrote:
> > > >> On 03/02/2025 at 16:44, Johannes Berg wrote:
> > > >>> On Sun, 2025-02-02 at 12:53 -0500, Yury Norov wrote:
> > > >>>>> Instead of creating another variant for
> > > >>>>> non-constant bitfields, wouldn't it be better to make the existing macro
> > > >>>>> accept both?
> > > >>>>
> > > >>>> Yes, it would definitely be better IMO.
> > > >>>
> > > >>> On the flip side, there have been discussions in the past (though I
> > > >>> think not all, if any, on the list(s)) about the argument order. Since
> > > >>> the value is typically not a constant, requiring the mask to be a
> > > >>> constant has ensured that the argument order isn't as easily mixed up as
> > > >>> otherwise.
> > > >>
> > > >> If this is a concern, then it can be checked with:
> > > >>
> > > >>   BUILD_BUG_ON_MSG(!__builtin_constant_p(_mask) &&
> > > >>                    __builtin_constant_p(_val),
> > > >>                    _pfx "mask is not constant");
> > > >>
> > > >> It means that we forbid FIELD_PREP(non_const_mask, const_val) but allow
> > > >> any other combination.
> > > >
> > > > Even that case looks valid to me. Actually there is already such a user
> > > > in drivers/iio/temperature/mlx90614.c:
> > > >
> > > >     ret |= field_prep(chip_info->fir_config_mask, MLX90614_CONST_FIR);
> > > >
> > > > So if you want enhanced safety, having both the safer/const upper-case
> > > > variants and the less-safe/non-const lower-case variants makes sense.
> >
> > I agree with that. I just don't want the same shift-and operation to be
> > opencoded again and again.
> >
> > What I actually meant is that I'm OK with whatever number of field_prep()
> > macro flavors, if we make sure that they don't duplicate each other. So
> > for me, something like this would be the best solution:
> >
> >  #define field_prep(mask, val) \
> >        (((typeof(_mask))(_val) << __bf_shf(_mask)) & (_mask))
> >
> >  #define FIELD_PREP(mask, val)                                         \
> >          (                                                             \
> >                  FIELD_PREP_INPUT_CHECK(_mask, _val,);                 \
> >                  field_prep(mask, val);                                \
> >          )
> >
> > #define FIELD_PREP_CONST(_mask, _val)                                  \
> >         (                                                              \
> >                 FIELD_PREP_CONST_INPUT_CHECK(mask, val);
> >                 FIELD_PREP(mask, val); // or field_prep()
> >         )
> >
> > We have a similar macro GENMASK() in linux/bits.h. It is implemented
> > like this:
> >
> >  #define GENMASK_INPUT_CHECK(h, l) BUILD_BUG_ON_ZERO(const_true((l) > (h)))
> >  #define GENMASK(h, l) \
> >          (GENMASK_INPUT_CHECK(h, l) + __GENMASK(h, l))
> >
> > And it works just well. Can we end up with a similar approach here?
> 
> Note that there already exists a FIELD_PREP_CONST() macro, which is
> intended for struct member initialization.

Hi Geert,

That was my suggestion. Now that we're going to have many flavors
of the same macro, can we subordinate them to each other?