* Re: [PATCH] cfg80211: Add HT and VHT information in start_ap
From: Malinen, Jouni @ 2016-08-16 12:34 UTC (permalink / raw)
To: Johannes Berg; +Cc: linux-wireless@vger.kernel.org, Xu, Peng
In-Reply-To: <1471330367.16783.23.camel@sipsolutions.net>
On Tue, Aug 16, 2016 at 08:52:47AM +0200, Johannes Berg wrote:
> On Mon, 2016-08-15 at 21:07 +0300, Jouni Malinen wrote:
> > From: Peng Xu <pxu@qca.qualcomm.com>
> > Add HT and VHT information in struct cfg80211_ap_settings when
> > starting ap so that driver does not need to parse IE to obtain
> > the information.
>=20
> > +enum ht_vht_support {
> > + HT_VHT_DISABLED,
> > + HT_VHT_ENABLED,
> > + HT_VHT_NOT_INDICATED
> > +};
>=20
> So if you get HT_VHT_NOT_INDICATED in the driver, don't you *still*
> have to parse the IEs?
Well.. Yes, I guess one would need to do that for some time until
relevant user space is expected to have been updated to support the new
attribute.
> Arguably, cfg80211 could know itself by parsing though, so it could
> already fall back to that, no?
>=20
> But if you do that, you already need the parsing code, so then perhaps
> it would make sense to just always use the parsing in cfg80211? Or
> export a parsing function to use in driver(s)?
I guess that could be considered reasonable approach for the existing
HT and VHT cases and new attributes would obviously be significantly
easier to introduce with new extensions (need to remember to do this for
HE from the beginning..).
The parsing for these HT/VHT enabled/required is a bit strange
combination having to go over three IEs. I'm not sure a parsing function
to do so would be that nice.. The parsing code could indeed be moved to
cfg80211 so that it would not need to be duplicated into each driver
needing this, though.
--=20
Jouni Malinen PGP id EFC895FA=
^ permalink raw reply
* [PATCHv6 1/2] add basic register-field manipulation macros
From: Jakub Kicinski @ 2016-08-16 15:18 UTC (permalink / raw)
To: torvalds, akpm, gregkh, davem, kvalo
Cc: linux-wireless, linux-kernel, dinan.gunawardena, Jakub Kicinski
In-Reply-To: <1471360704-10507-1-git-send-email-jakub.kicinski@netronome.com>
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
^ permalink raw reply related
* [PATCHv6 2/2] mt7601u: use linux/bitfield.h
From: Jakub Kicinski @ 2016-08-16 15:18 UTC (permalink / raw)
To: torvalds, akpm, gregkh, davem, kvalo
Cc: linux-wireless, linux-kernel, dinan.gunawardena, Jakub Kicinski
In-Reply-To: <1471360704-10507-1-git-send-email-jakub.kicinski@netronome.com>
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, ®, 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
^ permalink raw reply related
* [PATCHv6 0/2] register-field manipulation macros
From: Jakub Kicinski @ 2016-08-16 15:18 UTC (permalink / raw)
To: torvalds, akpm, gregkh, davem, kvalo
Cc: linux-wireless, linux-kernel, dinan.gunawardena, 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
^ permalink raw reply
* Re: [PATCH v3] nl80211: Allow GET_INTERFACE dumps to be filtered
From: Denis Kenzior @ 2016-08-16 15:26 UTC (permalink / raw)
To: Johannes Berg, linux-wireless
In-Reply-To: <1471332335-9251-1-git-send-email-johannes@sipsolutions.net>
Hi Johannes,
On 08/16/2016 02:25 AM, Johannes Berg wrote:
> From: Denis Kenzior <denkenz@gmail.com>
>
> This patch allows GET_INTERFACE dumps to be filtered based on
> NL80211_ATTR_WIPHY or NL80211_ATTR_WDEV. The documentation for
> GET_INTERFACE mentions that this is possible:
> "Request an interface's configuration; either a dump request on
> a %NL80211_ATTR_WIPHY or ..."
>
> However, this behavior has not been implemented until now.
>
> Johannes: rewrite most of the patch:
> * use nl80211_dump_wiphy_parse() to also allow passing an interface
> to be able to dump its siblings
> * fix locking (must hold rtnl around using nl80211_fam.attrbuf)
> * make init self-contained instead of relying on other cb->args
>
> Signed-off-by: Denis Kenzior <denkenz@gmail.com>
> Signed-off-by: Johannes Berg <johannes.berg@intel.com>
> ---
> net/wireless/nl80211.c | 27 +++++++++++++++++++++++++++
> 1 file changed, 27 insertions(+)
>
Looks good to me.
Regards,
-Denis
^ permalink raw reply
* Re: [PATCH 2/2] mwifiex: fix unaligned read in mwifiex_config_scan()
From: Petri Gynther @ 2016-08-16 17:47 UTC (permalink / raw)
To: linux-wireless
Cc: kvalo, David Miller, Joe Perches, Amitkumar Karwar, Petri Gynther
In-Reply-To: <1471057200-58166-2-git-send-email-pgynther@google.com>
On Fri, Aug 12, 2016 at 8:00 PM, Petri Gynther <pgynther@google.com> wrote:
> $ iwconfig mlan0 essid MySSID
> [ 36.930000] Path: /sbin/iwconfig
> [ 36.930000] CPU: 0 PID: 203 Comm: iwconfig Not tainted 4.7.0 #2
> [ 36.940000] task: 866f83a0 ti: 866a6000 task.ti: 866a6000
> [ 36.940000]
> [ECR ]: 0x00230400 => Misaligned r/w from 0x8677f403
> [ 36.960000] [EFA ]: 0x8677f403
> [ 36.960000] [BLINK ]: mwifiex_scan_networks+0x17a/0x198c [mwifiex]
> [ 36.960000] [ERET ]: mwifiex_scan_networks+0x18a/0x198c [mwifiex]
> [ 36.980000] [STAT32]: 0x00000206 : K E2 E1
> [ 36.980000] BTA: 0x700736e2 SP: 0x866a7d0c FP: 0x5faddc84
> [ 37.000000] LPS: 0x806a37ec LPE: 0x806a37fa LPC: 0x00000000
> [ 37.000000] r00: 0x8677f401 r01: 0x8668aa08 r02: 0x00000001
> r03: 0x00000000 r04: 0x8668b600 r05: 0x8677f406
> r06: 0x8702b600 r07: 0x00000000 r08: 0x8702b600
> r09: 0x00000000 r10: 0x870b3b00 r11: 0x00000000
> r12: 0x00000000
> [ 37.040000]
> [ 37.040000] Stack Trace:
> [ 37.040000] mwifiex_scan_networks+0x18a/0x198c [mwifiex]
>
> Root cause:
> mwifiex driver calls is_zero_ether_addr() against byte-aligned address:
>
> drivers/net/wireless/marvell/mwifiex/fw.h:
> struct mwifiex_scan_cmd_config {
> /*
> * BSS mode to be sent in the firmware command
> */
> u8 bss_mode;
>
> /* Specific BSSID used to filter scan results in the firmware */
> u8 specific_bssid[ETH_ALEN];
>
> ...
> } __packed;
>
> drivers/net/wireless/marvell/mwifiex/scan.c:
> mwifiex_config_scan(..., struct mwifiex_scan_cmd_config *scan_cfg_out, ...)
> ...
> if (adapter->ext_scan &&
> !is_zero_ether_addr(scan_cfg_out->specific_bssid)) {
> ...
> }
>
> Since firmware-related struct mwifiex_scan_cmd_config cannot be changed,
> we need to use the new function is_zero_ether_addr_unaligned() here.
>
> This is v2 of the original patch:
> [PATCH] Modify is_zero_ether_addr() to handle byte-aligned addresses
>
> Per Joe's suggestion -- instead of modifying is_zero_ether_addr() --
> add is_zero_ether_addr_unaligned() and use it where needed.
>
> Cc: Kalle Valo <kvalo@codeaurora.org>
> Cc: David S. Miller <davem@davemloft.net>
> Cc: Joe Perches <joe@perches.com>
> Cc: Amitkumar Karwar <akarwar@marvell.com>
> Signed-off-by: Petri Gynther <pgynther@google.com>
> ---
> drivers/net/wireless/marvell/mwifiex/scan.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/wireless/marvell/mwifiex/scan.c b/drivers/net/wireless/marvell/mwifiex/scan.c
> index bc5e52c..d648c88 100644
> --- a/drivers/net/wireless/marvell/mwifiex/scan.c
> +++ b/drivers/net/wireless/marvell/mwifiex/scan.c
> @@ -883,7 +883,8 @@ mwifiex_config_scan(struct mwifiex_private *priv,
> sizeof(scan_cfg_out->specific_bssid));
>
> if (adapter->ext_scan &&
> - !is_zero_ether_addr(scan_cfg_out->specific_bssid)) {
> + !is_zero_ether_addr_unaligned(
> + scan_cfg_out->specific_bssid)) {
Any comments? Is this approach of adding
is_zero_ether_addr_unaligned() fine? We already have similar routine
ether_addr_equal_unaligned().
I don't see much benefit making a local, aligned copy here. It would
have to use memcpy w/ byte operations anyways and then still run
is_zero_ether_addr().
Amitkumar -- Is it possible to modify struct mwifiex_scan_cmd_config
{} and align specific_bssid field to u16 boundary?
> bssid_tlv =
> (struct mwifiex_ie_types_bssid_list *)tlv_pos;
> bssid_tlv->header.type = cpu_to_le16(TLV_TYPE_BSSID);
> --
> 2.8.0.rc3.226.g39d4020
>
^ permalink raw reply
* Re: regarding RTL8188SU
From: Christian Lamparter @ 2016-08-16 18:14 UTC (permalink / raw)
To: Sambhu R; +Cc: linux-wireless
In-Reply-To: <1902731527.3857839.1471349378230.JavaMail.zimbra@am.amrita.edu>
Hello,
On Tuesday, August 16, 2016 5:39:38 PM CEST Sambhu R wrote:
> I was following your--> https://github.com/chunkeey/rtl8192su but i couldn't
> install. When i execute ./install.sh i'm getting the following error.
There's no install.sh file in this repository?!
>
> make[1]: Entering directory '/usr/src/linux-headers-3.4.112-sun8i'
> Makefile:568: /usr/src/linux-headers-3.4.112-sun8i/arch/armv7l/Makefile: No such file or directory
> make[1]: *** No rule to make target '/usr/src/linux-headers-3.4.112-sun8i/arch/armv7l/Makefile'. Stop.
> make[1]: Leaving directory '/usr/src/linux-headers-3.4.112-sun8i'
> Makefile:220: recipe for target 'modules' failed
> make: *** [modules] Error 2
> ##################################################
> Compile make driver error: 2
> Please check error Mesg
> ##################################################
>
> please help..
Please look at the project's README.md [0].
"This is "a" driver repository for the WIP rtl8192su for any
interested developer.
To build the driver you will need to have a recent and compatible kernel
source. Currently, only kernels built from the latest wireless-testing.git
are supported. If you want to know more about wireless-testing.git then
visit Linux wireless wiki's Git-Guide [1]."
[0] <https://github.com/chunkeey/rtl8192su#readme>
[1] <https://wireless.wiki.kernel.org/en/developers/Documentation/git-guide>
^ permalink raw reply
* Re: [Make-wifi-fast] On the ath9k performance regression with FQ and crypto
From: Eric Dumazet @ 2016-08-16 20:47 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: make-wifi-fast, linux-wireless, Felix Fietkau
In-Reply-To: <87pop85tvr.fsf@toke.dk>
Do you have tcpdumps of
1) sample with crypto
2) sample without crypto.
Looks like some TCP Small queue interaction with skb->truesize, if GSO
is involved, or encapsulation adding overhead.
On Tue, 2016-08-16 at 22:41 +0200, Toke Høiland-Jørgensen wrote:
> So Dave and I have been spending the last couple of days trying to
> narrow down why there's a performance regression in some cases on ath9k
> with the softq-FQ patches. Felix first noticed this regression, and LEDE
> currently carries a patch [1] to disable the FQ portion of the softq
> patches to avoid it.
>
> While we have been able to narrow it down a little bit, no solution has
> been forthcoming, so this is an attempt to describe the bug in the hope
> that someone else will have an idea about what could be causing it.
>
> What we're seeing is the following (when the access point is running
> ath9k with the softq patches):
>
> When running two or more flows to a station, their combined throughput
> will be roughly 20-30% lower than the throughput of a single flow to the
> same station. This happens:
>
> - for both TCP and UDP traffic.
> - independent of the base rate (i.e. signal quality).
> - but only with crypto enabled (WPA2 CCMP in this case).
>
> However, the regression completely disappears if either of the
> following is true:
>
> - no crypto is enabled.
> - the FQ part of mac80211 is disabled (as in [1]).
>
> We have been able to reproduce this behaviour on two different ath9k
> hardware chips and two different architectures.
>
> The cause of the regression seems to be that the aggregates are smaller
> when there are two flows than when there is only one. Adding debug
> statements to the aggregate forming code indicates that this is because
> no more packets are available when the aggregates are built (i.e.
> ieee80211_tx_dequeue() returns NULL).
>
> We have not been able to determine why the queues run empty when this
> combination of circumstances arise. Since we easily get upwards of 120
> Mbps of TCP throughput without crypto but with full FQ, it's clearly not
> the hashing overhead in itself that does it (and the hashing also
> happens with just one flow, so the overhead is still there). And the
> crypto itself should be offloaded to hardware (shouldn't it? we do see a
> marked drop in overall throughput from just enabling crypto), so how
> would the queueing (say, mixing of packets from different flows)
> influence that?
>
> Does anyone have any ideas? We are stumped...
>
> -Toke
>
> [1] https://git.lede-project.org/?p=lede/nbd/staging.git;a=blob;f=package/kernel/mac80211/patches/220-fq_disable_hack.patch;h=7f420beea56335d5043de6fd71b5febae3e9bd79;hb=HEAD
> _______________________________________________
> Make-wifi-fast mailing list
> Make-wifi-fast@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/make-wifi-fast
^ permalink raw reply
* On the ath9k performance regression with FQ and crypto
From: Toke Høiland-Jørgensen @ 2016-08-16 20:41 UTC (permalink / raw)
To: make-wifi-fast, linux-wireless; +Cc: Felix Fietkau, Michal Kazior, Dave Taht
So Dave and I have been spending the last couple of days trying to
narrow down why there's a performance regression in some cases on ath9k
with the softq-FQ patches. Felix first noticed this regression, and LEDE
currently carries a patch [1] to disable the FQ portion of the softq
patches to avoid it.
While we have been able to narrow it down a little bit, no solution has
been forthcoming, so this is an attempt to describe the bug in the hope
that someone else will have an idea about what could be causing it.
What we're seeing is the following (when the access point is running
ath9k with the softq patches):
When running two or more flows to a station, their combined throughput
will be roughly 20-30% lower than the throughput of a single flow to the
same station. This happens:
- for both TCP and UDP traffic.
- independent of the base rate (i.e. signal quality).
- but only with crypto enabled (WPA2 CCMP in this case).
However, the regression completely disappears if either of the
following is true:
- no crypto is enabled.
- the FQ part of mac80211 is disabled (as in [1]).
We have been able to reproduce this behaviour on two different ath9k
hardware chips and two different architectures.
The cause of the regression seems to be that the aggregates are smaller
when there are two flows than when there is only one. Adding debug
statements to the aggregate forming code indicates that this is because
no more packets are available when the aggregates are built (i.e.
ieee80211_tx_dequeue() returns NULL).
We have not been able to determine why the queues run empty when this
combination of circumstances arise. Since we easily get upwards of 120
Mbps of TCP throughput without crypto but with full FQ, it's clearly not
the hashing overhead in itself that does it (and the hashing also
happens with just one flow, so the overhead is still there). And the
crypto itself should be offloaded to hardware (shouldn't it? we do see a
marked drop in overall throughput from just enabling crypto), so how
would the queueing (say, mixing of packets from different flows)
influence that?
Does anyone have any ideas? We are stumped...
-Toke
[1] https://git.lede-project.org/?p=lede/nbd/staging.git;a=blob;f=package/kernel/mac80211/patches/220-fq_disable_hack.patch;h=7f420beea56335d5043de6fd71b5febae3e9bd79;hb=HEAD
^ permalink raw reply
* Re: [Make-wifi-fast] On the ath9k performance regression with FQ and crypto
From: Dave Täht @ 2016-08-16 23:16 UTC (permalink / raw)
To: Eric Dumazet, Toke Høiland-Jørgensen
Cc: make-wifi-fast, linux-wireless, Felix Fietkau
In-Reply-To: <1471380444.4943.17.camel@edumazet-glaptop3.roam.corp.google.com>
On 8/16/16 10:47 PM, Eric Dumazet wrote:
>
> Do you have tcpdumps of
>
> 1) sample with crypto
>
> 2) sample without crypto.
decrypted aircaps (ssid: borgen-public key: mysecret) for 1 flow and for
2 flows are at:
http://www.taht.net/~d/fqcryptbug/
There are also regular captures...
flent results for all test scenarios comparison graphed here:
http://www.taht.net/~d/fqcryptbug/cryptvsfqwndr3800.svg
Total throughput degrades somewhat relative of the total number of flows
in the crypted scenario - 80 mbits total with one flow. ~35 with 12.
(elsewhere: 120mbit without encryption, with fq, any number of flows,
and you can see codel working at least somewhat)
>
> Looks like some TCP Small queue interaction with skb->truesize, if GSO
> is involved, or encapsulation adding overhead.
My own suspicion has been around breaking the block ack window, or on
misunderstanding how complex aggregates are hw/sw retried.
>
>
> On Tue, 2016-08-16 at 22:41 +0200, Toke Høiland-Jørgensen wrote:
>> So Dave and I have been spending the last couple of days trying to
>> narrow down why there's a performance regression in some cases on ath9k
>> with the softq-FQ patches. Felix first noticed this regression, and LEDE
>> currently carries a patch [1] to disable the FQ portion of the softq
>> patches to avoid it.
>>
>> While we have been able to narrow it down a little bit, no solution has
>> been forthcoming, so this is an attempt to describe the bug in the hope
>> that someone else will have an idea about what could be causing it.
>>
>> What we're seeing is the following (when the access point is running
>> ath9k with the softq patches):
>>
>> When running two or more flows to a station, their combined throughput
>> will be roughly 20-30% lower than the throughput of a single flow to the
>> same station. This happens:
>>
>> - for both TCP and UDP traffic.
>> - independent of the base rate (i.e. signal quality).
>> - but only with crypto enabled (WPA2 CCMP in this case).
>>
>> However, the regression completely disappears if either of the
>> following is true:
>>
>> - no crypto is enabled.
>> - the FQ part of mac80211 is disabled (as in [1]).
>>
>> We have been able to reproduce this behaviour on two different ath9k
>> hardware chips and two different architectures.
>>
>> The cause of the regression seems to be that the aggregates are smaller
>> when there are two flows than when there is only one. Adding debug
>> statements to the aggregate forming code indicates that this is because
>> no more packets are available when the aggregates are built (i.e.
>> ieee80211_tx_dequeue() returns NULL).
>>
>> We have not been able to determine why the queues run empty when this
>> combination of circumstances arise. Since we easily get upwards of 120
>> Mbps of TCP throughput without crypto but with full FQ, it's clearly not
>> the hashing overhead in itself that does it (and the hashing also
>> happens with just one flow, so the overhead is still there). And the
>> crypto itself should be offloaded to hardware (shouldn't it? we do see a
>> marked drop in overall throughput from just enabling crypto), so how
>> would the queueing (say, mixing of packets from different flows)
>> influence that?
>>
>> Does anyone have any ideas? We are stumped...
>>
>> -Toke
>>
>> [1] https://git.lede-project.org/?p=lede/nbd/staging.git;a=blob;f=package/kernel/mac80211/patches/220-fq_disable_hack.patch;h=7f420beea56335d5043de6fd71b5febae3e9bd79;hb=HEAD
>> _______________________________________________
>> Make-wifi-fast mailing list
>> Make-wifi-fast@lists.bufferbloat.net
>> https://lists.bufferbloat.net/listinfo/make-wifi-fast
>
>
> _______________________________________________
> Make-wifi-fast mailing list
> Make-wifi-fast@lists.bufferbloat.net
> https://lists.bufferbloat.net/listinfo/make-wifi-fast
>
^ permalink raw reply
* Re: On the ath9k performance regression with FQ and crypto
From: Felix Fietkau @ 2016-08-17 4:18 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, make-wifi-fast, linux-wireless
Cc: Michal Kazior, Dave Taht
In-Reply-To: <87pop85tvr.fsf@toke.dk>
On 2016-08-16 22:41, Toke Høiland-Jørgensen wrote:
> So Dave and I have been spending the last couple of days trying to
> narrow down why there's a performance regression in some cases on ath9k
> with the softq-FQ patches. Felix first noticed this regression, and LEDE
> currently carries a patch [1] to disable the FQ portion of the softq
> patches to avoid it.
>
> While we have been able to narrow it down a little bit, no solution has
> been forthcoming, so this is an attempt to describe the bug in the hope
> that someone else will have an idea about what could be causing it.
>
> What we're seeing is the following (when the access point is running
> ath9k with the softq patches):
>
> When running two or more flows to a station, their combined throughput
> will be roughly 20-30% lower than the throughput of a single flow to the
> same station. This happens:
>
> - for both TCP and UDP traffic.
> - independent of the base rate (i.e. signal quality).
> - but only with crypto enabled (WPA2 CCMP in this case).
>
> However, the regression completely disappears if either of the
> following is true:
>
> - no crypto is enabled.
> - the FQ part of mac80211 is disabled (as in [1]).
>
> We have been able to reproduce this behaviour on two different ath9k
> hardware chips and two different architectures.
>
> The cause of the regression seems to be that the aggregates are smaller
> when there are two flows than when there is only one. Adding debug
> statements to the aggregate forming code indicates that this is because
> no more packets are available when the aggregates are built (i.e.
> ieee80211_tx_dequeue() returns NULL).
>
> We have not been able to determine why the queues run empty when this
> combination of circumstances arise. Since we easily get upwards of 120
> Mbps of TCP throughput without crypto but with full FQ, it's clearly not
> the hashing overhead in itself that does it (and the hashing also
> happens with just one flow, so the overhead is still there). And the
> crypto itself should be offloaded to hardware (shouldn't it? we do see a
> marked drop in overall throughput from just enabling crypto), so how
> would the queueing (say, mixing of packets from different flows)
> influence that?
>
> Does anyone have any ideas? We are stumped...
I have not done any further tests, but based on your analysis, I think I
finally understand what's causing this issue:
The CCMP PN (crypto IV) is assigned in the tx path before a packet is
put into the txq. It is also used to protect against replay attacks, so
it is sensitive to reordering. The receiver is simply dropping any
packet where the PN value is lower than the highest PN received so far.
To fix this, we will have to move the IV/PN assignment to
ieee80211_tx_dequeue.
- Felix
^ permalink raw reply
* Re: [PATCHv6 1/2] add basic register-field manipulation macros
From: Kalle Valo @ 2016-08-17 10:31 UTC (permalink / raw)
To: torvalds, akpm, gregkh, davem
Cc: linux-wireless, linux-kernel, dinan.gunawardena, Jakub Kicinski
In-Reply-To: <1471360704-10507-2-git-send-email-jakub.kicinski@netronome.com>
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
^ permalink raw reply
* [PATCH] ath10k: suppress warnings when getting wmi WDS peer event id
From: Mohammed Shafi Shajakhan @ 2016-08-17 11:28 UTC (permalink / raw)
To: ath10k; +Cc: mohammed, linux-wireless, Mohammed Shafi Shajakhan
From: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
'WMI_10_4_WDS_PEER_EVENTID' is not yet handled/implemented for WDS mode,
as of nowsuppress the warning message "Unknown eventid: 36903"
Signed-off-by: Mohammed Shafi Shajakhan <mohammed@qti.qualcomm.com>
---
found this issue while running ath10k WDS AP -> ath10k WDS STA UDP traffic
drivers/net/wireless/ath/ath10k/wmi.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/wireless/ath/ath10k/wmi.c b/drivers/net/wireless/ath/ath10k/wmi.c
index 6279ab4..a380616 100644
--- a/drivers/net/wireless/ath/ath10k/wmi.c
+++ b/drivers/net/wireless/ath/ath10k/wmi.c
@@ -5274,6 +5274,7 @@ static void ath10k_wmi_10_4_op_rx(struct ath10k *ar, struct sk_buff *skb)
break;
case WMI_10_4_WOW_WAKEUP_HOST_EVENTID:
case WMI_10_4_PEER_RATECODE_LIST_EVENTID:
+ case WMI_10_4_WDS_PEER_EVENTID:
ath10k_dbg(ar, ATH10K_DBG_WMI,
"received event id %d not implemented\n", id);
break;
--
1.9.1
^ permalink raw reply related
* Re: Configuring Minstrel-Blues
From: Thomas Hühn @ 2016-08-17 11:52 UTC (permalink / raw)
To: Mike Mu; +Cc: linux-wireless ???, Stanley Mui
In-Reply-To: <CAGTDyhHOxwTiXtsWuy1cLq+eraikTs9McS0kUBXS2W6B09CwgQ@mail.gmail.com>
Hi Mike,
I am working on the Minstrel-Blues part to get it integrated into minstrel_ht. My working demo is with ath5k an legacy minstrel.
As soon as I have a testing patch, I am going to announce it here.
Greetings from Berlin
Thomas
> On 15 Aug 2016, at 23:41, Mike Mu <mikemu@google.com> wrote:
>
> Hi Linux-Wireless,
>
> I am trying to enable Minstrel-Blues for the ath9k chipset that's used
> in Google Fiber's access points. I applied the patches in
> https://github.com/thuehn/Minstrel-Blues/tree/master/src onto our
> backports-20160122 tree, with some unsuspicious merge conflicts, and
> built an image.
>
> To test, I ran iperf from the AP to a station very close by. I
> captured the packets to look at the RSSI with and without the
> Minstrel-Blues patches applied, but did not see any difference in
> RSSI.
>
> Is there anything I need to configure first to make it work? Is it
> possible that I'm missing a patch that's not in backports-20160122?
>
> Thanks!
> Mike
^ permalink raw reply
* Re: On the ath9k performance regression with FQ and crypto
From: Toke Høiland-Jørgensen @ 2016-08-17 12:04 UTC (permalink / raw)
To: Felix Fietkau; +Cc: make-wifi-fast, linux-wireless, Michal Kazior, Dave Taht
In-Reply-To: <6d471495-fa0f-f750-a4a4-f467205b94bd@nbd.name>
Felix Fietkau <nbd@nbd.name> writes:
> I have not done any further tests, but based on your analysis, I think I
> finally understand what's causing this issue:
> The CCMP PN (crypto IV) is assigned in the tx path before a packet is
> put into the txq. It is also used to protect against replay attacks, so
> it is sensitive to reordering. The receiver is simply dropping any
> packet where the PN value is lower than the highest PN received so far.
> To fix this, we will have to move the IV/PN assignment to
> ieee80211_tx_dequeue.
Indeed this seems to be the cause of the problem. Thanks!
Will send a patch once we've run it through another couple rounds of
testing. Looks promising so far :)
-Toke
^ permalink raw reply
* [PATCH] mac80211: Move crypto IV generation to after TXQ dequeue.
From: Toke Høiland-Jørgensen @ 2016-08-17 12:58 UTC (permalink / raw)
To: make-wifi-fast, linux-wireless
Cc: Toke Høiland-Jørgensen, Felix Fietkau
The FQ portion of the intermediate queues will reorder packets, which
means that crypto IV generation needs to happen after dequeue when they
are enabled, or the receiver will throw packets away when receiving
them.
This fixes the performance regression introduced by enabling softq in
ath9k.
Cc: Felix Fietkau <nbd@nbd.name>
Tested-by: Dave Taht <dave@taht.net>
Signed-off-by: Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk>
---
include/net/mac80211.h | 2 ++
net/mac80211/sta_info.h | 3 +--
net/mac80211/tx.c | 55 +++++++++++++++++++++++++++++++------------=
------
3 files changed, 38 insertions(+), 22 deletions(-)
diff --git a/include/net/mac80211.h b/include/net/mac80211.h
index cca510a..b23deba 100644
--- a/include/net/mac80211.h
+++ b/include/net/mac80211.h
@@ -1556,6 +1556,7 @@ enum ieee80211_key_flags {
* @tx_pn: PN used for TX keys, may be used by the driver as well if it
* needs to do software PN assignment by itself (e.g. due to TSO)
* @flags: key flags, see &enum ieee80211_key_flags.
+ * @pn_offs: offset where to put PN for crypto (or 0 if not needed)
* @keyidx: the key index (0-3)
* @keylen: key material length
* @key: key material. For ALG_TKIP the key is encoded as a 256-bit (32 =
byte)
@@ -1573,6 +1574,7 @@ struct ieee80211_key_conf {
u8 iv_len;
u8 hw_key_idx;
u8 flags;
+ u8 pn_offs;
s8 keyidx;
u8 keylen;
u8 key[0];
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 0556be3..c9d4d69 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -266,7 +266,6 @@ struct sta_ampdu_mlme {
* @hdr_len: actual 802.11 header length
* @sa_offs: offset of the SA
* @da_offs: offset of the DA
- * @pn_offs: offset where to put PN for crypto (or 0 if not needed)
* @band: band this will be transmitted on, for tx_info
* @rcu_head: RCU head to free this struct
*
@@ -277,7 +276,7 @@ struct sta_ampdu_mlme {
struct ieee80211_fast_tx {
struct ieee80211_key *key;
u8 hdr_len;
- u8 sa_offs, da_offs, pn_offs;
+ u8 sa_offs, da_offs;
u8 band;
u8 hdr[30 + 2 + IEEE80211_FAST_XMIT_MAX_IV +
sizeof(rfc1042_header)] __aligned(2);
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 1d0746d..4ae1f2c 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1074,6 +1074,33 @@ ieee80211_tx_h_calculate_duration(struct ieee80211=
_tx_data *tx)
return TX_CONTINUE;
}
=20
+static inline void ieee80211_set_crypto_pn(struct ieee80211_key_conf *co=
nf,
+ struct sk_buff *skb)
+{
+ u64 pn;
+ u8 *crypto_hdr =3D skb->data + conf->pn_offs;
+
+ if (!conf->pn_offs)
+ return;
+
+ switch (conf->cipher) {
+ case WLAN_CIPHER_SUITE_CCMP:
+ case WLAN_CIPHER_SUITE_CCMP_256:
+ case WLAN_CIPHER_SUITE_GCMP:
+ case WLAN_CIPHER_SUITE_GCMP_256:
+ pn =3D atomic64_inc_return(&conf->tx_pn);
+ crypto_hdr[0] =3D pn;
+ crypto_hdr[1] =3D pn >> 8;
+ crypto_hdr[4] =3D pn >> 16;
+ crypto_hdr[5] =3D pn >> 24;
+ crypto_hdr[6] =3D pn >> 32;
+ crypto_hdr[7] =3D pn >> 40;
+ break;
+ }
+}
+
+
+
/* actual transmit path */
=20
static bool ieee80211_tx_prep_agg(struct ieee80211_tx_data *tx,
@@ -1503,6 +1530,10 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80=
211_hw *hw,
sta);
struct ieee80211_tx_info *info =3D IEEE80211_SKB_CB(skb);
=20
+ if (info->control.hw_key) {
+ ieee80211_set_crypto_pn(info->control.hw_key, skb);
+ }
+
hdr->seq_ctrl =3D ieee80211_tx_next_seq(sta, txq->tid);
if (test_bit(IEEE80211_TXQ_AMPDU, &txqi->flags))
info->flags |=3D IEEE80211_TX_CTL_AMPDU;
@@ -2874,7 +2905,7 @@ void ieee80211_check_fast_xmit(struct sta_info *sta=
)
if (gen_iv) {
(build.hdr + build.hdr_len)[3] =3D
0x20 | (build.key->conf.keyidx << 6);
- build.pn_offs =3D build.hdr_len;
+ build.key->conf.pn_offs =3D build.hdr_len;
}
if (gen_iv || iv_spc)
build.hdr_len +=3D IEEE80211_CCMP_HDR_LEN;
@@ -2885,7 +2916,7 @@ void ieee80211_check_fast_xmit(struct sta_info *sta=
)
if (gen_iv) {
(build.hdr + build.hdr_len)[3] =3D
0x20 | (build.key->conf.keyidx << 6);
- build.pn_offs =3D build.hdr_len;
+ build.key->conf.pn_offs =3D build.hdr_len;
}
if (gen_iv || iv_spc)
build.hdr_len +=3D IEEE80211_GCMP_HDR_LEN;
@@ -3289,24 +3320,8 @@ static bool ieee80211_xmit_fast(struct ieee80211_s=
ub_if_data *sdata,
sta->tx_stats.bytes[skb_get_queue_mapping(skb)] +=3D skb->len;
sta->tx_stats.packets[skb_get_queue_mapping(skb)]++;
=20
- if (fast_tx->pn_offs) {
- u64 pn;
- u8 *crypto_hdr =3D skb->data + fast_tx->pn_offs;
-
- switch (fast_tx->key->conf.cipher) {
- case WLAN_CIPHER_SUITE_CCMP:
- case WLAN_CIPHER_SUITE_CCMP_256:
- case WLAN_CIPHER_SUITE_GCMP:
- case WLAN_CIPHER_SUITE_GCMP_256:
- pn =3D atomic64_inc_return(&fast_tx->key->conf.tx_pn);
- crypto_hdr[0] =3D pn;
- crypto_hdr[1] =3D pn >> 8;
- crypto_hdr[4] =3D pn >> 16;
- crypto_hdr[5] =3D pn >> 24;
- crypto_hdr[6] =3D pn >> 32;
- crypto_hdr[7] =3D pn >> 40;
- break;
- }
+ if (fast_tx->key && !local->ops->wake_tx_queue) {
+ ieee80211_set_crypto_pn(&fast_tx->key->conf, skb);
}
=20
if (sdata->vif.type =3D=3D NL80211_IFTYPE_AP_VLAN)
--=20
2.9.2
^ permalink raw reply related
* Re: [PATCH] mac80211: Move crypto IV generation to after TXQ dequeue.
From: Johannes Berg @ 2016-08-17 13:08 UTC (permalink / raw)
To: Toke Høiland-Jørgensen, make-wifi-fast, linux-wireless
Cc: Felix Fietkau
In-Reply-To: <20160817125800.19154-1-toke@toke.dk>
> @@ -1573,6 +1574,7 @@ struct ieee80211_key_conf {
> u8 iv_len;
> u8 hw_key_idx;
> u8 flags;
> + u8 pn_offs;
>
This is completely wrong.
johannes
^ permalink raw reply
* Re: [PATCH] mac80211: Move crypto IV generation to after TXQ dequeue.
From: Toke Høiland-Jørgensen @ 2016-08-17 13:16 UTC (permalink / raw)
To: Johannes Berg; +Cc: make-wifi-fast, linux-wireless, Felix Fietkau
In-Reply-To: <1471439315.5173.7.camel@sipsolutions.net>
Johannes Berg <johannes@sipsolutions.net> writes:
>> @@ -1573,6 +1574,7 @@ struct ieee80211_key_conf {
>> =C2=A0 u8 iv_len;
>> =C2=A0 u8 hw_key_idx;
>> =C2=A0 u8 flags;
>> + u8 pn_offs;
>>=20
> This is completely wrong.
Well, the ieee80211_fast_tx struct is not available in
ieee80211_tx_dequeue, and I need the offset there. I thought about
sticking it into ieee80211_tx_info, but that is kinda full, and since
the ieee80211_key_conf is already available there, carrying it there
seems to work.
What would be a better way to do this?
-Toke
^ permalink raw reply
* Re: [PATCH] mac80211: Move crypto IV generation to after TXQ dequeue.
From: Johannes Berg @ 2016-08-17 13:18 UTC (permalink / raw)
To: Toke Høiland-Jørgensen
Cc: make-wifi-fast, linux-wireless, Felix Fietkau
In-Reply-To: <87h9aj5ydx.fsf@toke.dk>
On Wed, 2016-08-17 at 15:16 +0200, Toke Høiland-Jørgensen wrote:
> Johannes Berg <johannes@sipsolutions.net> writes:
>
> >
> > >
> > > @@ -1573,6 +1574,7 @@ struct ieee80211_key_conf {
> > > u8 iv_len;
> > > u8 hw_key_idx;
> > > u8 flags;
> > > + u8 pn_offs;
> > >
> > This is completely wrong.
>
> Well, the ieee80211_fast_tx struct is not available in
> ieee80211_tx_dequeue, and I need the offset there. I thought about
> sticking it into ieee80211_tx_info, but that is kinda full, and since
> the ieee80211_key_conf is already available there, carrying it there
> seems to work.
For very limited testing, perhaps. But this isn't static across all
usages of the key, so this is still completely broken.
> What would be a better way to do this?
>
Some redesign/rearchitecture, probably. Or just do it all in the driver
like iwlmvm?
johannes
^ permalink raw reply
* Re: [PATCH] mac80211: Move crypto IV generation to after TXQ dequeue.
From: Toke Høiland-Jørgensen @ 2016-08-17 13:23 UTC (permalink / raw)
To: Johannes Berg; +Cc: make-wifi-fast, linux-wireless, Felix Fietkau
In-Reply-To: <1471439894.5173.9.camel@sipsolutions.net>
Johannes Berg <johannes@sipsolutions.net> writes:
> On Wed, 2016-08-17 at 15:16 +0200, Toke H=C3=B8iland-J=C3=B8rgensen wrote:
>> Johannes Berg <johannes@sipsolutions.net> writes:
>>=20
>> >=20
>> > >=20
>> > > @@ -1573,6 +1574,7 @@ struct ieee80211_key_conf {
>> > > =C2=A0 u8 iv_len;
>> > > =C2=A0 u8 hw_key_idx;
>> > > =C2=A0 u8 flags;
>> > > + u8 pn_offs;
>> > >=20
>> > This is completely wrong.
>>=20
>> Well, the ieee80211_fast_tx struct is not available in
>> ieee80211_tx_dequeue, and I need the offset there. I thought about
>> sticking it into ieee80211_tx_info, but that is kinda full, and since
>> the ieee80211_key_conf is already available there, carrying it there
>> seems to work.
>
> For very limited testing, perhaps. But this isn't static across all
> usages of the key, so this is still completely broken.
OK, noted.
>> What would be a better way to do this?
>>=20
>
> Some redesign/rearchitecture, probably. Or just do it all in the driver
> like iwlmvm?
Will look it over again. Should be possible to re-calculate the offset,
I guess.
-Toke
^ permalink raw reply
* [PATCH v2] mac80211: Move crypto IV generation to after TXQ dequeue.
From: Toke Høiland-Jørgensen @ 2016-08-17 14:45 UTC (permalink / raw)
To: make-wifi-fast, linux-wireless
Cc: Toke Høiland-Jørgensen, Felix Fietkau
In-Reply-To: <20160817125800.19154-1-toke@toke.dk>
The FQ portion of the intermediate queues will reorder packets, which
means that crypto IV generation needs to happen after dequeue when they
are enabled, or the receiver will throw packets away when receiving
them.
This fixes the performance regression introduced by enabling softq in
ath9k.
Cc: Felix Fietkau <nbd@nbd.name>
Tested-by: Dave Taht <dave@taht.net>
Signed-off-by: Toke H=C3=B8iland-J=C3=B8rgensen <toke@toke.dk>
---
Changes since v1:
- Recalculate pn_offs when needed instead of storing it.
net/mac80211/sta_info.h | 3 +-
net/mac80211/tx.c | 85 +++++++++++++++++++++++++++++++++++++------=
------
2 files changed, 66 insertions(+), 22 deletions(-)
diff --git a/net/mac80211/sta_info.h b/net/mac80211/sta_info.h
index 0556be3..c9d4d69 100644
--- a/net/mac80211/sta_info.h
+++ b/net/mac80211/sta_info.h
@@ -266,7 +266,6 @@ struct sta_ampdu_mlme {
* @hdr_len: actual 802.11 header length
* @sa_offs: offset of the SA
* @da_offs: offset of the DA
- * @pn_offs: offset where to put PN for crypto (or 0 if not needed)
* @band: band this will be transmitted on, for tx_info
* @rcu_head: RCU head to free this struct
*
@@ -277,7 +276,7 @@ struct sta_ampdu_mlme {
struct ieee80211_fast_tx {
struct ieee80211_key *key;
u8 hdr_len;
- u8 sa_offs, da_offs, pn_offs;
+ u8 sa_offs, da_offs;
u8 band;
u8 hdr[30 + 2 + IEEE80211_FAST_XMIT_MAX_IV +
sizeof(rfc1042_header)] __aligned(2);
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 1d0746d..9caf75f 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1074,6 +1074,64 @@ ieee80211_tx_h_calculate_duration(struct ieee80211=
_tx_data *tx)
return TX_CONTINUE;
}
=20
+static void ieee80211_gen_crypto_iv(struct ieee80211_key_conf *conf,
+ struct sta_info *sta, struct sk_buff *skb)
+{
+ struct ieee80211_sub_if_data *sdata;
+ u64 pn;
+ u8 *crypto_hdr;
+ u8 pn_offs =3D 0;
+
+ if (!conf || !sta || !(conf->flags & IEEE80211_KEY_FLAG_GENERATE_IV))
+ return;
+
+ sdata =3D sta->sdata;
+
+ switch (sdata->vif.type) {
+ case NL80211_IFTYPE_STATION:
+ if (sdata->u.mgd.use_4addr) {
+ pn_offs =3D 30;
+ break;
+ }
+ pn_offs =3D 24;
+ break;
+ case NL80211_IFTYPE_AP_VLAN:
+ if (sdata->wdev.use_4addr) {
+ pn_offs =3D 30;
+ break;
+ }
+ /* fall through */
+ case NL80211_IFTYPE_ADHOC:
+ case NL80211_IFTYPE_AP:
+ pn_offs =3D 24;
+ break;
+ default:
+ return;
+ }
+
+ if (sta->sta.wme) {
+ pn_offs +=3D 2;
+ }
+
+ crypto_hdr =3D skb->data + pn_offs;
+ switch (conf->cipher) {
+ case WLAN_CIPHER_SUITE_CCMP:
+ case WLAN_CIPHER_SUITE_CCMP_256:
+ case WLAN_CIPHER_SUITE_GCMP:
+ case WLAN_CIPHER_SUITE_GCMP_256:
+ pn =3D atomic64_inc_return(&conf->tx_pn);
+ crypto_hdr[0] =3D pn;
+ crypto_hdr[1] =3D pn >> 8;
+ crypto_hdr[4] =3D pn >> 16;
+ crypto_hdr[5] =3D pn >> 24;
+ crypto_hdr[6] =3D pn >> 32;
+ crypto_hdr[7] =3D pn >> 40;
+ break;
+ }
+}
+
+
+
/* actual transmit path */
=20
static bool ieee80211_tx_prep_agg(struct ieee80211_tx_data *tx,
@@ -1503,6 +1561,11 @@ struct sk_buff *ieee80211_tx_dequeue(struct ieee80=
211_hw *hw,
sta);
struct ieee80211_tx_info *info =3D IEEE80211_SKB_CB(skb);
=20
+ if (info->control.hw_key) {
+ ieee80211_gen_crypto_iv(info->control.hw_key,
+ container_of(txq->sta, struct sta_info, sta), skb);
+ }
+
hdr->seq_ctrl =3D ieee80211_tx_next_seq(sta, txq->tid);
if (test_bit(IEEE80211_TXQ_AMPDU, &txqi->flags))
info->flags |=3D IEEE80211_TX_CTL_AMPDU;
@@ -2874,7 +2937,6 @@ void ieee80211_check_fast_xmit(struct sta_info *sta=
)
if (gen_iv) {
(build.hdr + build.hdr_len)[3] =3D
0x20 | (build.key->conf.keyidx << 6);
- build.pn_offs =3D build.hdr_len;
}
if (gen_iv || iv_spc)
build.hdr_len +=3D IEEE80211_CCMP_HDR_LEN;
@@ -2885,7 +2947,6 @@ void ieee80211_check_fast_xmit(struct sta_info *sta=
)
if (gen_iv) {
(build.hdr + build.hdr_len)[3] =3D
0x20 | (build.key->conf.keyidx << 6);
- build.pn_offs =3D build.hdr_len;
}
if (gen_iv || iv_spc)
build.hdr_len +=3D IEEE80211_GCMP_HDR_LEN;
@@ -3289,24 +3350,8 @@ static bool ieee80211_xmit_fast(struct ieee80211_s=
ub_if_data *sdata,
sta->tx_stats.bytes[skb_get_queue_mapping(skb)] +=3D skb->len;
sta->tx_stats.packets[skb_get_queue_mapping(skb)]++;
=20
- if (fast_tx->pn_offs) {
- u64 pn;
- u8 *crypto_hdr =3D skb->data + fast_tx->pn_offs;
-
- switch (fast_tx->key->conf.cipher) {
- case WLAN_CIPHER_SUITE_CCMP:
- case WLAN_CIPHER_SUITE_CCMP_256:
- case WLAN_CIPHER_SUITE_GCMP:
- case WLAN_CIPHER_SUITE_GCMP_256:
- pn =3D atomic64_inc_return(&fast_tx->key->conf.tx_pn);
- crypto_hdr[0] =3D pn;
- crypto_hdr[1] =3D pn >> 8;
- crypto_hdr[4] =3D pn >> 16;
- crypto_hdr[5] =3D pn >> 24;
- crypto_hdr[6] =3D pn >> 32;
- crypto_hdr[7] =3D pn >> 40;
- break;
- }
+ if (fast_tx->key && !local->ops->wake_tx_queue) {
+ ieee80211_gen_crypto_iv(&fast_tx->key->conf, sta, skb);
}
=20
if (sdata->vif.type =3D=3D NL80211_IFTYPE_AP_VLAN)
--=20
2.9.2
^ permalink raw reply related
* [PATCH] ath10k: improve wake_tx_queue ops performance
From: Rajkumar Manoharan @ 2016-08-17 15:32 UTC (permalink / raw)
To: ath10k; +Cc: linux-wireless, rmanohar, Rajkumar Manoharan
txqs_lock is interfering with wake_tx_queue submitting more frames.
so queues don't get filled in and don't keep firmware/hardware busy
enough. This change helps to reduce the txqs_lock contention and
wake_tx_queue() blockage to being possible in txrx_unref().
To reduce turn around time of wake_tx_queue ops and to maintain fairness
among all txqs, the callback is updated to push first txq alone from
pending list for every wake_tx_queue call. Remaining txqs will be
processed later upon tx completion.
Below improvements are observed in push-only mode and validated on
IPQ4019 platform. With this change, in AP mode ~10Mbps increase is
observed in downlink (AP -> STA) traffic and approx. 5-10% of CPU
usage is reduced.
Major improvement is observed in 1-hop Mesh mode topology in 11ACVHT80.
Compared to Infra mode, CPU overhead is higher in Mesh mode due to path
lookup and no fast-xmit support. So reducing spin lock contention is
helping in Mesh.
TOT +change
-------- --------
TCP DL 545 Mbps 595 Mbps
TCP UL 555 Mbps 585 Mbps
Signed-off-by: Rajkumar Manoharan <rmanohar@qti.qualcomm.com>
---
drivers/net/wireless/ath/ath10k/mac.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ath/ath10k/mac.c b/drivers/net/wireless/ath/ath10k/mac.c
index c7c7ceb9be73..93465ffec2eb 100644
--- a/drivers/net/wireless/ath/ath10k/mac.c
+++ b/drivers/net/wireless/ath/ath10k/mac.c
@@ -4106,13 +4106,29 @@ static void ath10k_mac_op_wake_tx_queue(struct ieee80211_hw *hw,
{
struct ath10k *ar = hw->priv;
struct ath10k_txq *artxq = (void *)txq->drv_priv;
+ struct ieee80211_txq *f_txq;
+ struct ath10k_txq *f_artxq;
+ int ret = 0;
+ int max = 16;
spin_lock_bh(&ar->txqs_lock);
if (list_empty(&artxq->list))
list_add_tail(&artxq->list, &ar->txqs);
+
+ f_artxq = list_first_entry(&ar->txqs, struct ath10k_txq, list);
+ f_txq = container_of((void *)f_artxq, struct ieee80211_txq, drv_priv);
+ list_del_init(&f_artxq->list);
+
+ while (ath10k_mac_tx_can_push(hw, f_txq) && max--) {
+ ret = ath10k_mac_tx_push_txq(hw, f_txq);
+ if (ret)
+ break;
+ }
+ if (ret != -ENOENT)
+ list_add_tail(&f_artxq->list, &ar->txqs);
spin_unlock_bh(&ar->txqs_lock);
- ath10k_mac_tx_push_pending(ar);
+ ath10k_htt_tx_txq_update(hw, f_txq);
ath10k_htt_tx_txq_update(hw, txq);
}
--
2.9.2
^ permalink raw reply related
* Re: [PATCHv6 1/2] add basic register-field manipulation macros
From: Linus Torvalds @ 2016-08-17 16:33 UTC (permalink / raw)
To: Kalle Valo
Cc: Andrew Morton, Greg Kroah-Hartman, David Miller,
Linux Wireless List, Linux Kernel Mailing List, dinan.gunawardena,
Jakub Kicinski
In-Reply-To: <8737m3bsau.fsf@kamboji.qca.qualcomm.com>
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
^ permalink raw reply
* Re: [PATCHv6 1/2] add basic register-field manipulation macros
From: Jakub Kicinski @ 2016-08-17 17:11 UTC (permalink / raw)
To: Linus Torvalds
Cc: Kalle Valo, Andrew Morton, Greg Kroah-Hartman, David Miller,
Linux Wireless List, Linux Kernel Mailing List, dinan.gunawardena
In-Reply-To: <CA+55aFwSUKorb-jho0eHzdzGNKr4c1UO8d0CRe=fsLPmUheKeg@mail.gmail.com>
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!
^ permalink raw reply
* Re: [PATCHv6 1/2] add basic register-field manipulation macros
From: Linus Torvalds @ 2016-08-17 17:16 UTC (permalink / raw)
To: Jakub Kicinski
Cc: Kalle Valo, Andrew Morton, Greg Kroah-Hartman, David Miller,
Linux Wireless List, Linux Kernel Mailing List, dinan.gunawardena
In-Reply-To: <20160817181109.080edf7a@jkicinski-Precision-T1700>
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
^ 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