* [PATCH 7/9] wifi: rtw89: switch to using FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-17 17:36 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Ping-Ke Shih,
Richard Cochran, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Alexandre Belloni, Yury Norov,
Rasmus Villemoes, Hans de Goede, Linus Walleij, Sakari Ailus,
Salah Triki, Achim Gratz, Ben Collins, linux-kernel, linux-iio,
linux-wireless, netdev, linux-rtc
Cc: Yury Norov
In-Reply-To: <20260417173621.368914-1-ynorov@nvidia.com>
Switch from sign_extend32(FIELD_GET()) to the dedicated
FIELD_GET_SIGNED() and don't calculate the fields length explicitly.
Signed-off-by: Yury Norov <ynorov@nvidia.com>
---
drivers/net/wireless/realtek/rtw89/rtw8852a_rfk.c | 4 ++--
drivers/net/wireless/realtek/rtw89/rtw8852b_common.c | 4 ++--
drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c | 4 ++--
drivers/net/wireless/realtek/rtw89/rtw8852c.c | 4 ++--
4 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw89/rtw8852a_rfk.c b/drivers/net/wireless/realtek/rtw89/rtw8852a_rfk.c
index 463399413318..8679b21fd3fd 100644
--- a/drivers/net/wireless/realtek/rtw89/rtw8852a_rfk.c
+++ b/drivers/net/wireless/realtek/rtw89/rtw8852a_rfk.c
@@ -334,8 +334,8 @@ static void _check_addc(struct rtw89_dev *rtwdev, enum rtw89_rf_path path)
for (i = 0; i < ADDC_T_AVG; i++) {
tmp = rtw89_phy_read32_mask(rtwdev, R_DBG32_D, MASKDWORD);
- dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
- dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
+ dc_re += FIELD_GET_SIGNED(0xfff000, tmp);
+ dc_im += FIELD_GET_SIGNED(0xfff, tmp);
}
dc_re /= ADDC_T_AVG;
diff --git a/drivers/net/wireless/realtek/rtw89/rtw8852b_common.c b/drivers/net/wireless/realtek/rtw89/rtw8852b_common.c
index 65b839323e3e..7894834091fe 100644
--- a/drivers/net/wireless/realtek/rtw89/rtw8852b_common.c
+++ b/drivers/net/wireless/realtek/rtw89/rtw8852b_common.c
@@ -206,9 +206,9 @@ static void rtw8852bx_efuse_parsing_tssi(struct rtw89_dev *rtwdev,
static bool _decode_efuse_gain(u8 data, s8 *high, s8 *low)
{
if (high)
- *high = sign_extend32(FIELD_GET(GENMASK(7, 4), data), 3);
+ *high = FIELD_GET_SIGNED(GENMASK(7, 4), data);
if (low)
- *low = sign_extend32(FIELD_GET(GENMASK(3, 0), data), 3);
+ *low = FIELD_GET(GENMASK(3, 0), data);
return data != 0xff;
}
diff --git a/drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c b/drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c
index 70b1515c00fa..8db6ea475128 100644
--- a/drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c
+++ b/drivers/net/wireless/realtek/rtw89/rtw8852b_rfk.c
@@ -497,8 +497,8 @@ static void _check_addc(struct rtw89_dev *rtwdev, enum rtw89_rf_path path)
for (i = 0; i < ADDC_T_AVG; i++) {
tmp = rtw89_phy_read32_mask(rtwdev, R_DBG32_D, MASKDWORD);
- dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
- dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
+ dc_re += FIELD_GET_SIGNED(0xfff000, tmp);
+ dc_im += FIELD_GET_SIGNED(0xfff, tmp);
}
dc_re /= ADDC_T_AVG;
diff --git a/drivers/net/wireless/realtek/rtw89/rtw8852c.c b/drivers/net/wireless/realtek/rtw89/rtw8852c.c
index 40db7e3c0d97..528f9f4b1fc3 100644
--- a/drivers/net/wireless/realtek/rtw89/rtw8852c.c
+++ b/drivers/net/wireless/realtek/rtw89/rtw8852c.c
@@ -517,9 +517,9 @@ static void rtw8852c_efuse_parsing_tssi(struct rtw89_dev *rtwdev,
static bool _decode_efuse_gain(u8 data, s8 *high, s8 *low)
{
if (high)
- *high = sign_extend32(FIELD_GET(GENMASK(7, 4), data), 3);
+ *high = FIELD_GET_SIGNED(GENMASK(7, 4), data);
if (low)
- *low = sign_extend32(FIELD_GET(GENMASK(3, 0), data), 3);
+ *low = FIELD_GET_SIGNED(GENMASK(3, 0), data);
return data != 0xff;
}
--
2.51.0
^ permalink raw reply related
* [PATCH 8/9] rtc: rv3032: switch to using FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-17 17:36 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Ping-Ke Shih,
Richard Cochran, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Alexandre Belloni, Yury Norov,
Rasmus Villemoes, Hans de Goede, Linus Walleij, Sakari Ailus,
Salah Triki, Achim Gratz, Ben Collins, linux-kernel, linux-iio,
linux-wireless, netdev, linux-rtc
Cc: Yury Norov
In-Reply-To: <20260417173621.368914-1-ynorov@nvidia.com>
Switch from sign_extend32(FIELD_GET()) to the dedicated
FIELD_GET_SIGNED() and don't calculate the fields length explicitly.
Signed-off-by: Yury Norov <ynorov@nvidia.com>
---
drivers/rtc/rtc-rv3032.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-rv3032.c b/drivers/rtc/rtc-rv3032.c
index 6c09da7738e1..6bafdec637ae 100644
--- a/drivers/rtc/rtc-rv3032.c
+++ b/drivers/rtc/rtc-rv3032.c
@@ -376,7 +376,7 @@ static int rv3032_read_offset(struct device *dev, long *offset)
if (ret < 0)
return ret;
- steps = sign_extend32(FIELD_GET(RV3032_OFFSET_MSK, value), 5);
+ steps = FIELD_GET_SIGNED(RV3032_OFFSET_MSK, value);
*offset = DIV_ROUND_CLOSEST(steps * OFFSET_STEP_PPT, 1000);
--
2.51.0
^ permalink raw reply related
* [PATCH 9/9] ptp: switch to using FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-17 17:36 UTC (permalink / raw)
To: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Ping-Ke Shih,
Richard Cochran, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Alexandre Belloni, Yury Norov,
Rasmus Villemoes, Hans de Goede, Linus Walleij, Sakari Ailus,
Salah Triki, Achim Gratz, Ben Collins, linux-kernel, linux-iio,
linux-wireless, netdev, linux-rtc
Cc: Yury Norov
In-Reply-To: <20260417173621.368914-1-ynorov@nvidia.com>
Switch from sign_extend32(FIELD_GET()) to the dedicated
FIELD_GET_SIGNED() and don't calculate the fields length explicitly.
Signed-off-by: Yury Norov <ynorov@nvidia.com>
---
drivers/ptp/ptp_fc3.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/ptp/ptp_fc3.c b/drivers/ptp/ptp_fc3.c
index 70002500170e..f0e000428a3f 100644
--- a/drivers/ptp/ptp_fc3.c
+++ b/drivers/ptp/ptp_fc3.c
@@ -55,8 +55,8 @@ static s64 tdc_meas2offset(struct idtfc3 *idtfc3, u64 meas_read)
{
s64 coarse, fine;
- fine = sign_extend64(FIELD_GET(FINE_MEAS_MASK, meas_read), 12);
- coarse = sign_extend64(FIELD_GET(COARSE_MEAS_MASK, meas_read), (39 - 13));
+ fine = FIELD_GET_SIGNED(FINE_MEAS_MASK, meas_read);
+ coarse = FIELD_GET_SIGNED(COARSE_MEAS_MASK, meas_read);
fine = div64_s64(fine * NSEC_PER_SEC, idtfc3->tdc_apll_freq * 62LL);
coarse = div64_s64(coarse * NSEC_PER_SEC, idtfc3->time_ref_freq);
--
2.51.0
^ permalink raw reply related
* Re: [PATCH 1/9] bitfield: add FIELD_GET_SIGNED()
From: Andy Shevchenko @ 2026-04-17 18:12 UTC (permalink / raw)
To: Yury Norov
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Ping-Ke Shih,
Richard Cochran, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Alexandre Belloni, Yury Norov,
Rasmus Villemoes, Hans de Goede, Linus Walleij, Sakari Ailus,
Salah Triki, Achim Gratz, Ben Collins, linux-kernel, linux-iio,
linux-wireless, netdev, linux-rtc
In-Reply-To: <20260417173621.368914-2-ynorov@nvidia.com>
On Fri, Apr 17, 2026 at 01:36:12PM -0400, Yury Norov wrote:
> The bitfields are designed in assumption that fields contain unsigned
> integer values, thus extracting the values from the field implies
> zero-extending.
>
> Some drivers need to sign-extend their fields, and currently do it like:
>
> dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
> dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
>
> It's error-prone because it relies on user to provide the correct
> index of the most significant bit and proper 32 vs 64 function flavor.
>
> Thus, introduce a FIELD_GET_SIGNED() macro, which is the more
> convenient and compiles (on x86_64) to just a couple instructions:
> shl and sar.
...
> +#define FIELD_GET_SIGNED(mask, reg) \
> + ({ \
> + __BF_FIELD_CHECK(mask, reg, 0U, "FIELD_GET_SIGNED: "); \
> + ((__signed_scalar_typeof(mask))((long long)(reg) << \
> + __builtin_clzll(mask) >> (__builtin_clzll(mask) + \
> + __builtin_ctzll(mask))));\
I would re-indent these lines as
((__signed_scalar_typeof(mask))
((long long)(reg) << __builtin_clzll(mask) >> \
(__builtin_clzll(mask) + __builtin_ctzll(mask)))); \
> + })
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 0/9] bitfield: add FIELD_GET_SIGNED()
From: Andy Shevchenko @ 2026-04-17 18:23 UTC (permalink / raw)
To: Yury Norov
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Ping-Ke Shih,
Richard Cochran, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Alexandre Belloni, Yury Norov,
Rasmus Villemoes, Hans de Goede, Linus Walleij, Sakari Ailus,
Salah Triki, Achim Gratz, Ben Collins, linux-kernel, linux-iio,
linux-wireless, netdev, linux-rtc
In-Reply-To: <20260417173621.368914-1-ynorov@nvidia.com>
On Fri, Apr 17, 2026 at 01:36:11PM -0400, Yury Norov wrote:
> The bitfields are designed in assumption that fields contain unsigned
> integer values, thus extracting the values from the field implies
> zero-extending.
>
> Some drivers need to sign-extend their fields, and currently do it like:
>
> dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
> dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
>
> It's error-prone because it relies on user to provide the correct
> index of the most significant bit.
>
> This series adds a signed version of FIELD_GET(), which is the more
> convenient and compiles (on x86_64) to just a couple instructions:
> shl and sar.
>
> Patch #1 adds FIELD_GET_SIGNED(), and the rest of the series applies it
> tree-wide.
Here the example is missing.
Nevertheless, I looked at the implementation a bit and wondering how would it
work for 64-bit mask of say GENMASK_ULL(63, 60)? Wouldn't it give an overflow?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* [PATCH v1] Fix missing RTC charge ctrl
From: Brian Sune @ 2026-04-17 18:38 UTC (permalink / raw)
To: alexandre.belloni; +Cc: linux-rtc, linux-kernel, Brian Sune
Default driver did not consider battery supported
use cases, which DTS and probe did not control
the charge switch and strength. As such battery
could be dried out and possible dmanage.
Signed-off-by: Brian Sune <briansune@gmail.com>
---
drivers/rtc/rtc-sd3078.c | 47 +++++++++++++++++++++++++++++++++++++++-
1 file changed, 46 insertions(+), 1 deletion(-)
diff --git a/drivers/rtc/rtc-sd3078.c b/drivers/rtc/rtc-sd3078.c
index 10cc1dcfc774..871e6d9acd86 100644
--- a/drivers/rtc/rtc-sd3078.c
+++ b/drivers/rtc/rtc-sd3078.c
@@ -22,11 +22,17 @@
#define SD3078_REG_CTRL1 0x0f
#define SD3078_REG_CTRL2 0x10
#define SD3078_REG_CTRL3 0x11
+#define SD3078_REG_AGTC 0x17
+#define SD3078_REG_CHARGE 0x18
#define KEY_WRITE1 0x80
#define KEY_WRITE2 0x04
#define KEY_WRITE3 0x80
+#define CLK_F32K 0x40
+
+#define BAT_IIC 0x80
+
#define NUM_TIME_REGS (SD3078_REG_YR - SD3078_REG_SC + 1)
/*
@@ -36,6 +42,13 @@
*/
#define WRITE_PROTECT_EN 0
+static const char * const sd3078_charge_names[] = {
+ "10k", /* 0x00 */
+ "5k", /* 0x01 */
+ "2k", /* 0x02 */
+ "inf", /* 0x03 */
+};
+
/*
* In order to prevent arbitrary modification of the time register,
* when modification of the register,
@@ -148,13 +161,15 @@ static const struct rtc_class_ops sd3078_rtc_ops = {
static const struct regmap_config regmap_config = {
.reg_bits = 8,
.val_bits = 8,
- .max_register = 0x11,
+ .max_register = 0x18,
};
static int sd3078_probe(struct i2c_client *client)
{
int ret;
+ unsigned int val;
struct regmap *regmap;
+ bool f32k_out, bat_iic;
struct rtc_device *rtc;
if (!i2c_check_functionality(client->adapter, I2C_FUNC_I2C))
@@ -182,6 +197,36 @@ static int sd3078_probe(struct i2c_client *client)
sd3078_enable_reg_write(regmap);
+ f32k_out = device_property_read_bool(&client->dev, "CLOCK_F32K");
+ regmap_update_bits(regmap, SD3078_REG_CTRL3,
+ CLK_F32K, !f32k_out);
+
+ bat_iic = device_property_read_bool(&client->dev, "IIC_ON_BAT");
+ regmap_update_bits(regmap, SD3078_REG_AGTC,
+ BAT_IIC, bat_iic);
+
+ ret = regmap_read(regmap, SD3078_REG_CHARGE, &val);
+ if (!ret) {
+ dev_info(&client->dev, "RTC BAT Charge: %s",
+ (val & 0x80) ? "ON" : "OFF");
+ }
+
+ ret = device_property_read_u32(&client->dev, "BAT_CHARGE", &val);
+ if (!ret) {
+ // 0: 10k, 1: 5k, 2: 2k, 3: inf
+ dev_info(&client->dev, "Enable Battery Charge.\n");
+ regmap_write(regmap, SD3078_REG_CHARGE,
+ (val < 3) ? (u8)(val|0x80) : 0x03);
+ }
+
+ ret = regmap_read(regmap, SD3078_REG_CHARGE, &val);
+ if (!ret) {
+ dev_info(&client->dev, "RTC BAT charge: %s",
+ (val & 0x80) ? "ON" : "OFF");
+ dev_info(&client->dev, "RTC BAT Charge Strength: %s",
+ sd3078_charge_names[val & 0x03]);
+ }
+
return 0;
}
--
2.34.1
^ permalink raw reply related
* Re: [PATCH 0/9] bitfield: add FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-17 19:21 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Ping-Ke Shih,
Richard Cochran, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Alexandre Belloni, Yury Norov,
Rasmus Villemoes, Hans de Goede, Linus Walleij, Sakari Ailus,
Salah Triki, Achim Gratz, Ben Collins, linux-kernel, linux-iio,
linux-wireless, netdev, linux-rtc
In-Reply-To: <aeJ6hnZSbo2DrLpi@ashevche-desk.local>
On Fri, Apr 17, 2026 at 09:23:02PM +0300, Andy Shevchenko wrote:
> On Fri, Apr 17, 2026 at 01:36:11PM -0400, Yury Norov wrote:
> > The bitfields are designed in assumption that fields contain unsigned
> > integer values, thus extracting the values from the field implies
> > zero-extending.
> >
> > Some drivers need to sign-extend their fields, and currently do it like:
> >
> > dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
> > dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
> >
> > It's error-prone because it relies on user to provide the correct
> > index of the most significant bit.
> >
> > This series adds a signed version of FIELD_GET(), which is the more
> > convenient and compiles (on x86_64) to just a couple instructions:
> > shl and sar.
> >
> > Patch #1 adds FIELD_GET_SIGNED(), and the rest of the series applies it
> > tree-wide.
>
> Here the example is missing.
This series is full of examples... I'll add one here if you prefer, if
it comes to v2.
> Nevertheless, I looked at the implementation a bit and wondering how would it
> work for 64-bit mask of say GENMASK_ULL(63, 60)? Wouldn't it give an overflow?
In that case, the '<< __builtin_clzll(mask)' part becomes a NOP, and
the compiler only emits a single sar:
long long foo(long long reg)
{
10: f3 0f 1e fa endbr64
return FIELD_GET_SIGNED(GENMASK_ULL(63, 60), reg);
14: 48 89 f8 mov %rdi,%rax
17: 48 c1 f8 3c sar $0x3c,%rax
}
Just tested it with a real kernel build with gcc-15.2, and it works as
intended.
^ permalink raw reply
* Re: [PATCH 1/9] bitfield: add FIELD_GET_SIGNED()
From: David Laight @ 2026-04-17 19:43 UTC (permalink / raw)
To: Yury Norov
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Ping-Ke Shih,
Richard Cochran, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Alexandre Belloni, Yury Norov,
Rasmus Villemoes, Hans de Goede, Linus Walleij, Sakari Ailus,
Salah Triki, Achim Gratz, Ben Collins, linux-kernel, linux-iio,
linux-wireless, netdev, linux-rtc
In-Reply-To: <20260417173621.368914-2-ynorov@nvidia.com>
On Fri, 17 Apr 2026 13:36:12 -0400
Yury Norov <ynorov@nvidia.com> wrote:
> The bitfields are designed in assumption that fields contain unsigned
> integer values, thus extracting the values from the field implies
> zero-extending.
>
> Some drivers need to sign-extend their fields, and currently do it like:
>
> dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
> dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
>
> It's error-prone because it relies on user to provide the correct
> index of the most significant bit and proper 32 vs 64 function flavor.
>
> Thus, introduce a FIELD_GET_SIGNED() macro, which is the more
> convenient and compiles (on x86_64) to just a couple instructions:
> shl and sar.
>
> Signed-off-by: Yury Norov <ynorov@nvidia.com>
> ---
> include/linux/bitfield.h | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 54aeeef1f0ec..35ef63972810 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -178,6 +178,22 @@
> __FIELD_GET(_mask, _reg, "FIELD_GET: "); \
> })
>
> +/**
> + * FIELD_GET_SIGNED() - extract a signed bitfield element
> + * @mask: shifted mask defining the field's length and position
> + * @reg: value of entire bitfield
> + *
> + * Returns the sign-extended field specified by @_mask from the
> + * bitfield passed in as @_reg by masking and shifting it down.
> + */
> +#define FIELD_GET_SIGNED(mask, reg) \
> + ({ \
> + __BF_FIELD_CHECK(mask, reg, 0U, "FIELD_GET_SIGNED: "); \
> + ((__signed_scalar_typeof(mask))((long long)(reg) << \
> + __builtin_clzll(mask) >> (__builtin_clzll(mask) + \
> + __builtin_ctzll(mask))));\
Have you looked at what that generates on a typical 32bit architecture?
It really a bad idea to use __signed_scalar_typeof() on anything that isn't
a simple variable.
The bloat from all this when 'mask' is an expansion of GENMASK() is horrid.
Indeed both signed_scalar_typeof() and unsigned_scalar_typeof() should
really not be used - there are generally much better ways.
In this case you can just write:
({
auto _mask = mask;
unsigned int __sl = __builtin_clzll(_mask);
unsigned int __sr = __sl + __builtin_ctzll(_mask);
__builtin_chose_expr(sizeof(_mask) <= 4,
(int)(reg) << __sl - 32 >> __sr - 32,
((long long)(reg) << __sl >> __sr)
})
and let the compiler do any more integer promotions (etc).
I'm also not convinced that the checks __BF_FIELD_CHECK() does
on 'reg' are in any sense worth the effort.
I have tried some simpler alternatives, eg:
!__builtin_constant_p(reg) && statically_true((reg & mask) == 0)
however that throws up some false positives due to some of weird ways
people have used FIELD_GET() where it is nothing like the simplest
(or most obvious) way to do things.
That might have been the code that split a 32bit value into bytes
in a printf with:
FIELD_GET(GENMASK(7, 0), val), FIELD_GET(GENMASK(15, 8), val),
FIELD_GET(GENMASK(23, 16), val), FIELD_GET(GENMASK(31, 24), val),
David
> + })
> +
> /**
> * FIELD_MODIFY() - modify a bitfield element
> * @_mask: shifted mask defining the field's length and position
^ permalink raw reply
* Re: [PATCH 1/9] bitfield: add FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-17 21:09 UTC (permalink / raw)
To: David Laight
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Ping-Ke Shih,
Richard Cochran, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Alexandre Belloni, Yury Norov,
Rasmus Villemoes, Hans de Goede, Linus Walleij, Sakari Ailus,
Salah Triki, Achim Gratz, Ben Collins, linux-kernel, linux-iio,
linux-wireless, netdev, linux-rtc
In-Reply-To: <20260417204355.37fd960d@pumpkin>
On Fri, Apr 17, 2026 at 08:43:55PM +0100, David Laight wrote:
> On Fri, 17 Apr 2026 13:36:12 -0400
> Yury Norov <ynorov@nvidia.com> wrote:
>
> > The bitfields are designed in assumption that fields contain unsigned
> > integer values, thus extracting the values from the field implies
> > zero-extending.
> >
> > Some drivers need to sign-extend their fields, and currently do it like:
> >
> > dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
> > dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
> >
> > It's error-prone because it relies on user to provide the correct
> > index of the most significant bit and proper 32 vs 64 function flavor.
> >
> > Thus, introduce a FIELD_GET_SIGNED() macro, which is the more
> > convenient and compiles (on x86_64) to just a couple instructions:
> > shl and sar.
> >
> > Signed-off-by: Yury Norov <ynorov@nvidia.com>
> > ---
> > include/linux/bitfield.h | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> > index 54aeeef1f0ec..35ef63972810 100644
> > --- a/include/linux/bitfield.h
> > +++ b/include/linux/bitfield.h
> > @@ -178,6 +178,22 @@
> > __FIELD_GET(_mask, _reg, "FIELD_GET: "); \
> > })
> >
> > +/**
> > + * FIELD_GET_SIGNED() - extract a signed bitfield element
> > + * @mask: shifted mask defining the field's length and position
> > + * @reg: value of entire bitfield
> > + *
> > + * Returns the sign-extended field specified by @_mask from the
> > + * bitfield passed in as @_reg by masking and shifting it down.
> > + */
> > +#define FIELD_GET_SIGNED(mask, reg) \
> > + ({ \
> > + __BF_FIELD_CHECK(mask, reg, 0U, "FIELD_GET_SIGNED: "); \
> > + ((__signed_scalar_typeof(mask))((long long)(reg) << \
> > + __builtin_clzll(mask) >> (__builtin_clzll(mask) + \
> > + __builtin_ctzll(mask))));\
>
> Have you looked at what that generates on a typical 32bit architecture?
Yes, for arm32:
#define FIELD_GET_SIGNED(mask, reg) \
((long long)(reg) << \
__builtin_clzll(mask) >> (__builtin_clzll(mask) + \
__builtin_ctzll(mask)))
long long foo(long long reg)
{
return FIELD_GET_SIGNED(0x00f00000ULL, reg);
}
generates:
foo(long long):
lsls r1, r0, #8
asrs r0, r1, #28
asrs r1, r1, #31
bx lr
Just as good as x86_64.
https://godbolt.org/z/eMnKrnocq
> It really a bad idea to use __signed_scalar_typeof() on anything that isn't
> a simple variable.
> The bloat from all this when 'mask' is an expansion of GENMASK() is horrid.
> Indeed both signed_scalar_typeof() and unsigned_scalar_typeof() should
> really not be used - there are generally much better ways.
David, it's not the first time you're throwing "bad idea, horrid bloat,
really not be used"-like rant with absolutely no evidence that people
do something wrong. Today I became another random victim of your style
of communication, and I don't think there's any benefit to tolerate it
for me or anybody else.
I encourage you to change your attitude, and use professional and
specific communication style in the kernel mailing list.
Starting from now, I'm not a free tester for your ideas anymore. If
you think that my patch is wrong, please prove it yourself. If you
think that 32-bit or whatever code generation is bad - please send
an example. If you believe that your implementation is any better -
please bother yourself to convince me.
I will continue receiving patches from you in my tree, but if your
patch is claimed to improve code generation, performance of any sort,
or similar things, and doesn't provide any numbers - I'll not waste
my time on it.
Thanks,
Yury
> In this case you can just write:
> ({
> auto _mask = mask;
> unsigned int __sl = __builtin_clzll(_mask);
> unsigned int __sr = __sl + __builtin_ctzll(_mask);
> __builtin_chose_expr(sizeof(_mask) <= 4,
> (int)(reg) << __sl - 32 >> __sr - 32,
> ((long long)(reg) << __sl >> __sr)
> })
> and let the compiler do any more integer promotions (etc).
>
> I'm also not convinced that the checks __BF_FIELD_CHECK() does
> on 'reg' are in any sense worth the effort.
>
> I have tried some simpler alternatives, eg:
> !__builtin_constant_p(reg) && statically_true((reg & mask) == 0)
> however that throws up some false positives due to some of weird ways
> people have used FIELD_GET() where it is nothing like the simplest
> (or most obvious) way to do things.
> That might have been the code that split a 32bit value into bytes
> in a printf with:
> FIELD_GET(GENMASK(7, 0), val), FIELD_GET(GENMASK(15, 8), val),
> FIELD_GET(GENMASK(23, 16), val), FIELD_GET(GENMASK(31, 24), val),
>
> David
>
> > + })
> > +
> > /**
> > * FIELD_MODIFY() - modify a bitfield element
> > * @_mask: shifted mask defining the field's length and position
^ permalink raw reply
* Re: [PATCH 3/9] iio: intel_dc_ti_adc: switch to using FIELD_GET_SIGNED()
From: Jonathan Cameron @ 2026-04-19 13:18 UTC (permalink / raw)
To: Yury Norov
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, David Lechner,
Nuno Sá, Andy Shevchenko, Ping-Ke Shih, Richard Cochran,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Alexandre Belloni, Yury Norov, Rasmus Villemoes,
Hans de Goede, Linus Walleij, Sakari Ailus, Salah Triki,
Achim Gratz, Ben Collins, linux-kernel, linux-iio, linux-wireless,
netdev, linux-rtc
In-Reply-To: <20260417173621.368914-4-ynorov@nvidia.com>
On Fri, 17 Apr 2026 13:36:14 -0400
Yury Norov <ynorov@nvidia.com> wrote:
> Switch from sign_extend32(FIELD_GET()) to the dedicated
> FIELD_GET_SIGNED() and don't provide the fields length explicitly.
>
> Signed-off-by: Yury Norov <ynorov@nvidia.com>
Assuming any remaining discussion on the implementation of the macro
shakes out, this looks like a good little cleanup to me.
Not sure how you want to merge this but if it's going through another tree.
Acked-by: Jonathan Cameron <jic23@kernel.org>
> ---
> drivers/iio/adc/intel_dc_ti_adc.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/iio/adc/intel_dc_ti_adc.c b/drivers/iio/adc/intel_dc_ti_adc.c
> index 0fe34f1c338e..b5afad713e2d 100644
> --- a/drivers/iio/adc/intel_dc_ti_adc.c
> +++ b/drivers/iio/adc/intel_dc_ti_adc.c
> @@ -290,8 +290,8 @@ static int dc_ti_adc_probe(struct platform_device *pdev)
> if (ret)
> return ret;
>
> - info->vbat_zse = sign_extend32(FIELD_GET(DC_TI_VBAT_ZSE, val), 3);
> - info->vbat_ge = sign_extend32(FIELD_GET(DC_TI_VBAT_GE, val), 3);
> + info->vbat_zse = FIELD_GET_SIGNED(DC_TI_VBAT_ZSE, val);
> + info->vbat_ge = FIELD_GET_SIGNED(DC_TI_VBAT_GE, val);
>
> dev_dbg(dev, "vbat-zse %d vbat-ge %d\n", info->vbat_zse, info->vbat_ge);
>
^ permalink raw reply
* Re: [PATCH 4/9] iio: magnetometer: yas530: switch to using FIELD_GET_SIGNED()
From: Jonathan Cameron @ 2026-04-19 13:20 UTC (permalink / raw)
To: Yury Norov
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, David Lechner,
Nuno Sá, Andy Shevchenko, Ping-Ke Shih, Richard Cochran,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Alexandre Belloni, Yury Norov, Rasmus Villemoes,
Hans de Goede, Linus Walleij, Sakari Ailus, Salah Triki,
Achim Gratz, Ben Collins, linux-kernel, linux-iio, linux-wireless,
netdev, linux-rtc
In-Reply-To: <20260417173621.368914-5-ynorov@nvidia.com>
On Fri, 17 Apr 2026 13:36:15 -0400
Yury Norov <ynorov@nvidia.com> wrote:
> Switch from sign_extend32(FIELD_GET()) to the dedicated
> FIELD_GET_SIGNED() and don't calculate the fields length explicitly.
>
> Signed-off-by: Yury Norov <ynorov@nvidia.com>
Acked-by: Jonathan Cameron <jic23@kernel.org>
^ permalink raw reply
* Re: [PATCH 5/9] iio: pressure: bmp280: switch to using FIELD_GET_SIGNED()
From: Jonathan Cameron @ 2026-04-19 13:21 UTC (permalink / raw)
To: Yury Norov
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, David Lechner,
Nuno Sá, Andy Shevchenko, Ping-Ke Shih, Richard Cochran,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Alexandre Belloni, Yury Norov, Rasmus Villemoes,
Hans de Goede, Linus Walleij, Sakari Ailus, Salah Triki,
Achim Gratz, Ben Collins, linux-kernel, linux-iio, linux-wireless,
netdev, linux-rtc
In-Reply-To: <20260417173621.368914-6-ynorov@nvidia.com>
On Fri, 17 Apr 2026 13:36:16 -0400
Yury Norov <ynorov@nvidia.com> wrote:
> Switch from sign_extend32(FIELD_GET()) to the dedicated
> FIELD_GET_SIGNED() and don't calculate the fields length explicitly.
>
> Signed-off-by: Yury Norov <ynorov@nvidia.com>
Acked-by: Jonathan Cameron <jic23@kernel.org>
^ permalink raw reply
* Re: [PATCH 6/9] iio: mcp9600: switch to using FIELD_GET_SIGNED()
From: Jonathan Cameron @ 2026-04-19 13:21 UTC (permalink / raw)
To: Yury Norov
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, David Lechner,
Nuno Sá, Andy Shevchenko, Ping-Ke Shih, Richard Cochran,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Alexandre Belloni, Yury Norov, Rasmus Villemoes,
Hans de Goede, Linus Walleij, Sakari Ailus, Salah Triki,
Achim Gratz, Ben Collins, linux-kernel, linux-iio, linux-wireless,
netdev, linux-rtc
In-Reply-To: <20260417173621.368914-7-ynorov@nvidia.com>
On Fri, 17 Apr 2026 13:36:17 -0400
Yury Norov <ynorov@nvidia.com> wrote:
> Switch from sign_extend32(FIELD_GET()) to the dedicated
> FIELD_GET_SIGNED() and don't calculate the fields length explicitly.
>
> Signed-off-by: Yury Norov <ynorov@nvidia.com>
Acked-by: Jonathan Cameron <jic23@kernel.org>
^ permalink raw reply
* Re: [PATCH 4/9] iio: magnetometer: yas530: switch to using FIELD_GET_SIGNED()
From: Linus Walleij @ 2026-04-19 20:11 UTC (permalink / raw)
To: Yury Norov
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Peter Zijlstra, Jonathan Cameron,
David Lechner, Nuno Sá, Andy Shevchenko, Ping-Ke Shih,
Richard Cochran, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Alexandre Belloni, Yury Norov,
Rasmus Villemoes, Hans de Goede, Sakari Ailus, Salah Triki,
Achim Gratz, Ben Collins, linux-kernel, linux-iio, linux-wireless,
netdev, linux-rtc
In-Reply-To: <20260417173621.368914-5-ynorov@nvidia.com>
On Fri, Apr 17, 2026 at 7:36 PM Yury Norov <ynorov@nvidia.com> wrote:
> Switch from sign_extend32(FIELD_GET()) to the dedicated
> FIELD_GET_SIGNED() and don't calculate the fields length explicitly.
>
> Signed-off-by: Yury Norov <ynorov@nvidia.com>
Very nice,
Reviewed-by: Linus Walleij <linusw@kernel.org>
Yours,
Linus Walleij
^ permalink raw reply
* [PATCH] rtc: ab8500: replace sprintf() with sysfs_emit()
From: Maxwell Doose @ 2026-04-19 22:36 UTC (permalink / raw)
To: linusw, alexandre.belloni; +Cc: linux-arm-kernel, linux-rtc, linux-kernel
This patch replaces sprintf() with sysfs_emit() to ensure proper
bounds checking. It also simplifies the return logic by directly
returning the error after logging, instead of logging, calling
sprintf(), then returning.
Signed-off-by: Maxwell Doose <m32285159@gmail.com>
---
drivers/rtc/rtc-ab8500.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/rtc/rtc-ab8500.c b/drivers/rtc/rtc-ab8500.c
index ed2b6b8bb3bf..c6147837f957 100644
--- a/drivers/rtc/rtc-ab8500.c
+++ b/drivers/rtc/rtc-ab8500.c
@@ -284,11 +284,10 @@ static ssize_t ab8500_sysfs_show_rtc_calibration(struct device *dev,
retval = ab8500_rtc_get_calibration(dev, &calibration);
if (retval < 0) {
dev_err(dev, "Failed to read RTC calibration attribute\n");
- sprintf(buf, "0\n");
return retval;
}
- return sprintf(buf, "%d\n", calibration);
+ return sysfs_emit(buf, "%d\n", calibration);
}
static DEVICE_ATTR(rtc_calibration, S_IRUGO | S_IWUSR,
--
2.53.0
^ permalink raw reply related
* RE: [PATCH 7/9] wifi: rtw89: switch to using FIELD_GET_SIGNED()
From: Ping-Ke Shih @ 2026-04-20 7:49 UTC (permalink / raw)
To: Yury Norov, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86@kernel.org, H. Peter Anvin, Andy Lutomirski,
Peter Zijlstra, Jonathan Cameron, David Lechner, Nuno Sá,
Andy Shevchenko, Richard Cochran, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexandre Belloni,
Yury Norov, Rasmus Villemoes, Hans de Goede, Linus Walleij,
Sakari Ailus, Salah Triki, Achim Gratz, Ben Collins,
linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
linux-rtc@vger.kernel.org
In-Reply-To: <20260417173621.368914-8-ynorov@nvidia.com>
Yury Norov <ynorov@nvidia.com> wrote:
> --- a/drivers/net/wireless/realtek/rtw89/rtw8852b_common.c
> +++ b/drivers/net/wireless/realtek/rtw89/rtw8852b_common.c
> @@ -206,9 +206,9 @@ static void rtw8852bx_efuse_parsing_tssi(struct rtw89_dev *rtwdev,
> static bool _decode_efuse_gain(u8 data, s8 *high, s8 *low)
> {
> if (high)
> - *high = sign_extend32(FIELD_GET(GENMASK(7, 4), data), 3);
> + *high = FIELD_GET_SIGNED(GENMASK(7, 4), data);
> if (low)
> - *low = sign_extend32(FIELD_GET(GENMASK(3, 0), data), 3);
> + *low = FIELD_GET(GENMASK(3, 0), data);
FIELD_GET_SIGNED()?
>
> return data != 0xff;
> }
^ permalink raw reply
* Re: [PATCH 1/9] bitfield: add FIELD_GET_SIGNED()
From: Johannes Berg @ 2026-04-20 8:43 UTC (permalink / raw)
To: Yury Norov, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Andy Lutomirski, Peter Zijlstra,
Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Ping-Ke Shih, Richard Cochran, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexandre Belloni,
Yury Norov, Rasmus Villemoes, Hans de Goede, Linus Walleij,
Sakari Ailus, Salah Triki, Achim Gratz, Ben Collins, linux-kernel,
linux-iio, linux-wireless, netdev, linux-rtc
In-Reply-To: <20260417173621.368914-2-ynorov@nvidia.com>
On Fri, 2026-04-17 at 13:36 -0400, Yury Norov wrote:
> The bitfields are designed in assumption that fields contain unsigned
> integer values, thus extracting the values from the field implies
> zero-extending.
>
> Some drivers need to sign-extend their fields, and currently do it like:
>
> dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
> dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
That's indeed pretty awful...
> +#define FIELD_GET_SIGNED(mask, reg) \
>
[...]
I (personally) tend to prefer the "__MAKE_OP" versions (*_get_bits()
etc.), in particular because WiFi and firmware interfaces deal a lot
with fixed endian fields.
Any chance it'd be simple to generate u32_get_bits_signed() etc.? Could
be especially useful for le32_get_bits_signed() for example, to have the
endian conversion built-in unlike FIELD_GET_SIGNED().
johannes
^ permalink raw reply
* Re: [PATCH 1/9] bitfield: add FIELD_GET_SIGNED()
From: Peter Zijlstra @ 2026-04-20 11:19 UTC (permalink / raw)
To: Yury Norov
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Jonathan Cameron, David Lechner,
Nuno Sá, Andy Shevchenko, Ping-Ke Shih, Richard Cochran,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Alexandre Belloni, Yury Norov, Rasmus Villemoes,
Hans de Goede, Linus Walleij, Sakari Ailus, Salah Triki,
Achim Gratz, Ben Collins, linux-kernel, linux-iio, linux-wireless,
netdev, linux-rtc
In-Reply-To: <20260417173621.368914-2-ynorov@nvidia.com>
On Fri, Apr 17, 2026 at 01:36:12PM -0400, Yury Norov wrote:
> The bitfields are designed in assumption that fields contain unsigned
> integer values, thus extracting the values from the field implies
> zero-extending.
>
> Some drivers need to sign-extend their fields, and currently do it like:
>
> dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
> dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
>
> It's error-prone because it relies on user to provide the correct
> index of the most significant bit and proper 32 vs 64 function flavor.
>
> Thus, introduce a FIELD_GET_SIGNED() macro, which is the more
> convenient and compiles (on x86_64) to just a couple instructions:
> shl and sar.
>
> Signed-off-by: Yury Norov <ynorov@nvidia.com>
> ---
> include/linux/bitfield.h | 16 ++++++++++++++++
> 1 file changed, 16 insertions(+)
>
> diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> index 54aeeef1f0ec..35ef63972810 100644
> --- a/include/linux/bitfield.h
> +++ b/include/linux/bitfield.h
> @@ -178,6 +178,22 @@
> __FIELD_GET(_mask, _reg, "FIELD_GET: "); \
> })
>
> +/**
> + * FIELD_GET_SIGNED() - extract a signed bitfield element
> + * @mask: shifted mask defining the field's length and position
> + * @reg: value of entire bitfield
> + *
> + * Returns the sign-extended field specified by @_mask from the
> + * bitfield passed in as @_reg by masking and shifting it down.
> + */
> +#define FIELD_GET_SIGNED(mask, reg) \
> + ({ \
> + __BF_FIELD_CHECK(mask, reg, 0U, "FIELD_GET_SIGNED: "); \
> + ((__signed_scalar_typeof(mask))((long long)(reg) << \
> + __builtin_clzll(mask) >> (__builtin_clzll(mask) + \
> + __builtin_ctzll(mask))));\
> + })
IIRC clz is count-leading-zeros and ctz is count-trailing-zeros. Most of
the other FIELD things use __bf_shf() which is defined in terms of ffs -
1 (which is another way of writing ctz).
So how about you start by redefining __bf_shf() in ctz, and then add
another helper for the clz and write the thing something like:
((long long)(reg) << __bf_clz(mask)) >> (__bf_clz(mask) + __bf_shf(mask));
Also, since the order of the shifts is rather important, I think it
makes sense to add this extra pair of (), even when not strictly needed,
just to make it easier to read.
^ permalink raw reply
* Re: [PATCH 2/9] x86/extable: switch to using FIELD_GET_SIGNED()
From: Peter Zijlstra @ 2026-04-20 11:24 UTC (permalink / raw)
To: Yury Norov
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Jonathan Cameron, David Lechner,
Nuno Sá, Andy Shevchenko, Ping-Ke Shih, Richard Cochran,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Alexandre Belloni, Yury Norov, Rasmus Villemoes,
Hans de Goede, Linus Walleij, Sakari Ailus, Salah Triki,
Achim Gratz, Ben Collins, linux-kernel, linux-iio, linux-wireless,
netdev, linux-rtc
In-Reply-To: <20260417173621.368914-3-ynorov@nvidia.com>
On Fri, Apr 17, 2026 at 01:36:13PM -0400, Yury Norov wrote:
> The EX_DATA register is laid out such that EX_DATA_IMM occupied MSB.
> It's done to make sure that FIELD_GET() will sign-extend the IMM
> field during extraction.
>
> To enforce that, all EX_DATA masks are made signed integers. This
> works, but relies on the particular implementation of FIELD_GET(),
> i.e. masking then shifting, not vice versa; and the particular
> placement of the fields in the register.
I don't think the order of the mask and shift matters in this case. If
we were to first shift down and then mask, it would still work (after
all, the mask would also need to be shifted and would also get sign
extended, effectively ending up as -1).
But yes, this very much depends on the signed field being the topmost
field and including the MSB.
^ permalink raw reply
* Re: [PATCH 2/9] x86/extable: switch to using FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-20 17:18 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Jonathan Cameron, David Lechner,
Nuno Sá, Andy Shevchenko, Ping-Ke Shih, Richard Cochran,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Alexandre Belloni, Yury Norov, Rasmus Villemoes,
Hans de Goede, Linus Walleij, Sakari Ailus, Salah Triki,
Achim Gratz, Ben Collins, linux-kernel, linux-iio, linux-wireless,
netdev, linux-rtc
In-Reply-To: <20260420112428.GF3102624@noisy.programming.kicks-ass.net>
On Mon, Apr 20, 2026 at 01:24:28PM +0200, Peter Zijlstra wrote:
> On Fri, Apr 17, 2026 at 01:36:13PM -0400, Yury Norov wrote:
> > The EX_DATA register is laid out such that EX_DATA_IMM occupied MSB.
> > It's done to make sure that FIELD_GET() will sign-extend the IMM
> > field during extraction.
> >
> > To enforce that, all EX_DATA masks are made signed integers. This
> > works, but relies on the particular implementation of FIELD_GET(),
> > i.e. masking then shifting, not vice versa; and the particular
> > placement of the fields in the register.
>
> I don't think the order of the mask and shift matters in this case. If
> we were to first shift down and then mask, it would still work (after
> all, the mask would also need to be shifted and would also get sign
> extended, effectively ending up as -1).
FIELD_GET() doesn't require mask to be signed when a reg is signed, so
shifting mask may become zero-extended in an alternative implementation:
(reg >> __bf_shf(mask)) & (mask >> __bf_shf(mask)
This all is hypothetical, anyways.
> But yes, this very much depends on the signed field being the topmost
> field and including the MSB.
This is the part I dislike mostly. This would look just like undefined
behavior for the API user: depending on fields placement or type of the
inputs, sometimes FIELD_GET() sign-extendeds the field, and sometimes
not.
We could likely force FIELD_GET() to treat both reg and mask as unsigned
types, and state that explicitly in the documentation.
^ permalink raw reply
* Re: [PATCH 1/9] bitfield: add FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-20 17:54 UTC (permalink / raw)
To: Peter Zijlstra
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen, x86,
H. Peter Anvin, Andy Lutomirski, Jonathan Cameron, David Lechner,
Nuno Sá, Andy Shevchenko, Ping-Ke Shih, Richard Cochran,
Andrew Lunn, David S. Miller, Eric Dumazet, Jakub Kicinski,
Paolo Abeni, Alexandre Belloni, Yury Norov, Rasmus Villemoes,
Hans de Goede, Linus Walleij, Sakari Ailus, Salah Triki,
Achim Gratz, Ben Collins, linux-kernel, linux-iio, linux-wireless,
netdev, linux-rtc
In-Reply-To: <20260420111940.GE3102624@noisy.programming.kicks-ass.net>
On Mon, Apr 20, 2026 at 01:19:40PM +0200, Peter Zijlstra wrote:
> On Fri, Apr 17, 2026 at 01:36:12PM -0400, Yury Norov wrote:
> > The bitfields are designed in assumption that fields contain unsigned
> > integer values, thus extracting the values from the field implies
> > zero-extending.
> >
> > Some drivers need to sign-extend their fields, and currently do it like:
> >
> > dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
> > dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
> >
> > It's error-prone because it relies on user to provide the correct
> > index of the most significant bit and proper 32 vs 64 function flavor.
> >
> > Thus, introduce a FIELD_GET_SIGNED() macro, which is the more
> > convenient and compiles (on x86_64) to just a couple instructions:
> > shl and sar.
> >
> > Signed-off-by: Yury Norov <ynorov@nvidia.com>
> > ---
> > include/linux/bitfield.h | 16 ++++++++++++++++
> > 1 file changed, 16 insertions(+)
> >
> > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> > index 54aeeef1f0ec..35ef63972810 100644
> > --- a/include/linux/bitfield.h
> > +++ b/include/linux/bitfield.h
> > @@ -178,6 +178,22 @@
> > __FIELD_GET(_mask, _reg, "FIELD_GET: "); \
> > })
> >
> > +/**
> > + * FIELD_GET_SIGNED() - extract a signed bitfield element
> > + * @mask: shifted mask defining the field's length and position
> > + * @reg: value of entire bitfield
> > + *
> > + * Returns the sign-extended field specified by @_mask from the
> > + * bitfield passed in as @_reg by masking and shifting it down.
> > + */
> > +#define FIELD_GET_SIGNED(mask, reg) \
> > + ({ \
> > + __BF_FIELD_CHECK(mask, reg, 0U, "FIELD_GET_SIGNED: "); \
> > + ((__signed_scalar_typeof(mask))((long long)(reg) << \
> > + __builtin_clzll(mask) >> (__builtin_clzll(mask) + \
> > + __builtin_ctzll(mask))));\
> > + })
>
> IIRC clz is count-leading-zeros and ctz is count-trailing-zeros. Most of
> the other FIELD things use __bf_shf() which is defined in terms of ffs -
> 1 (which is another way of writing ctz).
>
> So how about you start by redefining __bf_shf() in ctz, and then add
> another helper for the clz and write the thing something like:
>
> ((long long)(reg) << __bf_clz(mask)) >> (__bf_clz(mask) + __bf_shf(mask));
So...
I like the shorter form, but whatever we add in the bitfield.h - we'll
have to support it.
For example, __bf_shf() wasn't intended to be used outsize of the
header, thus double underscored. But there's over 100 external users
now. And to make it worse, it's broken for GCC 14 and earlier:
https://lore.kernel.org/all/20260409-field-prep-fix-v1-1-f0e9ae64f63c@imgtec.com/
So needs to get fixed.
The bitfield.h has two __bf macros: __bf_shf() and __bf_cast_unsigned().
They are thin wrappers, but after all do something with the corresponding
builtins output. The __bf_cls() would be a pure renaming. I'm OK with
that, but some people don't:
https://lore.kernel.org/all/20260303182845.250bb2de@kernel.org/
That's why I didn't make FIELD_GET_SIGNED() implementation looking nicer.
If you strongly prefer the shorter version, I can do that in v2.
> Also, since the order of the shifts is rather important, I think it
> makes sense to add this extra pair of (), even when not strictly needed,
> just to make it easier to read.
Sure, will do.
^ permalink raw reply
* Re: [PATCH 7/9] wifi: rtw89: switch to using FIELD_GET_SIGNED()
From: Yury Norov @ 2026-04-20 17:59 UTC (permalink / raw)
To: Ping-Ke Shih
Cc: Thomas Gleixner, Ingo Molnar, Borislav Petkov, Dave Hansen,
x86@kernel.org, H. Peter Anvin, Andy Lutomirski, Peter Zijlstra,
Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Richard Cochran, Andrew Lunn, David S. Miller, Eric Dumazet,
Jakub Kicinski, Paolo Abeni, Alexandre Belloni, Yury Norov,
Rasmus Villemoes, Hans de Goede, Linus Walleij, Sakari Ailus,
Salah Triki, Achim Gratz, Ben Collins,
linux-kernel@vger.kernel.org, linux-iio@vger.kernel.org,
linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
linux-rtc@vger.kernel.org
In-Reply-To: <5fea4ea146404b55919037594ab85f1a@realtek.com>
On Mon, Apr 20, 2026 at 07:49:19AM +0000, Ping-Ke Shih wrote:
> Yury Norov <ynorov@nvidia.com> wrote:
> > --- a/drivers/net/wireless/realtek/rtw89/rtw8852b_common.c
> > +++ b/drivers/net/wireless/realtek/rtw89/rtw8852b_common.c
> > @@ -206,9 +206,9 @@ static void rtw8852bx_efuse_parsing_tssi(struct rtw89_dev *rtwdev,
> > static bool _decode_efuse_gain(u8 data, s8 *high, s8 *low)
> > {
> > if (high)
> > - *high = sign_extend32(FIELD_GET(GENMASK(7, 4), data), 3);
> > + *high = FIELD_GET_SIGNED(GENMASK(7, 4), data);
> > if (low)
> > - *low = sign_extend32(FIELD_GET(GENMASK(3, 0), data), 3);
> > + *low = FIELD_GET(GENMASK(3, 0), data);
>
> FIELD_GET_SIGNED()?
>
> >
> > return data != 0xff;
> > }
Ah sorry. Will fix in v2
^ permalink raw reply
* Re: [PATCH 2/9] x86/extable: switch to using FIELD_GET_SIGNED()
From: David Laight @ 2026-04-20 22:00 UTC (permalink / raw)
To: Yury Norov
Cc: Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Andy Lutomirski,
Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Ping-Ke Shih, Richard Cochran, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexandre Belloni,
Yury Norov, Rasmus Villemoes, Hans de Goede, Linus Walleij,
Sakari Ailus, Salah Triki, Achim Gratz, Ben Collins, linux-kernel,
linux-iio, linux-wireless, netdev, linux-rtc
In-Reply-To: <aeZf98xjbxdHvZOS@yury>
On Mon, 20 Apr 2026 13:18:47 -0400
Yury Norov <ynorov@nvidia.com> wrote:
> On Mon, Apr 20, 2026 at 01:24:28PM +0200, Peter Zijlstra wrote:
> > On Fri, Apr 17, 2026 at 01:36:13PM -0400, Yury Norov wrote:
> > > The EX_DATA register is laid out such that EX_DATA_IMM occupied MSB.
> > > It's done to make sure that FIELD_GET() will sign-extend the IMM
> > > field during extraction.
> > >
> > > To enforce that, all EX_DATA masks are made signed integers. This
> > > works, but relies on the particular implementation of FIELD_GET(),
> > > i.e. masking then shifting, not vice versa; and the particular
> > > placement of the fields in the register.
> >
> > I don't think the order of the mask and shift matters in this case. If
> > we were to first shift down and then mask, it would still work (after
> > all, the mask would also need to be shifted and would also get sign
> > extended, effectively ending up as -1).
>
> FIELD_GET() doesn't require mask to be signed when a reg is signed, so
> shifting mask may become zero-extended in an alternative implementation:
>
> (reg >> __bf_shf(mask)) & (mask >> __bf_shf(mask)
>
> This all is hypothetical, anyways.
>
> > But yes, this very much depends on the signed field being the topmost
> > field and including the MSB.
>
> This is the part I dislike mostly. This would look just like undefined
> behavior for the API user: depending on fields placement or type of the
> inputs, sometimes FIELD_GET() sign-extendeds the field, and sometimes
> not.
>
> We could likely force FIELD_GET() to treat both reg and mask as unsigned
> types, and state that explicitly in the documentation.
>
There is already a BUILD_BUG_ON((_mask) == 0), changing it to >= 0
will detect negative masks.
I think the only one is the x86 exception table.
FIELD_GET() casts the result to typeof(_mask) so the sign of 'reg'
shouldn't matter.
I just tried building with a compile-time check for reg being negative.
But there are too many false positives from FIELD_GET(mask, readl(addr))
and FIELD_GET(mask, READ_ONCE(var)).
The pre-processor expansions of those don't bear thinking about.
It's late now, but I will check how __unsigned_scalar_typeof() handles
variables with const or volatile qualifiers.
I think they do though the 'default' the same at bitfields.
David
^ permalink raw reply
* Re: [PATCH 1/9] bitfield: add FIELD_GET_SIGNED()
From: David Laight @ 2026-04-21 9:27 UTC (permalink / raw)
To: Yury Norov
Cc: Peter Zijlstra, Thomas Gleixner, Ingo Molnar, Borislav Petkov,
Dave Hansen, x86, H. Peter Anvin, Andy Lutomirski,
Jonathan Cameron, David Lechner, Nuno Sá, Andy Shevchenko,
Ping-Ke Shih, Richard Cochran, Andrew Lunn, David S. Miller,
Eric Dumazet, Jakub Kicinski, Paolo Abeni, Alexandre Belloni,
Yury Norov, Rasmus Villemoes, Hans de Goede, Linus Walleij,
Sakari Ailus, Salah Triki, Achim Gratz, Ben Collins, linux-kernel,
linux-iio, linux-wireless, netdev, linux-rtc
In-Reply-To: <aeZocbNjbvzMZO8b@yury>
On Mon, 20 Apr 2026 13:54:57 -0400
Yury Norov <ynorov@nvidia.com> wrote:
> On Mon, Apr 20, 2026 at 01:19:40PM +0200, Peter Zijlstra wrote:
> > On Fri, Apr 17, 2026 at 01:36:12PM -0400, Yury Norov wrote:
> > > The bitfields are designed in assumption that fields contain unsigned
> > > integer values, thus extracting the values from the field implies
> > > zero-extending.
> > >
> > > Some drivers need to sign-extend their fields, and currently do it like:
> > >
> > > dc_re += sign_extend32(FIELD_GET(0xfff000, tmp), 11);
> > > dc_im += sign_extend32(FIELD_GET(0xfff, tmp), 11);
> > >
> > > It's error-prone because it relies on user to provide the correct
> > > index of the most significant bit and proper 32 vs 64 function flavor.
> > >
> > > Thus, introduce a FIELD_GET_SIGNED() macro, which is the more
> > > convenient and compiles (on x86_64) to just a couple instructions:
> > > shl and sar.
> > >
> > > Signed-off-by: Yury Norov <ynorov@nvidia.com>
> > > ---
> > > include/linux/bitfield.h | 16 ++++++++++++++++
> > > 1 file changed, 16 insertions(+)
> > >
> > > diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
> > > index 54aeeef1f0ec..35ef63972810 100644
> > > --- a/include/linux/bitfield.h
> > > +++ b/include/linux/bitfield.h
> > > @@ -178,6 +178,22 @@
> > > __FIELD_GET(_mask, _reg, "FIELD_GET: "); \
> > > })
> > >
> > > +/**
> > > + * FIELD_GET_SIGNED() - extract a signed bitfield element
> > > + * @mask: shifted mask defining the field's length and position
> > > + * @reg: value of entire bitfield
> > > + *
> > > + * Returns the sign-extended field specified by @_mask from the
> > > + * bitfield passed in as @_reg by masking and shifting it down.
> > > + */
> > > +#define FIELD_GET_SIGNED(mask, reg) \
> > > + ({ \
> > > + __BF_FIELD_CHECK(mask, reg, 0U, "FIELD_GET_SIGNED: "); \
> > > + ((__signed_scalar_typeof(mask))((long long)(reg) << \
> > > + __builtin_clzll(mask) >> (__builtin_clzll(mask) + \
> > > + __builtin_ctzll(mask))));\
> > > + })
> >
> > IIRC clz is count-leading-zeros and ctz is count-trailing-zeros. Most of
> > the other FIELD things use __bf_shf() which is defined in terms of ffs -
> > 1 (which is another way of writing ctz).
> >
> > So how about you start by redefining __bf_shf() in ctz, and then add
> > another helper for the clz and write the thing something like:
> >
> > ((long long)(reg) << __bf_clz(mask)) >> (__bf_clz(mask) + __bf_shf(mask));
>
> So...
>
> I like the shorter form, but whatever we add in the bitfield.h - we'll
> have to support it.
>
> For example, __bf_shf() wasn't intended to be used outsize of the
> header, thus double underscored. But there's over 100 external users
> now. And to make it worse, it's broken for GCC 14 and earlier:
For anyone who hasn't followed the gory details it isn't 'very broken'.
Basically __builtin_ffsll() doesn't always generate an 'integer constant
expression' from constant input so you can get a compile fail.
> https://lore.kernel.org/all/20260409-field-prep-fix-v1-1-f0e9ae64f63c@imgtec.com/
>
> So needs to get fixed.
>
> The bitfield.h has two __bf macros: __bf_shf() and __bf_cast_unsigned().
> They are thin wrappers,
__bf_cast_unsigned() isn't exactly thin.
David
> but after all do something with the corresponding
> builtins output. The __bf_cls() would be a pure renaming. I'm OK with
> that, but some people don't:
>
> https://lore.kernel.org/all/20260303182845.250bb2de@kernel.org/
>
> That's why I didn't make FIELD_GET_SIGNED() implementation looking nicer.
> If you strongly prefer the shorter version, I can do that in v2.
>
> > Also, since the order of the shifts is rather important, I think it
> > makes sense to add this extra pair of (), even when not strictly needed,
> > just to make it easier to read.
>
> Sure, will do.
>
^ permalink raw reply
* Re: [PATCH] rtc: ab8500: replace sprintf() with sysfs_emit()
From: Linus Walleij @ 2026-04-21 11:21 UTC (permalink / raw)
To: Maxwell Doose
Cc: alexandre.belloni, linux-arm-kernel, linux-rtc, linux-kernel
In-Reply-To: <20260419223630.67644-1-m32285159@gmail.com>
On Mon, Apr 20, 2026 at 12:36 AM Maxwell Doose <m32285159@gmail.com> wrote:
> This patch replaces sprintf() with sysfs_emit() to ensure proper
> bounds checking. It also simplifies the return logic by directly
> returning the error after logging, instead of logging, calling
> sprintf(), then returning.
>
> Signed-off-by: Maxwell Doose <m32285159@gmail.com>
Reviewed-by: Linus Walleij <linusw@kernel.org>
Yours,
Linus Walleij
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox