* [PATCH rtw-next 0/2] wifi: rtw89: adjust code to fit coming field_prep()
@ 2025-11-17 3:29 Ping-Ke Shih
2025-11-17 3:29 ` [PATCH rtw-next 1/2] wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO Ping-Ke Shih
2025-11-17 3:29 ` [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl() Ping-Ke Shih
0 siblings, 2 replies; 11+ messages in thread
From: Ping-Ke Shih @ 2025-11-17 3:29 UTC (permalink / raw)
To: linux-wireless; +Cc: geert
As Geert mentioned [1], two more tricky cases are pointed out by
implementation of field_prep(). Change the code to resolve the tricks.
[1] https://lore.kernel.org/linux-wireless/fccadfe07e4244b993f44ac7315d3d52@realtek.com/T/#mbdfd08117f67b39fabb72dd71acb00f4b3c22bd8
Ping-Ke Shih (2):
wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO
wifi: rtw89: avoid to use not consecutive mask in __write_ctrl()
drivers/net/wireless/realtek/rtw89/mac.h | 20 ++++++++++++++
drivers/net/wireless/realtek/rtw89/rtw8851b.c | 26 +++++--------------
.../net/wireless/realtek/rtw89/rtw8852a_rfk.c | 8 +++---
drivers/net/wireless/realtek/rtw89/rtw8852b.c | 26 +++++--------------
drivers/net/wireless/realtek/rtw89/rtw8852c.c | 26 +++++--------------
5 files changed, 45 insertions(+), 61 deletions(-)
--
2.25.1
^ permalink raw reply [flat|nested] 11+ messages in thread
* [PATCH rtw-next 1/2] wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO
2025-11-17 3:29 [PATCH rtw-next 0/2] wifi: rtw89: adjust code to fit coming field_prep() Ping-Ke Shih
@ 2025-11-17 3:29 ` Ping-Ke Shih
2025-11-18 10:24 ` Geert Uytterhoeven
2025-11-17 3:29 ` [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl() Ping-Ke Shih
1 sibling, 1 reply; 11+ messages in thread
From: Ping-Ke Shih @ 2025-11-17 3:29 UTC (permalink / raw)
To: linux-wireless; +Cc: geert
The field mask should be bits 16-31, but suddenly use wrong bits 24-31,
rarely causing a little performance degraded if DAC/DAC FIFO stays on
an unexpected state.
Found this by Geert who works on bit field functions.
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
drivers/net/wireless/realtek/rtw89/rtw8852a_rfk.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw89/rtw8852a_rfk.c b/drivers/net/wireless/realtek/rtw89/rtw8852a_rfk.c
index e74257d19412..463399413318 100644
--- a/drivers/net/wireless/realtek/rtw89/rtw8852a_rfk.c
+++ b/drivers/net/wireless/realtek/rtw89/rtw8852a_rfk.c
@@ -756,8 +756,8 @@ static void _iqk_rxk_setting(struct rtw89_dev *rtwdev, u8 path)
rtw89_phy_write32_mask(rtwdev, R_ANAPAR, B_ANAPAR_FLTRST, 0x1);
rtw89_phy_write32_mask(rtwdev, R_ANAPAR_PW15, B_ANAPAR_PW15_H2, 0x0);
udelay(1);
- rtw89_phy_write32_mask(rtwdev, R_ADC_FIFO, B_ADC_FIFO_RST, 0x0303);
- rtw89_phy_write32_mask(rtwdev, R_ADC_FIFO, B_ADC_FIFO_RST, 0x0000);
+ rtw89_phy_write32_mask(rtwdev, R_ADC_FIFO, B_ADC_FIFO_RXK, 0x0303);
+ rtw89_phy_write32_mask(rtwdev, R_ADC_FIFO, B_ADC_FIFO_RXK, 0x0000);
switch (iqk_info->iqk_band[path]) {
case RTW89_BAND_2G:
@@ -1239,8 +1239,8 @@ static void _iqk_txk_setting(struct rtw89_dev *rtwdev, u8 path)
udelay(1);
rtw89_phy_write32_mask(rtwdev, R_ANAPAR, B_ANAPAR_15, 0x0041);
udelay(1);
- rtw89_phy_write32_mask(rtwdev, R_ADC_FIFO, B_ADC_FIFO_RST, 0x0303);
- rtw89_phy_write32_mask(rtwdev, R_ADC_FIFO, B_ADC_FIFO_RST, 0x0000);
+ rtw89_phy_write32_mask(rtwdev, R_ADC_FIFO, B_ADC_FIFO_RXK, 0x0303);
+ rtw89_phy_write32_mask(rtwdev, R_ADC_FIFO, B_ADC_FIFO_RXK, 0x0000);
switch (iqk_info->iqk_band[path]) {
case RTW89_BAND_2G:
rtw89_write_rf(rtwdev, path, RR_XALNA2, RR_XALNA2_SW, 0x00);
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl()
2025-11-17 3:29 [PATCH rtw-next 0/2] wifi: rtw89: adjust code to fit coming field_prep() Ping-Ke Shih
2025-11-17 3:29 ` [PATCH rtw-next 1/2] wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO Ping-Ke Shih
@ 2025-11-17 3:29 ` Ping-Ke Shih
2025-11-18 10:40 ` Geert Uytterhoeven
1 sibling, 1 reply; 11+ messages in thread
From: Ping-Ke Shih @ 2025-11-17 3:29 UTC (permalink / raw)
To: linux-wireless; +Cc: geert
The power value and enable bit fields can be not consecutive mask, but
normally we expect mask argument of rtw89_mac_txpwr_write32_mask() is
consecutive bit mask. Therefore, change the code accordingly.
Cc: Geert Uytterhoeven <geert@linux-m68k.org>
Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
---
drivers/net/wireless/realtek/rtw89/mac.h | 20 ++++++++++++++
drivers/net/wireless/realtek/rtw89/rtw8851b.c | 26 +++++--------------
drivers/net/wireless/realtek/rtw89/rtw8852b.c | 26 +++++--------------
drivers/net/wireless/realtek/rtw89/rtw8852c.c | 26 +++++--------------
4 files changed, 41 insertions(+), 57 deletions(-)
diff --git a/drivers/net/wireless/realtek/rtw89/mac.h b/drivers/net/wireless/realtek/rtw89/mac.h
index 3cc97fd0c0ec..01a9fb7c9e31 100644
--- a/drivers/net/wireless/realtek/rtw89/mac.h
+++ b/drivers/net/wireless/realtek/rtw89/mac.h
@@ -1456,6 +1456,26 @@ static inline int rtw89_mac_txpwr_write32_mask(struct rtw89_dev *rtwdev,
return 0;
}
+static inline
+void rtw89_mac_write_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 reg, u32 mask, u32 val,
+ u32 mask_en, bool cond)
+{
+ u32 wrt = u32_encode_bits(val, mask);
+ u32 val32;
+ int ret;
+
+ if (cond)
+ wrt |= mask_en;
+
+ ret = rtw89_mac_txpwr_read32(rtwdev, RTW89_PHY_0, reg, &val32);
+ if (ret)
+ return;
+
+ val32 &= ~(mask | mask_en);
+ val32 |= wrt;
+ rtw89_mac_txpwr_write32(rtwdev, RTW89_PHY_0, reg, val32);
+}
+
static inline void rtw89_mac_ctrl_hci_dma_tx(struct rtw89_dev *rtwdev,
bool enable)
{
diff --git a/drivers/net/wireless/realtek/rtw89/rtw8851b.c b/drivers/net/wireless/realtek/rtw89/rtw8851b.c
index 2019f6022cbb..1253c4af2fb2 100644
--- a/drivers/net/wireless/realtek/rtw89/rtw8851b.c
+++ b/drivers/net/wireless/realtek/rtw89/rtw8851b.c
@@ -2291,18 +2291,6 @@ rtw8851b_btc_set_wl_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 txpwr_val)
union rtw8851b_btc_wl_txpwr_ctrl arg = { .txpwr_val = txpwr_val };
s32 val;
-#define __write_ctrl(_reg, _msk, _val, _en, _cond) \
-do { \
- u32 _wrt = FIELD_PREP(_msk, _val); \
- BUILD_BUG_ON(!!(_msk & _en)); \
- if (_cond) \
- _wrt |= _en; \
- else \
- _wrt &= ~_en; \
- rtw89_mac_txpwr_write32_mask(rtwdev, RTW89_PHY_0, _reg, \
- _msk | _en, _wrt); \
-} while (0)
-
switch (arg.ctrl_all_time) {
case 0xffff:
val = 0;
@@ -2312,9 +2300,10 @@ do { \
break;
}
- __write_ctrl(R_AX_PWR_RATE_CTRL, B_AX_FORCE_PWR_BY_RATE_VALUE_MASK,
- val, B_AX_FORCE_PWR_BY_RATE_EN,
- arg.ctrl_all_time != 0xffff);
+ rtw89_mac_write_txpwr_ctrl(rtwdev, R_AX_PWR_RATE_CTRL,
+ B_AX_FORCE_PWR_BY_RATE_VALUE_MASK,
+ val, B_AX_FORCE_PWR_BY_RATE_EN,
+ arg.ctrl_all_time != 0xffff);
switch (arg.ctrl_gnt_bt) {
case 0xffff:
@@ -2325,10 +2314,9 @@ do { \
break;
}
- __write_ctrl(R_AX_PWR_COEXT_CTRL, B_AX_TXAGC_BT_MASK, val,
- B_AX_TXAGC_BT_EN, arg.ctrl_gnt_bt != 0xffff);
-
-#undef __write_ctrl
+ rtw89_mac_write_txpwr_ctrl(rtwdev, R_AX_PWR_COEXT_CTRL,
+ B_AX_TXAGC_BT_MASK, val,
+ B_AX_TXAGC_BT_EN, arg.ctrl_gnt_bt != 0xffff);
}
static
diff --git a/drivers/net/wireless/realtek/rtw89/rtw8852b.c b/drivers/net/wireless/realtek/rtw89/rtw8852b.c
index 38cd151f8c3f..de02d52150c1 100644
--- a/drivers/net/wireless/realtek/rtw89/rtw8852b.c
+++ b/drivers/net/wireless/realtek/rtw89/rtw8852b.c
@@ -761,18 +761,6 @@ rtw8852b_btc_set_wl_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 txpwr_val)
union rtw8852b_btc_wl_txpwr_ctrl arg = { .txpwr_val = txpwr_val };
s32 val;
-#define __write_ctrl(_reg, _msk, _val, _en, _cond) \
-do { \
- u32 _wrt = FIELD_PREP(_msk, _val); \
- BUILD_BUG_ON(!!(_msk & _en)); \
- if (_cond) \
- _wrt |= _en; \
- else \
- _wrt &= ~_en; \
- rtw89_mac_txpwr_write32_mask(rtwdev, RTW89_PHY_0, _reg, \
- _msk | _en, _wrt); \
-} while (0)
-
switch (arg.ctrl_all_time) {
case 0xffff:
val = 0;
@@ -782,9 +770,10 @@ do { \
break;
}
- __write_ctrl(R_AX_PWR_RATE_CTRL, B_AX_FORCE_PWR_BY_RATE_VALUE_MASK,
- val, B_AX_FORCE_PWR_BY_RATE_EN,
- arg.ctrl_all_time != 0xffff);
+ rtw89_mac_write_txpwr_ctrl(rtwdev, R_AX_PWR_RATE_CTRL,
+ B_AX_FORCE_PWR_BY_RATE_VALUE_MASK,
+ val, B_AX_FORCE_PWR_BY_RATE_EN,
+ arg.ctrl_all_time != 0xffff);
switch (arg.ctrl_gnt_bt) {
case 0xffff:
@@ -795,10 +784,9 @@ do { \
break;
}
- __write_ctrl(R_AX_PWR_COEXT_CTRL, B_AX_TXAGC_BT_MASK, val,
- B_AX_TXAGC_BT_EN, arg.ctrl_gnt_bt != 0xffff);
-
-#undef __write_ctrl
+ rtw89_mac_write_txpwr_ctrl(rtwdev, R_AX_PWR_COEXT_CTRL,
+ B_AX_TXAGC_BT_MASK, val,
+ B_AX_TXAGC_BT_EN, arg.ctrl_gnt_bt != 0xffff);
}
static const struct rtw89_chip_ops rtw8852b_chip_ops = {
diff --git a/drivers/net/wireless/realtek/rtw89/rtw8852c.c b/drivers/net/wireless/realtek/rtw89/rtw8852c.c
index ee81a6792eee..7fd1485d1eb7 100644
--- a/drivers/net/wireless/realtek/rtw89/rtw8852c.c
+++ b/drivers/net/wireless/realtek/rtw89/rtw8852c.c
@@ -2760,18 +2760,6 @@ rtw8852c_btc_set_wl_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 txpwr_val)
union rtw8852c_btc_wl_txpwr_ctrl arg = { .txpwr_val = txpwr_val };
s32 val;
-#define __write_ctrl(_reg, _msk, _val, _en, _cond) \
-do { \
- u32 _wrt = FIELD_PREP(_msk, _val); \
- BUILD_BUG_ON((_msk & _en) != 0); \
- if (_cond) \
- _wrt |= _en; \
- else \
- _wrt &= ~_en; \
- rtw89_mac_txpwr_write32_mask(rtwdev, RTW89_PHY_0, _reg, \
- _msk | _en, _wrt); \
-} while (0)
-
switch (arg.ctrl_all_time) {
case 0xffff:
val = 0;
@@ -2781,9 +2769,10 @@ do { \
break;
}
- __write_ctrl(R_AX_PWR_RATE_CTRL, B_AX_FORCE_PWR_BY_RATE_VALUE_MASK,
- val, B_AX_FORCE_PWR_BY_RATE_EN,
- arg.ctrl_all_time != 0xffff);
+ rtw89_mac_write_txpwr_ctrl(rtwdev, R_AX_PWR_RATE_CTRL,
+ B_AX_FORCE_PWR_BY_RATE_VALUE_MASK,
+ val, B_AX_FORCE_PWR_BY_RATE_EN,
+ arg.ctrl_all_time != 0xffff);
switch (arg.ctrl_gnt_bt) {
case 0xffff:
@@ -2794,10 +2783,9 @@ do { \
break;
}
- __write_ctrl(R_AX_PWR_COEXT_CTRL, B_AX_TXAGC_BT_MASK, val,
- B_AX_TXAGC_BT_EN, arg.ctrl_gnt_bt != 0xffff);
-
-#undef __write_ctrl
+ rtw89_mac_write_txpwr_ctrl(rtwdev, R_AX_PWR_COEXT_CTRL,
+ B_AX_TXAGC_BT_MASK, val,
+ B_AX_TXAGC_BT_EN, arg.ctrl_gnt_bt != 0xffff);
}
static
--
2.25.1
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH rtw-next 1/2] wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO
2025-11-17 3:29 ` [PATCH rtw-next 1/2] wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO Ping-Ke Shih
@ 2025-11-18 10:24 ` Geert Uytterhoeven
2025-11-19 0:44 ` Ping-Ke Shih
0 siblings, 1 reply; 11+ messages in thread
From: Geert Uytterhoeven @ 2025-11-18 10:24 UTC (permalink / raw)
To: Ping-Ke Shih; +Cc: linux-wireless
On Mon, 17 Nov 2025 at 04:29, Ping-Ke Shih <pkshih@realtek.com> wrote:
> The field mask should be bits 16-31, but suddenly use wrong bits 24-31,
> rarely causing a little performance degraded if DAC/DAC FIFO stays on
> an unexpected state.
>
> Found this by Geert who works on bit field functions.
>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl()
2025-11-17 3:29 ` [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl() Ping-Ke Shih
@ 2025-11-18 10:40 ` Geert Uytterhoeven
2025-11-19 1:15 ` Ping-Ke Shih
0 siblings, 1 reply; 11+ messages in thread
From: Geert Uytterhoeven @ 2025-11-18 10:40 UTC (permalink / raw)
To: Ping-Ke Shih; +Cc: linux-wireless
Hi Ping-Ke,
On Mon, 17 Nov 2025 at 04:30, Ping-Ke Shih <pkshih@realtek.com> wrote:
> The power value and enable bit fields can be not consecutive mask, but
> normally we expect mask argument of rtw89_mac_txpwr_write32_mask() is
> consecutive bit mask. Therefore, change the code accordingly.
>
> Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
Thanks for your patch!
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> --- a/drivers/net/wireless/realtek/rtw89/mac.h
> +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> @@ -1456,6 +1456,26 @@ static inline int rtw89_mac_txpwr_write32_mask(struct rtw89_dev *rtwdev,
> return 0;
> }
>
> +static inline
> +void rtw89_mac_write_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 reg, u32 mask, u32 val,
> + u32 mask_en, bool cond)
> +{
> + u32 wrt = u32_encode_bits(val, mask);
Nit: you could do without this variable...
> + u32 val32;
> + int ret;
> +
> + if (cond)
> + wrt |= mask_en;
> +
> + ret = rtw89_mac_txpwr_read32(rtwdev, RTW89_PHY_0, reg, &val32);
> + if (ret)
> + return;
> +
> + val32 &= ~(mask | mask_en);
> + val32 |= wrt;
val32 |= u32_encode_bits(val, mask);
if (cond)
cal32 |= mask_en;
> + rtw89_mac_txpwr_write32(rtwdev, RTW89_PHY_0, reg, val32);
As this calls mac->get_txpwr_cr() a second time, perhaps it is better to
open-code rtw89_mac_txpwr_read32() and rtw89_mac_txpwr_write32()
in this function?
> +}
> +
> static inline void rtw89_mac_ctrl_hci_dma_tx(struct rtw89_dev *rtwdev,
> bool enable)
> {
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH rtw-next 1/2] wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO
2025-11-18 10:24 ` Geert Uytterhoeven
@ 2025-11-19 0:44 ` Ping-Ke Shih
2025-11-19 8:35 ` Geert Uytterhoeven
0 siblings, 1 reply; 11+ messages in thread
From: Ping-Ke Shih @ 2025-11-19 0:44 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: linux-wireless@vger.kernel.org
Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> On Mon, 17 Nov 2025 at 04:29, Ping-Ke Shih <pkshih@realtek.com> wrote:
> > The field mask should be bits 16-31, but suddenly use wrong bits 24-31,
> > rarely causing a little performance degraded if DAC/DAC FIFO stays on
> > an unexpected state.
> >
> > Found this by Geert who works on bit field functions.
> >
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
>
> Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
I suppose you meant Reviewed-by, but you also the reporter. I will add
these tags by v2.
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl()
2025-11-18 10:40 ` Geert Uytterhoeven
@ 2025-11-19 1:15 ` Ping-Ke Shih
2025-11-19 8:38 ` Geert Uytterhoeven
0 siblings, 1 reply; 11+ messages in thread
From: Ping-Ke Shih @ 2025-11-19 1:15 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: linux-wireless@vger.kernel.org
Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ping-Ke,
>
> On Mon, 17 Nov 2025 at 04:30, Ping-Ke Shih <pkshih@realtek.com> wrote:
> > The power value and enable bit fields can be not consecutive mask, but
> > normally we expect mask argument of rtw89_mac_txpwr_write32_mask() is
> > consecutive bit mask. Therefore, change the code accordingly.
> >
> > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
>
> Thanks for your patch!
>
> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
>
> > --- a/drivers/net/wireless/realtek/rtw89/mac.h
> > +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> > @@ -1456,6 +1456,26 @@ static inline int rtw89_mac_txpwr_write32_mask(struct rtw89_dev *rtwdev,
> > return 0;
> > }
> >
> > +static inline
> > +void rtw89_mac_write_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 reg, u32 mask, u32 val,
> > + u32 mask_en, bool cond)
> > +{
> > + u32 wrt = u32_encode_bits(val, mask);
>
> Nit: you could do without this variable...
>
> > + u32 val32;
> > + int ret;
> > +
> > + if (cond)
> > + wrt |= mask_en;
> > +
> > + ret = rtw89_mac_txpwr_read32(rtwdev, RTW89_PHY_0, reg, &val32);
> > + if (ret)
> > + return;
> > +
> > + val32 &= ~(mask | mask_en);
> > + val32 |= wrt;
>
> val32 |= u32_encode_bits(val, mask);
> if (cond)
> cal32 |= mask_en;
With this change, ARCH arm is failed to build (x86 is well):
In file included from /build/rtw89/core.h:9,
from /build/rtw89/coex.h:8,
from /build/rtw89/rtw8851b.c:5:
In function 'field_multiplier',
inlined from 'field_mask' at ./include/linux/bitfield.h:170:17,
inlined from 'u32_encode_bits' at ./include/linux/bitfield.h:200:1,
inlined from 'rtw89_mac_write_txpwr_ctrl' at /build/rtw89/mac.h:1468:11:
./include/linux/bitfield.h:165:17: error: call to '__bad_mask' declared with attribute error: bad bitfield mask
165 | __bad_mask();
| ^~~~~~~~~~~~
In function 'field_multiplier',
>
> > + rtw89_mac_txpwr_write32(rtwdev, RTW89_PHY_0, reg, val32);
>
> As this calls mac->get_txpwr_cr() a second time, perhaps it is better to
> open-code rtw89_mac_txpwr_read32() and rtw89_mac_txpwr_write32()
> in this function?
This function isn't in hot path, and using rtw89_mac_txpwr_read32() and
rtw89_mac_txpwr_write32() is clearer than open-code, so I'd keep it as was.
Then I don't plan to send v2.
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH rtw-next 1/2] wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO
2025-11-19 0:44 ` Ping-Ke Shih
@ 2025-11-19 8:35 ` Geert Uytterhoeven
0 siblings, 0 replies; 11+ messages in thread
From: Geert Uytterhoeven @ 2025-11-19 8:35 UTC (permalink / raw)
To: Ping-Ke Shih; +Cc: linux-wireless@vger.kernel.org
Hi Ping-Ke,
On Wed, 19 Nov 2025 at 01:44, Ping-Ke Shih <pkshih@realtek.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Mon, 17 Nov 2025 at 04:29, Ping-Ke Shih <pkshih@realtek.com> wrote:
> > > The field mask should be bits 16-31, but suddenly use wrong bits 24-31,
> > > rarely causing a little performance degraded if DAC/DAC FIFO stays on
> > > an unexpected state.
> > >
> > > Found this by Geert who works on bit field functions.
> > >
> > > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> >
> > Reported-by: Geert Uytterhoeven <geert@linux-m68k.org>
>
> I suppose you meant Reviewed-by, but you also the reporter. I will add
> these tags by v2.
I did mean Reported-by.
I cannot review the actual values, as I do not have hardware documentation.
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl()
2025-11-19 1:15 ` Ping-Ke Shih
@ 2025-11-19 8:38 ` Geert Uytterhoeven
2025-11-19 8:53 ` Ping-Ke Shih
0 siblings, 1 reply; 11+ messages in thread
From: Geert Uytterhoeven @ 2025-11-19 8:38 UTC (permalink / raw)
To: Ping-Ke Shih; +Cc: linux-wireless@vger.kernel.org
Hi Ping-Ke,
On Wed, 19 Nov 2025 at 02:15, Ping-Ke Shih <pkshih@realtek.com> wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > On Mon, 17 Nov 2025 at 04:30, Ping-Ke Shih <pkshih@realtek.com> wrote:
> > > The power value and enable bit fields can be not consecutive mask, but
> > > normally we expect mask argument of rtw89_mac_txpwr_write32_mask() is
> > > consecutive bit mask. Therefore, change the code accordingly.
> > >
> > > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> >
> > Thanks for your patch!
> >
> > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> >
> > > --- a/drivers/net/wireless/realtek/rtw89/mac.h
> > > +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> > > @@ -1456,6 +1456,26 @@ static inline int rtw89_mac_txpwr_write32_mask(struct rtw89_dev *rtwdev,
> > > return 0;
> > > }
> > >
> > > +static inline
> > > +void rtw89_mac_write_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 reg, u32 mask, u32 val,
> > > + u32 mask_en, bool cond)
> > > +{
> > > + u32 wrt = u32_encode_bits(val, mask);
> >
> > Nit: you could do without this variable...
> >
> > > + u32 val32;
> > > + int ret;
> > > +
> > > + if (cond)
> > > + wrt |= mask_en;
> > > +
> > > + ret = rtw89_mac_txpwr_read32(rtwdev, RTW89_PHY_0, reg, &val32);
> > > + if (ret)
> > > + return;
> > > +
> > > + val32 &= ~(mask | mask_en);
> > > + val32 |= wrt;
> >
> > val32 |= u32_encode_bits(val, mask);
> > if (cond)
> > cal32 |= mask_en;
>
> With this change, ARCH arm is failed to build (x86 is well):
>
> In file included from /build/rtw89/core.h:9,
> from /build/rtw89/coex.h:8,
> from /build/rtw89/rtw8851b.c:5:
> In function 'field_multiplier',
> inlined from 'field_mask' at ./include/linux/bitfield.h:170:17,
> inlined from 'u32_encode_bits' at ./include/linux/bitfield.h:200:1,
> inlined from 'rtw89_mac_write_txpwr_ctrl' at /build/rtw89/mac.h:1468:11:
> ./include/linux/bitfield.h:165:17: error: call to '__bad_mask' declared with attribute error: bad bitfield mask
> 165 | __bad_mask();
> | ^~~~~~~~~~~~
> In function 'field_multiplier',
Hmm...
Note that u32_encode_bits() really requires a constant mask, just
like FIELD_PREP(). So probably the compiler can no longer deduce it
is called with a constant after restructuring the code...
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
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl()
2025-11-19 8:38 ` Geert Uytterhoeven
@ 2025-11-19 8:53 ` Ping-Ke Shih
2025-12-12 2:09 ` Ping-Ke Shih
0 siblings, 1 reply; 11+ messages in thread
From: Ping-Ke Shih @ 2025-11-19 8:53 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: linux-wireless@vger.kernel.org
Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> Hi Ping-Ke,
>
> On Wed, 19 Nov 2025 at 02:15, Ping-Ke Shih <pkshih@realtek.com> wrote:
> > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > On Mon, 17 Nov 2025 at 04:30, Ping-Ke Shih <pkshih@realtek.com> wrote:
> > > > The power value and enable bit fields can be not consecutive mask, but
> > > > normally we expect mask argument of rtw89_mac_txpwr_write32_mask() is
> > > > consecutive bit mask. Therefore, change the code accordingly.
> > > >
> > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> > >
> > > Thanks for your patch!
> > >
> > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > >
> > > > --- a/drivers/net/wireless/realtek/rtw89/mac.h
> > > > +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> > > > @@ -1456,6 +1456,26 @@ static inline int rtw89_mac_txpwr_write32_mask(struct rtw89_dev *rtwdev,
> > > > return 0;
> > > > }
> > > >
> > > > +static inline
> > > > +void rtw89_mac_write_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 reg, u32 mask, u32 val,
> > > > + u32 mask_en, bool cond)
> > > > +{
> > > > + u32 wrt = u32_encode_bits(val, mask);
> > >
> > > Nit: you could do without this variable...
> > >
> > > > + u32 val32;
> > > > + int ret;
> > > > +
> > > > + if (cond)
> > > > + wrt |= mask_en;
> > > > +
> > > > + ret = rtw89_mac_txpwr_read32(rtwdev, RTW89_PHY_0, reg, &val32);
> > > > + if (ret)
> > > > + return;
> > > > +
> > > > + val32 &= ~(mask | mask_en);
> > > > + val32 |= wrt;
> > >
> > > val32 |= u32_encode_bits(val, mask);
> > > if (cond)
> > > cal32 |= mask_en;
> >
> > With this change, ARCH arm is failed to build (x86 is well):
> >
> > In file included from /build/rtw89/core.h:9,
> > from /build/rtw89/coex.h:8,
> > from /build/rtw89/rtw8851b.c:5:
> > In function 'field_multiplier',
> > inlined from 'field_mask' at ./include/linux/bitfield.h:170:17,
> > inlined from 'u32_encode_bits' at ./include/linux/bitfield.h:200:1,
> > inlined from 'rtw89_mac_write_txpwr_ctrl' at /build/rtw89/mac.h:1468:11:
> > ./include/linux/bitfield.h:165:17: error: call to '__bad_mask' declared with attribute error: bad
> bitfield mask
> > 165 | __bad_mask();
> > | ^~~~~~~~~~~~
> > In function 'field_multiplier',
>
> Hmm...
>
> Note that u32_encode_bits() really requires a constant mask, just
> like FIELD_PREP(). So probably the compiler can no longer deduce it
> is called with a constant after restructuring the code...
Do you think I need to add a macro to generate mask with u32_encode_bits()
and then call this inline function?
Or, compiler can always deduce it in the inline function, and current version
is okay?
^ permalink raw reply [flat|nested] 11+ messages in thread
* RE: [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl()
2025-11-19 8:53 ` Ping-Ke Shih
@ 2025-12-12 2:09 ` Ping-Ke Shih
0 siblings, 0 replies; 11+ messages in thread
From: Ping-Ke Shih @ 2025-12-12 2:09 UTC (permalink / raw)
To: Geert Uytterhoeven; +Cc: linux-wireless@vger.kernel.org
Hi Geert,
Ping-Ke Shih wrote:
> Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > Hi Ping-Ke,
> >
> > On Wed, 19 Nov 2025 at 02:15, Ping-Ke Shih <pkshih@realtek.com> wrote:
> > > Geert Uytterhoeven <geert@linux-m68k.org> wrote:
> > > > On Mon, 17 Nov 2025 at 04:30, Ping-Ke Shih <pkshih@realtek.com> wrote:
> > > > > The power value and enable bit fields can be not consecutive mask, but
> > > > > normally we expect mask argument of rtw89_mac_txpwr_write32_mask() is
> > > > > consecutive bit mask. Therefore, change the code accordingly.
> > > > >
> > > > > Cc: Geert Uytterhoeven <geert@linux-m68k.org>
> > > > > Signed-off-by: Ping-Ke Shih <pkshih@realtek.com>
> > > >
> > > > Thanks for your patch!
> > > >
> > > > Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
> > > >
> > > > > --- a/drivers/net/wireless/realtek/rtw89/mac.h
> > > > > +++ b/drivers/net/wireless/realtek/rtw89/mac.h
> > > > > @@ -1456,6 +1456,26 @@ static inline int rtw89_mac_txpwr_write32_mask(struct rtw89_dev *rtwdev,
> > > > > return 0;
> > > > > }
> > > > >
> > > > > +static inline
> > > > > +void rtw89_mac_write_txpwr_ctrl(struct rtw89_dev *rtwdev, u32 reg, u32 mask, u32 val,
> > > > > + u32 mask_en, bool cond)
> > > > > +{
> > > > > + u32 wrt = u32_encode_bits(val, mask);
> > > >
> > > > Nit: you could do without this variable...
Can you change u32_encode_bits() to field_prep() you are going to add?
And take this patch (v1) with your suggestion into your patchset.
Then the code looks better and thing can be smooth.
I will drop my v2 that looks not good.
> > > >
> > > > > + u32 val32;
> > > > > + int ret;
> > > > > +
> > > > > + if (cond)
> > > > > + wrt |= mask_en;
> > > > > +
> > > > > + ret = rtw89_mac_txpwr_read32(rtwdev, RTW89_PHY_0, reg, &val32);
> > > > > + if (ret)
> > > > > + return;
> > > > > +
> > > > > + val32 &= ~(mask | mask_en);
> > > > > + val32 |= wrt;
> > > >
> > > > val32 |= u32_encode_bits(val, mask);
> > > > if (cond)
> > > > cal32 |= mask_en;
> > >
> > > With this change, ARCH arm is failed to build (x86 is well):
> > >
> > > In file included from /build/rtw89/core.h:9,
> > > from /build/rtw89/coex.h:8,
> > > from /build/rtw89/rtw8851b.c:5:
> > > In function 'field_multiplier',
> > > inlined from 'field_mask' at ./include/linux/bitfield.h:170:17,
> > > inlined from 'u32_encode_bits' at ./include/linux/bitfield.h:200:1,
> > > inlined from 'rtw89_mac_write_txpwr_ctrl' at /build/rtw89/mac.h:1468:11:
> > > ./include/linux/bitfield.h:165:17: error: call to '__bad_mask' declared with attribute error: bad
> > bitfield mask
> > > 165 | __bad_mask();
> > > | ^~~~~~~~~~~~
> > > In function 'field_multiplier',
> >
> > Hmm...
> >
> > Note that u32_encode_bits() really requires a constant mask, just
> > like FIELD_PREP(). So probably the compiler can no longer deduce it
> > is called with a constant after restructuring the code...
>
> Do you think I need to add a macro to generate mask with u32_encode_bits()
> and then call this inline function?
>
> Or, compiler can always deduce it in the inline function, and current version
> is okay?
>
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2025-12-12 2:09 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2025-11-17 3:29 [PATCH rtw-next 0/2] wifi: rtw89: adjust code to fit coming field_prep() Ping-Ke Shih
2025-11-17 3:29 ` [PATCH rtw-next 1/2] wifi: rtw89: 8852a: correct field mask of reset DAC/ADC FIFO Ping-Ke Shih
2025-11-18 10:24 ` Geert Uytterhoeven
2025-11-19 0:44 ` Ping-Ke Shih
2025-11-19 8:35 ` Geert Uytterhoeven
2025-11-17 3:29 ` [PATCH rtw-next 2/2] wifi: rtw89: avoid to use not consecutive mask in __write_ctrl() Ping-Ke Shih
2025-11-18 10:40 ` Geert Uytterhoeven
2025-11-19 1:15 ` Ping-Ke Shih
2025-11-19 8:38 ` Geert Uytterhoeven
2025-11-19 8:53 ` Ping-Ke Shih
2025-12-12 2:09 ` Ping-Ke Shih
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).