[PATCHv6 0/2] register-field manipulation macros

[PATCHv6 0/2] register-field manipulation macros

[Jakub Kicinski]

Hi!

This set moves to a global header file macros which I find
very useful and worth popularising.  The basic problem is
that since C bitfields are not very dependable accessing
subfields of registers becomes slightly inconvenient.
It is nice to have the necessary mask and shift operations
wrapped in a macro and have that macro compute shift
at compilation time using FFS.

My implementation follows what Felix Fietkau has done in
mt76.  Hannes Frederic Sowa suggested more use of standard
Linux/GCC functions.  Since the RFC I've also added a 
compile-time check to validate that the value passed to
setters fits in the mask.

I attempted the use of static inlines instead of macros
but it makes GCC < 6.0 barf at the BUILD_BUG_ON()s.
I also noticed that forcing arguments to be u32 for inlines
makes the compiler use 32bit arithmetic where it could
get away with 64bit before (on 64bit machines, obviously).
That's a potential performance concern but probably not
a very practical one today.  Apart from looking "cleaner"
static inlines would have the advantage that we could #undef
the auxiliary macros at the end of the header.

Build bot caught a build failure with -Os set.  AFAICT gcc
did not handle temporary variable I put in the macro
expression too well.  I work around that by defining
__BUILD_BUG_ON_NOT_POWER_OF_2 and using it instead of
BUILD_BUG_ON(!tmp || is_power_of_2(tmp)).

I'm planning to use those macros in another driver soon,
Felix may also use them when upstreaming mt76 if he chooses
to do so.

IMHO if accepted it would be best to push these through
Kalle's wireless drivers' tree, since the only existing
user is in drivers/net/wireless/.

As lowly device driver developers we would greatly
appreciate an Ack or a nod from the experts.

v6:
Do a full rename in patch 2.  CC many people.

v4:
 - add documentation in the header.
v3:
 - don't use variables in statement expressions;
 - use __BUILD_BUG_ON_NOT_POWER_OF_2.
v2:
 - change Felix's email address.

Jakub Kicinski (2):
  add basic register-field manipulation macros
  mt7601u: use linux/bitfield.h

 drivers/net/wireless/mediatek/mt7601u/dma.c     |   2 +-
 drivers/net/wireless/mediatek/mt7601u/dma.h     |  10 +--
 drivers/net/wireless/mediatek/mt7601u/eeprom.c  |  12 +--
 drivers/net/wireless/mediatek/mt7601u/init.c    |   9 +-
 drivers/net/wireless/mediatek/mt7601u/mac.c     |  38 ++++-----
 drivers/net/wireless/mediatek/mt7601u/mcu.c     |  18 ++--
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |   4 +-
 drivers/net/wireless/mediatek/mt7601u/phy.c     |  36 ++++----
 drivers/net/wireless/mediatek/mt7601u/tx.c      |  16 ++--
 drivers/net/wireless/mediatek/mt7601u/util.h    |  77 -----------------
 include/linux/bitfield.h                        | 109 ++++++++++++++++++++++++
 include/linux/bug.h                             |   3 +
 12 files changed, 184 insertions(+), 150 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h
 create mode 100644 include/linux/bitfield.h

-- 
1.9.1

[PATCHv6 1/2] add basic register-field manipulation macros

[Jakub Kicinski]

Common approach to accessing register fields is to define
structures or sets of macros containing mask and shift pair.
Operations on the register are then performed as follows:

 field = (reg >> shift) & mask;

 reg &= ~(mask << shift);
 reg |= (field & mask) << shift;

Defining shift and mask separately is tedious.  Ivo van Doorn
came up with an idea of computing them at compilation time
based on a single shifted mask (later refined by Felix) which
can be used like this:

 #define REG_FIELD 0x000ff000

 field = FIELD_GET(REG_FIELD, reg);

 reg &= ~REG_FIELD;
 reg |= FIELD_PUT(REG_FIELD, field);

FIELD_{GET,PUT} macros take care of finding out what the
appropriate shift is based on compilation time ffs operation.

GENMASK can be used to define registers (which is usually
less error-prone and easier to match with datasheets).

This approach is the most convenient I've seen so to limit code
multiplication let's move the macros to a global header file.
Attempts to use static inlines instead of macros failed due
to false positive triggering of BUILD_BUG_ON()s, especially with
GCC < 6.0.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Dinan Gunawardena <dinan.gunawardena@netronome.com>
---
 include/linux/bitfield.h | 109 +++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/bug.h      |   3 ++
 2 files changed, 112 insertions(+)
 create mode 100644 include/linux/bitfield.h

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
new file mode 100644
index 000000000000..ff9fd0af2ac7
--- /dev/null
+++ b/include/linux/bitfield.h
@@ -0,0 +1,109 @@
+/*
+ * Copyright (C) 2014 Felix Fietkau <nbd@nbd.name>
+ * Copyright (C) 2004 - 2009 Ivo van Doorn <IvDoorn@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _LINUX_BITFIELD_H
+#define _LINUX_BITFIELD_H
+
+#include <asm/types.h>
+#include <linux/bug.h>
+
+#define _bf_shf(x) (__builtin_ffsll(x) - 1)
+
+#define _BF_FIELD_CHECK(_mask, _val)					\
+	({								\
+		BUILD_BUG_ON(!(_mask));					\
+		BUILD_BUG_ON(__builtin_constant_p(_val) ?		\
+			     ~((_mask) >> _bf_shf(_mask)) & (_val) :	\
+			     0);					\
+		__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +			\
+					      (1ULL << _bf_shf(_mask))); \
+	})
+
+/*
+ * Bitfield access macros
+ *
+ * This file contains macros which take as input shifted mask
+ * from which they extract the base mask and shift amount at
+ * compilation time.  There are two separate sets of the macros
+ * one for 32bit registers and one for 64bit ones.
+ *
+ * Fields can be defined using GENMASK (which is usually
+ * less error-prone and easier to match with datasheets).
+ *
+ * FIELD_{GET,PUT} macros are designed to be used with masks which
+ * are compilation time constants.
+ *
+ * Example:
+ *
+ *  #define REG_FIELD_A  GENMASK(6, 0)
+ *  #define REG_FIELD_B  BIT(7)
+ *  #define REG_FIELD_C  GENMASK(15, 8)
+ *  #define REG_FIELD_D  GENMASK(31, 16)
+ *
+ * Get:
+ *  a = FIELD_GET(REG_FIELD_A, reg);
+ *  b = FIELD_GET(REG_FIELD_B, reg);
+ *
+ * Set:
+ *  reg = FIELD_PUT(REG_FIELD_A, 1) |
+ *	  FIELD_PUT(REG_FIELD_B, 0) |
+ *	  FIELD_PUT(REG_FIELD_C, c) |
+ *	  FIELD_PUT(REG_FIELD_D, 0x40);
+ *
+ * Modify:
+ *  reg &= ~REG_FIELD_C;
+ *  reg |= FIELD_PUT(REG_FIELD_C, c);
+ */
+
+/**
+ * FIELD_PUT() - construct a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_val:  value to put in the field
+ *
+ * FIELD_PUT() masks and shifts up the value.  The result should
+ * be combined with other fields of the bitfield using logical OR.
+ */
+#define FIELD_PUT(_mask, _val)					\
+	({							\
+		_BF_FIELD_CHECK(_mask, _val);			\
+		((u32)(_val) << _bf_shf(_mask)) & (_mask);	\
+	})
+
+/**
+ * FIELD_GET() - extract a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_val:  32bit value of entire bitfield
+ *
+ * FIELD_GET() extracts the field specified by @_mask from the
+ * bitfield passed in as @_val.
+ */
+#define FIELD_GET(_mask, _val)					\
+	({							\
+		_BF_FIELD_CHECK(_mask, 0);			\
+		(u32)(((_val) & (_mask)) >> _bf_shf(_mask));	\
+	})
+
+#define FIELD_PUT64(_mask, _val)				\
+	({							\
+		_BF_FIELD_CHECK(_mask, _val);			\
+		((u64)(_val) << _bf_shf(_mask)) & (_mask);	\
+	})
+
+#define FIELD_GET64(_mask, _val)				\
+	({							\
+		_BF_FIELD_CHECK(_mask, 0);			\
+		(u64)(((_val) & (_mask)) >> _bf_shf(_mask));	\
+	})
+
+#endif
diff --git a/include/linux/bug.h b/include/linux/bug.h
index e51b0709e78d..bba5bdae1681 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -13,6 +13,7 @@ enum bug_trap_type {
 struct pt_regs;
 
 #ifdef __CHECKER__
+#define __BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
 #define BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
 #define BUILD_BUG_ON_ZERO(e) (0)
 #define BUILD_BUG_ON_NULL(e) ((void*)0)
@@ -24,6 +25,8 @@ struct pt_regs;
 #else /* __CHECKER__ */
 
 /* 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)
 #define BUILD_BUG_ON_NOT_POWER_OF_2(n)			\
 	BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
 
-- 
1.9.1

[PATCHv6 2/2] mt7601u: use linux/bitfield.h

[Jakub Kicinski]

Use the newly added linux/bitfield.h.

Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
Reviewed-by: Dinan Gunawardena <dinan.gunawardena@netronome.com>
---
 drivers/net/wireless/mediatek/mt7601u/dma.c     |  2 +-
 drivers/net/wireless/mediatek/mt7601u/dma.h     | 10 ++--
 drivers/net/wireless/mediatek/mt7601u/eeprom.c  | 12 ++--
 drivers/net/wireless/mediatek/mt7601u/init.c    |  9 +--
 drivers/net/wireless/mediatek/mt7601u/mac.c     | 38 ++++++------
 drivers/net/wireless/mediatek/mt7601u/mcu.c     | 18 +++---
 drivers/net/wireless/mediatek/mt7601u/mt7601u.h |  4 +-
 drivers/net/wireless/mediatek/mt7601u/phy.c     | 36 ++++++------
 drivers/net/wireless/mediatek/mt7601u/tx.c      | 16 ++---
 drivers/net/wireless/mediatek/mt7601u/util.h    | 77 -------------------------
 10 files changed, 72 insertions(+), 150 deletions(-)
 delete mode 100644 drivers/net/wireless/mediatek/mt7601u/util.h

diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.c b/drivers/net/wireless/mediatek/mt7601u/dma.c
index 57a80cfa39b1..a8bc064bc14f 100644
--- a/drivers/net/wireless/mediatek/mt7601u/dma.c
+++ b/drivers/net/wireless/mediatek/mt7601u/dma.c
@@ -103,7 +103,7 @@ static void mt7601u_rx_process_seg(struct mt7601u_dev *dev, u8 *data,
 
 	if (unlikely(rxwi->zero[0] || rxwi->zero[1] || rxwi->zero[2]))
 		dev_err_once(dev->dev, "Error: RXWI zero fields are set\n");
-	if (unlikely(MT76_GET(MT_RXD_INFO_TYPE, fce_info)))
+	if (unlikely(FIELD_GET(MT_RXD_INFO_TYPE, fce_info)))
 		dev_err_once(dev->dev, "Error: RX path seen a non-pkt urb\n");
 
 	trace_mt_rx(dev, rxwi, fce_info);
diff --git a/drivers/net/wireless/mediatek/mt7601u/dma.h b/drivers/net/wireless/mediatek/mt7601u/dma.h
index 978e8a90b87f..46c6a7860c38 100644
--- a/drivers/net/wireless/mediatek/mt7601u/dma.h
+++ b/drivers/net/wireless/mediatek/mt7601u/dma.h
@@ -18,8 +18,6 @@
 #include <asm/unaligned.h>
 #include <linux/skbuff.h>
 
-#include "util.h"
-
 #define MT_DMA_HDR_LEN			4
 #define MT_RX_INFO_LEN			4
 #define MT_FCE_INFO_LEN			4
@@ -79,9 +77,9 @@ static inline int mt7601u_dma_skb_wrap(struct sk_buff *skb,
 	 */
 
 	info = flags |
-		MT76_SET(MT_TXD_INFO_LEN, round_up(skb->len, 4)) |
-		MT76_SET(MT_TXD_INFO_D_PORT, d_port) |
-		MT76_SET(MT_TXD_INFO_TYPE, type);
+		FIELD_PUT(MT_TXD_INFO_LEN, round_up(skb->len, 4)) |
+		FIELD_PUT(MT_TXD_INFO_D_PORT, d_port) |
+		FIELD_PUT(MT_TXD_INFO_TYPE, type);
 
 	put_unaligned_le32(info, skb_push(skb, sizeof(info)));
 	return skb_put_padto(skb, round_up(skb->len, 4) + 4);
@@ -90,7 +88,7 @@ static inline int mt7601u_dma_skb_wrap(struct sk_buff *skb,
 static inline int
 mt7601u_dma_skb_wrap_pkt(struct sk_buff *skb, enum mt76_qsel qsel, u32 flags)
 {
-	flags |= MT76_SET(MT_TXD_PKT_INFO_QSEL, qsel);
+	flags |= FIELD_PUT(MT_TXD_PKT_INFO_QSEL, qsel);
 	return mt7601u_dma_skb_wrap(skb, WLAN_PORT, DMA_PACKET, flags);
 }
 
diff --git a/drivers/net/wireless/mediatek/mt7601u/eeprom.c b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
index 8d8ee0344f7b..14ac2c0f8838 100644
--- a/drivers/net/wireless/mediatek/mt7601u/eeprom.c
+++ b/drivers/net/wireless/mediatek/mt7601u/eeprom.c
@@ -45,8 +45,8 @@ mt7601u_efuse_read(struct mt7601u_dev *dev, u16 addr, u8 *data,
 	val = mt76_rr(dev, MT_EFUSE_CTRL);
 	val &= ~(MT_EFUSE_CTRL_AIN |
 		 MT_EFUSE_CTRL_MODE);
-	val |= MT76_SET(MT_EFUSE_CTRL_AIN, addr & ~0xf) |
-	       MT76_SET(MT_EFUSE_CTRL_MODE, mode) |
+	val |= FIELD_PUT(MT_EFUSE_CTRL_AIN, addr & ~0xf) |
+	       FIELD_PUT(MT_EFUSE_CTRL_MODE, mode) |
 	       MT_EFUSE_CTRL_KICK;
 	mt76_wr(dev, MT_EFUSE_CTRL, val);
 
@@ -128,8 +128,8 @@ mt7601u_set_chip_cap(struct mt7601u_dev *dev, u8 *eeprom)
 	if (!field_valid(nic_conf0 >> 8))
 		return;
 
-	if (MT76_GET(MT_EE_NIC_CONF_0_RX_PATH, nic_conf0) > 1 ||
-	    MT76_GET(MT_EE_NIC_CONF_0_TX_PATH, nic_conf0) > 1)
+	if (FIELD_GET(MT_EE_NIC_CONF_0_RX_PATH, nic_conf0) > 1 ||
+	    FIELD_GET(MT_EE_NIC_CONF_0_TX_PATH, nic_conf0) > 1)
 		dev_err(dev->dev,
 			"Error: device has more than 1 RX/TX stream!\n");
 }
@@ -150,7 +150,7 @@ mt7601u_set_macaddr(struct mt7601u_dev *dev, const u8 *eeprom)
 
 	mt76_wr(dev, MT_MAC_ADDR_DW0, get_unaligned_le32(dev->macaddr));
 	mt76_wr(dev, MT_MAC_ADDR_DW1, get_unaligned_le16(dev->macaddr + 4) |
-		MT76_SET(MT_MAC_ADDR_DW1_U2ME_MASK, 0xff));
+		FIELD_PUT(MT_MAC_ADDR_DW1_U2ME_MASK, 0xff));
 
 	return 0;
 }
@@ -176,7 +176,7 @@ mt7601u_set_channel_power(struct mt7601u_dev *dev, u8 *eeprom)
 	u8 max_pwr;
 
 	val = mt7601u_rr(dev, MT_TX_ALC_CFG_0);
-	max_pwr = MT76_GET(MT_TX_ALC_CFG_0_LIMIT_0, val);
+	max_pwr = FIELD_GET(MT_TX_ALC_CFG_0_LIMIT_0, val);
 
 	if (mt7601u_has_tssi(dev, eeprom)) {
 		mt7601u_set_channel_target_power(dev, eeprom, max_pwr);
diff --git a/drivers/net/wireless/mediatek/mt7601u/init.c b/drivers/net/wireless/mediatek/mt7601u/init.c
index 8fa78d7156be..f424f51a55a3 100644
--- a/drivers/net/wireless/mediatek/mt7601u/init.c
+++ b/drivers/net/wireless/mediatek/mt7601u/init.c
@@ -108,8 +108,9 @@ static void mt7601u_init_usb_dma(struct mt7601u_dev *dev)
 {
 	u32 val;
 
-	val = MT76_SET(MT_USB_DMA_CFG_RX_BULK_AGG_TOUT, MT_USB_AGGR_TIMEOUT) |
-	      MT76_SET(MT_USB_DMA_CFG_RX_BULK_AGG_LMT, MT_USB_AGGR_SIZE_LIMIT) |
+	val = FIELD_PUT(MT_USB_DMA_CFG_RX_BULK_AGG_TOUT, MT_USB_AGGR_TIMEOUT) |
+	      FIELD_PUT(MT_USB_DMA_CFG_RX_BULK_AGG_LMT,
+			MT_USB_AGGR_SIZE_LIMIT) |
 	      MT_USB_DMA_CFG_RX_BULK_EN |
 	      MT_USB_DMA_CFG_TX_BULK_EN;
 	if (dev->in_max_packet == 512)
@@ -396,8 +397,8 @@ int mt7601u_init_hardware(struct mt7601u_dev *dev)
 
 	mt7601u_rmw(dev, MT_US_CYC_CFG, MT_US_CYC_CNT, 0x1e);
 
-	mt7601u_wr(dev, MT_TXOP_CTRL_CFG, MT76_SET(MT_TXOP_TRUN_EN, 0x3f) |
-					  MT76_SET(MT_TXOP_EXT_CCA_DLY, 0x58));
+	mt7601u_wr(dev, MT_TXOP_CTRL_CFG, FIELD_PUT(MT_TXOP_TRUN_EN, 0x3f) |
+					  FIELD_PUT(MT_TXOP_EXT_CCA_DLY, 0x58));
 
 	ret = mt7601u_eeprom_init(dev);
 	if (ret)
diff --git a/drivers/net/wireless/mediatek/mt7601u/mac.c b/drivers/net/wireless/mediatek/mt7601u/mac.c
index e21c53ed09fb..7f3221ad0bd2 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mac.c
+++ b/drivers/net/wireless/mediatek/mt7601u/mac.c
@@ -19,13 +19,13 @@
 static void
 mt76_mac_process_tx_rate(struct ieee80211_tx_rate *txrate, u16 rate)
 {
-	u8 idx = MT76_GET(MT_TXWI_RATE_MCS, rate);
+	u8 idx = FIELD_GET(MT_TXWI_RATE_MCS, rate);
 
 	txrate->idx = 0;
 	txrate->flags = 0;
 	txrate->count = 1;
 
-	switch (MT76_GET(MT_TXWI_RATE_PHY_MODE, rate)) {
+	switch (FIELD_GET(MT_TXWI_RATE_PHY_MODE, rate)) {
 	case MT_PHY_TYPE_OFDM:
 		txrate->idx = idx + 4;
 		return;
@@ -47,7 +47,7 @@ mt76_mac_process_tx_rate(struct ieee80211_tx_rate *txrate, u16 rate)
 		return;
 	}
 
-	if (MT76_GET(MT_TXWI_RATE_BW, rate) == MT_PHY_BW_40)
+	if (FIELD_GET(MT_TXWI_RATE_BW, rate) == MT_PHY_BW_40)
 		txrate->flags |= IEEE80211_TX_RC_40_MHZ_WIDTH;
 
 	if (rate & MT_TXWI_RATE_SGI)
@@ -125,9 +125,9 @@ u16 mt76_mac_tx_rate_val(struct mt7601u_dev *dev,
 		bw = 0;
 	}
 
-	rateval = MT76_SET(MT_RXWI_RATE_MCS, rate_idx);
-	rateval |= MT76_SET(MT_RXWI_RATE_PHY, phy);
-	rateval |= MT76_SET(MT_RXWI_RATE_BW, bw);
+	rateval = FIELD_PUT(MT_RXWI_RATE_MCS, rate_idx);
+	rateval |= FIELD_PUT(MT_RXWI_RATE_PHY, phy);
+	rateval |= FIELD_PUT(MT_RXWI_RATE_BW, bw);
 	if (rate->flags & IEEE80211_TX_RC_SHORT_GI)
 		rateval |= MT_RXWI_RATE_SGI;
 
@@ -156,9 +156,9 @@ struct mt76_tx_status mt7601u_mac_fetch_tx_status(struct mt7601u_dev *dev)
 	stat.success = !!(val & MT_TX_STAT_FIFO_SUCCESS);
 	stat.aggr = !!(val & MT_TX_STAT_FIFO_AGGR);
 	stat.ack_req = !!(val & MT_TX_STAT_FIFO_ACKREQ);
-	stat.pktid = MT76_GET(MT_TX_STAT_FIFO_PID_TYPE, val);
-	stat.wcid = MT76_GET(MT_TX_STAT_FIFO_WCID, val);
-	stat.rate = MT76_GET(MT_TX_STAT_FIFO_RATE, val);
+	stat.pktid = FIELD_GET(MT_TX_STAT_FIFO_PID_TYPE, val);
+	stat.wcid = FIELD_GET(MT_TX_STAT_FIFO_WCID, val);
+	stat.rate = FIELD_GET(MT_TX_STAT_FIFO_RATE, val);
 
 	return stat;
 }
@@ -270,7 +270,7 @@ void mt7601u_mac_config_tsf(struct mt7601u_dev *dev, bool enable, int interval)
 	}
 
 	val &= ~MT_BEACON_TIME_CFG_INTVAL;
-	val |= MT76_SET(MT_BEACON_TIME_CFG_INTVAL, interval << 4) |
+	val |= FIELD_PUT(MT_BEACON_TIME_CFG_INTVAL, interval << 4) |
 		MT_BEACON_TIME_CFG_TIMER_EN |
 		MT_BEACON_TIME_CFG_SYNC_MODE |
 		MT_BEACON_TIME_CFG_TBTT_EN;
@@ -349,8 +349,8 @@ mt7601u_mac_wcid_setup(struct mt7601u_dev *dev, u8 idx, u8 vif_idx, u8 *mac)
 	u8 zmac[ETH_ALEN] = {};
 	u32 attr;
 
-	attr = MT76_SET(MT_WCID_ATTR_BSS_IDX, vif_idx & 7) |
-	       MT76_SET(MT_WCID_ATTR_BSS_IDX_EXT, !!(vif_idx & 8));
+	attr = FIELD_PUT(MT_WCID_ATTR_BSS_IDX, vif_idx & 7) |
+	       FIELD_PUT(MT_WCID_ATTR_BSS_IDX_EXT, !!(vif_idx & 8));
 
 	mt76_wr(dev, MT_WCID_ATTR(idx), attr);
 
@@ -382,15 +382,15 @@ void mt7601u_mac_set_ampdu_factor(struct mt7601u_dev *dev)
 	rcu_read_unlock();
 
 	mt7601u_wr(dev, MT_MAX_LEN_CFG, 0xa0fff |
-		   MT76_SET(MT_MAX_LEN_CFG_AMPDU, min_factor));
+		   FIELD_PUT(MT_MAX_LEN_CFG_AMPDU, min_factor));
 }
 
 static void
 mt76_mac_process_rate(struct ieee80211_rx_status *status, u16 rate)
 {
-	u8 idx = MT76_GET(MT_RXWI_RATE_MCS, rate);
+	u8 idx = FIELD_GET(MT_RXWI_RATE_MCS, rate);
 
-	switch (MT76_GET(MT_RXWI_RATE_PHY, rate)) {
+	switch (FIELD_GET(MT_RXWI_RATE_PHY, rate)) {
 	case MT_PHY_TYPE_OFDM:
 		if (WARN_ON(idx >= 8))
 			idx = 0;
@@ -436,7 +436,7 @@ mt7601u_rx_monitor_beacon(struct mt7601u_dev *dev, struct mt7601u_rxwi *rxwi,
 			  u16 rate, int rssi)
 {
 	dev->bcn_freq_off = rxwi->freq_off;
-	dev->bcn_phy_mode = MT76_GET(MT_RXWI_RATE_PHY, rate);
+	dev->bcn_phy_mode = FIELD_GET(MT_RXWI_RATE_PHY, rate);
 	dev->avg_rssi = (dev->avg_rssi * 15) / 16 + (rssi << 8);
 }
 
@@ -458,7 +458,7 @@ u32 mt76_mac_process_rx(struct mt7601u_dev *dev, struct sk_buff *skb,
 	u16 rate = le16_to_cpu(rxwi->rate);
 	int rssi;
 
-	len = MT76_GET(MT_RXWI_CTL_MPDU_LEN, ctl);
+	len = FIELD_GET(MT_RXWI_CTL_MPDU_LEN, ctl);
 	if (len < 10)
 		return 0;
 
@@ -542,8 +542,8 @@ int mt76_mac_wcid_set_key(struct mt7601u_dev *dev, u8 idx,
 
 	val = mt7601u_rr(dev, MT_WCID_ATTR(idx));
 	val &= ~MT_WCID_ATTR_PKEY_MODE & ~MT_WCID_ATTR_PKEY_MODE_EXT;
-	val |= MT76_SET(MT_WCID_ATTR_PKEY_MODE, cipher & 7) |
-	       MT76_SET(MT_WCID_ATTR_PKEY_MODE_EXT, cipher >> 3);
+	val |= FIELD_PUT(MT_WCID_ATTR_PKEY_MODE, cipher & 7) |
+	       FIELD_PUT(MT_WCID_ATTR_PKEY_MODE_EXT, cipher >> 3);
 	val &= ~MT_WCID_ATTR_PAIRWISE;
 	val |= MT_WCID_ATTR_PAIRWISE *
 		!!(key && key->flags & IEEE80211_KEY_FLAG_PAIRWISE);
diff --git a/drivers/net/wireless/mediatek/mt7601u/mcu.c b/drivers/net/wireless/mediatek/mt7601u/mcu.c
index 91c4b3427965..57699012e776 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mcu.c
+++ b/drivers/net/wireless/mediatek/mt7601u/mcu.c
@@ -43,8 +43,8 @@ static inline void mt7601u_dma_skb_wrap_cmd(struct sk_buff *skb,
 					    u8 seq, enum mcu_cmd cmd)
 {
 	WARN_ON(mt7601u_dma_skb_wrap(skb, CPU_TX_PORT, DMA_COMMAND,
-				     MT76_SET(MT_TXD_CMD_INFO_SEQ, seq) |
-				     MT76_SET(MT_TXD_CMD_INFO_TYPE, cmd)));
+				     FIELD_PUT(MT_TXD_CMD_INFO_SEQ, seq) |
+				     FIELD_PUT(MT_TXD_CMD_INFO_TYPE, cmd)));
 }
 
 static inline void trace_mt_mcu_msg_send_cs(struct mt7601u_dev *dev,
@@ -100,13 +100,13 @@ static int mt7601u_mcu_wait_resp(struct mt7601u_dev *dev, u8 seq)
 			dev_err(dev->dev, "Error: MCU resp urb failed:%d\n",
 				urb_status);
 
-		if (MT76_GET(MT_RXD_CMD_INFO_CMD_SEQ, rxfce) == seq &&
-		    MT76_GET(MT_RXD_CMD_INFO_EVT_TYPE, rxfce) == CMD_DONE)
+		if (FIELD_GET(MT_RXD_CMD_INFO_CMD_SEQ, rxfce) == seq &&
+		    FIELD_GET(MT_RXD_CMD_INFO_EVT_TYPE, rxfce) == CMD_DONE)
 			return 0;
 
 		dev_err(dev->dev, "Error: MCU resp evt:%hhx seq:%hhx-%hhx!\n",
-			MT76_GET(MT_RXD_CMD_INFO_EVT_TYPE, rxfce),
-			seq, MT76_GET(MT_RXD_CMD_INFO_CMD_SEQ, rxfce));
+			FIELD_GET(MT_RXD_CMD_INFO_EVT_TYPE, rxfce),
+			seq, FIELD_GET(MT_RXD_CMD_INFO_CMD_SEQ, rxfce));
 	}
 
 	dev_err(dev->dev, "Error: %s timed out\n", __func__);
@@ -291,9 +291,9 @@ static int __mt7601u_dma_fw(struct mt7601u_dev *dev,
 	u32 val;
 	int ret;
 
-	reg = cpu_to_le32(MT76_SET(MT_TXD_INFO_TYPE, DMA_PACKET) |
-			  MT76_SET(MT_TXD_INFO_D_PORT, CPU_TX_PORT) |
-			  MT76_SET(MT_TXD_INFO_LEN, len));
+	reg = cpu_to_le32(FIELD_PUT(MT_TXD_INFO_TYPE, DMA_PACKET) |
+			  FIELD_PUT(MT_TXD_INFO_D_PORT, CPU_TX_PORT) |
+			  FIELD_PUT(MT_TXD_INFO_LEN, len));
 	memcpy(buf.buf, &reg, sizeof(reg));
 	memcpy(buf.buf + sizeof(reg), data, len);
 	memset(buf.buf + sizeof(reg) + len, 0, 8);
diff --git a/drivers/net/wireless/mediatek/mt7601u/mt7601u.h b/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
index 428bd2f10b7b..aa2b15fb7289 100644
--- a/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
+++ b/drivers/net/wireless/mediatek/mt7601u/mt7601u.h
@@ -15,6 +15,7 @@
 #ifndef MT7601U_H
 #define MT7601U_H
 
+#include <linux/bitfield.h>
 #include <linux/kernel.h>
 #include <linux/device.h>
 #include <linux/mutex.h>
@@ -24,7 +25,6 @@
 #include <linux/debugfs.h>
 
 #include "regs.h"
-#include "util.h"
 
 #define MT_CALIBRATE_INTERVAL		(4 * HZ)
 
@@ -299,7 +299,7 @@ bool mt76_poll_msec(struct mt7601u_dev *dev, u32 offset, u32 mask, u32 val,
 
 /* Compatibility with mt76 */
 #define mt76_rmw_field(_dev, _reg, _field, _val)	\
-	mt76_rmw(_dev, _reg, _field, MT76_SET(_field, _val))
+	mt76_rmw(_dev, _reg, _field, FIELD_PUT(_field, _val))
 
 static inline u32 mt76_rr(struct mt7601u_dev *dev, u32 offset)
 {
diff --git a/drivers/net/wireless/mediatek/mt7601u/phy.c b/drivers/net/wireless/mediatek/mt7601u/phy.c
index 1908af6add87..a1339c6e01e7 100644
--- a/drivers/net/wireless/mediatek/mt7601u/phy.c
+++ b/drivers/net/wireless/mediatek/mt7601u/phy.c
@@ -41,9 +41,9 @@ mt7601u_rf_wr(struct mt7601u_dev *dev, u8 bank, u8 offset, u8 value)
 		goto out;
 	}
 
-	mt7601u_wr(dev, MT_RF_CSR_CFG, MT76_SET(MT_RF_CSR_CFG_DATA, value) |
-				       MT76_SET(MT_RF_CSR_CFG_REG_BANK, bank) |
-				       MT76_SET(MT_RF_CSR_CFG_REG_ID, offset) |
+	mt7601u_wr(dev, MT_RF_CSR_CFG, FIELD_PUT(MT_RF_CSR_CFG_DATA, value) |
+				       FIELD_PUT(MT_RF_CSR_CFG_REG_BANK, bank) |
+				       FIELD_PUT(MT_RF_CSR_CFG_REG_ID, offset) |
 				       MT_RF_CSR_CFG_WR |
 				       MT_RF_CSR_CFG_KICK);
 	trace_rf_write(dev, bank, offset, value);
@@ -74,17 +74,17 @@ mt7601u_rf_rr(struct mt7601u_dev *dev, u8 bank, u8 offset)
 	if (!mt76_poll(dev, MT_RF_CSR_CFG, MT_RF_CSR_CFG_KICK, 0, 100))
 		goto out;
 
-	mt7601u_wr(dev, MT_RF_CSR_CFG, MT76_SET(MT_RF_CSR_CFG_REG_BANK, bank) |
-				       MT76_SET(MT_RF_CSR_CFG_REG_ID, offset) |
+	mt7601u_wr(dev, MT_RF_CSR_CFG, FIELD_PUT(MT_RF_CSR_CFG_REG_BANK, bank) |
+				       FIELD_PUT(MT_RF_CSR_CFG_REG_ID, offset) |
 				       MT_RF_CSR_CFG_KICK);
 
 	if (!mt76_poll(dev, MT_RF_CSR_CFG, MT_RF_CSR_CFG_KICK, 0, 100))
 		goto out;
 
 	val = mt7601u_rr(dev, MT_RF_CSR_CFG);
-	if (MT76_GET(MT_RF_CSR_CFG_REG_ID, val) == offset &&
-	    MT76_GET(MT_RF_CSR_CFG_REG_BANK, val) == bank) {
-		ret = MT76_GET(MT_RF_CSR_CFG_DATA, val);
+	if (FIELD_GET(MT_RF_CSR_CFG_REG_ID, val) == offset &&
+	    FIELD_GET(MT_RF_CSR_CFG_REG_BANK, val) == bank) {
+		ret = FIELD_GET(MT_RF_CSR_CFG_DATA, val);
 		trace_rf_read(dev, bank, offset, ret);
 	}
 out:
@@ -139,8 +139,8 @@ static void mt7601u_bbp_wr(struct mt7601u_dev *dev, u8 offset, u8 val)
 	}
 
 	mt7601u_wr(dev, MT_BBP_CSR_CFG,
-		   MT76_SET(MT_BBP_CSR_CFG_VAL, val) |
-		   MT76_SET(MT_BBP_CSR_CFG_REG_NUM, offset) |
+		   FIELD_PUT(MT_BBP_CSR_CFG_VAL, val) |
+		   FIELD_PUT(MT_BBP_CSR_CFG_REG_NUM, offset) |
 		   MT_BBP_CSR_CFG_RW_MODE | MT_BBP_CSR_CFG_BUSY);
 	trace_bbp_write(dev, offset, val);
 out:
@@ -163,7 +163,7 @@ static int mt7601u_bbp_rr(struct mt7601u_dev *dev, u8 offset)
 		goto out;
 
 	mt7601u_wr(dev, MT_BBP_CSR_CFG,
-		   MT76_SET(MT_BBP_CSR_CFG_REG_NUM, offset) |
+		   FIELD_PUT(MT_BBP_CSR_CFG_REG_NUM, offset) |
 		   MT_BBP_CSR_CFG_RW_MODE | MT_BBP_CSR_CFG_BUSY |
 		   MT_BBP_CSR_CFG_READ);
 
@@ -171,8 +171,8 @@ static int mt7601u_bbp_rr(struct mt7601u_dev *dev, u8 offset)
 		goto out;
 
 	val = mt7601u_rr(dev, MT_BBP_CSR_CFG);
-	if (MT76_GET(MT_BBP_CSR_CFG_REG_NUM, val) == offset) {
-		ret = MT76_GET(MT_BBP_CSR_CFG_VAL, val);
+	if (FIELD_GET(MT_BBP_CSR_CFG_REG_NUM, val) == offset) {
+		ret = FIELD_GET(MT_BBP_CSR_CFG_VAL, val);
 		trace_bbp_read(dev, offset, ret);
 	}
 out:
@@ -249,9 +249,9 @@ int mt7601u_phy_get_rssi(struct mt7601u_dev *dev,
 			/* bw40 */ { -2, 16, 34 }
 		}
 	};
-	int bw = MT76_GET(MT_RXWI_RATE_BW, rate);
-	int aux_lna = MT76_GET(MT_RXWI_ANT_AUX_LNA, rxwi->ant);
-	int lna_id = MT76_GET(MT_RXWI_GAIN_RSSI_LNA_ID, rxwi->gain);
+	int bw = FIELD_GET(MT_RXWI_RATE_BW, rate);
+	int aux_lna = FIELD_GET(MT_RXWI_ANT_AUX_LNA, rxwi->ant);
+	int lna_id = FIELD_GET(MT_RXWI_GAIN_RSSI_LNA_ID, rxwi->gain);
 	int val;
 
 	if (lna_id) /* LNA id can be 0, 2, 3. */
@@ -259,7 +259,7 @@ int mt7601u_phy_get_rssi(struct mt7601u_dev *dev,
 
 	val = 8;
 	val -= lna[aux_lna][bw][lna_id];
-	val -= MT76_GET(MT_RXWI_GAIN_RSSI_VAL, rxwi->gain);
+	val -= FIELD_GET(MT_RXWI_GAIN_RSSI_VAL, rxwi->gain);
 	val -= dev->ee->lna_gain;
 	val -= dev->ee->rssi_offset[0];
 
@@ -939,7 +939,7 @@ static int mt7601u_tssi_cal(struct mt7601u_dev *dev)
 	dev_dbg(dev->dev, "final diff: %08x\n", diff_pwr);
 
 	val = mt7601u_rr(dev, MT_TX_ALC_CFG_1);
-	curr_pwr = s6_to_int(MT76_GET(MT_TX_ALC_CFG_1_TEMP_COMP, val));
+	curr_pwr = s6_to_int(FIELD_GET(MT_TX_ALC_CFG_1_TEMP_COMP, val));
 	diff_pwr += curr_pwr;
 	val = (val & ~MT_TX_ALC_CFG_1_TEMP_COMP) | int_to_s6(diff_pwr);
 	mt7601u_wr(dev, MT_TX_ALC_CFG_1, val);
diff --git a/drivers/net/wireless/mediatek/mt7601u/tx.c b/drivers/net/wireless/mediatek/mt7601u/tx.c
index a0a33dc8f6bc..cd6966fe4e4e 100644
--- a/drivers/net/wireless/mediatek/mt7601u/tx.c
+++ b/drivers/net/wireless/mediatek/mt7601u/tx.c
@@ -175,11 +175,11 @@ mt7601u_push_txwi(struct mt7601u_dev *dev, struct sk_buff *skb,
 		ba_size = min_t(int, 63, ba_size);
 		if (info->flags & IEEE80211_TX_CTL_RATE_CTRL_PROBE)
 			ba_size = 0;
-		txwi->ack_ctl |= MT76_SET(MT_TXWI_ACK_CTL_BA_WINDOW, ba_size);
+		txwi->ack_ctl |= FIELD_PUT(MT_TXWI_ACK_CTL_BA_WINDOW, ba_size);
 
 		txwi->flags = cpu_to_le16(MT_TXWI_FLAGS_AMPDU |
-					  MT76_SET(MT_TXWI_FLAGS_MPDU_DENSITY,
-						   sta->ht_cap.ampdu_density));
+					  FIELD_PUT(MT_TXWI_FLAGS_MPDU_DENSITY,
+						    sta->ht_cap.ampdu_density));
 		if (info->flags & IEEE80211_TX_CTL_RATE_CTRL_PROBE)
 			txwi->flags = 0;
 	}
@@ -188,7 +188,7 @@ mt7601u_push_txwi(struct mt7601u_dev *dev, struct sk_buff *skb,
 
 	is_probe = !!(info->flags & IEEE80211_TX_CTL_RATE_CTRL_PROBE);
 	pkt_id = mt7601u_tx_pktid_enc(dev, rate_ctl & 0x7, is_probe);
-	pkt_len |= MT76_SET(MT_TXWI_LEN_PKTID, pkt_id);
+	pkt_len |= FIELD_PUT(MT_TXWI_LEN_PKTID, pkt_id);
 	txwi->len_ctl = cpu_to_le16(pkt_len);
 
 	return txwi;
@@ -285,9 +285,9 @@ int mt7601u_conf_tx(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 	WARN_ON(cw_min > 0xf);
 	WARN_ON(cw_max > 0xf);
 
-	val = MT76_SET(MT_EDCA_CFG_AIFSN, params->aifs) |
-	      MT76_SET(MT_EDCA_CFG_CWMIN, cw_min) |
-	      MT76_SET(MT_EDCA_CFG_CWMAX, cw_max);
+	val = FIELD_PUT(MT_EDCA_CFG_AIFSN, params->aifs) |
+	      FIELD_PUT(MT_EDCA_CFG_CWMIN, cw_min) |
+	      FIELD_PUT(MT_EDCA_CFG_CWMAX, cw_max);
 	/* TODO: based on user-controlled EnableTxBurst var vendor drv sets
 	 *	 a really long txop on AC0 (see connect.c:2009) but only on
 	 *	 connect? When not connected should be 0.
@@ -295,7 +295,7 @@ int mt7601u_conf_tx(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
 	if (!hw_q)
 		val |= 0x60;
 	else
-		val |= MT76_SET(MT_EDCA_CFG_TXOP, params->txop);
+		val |= FIELD_PUT(MT_EDCA_CFG_TXOP, params->txop);
 	mt76_wr(dev, MT_EDCA_CFG_AC(hw_q), val);
 
 	val = mt76_rr(dev, MT_WMM_TXOP(hw_q));
diff --git a/drivers/net/wireless/mediatek/mt7601u/util.h b/drivers/net/wireless/mediatek/mt7601u/util.h
deleted file mode 100644
index b89140bf1210..000000000000
--- a/drivers/net/wireless/mediatek/mt7601u/util.h
+++ /dev/null
@@ -1,77 +0,0 @@
-/*
- * Copyright (C) 2014 Felix Fietkau <nbd@openwrt.org>
- * Copyright (C) 2004 - 2009 Ivo van Doorn <IvDoorn@gmail.com>
- *
- * This program is free software; you can redistribute it and/or modify
- * it under the terms of the GNU General Public License version 2
- * as published by the Free Software Foundation
- *
- * This program is distributed in the hope that it will be useful,
- * but WITHOUT ANY WARRANTY; without even the implied warranty of
- * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
- * GNU General Public License for more details.
- */
-
-#ifndef __MT76_UTIL_H
-#define __MT76_UTIL_H
-
-/*
- * Power of two check, this will check
- * if the mask that has been given contains and contiguous set of bits.
- * Note that we cannot use the is_power_of_2() function since this
- * check must be done at compile-time.
- */
-#define is_power_of_two(x)	( !((x) & ((x)-1)) )
-#define low_bit_mask(x)		( ((x)-1) & ~(x) )
-#define is_valid_mask(x)	is_power_of_two(1LU + (x) + low_bit_mask(x))
-
-/*
- * Macros to find first set bit in a variable.
- * These macros behave the same as the __ffs() functions but
- * the most important difference that this is done during
- * compile-time rather then run-time.
- */
-#define compile_ffs2(__x) \
-	__builtin_choose_expr(((__x) & 0x1), 0, 1)
-
-#define compile_ffs4(__x) \
-	__builtin_choose_expr(((__x) & 0x3), \
-			      (compile_ffs2((__x))), \
-			      (compile_ffs2((__x) >> 2) + 2))
-
-#define compile_ffs8(__x) \
-	__builtin_choose_expr(((__x) & 0xf), \
-			      (compile_ffs4((__x))), \
-			      (compile_ffs4((__x) >> 4) + 4))
-
-#define compile_ffs16(__x) \
-	__builtin_choose_expr(((__x) & 0xff), \
-			      (compile_ffs8((__x))), \
-			      (compile_ffs8((__x) >> 8) + 8))
-
-#define compile_ffs32(__x) \
-	__builtin_choose_expr(((__x) & 0xffff), \
-			      (compile_ffs16((__x))), \
-			      (compile_ffs16((__x) >> 16) + 16))
-
-/*
- * This macro will check the requirements for the FIELD{8,16,32} macros
- * The mask should be a constant non-zero contiguous set of bits which
- * does not exceed the given typelimit.
- */
-#define FIELD_CHECK(__mask) \
-	BUILD_BUG_ON(!(__mask) || !is_valid_mask(__mask))
-
-#define MT76_SET(_mask, _val)						\
-	({								\
-		FIELD_CHECK(_mask);					\
-		(((u32) (_val)) << compile_ffs32(_mask)) & _mask;	\
-	})
-
-#define MT76_GET(_mask, _val)						\
-	({								\
-		FIELD_CHECK(_mask);					\
-		(u32) (((_val) & _mask) >> compile_ffs32(_mask));	\
-	})
-
-#endif
-- 
1.9.1

Re: [PATCHv6 1/2] add basic register-field manipulation macros

[Kalle Valo]

Jakub Kicinski <jakub.kicinski@netronome.com> writes:

> Common approach to accessing register fields is to define
> structures or sets of macros containing mask and shift pair.
> Operations on the register are then performed as follows:
>
>  field = (reg >> shift) & mask;
>
>  reg &= ~(mask << shift);
>  reg |= (field & mask) << shift;
>
> Defining shift and mask separately is tedious.  Ivo van Doorn
> came up with an idea of computing them at compilation time
> based on a single shifted mask (later refined by Felix) which
> can be used like this:
>
>  #define REG_FIELD 0x000ff000
>
>  field = FIELD_GET(REG_FIELD, reg);
>
>  reg &= ~REG_FIELD;
>  reg |= FIELD_PUT(REG_FIELD, field);
>
> FIELD_{GET,PUT} macros take care of finding out what the
> appropriate shift is based on compilation time ffs operation.
>
> GENMASK can be used to define registers (which is usually
> less error-prone and easier to match with datasheets).
>
> This approach is the most convenient I've seen so to limit code
> multiplication let's move the macros to a global header file.
> Attempts to use static inlines instead of macros failed due
> to false positive triggering of BUILD_BUG_ON()s, especially with
> GCC < 6.0.
>
> Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
> Reviewed-by: Dinan Gunawardena <dinan.gunawardena@netronome.com>

Are people ok with this? I think they are useful and I can take these
through my tree, but I would prefer to get an ack from other maintainers
first. Dave? Andrew?

Full patches here:

https://patchwork.kernel.org/patch/9284153/

https://patchwork.kernel.org/patch/9284155/

-- 
Kalle Valo

Re: [PATCHv6 1/2] add basic register-field manipulation macros

[Linus Torvalds]

On Wed, Aug 17, 2016 at 3:31 AM, Kalle Valo <kvalo@codeaurora.org> wrote:
>
> Are people ok with this? I think they are useful and I can take these
> through my tree, but I would prefer to get an ack from other maintainers
> first. Dave? Andrew?

I'm not a huge fan, since the interface fundamentally seems to be
oddly designed (why pass in the mask rather than "start bit +
length"?).

I also don't like how this very much would match the GENMASK() macros
we have, but then it clashes with them in other ways

 - it's in a different header file

 - it has completely different naming (GENMASK_ULL vs FIELD_GET64}.

I actually think the naming could/should be fixed by just
automatically doing the right thing based on sizes.  For example,
GENMASK could just have something like

  #define GENMASK(end,start) __builtin_choose_expr((end)>31,
__GENMASK_64(end,start), __GENMASK_32(end,start))

and doing similar things with the FIELD_GET/SET ops.

So I'm not entirely happy about this all.

But if people love the interface, and think the above kind of cleanups
might be possible, I'd just want to make sure that there is also a

       BUILD_BUG_ON(!__builtin_constant_p(_mask));

because if the mask isn't a build-time constant, the code ends up
being *complete* shit. Also preferably something like

       BUILD_BUG_ON((_mask) > (typeof(_val)~0ull);

to make sure you can't try to mask bits that don't exist in the value.

Or something like that to make mis-uses *really* obvious.

The FIELD_PUT macro also seems misnamed. It doesn't "put" anything
(unlike the GET macro). It just prepares the field for inserting
later. As exemplified by how you actually have to put things:

First, "GET" - yeah, that looks like a "get" operation:

 * Get:
 *  a = FIELD_GET(REG_FIELD_A, reg);

But then "PUT" isn't actually a PUT operation at all, but the comments
kind of gloss over it by talking about "Modify" instead:

 * Modify:
 *  reg &= ~REG_FIELD_C;
 *  reg |= FIELD_PUT(REG_FIELD_C, c);

so I'm not entirely sure about the naming.

I can live with the FIELD_PUT naming, because I see how it comes
about, even if I think it's a bit odd. I might have called it
"FIELD_PREP" instead, but I'm not really sure that's all that much
better.

Am I being a bit anal? Yeah. But when we add whole new abstractions
that we haven't used historically, I'd really like those to be obvious
and easy to use (or rather, really _hard_ to get wrong by mistake).

Hmm?

                  Linus

Re: [PATCHv6 1/2] add basic register-field manipulation macros

[Jakub Kicinski]

On Wed, 17 Aug 2016 09:33:26 -0700, Linus Torvalds wrote:
> On Wed, Aug 17, 2016 at 3:31 AM, Kalle Valo <kvalo@codeaurora.org> wrote:
> >
> > Are people ok with this? I think they are useful and I can take these
> > through my tree, but I would prefer to get an ack from other maintainers
> > first. Dave? Andrew?  
> 
> I'm not a huge fan, since the interface fundamentally seems to be
> oddly designed (why pass in the mask rather than "start bit +
> length"?).

Would that not require start and length to have separate defines?

I assume doing:

    #define REG_BLA_FIELD_FOO  3, 4
    val = FIELD_GET(REG_BLA_FIELD_FOO, reg);

is not acceptable.  Attempts to define a single value brought us to the
shifted mask.

> I also don't like how this very much would match the GENMASK() macros
> we have, but then it clashes with them in other ways
> 
>  - it's in a different header file

I'll move to bitops.h.

>  - it has completely different naming (GENMASK_ULL vs FIELD_GET64}.
> 
> I actually think the naming could/should be fixed by just
> automatically doing the right thing based on sizes.  For example,
> GENMASK could just have something like
> 
>   #define GENMASK(end,start) __builtin_choose_expr((end)>31,
> __GENMASK_64(end,start), __GENMASK_32(end,start))
> 
> and doing similar things with the FIELD_GET/SET ops.
> 
> So I'm not entirely happy about this all.

Seems feasible.

> But if people love the interface, and think the above kind of cleanups
> might be possible, I'd just want to make sure that there is also a
> 
>        BUILD_BUG_ON(!__builtin_constant_p(_mask));
> 
> because if the mask isn't a build-time constant, the code ends up
> being *complete* shit. Also preferably something like
> 
>        BUILD_BUG_ON((_mask) > (typeof(_val)~0ull);
> 
> to make sure you can't try to mask bits that don't exist in the value.
> 
> Or something like that to make mis-uses *really* obvious.

OK!

> The FIELD_PUT macro also seems misnamed. It doesn't "put" anything
> (unlike the GET macro). It just prepares the field for inserting
> later. As exemplified by how you actually have to put things:
> 
> First, "GET" - yeah, that looks like a "get" operation:
> 
>  * Get:
>  *  a = FIELD_GET(REG_FIELD_A, reg);
> 
> But then "PUT" isn't actually a PUT operation at all, but the comments
> kind of gloss over it by talking about "Modify" instead:
> 
>  * Modify:
>  *  reg &= ~REG_FIELD_C;
>  *  reg |= FIELD_PUT(REG_FIELD_C, c);
> 
> so I'm not entirely sure about the naming.
> 
> I can live with the FIELD_PUT naming, because I see how it comes
> about, even if I think it's a bit odd. I might have called it
> "FIELD_PREP" instead, but I'm not really sure that's all that much
> better.

Yes, it used to be called FIELD_SET, which was even worse.  I think
PREP sounds good.

Thanks!

Re: [PATCHv6 1/2] add basic register-field manipulation macros

[Linus Torvalds]

On Wed, Aug 17, 2016 at 10:11 AM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> On Wed, 17 Aug 2016 09:33:26 -0700, Linus Torvalds wrote:
>>
>> I'm not a huge fan, since the interface fundamentally seems to be
>> oddly designed (why pass in the mask rather than "start bit +
>> length"?).
>
> Would that not require start and length to have separate defines?

Yeah.

> I assume doing:
>
>     #define REG_BLA_FIELD_FOO  3, 4
>     val = FIELD_GET(REG_BLA_FIELD_FOO, reg);
>
> is not acceptable.  Attempts to define a single value brought us to the
> shifted mask.

Agreed. Maybe the mask with the complexity to then undo it (at compile
time) is the better approach.

            Linus

[RFC (v7)] add basic register-field manipulation macros

[Jakub Kicinski]

Hi!

This is what I came up with.  Changes:
 - drop the explicit type marking (u32/u64) - depend on the type
   of the mask instead;
 - only allow compilation time constant masks;
 - barf at "type of register too small to ever match mask" on get;
 - rename PUT -> PREP.

I did not put the new marcos in bitops.h (where GENMASK lives)
because of the dependency on bug.h.  If I did there would be
a circular dependency like this:

bitops.h -> linux/bug.h -> asm-generic/bug.h -> kernel.h -> bitops.h

Which makes things really ugly.  I tried to remove the kernel.h
include from bug.h by separating panic-related definitions into
a new header.  There were still cycles coming from the notifier.h
which I had to include for panic notifier...

Reviewed-by: Dinan Gunawardena <dinan.gunawardena@netronome.com>
Signed-off-by: Jakub Kicinski <jakub.kicinski@netronome.com>
---
 include/linux/bitfield.h | 93 ++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/bug.h      |  3 ++
 2 files changed, 96 insertions(+)
 create mode 100644 include/linux/bitfield.h

diff --git a/include/linux/bitfield.h b/include/linux/bitfield.h
new file mode 100644
index 000000000000..32ca8863e66d
--- /dev/null
+++ b/include/linux/bitfield.h
@@ -0,0 +1,93 @@
+/*
+ * Copyright (C) 2014 Felix Fietkau <nbd@nbd.name>
+ * Copyright (C) 2004 - 2009 Ivo van Doorn <IvDoorn@gmail.com>
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2
+ * as published by the Free Software Foundation
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#ifndef _LINUX_BITFIELD_H
+#define _LINUX_BITFIELD_H
+
+#include <linux/bug.h>
+
+/*
+ * Bitfield access macros
+ *
+ * FIELD_{GET,PREP} macros take as first parameter shifted mask
+ * from which they extract the base mask and shift amount.
+ * Mask must be a compilation time constant.
+ *
+ * Example:
+ *
+ *  #define REG_FIELD_A  GENMASK(6, 0)
+ *  #define REG_FIELD_B  BIT(7)
+ *  #define REG_FIELD_C  GENMASK(15, 8)
+ *  #define REG_FIELD_D  GENMASK(31, 16)
+ *
+ * Get:
+ *  a = FIELD_GET(REG_FIELD_A, reg);
+ *  b = FIELD_GET(REG_FIELD_B, reg);
+ *
+ * Set:
+ *  reg = FIELD_PREP(REG_FIELD_A, 1) |
+ *	  FIELD_PREP(REG_FIELD_B, 0) |
+ *	  FIELD_PREP(REG_FIELD_C, c) |
+ *	  FIELD_PREP(REG_FIELD_D, 0x40);
+ *
+ * Modify:
+ *  reg &= ~REG_FIELD_C;
+ *  reg |= FIELD_PREP(REG_FIELD_C, c);
+ */
+
+#define _bf_shf(x) (__builtin_ffsll(x) - 1)
+
+#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), _pfx "mask is zero");	\
+		BUILD_BUG_ON_MSG(__builtin_constant_p(_val) ?		\
+				 ~((_mask) >> _bf_shf(_mask)) & (_val) : 0, \
+				 _pfx "value too large for the field"); \
+		BUILD_BUG_ON_MSG((_mask) > (typeof(_reg))~0ull,		\
+				 _pfx "type of reg too small for mask"); \
+		__BUILD_BUG_ON_NOT_POWER_OF_2((_mask) +			\
+					      (1ULL << _bf_shf(_mask))); \
+	})
+
+/**
+ * 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.
+ */
+#define FIELD_PREP(_mask, _val)						\
+	({								\
+		_BF_FIELD_CHECK(_mask, 0ULL, _val, "FIELD_PREP: ");	\
+		((typeof(_mask))(_val) << _bf_shf(_mask)) & (_mask);	\
+	})
+
+/**
+ * FIELD_GET() - extract a bitfield element
+ * @_mask: shifted mask defining the field's length and position
+ * @_reg:  32bit 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.
+ */
+#define FIELD_GET(_mask, _reg)						\
+	({								\
+		_BF_FIELD_CHECK(_mask, _reg, 0U, "FIELD_GET: ");	\
+		(typeof(_mask))(((_reg) & (_mask)) >> _bf_shf(_mask));	\
+	})
+
+#endif
diff --git a/include/linux/bug.h b/include/linux/bug.h
index e51b0709e78d..292d6a10b0c2 100644
--- a/include/linux/bug.h
+++ b/include/linux/bug.h
@@ -13,6 +13,7 @@ enum bug_trap_type {
 struct pt_regs;
 
 #ifdef __CHECKER__
+#define __BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
 #define BUILD_BUG_ON_NOT_POWER_OF_2(n) (0)
 #define BUILD_BUG_ON_ZERO(e) (0)
 #define BUILD_BUG_ON_NULL(e) ((void*)0)
@@ -24,6 +25,8 @@ struct pt_regs;
 #else /* __CHECKER__ */
 
 /* 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)
 #define BUILD_BUG_ON_NOT_POWER_OF_2(n)			\
 	BUILD_BUG_ON((n) == 0 || (((n) & ((n) - 1)) != 0))
 
-- 
1.9.1

Re: [RFC (v7)] add basic register-field manipulation macros

[Linus Torvalds]

On Thu, Aug 18, 2016 at 10:11 AM, Jakub Kicinski
<jakub.kicinski@netronome.com> wrote:
> Hi!
>
> This is what I came up with.  Changes:

I can live with this, certainly. I'm not really sure how many drivers
(or perhaps core code, for that matter) will actually start using it,
but it at least _looks_ like a usable interface that seems to be quite
resistant to people doing stupid things with it that would result in
surprising results (either performance or semantics).

So I'm ok with something like this coming through (for example) the
wireless tree if the drivers there are the first ones to start using
this.

Let's see if anybody else objects.

               Linus

Re: [RFC (v7)] add basic register-field manipulation macros

[Kalle Valo]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Thu, Aug 18, 2016 at 10:11 AM, Jakub Kicinski
> <jakub.kicinski@netronome.com> wrote:
>> Hi!
>>
>> This is what I came up with.  Changes:
>
> I can live with this, certainly. I'm not really sure how many drivers
> (or perhaps core code, for that matter) will actually start using it,
> but it at least _looks_ like a usable interface that seems to be quite
> resistant to people doing stupid things with it that would result in
> surprising results (either performance or semantics).

I'm guessing that at least in ath9k and ath10k there would be use of
these macros.

> So I'm ok with something like this coming through (for example) the
> wireless tree if the drivers there are the first ones to start using
> this.
>
> Let's see if anybody else objects.

Great, thanks for the help. Let's wait for other comments and Jakub can
then resend this without RFC. I can then take it through my tree.

-- 
Kalle Valo