Re: RTL8192EU on rtl8xxxu driver breaks every few minutes

[Jes Sorensen]

"Franc[e]sco"  writes:
> Oops, forgot to forward the previous reply to the mailing list.
>
> I have attached the output of iw link
>
> the AP is an asus dsl-n55u router in 2.4 GHz mode, using WPA2-Personal
> with AES encryption. It's also running a second 5GHz wireless network
> which has a different SSID.
>
> Also, this seems to be some kind of power saving kicking in, as the
> dongle keeps working as long as I keep doing things over ssh.

I just pushed a patch into rtl8xxxu-devel which may resolve this issue.
There were problems with the 8192eu not handling driver reload very
well. It is possible the network scripts you run would trigger the shut
down and restart that caused this problem.

I would be interested in knowing if this patch resolves the problem for
you.

Jes

PS: Please fix your mail client - adding unlisted-recipients to the Cc
line and cutting out the person you are responding to is really
annoying.

>From 93064d0ae3e9d97c03a3aabd71e6048e1ac82f46 Mon Sep 17 00:00:00 2001
From: Jes Sorensen 
Date: Fri, 30 Sep 2016 19:18:34 -0400
Subject: [PATCH 2/2] rtl8xxxu: Fix rtl8192eu driver reload issue

The 8192eu suffered from two issues when reloading the driver.

The same problems as with the 8723bu where REG_RX_WAIT_CCA bits 22 and
23 didn't get set in rtl8192e_enable_rf().

In addition it also seems prone to issues when setting REG_RF_CTRL to
0 intead of just disabling the RF_ENABLE bit. Similar to what was
causing issues with the 8188eu.

With this patch I can successfully reload the driver and reassociate
to an APi with an 8192eu dongle.

Signed-off-by: Jes Sorensen 
---
 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
index df54d27..a793fed 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
@@ -1461,7 +1461,9 @@ static int rtl8192eu_active_to_emu(struct rtl8xxxu_priv *priv)
 	int count, ret = 0;
 
 	/* Turn off RF */
-	rtl8xxxu_write8(priv, REG_RF_CTRL, 0);
+	val8 = rtl8xxxu_read8(priv, REG_RF_CTRL);
+	val8 &= ~RF_ENABLE;
+	rtl8xxxu_write8(priv, REG_RF_CTRL, val8);
 
 	/* Switch DPDT_SEL_P output from register 0x65[2] */
 	val8 = rtl8xxxu_read8(priv, REG_LEDCFG2);
@@ -1593,6 +1595,10 @@ static void rtl8192e_enable_rf(struct rtl8xxxu_priv *priv)
 	u32 val32;
 	u8 val8;
 
+	val32 = rtl8xxxu_read32(priv, REG_RX_WAIT_CCA);
+	val32 |= (BIT(22) | BIT(23));
+	rtl8xxxu_write32(priv, REG_RX_WAIT_CCA, val32);
+
 	val8 = rtl8xxxu_read8(priv, REG_GPIO_MUXCFG);
 	val8 |= BIT(5);
 	rtl8xxxu_write8(priv, REG_GPIO_MUXCFG, val8);
-- 
2.7.4

[PATCH 1/2] rtl8xxxu: Fix rtl8723bu driver reload issue

[Jes . Sorensen]

From: Jes Sorensen 

The generic disable_rf() function clears bits 22 and 23 in
REG_RX_WAIT_CCA, however we did not re-enable them again in
rtl8723b_enable_rf()

This resolves the problem for me with 8723bu devices not working again
after reloading the driver.

Signed-off-by: Jes Sorensen 
---
 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c 
b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
index 6c086b5..02b8ddd 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c
@@ -1498,6 +1498,10 @@ static void rtl8723b_enable_rf(struct rtl8xxxu_priv 
*priv)
u32 val32;
u8 val8;
 
+   val32 = rtl8xxxu_read32(priv, REG_RX_WAIT_CCA);
+   val32 |= (BIT(22) | BIT(23));
+   rtl8xxxu_write32(priv, REG_RX_WAIT_CCA, val32);
+
/*
 * No indication anywhere as to what 0x0790 does. The 2 antenna
 * vendor code preserves bits 6-7 here.
-- 
2.7.4

[PATCH 2/2] rtl8xxxu: Fix rtl8192eu driver reload issue

[Jes . Sorensen]

From: Jes Sorensen 

The 8192eu suffered from two issues when reloading the driver.

The same problems as with the 8723bu where REG_RX_WAIT_CCA bits 22 and
23 didn't get set in rtl8192e_enable_rf().

In addition it also seems prone to issues when setting REG_RF_CTRL to
0 intead of just disabling the RF_ENABLE bit. Similar to what was
causing issues with the 8188eu.

With this patch I can successfully reload the driver and reassociate
to an APi with an 8192eu dongle.

Signed-off-by: Jes Sorensen 
---
 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c | 8 +++-
 1 file changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c 
b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
index df54d27..a793fed 100644
--- a/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
+++ b/drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c
@@ -1461,7 +1461,9 @@ static int rtl8192eu_active_to_emu(struct rtl8xxxu_priv 
*priv)
int count, ret = 0;
 
/* Turn off RF */
-   rtl8xxxu_write8(priv, REG_RF_CTRL, 0);
+   val8 = rtl8xxxu_read8(priv, REG_RF_CTRL);
+   val8 &= ~RF_ENABLE;
+   rtl8xxxu_write8(priv, REG_RF_CTRL, val8);
 
/* Switch DPDT_SEL_P output from register 0x65[2] */
val8 = rtl8xxxu_read8(priv, REG_LEDCFG2);
@@ -1593,6 +1595,10 @@ static void rtl8192e_enable_rf(struct rtl8xxxu_priv 
*priv)
u32 val32;
u8 val8;
 
+   val32 = rtl8xxxu_read32(priv, REG_RX_WAIT_CCA);
+   val32 |= (BIT(22) | BIT(23));
+   rtl8xxxu_write32(priv, REG_RX_WAIT_CCA, val32);
+
val8 = rtl8xxxu_read8(priv, REG_GPIO_MUXCFG);
val8 |= BIT(5);
rtl8xxxu_write8(priv, REG_GPIO_MUXCFG, val8);
-- 
2.7.4

[PATCH 0/2] rtl8xxxu: Fix driver reload issues with 8723bu and 8192eu

[Jes . Sorensen]

From: Jes Sorensen 

Hi,

These two patches fix issues where rtl8723bu and rtl8192eu dongles
would stop receving data if one reloaded the driver module, requiring
it to be physically removed and reinserted.

Both issues are applicable to 4.9. The first patch for 8723bu is
applicable to stable 4.7 and 4.8. The second patch will only be
applicable to stable 4.8, once patches currently in flight are merged
by the network maintainers.

Cheers,
Jes


Jes Sorensen (2):
  rtl8xxxu: Fix rtl8723bu driver reload issue
  rtl8xxxu: Fix rtl8192eu driver reload issue

 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8192e.c | 8 +++-
 drivers/net/wireless/realtek/rtl8xxxu/rtl8xxxu_8723b.c | 4 
 2 files changed, 11 insertions(+), 1 deletion(-)

-- 
2.7.4

pull-request: mac80211-next 2016-09-30

[Johannes Berg]

Hi Dave,

Last pull request before the merge window, since it's about to open :)
It seems that everyone finally agreed on the NAN (neighbor awareness
networking) APIs, so we have that, along with some other things.

Let me know if there's any problem.

Thanks,
johannes



The following changes since commit c13ed534b8db543e4d8ead3885f4b06585a5771c:

  Merge tag 'mac80211-next-for-davem-2016-09-16' of 
git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next (2016-09-18 
22:29:08 -0400)

are available in the git repository at:

  git://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211-next.git 
tags/mac80211-next-for-davem-2016-09-30

for you to fetch changes up to bb42f2d13ffcd0baed7547b37d05add51fcd50e1:

  mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue (2016-09-30 
14:46:57 +0200)


This time around, we have
 * Neighbor Awareness Networking (NAN) APIs
 * a fix for a previous patch that caused memory corruption
   in wireless extensions key settings
 * beacon rate configuration for AP and mesh
 * memory limits for mac80211's internal TXQs
 * a (fairly involved) fix for the TXQ vs. crypto problems
 * direct cfg80211 driver API for WEP keys


Ayala Beker (9):
  cfg80211: add start / stop NAN commands
  mac80211: add boilerplate code for start / stop NAN
  cfg80211: add add_nan_func / del_nan_func
  cfg80211: allow the user space to change current NAN configuration
  cfg80211: provide a function to report a match for NAN
  cfg80211: Provide an API to report NAN function termination
  mac80211: implement nan_change_conf
  mac80211: Implement add_nan_func and rm_nan_func
  mac80211: Add API to report NAN function match

David Spinadel (1):
  cfg80211: Add support for static WEP in the driver

Johannes Berg (2):
  cfg80211: add checks for beacon rate, extend to mesh
  cfg80211: wext: really don't store non-WEP keys

Pedersen, Thomas (2):
  mac80211: add offset_tsf driver op and use it for mesh
  mac80211: mesh: decrease max drift

Purushottam Kushwaha (1):
  cfg80211: Add support to configure a beacon data rate

Toke Høiland-Jørgensen (5):
  mac80211: Move ieee802111_tx_dequeue() to later in tx.c
  fq.h: Port memory limit mechanism from fq_codel
  mac80211: Export fq memory limit information in debugfs
  mac80211: Set lower memory limit for non-VHT devices
  mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

 include/net/cfg80211.h|  223 +++-
 include/net/fq.h  |3 +
 include/net/fq_impl.h |7 +-
 include/net/mac80211.h|   75 +++
 include/uapi/linux/nl80211.h  |  270 -
 net/mac80211/cfg.c|  208 +++
 net/mac80211/chan.c   |6 +
 net/mac80211/debugfs.c|8 +
 net/mac80211/debugfs_netdev.c |   12 +-
 net/mac80211/driver-ops.c |   15 +
 net/mac80211/driver-ops.h |   83 +++
 net/mac80211/ieee80211_i.h|   26 +
 net/mac80211/iface.c  |   28 +-
 net/mac80211/main.c   |8 +
 net/mac80211/mesh_sync.c  |   12 +-
 net/mac80211/offchannel.c |4 +-
 net/mac80211/rx.c |7 +-
 net/mac80211/sta_info.c   |   10 +-
 net/mac80211/trace.h  |  159 ++
 net/mac80211/tx.c |  351 
 net/mac80211/util.c   |   61 ++-
 net/wireless/chan.c   |2 +
 net/wireless/core.c   |   35 ++
 net/wireless/core.h   |7 +-
 net/wireless/ibss.c   |5 +-
 net/wireless/mlme.c   |1 +
 net/wireless/nl80211.c| 1220 -
 net/wireless/rdev-ops.h   |   58 ++
 net/wireless/sme.c|6 +-
 net/wireless/trace.h  |   90 +++
 net/wireless/util.c   |   30 +-
 net/wireless/wext-compat.c|   14 +-
 net/wireless/wext-sme.c   |2 +-
 33 files changed, 2648 insertions(+), 398 deletions(-)

Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding

[Arnd Bergmann]

On Friday 30 September 2016, Felix Fietkau wrote:
> >> >> >> + device_type = "pci";
> >> >> >> + mediatek,mtd-eeprom = < 0x8000>;
> >> >> >> + mediatek,2ghz = <0>;
> >> > 
> >> > It's not clear what the possible values for the 2ghz property are,
> >> > can you be more verbose in the description? How is <0> different
> >> > from no property?
> >> 0 means disabled, no property means unchanged (compared to EEPROM).
> > 
> > Maybe have a boolean property instead then to say "mediatek,2ghz-disabled" ?
> > 
> > If zero is the only possible value, there is no need to put a number in 
> > there.
> 1 is also possible, which will force-enable the capability.

Ok, then both those values should be documented in the binding.

Arnd

Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding

[Felix Fietkau]

On 2016-09-30 16:36, Arnd Bergmann wrote:
> On Friday 30 September 2016, Felix Fietkau wrote:
>> >> >> + pcie0 {
>> >> >> + mt76@0,0 {
>> >> >> + reg = <0x 0 0 0 0>;
>> > 
>> > Maybe have an examplep of a real register address other than zero?
>> This is a real example referring to the first device on a PCI bus.
>> I copy this from a .dts file that we use in LEDE.
> 
> Ok, I see.
> 
>> >> >> + device_type = "pci";
>> >> >> + mediatek,mtd-eeprom = < 0x8000>;
>> >> >> + mediatek,2ghz = <0>;
>> > 
>> > It's not clear what the possible values for the 2ghz property are,
>> > can you be more verbose in the description? How is <0> different
>> > from no property?
>> 0 means disabled, no property means unchanged (compared to EEPROM).
> 
> Maybe have a boolean property instead then to say "mediatek,2ghz-disabled" ?
> 
> If zero is the only possible value, there is no need to put a number in there.
1 is also possible, which will force-enable the capability.

- Felix

Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding

[Arnd Bergmann]

On Friday 30 September 2016, Felix Fietkau wrote:
> >> >> + pcie0 {
> >> >> + mt76@0,0 {
> >> >> + reg = <0x 0 0 0 0>;
> > 
> > Maybe have an examplep of a real register address other than zero?
> This is a real example referring to the first device on a PCI bus.
> I copy this from a .dts file that we use in LEDE.

Ok, I see.

> >> >> + device_type = "pci";
> >> >> + mediatek,mtd-eeprom = < 0x8000>;
> >> >> + mediatek,2ghz = <0>;
> > 
> > It's not clear what the possible values for the 2ghz property are,
> > can you be more verbose in the description? How is <0> different
> > from no property?
> 0 means disabled, no property means unchanged (compared to EEPROM).

Maybe have a boolean property instead then to say "mediatek,2ghz-disabled" ?

If zero is the only possible value, there is no need to put a number in there.

Arnd

Re: [PATCH 3/3] mac80211: Set lower memory limit for non-VHT devices

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

>> > I kinda see the logic here - we really don't need to queue as much
>> > if we can't possibly transmit it out quickly - but it seems to me
>> > we should also throw in some kind of limit that's relative to the
>> > amount of memory you have on the system?
>> 
>> Yes, ideally. That goes for FQ-CoDel as well, BTW. LEDE currently
>> carries a patch for that which just changes the hard-coded default to
>> another hard-coded default. Not sure how to get a good value to use,
>> though; and deciding on how large a fraction of memory to use for
>> packets starts smelling an awful lot like setting policy in the
>> kernel, doesn't it?
>
> Yeah, I agree it does seem awkward.
>
> Perhaps we should instead pick a low limit and let users change it
> more easily (i.e. not debugfs)? I don't know a good answer to this
> either.

Hmm, I'll talk it over with some of the LEDE people who are more used to
dealing with these sorts of memory-constrained devices than I am. Will
send a patch if we come up with a good solution :)

-Toke

Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> Applied, with the nits fixed as discussed.

Awesome, thanks!

> Come to think of it, if somebody is bored ;-) perhaps a hwsim option
> to use TXQs (should be optional I guess) would be nice so we can
> exercise this code with the wpa_supplicant hwsim tests. That would
> have caught the TKIP issues etc. pretty early on too, I think.

Noted. I'll look into that the next time I'm bored ;)

-Toke

[PATCH v2] ath10k: allow setting coverage class

[Kalle Valo]

From: Benjamin Berg 

Unfortunately ath10k does not generally allow modifying the coverage class
with the stock firmware and Qualcomm has so far refused to implement this
feature so that it can be properly supported in ath10k. If we however know
the registers that need to be modified for proper operation with a higher
coverage class, then we can do these modifications from the driver.

This is a hack and might cause subtle problems but as it's not enabled by
default (only when user space changes the coverage class explicitly) it should
not cause new problems for existing setups. But still this should be considered
as an experimental feature and used with caution.

This patch implements the support for first generation cards (QCA9880, QCA9887
and so on) which are based on a core that is similar to ath9k. The registers
are modified in place and need to be re-written every time the firmware sets
them. To achieve this the register status is verified after certain WMI events
from the firmware.

The coverage class may not be modified temporarily right after the card
re-initializes the registers. This is for example the case during scanning.

Thanks to Sebastian Gottschall  for initially
working on a userspace support for this. This patch wouldn't have been
possible without this documentation.

Signed-off-by: Benjamin Berg 
Signed-off-by: Simon Wunderlich 
Signed-off-by: Mathias Kretschmer 
Signed-off-by: Kalle Valo 
---

v2:

* cc linux-wireless
* fold "ath10k: Fix spinlock use in coverage class hack" into this one 
(Benjamin)
* emphasise more on the commit log that this is a hack
* remove unnecessary use of unlikely(), this is not in hotpath
* workaround ifdef CONFIG_ATH10K_DEBUGFS with adding
  ath10k_debug_get_fw_dbglog_[mask|level]()

 drivers/net/wireless/ath/ath10k/core.c  |   11 +++
 drivers/net/wireless/ath/ath10k/core.h  |   13 +++
 drivers/net/wireless/ath/ath10k/debug.h |   22 +
 drivers/net/wireless/ath/ath10k/hw.c|  142 +++
 drivers/net/wireless/ath/ath10k/hw.h|   28 +-
 drivers/net/wireless/ath/ath10k/mac.c   |   19 +
 drivers/net/wireless/ath/ath10k/wmi.c   |   48 +++
 7 files changed, 282 insertions(+), 1 deletion(-)

diff --git a/drivers/net/wireless/ath/ath10k/core.c 
b/drivers/net/wireless/ath/ath10k/core.c
index 867a8403b15b..7005e2a98726 100644
--- a/drivers/net/wireless/ath/ath10k/core.c
+++ b/drivers/net/wireless/ath/ath10k/core.c
@@ -1562,6 +1562,15 @@ static void ath10k_core_restart(struct work_struct *work)
mutex_unlock(>conf_mutex);
 }
 
+static void ath10k_core_set_coverage_class_work(struct work_struct *work)
+{
+   struct ath10k *ar = container_of(work, struct ath10k,
+set_coverage_class_work);
+
+   if (ar->hw_params.hw_ops->set_coverage_class)
+   ar->hw_params.hw_ops->set_coverage_class(ar, -1);
+}
+
 static int ath10k_core_init_firmware_features(struct ath10k *ar)
 {
struct ath10k_fw_file *fw_file = >normal_mode_fw.fw_file;
@@ -2344,6 +2353,8 @@ struct ath10k *ath10k_core_create(size_t priv_size, 
struct device *dev,
 
INIT_WORK(>register_work, ath10k_core_register_work);
INIT_WORK(>restart_work, ath10k_core_restart);
+   INIT_WORK(>set_coverage_class_work,
+ ath10k_core_set_coverage_class_work);
 
init_dummy_netdev(>napi_dev);
 
diff --git a/drivers/net/wireless/ath/ath10k/core.h 
b/drivers/net/wireless/ath/ath10k/core.h
index c091306bca33..6e5aa2d09699 100644
--- a/drivers/net/wireless/ath/ath10k/core.h
+++ b/drivers/net/wireless/ath/ath10k/core.h
@@ -912,6 +912,19 @@ struct ath10k {
struct net_device napi_dev;
struct napi_struct napi;
 
+   struct work_struct set_coverage_class_work;
+   /* protected by conf_mutex */
+   struct {
+   /* writing also protected by data_lock */
+   s16 coverage_class;
+
+   u32 reg_phyclk;
+   u32 reg_slottime_conf;
+   u32 reg_slottime_orig;
+   u32 reg_ack_cts_timeout_conf;
+   u32 reg_ack_cts_timeout_orig;
+   } fw_coverage;
+
/* must be last */
u8 drv_priv[0] __aligned(sizeof(void *));
 };
diff --git a/drivers/net/wireless/ath/ath10k/debug.h 
b/drivers/net/wireless/ath/ath10k/debug.h
index c458fa96a6d4..335512b11ca2 100644
--- a/drivers/net/wireless/ath/ath10k/debug.h
+++ b/drivers/net/wireless/ath/ath10k/debug.h
@@ -94,7 +94,19 @@ int ath10k_debug_get_et_sset_count(struct ieee80211_hw *hw,
 void ath10k_debug_get_et_stats(struct ieee80211_hw *hw,
   struct ieee80211_vif *vif,
   struct ethtool_stats *stats, u64 *data);
+
+static inline u64 ath10k_debug_get_fw_dbglog_mask(struct ath10k *ar)
+{
+   return 

Re: pull-request: wireless-drivers-next 2016-09-29

[Aaron Conole]

David Miller  writes:

> From: Kalle Valo 
> Date: Thu, 29 Sep 2016 19:57:28 +0300
>
...
>> Or actually I had one problem. While doing a test merge I noticed that
>> net-next fails to compile for me, but I don't think this is anything
>> wireless related:
>> 
>>   CC  net/netfilter/core.o
>> net/netfilter/core.c: In function 'nf_set_hooks_head':
>> net/netfilter/core.c:96:149: error: 'struct net_device' has no
>> member named 'nf_hooks_ingress'
>
> Yes, I am aware of this build issue and will tackle it myself if someone
> doesn't beat me to it.

Sorry, I introduced this.  I posted a series targetted at nf-next to
solve this, but it could be merged to net-next instead, if that makes
sense.

The patches are here:

https://patchwork.ozlabs.org/patch/676287/
https://patchwork.ozlabs.org/patch/676288/


Again, sorry for this.

Re: [PATCH 2/2] Revert "staging: wilc1000: Replace kthread with workqueue for host interface"

[Greg KH]

On Fri, Sep 30, 2016 at 03:43:18PM +0530, Aditya Shankar wrote:
> This reverts commit 2518ac59eb27 ("staging: wilc1000: Replace kthread
> with workqueue for host interface")
> 
> This commit breaks wilc1000 driver init. A crash was seen
> everytime the wlan interface was brought up and wilc device
> open was attempted. This change is being reverted until we
> figure out the problem in this change. The driver is
> usable now with this change reverted.
> 
> Signed-off-by: Aditya Shankar 
> 
> Conflicts:
>   drivers/staging/wilc1000/host_interface.c

What is this line doing here?

And shouldn't we add a cc: stable tag as well?  Or at the least, put a
"fixes:" tag to let people know exactly what commit it is fixing (the
id that it is reverting.)

thanks,

greg k-h

Re: [PATCH 1/3] cw1200: Don't leak memory if krealloc failes

[Johannes Thumshirn]

On Fri, Sep 30, 2016 at 03:56:45PM +0300, Sergei Shtylyov wrote:
> Hello.
> 
> On 9/30/2016 3:11 PM, Johannes Thumshirn wrote:
> 
> > The call to krealloc() in wsm_buf_reserve() directly assigns the newly
> > returned memory to buf->begin. This is all fine except when krealloc()
> > failes we loose the ability to free the old memory pointed to by
> 
>Fails.
> 
> > buf->begin. If we just create a temporary variable to assign memory to
> > and assign the memory to it we can mitigate the memory leak.
> > 
> > Signed-off-by: Johannes Thumshirn 
> > ---
> >  drivers/net/wireless/st/cw1200/wsm.c | 16 +---
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/st/cw1200/wsm.c 
> > b/drivers/net/wireless/st/cw1200/wsm.c
> > index 680d60e..12fad99 100644
> > --- a/drivers/net/wireless/st/cw1200/wsm.c
> > +++ b/drivers/net/wireless/st/cw1200/wsm.c
> > @@ -1807,16 +1807,18 @@ static int wsm_buf_reserve(struct wsm_buf *buf, 
> > size_t extra_size)
> >  {
> > size_t pos = buf->data - buf->begin;
> > size_t size = pos + extra_size;
> > +   u8 *tmp;
> > 
> > size = round_up(size, FWLOAD_BLOCK_SIZE);
> > 
> > -   buf->begin = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
> > -   if (buf->begin) {
> > -   buf->data = >begin[pos];
> > -   buf->end = >begin[size];
> > -   return 0;
> > -   } else {
> > -   buf->end = buf->data = buf->begin;
> > +   tmp = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
> > +   if (tmp) {
> 
>!tmp, you mean?

Yes, I've already sent out a v2.

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH] ath10k: Fix spinlock use in coverage class hack

[Valo, Kalle]

Benjamin Berg  writes:

> ath10k_hw_qca988x_set_coverage_class needs to hold both conf_mutex and
> the data_lock spin lock for parts of the function. However, data_lock
> is only needed while storing the coverage_class to store the value that
> the card is configured to.
>
> Fix the locking issue by only holding data_lock for the required duration.
>
> Signed-off-by: Benjamin Berg 

Thanks, I also folded this with the patch in the pending branch.

> And yes, I fully agree with your points of it being rather fragile. But as
> you said, it should be entirely safe if not used.

That's good.

> Obviously a firmware implementation would be preferential.

That's a shame as this feature is quite often requested. But if the
firmware ever starts supporting the featrue we can then remove this hack
from ath10k.

> This locking issue was pretty unnecessary. Lets see if any more issues show
> up in a closer review.

I can't see the locking problem anymore so it seems to be fixed. I'll
fix some minor things and send v2. I'll also CC linux-wireless.

-- 
Kalle Valo

Re: [PATCH 1/3] cw1200: Don't leak memory if krealloc failes

[Sergei Shtylyov]

Hello.

On 9/30/2016 3:11 PM, Johannes Thumshirn wrote:


The call to krealloc() in wsm_buf_reserve() directly assigns the newly
returned memory to buf->begin. This is all fine except when krealloc()
failes we loose the ability to free the old memory pointed to by


   Fails.


buf->begin. If we just create a temporary variable to assign memory to
and assign the memory to it we can mitigate the memory leak.

Signed-off-by: Johannes Thumshirn 
---
 drivers/net/wireless/st/cw1200/wsm.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/st/cw1200/wsm.c 
b/drivers/net/wireless/st/cw1200/wsm.c
index 680d60e..12fad99 100644
--- a/drivers/net/wireless/st/cw1200/wsm.c
+++ b/drivers/net/wireless/st/cw1200/wsm.c
@@ -1807,16 +1807,18 @@ static int wsm_buf_reserve(struct wsm_buf *buf, size_t 
extra_size)
 {
size_t pos = buf->data - buf->begin;
size_t size = pos + extra_size;
+   u8 *tmp;

size = round_up(size, FWLOAD_BLOCK_SIZE);

-   buf->begin = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
-   if (buf->begin) {
-   buf->data = >begin[pos];
-   buf->end = >begin[size];
-   return 0;
-   } else {
-   buf->end = buf->data = buf->begin;
+   tmp = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
+   if (tmp) {


   !tmp, you mean?


+   wsm_buf_deinit(buf);
return -ENOMEM;
}
+
+   buf->begin = tmp;
+   buf->data = >begin[pos];
+   buf->end = >begin[size];
+   return 0;
 }


MBR, Sergei

Re: [PATCH 3/3] mac80211: Set lower memory limit for non-VHT devices

[Johannes Berg]

> > I kinda see the logic here - we really don't need to queue as much
> > if we can't possibly transmit it out quickly - but it seems to me
> > we should also throw in some kind of limit that's relative to the
> > amount of memory you have on the system?
> 
> Yes, ideally. That goes for FQ-CoDel as well, BTW. LEDE currently
> carries a patch for that which just changes the hard-coded default to
> another hard-coded default. Not sure how to get a good value to use,
> though; and deciding on how large a fraction of memory to use for
> packets starts smelling an awful lot like setting policy in the
> kernel, doesn't it?

Yeah, I agree it does seem awkward.

Perhaps we should instead pick a low limit and let users change it more
easily (i.e. not debugfs)? I don't know a good answer to this either.

johannes

Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

[Johannes Berg]

Applied, with the nits fixed as discussed.

Come to think of it, if somebody is bored ;-) perhaps a hwsim option to
use TXQs (should be optional I guess) would be nice so we can exercise
this code with the wpa_supplicant hwsim tests. That would have caught
the TKIP issues etc. pretty early on too, I think.

johannes

Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

>> Not sure if you want a v10, or if you're satisfied with the above
>> comments and will just fix up the nits on merging?
>> 
>
> I'll fix it up. Thanks!

Cool, thanks :)

-Toke

Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

[Johannes Berg]

> Because I need to run it anyway for the xmit_fast path on dequeue. I
> thought doing it this way simplifies the code (at the cost of the
> handler getting called twice when xmit_fast is not active).

Ok, that's fair.

> > I *think* it should commute with the rate control handler, but even
> > so, wouldn't it make more sense to have rate control late? Assuming
> > the packets are queued for some amount of time, having rate control
> > information queued with them would get stale.
> 
> Yes, having rate control run at dequeue would be good, and that's
> what I did initially. However, I found that this would lead to a
> deadlock because the rate control handler would send out packets in
> some cases (I forget the details but can go back and check if
> needed). And since the dequeue function is called with the driver TXQ
> lock held, that would lead to a deadlock when those packets made it
> to the driver TX path.

That seems really odd, but I can see how a deadlock happens then.

> So I decided to just keep it this way for now; I plan to go poking
> into the rate controller later anyway, so moving the handler to later
> could be part of that.

Sure, that's fair.

> But that handler only sets a few flags? Is
> tx->sdata->control_port_protocol likely to change while the packet is
> queued?

Oh right, I confused things there. We check the controlled port much
earlier, but anyway that should be OK.

> > It's a bit unfortunate that you lose fast-xmit here completely for
> > the key stuff, but I don't see a good way to avoid that, other than
> > completely rejiggering all the (possibly affected) queues when keys
> > change... might be very complex to do that, certainly a follow-up
> > patch if it's desired.
> 
> Yeah, figured it was better to have something that's correct and then
> go back and change it if the performance hit turns out to be too
> high.

Makes sense.

> > This check seems a bit weird though - how could fast-xmit be set
> > without a TXQ station?
> 
> I think that is probably just left over from before I introduced the
> control flag. Should be fine to remove it.

Ok.

> > 
> > > 
> > > +++ b/net/mac80211/util.c
> > > @@ -3393,11 +3393,18 @@ void ieee80211_txq_get_depth(struct
> > > ieee80211_txq *txq,
> > >    unsigned long *byte_cnt)
> > >  {
> > >   struct txq_info *txqi = to_txq_info(txq);
> > > + u32 frag_cnt = 0, frag_bytes = 0;
> > > + struct sk_buff *skb;
> > > +
> > > + skb_queue_walk(>frags, skb) {
> > > + frag_cnt++;
> > > + frag_bytes += skb->len;
> > > + }
> > 
> > I hope this is called infrequently :)
> 
> Well, ath10k is the only user. It does get called on each
> wake_tx_queue, though, so not that infrequently. My reasoning was
> that since the frags queue is never going to have more than a fairly
> small number of packets in it (those produced from a single split
> packet), counting this way is acceptable instead of keeping a state
> variable up to date. Can change it if you disagree :)

No, I guess you're right, it can't be a long queue.

> Not sure if you want a v10, or if you're satisfied with the above
> comments and will just fix up the nits on merging?
> 

I'll fix it up. Thanks!

johannes

Re: [PATCH 3/3] mac80211: Set lower memory limit for non-VHT devices

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> On Fri, 2016-09-23 at 21:59 +0200, Toke Høiland-Jørgensen wrote:
>> Small devices can run out of memory from queueing too many packets.
>> If VHT is not supported by the PHY, having more than 4 MBytes of
>> total queue in the TXQ intermediate queues is not needed, and so we
>> can safely limit the memory usage in these cases and avoid OOM.
>
> I kinda see the logic here - we really don't need to queue as much if
> we can't possibly transmit it out quickly - but it seems to me we
> should also throw in some kind of limit that's relative to the amount
> of memory you have on the system?

Yes, ideally. That goes for FQ-CoDel as well, BTW. LEDE currently
carries a patch for that which just changes the hard-coded default to
another hard-coded default. Not sure how to get a good value to use,
though; and deciding on how large a fraction of memory to use for
packets starts smelling an awful lot like setting policy in the kernel,
doesn't it?

> I've applied these anyway though. I just don't like your assumption (b)
> much as the rationale for it.

Right, thanks. I'll come up with a better rationale next time ;)

-Toke

[PATCH v2] cw1200: Don't leak memory if krealloc failes

[Johannes Thumshirn]

The call to krealloc() in wsm_buf_reserve() directly assigns the newly
returned memory to buf->begin. This is all fine except when krealloc()
failes we loose the ability to free the old memory pointed to by
buf->begin. If we just create a temporary variable to assign memory to
and assign the memory to it we can mitigate the memory leak.

Signed-off-by: Johannes Thumshirn 
Cc: Johannes Berg 
---
 drivers/net/wireless/st/cw1200/wsm.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

Changes from v1:
* Do the correct check for a failed re-allocation (Johannes Berg)

diff --git a/drivers/net/wireless/st/cw1200/wsm.c 
b/drivers/net/wireless/st/cw1200/wsm.c
index 680d60e..ffb245e 100644
--- a/drivers/net/wireless/st/cw1200/wsm.c
+++ b/drivers/net/wireless/st/cw1200/wsm.c
@@ -1807,16 +1807,18 @@ static int wsm_buf_reserve(struct wsm_buf *buf, size_t 
extra_size)
 {
size_t pos = buf->data - buf->begin;
size_t size = pos + extra_size;
+   u8 *tmp;
 
size = round_up(size, FWLOAD_BLOCK_SIZE);
 
-   buf->begin = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
-   if (buf->begin) {
-   buf->data = >begin[pos];
-   buf->end = >begin[size];
-   return 0;
-   } else {
-   buf->end = buf->data = buf->begin;
+   tmp = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
+   if (!tmp) {
+   wsm_buf_deinit(buf);
return -ENOMEM;
}
+
+   buf->begin = tmp;
+   buf->data = >begin[pos];
+   buf->end = >begin[size];
+   return 0;
 }
-- 
1.8.5.6

Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

[Toke Høiland-Jørgensen]

Johannes Berg  writes:

> Hi Toke,
>
> Sorry for the delay reviewing this.
>
> I think I still have a few comments/questions.

No worries. And not terribly surprised ;)

>> +static inline bool txq_has_queue(struct ieee80211_txq *txq)
>> +{
>> +struct txq_info *txqi = to_txq_info(txq);
>> +return !(skb_queue_empty(>frags) && !txqi->tin.backlog_packets);
>> +}
>
> Tiny nit - there should probably be a blank line between the two lines
> here, but I could just fix that when I apply if you don't resend anyway
> for some other reason.

Noted.

> [snip helper stuff that looks fine]
>
>> -if (!tx->sta->sta.txq[0])
>> -hdr->seq_ctrl = ieee80211_tx_next_seq(tx->sta, tid);
>> +hdr->seq_ctrl = ieee80211_tx_next_seq(tx->sta, tid);
>
> Just to make sure I get this right - this is because the handler is now
> run on dequeue, so the special case is no longer needed?

Yup. The same change is made in xmit_fast (but obscured by the moving of
the surrounding code into _finish()).

>>  #define CALL_TXH(txh) \
>> @@ -1656,6 +1684,31 @@ static int invoke_tx_handlers(struct 
>> ieee80211_tx_data *tx)
>>  if (!ieee80211_hw_check(>local->hw, HAS_RATE_CONTROL))
>>  CALL_TXH(ieee80211_tx_h_rate_ctrl);
>
> Just for reference - the code block here that's unchanged contains
> this:
>
>         CALL_TXH(ieee80211_tx_h_dynamic_ps);
> CALL_TXH(ieee80211_tx_h_check_assoc);
> CALL_TXH(ieee80211_tx_h_ps_buf);
> CALL_TXH(ieee80211_tx_h_check_control_port_protocol);
> CALL_TXH(ieee80211_tx_h_select_key);
> if (!ieee80211_hw_check(>local->hw, HAS_RATE_CONTROL))
> CALL_TXH(ieee80211_tx_h_rate_ctrl);
>
>> +static int invoke_tx_handlers_late(struct ieee80211_tx_data *tx)
>> +{
>> +struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
>> +ieee80211_tx_result res = TX_CONTINUE;
>> +
>>  if (unlikely(info->flags &
>> IEEE80211_TX_INTFL_RETRANSMISSION)) {
>>  __skb_queue_tail(>skbs, tx->skb);
>>  tx->skb = NULL;
>
> And this code here is also unchanged from the original TX handler
> invocation, so contains this:
>
>         if (unlikely(info->flags & IEEE80211_TX_INTFL_RETRANSMISSION)) {
> __skb_queue_tail(>skbs, tx->skb);
> tx->skb = NULL;
> goto txh_done;
> }
>
> CALL_TXH(ieee80211_tx_h_michael_mic_add);
> CALL_TXH(ieee80211_tx_h_sequence);
> CALL_TXH(ieee80211_tx_h_fragment);
> /* handlers after fragment must be aware of tx info fragmentation! */
> CALL_TXH(ieee80211_tx_h_stats);
> CALL_TXH(ieee80211_tx_h_encrypt);
> if (!ieee80211_hw_check(>local->hw, HAS_RATE_CONTROL))
> CALL_TXH(ieee80211_tx_h_calculate_duration);
>
> But now you have a problem (that you solved) that the key pointer can
> be invalidated while you have the packet queued between the two points,
> and then the tx_h_michael_mic_add and/or tx_h_encrypt would crash.
>
> You solve this by re-running tx_h_select_key() on dequeue, but it's not
> clear to me why you didn't move that to the late handlers instead?

Because I need to run it anyway for the xmit_fast path on dequeue. I
thought doing it this way simplifies the code (at the cost of the
handler getting called twice when xmit_fast is not active).

> I *think* it should commute with the rate control handler, but even
> so, wouldn't it make more sense to have rate control late? Assuming
> the packets are queued for some amount of time, having rate control
> information queued with them would get stale.

Yes, having rate control run at dequeue would be good, and that's what I
did initially. However, I found that this would lead to a deadlock
because the rate control handler would send out packets in some cases (I
forget the details but can go back and check if needed). And since the
dequeue function is called with the driver TXQ lock held, that would
lead to a deadlock when those packets made it to the driver TX path.

So I decided to just keep it this way for now; I plan to go poking into
the rate controller later anyway, so moving the handler to later could
be part of that.

> Similarly, it seems to me that checking the control port protocol later
> (or perhaps duplicating that?) would be a good idea?

But that handler only sets a few flags? Is
tx->sdata->control_port_protocol likely to change while the packet is
queued?

>> +/*
>> + * Can be called while the sta lock is held. Anything that can cause
>> packets to
>> + * be generated will cause deadlock!
>> + */
>> +static bool ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data
>> *sdata,
>> +   struct sta_info *sta, u8
>> pn_offs,
>> +   struct ieee80211_key *key,
>> +   struct sk_buff *skb)
>
> That should be a void function now, you never check the return value
> and only return true 

Re: [PATCH 1/3] cw1200: Don't leak memory if krealloc failes

[Johannes Thumshirn]

On Fri, Sep 30, 2016 at 02:29:39PM +0200, Johannes Berg wrote:
> 
> > +   tmp = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
> > +   if (tmp) {
> > 
> I think that check is inverted?
> 
> johannes

Fu.. you're right, of cause it's !tmp.

I'll resend.

Thanks,
Johannes

-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850

Re: [PATCH 1/3] cw1200: Don't leak memory if krealloc failes

[Johannes Berg]

> + tmp = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
> + if (tmp) {
> 
I think that check is inverted?

johannes

[PATCH 1/3] cw1200: Don't leak memory if krealloc failes

[Johannes Thumshirn]

The call to krealloc() in wsm_buf_reserve() directly assigns the newly
returned memory to buf->begin. This is all fine except when krealloc()
failes we loose the ability to free the old memory pointed to by
buf->begin. If we just create a temporary variable to assign memory to
and assign the memory to it we can mitigate the memory leak.

Signed-off-by: Johannes Thumshirn 
---
 drivers/net/wireless/st/cw1200/wsm.c | 16 +---
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/net/wireless/st/cw1200/wsm.c 
b/drivers/net/wireless/st/cw1200/wsm.c
index 680d60e..12fad99 100644
--- a/drivers/net/wireless/st/cw1200/wsm.c
+++ b/drivers/net/wireless/st/cw1200/wsm.c
@@ -1807,16 +1807,18 @@ static int wsm_buf_reserve(struct wsm_buf *buf, size_t 
extra_size)
 {
size_t pos = buf->data - buf->begin;
size_t size = pos + extra_size;
+   u8 *tmp;
 
size = round_up(size, FWLOAD_BLOCK_SIZE);
 
-   buf->begin = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
-   if (buf->begin) {
-   buf->data = >begin[pos];
-   buf->end = >begin[size];
-   return 0;
-   } else {
-   buf->end = buf->data = buf->begin;
+   tmp = krealloc(buf->begin, size, GFP_KERNEL | GFP_DMA);
+   if (tmp) {
+   wsm_buf_deinit(buf);
return -ENOMEM;
}
+
+   buf->begin = tmp;
+   buf->data = >begin[pos];
+   buf->end = >begin[size];
+   return 0;
 }
-- 
1.8.5.6

Re: [PATCH 0/4] ath10k: fix mesh sync operation

[Johannes Berg]

On Wed, 2016-09-28 at 16:56 -0700, Thomas Pedersen wrote:
> This patchset introduces a new ieee80211_op offset_tsf(), which gives
> the driver a s64 TSF offset for TSF adjustment. This is more accurate
> than programming absolute TSF since programming delay is avoided.
> 

You were lucky we have patchwork, otherwise I would've missed the
mac80211 patches in this series :)

Applied both now - no reason to apply them in the order you sent them.

johannes

Re: [PATCH] mac80211: fix CMD_FRAME for AP_VLAN

[Johannes Berg]

>  net/mac80211/offchannel.c | 5 -
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/net/mac80211/offchannel.c b/net/mac80211/offchannel.c
> index 55a9c5b..2afd329 100644
> --- a/net/mac80211/offchannel.c
> +++ b/net/mac80211/offchannel.c
> @@ -819,7 +819,10 @@ int ieee80211_mgmt_tx(struct wiphy *wiphy,
> struct wireless_dev *wdev,
>   mgmt->u.action.category ==
> WLAN_CATEGORY_SPECTRUM_MGMT)
>   break;
>   rcu_read_lock();
> - sta = sta_info_get(sdata, mgmt->da);
> + if (ieee80211_vif_is_mesh(>vif))
> + sta = sta_info_get(sdata, mgmt->da);
> + else
> + sta = sta_info_get_bss(sdata, mgmt->da);
> 
I don't see why you need to distinguish between mesh and non-mesh here?
get_bss() will ignore the BSS pointer if it's NULL, and that will
always be the case when the type is mesh, so ... why?

johannes

Re: [PATCH 3/3] mac80211: Set lower memory limit for non-VHT devices

[Johannes Berg]

On Fri, 2016-09-23 at 21:59 +0200, Toke Høiland-Jørgensen wrote:
> Small devices can run out of memory from queueing too many packets.
> If VHT is not supported by the PHY, having more than 4 MBytes of
> total queue in the TXQ intermediate queues is not needed, and so we
> can safely limit the memory usage in these cases and avoid OOM.

I kinda see the logic here - we really don't need to queue as much if
we can't possibly transmit it out quickly - but it seems to me we
should also throw in some kind of limit that's relative to the amount
of memory you have on the system?

I've applied these anyway though. I just don't like your assumption (b)
much as the rationale for it.

johannes

Re: [PATCH v3 1/9] cfg80211: add start / stop NAN commands

[Johannes Berg]

All applied.

johannes

Re: [PATCH] cfg80211: Add support for static WEP in the driver

[Johannes Berg]

On Thu, 2016-09-22 at 23:16 +0300, Luca Coelho wrote:
> From: David Spinadel 
> 
> Add support for drivers that implement static WEP internally, i.e.
> expose connection keys to the driver in connect flow and don't
> upload the keys after the connection.
> 
Applied.

johannes

Re: [PATCH v9 1/2] mac80211: Move ieee802111_tx_dequeue() to later in tx.c

[Johannes Berg]

On Thu, 2016-09-22 at 19:04 +0200, Toke Høiland-Jørgensen wrote:
> The TXQ path restructure requires ieee80211_tx_dequeue() to call TX
> handlers and parts of the xmit_fast path. Move the function to later
> in tx.c in preparation for this.
> 
Applied.

johannes

Re: [PATCH v9 2/2] mac80211: Move reorder-sensitive TX handlers to after TXQ dequeue

[Johannes Berg]

Hi Toke,

Sorry for the delay reviewing this.

I think I still have a few comments/questions.

> +static inline bool txq_has_queue(struct ieee80211_txq *txq)
> +{
> + struct txq_info *txqi = to_txq_info(txq);
> + return !(skb_queue_empty(>frags) && !txqi->tin.backlog_packets);
> +}

Tiny nit - there should probably be a blank line between the two lines
here, but I could just fix that when I apply if you don't resend anyway
for some other reason.

[snip helper stuff that looks fine]

> - if (!tx->sta->sta.txq[0])
> - hdr->seq_ctrl = ieee80211_tx_next_seq(tx->sta, tid);
> + hdr->seq_ctrl = ieee80211_tx_next_seq(tx->sta, tid);

Just to make sure I get this right - this is because the handler is now
run on dequeue, so the special case is no longer needed?

>  #define CALL_TXH(txh) \
> @@ -1656,6 +1684,31 @@ static int invoke_tx_handlers(struct ieee80211_tx_data 
> *tx)
>   if (!ieee80211_hw_check(>local->hw, HAS_RATE_CONTROL))
>   CALL_TXH(ieee80211_tx_h_rate_ctrl);

Just for reference - the code block here that's unchanged contains
this:

        CALL_TXH(ieee80211_tx_h_dynamic_ps);
CALL_TXH(ieee80211_tx_h_check_assoc);
CALL_TXH(ieee80211_tx_h_ps_buf);
CALL_TXH(ieee80211_tx_h_check_control_port_protocol);
CALL_TXH(ieee80211_tx_h_select_key);
if (!ieee80211_hw_check(>local->hw, HAS_RATE_CONTROL))
CALL_TXH(ieee80211_tx_h_rate_ctrl);

> +static int invoke_tx_handlers_late(struct ieee80211_tx_data *tx)
> +{
> + struct ieee80211_tx_info *info = IEEE80211_SKB_CB(tx->skb);
> + ieee80211_tx_result res = TX_CONTINUE;
> +
>   if (unlikely(info->flags &
> IEEE80211_TX_INTFL_RETRANSMISSION)) {
>   __skb_queue_tail(>skbs, tx->skb);
>   tx->skb = NULL;

And this code here is also unchanged from the original TX handler
invocation, so contains this:

        if (unlikely(info->flags & IEEE80211_TX_INTFL_RETRANSMISSION)) {
__skb_queue_tail(>skbs, tx->skb);
tx->skb = NULL;
goto txh_done;
}

CALL_TXH(ieee80211_tx_h_michael_mic_add);
CALL_TXH(ieee80211_tx_h_sequence);
CALL_TXH(ieee80211_tx_h_fragment);
/* handlers after fragment must be aware of tx info fragmentation! */
CALL_TXH(ieee80211_tx_h_stats);
CALL_TXH(ieee80211_tx_h_encrypt);
if (!ieee80211_hw_check(>local->hw, HAS_RATE_CONTROL))
CALL_TXH(ieee80211_tx_h_calculate_duration);

But now you have a problem (that you solved) that the key pointer can
be invalidated while you have the packet queued between the two points,
and then the tx_h_michael_mic_add and/or tx_h_encrypt would crash.

You solve this by re-running tx_h_select_key() on dequeue, but it's not
clear to me why you didn't move that to the late handlers instead?

I *think* it should commute with the rate control handler, but even so,
wouldn't it make more sense to have rate control late? Assuming the
packets are queued for some amount of time, having rate control
information queued with them would get stale.

Similarly, it seems to me that checking the control port protocol later
(or perhaps duplicating that?) would be a good idea?


> +/*
> + * Can be called while the sta lock is held. Anything that can cause
> packets to
> + * be generated will cause deadlock!
> + */
> +static bool ieee80211_xmit_fast_finish(struct ieee80211_sub_if_data
> *sdata,
> +    struct sta_info *sta, u8
> pn_offs,
> +    struct ieee80211_key *key,
> +    struct sk_buff *skb)

That should be a void function now, you never check the return value
and only return true anyway.

> + struct ieee80211_tx_info *info;
> + struct ieee80211_tx_data tx;
> + ieee80211_tx_result r;
> +

nit: extra blank line

>   spin_lock_bh(>lock);
>  
>   if (test_bit(IEEE80211_TXQ_STOP, >flags))
>   goto out;
>  
> + /* Make sure fragments stay together. */
> + skb = __skb_dequeue(>frags);
> + if (skb)
> + goto out;
> +
> +begin:

I guess now that you introduced that anyway, we should consider making
the skb_linearize() failure go there. Should be a follow-up patch
though.

> + /*
> +  * The key can be removed while the packet was queued, so
> need to call
> +  * this here to get the current key.
> +  */
> + r = ieee80211_tx_h_select_key();
> + if (r != TX_CONTINUE) {
> + ieee80211_free_txskb(>hw, skb);
> + goto begin;
> + }
> +
> + if (txq->sta && info->control.flags & IEEE80211_TX_CTRL_FAST_XMIT) {

It's a bit unfortunate that you lose fast-xmit here completely for the
key stuff, but I don't see a good way to avoid that, other than
completely rejiggering all the (possibly affected) queues when keys
change... might be very complex to do that, certainly a follow-up patch
if it's desired.

This check 

Re: [PATCHv3] wireless: check A-MSDU inner frame source address on AP interfaces

[Johannes Berg]

A few more things:

First of all - there's nothing specific to "AP interfaces", which you
say in the subject, as far as I can tell? That should be removed?

On Wed, 2016-09-28 at 17:14 +0200, Michael Braun wrote:
> When using WPA security, the station and thus the required key is
> identified by its mac address when packets are received. So a
> station usually cannot spoof its source mac address.
> 
> But when a station sends an A-MSDU frame, port control and crypto
> is done using the outer mac address, while the packets delivered
> and forwarded use the inner mac address.
> 
> IEEE 802.11-2012 mandates that the outer source mac address should
> match the inner source address (section 8.3.2.2). For the
> destination mac address, matching is not required (section 10.23.15).

I think this is wrong. As we do not support DMS (yet), we should adhere
to 8.3.2.2 and only accept matching TA/SA and DA/RA.

And even when we add DMS, we should also restrict it to multicast
addresses, but that's future work anyway.

> > -   if (ieee80211_data_to_8023(pkt, vif->addr, vif->type))
> > +   if (ieee80211_data_to_8023(pkt, NULL, vif->addr,
> > +   vif->type))

indentation is bad here - consider running checkpatch

>  static int mwifiex_11n_dispatch_amsdu_pkt(struct mwifiex_private *priv,
> > -     struct sk_buff *skb)
> > +     struct sk_buff *skb,
> > +     const u8 *ta)

[... more mwifiex changes ...]

It's great that you're doing this, but I think this patch should only
carry the bare minimum change for mwifiex to keep it compiling, with a
follow-up patch that actually implements the correct checks. That way,
should any issues arise, it's easier to determine where the problem is
and to fix/revert it.

> diff --git a/drivers/staging/rtl8723au/core/rtw_recv.c 
> b/drivers/staging/rtl8723au/core/rtw_recv.c
> index 150dabc..38ba7dd 100644
> --- a/drivers/staging/rtl8723au/core/rtw_recv.c
> +++ b/drivers/staging/rtl8723au/core/rtw_recv.c
> @@ -1687,7 +1687,7 @@ int amsdu_to_msdu(struct rtw_adapter *padapter, struct 
> recv_frame *prframe)
> >     skb_pull(skb, prframe->attrib.hdrlen);
> >     __skb_queue_head_init(_list);
>  
> > -   ieee80211_amsdu_to_8023s(skb, _list, NULL, 0, 0, false);
> > +   ieee80211_amsdu_to_8023s(skb, _list, NULL, 0, 0, pattrib->ta);

A bit debatable here, but I'd actually also prefer you add NULL here
first and then submit a separate patch that puts the right thing.

Not that it actually matters, since this driver has been removed
already... since you have to resubmit anyway though, I'd prefer you
just put NULL, and then not worry about it from there on.

> > -   if (has_80211_header) {
> > -   err = __ieee80211_data_to_8023(skb, , addr, iftype);
> > -   if (err)
> > -   goto out;
> > -   }

Good approach to split that :)

> > +   if (unlikely(ta &&
> > +    (iftype == NL80211_IFTYPE_AP ||
> > +     iftype == NL80211_IFTYPE_AP_VLAN) &&
> > +    !ether_addr_equal(ta, eth.h_source)
> > +      ))
> > +   goto purge;

Coding style here is very odd. All those closing parens should be on
the line before.

Thanks,
johannes

Re: [PATCH 3/3] mac80211: cache the only AP_VLAN station

[Johannes Berg]

On Sun, 2016-09-25 at 18:39 +0200, Michael Braun wrote:
> If an AP_VLAN is only used with one station, cache a pointer to this
> station to avoid lookup. This should speed up station lookup when 

"should speed up"? Do you have numbers to back that up?

Read this as - you need to do more to convince me that the double
accounting and software complexity is worth it.

And then, if we really want/need to have the double accounting and all
the associated complexity and potential problems, perhaps making per-
interface lists, perhaps even replacing the global list (since we still
have a global hash table), or something similar would make more sense?

johannes

Re: [PATCH v3 1/3] Documentation: dt: net: add mt76 wireless device binding

[Felix Fietkau]

On 2016-09-30 00:41, Arnd Bergmann wrote:
> On Thursday 29 September 2016, Felix Fietkau wrote:
>> On 2016-09-08 12:54, Kalle Valo wrote:
>> > Felix Fietkau  writes:
>> > 
>> >> Signed-off-by: Felix Fietkau 
>> >> ---
>> >>  .../bindings/net/wireless/mediatek,mt76.txt| 26 
>> >> ++
>> >>  1 file changed, 26 insertions(+)
>> >>  create mode 100644 
>> >> Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
>> >>
>> >> diff --git 
>> >> a/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt 
>> >> b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
>> >> new file mode 100644
>> >> index 000..d51c35f
>> >> --- /dev/null
>> >> +++ b/Documentation/devicetree/bindings/net/wireless/mediatek,mt76.txt
>> >> @@ -0,0 +1,26 @@
>> >> +* MediaTek mt76xx devices
>> >> +
>> >> +This node provides properties for configuring the MediaTek mt76xx 
>> >> wireless
>> >> +device. The node is expected to be specified as a child node of the PCI
>> >> +controller to which the wireless chip is connected.
>> >> +
>> >> +Optional properties:
>> >> +
>> >> +- mac-address: See ethernet.txt in the parent directory
>> >> +- local-mac-address: See ethernet.txt in the parent directory
>> >> +- mediatek,2ghz: Override the 2.4 GHz band capability from EEPROM
>> >> +- mediatek,5ghz: Override the 5 GHz band capability from EEPROM
>> >> +- mediatek,mtd-eeprom: Specify a MTD partition + offset containing 
>> >> EEPROM data
>> >> +
>> >> + {
>> >> + status = "okay";
>> >> +
>> >> + pcie0 {
>> >> + mt76@0,0 {
>> >> + reg = <0x 0 0 0 0>;
> 
> Maybe have an examplep of a real register address other than zero?
This is a real example referring to the first device on a PCI bus.
I copy this from a .dts file that we use in LEDE.

>> >> + device_type = "pci";
>> >> + mediatek,mtd-eeprom = < 0x8000>;
>> >> + mediatek,2ghz = <0>;
> 
> It's not clear what the possible values for the 2ghz property are,
> can you be more verbose in the description? How is <0> different
> from no property?
0 means disabled, no property means unchanged (compared to EEPROM).

- Felix

Re: [PATCH] brcmfmac: implement more accurate skb tracking

[Arend Van Spriel]

On 29-9-2016 23:57, Rafał Miłecki wrote:
> On 27 September 2016 at 11:24, Arend Van Spriel
>  wrote:
>> On 26-9-2016 14:38, Rafał Miłecki wrote:
>>> On 26 September 2016 at 14:13, Rafał Miłecki  wrote:
 On 26 September 2016 at 13:46, Arend Van Spriel
  wrote:
> On 26-9-2016 12:23, Rafał Miłecki wrote:
>> From: Rafał Miłecki 
>>
>> We need to track 802.1x packets to know if there are any pending ones
>> for transmission. This is required for performing key update in the
>> firmware.
>
> The problem we are trying to solve is a pretty old one. The problem is
> that wpa_supplicant uses two separate code paths: EAPOL messaging
> through data path and key configuration though nl80211.

 Can I find it described/reported somewhere?


>> Unfortunately our old tracking code wasn't very accurate. It was
>> treating skb as pending as soon as it was passed by the netif. Actual
>> handling packet to the firmware was happening later as brcmfmac
>> internally queues them and uses its own worker(s).
>
> That does not seem right. As soon as we get a 1x packet we need to wait
> with key configuration regardless whether it is still in the driver or
> handed over to firmware already.

 OK, thanks.
>>>
>>> Actually, it's not OK. I was trying to report/describe/discuss this
>>> problem for over a week. I couldn't get much of answer from you.
>>>
>>> I had to come with a patch I worked on for quite some time. Only then
>>> you decided to react and reply with a reason for a nack. I see this
>>> patch may be wrong (but it's still hard to know what's going wrong
>>> without a proper hostapd bug report). I'd expect you to somehow work &
>>> communicate with open source community.
>>
>> We do or at least make an honest attempt, but there is more on our plate
>> so responses may be delayed. It also does not help when you get anal and
>> preachy when we do respond. Also not OK. In this case the delay is
>> caused because I had to pick up the thread(s) as Hante is on vacation
>> (he needed a break :-p ). However, you started sending patches so I
>> decided to look at and respond to those. Sorry if you felt like we left
>> you hanging to dry.
> 
> I believe I get easily irritated due to my communication experience I
> got so far :(
> 
> 
> Over a year ago I reported brcmfmac can't recover from failed
> register_netdev(ice). This bug remains unfixed.
> 
> In 2014 I reported problem with 80 MHz support. I didn't have hardware
> to fix & test it on my own (you weren't able/allowed to send me one of
> your PCIe cards). In remained broken until I fixed it year later.
> 
> You missed my crash bug report about caused by missing eth_type_trans
> and came with patch on your own a month later.
> 
> Earlier this year I reported you problem with BCM4366 and multiple
> interfaces. I didn't get much help. 3 months later I came with patch
> to workaround the problem but you said there's a better way to do
> this. It took me 2 weeks to figure out a new wlioctl API for that
> while all I needed was a simple hint on "interface_remove".
> 
> Right now I'm waiting to get any answer from you about 4366c0
> firmware. It's still less than 2 weeks since I asked for it, but a
> simple ETA would be nice. I'm actually not sure if I should report
> more problems to you to don't distract you from pending things.

This is a difficult question. All upstream firmware releases for router
chips are put on hold until further notice. Some decisions have been
made, but I have not seen a detailed plan to give an ETA.

> Problems with brcmf_netdev_wait_pend8021x were reported multiples
> times for last few months. When I finally got time for that it took me
> a week to debug them.

For the pend8021x you were sending a number of messages showing debug
progress so not sure whether you wanted our feedback on that. If so a
ping might have done it.

> As you can see, it takes me months to get help on some things. And in
> few cases I never got much help at all. Yes, I was hoping to have you
> more involved into brcmfmac development and problems solving. I guess
> things didn't meet my expectations and I got grumpy & preachy.

Thanks for listing all our failures. Somehow we are very good at getting
each other grumpy. When we provide a patch and you break it up and
submit that to Kalle, we get grumpy and it all piles up to the point
where we have this kind of conversation. As long as it helps to get
things of our chest I can live with that. Hope you can too. We strive to
give support to the community, but the priority is low as it is not
full-time activity.

Regards,
Arend

Re: [PATCH 2/3] mac80211: multicast to unicast conversion

[Johannes Berg]

> +static ssize_t ieee80211_if_fmt_unicast(
> + const struct ieee80211_sub_if_data *sdata, char *buf, int
> buflen)

That's a very ... unusual way to break the lines here - please either
break after the first argument, or before the function name. You should
see lots of examples for both.

However, regardless of that, I don't think that debugfs is an
appropriate way to configure a production option like this.

> + int unicast;

bool, quite obviously.

> +/* rewrite destination mac address */
> +static int ieee80211_tx_dnat(struct sk_buff *skb, struct sta_info
> *sta)

How is this DNAT?

johannes

Re: ath10k stuck on 6Mbps and spam syslog

[Matteo Grandi]

Dear Ben,
Thank you for your reply.
The fact that stock ath10k firmware does not report tx-rate explains
why the station dump shows 6Mbps but the iperf test reach 14.6 (even
if it is really under the expected data rate...).
About the syslog spam, this is the code of the function that seems to
cause the error (I put a comment at line 2621 side). (The warning
start to spam as soon as I assign the channel "iw dev  set
channel )

/**
 * ieee80211_calculate_rx_timestamp - calculate timestamp in frame
 * @local: mac80211 hw info struct
 * @status: RX status
 * @mpdu_len: total MPDU length (including FCS)
 * @mpdu_offset: offset into MPDU to calculate timestamp at
 *
 * This function calculates the RX timestamp at the given MPDU offset, taking
 * into account what the RX timestamp was. An offset of 0 will just normalize
 * the timestamp to TSF at beginning of MPDU reception.
 */
u64 ieee80211_calculate_rx_timestamp(struct ieee80211_local *local,
struct ieee80211_rx_status *status,
unsigned int mpdu_len,
unsigned int mpdu_offset)
{
u64 ts = status->mactime;
struct rate_info ri;
u16 rate;

if (WARN_ON(!ieee80211_have_rx_timestamp(status)))//this is the
line 2621 reported in the warning
return 0;

memset(, 0, sizeof(ri));

/* Fill cfg80211 rate info */
if (status->flag & RX_FLAG_HT) {
ri.mcs = status->rate_idx;
ri.flags |= RATE_INFO_FLAGS_MCS;
if (status->flag & RX_FLAG_40MHZ)
ri.bw = RATE_INFO_BW_40;
else
ri.bw = RATE_INFO_BW_20;
if (status->flag & RX_FLAG_SHORT_GI)
ri.flags |= RATE_INFO_FLAGS_SHORT_GI;
} else if (status->flag & RX_FLAG_VHT) {
ri.flags |= RATE_INFO_FLAGS_VHT_MCS;
ri.mcs = status->rate_idx;
ri.nss = status->vht_nss;
if (status->flag & RX_FLAG_40MHZ)
ri.bw = RATE_INFO_BW_40;
else if (status->vht_flag & RX_VHT_FLAG_80MHZ)
ri.bw = RATE_INFO_BW_80;
else if (status->vht_flag & RX_VHT_FLAG_160MHZ)
ri.bw = RATE_INFO_BW_160;
else
ri.bw = RATE_INFO_BW_20;
if (status->flag & RX_FLAG_SHORT_GI)
ri.flags |= RATE_INFO_FLAGS_SHORT_GI;
} else {
struct ieee80211_supported_band *sband;
int shift = 0;
int bitrate;

if (status->flag & RX_FLAG_10MHZ) {
shift = 1;
ri.bw = RATE_INFO_BW_10;
} else if (status->flag & RX_FLAG_5MHZ) {
shift = 2;
ri.bw = RATE_INFO_BW_5;
} else {
ri.bw = RATE_INFO_BW_20;
}

sband = local->hw.wiphy->bands[status->band];
bitrate = sband->bitrates[status->rate_idx].bitrate;
ri.legacy = DIV_ROUND_UP(bitrate, (1 << shift));
}

rate = cfg80211_calculate_bitrate();
if (WARN_ONCE(!rate,
 "Invalid bitrate: flags=0x%x, idx=%d, vht_nss=%d\n",
 status->flag, status->rate_idx, status->vht_nss))
return 0;

/* rewind from end of MPDU */
if (status->flag & RX_FLAG_MACTIME_END)
ts -= mpdu_len * 8 * 10 / rate;

ts += mpdu_offset * 8 * 10 / rate;
/* [SESAME] I2CAT. dbg*/
//printk(KERN_DEBUG "calculate_rx_timestamp: ts = %lu;\t rate =
%lu;\tmpdu_offset = %lu;\tmpdu_len = %lu\n",
// (long unsigned int) ts, (long unsigned int) rate, (long unsigned
int) mpdu_offset, (long unsigned int) mpdu_len );

return ts;
}

Thank you so much!

All the best

Matteo


2016-09-29 16:47 GMT+02:00 Ben Greear :
> stock ath10k firmware does not report tx-rate so the kernel always sees
> 6Mbps.
>
> I don't know about the splat..maybe post the function
> that is causing that?
>
> /home/matteo/linux-imx6/backports4.4.2-i2CAT/net/mac80211/util.c:2621
>
> Thanks,
> Ben
>
>
> On 09/29/2016 06:39 AM, Matteo Grandi wrote:
>>
>> Hello all,
>>
>> I'm struggling with a problem related on ath10k drivers:
>> I'm using a Compex WLE600V5-27 (802.11ac) miniPCIe card for some HT
>> tests needed for my thesis.
>> I'm using ath10k drivers for this card, and backports-4.4.2, in
>> particular the firmware-5.bin_10.2.4.70.54 because it seem to be the
>> more recent.
>> I've connected in mesh mode the WLE600V5 card with an 802.11n card
>> (using ath9k drivers) but looking at the station dump, the tx bitrate
>> is stuck on 6.0 Mbit/s for the ath10k. The ath9k one works well and
>> watch -n1 iw  station dump
>> let me see changes of the tx rate and MCS on the ath9k during iperf
>> tests, but the ath10k stucks on 6.0 Mbit/s.
>>
>> Then something misterious (for me) happen while the channel
>> assignment: the syslog is spammed by this message:
>>
>> [17554.919459] [ cut here ]
>> [17554.919839] WARNING: CPU: 0 PID: 0 at
>> /home/matteo/linux-imx6/backports4.4.2-i2CAT/net/mac80211/util.c:2621
>> ieee80211_calculate_rx_timestamp+0x204/0x278 [mac80211]()
>> [17554.919855] Modules linked in: arc4 sky2 ath10k_pci(O)
>> ath10k_core(O) ath(O) mac80211(O) cfg80211(O) compat(O)
>> [17554.919926] CPU: 0 PID: 0 Comm: swapper/0 Tainted: GW  O
>> 3.14.48-g408ccb9 #4
>> [17554.919990] [<80015050>] (unwind_backtrace) from [<80011330>]
>> (show_stack+0x10/0x14)
>> [17554.920038] [<80011330>] (show_stack) from [<806537dc>]
>> (dump_stack+0x80/0x90)
>> [17554.920074] [<806537dc>] (dump_stack) from [<8002c578>]
>> (warn_slowpath_common+0x6c/0x88)
>> [17554.920103] [<8002c578>] (warn_slowpath_common) 

Re: [PATCH 1/3] mac80211: filter multicast data packets on AP / AP_VLAN

[Johannes Berg]

[snip]

I think this makes sense, but it's not clear to me why you add two
counters and keep the old one? It seems to me that it would be
sufficient to have a single counter per AP/VLAN interface?

The usage in __ieee80211_request_smps_ap() can just be removed since it
goes to iterate the stations next. That should be a separate, first,
patch in the series, but after that I don't see a need to keep
num_mcast_sta, or rather, I see no reason not to remove the VLAN
stations from the AP's num_mcast_sta, and add a new per-VLAN
num_mcast_sta.

> +/**
> + * @returns number of multicast stations connected
> + *  -1 if unsupported (no-AP, 4addr mode)
> + */
> +static inline int
> +ieee80211_vif_get_num_mcast_if(struct ieee80211_sub_if_data *sdata)

That's not a valid kernel-doc comment, but you've tagged it as one with
the /** - please fix by removing the /** and the @, and writing a real
sentence out of that, or by making it a real kernel-doc comment.

johannes

Re: ath10k mesh mode issue

[Matteo Grandi]

Hi Chun-Yeoh,
These https://wireless.wiki.kernel.org/en/users/drivers/ath10k/mesh
are the staeps that i followed to set-up the mesh.
Thanks!

Matteo

2016-09-29 17:10 GMT+02:00 Yeoh Chun-Yeow :
> Try refer the following for latest mesh setup using ath10k:
> https://wireless.wiki.kernel.org/en/users/drivers/ath10k/mesh
>
> ---
> Chun-Yeow