Re: [PATCH v2] mt76: refactor cc_lock locking scheme
On Tue, Oct 15, 2019 at 05:16:43PM +0200, Lorenzo Bianconi wrote:
> Read busy counters not holding cc_lock spinlock since usb read can't be
> performed in interrupt context. Move cc_active and cc_rx counters out of
> cc_lock since they are not modified in interrupt context.
> Grab cc_lock updating cur_cc_bss_rx in mt76_airtime_report and do not
> hold rx_lock in mt76_update_survey.
> Fixes: 168aea24f4bb ("mt76: mt76x02u: enable survey support")
I think problem was introduced currently in mt76 driver version
that is not yet in mainline tree, so this is not right commit.
On Linus' tree we still read registers outside of cc_lock section.
void mt76x02_update_channel(struct mt76_dev *mdev)
{
...
busy = mt76_rr(dev, MT_CH_BUSY);
active = busy + mt76_rr(dev, MT_CH_IDLE);
spin_lock_bh(&dev->mt76.cc_lock);
state->cc_busy += busy;
state->cc_active += active;
spin_unlock_bh(&dev->mt76.cc_lock);
}
> if (dev->drv->drv_flags & MT_DRV_SW_RX_AIRTIME) {
> - spin_lock_bh(&dev->rx_lock);
> - spin_lock(&dev->cc_lock);
> + spin_lock_bh(&dev->cc_lock);
> state->cc_bss_rx += dev->cur_cc_bss_rx;
> dev->cur_cc_bss_rx = 0;
> - spin_unlock(&dev->cc_lock);
> - spin_unlock_bh(&dev->rx_lock);
> + spin_unlock_bh(&dev->cc_lock);
Why dev->rx_lock was needed before and is not needed now ?
Stanislaw
Re: [PATCH] mac80211: simplify TX aggregation start
On Tue, Oct 01, 2019 at 10:06:29PM +0200, Johannes Berg wrote: > index f1cdcd61c54a..b74e758cce09 100644 > --- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c > +++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c > @@ -10489,7 +10489,7 @@ int rt2800_ampdu_action(struct ieee80211_hw *hw, > struct ieee80211_vif *vif, >*/ > break; > case IEEE80211_AMPDU_TX_START: > - ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); > + ret = IEEE80211_AMPDU_TX_START_IMMEDIATE; > break; > case IEEE80211_AMPDU_TX_STOP_CONT: > case IEEE80211_AMPDU_TX_STOP_FLUSH: > +#define IEEE80211_AMPDU_TX_START_IMMEDIATE 1 > + > /** On rt2x00 we already return 1 for case we do not have free HW WCID number for a remote station. if (sta_priv->wcid > WCID_END) return 1; So we should change that to some other error code i.e. -ENOSPC. Stanislaw
Re: rt2x00 regression
On Thu, Sep 26, 2019 at 06:32:23PM +0200, Anton Olsson wrote: > Hello I have a USB based ID 148f:3070 Ralink Technology, Corp. RT2870/RT3070 > Wireless Adapter, that stops working with recent kernels. It works on kernel > 5.1.15 and does not work with 5.2.7 or 5.3.1 (I have not tested other > versions). I use it in AP mode. > > I found this similar bug report > https://marc.info/?l=linux-wireless&m=156630037103575&w=2 but that did not > have related error messages so I assume this is different? > > Logs of working kernel 5.1.15-arch1-1-ARCH. > [ 78.680555] ieee80211 phy0: rt2x00_set_rt: Info - RT chipset 3070, rev > 0201 detected > [ 78.690992] ieee80211 phy0: rt2x00_set_rf: Info - RF chipset 0005 detected > [ 78.799625] ieee80211 phy0: Selected rate control algorithm 'minstrel_ht' > sep 26 17:13:03 kernel: usbcore: registered new interface driver rt2800usb > sep 26 17:13:03 systemd[1]: Found device RT2870/RT3070 Wireless Adapter. > [ 113.812454] ieee80211 phy0: rt2x00lib_request_firmware: Info - Loading > firmware file 'rt2870.bin' > [ 113.905279] ieee80211 phy0: rt2x00lib_request_firmware: Info - Firmware > detected - version: 0.36 > [ 114.028703] ieee80211 phy0: rt2x00usb_vendor_request: Error - Vendor > Request 0x06 failed for offset 0x0404 with error -71 > > The last error there does not seem to affect the operation of the device. > > Logs of not working with kernel 5.3.1, 5.2.7 has similar output. > sep 26 17:06:12 kernel: ieee80211 phy0: rt2x00_set_rt: Info - RT chipset > 3070, rev 0201 detected > sep 26 17:06:12 kernel: ieee80211 phy0: rt2x00_set_rf: Info - RF chipset 0005 > detected > sep 26 17:06:12 kernel: ieee80211 phy0: Selected rate control algorithm > 'minstrel_ht' > sep 26 17:06:12 kernel: usbcore: registered new interface driver rt2800usb > sep 26 17:06:12 systemd[1]: Found device RT2870/RT3070 Wireless Adapter. > sep 26 17:06:21 ieee80211 phy0: rt2x00lib_request_firmware: Info - Loading > firmware file 'rt2870.bin' > sep 26 17:06:21 ieee80211 phy0: rt2x00lib_request_firmware: Info - Firmware > detected - version: 0.36 > sep 26 17:06:21 ieee80211 phy0: rt2x00usb_vendor_request: Error - Vendor > Request 0x06 failed for offset 0x0404 with> > sep 26 17:06:22 ieee80211 phy0: rt2800_wait_csr_ready: Error - Unstable > hardware > sep 26 17:06:22 ieee80211 phy0: rt2800usb_set_device_state: Error - Device > failed to enter state 4 (-5) > > Unable to bring up the network interface here. This most likely is the problem introduced by commit: commit e383c70474db32b9d4a3de6dfbd08784d19e6751 Author: Stanislaw Gruszka Date: Tue Mar 12 10:51:42 2019 +0100 rt2x00: check number of EPROTO errors Plase check below patch that increase number of EPROTO checks before marking device removed. If it does not help, plese check if reverting above commits helps. diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c b/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c index bc2dfef0de22..215c3f092306 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00usb.c @@ -30,7 +30,7 @@ static bool rt2x00usb_check_usb_error(struct rt2x00_dev *rt2x00dev, int status) else rt2x00dev->num_proto_errs = 0; - if (rt2x00dev->num_proto_errs > 3) + if (rt2x00dev->num_proto_errs > 8) return true; return false;
[PATCH] rt2x00: initialize last_reset
Initialize last_reset variable to INITIAL_JIFFIES, otherwise it is not
possible to test H/W reset for first 5 minutes of system run.
Fixes: e403fa31ed71 ("rt2x00: add restart hw")
Reported-and-tested-by: Jonathan Liu
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/ralink/rt2x00/rt2x00debug.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
index 4d4e3888ef20..f2395309ec00 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
@@ -555,7 +555,7 @@ static ssize_t rt2x00debug_write_restart_hw(struct file
*file,
{
struct rt2x00debug_intf *intf = file->private_data;
struct rt2x00_dev *rt2x00dev = intf->rt2x00dev;
- static unsigned long last_reset;
+ static unsigned long last_reset = INITIAL_JIFFIES;
if (!rt2x00_has_cap_restart_hw(rt2x00dev))
return -EOPNOTSUPP;
--
2.20.1
[PATCH 5.3] Revert "rt2800: enable TX_PIN_CFG_LNA_PE_ bits per band"
This reverts commit 9ad3b55654455258a9463384edb40077439d879f.
As reported by Sergey:
"I got some problem after upgrade kernel to 5.2 version (debian testing
linux-image-5.2.0-2-amd64). 5Ghz client stopped to see AP.
Some tests with 1metre distance between client-AP: 2.4Ghz -22dBm, for
5Ghz - 53dBm !, for longer distance (8m + walls) 2.4 - 61dBm, 5Ghz not
visible."
It was identified that rx signal level degradation was caused by
9ad3b5565445 ("rt2800: enable TX_PIN_CFG_LNA_PE_ bits per band").
So revert this commit.
Cc: # v5.1+
Reported-and-tested-by: Sergey Maranchuk
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 18 ++
1 file changed, 6 insertions(+), 12 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index c9b957ac5733..b2bfa83819b0 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -4237,24 +4237,18 @@ static void rt2800_config_channel(struct rt2x00_dev
*rt2x00dev,
switch (rt2x00dev->default_ant.rx_chain_num) {
case 3:
/* Turn on tertiary LNAs */
- rt2x00_set_field32(&tx_pin, TX_PIN_CFG_LNA_PE_A2_EN,
- rf->channel > 14);
- rt2x00_set_field32(&tx_pin, TX_PIN_CFG_LNA_PE_G2_EN,
- rf->channel <= 14);
+ rt2x00_set_field32(&tx_pin, TX_PIN_CFG_LNA_PE_A2_EN, 1);
+ rt2x00_set_field32(&tx_pin, TX_PIN_CFG_LNA_PE_G2_EN, 1);
/* fall-through */
case 2:
/* Turn on secondary LNAs */
- rt2x00_set_field32(&tx_pin, TX_PIN_CFG_LNA_PE_A1_EN,
- rf->channel > 14);
- rt2x00_set_field32(&tx_pin, TX_PIN_CFG_LNA_PE_G1_EN,
- rf->channel <= 14);
+ rt2x00_set_field32(&tx_pin, TX_PIN_CFG_LNA_PE_A1_EN, 1);
+ rt2x00_set_field32(&tx_pin, TX_PIN_CFG_LNA_PE_G1_EN, 1);
/* fall-through */
case 1:
/* Turn on primary LNAs */
- rt2x00_set_field32(&tx_pin, TX_PIN_CFG_LNA_PE_A0_EN,
- rf->channel > 14);
- rt2x00_set_field32(&tx_pin, TX_PIN_CFG_LNA_PE_G0_EN,
- rf->channel <= 14);
+ rt2x00_set_field32(&tx_pin, TX_PIN_CFG_LNA_PE_A0_EN, 1);
+ rt2x00_set_field32(&tx_pin, TX_PIN_CFG_LNA_PE_G0_EN, 1);
break;
}
--
1.9.3
Re: rt2x00: 5Ghz signal power degradation with linux 5.2 kernel
On Thu, Aug 29, 2019 at 12:42:44PM +0300, Sergey Maranchuk wrote: > I got some problem after upgrade kernel to 5.2 version (debian testing > linux-image-5.2.0-2-amd64). 5Ghz client stopped to see AP. > Some tests with 1metre distance between client-AP: 2.4Ghz -22dBm, for > 5Ghz - 53dBm !, for longer distance (8m + walls) 2.4 - 61dBm, 5Ghz not > visible. > All works fine with kernel 4.19 and on windows 10, other devices also > see AP without any problems with same distance. After looking on the changlog most possible cause of it seems to be commit 9ad3b55654455258a9463384edb40077439d879f: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?id=9ad3b55654455258a9463384edb40077439d879f Please try to revert it, and see if that improve rx signal level. It that would not help, please try to narrow regression to two consecutive kernel versions - it is: 4.19 -> 5.0 or 5.0 -> 5.1 or 5.1 -> 5.2 ? Stanislaw
[PATCH 5.3] rt2x00: clear up IV's on key removal
After looking at code I realized that my previous fix
95844124385e ("rt2x00: clear IV's on start to fix AP mode regression")
was incomplete. We can still have wrong IV's after re-keyring.
To fix that, clear up IV's also on key removal.
Fixes: 710e6cc1595e ("rt2800: do not nullify initialization vector data")
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index ecbe78b8027b..28e2de04834e 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -1654,13 +1654,18 @@ static void rt2800_config_wcid_attr_cipher(struct
rt2x00_dev *rt2x00dev,
offset = MAC_IVEIV_ENTRY(key->hw_key_idx);
- rt2800_register_multiread(rt2x00dev, offset,
- &iveiv_entry, sizeof(iveiv_entry));
- if ((crypto->cipher == CIPHER_TKIP) ||
- (crypto->cipher == CIPHER_TKIP_NO_MIC) ||
- (crypto->cipher == CIPHER_AES))
- iveiv_entry.iv[3] |= 0x20;
- iveiv_entry.iv[3] |= key->keyidx << 6;
+ if (crypto->cmd == SET_KEY) {
+ rt2800_register_multiread(rt2x00dev, offset,
+ &iveiv_entry, sizeof(iveiv_entry));
+ if ((crypto->cipher == CIPHER_TKIP) ||
+ (crypto->cipher == CIPHER_TKIP_NO_MIC) ||
+ (crypto->cipher == CIPHER_AES))
+ iveiv_entry.iv[3] |= 0x20;
+ iveiv_entry.iv[3] |= key->keyidx << 6;
+ } else {
+ memset(&iveiv_entry, 0, sizeof(iveiv_entry));
+ }
+
rt2800_register_multiwrite(rt2x00dev, offset,
&iveiv_entry, sizeof(iveiv_entry));
}
--
1.9.3
Re: [PATCH 1/3] mt76: remove redundant mt76_txq_schedule_all
On Fri, Aug 23, 2019 at 11:27:41AM +0200, Felix Fietkau wrote: > On 2019-08-23 10:52, Stanislaw Gruszka wrote: > > Waking tx queues will cause that txq's will be scheduled. Calling > > mt76_txq_schedule_all() while queues are blocked is not necessary. > > We will not get any skb's from ieee80211_tx_dequeue() anyway, but > > patch changes that transmit of mtxq->retry_q skb's will be a bit > > deferred (on the moment after channel switch or other situation > > when we wake up queues). > > > > Signed-off-by: Stanislaw Gruszka > Waking tx queues will not always cause txqs to be scheduled - only if an > attempt to dequeue was blocked because queues were stopped at that time. > Because of that, I don't think this patch is correct. Ok, please drop it. Stanislaw
[PATCH 1/3] mt76: remove redundant mt76_txq_schedule_all
Waking tx queues will cause that txq's will be scheduled. Calling
mt76_txq_schedule_all() while queues are blocked is not necessary.
We will not get any skb's from ieee80211_tx_dequeue() anyway, but
patch changes that transmit of mtxq->retry_q skb's will be a bit
deferred (on the moment after channel switch or other situation
when we wake up queues).
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/mediatek/mt76/mt7603/mac.c | 1 -
drivers/net/wireless/mediatek/mt76/mt7603/main.c | 2 --
drivers/net/wireless/mediatek/mt76/mt7615/main.c | 1 -
drivers/net/wireless/mediatek/mt76/mt76x0/main.c | 2 --
drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c| 1 -
drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c | 2 --
drivers/net/wireless/mediatek/mt76/mt76x2/usb_main.c | 1 -
7 files changed, 10 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
b/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
index c328192307c4..eb33de264c8a 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/mac.c
@@ -1344,7 +1344,6 @@ static void mt7603_mac_watchdog_reset(struct mt7603_dev
*dev)
napi_schedule(&dev->mt76.napi[1]);
ieee80211_wake_queues(dev->mt76.hw);
- mt76_txq_schedule_all(&dev->mt76);
}
static u32 mt7603_dma_debug(struct mt7603_dev *dev, u8 index)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/main.c
b/drivers/net/wireless/mediatek/mt76/mt7603/main.c
index 25d5b1608bc9..3a1b18795e05 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/main.c
@@ -170,8 +170,6 @@ void mt7603_init_edcca(struct mt7603_dev *dev)
clear_bit(MT76_RESET, &dev->mt76.state);
- mt76_txq_schedule_all(&dev->mt76);
-
ieee80211_queue_delayed_work(mt76_hw(dev), &dev->mt76.mac_work,
msecs_to_jiffies(MT7603_WATCHDOG_TIME));
diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/main.c
b/drivers/net/wireless/mediatek/mt76/mt7615/main.c
index 87c748715b5d..41ca44ffb058 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/main.c
@@ -158,7 +158,6 @@ static int mt7615_set_channel(struct mt7615_dev *dev)
clear_bit(MT76_RESET, &dev->mt76.state);
mutex_unlock(&dev->mt76.mutex);
- mt76_txq_schedule_all(&dev->mt76);
ieee80211_queue_delayed_work(mt76_hw(dev), &dev->mt76.mac_work,
MT7615_WATCHDOG_TIME);
return ret;
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/main.c
b/drivers/net/wireless/mediatek/mt76/mt76x0/main.c
index 3bc665643e51..562249eb918c 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/main.c
@@ -33,8 +33,6 @@
}
mt76x02_pre_tbtt_enable(dev, true);
- mt76_txq_schedule_all(&dev->mt76);
-
return ret;
}
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
index dc773070481d..fdc0297c1d27 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
@@ -513,7 +513,6 @@ static void mt76x02_watchdog_reset(struct mt76x02_dev *dev)
ieee80211_restart_hw(dev->mt76.hw);
} else {
ieee80211_wake_queues(dev->mt76.hw);
- mt76_txq_schedule_all(&dev->mt76);
}
}
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c
b/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c
index 4971685aafe8..8275a211fd20 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c
@@ -68,8 +68,6 @@
tasklet_enable(&dev->dfs_pd.dfs_tasklet);
tasklet_enable(&dev->mt76.pre_tbtt_tasklet);
- mt76_txq_schedule_all(&dev->mt76);
-
return ret;
}
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/usb_main.c
b/drivers/net/wireless/mediatek/mt76/mt76x2/usb_main.c
index eb73cb856c81..d5bfffd6099d 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x2/usb_main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x2/usb_main.c
@@ -58,7 +58,6 @@ static void mt76x2u_stop(struct ieee80211_hw *hw)
mutex_unlock(&dev->mt76.mutex);
mt76x02_pre_tbtt_enable(dev, true);
- mt76_txq_schedule_all(&dev->mt76);
return err;
}
--
1.9.3
[PATCH 3/3] mt76: mt76x0: remove unneeded return value on set channel
We allways return 0 from mt76x0_phy_set_channel(), no need to pass
return value upward.
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/mediatek/mt76/mt76x0/main.c | 13 -
drivers/net/wireless/mediatek/mt76/mt76x0/mt76x0.h | 4 ++--
drivers/net/wireless/mediatek/mt76/mt76x0/phy.c| 8 +++-
3 files changed, 9 insertions(+), 16 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/main.c
b/drivers/net/wireless/mediatek/mt76/mt76x0/main.c
index 562249eb918c..8a3bb924bef4 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/main.c
@@ -8,18 +8,16 @@
#include
#include "mt76x0.h"
-static int
+static void
mt76x0_set_channel(struct mt76x02_dev *dev, struct cfg80211_chan_def *chandef)
{
- int ret;
-
cancel_delayed_work_sync(&dev->cal_work);
mt76x02_pre_tbtt_enable(dev, false);
if (mt76_is_mmio(dev))
tasklet_disable(&dev->dfs_pd.dfs_tasklet);
mt76_set_channel(&dev->mt76);
- ret = mt76x0_phy_set_channel(dev, chandef);
+ mt76x0_phy_set_channel(dev, chandef);
/* channel cycle counters read-and-clear */
mt76_rr(dev, MT_CH_IDLE);
@@ -32,20 +30,17 @@
tasklet_enable(&dev->dfs_pd.dfs_tasklet);
}
mt76x02_pre_tbtt_enable(dev, true);
-
- return ret;
}
int mt76x0_config(struct ieee80211_hw *hw, u32 changed)
{
struct mt76x02_dev *dev = hw->priv;
- int ret = 0;
mutex_lock(&dev->mt76.mutex);
if (changed & IEEE80211_CONF_CHANGE_CHANNEL) {
ieee80211_stop_queues(hw);
- ret = mt76x0_set_channel(dev, &hw->conf.chandef);
+ mt76x0_set_channel(dev, &hw->conf.chandef);
ieee80211_wake_queues(hw);
}
@@ -67,6 +62,6 @@ int mt76x0_config(struct ieee80211_hw *hw, u32 changed)
mutex_unlock(&dev->mt76.mutex);
- return ret;
+ return 0;
}
EXPORT_SYMBOL_GPL(mt76x0_config);
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/mt76x0.h
b/drivers/net/wireless/mediatek/mt76/mt76x0/mt76x0.h
index caa87f0c3cb8..26517e062bdb 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/mt76x0.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/mt76x0.h
@@ -54,8 +54,8 @@ static inline bool is_mt7630(struct mt76x02_dev *dev)
/* PHY */
void mt76x0_phy_init(struct mt76x02_dev *dev);
int mt76x0_phy_wait_bbp_ready(struct mt76x02_dev *dev);
-int mt76x0_phy_set_channel(struct mt76x02_dev *dev,
- struct cfg80211_chan_def *chandef);
+void mt76x0_phy_set_channel(struct mt76x02_dev *dev,
+ struct cfg80211_chan_def *chandef);
void mt76x0_phy_set_txpower(struct mt76x02_dev *dev);
void mt76x0_phy_calibrate(struct mt76x02_dev *dev, bool power_on);
#endif
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/phy.c
b/drivers/net/wireless/mediatek/mt76/mt76x0/phy.c
index 1d00aa5da95b..711a352dfd5c 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/phy.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/phy.c
@@ -909,8 +909,8 @@ void mt76x0_phy_calibrate(struct mt76x02_dev *dev, bool
power_on)
}
EXPORT_SYMBOL_GPL(mt76x0_phy_calibrate);
-int mt76x0_phy_set_channel(struct mt76x02_dev *dev,
- struct cfg80211_chan_def *chandef)
+void mt76x0_phy_set_channel(struct mt76x02_dev *dev,
+ struct cfg80211_chan_def *chandef)
{
u32 ext_cca_chan[4] = {
[0] = FIELD_PREP(MT_EXT_CCA_CFG_CCA0, 0) |
@@ -1004,7 +1004,7 @@ int mt76x0_phy_set_channel(struct mt76x02_dev *dev,
/* enable vco */
mt76x0_rf_set(dev, MT_RF(0, 4), BIT(7));
if (scan)
- return 0;
+ return;
mt76x02_init_agc_gain(dev);
mt76x0_phy_calibrate(dev, false);
@@ -1012,8 +1012,6 @@ int mt76x0_phy_set_channel(struct mt76x02_dev *dev,
ieee80211_queue_delayed_work(dev->mt76.hw, &dev->cal_work,
MT_CALIBRATE_INTERVAL);
-
- return 0;
}
static void mt76x0_phy_temp_sensor(struct mt76x02_dev *dev)
--
1.9.3
[PATCH 2/3] mt76: mt76x0: remove redundant chandef copy
We set dev->mt76.chandef in mt76_set_channel() already.
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/mediatek/mt76/mt76x0/phy.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/phy.c
b/drivers/net/wireless/mediatek/mt76/mt76x0/phy.c
index 31f988e86d92..1d00aa5da95b 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/phy.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/phy.c
@@ -944,7 +944,6 @@ int mt76x0_phy_set_channel(struct mt76x02_dev *dev,
freq1 = chandef->center_freq1;
channel = chandef->chan->hw_value;
rf_bw_band = (channel <= 14) ? RF_G_BAND : RF_A_BAND;
- dev->mt76.chandef = *chandef;
switch (chandef->width) {
case NL80211_CHAN_WIDTH_40:
--
1.9.3
[PATCH 0/3] mt76: channel switch related cleanups
Stanislaw Gruszka (3): mt76: remove redundant mt76_txq_schedule_all mt76: mt76x0: remove redundant chandef copy mt76: mt76x0: remove unneeded return value on set channel drivers/net/wireless/mediatek/mt76/mt7603/mac.c | 1 - drivers/net/wireless/mediatek/mt76/mt7603/main.c | 2 -- drivers/net/wireless/mediatek/mt76/mt7615/main.c | 1 - drivers/net/wireless/mediatek/mt76/mt76x0/main.c | 15 --- drivers/net/wireless/mediatek/mt76/mt76x0/mt76x0.h | 4 ++-- drivers/net/wireless/mediatek/mt76/mt76x0/phy.c | 9 +++-- drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c| 1 - drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c | 2 -- drivers/net/wireless/mediatek/mt76/mt76x2/usb_main.c | 1 - 9 files changed, 9 insertions(+), 27 deletions(-) -- 1.9.3
[PATCH] mt76: make mt76_rx_convert static
mt76_rx_convert() not need to be exported any longer.
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/mediatek/mt76/mac80211.c | 3 +--
drivers/net/wireless/mediatek/mt76/mt76.h | 2 --
2 files changed, 1 insertion(+), 4 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mac80211.c
b/drivers/net/wireless/mediatek/mt76/mac80211.c
index fa481d2f11bd..1a2c143b34d0 100644
--- a/drivers/net/wireless/mediatek/mt76/mac80211.c
+++ b/drivers/net/wireless/mediatek/mt76/mac80211.c
@@ -484,7 +484,7 @@ void mt76_wcid_key_setup(struct mt76_dev *dev, struct
mt76_wcid *wcid,
}
EXPORT_SYMBOL(mt76_wcid_key_setup);
-struct ieee80211_sta *mt76_rx_convert(struct sk_buff *skb)
+static struct ieee80211_sta *mt76_rx_convert(struct sk_buff *skb)
{
struct ieee80211_rx_status *status = IEEE80211_SKB_RXCB(skb);
struct mt76_rx_status mstat;
@@ -511,7 +511,6 @@ struct ieee80211_sta *mt76_rx_convert(struct sk_buff *skb)
return wcid_to_sta(mstat.wcid);
}
-EXPORT_SYMBOL(mt76_rx_convert);
static int
mt76_check_ccmp_pn(struct sk_buff *skb)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h
b/drivers/net/wireless/mediatek/mt76/mt76.h
index d67c6a26c87c..570c159515a0 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -749,8 +749,6 @@ int mt76_sta_state(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
void __mt76_sta_remove(struct mt76_dev *dev, struct ieee80211_vif *vif,
struct ieee80211_sta *sta);
-struct ieee80211_sta *mt76_rx_convert(struct sk_buff *skb);
-
int mt76_get_min_avg_rssi(struct mt76_dev *dev);
int mt76_get_txpower(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
--
1.9.3
[PATCH] rt2x00: do not set IEEE80211_TX_STAT_AMPDU_NO_BACK on tx status
According to documentation IEEE80211_TX_STAT_AMPDU_NO_BACK is suppose
to be used when we do not recive BA (BlockAck). However on rt2x00 we
use it when remote station fail to decode one or more subframes within
AMPDU (some bits are not set in BlockAck bitmap). Setting the flag result
in sent of BAR (BlockAck Request) frame and this might result of abuse
of BA session, since remote station can sent BA with incorrect
sequence numbers after receiving BAR. This problem is visible especially
when connecting two rt2800 devices.
Previously I observed some performance benefits when using the flag
when connecting with iwlwifi devices. But currently possibly due
to reacent changes in rt2x00 removing the flag has no effect on
those test cases.
So remove the IEEE80211_TX_STAT_AMPDU_NO_BACK.
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/ralink/rt2x00/rt2x00dev.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
index 9d158237ac67..c3eab767bc21 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
@@ -371,9 +371,6 @@ static void rt2x00lib_fill_tx_status(struct rt2x00_dev
*rt2x00dev,
IEEE80211_TX_CTL_AMPDU;
tx_info->status.ampdu_len = 1;
tx_info->status.ampdu_ack_len = success ? 1 : 0;
-
- if (!success)
- tx_info->flags |= IEEE80211_TX_STAT_AMPDU_NO_BACK;
}
if (rate_flags & IEEE80211_TX_RC_USE_RTS_CTS) {
--
1.9.3
Re: [RFC/RFT v2] rt2x00: do not set IEEE80211_TX_STAT_AMPDU_NO_BACK on tx status
On Fri, Jul 12, 2019 at 12:32:28PM +0200, Stanislaw Gruszka wrote: > According to documentation IEEE80211_TX_STAT_AMPDU_NO_BACK is suppose > to be used when we do not receive BA (BlockAck). However on rt2x00 we > use it when remote station fail to decode one or more subframes within > AMPDU (some bits are not set in BlockAck bitmap). Setting the flag result > in sent of BAR (BlockAck Request) frame and this might result of abuse > of BA session, since remote station can sent BA with incorrect > sequence numbers after receiving BAR. This problem is visible especially > when connecting two rt2800 devices. > > Previously I observed some performance benefits when using the flag > when connecting with iwlwifi devices. But currently possibly due > to recent changes in rt2x00 removing the flag has no effect on > those test cases. > > So remove the IEEE80211_TX_STAT_AMPDU_NO_BACK. > > Additionally partially mimic mt76 behaviour: send BAR when > starting/stopping BA session to workaround problems with some buggy > clients. Do not sent BAR on PS wakeup since we lack all PS handling > code what mt76 has. Currently Felix posted patch that removed sending BAR on BA session stop. And I do not see necessity for sending BAR on start, so I will precede with first version of this patch, that just remove NO_BACK flag. Stanislaw
Re: [PATCH] mt76: remove empty flag in mt76_txq_schedule_list
On Thu, Aug 22, 2019 at 01:50:52PM +0200, Lorenzo Bianconi wrote:
> > On Thu, Aug 22, 2019 at 11:49:10AM +0200, Lorenzo Bianconi wrote:
> > > Remove empty flag in mt76_txq_schedule_list and mt76_txq_send_burst
> > > since we just need retry_q length to notify mac80211 to reschedule the
> > > current tx queue
> > >
> > > Signed-off-by: Lorenzo Bianconi
> > > ---
> > > drivers/net/wireless/mediatek/mt76/tx.c | 23 +++
> > > 1 file changed, 7 insertions(+), 16 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/mediatek/mt76/tx.c
> > > b/drivers/net/wireless/mediatek/mt76/tx.c
> > > index d7982aa83c11..51d69329ed06 100644
> > > --- a/drivers/net/wireless/mediatek/mt76/tx.c
> > > +++ b/drivers/net/wireless/mediatek/mt76/tx.c
> > > @@ -378,7 +378,7 @@ EXPORT_SYMBOL_GPL(mt76_release_buffered_frames);
> > >
> > > static int
> > > mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_sw_queue *sq,
> > > - struct mt76_txq *mtxq, bool *empty)
> > > + struct mt76_txq *mtxq)
> > > {
> > > struct ieee80211_txq *txq = mtxq_to_txq(mtxq);
> > > enum mt76_txq_id qid = mt76_txq_get_qid(txq);
> > > @@ -392,16 +392,12 @@ mt76_txq_send_burst(struct mt76_dev *dev, struct
> > > mt76_sw_queue *sq,
> > > bool probe;
> > > int idx;
> > >
> > > - if (test_bit(MT_WCID_FLAG_PS, &wcid->flags)) {
> > > - *empty = true;
> > > + if (test_bit(MT_WCID_FLAG_PS, &wcid->flags))
> > > return 0;
> >
> > This changes behaviour for station in PS state. If retry_q is not
> > empty, now we will be rescheduling tx queue for STA in PS mode.
> > Not sure if this is problem or not, though.
>
> good point..looking at the code it seems not a issue since we will not
> actually
> tx frames for PS stations. What do you think?
I do not see how changing this could possibly break things, but it
was explicitly added by below commit, with changelog sugesting it is
needed:
commit d225581df3147060bc99e931b11f7cf2dcb1b2ca
Author: Felix Fietkau
Date: Mon Jan 21 17:33:38 2019 +0100
mt76: avoid scheduling tx queues for powersave stations
In case a tx queue wake call arrives after a client has transitioned to
powersave, make sure that the queue is not kept active until the client
has left powersave mode
Stanislaw
Re: [PATCH] mt76: remove empty flag in mt76_txq_schedule_list
On Thu, Aug 22, 2019 at 11:49:10AM +0200, Lorenzo Bianconi wrote:
> Remove empty flag in mt76_txq_schedule_list and mt76_txq_send_burst
> since we just need retry_q length to notify mac80211 to reschedule the
> current tx queue
>
> Signed-off-by: Lorenzo Bianconi
> ---
> drivers/net/wireless/mediatek/mt76/tx.c | 23 +++
> 1 file changed, 7 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/tx.c
> b/drivers/net/wireless/mediatek/mt76/tx.c
> index d7982aa83c11..51d69329ed06 100644
> --- a/drivers/net/wireless/mediatek/mt76/tx.c
> +++ b/drivers/net/wireless/mediatek/mt76/tx.c
> @@ -378,7 +378,7 @@ EXPORT_SYMBOL_GPL(mt76_release_buffered_frames);
>
> static int
> mt76_txq_send_burst(struct mt76_dev *dev, struct mt76_sw_queue *sq,
> - struct mt76_txq *mtxq, bool *empty)
> + struct mt76_txq *mtxq)
> {
> struct ieee80211_txq *txq = mtxq_to_txq(mtxq);
> enum mt76_txq_id qid = mt76_txq_get_qid(txq);
> @@ -392,16 +392,12 @@ mt76_txq_send_burst(struct mt76_dev *dev, struct
> mt76_sw_queue *sq,
> bool probe;
> int idx;
>
> - if (test_bit(MT_WCID_FLAG_PS, &wcid->flags)) {
> - *empty = true;
> + if (test_bit(MT_WCID_FLAG_PS, &wcid->flags))
> return 0;
This changes behaviour for station in PS state. If retry_q is not
empty, now we will be rescheduling tx queue for STA in PS mode.
Not sure if this is problem or not, though.
Stanislaw
Re: [PATCH 5.3] mt76: mt76x0e: don't use hw encryption for MT7630E
On Wed, Aug 21, 2019 at 12:40:14PM +0200, Felix Fietkau wrote: > On 2019-08-21 11:03, Felix Fietkau wrote: > > On 2019-08-21 10:47, Stanislaw Gruszka wrote: > >> On Tue, Aug 20, 2019 at 01:24:39PM +0200, Stanislaw Gruszka wrote: > >>> > Can you test if disabling hw encryption only for shared or only for > >>> > pairwise keys makes any difference? > >>> > >>> Disabling only pairwise keys helps. Disabling only shared keys does > >>> not help. > >>> > >>> Not sure if this will be helpful information or make things more > >>> confusing, but seems the difference between mt76_txq_schedule() > >>> and tasklet_schedule() in mt76_wake_tx_queue() is that on > >>> mt76_txq_schedule() some tx packets are serialized by dev->rx_lock > >>> (because some ARP and TCP packets are sent via network stack as response > >>> of incoming packet within ieee80211_rx_napi() call). Removing > >>> spin_lock(&dev->rx_lock) in mt76_rx_complete() make the problem > >>> reproducible again with mt76_txq_schedule() & HW encryption. > >> > >> So, I think this is FW/HW issue related with encryption and ordering > >> and we should apply patch originally posted in this thread that > >> disable HW encryption for MT7630E. > >> > >> I do not think we should disable HW encryption only for pairwise keys, > >> because FW/HW can have the same bug for shared keys, but is not > >> triggered in my test, as we do not sent lot of group frames. > > I'm still not convinced that this is just the hardware implementation of > > hw crypto being faulty. I think it's more likely that there's a bug in > > the tx path somewhere, which causes hangs on MT7630E but remains hidden > > (or at least recoverable) on other devices. > > I'm currently reviewing key handling in the mac80211 fast-xmit codepath > > and get the feeling that something might be racy there. > > I will let you know when I make some progress with that review. > > If we can't find the bug soon, then I'm fine with merging this patch. > > Right now, I would like to see first if we can fix it properly. > Another question: Does a watchdog restart happen before tx fails? No, we do not run wdt_work for mt76x0e. Stanislaw
Re: [PATCH 5.3] mt76: mt76x0e: don't use hw encryption for MT7630E
On Tue, Aug 20, 2019 at 01:24:39PM +0200, Stanislaw Gruszka wrote: > > Can you test if disabling hw encryption only for shared or only for > > pairwise keys makes any difference? > > Disabling only pairwise keys helps. Disabling only shared keys does > not help. > > Not sure if this will be helpful information or make things more > confusing, but seems the difference between mt76_txq_schedule() > and tasklet_schedule() in mt76_wake_tx_queue() is that on > mt76_txq_schedule() some tx packets are serialized by dev->rx_lock > (because some ARP and TCP packets are sent via network stack as response > of incoming packet within ieee80211_rx_napi() call). Removing > spin_lock(&dev->rx_lock) in mt76_rx_complete() make the problem > reproducible again with mt76_txq_schedule() & HW encryption. So, I think this is FW/HW issue related with encryption and ordering and we should apply patch originally posted in this thread that disable HW encryption for MT7630E. I do not think we should disable HW encryption only for pairwise keys, because FW/HW can have the same bug for shared keys, but is not triggered in my test, as we do not sent lot of group frames. Stanislaw
Re: [PATCH 1/1] rt2x00: Queue flush fix
On Wed, Aug 21, 2019 at 03:43:05AM +0530, Balakrishna Bandi wrote:
> Added rt2x00 queue flush fix and beacon frames checks.
Please post separate patch for each issue and provide
more descriptive information about the changes, especially
what problems are intended to solve.
> diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c
> b/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c
> index 110bb39..9964371 100644
> --- a/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c
> +++ b/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c
> @@ -566,7 +566,7 @@ void rt2800mmio_queue_init(struct data_queue *queue)
>
> switch (queue->qid) {
> case QID_RX:
> - queue->limit = 128;
> + queue->limit = 512;
How this is related with flush or beaconing ?
At this point of rt2x00 driver development I'm pretty reluctant to
increase queue size. But maybe we can do this for some particular
chip if things were tested on this chip and improve or fix something.
> if (!test_bit(DEVICE_STATE_PRESENT, &rt2x00dev->flags))
> goto exit_free_skb;
> + /* Dirty hack for queue overrun protection,
> + * if AC_VO/AC_VI/AC_BE is full, use next queue.
> + * if AC_BK is full use previous queue.
> + */
No dirty hacks please.
> void rt2x00queue_flush_queue(struct data_queue *queue, bool drop)
> {
> + unsigned int i;
> + bool started;
> bool tx_queue =
> (queue->qid == QID_AC_VO) ||
> (queue->qid == QID_AC_VI) ||
> (queue->qid == QID_AC_BE) ||
> (queue->qid == QID_AC_BK);
> + mutex_lock(&queue->status_lock);
> + /* If the queue has been started, we must stop it temporarily
> + * to prevent any new frames to be queued on the device. If
> + * we are not dropping the pending frames, the queue must
> + * only be stopped in the software and not the hardware,
> + * otherwise the queue will never become empty on its own.
> + */
Since linux 5.2 there is rework done on related area. So maybe flush
issue you are trying to fix by this patch is already fixed. If not let
me know and provide description what is the problem.
> Disclaimer:- The information contained in this electronic message and any
> attachments to this message are intended for the exclusive use of the
> addressee(s) and may contain proprietary, confidential or privileged
> information. If you are not the intended recipient, you should not
> disseminate, distribute or copy this e-mail. Please notify the sender
> immediately and destroy all copies of this message and any attachments. The
> views expressed in this E-mail message (including the enclosure/(s) or
> attachment/(s) if any) are those of the individual sender, except where the
> sender expressly, and with authority, states them to be the views of
> GlobalEdge. Before opening any mail and attachments please check them for
> viruses .GlobalEdge does not accept any liability for virus infected mails.
>
You should not sent this disclaimer to open mailing list.
Fix this or use different email service to post patches.
Stanislaw
Re: rt2800: Commit 710e6cc1595e breaks ID 148f:3070 RT2870/RT3070 USB wifi
On Tue, Aug 20, 2019 at 01:20:50PM +0200, Fredrik Noring wrote:
> Hi Stanislaw,
>
> Commit 710e6cc1595e ("rt2800: do not nullify initialization vector data")
> breaks USB device 006: ID 148f:3070 Ralink Technology, Corp. RT2870/RT3070
> Wireless Adapter. No particular error messages are produced, but its
> interface becomes unreachable. Reverting this commit in 5.3.0-rc5 solves
> the problem.
Hi, I've recently sent the patch that should fix the problem:
https://lore.kernel.org/linux-wireless/1566213607-6723-1-git-send-email-sgrus...@redhat.com/T/#u
Stanislaw
Re: [PATCH 5.3] mt76: mt76x0e: don't use hw encryption for MT7630E
On Tue, Aug 20, 2019 at 12:31:56PM +0200, Felix Fietkau wrote:
> >> >> void mt76x0e_wake_tx_queue(struct ieee80211_hw *hw, struct
> >> >> ieee80211_txq *txq)
> >> >> {
> >> >> if (is_mt7630(dev)) {
> >> >> mt76_txq_schedule(dev, txq->ac);
> >> >> } else {
> >> >> tasklet_schedule(&dev->tx_tasklet);
> >> >> }
> >> >> }
> >> >
> >> > Not sure about reduction of lock contention for which the tx_tasklet
> >> > was introduced here, but looks ok for me as fix.
> >> I think if we work around the bug like this, it can easily come back to
> >> bite us again later.
> >
> > I'm not into workarounds any kind, but this is really strange issue,
> > maybe FW bug that triggers just by slightly different driver behaviour.
> >
> >> I don't see any logical explanation as to how this
> >> makes a difference with hardware encryption.
> >> Also, I think it would be helpful to figure out what key operation (if
> >> any) triggers this, adding or removing keys.
> >
> > Seems not to be related with set_key operation at all. We set 2 HW
> > keys at the beginning and hang happen after some tx/rx traffic
> > without any re-keyring.
> >
> > I'm not sure why disabling HW encryption helps. Maybe it is due to
> > ordering or timing. With SW encryption we spend more time in mac80211
> > before pass skb's to the driver. Or maybe we just mix some HW keys
> > and SW (group) keys in way that FW does not like.
> >
> >> Maybe it could also help if we change the order in which the WCID table
> >> entries are updated, i.e. changing MT_WCID_ATTR first when removing keys.
> >>
> >> Maybe temporarily clearing MT_MAC_SYS_CTRL_ENABLE_TX before the key
> >> update and setting it again afterwards could also help.
> >
> > I tested below patch and it did not help.
> Can you test if disabling hw encryption only for shared or only for
> pairwise keys makes any difference?
Disabling only pairwise keys helps. Disabling only shared keys does
not help.
Not sure if this will be helpful information or make things more
confusing, but seems the difference between mt76_txq_schedule()
and tasklet_schedule() in mt76_wake_tx_queue() is that on
mt76_txq_schedule() some tx packets are serialized by dev->rx_lock
(because some ARP and TCP packets are sent via network stack as response
of incoming packet within ieee80211_rx_napi() call). Removing
spin_lock(&dev->rx_lock) in mt76_rx_complete() make the problem
reproducible again with mt76_txq_schedule() & HW encryption.
Stanislaw
[PATCH 5.3] rt2x00: clear IV's on start to fix AP mode regression
To do not brake HW restart we should keep initialization vectors data.
I assumed that on start the data is already initialized to zeros, but
that not true on some scenarios and we should clear it. So add
additional flag to check if we are under HW restart and clear IV's
data if we are not.
Patch fixes AP mode regression.
Reported-and-tested-by: Emil Karlson
Fixes: 710e6cc1595e ("rt2800: do not nullify initialization vector data")
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 9 +
drivers/net/wireless/ralink/rt2x00/rt2x00.h| 1 +
drivers/net/wireless/ralink/rt2x00/rt2x00dev.c | 13 -
3 files changed, 18 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index 4a996550288e..cbec2131e943 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -6095,6 +6095,15 @@ static int rt2800_init_registers(struct rt2x00_dev
*rt2x00dev)
}
/*
+* Clear encryption initialization vectors on start, but keep them
+* for watchdog reset. Otherwise we will have wrong IVs and not be
+* able to keep connections after reset.
+*/
+ if (!test_bit(DEVICE_STATE_RESET, &rt2x00dev->flags))
+ for (i = 0; i < 256; i++)
+ rt2800_register_write(rt2x00dev, MAC_IVEIV_ENTRY(i), 0);
+
+ /*
* Clear all beacons
*/
for (i = 0; i < 8; i++)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00.h
b/drivers/net/wireless/ralink/rt2x00/rt2x00.h
index d35ef06c5c7a..1dd54a0d083d 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00.h
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00.h
@@ -659,6 +659,7 @@ enum rt2x00_state_flags {
DEVICE_STATE_ENABLED_RADIO,
DEVICE_STATE_SCANNING,
DEVICE_STATE_FLUSHING,
+ DEVICE_STATE_RESET,
/*
* Driver configuration
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
index ad063c920323..c3eab767bc21 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
@@ -1253,13 +1253,14 @@ static int rt2x00lib_initialize(struct rt2x00_dev
*rt2x00dev)
int rt2x00lib_start(struct rt2x00_dev *rt2x00dev)
{
- int retval;
+ int retval = 0;
if (test_bit(DEVICE_STATE_STARTED, &rt2x00dev->flags)) {
/*
* This is special case for ieee80211_restart_hw(), otherwise
* mac80211 never call start() two times in row without stop();
*/
+ set_bit(DEVICE_STATE_RESET, &rt2x00dev->flags);
rt2x00dev->ops->lib->pre_reset_hw(rt2x00dev);
rt2x00lib_stop(rt2x00dev);
}
@@ -1270,14 +1271,14 @@ int rt2x00lib_start(struct rt2x00_dev *rt2x00dev)
*/
retval = rt2x00lib_load_firmware(rt2x00dev);
if (retval)
- return retval;
+ goto out;
/*
* Initialize the device.
*/
retval = rt2x00lib_initialize(rt2x00dev);
if (retval)
- return retval;
+ goto out;
rt2x00dev->intf_ap_count = 0;
rt2x00dev->intf_sta_count = 0;
@@ -1286,11 +1287,13 @@ int rt2x00lib_start(struct rt2x00_dev *rt2x00dev)
/* Enable the radio */
retval = rt2x00lib_enable_radio(rt2x00dev);
if (retval)
- return retval;
+ goto out;
set_bit(DEVICE_STATE_STARTED, &rt2x00dev->flags);
- return 0;
+out:
+ clear_bit(DEVICE_STATE_RESET, &rt2x00dev->flags);
+ return retval;
}
void rt2x00lib_stop(struct rt2x00_dev *rt2x00dev)
--
1.9.3
Re: [PATCH 5.3] mt76: mt76x0e: don't use hw encryption for MT7630E
On Thu, Aug 15, 2019 at 12:20:54PM +0200, Felix Fietkau wrote:
> On 2019-08-15 12:09, Stanislaw Gruszka wrote:
> >> Hi Stanislaw,
> >>
> >> Can you please try if disabling/enabling the tx tasklet during hw key
> >> configuration fixes the issue?
> >> Doing something like:
> >>
> >> tasklet_disable(tx_tasklet)
> >> mt76x02_set_key()
> >> tasklet_enable(tx_tasklet)
> >
> > It does not help with the problem.
> >
> >> Moreover, have you double checked if there is any performance impact
> >> of not using hw encryption?
> >
> > I didn't observe any, but realized on this machine I have
> > aesni_intel encryption accelerator. After rebuild kernel without
> > CONFIG_CRYPTO_AES_NI_INTEL, 'perf top' showed extra 20% of cpu usage
> > in aes_encrypt() when sending data with HW encryption disabled.
> >
> >> If so, I guess it is better to just redefine mt76_wake_tx_queue for
> >> mt76x0e and run mt76_txq_schedule for 7630e:
> >>
> >> void mt76x0e_wake_tx_queue(struct ieee80211_hw *hw, struct ieee80211_txq
> >> *txq)
> >> {
> >> if (is_mt7630(dev)) {
> >> mt76_txq_schedule(dev, txq->ac);
> >> } else {
> >> tasklet_schedule(&dev->tx_tasklet);
> >> }
> >> }
> >
> > Not sure about reduction of lock contention for which the tx_tasklet
> > was introduced here, but looks ok for me as fix.
> I think if we work around the bug like this, it can easily come back to
> bite us again later.
I'm not into workarounds any kind, but this is really strange issue,
maybe FW bug that triggers just by slightly different driver behaviour.
> I don't see any logical explanation as to how this
> makes a difference with hardware encryption.
> Also, I think it would be helpful to figure out what key operation (if
> any) triggers this, adding or removing keys.
Seems not to be related with set_key operation at all. We set 2 HW
keys at the beginning and hang happen after some tx/rx traffic
without any re-keyring.
I'm not sure why disabling HW encryption helps. Maybe it is due to
ordering or timing. With SW encryption we spend more time in mac80211
before pass skb's to the driver. Or maybe we just mix some HW keys
and SW (group) keys in way that FW does not like.
> Maybe it could also help if we change the order in which the WCID table
> entries are updated, i.e. changing MT_WCID_ATTR first when removing keys.
>
> Maybe temporarily clearing MT_MAC_SYS_CTRL_ENABLE_TX before the key
> update and setting it again afterwards could also help.
I tested below patch and it did not help.
Stanislaw
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_mac.c
b/drivers/net/wireless/mediatek/mt76/mt76x02_mac.c
index 82bafb5ac326..846652f2b3f1 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_mac.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_mac.c
@@ -105,11 +105,14 @@ int mt76x02_mac_wcid_set_key(struct mt76x02_dev *dev, u8
idx,
if (cipher == MT_CIPHER_NONE && key)
return -EOPNOTSUPP;
+ if (!key)
+ mt76_rmw_field(dev, MT_WCID_ATTR(idx), MT_WCID_ATTR_PKEY_MODE,
MT_CIPHER_NONE);
+
mt76_wr_copy(dev, MT_WCID_KEY(idx), key_data, sizeof(key_data));
- mt76_rmw_field(dev, MT_WCID_ATTR(idx), MT_WCID_ATTR_PKEY_MODE, cipher);
memset(iv_data, 0, sizeof(iv_data));
if (key) {
+ mt76_rmw_field(dev, MT_WCID_ATTR(idx), MT_WCID_ATTR_PKEY_MODE,
cipher);
mt76_rmw_field(dev, MT_WCID_ATTR(idx), MT_WCID_ATTR_PAIRWISE,
!!(key->flags & IEEE80211_KEY_FLAG_PAIRWISE));
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
index fa45ed280ab1..6f624e3329f9 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
@@ -391,7 +391,7 @@ int mt76x02_ampdu_action(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
}
EXPORT_SYMBOL_GPL(mt76x02_ampdu_action);
-int mt76x02_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
+int __mt76x02_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
struct ieee80211_vif *vif, struct ieee80211_sta *sta,
struct ieee80211_key_conf *key)
{
@@ -466,6 +466,35 @@ int mt76x02_set_key(struct ieee80211_hw *hw, enum
set_key_cmd cmd,
return mt76x02_mac_wcid_set_key(dev, msta->wcid.idx, key);
}
+
+int mt76x02_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
+ struct ieee80211_vif *vif, struct ieee80211_sta *sta,
+ struct ieee80211_key_conf *key)
+
Re: Ap mode regression in linux-5.3-rc1 in rt2800usb
On Fri, Aug 16, 2019 at 11:00:12AM +0300, Emil Karlson wrote:
> Greetings
>
> On Wed, 14 Aug 2019 10:50:19 +0200
> Stanislaw Gruszka wrote:
>
> > (cc linux-wireless mailing list)
> >
> > On Tue, Aug 13, 2019 at 09:50:00PM +0300, Emil Karlson wrote:
> > > Greetings
> > >
> > > After upgrading my ap running rt2800usb to linux-5.3-rc1 I noticed
> > > an unusual problem of not being able to connect to my ap with my
> > > android devices (nexus7/flo and nexus5x/bullhead), from tcpdump it
> > > seemed ap was receiving packets from the android devices after
> > > successful association, but android devices were not seeing the
> > > dhcp replies.
> > >
> > > I reverted drivers/net/wireless/ralink to the state it is in v5.2.8
> > > and android clients can connect again normally. I did not
> > > explicitly set watchdog parameter to any value.
>
> > Most suspicious are 710e6cc1595e and 41a531ffa4c5 .
>
> It seems to me that reverting only
> 710e6cc1595e25378c4b9977f7a8b4ad4a72a109
> allows all my android devices to successfully connect to the internet.
Please test attached patch as proposed fix for
710e6cc1595e25378c4b9977f7a8b4ad4a72a109 and report back. Thanks.
Stanislaw
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index 4a996550288e..cbec2131e943 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -6095,6 +6095,15 @@ static int rt2800_init_registers(struct rt2x00_dev
*rt2x00dev)
}
/*
+* Clear encryption initialization vectors on start, but keep them
+* for watchdog reset. Otherwise we will have wrong IVs and not be
+* able to keep connections after reset.
+*/
+ if (!test_bit(DEVICE_STATE_RESET, &rt2x00dev->flags))
+ for (i = 0; i < 256; i++)
+ rt2800_register_write(rt2x00dev, MAC_IVEIV_ENTRY(i), 0);
+
+ /*
* Clear all beacons
*/
for (i = 0; i < 8; i++)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00.h
b/drivers/net/wireless/ralink/rt2x00/rt2x00.h
index d35ef06c5c7a..1dd54a0d083d 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00.h
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00.h
@@ -659,6 +659,7 @@ enum rt2x00_state_flags {
DEVICE_STATE_ENABLED_RADIO,
DEVICE_STATE_SCANNING,
DEVICE_STATE_FLUSHING,
+ DEVICE_STATE_RESET,
/*
* Driver configuration
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
index ad063c920323..c3eab767bc21 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
@@ -1253,13 +1253,14 @@ static int rt2x00lib_initialize(struct rt2x00_dev
*rt2x00dev)
int rt2x00lib_start(struct rt2x00_dev *rt2x00dev)
{
- int retval;
+ int retval = 0;
if (test_bit(DEVICE_STATE_STARTED, &rt2x00dev->flags)) {
/*
* This is special case for ieee80211_restart_hw(), otherwise
* mac80211 never call start() two times in row without stop();
*/
+ set_bit(DEVICE_STATE_RESET, &rt2x00dev->flags);
rt2x00dev->ops->lib->pre_reset_hw(rt2x00dev);
rt2x00lib_stop(rt2x00dev);
}
@@ -1270,14 +1271,14 @@ int rt2x00lib_start(struct rt2x00_dev *rt2x00dev)
*/
retval = rt2x00lib_load_firmware(rt2x00dev);
if (retval)
- return retval;
+ goto out;
/*
* Initialize the device.
*/
retval = rt2x00lib_initialize(rt2x00dev);
if (retval)
- return retval;
+ goto out;
rt2x00dev->intf_ap_count = 0;
rt2x00dev->intf_sta_count = 0;
@@ -1286,11 +1287,13 @@ int rt2x00lib_start(struct rt2x00_dev *rt2x00dev)
/* Enable the radio */
retval = rt2x00lib_enable_radio(rt2x00dev);
if (retval)
- return retval;
+ goto out;
set_bit(DEVICE_STATE_STARTED, &rt2x00dev->flags);
- return 0;
+out:
+ clear_bit(DEVICE_STATE_RESET, &rt2x00dev->flags);
+ return retval;
}
void rt2x00lib_stop(struct rt2x00_dev *rt2x00dev)
Re: [PATCH 5.3] mt76: mt76x0e: don't use hw encryption for MT7630E
On Wed, Aug 14, 2019 at 11:50:12AM +0200, Lorenzo Bianconi wrote:
> >
> > Since 41634aa8d6db ("mt76: only schedule txqs from the tx tasklet")
> > I can observe firmware hangs on MT7630E on station mode: tx stop
> > functioning after minor activity (rx keep working) and on module
> > unload device fail to stop with messages:
> >
> > [ 5446.141413] mt76x0e :06:00.0: TX DMA did not stop
> > [ 5449.176764] mt76x0e :06:00.0: TX DMA did not stop
> >
> > Loading module again results in failure to associate with AP.
> > Only machine power off / power on cycle can make device work again.
> >
> > It's unclear why commit 41634aa8d6db causes the problem, but it is
> > related to HW encryption. Since issue is a firmware hang, that is super
> > hard to debug, just disable HW encryption as fix for the issue.
> >
> > Fixes: 41634aa8d6db ("mt76: only schedule txqs from the tx tasklet")
> > Signed-off-by: Stanislaw Gruszka
> > ---
> > drivers/net/wireless/mediatek/mt76/mt76x0/pci.c | 15 ++-
> > 1 file changed, 14 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/pci.c
> > b/drivers/net/wireless/mediatek/mt76/mt76x0/pci.c
> > index 4585e1b756c2..6117e6ca08cb 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt76x0/pci.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt76x0/pci.c
> > @@ -62,6 +62,19 @@ static void mt76x0e_stop(struct ieee80211_hw *hw)
> > mt76x0e_stop_hw(dev);
> > }
> >
> > +static int
> > +mt76x0e_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
> > + struct ieee80211_vif *vif, struct ieee80211_sta *sta,
> > + struct ieee80211_key_conf *key)
> > +{
> > + struct mt76x02_dev *dev = hw->priv;
> > +
> > + if (is_mt7630(dev))
> > + return -EOPNOTSUPP;
>
> Hi Stanislaw,
>
> Can you please try if disabling/enabling the tx tasklet during hw key
> configuration fixes the issue?
> Doing something like:
>
> tasklet_disable(tx_tasklet)
> mt76x02_set_key()
> tasklet_enable(tx_tasklet)
It does not help with the problem.
> Moreover, have you double checked if there is any performance impact
> of not using hw encryption?
I didn't observe any, but realized on this machine I have
aesni_intel encryption accelerator. After rebuild kernel without
CONFIG_CRYPTO_AES_NI_INTEL, 'perf top' showed extra 20% of cpu usage
in aes_encrypt() when sending data with HW encryption disabled.
> If so, I guess it is better to just redefine mt76_wake_tx_queue for
> mt76x0e and run mt76_txq_schedule for 7630e:
>
> void mt76x0e_wake_tx_queue(struct ieee80211_hw *hw, struct ieee80211_txq *txq)
> {
> if (is_mt7630(dev)) {
> mt76_txq_schedule(dev, txq->ac);
> } else {
> tasklet_schedule(&dev->tx_tasklet);
> }
> }
Not sure about reduction of lock contention for which the tx_tasklet
was introduced here, but looks ok for me as fix.
Stanislaw
Re: [PATCH 5.3] mt76: mt76x0e: disable 5GHz band for MT7630E
On Tue, Aug 13, 2019 at 05:55:26PM +, Sasha Levin wrote:
> Hi,
>
> [This is an automated email]
>
> This commit has been processed because it contains a -stable tag.
> The stable tag indicates that it's relevant for the following trees: all
>
> The bot has tested the following trees: v5.2.8, v4.19.66, v4.14.138,
> v4.9.189, v4.4.189.
>
> v5.2.8: Build OK!
> v4.19.66: Failed to apply! Possible dependencies:
> 86c71d3deefa ("mt76: move eeprom utility routines in mt76x02_eeprom.h")
> d6500cf3700f ("mt76x0: add quirk to disable 2.4GHz band for Archer T1U")
> eef40d209ad0 ("mt76: move common eeprom definitions in mt76x02-lib
> module")
> NOTE: The patch will not be queued to stable trees until it is upstream.
>
> How should we proceed with this patch?
mt76x0e support was added in 4.20 , so it's fine just to apply this
commit in 5.2 .
Stanislaw
Re: Ap mode regression in linux-5.3-rc1 in rt2800usb
(cc linux-wireless mailing list) On Tue, Aug 13, 2019 at 09:50:00PM +0300, Emil Karlson wrote: > Greetings > > After upgrading my ap running rt2800usb to linux-5.3-rc1 I noticed an > unusual problem of not being able to connect to my ap with my android > devices (nexus7/flo and nexus5x/bullhead), from tcpdump it seemed ap > was receiving packets from the android devices after successful > association, but android devices were not seeing the dhcp replies. > > I reverted drivers/net/wireless/ralink to the state it is in v5.2.8 and > android clients can connect again normally. I did not explicitly set > watchdog parameter to any value. > > Do you have any insights or is this already fixed? Most likely is not fixed. We have those new commits in 5.3: 41a531ffa4c5 rt2x00usb: fix rx queue hang 0f47aeeada2a rt2800: do not enable watchdog by default e403fa31ed71 rt2x00: add restart hw 710e6cc1595e rt2800: do not nullify initialization vector data 09db3b000619 rt2800: add pre_reset_hw callback 759c5b599cf4 rt2800: initial watchdog implementation 2034afe4db4a rt2800: add helpers for reading dma done index 9f3e3323e996 rt2x00: allow to specify watchdog interval Most suspicious are 710e6cc1595e and 41a531ffa4c5 . Could you test by reverting one single commit on 5.3 (first 710e6cc1595e and if that not help 41a531ffa4c5) and check if it makes the problem gone? Stanislaw
[PATCH 5.3] mt76: mt76x0e: disable 5GHz band for MT7630E
MT7630E hardware does support 5GHz, but we do not properly configure phy
for 5GHz channels. Scanning at this band not only do not show any APs
but also can hang the firmware.
Since vendor reference driver do not support 5GHz we don't know how
properly configure 5GHz channels. So disable this band for MT7630E .
Cc: sta...@vger.kernel.org
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c | 5 +
1 file changed, 5 insertions(+)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c
b/drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c
index ab6dfc026acb..36918b1bd653 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/eeprom.c
@@ -67,6 +67,11 @@ static void mt76x0_set_chip_cap(struct mt76x02_dev *dev)
dev_dbg(dev->mt76.dev, "mask out 2GHz support\n");
}
+ if (is_mt7630(dev)) {
+ dev->mt76.cap.has_5ghz = false;
+ dev_dbg(dev->mt76.dev, "mask out 5GHz support\n");
+ }
+
if (!mt76x02_field_valid(nic_conf1 & 0xff))
nic_conf1 &= 0xff00;
--
1.9.3
[PATCH 5.3] mt76: mt76x0e: don't use hw encryption for MT7630E
Since 41634aa8d6db ("mt76: only schedule txqs from the tx tasklet")
I can observe firmware hangs on MT7630E on station mode: tx stop
functioning after minor activity (rx keep working) and on module
unload device fail to stop with messages:
[ 5446.141413] mt76x0e :06:00.0: TX DMA did not stop
[ 5449.176764] mt76x0e :06:00.0: TX DMA did not stop
Loading module again results in failure to associate with AP.
Only machine power off / power on cycle can make device work again.
It's unclear why commit 41634aa8d6db causes the problem, but it is
related to HW encryption. Since issue is a firmware hang, that is super
hard to debug, just disable HW encryption as fix for the issue.
Fixes: 41634aa8d6db ("mt76: only schedule txqs from the tx tasklet")
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/mediatek/mt76/mt76x0/pci.c | 15 ++-
1 file changed, 14 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/pci.c
b/drivers/net/wireless/mediatek/mt76/mt76x0/pci.c
index 4585e1b756c2..6117e6ca08cb 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/pci.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/pci.c
@@ -62,6 +62,19 @@ static void mt76x0e_stop(struct ieee80211_hw *hw)
mt76x0e_stop_hw(dev);
}
+static int
+mt76x0e_set_key(struct ieee80211_hw *hw, enum set_key_cmd cmd,
+ struct ieee80211_vif *vif, struct ieee80211_sta *sta,
+ struct ieee80211_key_conf *key)
+{
+ struct mt76x02_dev *dev = hw->priv;
+
+ if (is_mt7630(dev))
+ return -EOPNOTSUPP;
+
+ return mt76x02_set_key(hw, cmd, vif, sta, key);
+}
+
static void
mt76x0e_flush(struct ieee80211_hw *hw, struct ieee80211_vif *vif,
u32 queues, bool drop)
@@ -78,7 +91,7 @@ static void mt76x0e_stop(struct ieee80211_hw *hw)
.configure_filter = mt76x02_configure_filter,
.bss_info_changed = mt76x02_bss_info_changed,
.sta_state = mt76_sta_state,
- .set_key = mt76x02_set_key,
+ .set_key = mt76x0e_set_key,
.conf_tx = mt76x02_conf_tx,
.sw_scan_start = mt76x02_sw_scan,
.sw_scan_complete = mt76x02_sw_scan_complete,
--
1.9.3
Re: [RFC] mt76: fix tx hung regression on MT7630E
On Mon, Aug 05, 2019 at 02:39:16PM +0200, Stanislaw Gruszka wrote:
> On Mon, Aug 05, 2019 at 01:27:19PM +0200, Lorenzo Bianconi wrote:
> > > ... but I think we have bug when do mt76_txq_schedule_all() in
> > > tx_tasklet, because we can schedule on queues that are stoped.
> > > So reverting 41634aa8d6db and then optimize by removing tx_tasklet
> > > for mmio and remove not needed mt76_txq_schedule_all() calls looks
> > > more reasoneble to me.
> >
> > schedule a stopped queue seems not harmful at a first glance since we do not
> > copy pending skbs if we have not enough room in the dma ring.
>
> mac80211 stop queues for various other reasons than
> IEEE80211_QUEUE_STOP_REASON_DRIVER .
After looking in more details, we check if queue is stopped in
ieee80211_tx_dequeue(). But we do not check that for skb's queued
in mtxq->retry_q .
> > diff --git a/drivers/net/wireless/mediatek/mt76/tx.c
> > b/drivers/net/wireless/mediatek/mt76/tx.c
> > index 5397827668b9..bd2d34c4f326 100644
> > --- a/drivers/net/wireless/mediatek/mt76/tx.c
> > +++ b/drivers/net/wireless/mediatek/mt76/tx.c
> > @@ -495,6 +495,9 @@ mt76_txq_schedule_list(struct mt76_dev *dev, enum
> > mt76_txq_id qid)
> > while (1) {
> > bool empty = false;
> >
> > + if (hwq->stopped)
> > + break;
> > +
> > if (sq->swq_queued >= 4)
> > break;
> >
> > Does it fix the issue you are facing?
>
> I'll not be able to test this patch this week. Will have access to
> the hardware next week.
>
> I checeked before, if
> 'q->queued + (tx_info.nbuf + 1) / 2 >= q->ndesc - 1' is triggered
> when MT7630E hangs and it is not. But maybe second part of the patch
> will help.
Patch did not help.
I debugged problem a bit more and issue is related with HW encryption.
Using full mac80211 SW encyption by retuning -EOPNOTSUPP
mt76x02_set_key() make the problem gone as well. Looks there is
some race between setting HW keys and transmitting.
Stanislaw
Re: [PATCH] mt76: mt76x02u: enable multi-vif support
On Mon, Aug 05, 2019 at 01:08:15PM +0200, Lorenzo Bianconi wrote: > > > > On Fri, Aug 02, 2019 at 04:36:20PM +0200, Lorenzo Bianconi wrote: > > > Enable multi-interface support for mt76x02u driver. For the moment > > > allow max two concurrent interfaces in order to preserve enough room > > > for ps traffic since we are using beacon slots for it. > > > I have successfully tested the following configuration: > > > - AP + STA > > > - AP0 + AP1 > > > > The combination that did not work in my testing was STA + STA . > > This should be fixed or disabled. > > > > Hi Stanislaw, > > I have just tested STA/STA configuration and it works in my setup. Confirmed, it works for me as well on current code base. Tested-by: Stanislaw Gruszka
Re: [RFC] mt76: fix tx hung regression on MT7630E
On Mon, Aug 05, 2019 at 01:27:19PM +0200, Lorenzo Bianconi wrote:
> > ... but I think we have bug when do mt76_txq_schedule_all() in
> > tx_tasklet, because we can schedule on queues that are stoped.
> > So reverting 41634aa8d6db and then optimize by removing tx_tasklet
> > for mmio and remove not needed mt76_txq_schedule_all() calls looks
> > more reasoneble to me.
>
> schedule a stopped queue seems not harmful at a first glance since we do not
> copy pending skbs if we have not enough room in the dma ring.
mac80211 stop queues for various other reasons than
IEEE80211_QUEUE_STOP_REASON_DRIVER .
> Maybe we can be
> more conservative doing something like:
>
> diff --git a/drivers/net/wireless/mediatek/mt76/dma.c
> b/drivers/net/wireless/mediatek/mt76/dma.c
> index d8f61e540bfd..c6482155e5e4 100644
> --- a/drivers/net/wireless/mediatek/mt76/dma.c
> +++ b/drivers/net/wireless/mediatek/mt76/dma.c
> @@ -346,6 +346,11 @@ mt76_dma_tx_queue_skb(struct mt76_dev *dev, enum
> mt76_txq_id qid,
> goto unmap;
>
> if (q->queued + (tx_info.nbuf + 1) / 2 >= q->ndesc - 1) {
> + if (!q->stopped) {
> + ieee80211_stop_queue(dev->hw,
> + skb_get_queue_mapping(skb));
> + q->stopped = true;
> + }
> ret = -ENOMEM;
> goto unmap;
> }
> diff --git a/drivers/net/wireless/mediatek/mt76/tx.c
> b/drivers/net/wireless/mediatek/mt76/tx.c
> index 5397827668b9..bd2d34c4f326 100644
> --- a/drivers/net/wireless/mediatek/mt76/tx.c
> +++ b/drivers/net/wireless/mediatek/mt76/tx.c
> @@ -495,6 +495,9 @@ mt76_txq_schedule_list(struct mt76_dev *dev, enum
> mt76_txq_id qid)
> while (1) {
> bool empty = false;
>
> + if (hwq->stopped)
> + break;
> +
> if (sq->swq_queued >= 4)
> break;
>
> Does it fix the issue you are facing?
I'll not be able to test this patch this week. Will have access to
the hardware next week.
I checeked before, if
'q->queued + (tx_info.nbuf + 1) / 2 >= q->ndesc - 1' is triggered
when MT7630E hangs and it is not. But maybe second part of the patch
will help.
Stanislaw
Re: rt2x00usb warning while channel hopping
On Thu, Aug 01, 2019 at 12:43:26PM -0400, Sid Hayn wrote: > While testing wireless-testing kernel for some other fixes, I > encountered this warning. I have a few different chipsets and drivers > all channel hopping in monitor mode on the test box, but I get this > warning from rt2x00 over and over again (lots). I am testing two > patches from johill on top of wireless testing, I don't believe they > are causing this, but I'll include them for completeness. > > https://patchwork.kernel.org/patch/11063915/ > https://patchwork.kernel.org/patch/11069625/ The way to find out is drop those patches and see if the warning is still there. But it still can be rt2x00 problem that is only triggered with those patches, so let's try to debug. > I'm happy to test anything required, although I will be traveling next > week which may cause delays. > [ 170.055276] [ cut here ] > [ 170.055305] WARNING: CPU: 3 PID: 5777 at > rt2x00lib_config.cold.0+0xc/0x2a [rt2x00lib] This seems to be warning at rt2x00ht_center_channel() in: for (i = 0; i < spec->num_channels; i++) if (spec->channels[i].channel == center_channel) return i; WARN_ON(1); return conf->chandef.chan->hw_value; So looks like we set channel that is not specified in rt2800 channels table. Not sure at the moment if mac80211 provide wrong channel spec or rt2x00 do something wrong. Please apply below patch and provide prints. Stanislaw diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00config.c b/drivers/net/wireless/ralink/rt2x00/rt2x00config.c index 0ee1813e8453..5f6dadef9f7c 100644 --- a/drivers/net/wireless/ralink/rt2x00/rt2x00config.c +++ b/drivers/net/wireless/ralink/rt2x00/rt2x00config.c @@ -185,7 +185,11 @@ static u16 rt2x00ht_center_channel(struct rt2x00_dev *rt2x00dev, if (spec->channels[i].channel == center_channel) return i; - WARN_ON(1); + WARN_ONCE(1, "hw_value %d channel %d center_channel %d\n", + conf->chandef.chan->hw_value, + spec->channels[conf->chandef.chan->hw_value].channel, + center_channel); + return conf->chandef.chan->hw_value; }
Re: [PATCH] mt76: mt76x02u: enable multi-vif support
On Fri, Aug 02, 2019 at 04:36:20PM +0200, Lorenzo Bianconi wrote: > Enable multi-interface support for mt76x02u driver. For the moment > allow max two concurrent interfaces in order to preserve enough room > for ps traffic since we are using beacon slots for it. > I have successfully tested the following configuration: > - AP + STA > - AP0 + AP1 The combination that did not work in my testing was STA + STA . This should be fixed or disabled. Stanislaw
Re: [RFC] mt76: fix tx hung regression on MT7630E
On Wed, Jul 31, 2019 at 11:09:27AM +0200, Lorenzo Bianconi wrote:
> > On Wed, Jul 31, 2019 at 10:19:58AM +0200, Stanislaw Gruszka wrote:
> > > On Tue, Jul 30, 2019 at 04:55:31PM +0200, Lorenzo Bianconi wrote:
> > > > > > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
> > > > > > > b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
> > > > > > > index 467b28379870..622251faa415 100644
> > > > > > > --- a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
> > > > > > > +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
> > > > > > > @@ -170,7 +170,7 @@ static int mt76x02_poll_tx(struct napi_struct
> > > > > > > *napi, int budget)
> > > > > > > mt76.tx_napi);
> > > > > > > int i;
> > > > > > >
> > > > > > > - mt76x02_mac_poll_tx_status(dev, false);
> > > > > > > + mt76x02_mac_poll_tx_status(dev, true);
> > > > > >
> > > > > > I am not sure if we really need mt76x02_mac_poll_tx_status() here
> > > > > > since we run
> > > > > > it in mt76x02_tx_complete_skb() and in mt76x02_tx_tasklet(). Anyway
> > > > > > the only
> > > > > > difference doing so is we do not run mt76x02_send_tx_status().
> > > > >
> > > > > I thought this is the problem, but it was my mistake during testing.
> > > > > I tested the above change together with mt76_txq_schedule(dev,
> > > > > txq->ac)
> > > > > change and get wrong impression it fixes the issue. But above change
> > > > > alone does not help.
> > > > >
> > > > > I tried to add some locking to avoid parallel execution of
> > > > > mt76x02_poll_tx()
> > > > > and mt76x02_tx_tasklet(), but it didn't help either. So far only patch
> > > > > originally posted here make the problem gone.
> > > >
> > > > so, in order to be on the same page, if you comment out
> > > > mt76x02_mac_poll_tx_status()
> > > > in mt76x02_poll_tx() the issue will still occur. The only to 'fix' it
> > > > is to run
> > > > mt76_txq_schedule_all() in mt76x02_poll_tx(), right?
> > >
> > > Yes.
> >
> > Err, no, I should read more cerfully. It is partiall revert of
> > 41634aa8d6db ("mt76: only schedule txqs from the tx tasklet") :
> >
> > diff --git a/drivers/net/wireless/mediatek/mt76/tx.c
> > b/drivers/net/wireless/mediatek/mt76/tx.c
> > index 5397827668b9..fefe0ee52584 100644
> > --- a/drivers/net/wireless/mediatek/mt76/tx.c
> > +++ b/drivers/net/wireless/mediatek/mt76/tx.c
> > @@ -598,7 +598,7 @@ void mt76_wake_tx_queue(struct ieee80211_hw *hw, struct
> > ieee80211_txq *txq)
> > if (!test_bit(MT76_STATE_RUNNING, &dev->state))
> > return;
> >
> > - tasklet_schedule(&dev->tx_tasklet);
> > + mt76_txq_schedule(dev, txq->ac);
> > }
> > EXPORT_SYMBOL_GPL(mt76_wake_tx_queue);
>
> reviewing the code I think:
>
> - we should not run mt76u_tx_tasklet() from mt76_wake_tx_queue() since we do
> not have tx_napi for usb and it will unnecessary go through tx queue checks.
> We should probably do in mt76_wake_tx_queue() something like:
>
> if (is_mmio())
Adding '&& !is_mt7630()' will solve the problem for MT7630E as well ...
> tasklet_schedule(&dev->tx_tasklet);
> else
> mt76_txq_schedule(dev, txq->ac);
>
> Another solution would be add a status_tasklet that just goes through the tx
> queues receiving the usb tx completion and it schedules the tx_tasklet
> What do you think?
>
> - I guess it does not fix the 76x0e issue but we should just schedule tx
> queues in
> mt76x02_tx_tasklet() (like it is done for mt7603 and mt7615) and move status
> processing in mt76x02_poll_tx()
... but I think we have bug when do mt76_txq_schedule_all() in
tx_tasklet, because we can schedule on queues that are stoped.
So reverting 41634aa8d6db and then optimize by removing tx_tasklet
for mmio and remove not needed mt76_txq_schedule_all() calls looks
more reasoneble to me.
Stanislaw
Re: [RFC] mt76: fix tx hung regression on MT7630E
On Wed, Jul 31, 2019 at 10:19:58AM +0200, Stanislaw Gruszka wrote:
> On Tue, Jul 30, 2019 at 04:55:31PM +0200, Lorenzo Bianconi wrote:
> > > > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
> > > > > b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
> > > > > index 467b28379870..622251faa415 100644
> > > > > --- a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
> > > > > +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
> > > > > @@ -170,7 +170,7 @@ static int mt76x02_poll_tx(struct napi_struct
> > > > > *napi, int budget)
> > > > > mt76.tx_napi);
> > > > > int i;
> > > > >
> > > > > - mt76x02_mac_poll_tx_status(dev, false);
> > > > > + mt76x02_mac_poll_tx_status(dev, true);
> > > >
> > > > I am not sure if we really need mt76x02_mac_poll_tx_status() here since
> > > > we run
> > > > it in mt76x02_tx_complete_skb() and in mt76x02_tx_tasklet(). Anyway the
> > > > only
> > > > difference doing so is we do not run mt76x02_send_tx_status().
> > >
> > > I thought this is the problem, but it was my mistake during testing.
> > > I tested the above change together with mt76_txq_schedule(dev, txq->ac)
> > > change and get wrong impression it fixes the issue. But above change
> > > alone does not help.
> > >
> > > I tried to add some locking to avoid parallel execution of
> > > mt76x02_poll_tx()
> > > and mt76x02_tx_tasklet(), but it didn't help either. So far only patch
> > > originally posted here make the problem gone.
> >
> > so, in order to be on the same page, if you comment out
> > mt76x02_mac_poll_tx_status()
> > in mt76x02_poll_tx() the issue will still occur. The only to 'fix' it is to
> > run
> > mt76_txq_schedule_all() in mt76x02_poll_tx(), right?
>
> Yes.
Err, no, I should read more cerfully. It is partiall revert of
41634aa8d6db ("mt76: only schedule txqs from the tx tasklet") :
diff --git a/drivers/net/wireless/mediatek/mt76/tx.c
b/drivers/net/wireless/mediatek/mt76/tx.c
index 5397827668b9..fefe0ee52584 100644
--- a/drivers/net/wireless/mediatek/mt76/tx.c
+++ b/drivers/net/wireless/mediatek/mt76/tx.c
@@ -598,7 +598,7 @@ void mt76_wake_tx_queue(struct ieee80211_hw *hw, struct
ieee80211_txq *txq)
if (!test_bit(MT76_STATE_RUNNING, &dev->state))
return;
- tasklet_schedule(&dev->tx_tasklet);
+ mt76_txq_schedule(dev, txq->ac);
}
EXPORT_SYMBOL_GPL(mt76_wake_tx_queue);
Re: [RFC] mt76: fix tx hung regression on MT7630E
On Tue, Jul 30, 2019 at 04:55:31PM +0200, Lorenzo Bianconi wrote: > > > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c > > > > b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c > > > > index 467b28379870..622251faa415 100644 > > > > --- a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c > > > > +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c > > > > @@ -170,7 +170,7 @@ static int mt76x02_poll_tx(struct napi_struct > > > > *napi, int budget) > > > >mt76.tx_napi); > > > > int i; > > > > > > > > - mt76x02_mac_poll_tx_status(dev, false); > > > > + mt76x02_mac_poll_tx_status(dev, true); > > > > > > I am not sure if we really need mt76x02_mac_poll_tx_status() here since > > > we run > > > it in mt76x02_tx_complete_skb() and in mt76x02_tx_tasklet(). Anyway the > > > only > > > difference doing so is we do not run mt76x02_send_tx_status(). > > > > I thought this is the problem, but it was my mistake during testing. > > I tested the above change together with mt76_txq_schedule(dev, txq->ac) > > change and get wrong impression it fixes the issue. But above change > > alone does not help. > > > > I tried to add some locking to avoid parallel execution of mt76x02_poll_tx() > > and mt76x02_tx_tasklet(), but it didn't help either. So far only patch > > originally posted here make the problem gone. > > so, in order to be on the same page, if you comment out > mt76x02_mac_poll_tx_status() > in mt76x02_poll_tx() the issue will still occur. The only to 'fix' it is to > run > mt76_txq_schedule_all() in mt76x02_poll_tx(), right? Yes. Stanislaw
Re: [RFC] mt76: fix tx hung regression on MT7630E
On Mon, Jul 29, 2019 at 04:02:41PM +0200, Lorenzo Bianconi wrote:
> > On Fri, Jul 26, 2019 at 02:10:56PM +0200, Stanislaw Gruszka wrote:
> > > Since 41634aa8d6db ("mt76: only schedule txqs from the tx tasklet")
> > > I can observe firmware hangs on MT7630E on station mode: tx stop
> > > functioning after minor activity (rx keep working) and on module
> > > unload device fail to stop with messages:
> > >
> > > [ 5446.141413] mt76x0e :06:00.0: TX DMA did not stop
> > > [ 5449.176764] mt76x0e :06:00.0: TX DMA did not stop
> > >
> > > Loading module again results in failure to associate with AP.
> > > Only machine power off / power on cycle can make device work again.
> > >
> > > I have no idea why the commit caused F/W hangs. Most likely some proper
> > > fix is needed of changing registers programming (or assuring proper order
> > > of actions), but so far I can not came up with any better fix than
> > > a partial revert of 41634aa8d6db.
> >
> > The difference is that with 41634aa8d6db we can run mt76x02_poll_tx()
> > and mt76x02_tx_tasklet() in parallel on 2 different CPUs. Without
> > the commit, since tasklet is not scheduled from mt76_wake_tx_queue(),
> > it does not happen.
> >
> > I'm not quite sure why, but MT7630E does not like when we poll tx status
> > on 2 cpus at once. Change like below:
>
> Hi Stanislaw,
Hi
> have you tried to look at the commit: 6fe533378795f87
> ("mt76: mt76x02: remove irqsave/restore in locking for tx status fifo")?
> Could it be a race between a registermap update and a stats load? (we are
> using a
> different lock now)
This commit seems to be fine, reverting it did not solve the issue.
> > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
> > b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
> > index 467b28379870..622251faa415 100644
> > --- a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
> > +++ b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
> > @@ -170,7 +170,7 @@ static int mt76x02_poll_tx(struct napi_struct *napi,
> > int budget)
> >mt76.tx_napi);
> > int i;
> >
> > - mt76x02_mac_poll_tx_status(dev, false);
> > + mt76x02_mac_poll_tx_status(dev, true);
>
> I am not sure if we really need mt76x02_mac_poll_tx_status() here since we run
> it in mt76x02_tx_complete_skb() and in mt76x02_tx_tasklet(). Anyway the only
> difference doing so is we do not run mt76x02_send_tx_status().
I thought this is the problem, but it was my mistake during testing.
I tested the above change together with mt76_txq_schedule(dev, txq->ac)
change and get wrong impression it fixes the issue. But above change
alone does not help.
I tried to add some locking to avoid parallel execution of mt76x02_poll_tx()
and mt76x02_tx_tasklet(), but it didn't help either. So far only patch
originally posted here make the problem gone.
Stanislaw
Re: [PATCH v2 5/5] rtw88: add BT co-existence support
On Tue, Jul 30, 2019 at 03:13:35AM +, Tony Chuang wrote: > > Those coex response skb buffers are allocated in rtw_pci_rx_isr(), > > but I do not see where they are freed (seems we do not process > > them in c2h_work which does dev_kfree_skb()). > > You're right, that SKB leaked. Should free them after responded. > I will send v2 to fix it :) FWIW maybe would be better to process coex commands entairly in c2h_work ? Not sure if that would work or really would be better, just an idea you can consider :-) Stanislaw
Re: [RFC] mt76: fix tx hung regression on MT7630E
On Fri, Jul 26, 2019 at 02:10:56PM +0200, Stanislaw Gruszka wrote:
> Since 41634aa8d6db ("mt76: only schedule txqs from the tx tasklet")
> I can observe firmware hangs on MT7630E on station mode: tx stop
> functioning after minor activity (rx keep working) and on module
> unload device fail to stop with messages:
>
> [ 5446.141413] mt76x0e :06:00.0: TX DMA did not stop
> [ 5449.176764] mt76x0e :06:00.0: TX DMA did not stop
>
> Loading module again results in failure to associate with AP.
> Only machine power off / power on cycle can make device work again.
>
> I have no idea why the commit caused F/W hangs. Most likely some proper
> fix is needed of changing registers programming (or assuring proper order
> of actions), but so far I can not came up with any better fix than
> a partial revert of 41634aa8d6db.
The difference is that with 41634aa8d6db we can run mt76x02_poll_tx()
and mt76x02_tx_tasklet() in parallel on 2 different CPUs. Without
the commit, since tasklet is not scheduled from mt76_wake_tx_queue(),
it does not happen.
I'm not quite sure why, but MT7630E does not like when we poll tx status
on 2 cpus at once. Change like below:
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
index 467b28379870..622251faa415 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_mmio.c
@@ -170,7 +170,7 @@ static int mt76x02_poll_tx(struct napi_struct *napi, int
budget)
mt76.tx_napi);
int i;
- mt76x02_mac_poll_tx_status(dev, false);
+ mt76x02_mac_poll_tx_status(dev, true);
for (i = MT_TXQ_MCU; i >= 0; i--)
mt76_queue_tx_cleanup(dev, i, false);
is sufficient to avoid the hangs.
> diff --git a/drivers/net/wireless/mediatek/mt76/tx.c
> b/drivers/net/wireless/mediatek/mt76/tx.c
> index 5397827668b9..fefe0ee52584 100644
> --- a/drivers/net/wireless/mediatek/mt76/tx.c
> +++ b/drivers/net/wireless/mediatek/mt76/tx.c
> @@ -598,7 +598,7 @@ void mt76_wake_tx_queue(struct ieee80211_hw *hw, struct
> ieee80211_txq *txq)
> if (!test_bit(MT76_STATE_RUNNING, &dev->state))
> return;
>
> - tasklet_schedule(&dev->tx_tasklet);
> + mt76_txq_schedule(dev, txq->ac);
However I'm not sure if change to tasklet_schedule() is indeed correct,
since on tasklet we schedule all queues, hence queues that could
possibly be still blocked? And on mt76_wake_tx_queue() we schedule only
one queue.
Stanislaw
Re: [PATCH v2 5/5] rtw88: add BT co-existence support
On Thu, Jul 25, 2019 at 10:53:31AM +0800, yhchu...@realtek.com wrote:
> +static struct sk_buff *rtw_coex_info_request(struct rtw_dev *rtwdev,
> + struct rtw_coex_info_req *req)
> +{
> + struct rtw_coex *coex = &rtwdev->coex;
> + struct sk_buff *skb_resp = NULL;
> +
> + mutex_lock(&coex->mutex);
> +
> + rtw_fw_query_bt_mp_info(rtwdev, req);
> +
> + if (!wait_event_timeout(coex->wait, !skb_queue_empty(&coex->queue),
> + COEX_REQUEST_TIMEOUT)) {
> + rtw_err(rtwdev, "coex request time out\n");
> + goto out;
> + }
> +
> + skb_resp = skb_dequeue(&coex->queue);
> + if (!skb_resp) {
> + rtw_err(rtwdev, "failed to get coex info response\n");
> + goto out;
> + }
> +
> +out:
> + mutex_unlock(&coex->mutex);
> + return skb_resp;
> +}
> +
> +static bool rtw_coex_get_bt_scan_type(struct rtw_dev *rtwdev, u8 *scan_type)
> +{
> + struct rtw_coex_info_req req = {0};
> + struct sk_buff *skb;
> + u8 *payload;
> + bool ret = false;
> +
> + req.op_code = BT_MP_INFO_OP_SCAN_TYPE;
> + skb = rtw_coex_info_request(rtwdev, &req);
> + if (!skb)
> + goto out;
> +
> + payload = get_payload_from_coex_resp(skb);
> + *scan_type = GET_COEX_RESP_BT_SCAN_TYPE(payload);
> + ret = true;
> +
> +out:
> + return ret;
> +}
> +
> +static bool rtw_coex_set_lna_constrain_level(struct rtw_dev *rtwdev,
> + u8 lna_constrain_level)
> +{
> + struct rtw_coex_info_req req = {0};
> + struct sk_buff *skb;
> + bool ret = false;
> +
> + req.op_code = BT_MP_INFO_OP_LNA_CONSTRAINT;
> + req.para1 = lna_constrain_level;
> + skb = rtw_coex_info_request(rtwdev, &req);
> + if (!skb)
> + goto out;
Those coex response skb buffers are allocated in rtw_pci_rx_isr(),
but I do not see where they are freed (seems we do not process
them in c2h_work which does dev_kfree_skb()).
Stanislaw
[RFC] mt76: fix tx hung regression on MT7630E
Since 41634aa8d6db ("mt76: only schedule txqs from the tx tasklet")
I can observe firmware hangs on MT7630E on station mode: tx stop
functioning after minor activity (rx keep working) and on module
unload device fail to stop with messages:
[ 5446.141413] mt76x0e :06:00.0: TX DMA did not stop
[ 5449.176764] mt76x0e :06:00.0: TX DMA did not stop
Loading module again results in failure to associate with AP.
Only machine power off / power on cycle can make device work again.
I have no idea why the commit caused F/W hangs. Most likely some proper
fix is needed of changing registers programming (or assuring proper order
of actions), but so far I can not came up with any better fix than
a partial revert of 41634aa8d6db.
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/mediatek/mt76/tx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/mediatek/mt76/tx.c
b/drivers/net/wireless/mediatek/mt76/tx.c
index 5397827668b9..fefe0ee52584 100644
--- a/drivers/net/wireless/mediatek/mt76/tx.c
+++ b/drivers/net/wireless/mediatek/mt76/tx.c
@@ -598,7 +598,7 @@ void mt76_wake_tx_queue(struct ieee80211_hw *hw, struct
ieee80211_txq *txq)
if (!test_bit(MT76_STATE_RUNNING, &dev->state))
return;
- tasklet_schedule(&dev->tx_tasklet);
+ mt76_txq_schedule(dev, txq->ac);
}
EXPORT_SYMBOL_GPL(mt76_wake_tx_queue);
--
1.9.3
Re: mt76x0u blocks resume from suspend
Hi On Mon, Jul 22, 2019 at 06:40:20PM +0200, Johannes Stezenbach wrote: > I met failure to resume from suspend to RAM with v5.1.19 > and TP-Link Archer T2UH connected to my PC (Asus P8H77-V). > I tried about a dozen times while trying to figure out the reason, > the issue happened every time. > Eventually I connected a serial console and captured this: > > [ 87.803576][ T4640] mt76x0u 2-1.6:1.0: vendor request req:06 off:ac80 > failed:-110 > [ 91.030328][ T4640] mt76x0u 2-1.6:1.0: vendor request req:07 off:b000 > failed:-110 > [ 94.256950][ T4640] mt76x0u 2-1.6:1.0: vendor request req:06 off:b000 > failed:-110 > [ 97.483579][ T4640] mt76x0u 2-1.6:1.0: vendor request req:06 off:aca0 > failed:-110 > (repeats) > [ 107.492106][C0] Call Trace: > [ 107.495293][C0] ? __schedule+0x381/0xa30 > [ 107.499706][C0] schedule+0x36/0x90 > [ 107.503575][C0] schedule_timeout+0x1e8/0x4c0 > [ 107.508310][C0] ? collect_expired_timers+0xb0/0xb0 > [ 107.513593][C0] wait_for_common+0x15f/0x190 > [ 107.518249][C0] ? wake_up_q+0x80/0x80 > [ 107.522412][C0] usb_start_wait_urb+0x82/0x160 > [ 107.527222][C0] ? wait_for_common+0x38/0x190 > [ 107.531961][C0] usb_control_msg+0xdc/0x140 > [ 107.536542][C0] __mt76u_vendor_request+0xc4/0x100 [mt76_usb] > [ 107.542687][C0] mt76u_copy+0x8b/0xb0 [mt76_usb] > [ 107.547721][C0] mt76x02_mac_shared_key_setup+0xdf/0x130 [mt76x02_lib] > The whole machine is hanging at this point. without serial console > my only choice was to press the reset button. At least mt76x0u should > bail out with an error and not block the resume completely. > Unplugging the hardware also didn't cause it to bail. I recently sent fix for that https://lore.kernel.org/linux-wireless/1563446290-2813-1-git-send-email-sgrus...@redhat.com/raw Stanislaw
Re: [PATCH 5.3] mt76: mt76x0u: do not reset radio on resume
On Fri, Jul 19, 2019 at 12:45:23AM +, Sasha Levin wrote: > v5.1.18: Failed to apply! Possible dependencies: > Unable to calculate > > v4.19.59: Failed to apply! Possible dependencies: > How should we proceed with this patch? I'll provide backported version of the patch for 4.19 and 5.1 after it will be merged to Linus' tree. Stanislaw
[PATCH 5.3] mt76: mt76x0u: do not reset radio on resume
On some machines mt76x0u firmware can hung during resume,
what result on messages like below:
[ 475.480062] mt76x0 1-8:1.0: Error: MCU response pre-completed!
[ 475.990066] mt76x0 1-8:1.0: Error: send MCU cmd failed:-110
[ 475.990075] mt76x0 1-8:1.0: Error: MCU response pre-completed!
[ 476.53] mt76x0 1-8:1.0: Error: send MCU cmd failed:-110
[ 476.500012] mt76x0 1-8:1.0: Error: MCU response pre-completed!
[ 477.010046] mt76x0 1-8:1.0: Error: send MCU cmd failed:-110
[ 477.010055] mt76x0 1-8:1.0: Error: MCU response pre-completed!
[ 477.529997] mt76x0 1-8:1.0: Error: send MCU cmd failed:-110
[ 477.530006] mt76x0 1-8:1.0: Error: MCU response pre-completed!
[ 477.824907] mt76x0 1-8:1.0: Error: send MCU cmd failed:-71
[ 477.824916] mt76x0 1-8:1.0: Error: MCU response pre-completed!
[ 477.825029] usb 1-8: USB disconnect, device number 6
and possible whole system freeze.
This can be avoided, if we do not perform mt76x0_chip_onoff() reset.
Cc: sta...@vger.kernel.org
Fixes: 134b2d0d1fcf ("mt76x0: init files")
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/mediatek/mt76/mt76x0/usb.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
index 627ed1fc7b15..645f4d15fb61 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x0/usb.c
@@ -136,11 +136,11 @@ static int mt76x0u_start(struct ieee80211_hw *hw)
.release_buffered_frames = mt76_release_buffered_frames,
};
-static int mt76x0u_init_hardware(struct mt76x02_dev *dev)
+static int mt76x0u_init_hardware(struct mt76x02_dev *dev, bool reset)
{
int err;
- mt76x0_chip_onoff(dev, true, true);
+ mt76x0_chip_onoff(dev, true, reset);
if (!mt76x02_wait_for_mac(&dev->mt76))
return -ETIMEDOUT;
@@ -173,7 +173,7 @@ static int mt76x0u_register_device(struct mt76x02_dev *dev)
if (err < 0)
goto out_err;
- err = mt76x0u_init_hardware(dev);
+ err = mt76x0u_init_hardware(dev, true);
if (err < 0)
goto out_err;
@@ -309,7 +309,7 @@ static int __maybe_unused mt76x0_resume(struct
usb_interface *usb_intf)
if (ret < 0)
goto err;
- ret = mt76x0u_init_hardware(dev);
+ ret = mt76x0u_init_hardware(dev, false);
if (ret)
goto err;
--
1.9.3
[PATCH] mt7601u: use params->ssn value directly
There is no point to use pointer to params->ssn. Signed-off-by: Stanislaw Gruszka --- drivers/net/wireless/mediatek/mt7601u/main.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/drivers/net/wireless/mediatek/mt7601u/main.c b/drivers/net/wireless/mediatek/mt7601u/main.c index 89a7b1234ffb..72e608cc53af 100644 --- a/drivers/net/wireless/mediatek/mt7601u/main.c +++ b/drivers/net/wireless/mediatek/mt7601u/main.c @@ -351,7 +351,7 @@ mt76_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif, struct ieee80211_sta *sta = params->sta; enum ieee80211_ampdu_mlme_action action = params->action; u16 tid = params->tid; - u16 *ssn = ¶ms->ssn; + u16 ssn = params->ssn; struct mt76_sta *msta = (struct mt76_sta *) sta->drv_priv; WARN_ON(msta->wcid.idx > GROUP_WCID(0)); @@ -371,7 +371,7 @@ mt76_ampdu_action(struct ieee80211_hw *hw, struct ieee80211_vif *vif, case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT: break; case IEEE80211_AMPDU_TX_START: - msta->agg_ssn[tid] = *ssn << 4; + msta->agg_ssn[tid] = ssn << 4; ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid); break; case IEEE80211_AMPDU_TX_STOP_CONT: -- 2.20.1
[PATCH 1/3] mt76: mt76x02: use params->ssn value directly
There is no point to use pointer to params->ssn.
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/mediatek/mt76/mt76x02_util.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
index ad5323447ed4..fa45ed280ab1 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
+++ b/drivers/net/wireless/mediatek/mt76/mt76x02_util.c
@@ -348,7 +348,7 @@ int mt76x02_ampdu_action(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
struct mt76x02_sta *msta = (struct mt76x02_sta *) sta->drv_priv;
struct ieee80211_txq *txq = sta->txq[params->tid];
u16 tid = params->tid;
- u16 *ssn = ¶ms->ssn;
+ u16 ssn = params->ssn;
struct mt76_txq *mtxq;
if (!txq)
@@ -359,7 +359,7 @@ int mt76x02_ampdu_action(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
switch (action) {
case IEEE80211_AMPDU_RX_START:
mt76_rx_aggr_start(&dev->mt76, &msta->wcid, tid,
- *ssn, params->buf_size);
+ ssn, params->buf_size);
mt76_set(dev, MT_WCID_ADDR(msta->wcid.idx) + 4, BIT(16 + tid));
break;
case IEEE80211_AMPDU_RX_STOP:
@@ -378,7 +378,7 @@ int mt76x02_ampdu_action(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
ieee80211_send_bar(vif, sta->addr, tid, mtxq->agg_ssn);
break;
case IEEE80211_AMPDU_TX_START:
- mtxq->agg_ssn = IEEE80211_SN_TO_SEQ(*ssn);
+ mtxq->agg_ssn = IEEE80211_SN_TO_SEQ(ssn);
ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid);
break;
case IEEE80211_AMPDU_TX_STOP_CONT:
--
2.20.1
[PATCH 3/3] mt76: mt7615: use params->ssn value directly
There is no point to use pointer to params->ssn.
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/mediatek/mt76/mt7615/main.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7615/main.c
b/drivers/net/wireless/mediatek/mt76/mt7615/main.c
index 3f5f355d1f9b..2c702b31d55f 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7615/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7615/main.c
@@ -443,7 +443,7 @@ mt7615_ampdu_action(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
struct ieee80211_txq *txq = sta->txq[params->tid];
struct mt7615_sta *msta = (struct mt7615_sta *)sta->drv_priv;
u16 tid = params->tid;
- u16 *ssn = ¶ms->ssn;
+ u16 ssn = params->ssn;
struct mt76_txq *mtxq;
if (!txq)
@@ -453,7 +453,7 @@ mt7615_ampdu_action(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
switch (action) {
case IEEE80211_AMPDU_RX_START:
- mt76_rx_aggr_start(&dev->mt76, &msta->wcid, tid, *ssn,
+ mt76_rx_aggr_start(&dev->mt76, &msta->wcid, tid, ssn,
params->buf_size);
mt7615_mcu_set_rx_ba(dev, params, 1);
break;
@@ -473,7 +473,7 @@ mt7615_ampdu_action(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
mt7615_mcu_set_tx_ba(dev, params, 0);
break;
case IEEE80211_AMPDU_TX_START:
- mtxq->agg_ssn = IEEE80211_SN_TO_SEQ(*ssn);
+ mtxq->agg_ssn = IEEE80211_SN_TO_SEQ(ssn);
ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid);
break;
case IEEE80211_AMPDU_TX_STOP_CONT:
--
2.20.1
[PATCH 2/3] mt76: mt7603: use params->ssn value directly
There is no point to use pointer to params->ssn.
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/mediatek/mt76/mt7603/main.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt7603/main.c
b/drivers/net/wireless/mediatek/mt76/mt7603/main.c
index e5d4cb6381a8..d70f42dac923 100644
--- a/drivers/net/wireless/mediatek/mt76/mt7603/main.c
+++ b/drivers/net/wireless/mediatek/mt76/mt7603/main.c
@@ -569,7 +569,7 @@ mt7603_ampdu_action(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
struct ieee80211_txq *txq = sta->txq[params->tid];
struct mt7603_sta *msta = (struct mt7603_sta *)sta->drv_priv;
u16 tid = params->tid;
- u16 *ssn = ¶ms->ssn;
+ u16 ssn = params->ssn;
u8 ba_size = params->buf_size;
struct mt76_txq *mtxq;
@@ -580,7 +580,7 @@ mt7603_ampdu_action(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
switch (action) {
case IEEE80211_AMPDU_RX_START:
- mt76_rx_aggr_start(&dev->mt76, &msta->wcid, tid, *ssn,
+ mt76_rx_aggr_start(&dev->mt76, &msta->wcid, tid, ssn,
params->buf_size);
mt7603_mac_rx_ba_reset(dev, sta->addr, tid);
break;
@@ -599,7 +599,7 @@ mt7603_ampdu_action(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
mt7603_mac_tx_ba_reset(dev, msta->wcid.idx, tid, -1);
break;
case IEEE80211_AMPDU_TX_START:
- mtxq->agg_ssn = IEEE80211_SN_TO_SEQ(*ssn);
+ mtxq->agg_ssn = IEEE80211_SN_TO_SEQ(ssn);
ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid);
break;
case IEEE80211_AMPDU_TX_STOP_CONT:
--
2.20.1
[RFC/RFT v2] rt2x00: do not set IEEE80211_TX_STAT_AMPDU_NO_BACK on tx status
According to documentation IEEE80211_TX_STAT_AMPDU_NO_BACK is suppose
to be used when we do not receive BA (BlockAck). However on rt2x00 we
use it when remote station fail to decode one or more subframes within
AMPDU (some bits are not set in BlockAck bitmap). Setting the flag result
in sent of BAR (BlockAck Request) frame and this might result of abuse
of BA session, since remote station can sent BA with incorrect
sequence numbers after receiving BAR. This problem is visible especially
when connecting two rt2800 devices.
Previously I observed some performance benefits when using the flag
when connecting with iwlwifi devices. But currently possibly due
to recent changes in rt2x00 removing the flag has no effect on
those test cases.
So remove the IEEE80211_TX_STAT_AMPDU_NO_BACK.
Additionally partially mimic mt76 behaviour: send BAR when
starting/stopping BA session to workaround problems with some buggy
clients. Do not sent BAR on PS wakeup since we lack all PS handling
code what mt76 has.
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 4
drivers/net/wireless/ralink/rt2x00/rt2x00.h| 1 +
drivers/net/wireless/ralink/rt2x00/rt2x00dev.c | 3 ---
drivers/net/wireless/ralink/rt2x00/rt2x00mac.c | 18 ++
4 files changed, 23 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index c9b957ac5733..4a996550288e 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -10481,14 +10481,18 @@ int rt2800_ampdu_action(struct ieee80211_hw *hw,
struct ieee80211_vif *vif,
*/
break;
case IEEE80211_AMPDU_TX_START:
+ sta_priv->agg_ssn[tid] = IEEE80211_SN_TO_SEQ(params->ssn);
ieee80211_start_tx_ba_cb_irqsafe(vif, sta->addr, tid);
break;
case IEEE80211_AMPDU_TX_STOP_CONT:
case IEEE80211_AMPDU_TX_STOP_FLUSH:
+ ieee80211_send_bar(vif, sta->addr, tid, sta_priv->agg_ssn[tid]);
+ break;
case IEEE80211_AMPDU_TX_STOP_FLUSH_CONT:
ieee80211_stop_tx_ba_cb_irqsafe(vif, sta->addr, tid);
break;
case IEEE80211_AMPDU_TX_OPERATIONAL:
+ ieee80211_send_bar(vif, sta->addr, tid, sta_priv->agg_ssn[tid]);
break;
default:
rt2x00_warn((struct rt2x00_dev *)hw->priv,
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00.h
b/drivers/net/wireless/ralink/rt2x00/rt2x00.h
index 7e43690a861c..d35ef06c5c7a 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00.h
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00.h
@@ -500,6 +500,7 @@ struct rt2x00intf_conf {
*/
struct rt2x00_sta {
int wcid;
+ u16 agg_ssn[IEEE80211_NUM_TIDS];
};
static inline struct rt2x00_sta* sta_to_rt2x00_sta(struct ieee80211_sta *sta)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
index 35414f97a978..ad063c920323 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
@@ -371,9 +371,6 @@ static void rt2x00lib_fill_tx_status(struct rt2x00_dev
*rt2x00dev,
IEEE80211_TX_CTL_AMPDU;
tx_info->status.ampdu_len = 1;
tx_info->status.ampdu_ack_len = success ? 1 : 0;
-
- if (!success)
- tx_info->flags |= IEEE80211_TX_STAT_AMPDU_NO_BACK;
}
if (rate_flags & IEEE80211_TX_RC_USE_RTS_CTS) {
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
index beb20c5faf5f..14896d37f0cc 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00mac.c
@@ -86,6 +86,21 @@ static int rt2x00mac_tx_rts_cts(struct rt2x00_dev *rt2x00dev,
return retval;
}
+static void rt2x00mac_save_agg_seqno(struct sk_buff *skb,
+struct ieee80211_sta *sta)
+{
+ struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
+ struct rt2x00_sta *sta_priv = sta_to_rt2x00_sta(sta);
+ u8 tid;
+
+ if (!ieee80211_is_data_qos(hdr->frame_control) ||
+ !ieee80211_is_data_present(hdr->frame_control))
+ return;
+
+ tid = skb->priority & IEEE80211_QOS_CTL_TID_MASK;
+ sta_priv->agg_ssn[tid] = le16_to_cpu(hdr->seq_ctrl) + 0x10;
+}
+
void rt2x00mac_tx(struct ieee80211_hw *hw,
struct ieee80211_tx_control *control,
struct sk_buff *skb)
@@ -119,6 +134,9 @@ void rt2x00mac_tx(struct ieee80211_hw *hw,
goto exit_free_skb;
}
+ if (control->sta)
+ rt2x00mac_save_agg_seqno(skb, control->sta);
+
/*
Re: [RFC/RFT] rt2x00: do not set IEEE80211_TX_STAT_AMPDU_NO_BACK on tx status
On Thu, Jul 04, 2019 at 01:06:52PM +0200, Stanislaw Gruszka wrote: > According to documentation IEEE80211_TX_STAT_AMPDU_NO_BACK is suppose > to be used when we do not recive BA (BlockAck). However on rt2x00 we > use it when remote station fail to decode one or more subframes within > AMPDU (some bits are not set in BlockAck bitmap). Setting the flag result > in sent of BAR (BlockAck Request) frame and this might result of abuse > of BA session, since remote station can sent BA with incorrect > sequence numbers after receiving BAR. This problem is visible especially > when connecting two rt2800 devices. > > Previously I observed some performance benefits when using the flag > when connecting with iwlwifi devices. But currently possibly due > to reacent changes in rt2x00 removing the flag has no effect on > those test cases. > > So remove the IEEE80211_TX_STAT_AMPDU_NO_BACK. > > Perhaps we should send BAR exlicitly on BA session start/stop > and when remote STA went to PowerSave mode (for AP) like mt76 does. > But I do not understand for what this is needed. This commit https://github.com/openwrt/mt76/commit/3e447e7797d64dbf4dc1dd5553d08be0d8150d7e explained that sending BAR on stop aggregation is workaround for buggy clients. I can implement that on rt2x00. But I will skip sending BAR on remote station PS wakeup, since we do not implement all PS related code in rt2x00. Stanislaw
[PATCH v2 1/2] mt76: usb: fix endian in mt76u_copy
In contrast to mt76_wr() which we use to program registers,
on mt76_wr_copy() we should not change endian of the data.
Fixes: b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c
b/drivers/net/wireless/mediatek/mt76/usb.c
index 5be584ca7497..828939e78a3b 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -165,11 +165,11 @@ static void mt76u_copy(struct mt76_dev *dev, u32 offset,
mutex_lock(&usb->usb_ctrl_mtx);
for (i = 0; i < DIV_ROUND_UP(len, 4); i++) {
- put_unaligned_le32(val[i], usb->data);
+ put_unaligned(val[i], (u32 *) usb->data);
ret = __mt76u_vendor_request(dev, MT_VEND_MULTI_WRITE,
USB_DIR_OUT | USB_TYPE_VENDOR,
0, offset + i * 4, usb->data,
-sizeof(__le32));
+sizeof(u32));
if (ret < 0)
break;
}
--
2.20.1
[PATCH v2 0/2] mt76: usb: alignment and endianes improvements
Fix endian bug and do some minor optimizations in mt76u_{copy,rr,wr} .
v1 -> v2:
- drop patch 3
- fix pointer size in patch 1
Stanislaw Gruszka (2):
mt76: usb: fix endian in mt76u_copy
mt76: usb: remove unneeded {put,get}_unaligned
drivers/net/wireless/mediatek/mt76/mt76.h | 5 -
drivers/net/wireless/mediatek/mt76/usb.c | 12 ++--
2 files changed, 10 insertions(+), 7 deletions(-)
--
2.20.1
[PATCH v2 2/2] mt76: usb: remove unneeded {put,get}_unaligned
Compiler give us guaranties on variables alignment, so use
an variable as buffer when read/write registers and remove
unneeded {put,get}_unaligned.
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/mediatek/mt76/mt76.h | 5 -
drivers/net/wireless/mediatek/mt76/usb.c | 8
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h
b/drivers/net/wireless/mediatek/mt76/mt76.h
index 56bf93a8988e..094e6e543542 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -389,7 +389,10 @@ enum mt76u_out_ep {
#define MCU_RESP_URB_SIZE 1024
struct mt76_usb {
struct mutex usb_ctrl_mtx;
- u8 data[32];
+ union {
+ u8 data[32];
+ __le32 reg_val;
+ };
struct tasklet_struct rx_tasklet;
struct delayed_work stat_work;
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c
b/drivers/net/wireless/mediatek/mt76/usb.c
index 828939e78a3b..00069c2536f8 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -95,9 +95,9 @@ static u32 __mt76u_rr(struct mt76_dev *dev, u32 addr)
ret = __mt76u_vendor_request(dev, req,
USB_DIR_IN | USB_TYPE_VENDOR,
-0, offset, usb->data, sizeof(__le32));
+0, offset, &usb->reg_val, sizeof(__le32));
if (ret == sizeof(__le32))
- data = get_unaligned_le32(usb->data);
+ data = le32_to_cpu(usb->reg_val);
trace_usb_reg_rr(dev, addr, data);
return data;
@@ -131,10 +131,10 @@ static void __mt76u_wr(struct mt76_dev *dev, u32 addr,
u32 val)
}
offset = addr & ~MT_VEND_TYPE_MASK;
- put_unaligned_le32(val, usb->data);
+ usb->reg_val = cpu_to_le32(val);
__mt76u_vendor_request(dev, req,
USB_DIR_OUT | USB_TYPE_VENDOR, 0,
- offset, usb->data, sizeof(__le32));
+ offset, &usb->reg_val, sizeof(__le32));
trace_usb_reg_wr(dev, addr, val);
}
--
2.20.1
Re: [PATCH 3/3] mt76: usb: use full intermediate buffer in mt76u_copy
On Tue, Jul 02, 2019 at 05:06:01PM +0200, Stanislaw Gruszka wrote: > Instead of use several 4 bytes usb requests, use full 32 bytes buffer > to copy data to device. With the change we use less requests and copy > exact data size to the device regardless size is multiple of 4 or not. And this does not work correctly on some usb hosts, request which are not multiple of 4 do not ended being written to hardware, what results in original problem of having last part of beacon corrupted. I would prefer to drop this set - and I would post 2 patches (first patch fixed and third dropped). But since this is now in Felix's wireless tree I guess I need to post fixes on top of this set? Stanislaw
Re: [PATCH 1/3] mt76: usb: fix endian in mt76u_copy
On Tue, Jul 02, 2019 at 05:05:59PM +0200, Stanislaw Gruszka wrote:
> In contrast to mt76_wr() which we use to program registers,
> on mt76_wr_copy() we should not change endian of the data.
>
> Fixes: b40b15e1521f ("mt76: add usb support to mt76 layer")
> Signed-off-by: Stanislaw Gruszka
> ---
> drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/usb.c
> b/drivers/net/wireless/mediatek/mt76/usb.c
> index 87ecbe290f99..db90ec6b8775 100644
> --- a/drivers/net/wireless/mediatek/mt76/usb.c
> +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> @@ -165,11 +165,11 @@ static void mt76u_copy(struct mt76_dev *dev, u32 offset,
>
> mutex_lock(&usb->usb_ctrl_mtx);
> for (i = 0; i < DIV_ROUND_UP(len, 4); i++) {
> - put_unaligned_le32(val[i], usb->data);
> + put_unaligned(val[i], usb->data);
This is bug as put_unaligned() use size based second argument
pointer type, correct version should looks like this:
put_unaligned(val[i], (u32 *) usb->data);
Stanislaw
[RFC/RFT] rt2x00: do not set IEEE80211_TX_STAT_AMPDU_NO_BACK on tx status
According to documentation IEEE80211_TX_STAT_AMPDU_NO_BACK is suppose
to be used when we do not recive BA (BlockAck). However on rt2x00 we
use it when remote station fail to decode one or more subframes within
AMPDU (some bits are not set in BlockAck bitmap). Setting the flag result
in sent of BAR (BlockAck Request) frame and this might result of abuse
of BA session, since remote station can sent BA with incorrect
sequence numbers after receiving BAR. This problem is visible especially
when connecting two rt2800 devices.
Previously I observed some performance benefits when using the flag
when connecting with iwlwifi devices. But currently possibly due
to reacent changes in rt2x00 removing the flag has no effect on
those test cases.
So remove the IEEE80211_TX_STAT_AMPDU_NO_BACK.
Perhaps we should send BAR exlicitly on BA session start/stop
and when remote STA went to PowerSave mode (for AP) like mt76 does.
But I do not understand for what this is needed.
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/ralink/rt2x00/rt2x00dev.c | 3 ---
1 file changed, 3 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
index a6c374c483c2..c547bec044a8 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00dev.c
@@ -371,9 +371,6 @@ static void rt2x00lib_fill_tx_status(struct rt2x00_dev
*rt2x00dev,
IEEE80211_TX_CTL_AMPDU;
tx_info->status.ampdu_len = 1;
tx_info->status.ampdu_ack_len = success ? 1 : 0;
-
- if (!success)
- tx_info->flags |= IEEE80211_TX_STAT_AMPDU_NO_BACK;
}
if (rate_flags & IEEE80211_TX_RC_USE_RTS_CTS) {
--
2.20.1
Re: [PATCH v2] rt2x00: no need to check return value of debugfs_create functions
On Wed, Jul 03, 2019 at 01:39:56PM +0200, Greg Kroah-Hartman wrote: > > When calling debugfs functions, there is no need to ever check the > return value. The function can work or not, but the code logic should > never do something different based on this. > > Because we don't need to save the individual debugfs files and > directories, remove the local storage of them and just remove the entire > debugfs directory in a single call, making things a lot simpler. > > Cc: Stanislaw Gruszka > Cc: Helmut Schaa > Cc: Kalle Valo > Cc: "David S. Miller" > Cc: linux-wireless@vger.kernel.org > Cc: net...@vger.kernel.org > Signed-off-by: Greg Kroah-Hartman Acked-by: Stanislaw Gruszka
Re: [PATCH] rt2x00: no need to check return value of debugfs_create functions
On Wed, Jul 03, 2019 at 08:56:31AM +0200, Greg Kroah-Hartman wrote: > When calling debugfs functions, there is no need to ever check the > return value. The function can work or not, but the code logic should > never do something different based on this. > > Because we don't need to save the individual debugfs files and > directories, remove the local storage of them and just remove the entire > debugfs directory in a single call, making things a lot simpler. > > Cc: Stanislaw Gruszka > Cc: Helmut Schaa > Cc: Kalle Valo > Cc: "David S. Miller" > Cc: linux-wireless@vger.kernel.org > Cc: net...@vger.kernel.org > Signed-off-by: Greg Kroah-Hartman > --- > .../net/wireless/ralink/rt2x00/rt2x00debug.c | 100 -- > 1 file changed, 23 insertions(+), 77 deletions(-) This patch will not apply on wireless-drivers-next due to my recent change which add new debugfs file: commit e7f15d58dfe43f18199251f430d7713b0b8fad34 Author: Stanislaw Gruszka Date: Thu May 2 11:07:00 2019 +0200 rt2x00: add restart hw Could you please rebase the patch ? (I can do this as well if you want me to). Stanislaw
Re: rtl8xxxu do not see/connect any wifi network until suspend/resume
On Mon, Jul 01, 2019 at 07:25:16PM +0500, Andrej Surkov wrote: > Hi! > > Do you aware of rtl8xxxu do not work until suspend/resume issue? > > On my laptop, see below for details, after reboot wifi-menu tool shows one > single wifi network found and can't connect to it, but after 'sudo systemctl > suspend' and resume by keystroke wifi-menu and connections work well. This sound like this bug: https://bugzilla.kernel.org/show_bug.cgi?id=202541 Which happen to be xhci_hcd regression, not yet fixed AFAIK. Stanislaw
[PATCH 3/3] mt76: usb: use full intermediate buffer in mt76u_copy
Instead of use several 4 bytes usb requests, use full 32 bytes buffer
to copy data to device. With the change we use less requests and copy
exact data size to the device regardless size is multiple of 4 or not.
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/mediatek/mt76/usb.c | 19 ---
1 file changed, 12 insertions(+), 7 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c
b/drivers/net/wireless/mediatek/mt76/usb.c
index a61bb8171557..7c564cc68c7c 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -160,19 +160,24 @@ static void mt76u_copy(struct mt76_dev *dev, u32 offset,
const void *data, int len)
{
struct mt76_usb *usb = &dev->usb;
- const u32 *val = data;
- int i, ret;
+ int ret, req_len;
mutex_lock(&usb->usb_ctrl_mtx);
- for (i = 0; i < DIV_ROUND_UP(len, 4); i++) {
- put_unaligned(val[i], usb->data);
+ do {
+ req_len = min_t(int, len, sizeof(usb->data));
+
+ memcpy(usb->data, data, req_len);
+
ret = __mt76u_vendor_request(dev, MT_VEND_MULTI_WRITE,
USB_DIR_OUT | USB_TYPE_VENDOR,
-0, offset + i * 4, usb->data,
-sizeof(u32));
+0, offset, usb->data, req_len);
if (ret < 0)
break;
- }
+
+ data += req_len;
+ offset += req_len;
+ len -= req_len;
+ } while (len > 0);
mutex_unlock(&usb->usb_ctrl_mtx);
}
--
1.9.3
[PATCH 1/3] mt76: usb: fix endian in mt76u_copy
In contrast to mt76_wr() which we use to program registers,
on mt76_wr_copy() we should not change endian of the data.
Fixes: b40b15e1521f ("mt76: add usb support to mt76 layer")
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/mediatek/mt76/usb.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c
b/drivers/net/wireless/mediatek/mt76/usb.c
index 87ecbe290f99..db90ec6b8775 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -165,11 +165,11 @@ static void mt76u_copy(struct mt76_dev *dev, u32 offset,
mutex_lock(&usb->usb_ctrl_mtx);
for (i = 0; i < DIV_ROUND_UP(len, 4); i++) {
- put_unaligned_le32(val[i], usb->data);
+ put_unaligned(val[i], usb->data);
ret = __mt76u_vendor_request(dev, MT_VEND_MULTI_WRITE,
USB_DIR_OUT | USB_TYPE_VENDOR,
0, offset + i * 4, usb->data,
-sizeof(__le32));
+sizeof(u32));
if (ret < 0)
break;
}
--
1.9.3
[PATCH 0/3] mt76: usb: alignment and endianes improvements
Fix endian bug and do some minor optimizations in mt76u_{copy,rr,wr} .
I do not mark cc stable endian fix as noboby reported this issue,
i.e. use the driver on big endian machine, but make it a separate patch,
so it can be backported if needed.
This is on top of:
[PATCH] mt76: round up length on mt76_wr_copy
Stanislaw Gruszka (3):
mt76: usb: fix endian in mt76u_copy
mt76: usb: remove unneeded {put,get}_unaligned
mt76: usb: use full intermediate buffer in mt76u_copy
drivers/net/wireless/mediatek/mt76/mt76.h | 5 -
drivers/net/wireless/mediatek/mt76/usb.c | 27 ---
2 files changed, 20 insertions(+), 12 deletions(-)
--
1.9.3
[PATCH 2/3] mt76: usb: remove unneeded {put,get}_unaligned
Compiler give us guaranties on variables alignment, so use
an variable as buffer when read/write registers and remove
unneeded {put,get}_unaligned.
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/mediatek/mt76/mt76.h | 5 -
drivers/net/wireless/mediatek/mt76/usb.c | 8
2 files changed, 8 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h
b/drivers/net/wireless/mediatek/mt76/mt76.h
index 56bf93a8988e..094e6e543542 100644
--- a/drivers/net/wireless/mediatek/mt76/mt76.h
+++ b/drivers/net/wireless/mediatek/mt76/mt76.h
@@ -389,7 +389,10 @@ enum mt76u_out_ep {
#define MCU_RESP_URB_SIZE 1024
struct mt76_usb {
struct mutex usb_ctrl_mtx;
- u8 data[32];
+ union {
+ u8 data[32];
+ __le32 reg_val;
+ };
struct tasklet_struct rx_tasklet;
struct delayed_work stat_work;
diff --git a/drivers/net/wireless/mediatek/mt76/usb.c
b/drivers/net/wireless/mediatek/mt76/usb.c
index db90ec6b8775..a61bb8171557 100644
--- a/drivers/net/wireless/mediatek/mt76/usb.c
+++ b/drivers/net/wireless/mediatek/mt76/usb.c
@@ -95,9 +95,9 @@ static u32 __mt76u_rr(struct mt76_dev *dev, u32 addr)
ret = __mt76u_vendor_request(dev, req,
USB_DIR_IN | USB_TYPE_VENDOR,
-0, offset, usb->data, sizeof(__le32));
+0, offset, &usb->reg_val, sizeof(__le32));
if (ret == sizeof(__le32))
- data = get_unaligned_le32(usb->data);
+ data = le32_to_cpu(usb->reg_val);
trace_usb_reg_rr(dev, addr, data);
return data;
@@ -131,10 +131,10 @@ static void __mt76u_wr(struct mt76_dev *dev, u32 addr,
u32 val)
}
offset = addr & ~MT_VEND_TYPE_MASK;
- put_unaligned_le32(val, usb->data);
+ usb->reg_val = cpu_to_le32(val);
__mt76u_vendor_request(dev, req,
USB_DIR_OUT | USB_TYPE_VENDOR, 0,
- offset, usb->data, sizeof(__le32));
+ offset, &usb->reg_val, sizeof(__le32));
trace_usb_reg_wr(dev, addr, val);
}
--
1.9.3
[PATCH v2 6/7] rt2x00: add restart hw
Add ieee80211_restart_hw() to watchdog and debugfs file for testing
if restart works as expected.
Signed-off-by: Stanislaw Gruszka
---
.../net/wireless/ralink/rt2x00/rt2800lib.c| 4 +++
drivers/net/wireless/ralink/rt2x00/rt2x00.h | 7
.../net/wireless/ralink/rt2x00/rt2x00debug.c | 35 +++
.../net/wireless/ralink/rt2x00/rt2x00dev.c| 10 --
4 files changed, 54 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index 32a4b84e6e05..0fb519f2b83f 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -1263,6 +1263,9 @@ void rt2800_watchdog(struct rt2x00_dev *rt2x00dev)
if (hung_rx)
rt2x00_warn(rt2x00dev, "Watchdog RX hung detected\n");
+
+ if (hung_tx || hung_rx)
+ ieee80211_restart_hw(rt2x00dev->hw);
}
EXPORT_SYMBOL_GPL(rt2800_watchdog);
@@ -10283,6 +10286,7 @@ int rt2800_probe_hw(struct rt2x00_dev *rt2x00dev)
__set_bit(REQUIRE_TASKLET_CONTEXT, &rt2x00dev->cap_flags);
}
+ __set_bit(CAPABILITY_RESTART_HW, &rt2x00dev->cap_flags);
rt2x00dev->link.watchdog_interval = msecs_to_jiffies(100);
/*
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00.h
b/drivers/net/wireless/ralink/rt2x00/rt2x00.h
index dc6b79e4be3b..7c7cced009bd 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00.h
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00.h
@@ -712,6 +712,7 @@ enum rt2x00_capability_flags {
CAPABILITY_VCO_RECALIBRATION,
CAPABILITY_EXTERNAL_PA_TX0,
CAPABILITY_EXTERNAL_PA_TX1,
+ CAPABILITY_RESTART_HW,
};
/*
@@ -1268,6 +1269,12 @@ rt2x00_has_cap_vco_recalibration(struct rt2x00_dev
*rt2x00dev)
return rt2x00_has_cap_flag(rt2x00dev, CAPABILITY_VCO_RECALIBRATION);
}
+static inline bool
+rt2x00_has_cap_restart_hw(struct rt2x00_dev *rt2x00dev)
+{
+ return rt2x00_has_cap_flag(rt2x00dev, CAPABILITY_RESTART_HW);
+}
+
/**
* rt2x00queue_map_txskb - Map a skb into DMA for TX purposes.
* @entry: Pointer to &struct queue_entry
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
index aac3aae7afaa..ef5f51512212 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00debug.c
@@ -52,6 +52,7 @@ struct rt2x00debug_intf {
* - chipset file
* - device state flags file
* - device capability flags file
+* - hardware restart file
* - register folder
* - csr offset/value files
* - eeprom offset/value files
@@ -68,6 +69,7 @@ struct rt2x00debug_intf {
struct dentry *chipset_entry;
struct dentry *dev_flags;
struct dentry *cap_flags;
+ struct dentry *restart_hw;
struct dentry *register_folder;
struct dentry *csr_off_entry;
struct dentry *csr_val_entry;
@@ -566,6 +568,34 @@ static const struct file_operations
rt2x00debug_fop_cap_flags = {
.llseek = default_llseek,
};
+static ssize_t rt2x00debug_write_restart_hw(struct file *file,
+ const char __user *buf,
+ size_t length,
+ loff_t *offset)
+{
+ struct rt2x00debug_intf *intf = file->private_data;
+ struct rt2x00_dev *rt2x00dev = intf->rt2x00dev;
+ static unsigned long last_reset;
+
+ if (!rt2x00_has_cap_restart_hw(rt2x00dev))
+ return -EOPNOTSUPP;
+
+ if (time_before(jiffies, last_reset + msecs_to_jiffies(2000)))
+ return -EBUSY;
+
+ last_reset = jiffies;
+
+ ieee80211_restart_hw(rt2x00dev->hw);
+ return length;
+}
+
+static const struct file_operations rt2x00debug_restart_hw = {
+ .owner = THIS_MODULE,
+ .write = rt2x00debug_write_restart_hw,
+ .open = simple_open,
+ .llseek = generic_file_llseek,
+};
+
static struct dentry *rt2x00debug_create_file_driver(const char *name,
struct rt2x00debug_intf
*intf,
@@ -661,6 +691,10 @@ void rt2x00debug_register(struct rt2x00_dev *rt2x00dev)
intf->driver_folder, intf,
&rt2x00debug_fop_cap_flags);
+ intf->restart_hw = debugfs_create_file("restart_hw", 0200,
+ intf->driver_folder, intf,
+ &rt2x00debug_restart_hw);
+
intf->register_folder =
debugfs_create_dir("register", intf->driver_folder);
@@ -742,6 +776,7 @@ void rt2x00deb
[PATCH v2 7/7] rt2800: do not enable watchdog by default
Make watchdog disabled by default and add module parameter to enable it.
User will have to create file in /etc/modprobe.d/ with
options rt2800lib watchdog=1
to enable the watchdog or load "rt2800lib watchdog=1" module manually
before loading rt2800{soc,pci,usb} module.
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 12 ++--
drivers/net/wireless/ralink/rt2x00/rt2x00.h | 1 +
drivers/net/wireless/ralink/rt2x00/rt2x00link.c | 2 +-
3 files changed, 12 insertions(+), 3 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index 0fb519f2b83f..c9b957ac5733 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -30,6 +30,10 @@
#include "rt2800lib.h"
#include "rt2800.h"
+static bool modparam_watchdog;
+module_param_named(watchdog, modparam_watchdog, bool, S_IRUGO);
+MODULE_PARM_DESC(watchdog, "Enable watchdog to detect tx/rx hangs and reset
hardware if detected");
+
/*
* Register access.
* All access to the CSR registers will go through the methods
@@ -10286,8 +10290,12 @@ int rt2800_probe_hw(struct rt2x00_dev *rt2x00dev)
__set_bit(REQUIRE_TASKLET_CONTEXT, &rt2x00dev->cap_flags);
}
- __set_bit(CAPABILITY_RESTART_HW, &rt2x00dev->cap_flags);
- rt2x00dev->link.watchdog_interval = msecs_to_jiffies(100);
+ if (modparam_watchdog) {
+ __set_bit(CAPABILITY_RESTART_HW, &rt2x00dev->cap_flags);
+ rt2x00dev->link.watchdog_interval = msecs_to_jiffies(100);
+ } else {
+ rt2x00dev->link.watchdog_disabled = true;
+ }
/*
* Set the rssi offset.
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00.h
b/drivers/net/wireless/ralink/rt2x00/rt2x00.h
index 7c7cced009bd..7e43690a861c 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00.h
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00.h
@@ -326,6 +326,7 @@ struct link {
*/
struct delayed_work watchdog_work;
unsigned int watchdog_interval;
+ bool watchdog_disabled;
/*
* Work structure for scheduling periodic AGC adjustments.
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00link.c
b/drivers/net/wireless/ralink/rt2x00/rt2x00link.c
index 15ebebf88e72..b052c96347d6 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00link.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00link.c
@@ -384,7 +384,7 @@ void rt2x00link_start_watchdog(struct rt2x00_dev *rt2x00dev)
struct link *link = &rt2x00dev->link;
if (test_bit(DEVICE_STATE_PRESENT, &rt2x00dev->flags) &&
- rt2x00dev->ops->lib->watchdog)
+ rt2x00dev->ops->lib->watchdog && !link->watchdog_disabled)
ieee80211_queue_delayed_work(rt2x00dev->hw,
&link->watchdog_work,
link->watchdog_interval);
--
2.20.1
[PATCH v2 5/7] rt2800: do not nullify initialization vector data
If we restart hw we should keep existing IV (initialization vector)
otherwise HW encryption will be broken after restart.
Also fix some coding style issues on the way.
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/ralink/rt2x00/rt2800lib.c | 9 -
1 file changed, 4 insertions(+), 5 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index 499e9afa0026..32a4b84e6e05 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -1647,14 +1647,15 @@ static void rt2800_config_wcid_attr_cipher(struct
rt2x00_dev *rt2x00dev,
offset = MAC_IVEIV_ENTRY(key->hw_key_idx);
- memset(&iveiv_entry, 0, sizeof(iveiv_entry));
+ rt2800_register_multiread(rt2x00dev, offset,
+ &iveiv_entry, sizeof(iveiv_entry));
if ((crypto->cipher == CIPHER_TKIP) ||
(crypto->cipher == CIPHER_TKIP_NO_MIC) ||
(crypto->cipher == CIPHER_AES))
iveiv_entry.iv[3] |= 0x20;
iveiv_entry.iv[3] |= key->keyidx << 6;
rt2800_register_multiwrite(rt2x00dev, offset,
- &iveiv_entry, sizeof(iveiv_entry));
+ &iveiv_entry, sizeof(iveiv_entry));
}
int rt2800_config_shared_key(struct rt2x00_dev *rt2x00dev,
@@ -6079,13 +6080,11 @@ static int rt2800_init_registers(struct rt2x00_dev
*rt2x00dev)
* ASIC will keep garbage value after boot, clear encryption keys.
*/
for (i = 0; i < 4; i++)
- rt2800_register_write(rt2x00dev,
-SHARED_KEY_MODE_ENTRY(i), 0);
+ rt2800_register_write(rt2x00dev, SHARED_KEY_MODE_ENTRY(i), 0);
for (i = 0; i < 256; i++) {
rt2800_config_wcid(rt2x00dev, NULL, i);
rt2800_delete_wcid_attr(rt2x00dev, i);
- rt2800_register_write(rt2x00dev, MAC_IVEIV_ENTRY(i), 0);
}
/*
--
2.20.1
[PATCH v2 2/7] rt2800: add helpers for reading dma done index
For mmio we do not properlly trace dma done Q_INDEX_DMA_DONE index
for TX queues. That would require implementing INT_SOURCE_CSR_*_DMA_DONE
interrupts, what is rather not worth to do due to adding extra
CPU load (small but still somewhat not necessary otherwise).
We can just read TX DMA done indexes from registers directly. What
will be used by watchdog.
Signed-off-by: Stanislaw Gruszka
---
.../net/wireless/ralink/rt2x00/rt2800lib.h| 8 +
.../net/wireless/ralink/rt2x00/rt2800mmio.c | 31 +++
.../net/wireless/ralink/rt2x00/rt2800mmio.h | 2 ++
.../net/wireless/ralink/rt2x00/rt2800pci.c| 1 +
.../net/wireless/ralink/rt2x00/rt2800soc.c| 1 +
.../net/wireless/ralink/rt2x00/rt2800usb.c| 9 ++
6 files changed, 52 insertions(+)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.h
b/drivers/net/wireless/ralink/rt2x00/rt2800lib.h
index 48adc6cc3233..dbb413ec8b08 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.h
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.h
@@ -65,6 +65,7 @@ struct rt2800_ops {
const u8 *data, const size_t len);
int (*drv_init_registers)(struct rt2x00_dev *rt2x00dev);
__le32 *(*drv_get_txwi)(struct queue_entry *entry);
+ unsigned int (*drv_get_dma_done)(struct data_queue *queue);
};
static inline u32 rt2800_register_read(struct rt2x00_dev *rt2x00dev,
@@ -166,6 +167,13 @@ static inline __le32 *rt2800_drv_get_txwi(struct
queue_entry *entry)
return rt2800ops->drv_get_txwi(entry);
}
+static inline unsigned int rt2800_drv_get_dma_done(struct data_queue *queue)
+{
+ const struct rt2800_ops *rt2800ops = queue->rt2x00dev->ops->drv;
+
+ return rt2800ops->drv_get_dma_done(queue);
+}
+
void rt2800_mcu_request(struct rt2x00_dev *rt2x00dev,
const u8 command, const u8 token,
const u8 arg0, const u8 arg1);
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c
b/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c
index d1de8e2ff690..110bb391c372 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800mmio.c
@@ -24,6 +24,37 @@
#include "rt2800lib.h"
#include "rt2800mmio.h"
+unsigned int rt2800mmio_get_dma_done(struct data_queue *queue)
+{
+ struct rt2x00_dev *rt2x00dev = queue->rt2x00dev;
+ struct queue_entry *entry;
+ int idx, qid;
+
+ switch (queue->qid) {
+ case QID_AC_VO:
+ case QID_AC_VI:
+ case QID_AC_BE:
+ case QID_AC_BK:
+ qid = queue->qid;
+ idx = rt2x00mmio_register_read(rt2x00dev, TX_DTX_IDX(qid));
+ break;
+ case QID_MGMT:
+ idx = rt2x00mmio_register_read(rt2x00dev, TX_DTX_IDX(5));
+ break;
+ case QID_RX:
+ entry = rt2x00queue_get_entry(queue, Q_INDEX_DMA_DONE);
+ idx = entry->entry_idx;
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ idx = 0;
+ break;
+ }
+
+ return idx;
+}
+EXPORT_SYMBOL_GPL(rt2800mmio_get_dma_done);
+
/*
* TX descriptor initialization
*/
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800mmio.h
b/drivers/net/wireless/ralink/rt2x00/rt2800mmio.h
index 29b5cfd2856f..adcd9d54ac1c 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800mmio.h
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800mmio.h
@@ -114,6 +114,8 @@
#define RXD_W3_PLCP_SIGNAL FIELD32(0x0002)
#define RXD_W3_PLCP_RSSI FIELD32(0x0004)
+unsigned int rt2800mmio_get_dma_done(struct data_queue *queue);
+
/* TX descriptor initialization */
__le32 *rt2800mmio_get_txwi(struct queue_entry *entry);
void rt2800mmio_write_tx_desc(struct queue_entry *entry,
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800pci.c
b/drivers/net/wireless/ralink/rt2x00/rt2800pci.c
index ead8bd3e9236..71ef8f07b47b 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800pci.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800pci.c
@@ -326,6 +326,7 @@ static const struct rt2800_ops rt2800pci_rt2800_ops = {
.drv_write_firmware = rt2800pci_write_firmware,
.drv_init_registers = rt2800mmio_init_registers,
.drv_get_txwi = rt2800mmio_get_txwi,
+ .drv_get_dma_done = rt2800mmio_get_dma_done,
};
static const struct rt2x00lib_ops rt2800pci_rt2x00_ops = {
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800soc.c
b/drivers/net/wireless/ralink/rt2x00/rt2800soc.c
index 230557d36c52..34e9291d949c 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800soc.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800soc.c
@@ -171,6 +171,7 @@ static const struct rt2800_ops rt2800soc_rt2800_ops = {
.drv_write_firmware = rt2800soc_write_firmware,
.drv_init_registers = rt2800mmio_init_registers,
.drv_get_txwi =
[PATCH v2 1/7] rt2x00: allow to specify watchdog interval
Allow subdriver to change watchdog interval by intialize
link->watchdog_interval value before rt2x00link_register().
Signed-off-by: Stanislaw Gruszka
---
drivers/net/wireless/ralink/rt2x00/rt2x00.h | 1 +
drivers/net/wireless/ralink/rt2x00/rt2x00link.c | 13 +
2 files changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00.h
b/drivers/net/wireless/ralink/rt2x00/rt2x00.h
index 64a792a8fb2c..2b551dbe9844 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00.h
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00.h
@@ -325,6 +325,7 @@ struct link {
* to bring the device/driver back into the desired state.
*/
struct delayed_work watchdog_work;
+ unsigned int watchdog_interval;
/*
* Work structure for scheduling periodic AGC adjustments.
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00link.c
b/drivers/net/wireless/ralink/rt2x00/rt2x00link.c
index 939cfa5141c6..15ebebf88e72 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00link.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00link.c
@@ -387,7 +387,7 @@ void rt2x00link_start_watchdog(struct rt2x00_dev *rt2x00dev)
rt2x00dev->ops->lib->watchdog)
ieee80211_queue_delayed_work(rt2x00dev->hw,
&link->watchdog_work,
-WATCHDOG_INTERVAL);
+link->watchdog_interval);
}
void rt2x00link_stop_watchdog(struct rt2x00_dev *rt2x00dev)
@@ -413,11 +413,16 @@ static void rt2x00link_watchdog(struct work_struct *work)
if (test_bit(DEVICE_STATE_PRESENT, &rt2x00dev->flags))
ieee80211_queue_delayed_work(rt2x00dev->hw,
&link->watchdog_work,
-WATCHDOG_INTERVAL);
+link->watchdog_interval);
}
void rt2x00link_register(struct rt2x00_dev *rt2x00dev)
{
- INIT_DELAYED_WORK(&rt2x00dev->link.watchdog_work, rt2x00link_watchdog);
- INIT_DELAYED_WORK(&rt2x00dev->link.work, rt2x00link_tuner);
+ struct link *link = &rt2x00dev->link;
+
+ INIT_DELAYED_WORK(&link->work, rt2x00link_tuner);
+ INIT_DELAYED_WORK(&link->watchdog_work, rt2x00link_watchdog);
+
+ if (link->watchdog_interval == 0)
+ link->watchdog_interval = WATCHDOG_INTERVAL;
}
--
2.20.1
[PATCH v2 0/7] rt2800: add experimental watchdog implementation
Add watchdog to address some remaining problems of Ralink devices random hungs. RFC -> v1 - better description for module parameter - fix white space v1 -> v2: - rebase on latest tree. Stanislaw Gruszka (7): rt2x00: allow to specify watchdog interval rt2800: add helpers for reading dma done index rt2800: initial watchdog implementation rt2800: add pre_reset_hw callback rt2800: do not nullify initialization vector data rt2x00: add restart hw rt2800: do not enable watchdog by default .../net/wireless/ralink/rt2x00/rt2800lib.c| 96 ++- .../net/wireless/ralink/rt2x00/rt2800lib.h| 11 +++ .../net/wireless/ralink/rt2x00/rt2800mmio.c | 31 ++ .../net/wireless/ralink/rt2x00/rt2800mmio.h | 2 + .../net/wireless/ralink/rt2x00/rt2800pci.c| 3 + .../net/wireless/ralink/rt2x00/rt2800soc.c| 3 + .../net/wireless/ralink/rt2x00/rt2800usb.c| 11 +++ drivers/net/wireless/ralink/rt2x00/rt2x00.h | 10 ++ .../net/wireless/ralink/rt2x00/rt2x00debug.c | 35 +++ .../net/wireless/ralink/rt2x00/rt2x00dev.c| 10 +- .../net/wireless/ralink/rt2x00/rt2x00link.c | 15 ++- .../net/wireless/ralink/rt2x00/rt2x00queue.h | 6 ++ 12 files changed, 221 insertions(+), 12 deletions(-) -- 2.20.1
[PATCH v2 3/7] rt2800: initial watchdog implementation
Add watchdog for rt2800 devices. For now it only detect hung
and print error.
[Note: I verified that printing messages from process context is
fine on MT7620 (WT3020) platform that have problem when printk
is called from interrupt context].
Signed-off-by: Stanislaw Gruszka
---
.../net/wireless/ralink/rt2x00/rt2800lib.c| 56 +++
.../net/wireless/ralink/rt2x00/rt2800lib.h| 2 +
.../net/wireless/ralink/rt2x00/rt2800pci.c| 1 +
.../net/wireless/ralink/rt2x00/rt2800soc.c| 1 +
.../net/wireless/ralink/rt2x00/rt2800usb.c| 1 +
.../net/wireless/ralink/rt2x00/rt2x00queue.h | 6 ++
6 files changed, 67 insertions(+)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index 621cd4ce69e2..d420d759a877 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -1212,6 +1212,60 @@ void rt2800_txdone_nostatus(struct rt2x00_dev *rt2x00dev)
}
EXPORT_SYMBOL_GPL(rt2800_txdone_nostatus);
+static int rt2800_check_hung(struct data_queue *queue)
+{
+ unsigned int cur_idx = rt2800_drv_get_dma_done(queue);
+
+ if (queue->wd_idx != cur_idx)
+ queue->wd_count = 0;
+ else
+ queue->wd_count++;
+
+ return queue->wd_count > 16;
+}
+
+void rt2800_watchdog(struct rt2x00_dev *rt2x00dev)
+{
+ struct data_queue *queue;
+ bool hung_tx = false;
+ bool hung_rx = false;
+
+ if (test_bit(DEVICE_STATE_SCANNING, &rt2x00dev->flags))
+ return;
+
+ queue_for_each(rt2x00dev, queue) {
+ switch (queue->qid) {
+ case QID_AC_VO:
+ case QID_AC_VI:
+ case QID_AC_BE:
+ case QID_AC_BK:
+ case QID_MGMT:
+ if (rt2x00queue_empty(queue))
+ continue;
+ hung_tx = rt2800_check_hung(queue);
+ break;
+ case QID_RX:
+ /* For station mode we should reactive at least
+* beacons. TODO: need to find good way detect
+* RX hung for AP mode.
+*/
+ if (rt2x00dev->intf_sta_count == 0)
+ continue;
+ hung_rx = rt2800_check_hung(queue);
+ break;
+ default:
+ break;
+ }
+ }
+
+ if (hung_tx)
+ rt2x00_warn(rt2x00dev, "Watchdog TX hung detected\n");
+
+ if (hung_rx)
+ rt2x00_warn(rt2x00dev, "Watchdog RX hung detected\n");
+}
+EXPORT_SYMBOL_GPL(rt2800_watchdog);
+
static unsigned int rt2800_hw_beacon_base(struct rt2x00_dev *rt2x00dev,
unsigned int index)
{
@@ -10211,6 +10265,8 @@ int rt2800_probe_hw(struct rt2x00_dev *rt2x00dev)
__set_bit(REQUIRE_TASKLET_CONTEXT, &rt2x00dev->cap_flags);
}
+ rt2x00dev->link.watchdog_interval = msecs_to_jiffies(100);
+
/*
* Set the rssi offset.
*/
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.h
b/drivers/net/wireless/ralink/rt2x00/rt2800lib.h
index dbb413ec8b08..f35f4e20d5aa 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.h
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.h
@@ -197,6 +197,8 @@ void rt2800_txdone_nostatus(struct rt2x00_dev *rt2x00dev);
bool rt2800_txstatus_timeout(struct rt2x00_dev *rt2x00dev);
bool rt2800_txstatus_pending(struct rt2x00_dev *rt2x00dev);
+void rt2800_watchdog(struct rt2x00_dev *rt2x00dev);
+
void rt2800_write_beacon(struct queue_entry *entry, struct txentry_desc
*txdesc);
void rt2800_clear_beacon(struct queue_entry *entry);
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800pci.c
b/drivers/net/wireless/ralink/rt2x00/rt2800pci.c
index 71ef8f07b47b..df6345a92f72 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800pci.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800pci.c
@@ -351,6 +351,7 @@ static const struct rt2x00lib_ops rt2800pci_rt2x00_ops = {
.link_tuner = rt2800_link_tuner,
.gain_calibration = rt2800_gain_calibration,
.vco_calibration= rt2800_vco_calibration,
+ .watchdog = rt2800_watchdog,
.start_queue= rt2800mmio_start_queue,
.kick_queue = rt2800mmio_kick_queue,
.stop_queue = rt2800mmio_stop_queue,
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800soc.c
b/drivers/net/wireless/ralink/rt2x00/rt2800soc.c
index 34e9291d949c..1054ade65af2 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800soc.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800soc.c
@@ -196,6 +196,7 @@ static const struct rt2x00lib_ops rt2800soc_rt2x00_ops = {
.link_tuner
[PATCH v2 4/7] rt2800: add pre_reset_hw callback
Add routine to cleanup interfaces data before hw reset as
ieee80211_restart_hw() will do setup interfaces again.
Signed-off-by: Stanislaw Gruszka
---
.../net/wireless/ralink/rt2x00/rt2800lib.c| 19 +++
.../net/wireless/ralink/rt2x00/rt2800lib.h| 1 +
.../net/wireless/ralink/rt2x00/rt2800pci.c| 1 +
.../net/wireless/ralink/rt2x00/rt2800soc.c| 1 +
.../net/wireless/ralink/rt2x00/rt2800usb.c| 1 +
drivers/net/wireless/ralink/rt2x00/rt2x00.h | 1 +
6 files changed, 24 insertions(+)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
index d420d759a877..499e9afa0026 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.c
@@ -1843,6 +1843,25 @@ int rt2800_sta_remove(struct ieee80211_hw *hw, struct
ieee80211_vif *vif,
}
EXPORT_SYMBOL_GPL(rt2800_sta_remove);
+void rt2800_pre_reset_hw(struct rt2x00_dev *rt2x00dev)
+{
+ struct rt2800_drv_data *drv_data = rt2x00dev->drv_data;
+ struct data_queue *queue = rt2x00dev->bcn;
+ struct queue_entry *entry;
+ int i, wcid;
+
+ for (wcid = WCID_START; wcid < WCID_END; wcid++) {
+ drv_data->wcid_to_sta[wcid - WCID_START] = NULL;
+ __clear_bit(wcid - WCID_START, drv_data->sta_ids);
+ }
+
+ for (i = 0; i < queue->limit; i++) {
+ entry = &queue->entries[i];
+ clear_bit(ENTRY_BCN_ASSIGNED, &entry->flags);
+ }
+}
+EXPORT_SYMBOL_GPL(rt2800_pre_reset_hw);
+
void rt2800_config_filter(struct rt2x00_dev *rt2x00dev,
const unsigned int filter_flags)
{
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800lib.h
b/drivers/net/wireless/ralink/rt2x00/rt2800lib.h
index f35f4e20d5aa..1139405c0ebb 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800lib.h
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800lib.h
@@ -257,5 +257,6 @@ void rt2800_disable_wpdma(struct rt2x00_dev *rt2x00dev);
void rt2800_get_txwi_rxwi_size(struct rt2x00_dev *rt2x00dev,
unsigned short *txwi_size,
unsigned short *rxwi_size);
+void rt2800_pre_reset_hw(struct rt2x00_dev *rt2x00dev);
#endif /* RT2800LIB_H */
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800pci.c
b/drivers/net/wireless/ralink/rt2x00/rt2800pci.c
index df6345a92f72..a23c26574002 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800pci.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800pci.c
@@ -368,6 +368,7 @@ static const struct rt2x00lib_ops rt2800pci_rt2x00_ops = {
.config_erp = rt2800_config_erp,
.config_ant = rt2800_config_ant,
.config = rt2800_config,
+ .pre_reset_hw = rt2800_pre_reset_hw,
};
static const struct rt2x00_ops rt2800pci_ops = {
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800soc.c
b/drivers/net/wireless/ralink/rt2x00/rt2800soc.c
index 1054ade65af2..7b931bb96a9e 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800soc.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800soc.c
@@ -213,6 +213,7 @@ static const struct rt2x00lib_ops rt2800soc_rt2x00_ops = {
.config_erp = rt2800_config_erp,
.config_ant = rt2800_config_ant,
.config = rt2800_config,
+ .pre_reset_hw = rt2800_pre_reset_hw,
};
static const struct rt2x00_ops rt2800soc_ops = {
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2800usb.c
b/drivers/net/wireless/ralink/rt2x00/rt2800usb.c
index da813dfa9a91..fdf0504b5f1d 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2800usb.c
+++ b/drivers/net/wireless/ralink/rt2x00/rt2800usb.c
@@ -706,6 +706,7 @@ static const struct rt2x00lib_ops rt2800usb_rt2x00_ops = {
.config_erp = rt2800_config_erp,
.config_ant = rt2800_config_ant,
.config = rt2800_config,
+ .pre_reset_hw = rt2800_pre_reset_hw,
};
static void rt2800usb_queue_init(struct data_queue *queue)
diff --git a/drivers/net/wireless/ralink/rt2x00/rt2x00.h
b/drivers/net/wireless/ralink/rt2x00/rt2x00.h
index 2b551dbe9844..dc6b79e4be3b 100644
--- a/drivers/net/wireless/ralink/rt2x00/rt2x00.h
+++ b/drivers/net/wireless/ralink/rt2x00/rt2x00.h
@@ -616,6 +616,7 @@ struct rt2x00lib_ops {
void (*config) (struct rt2x00_dev *rt2x00dev,
struct rt2x00lib_conf *libconf,
const unsigned int changed_flags);
+ void (*pre_reset_hw) (struct rt2x00_dev *rt2x00dev);
int (*sta_add) (struct rt2x00_dev *rt2x00dev,
struct ieee80211_vif *vif,
struct ieee80211_sta *sta);
--
2.20.1
Re: [PATCH v3 wireless-drivers 3/3] mt76: usb: do not always copy the first part of received frames
On Fri, Jun 14, 2019 at 02:46:36PM +0200, Lorenzo Bianconi wrote:
> > >
> > > ack, right. I think patch 2/3 and 3/3 can go directly in Felix's tree
> > >
> > > >
> > > > > + int i, data_size;
> > > > >
> > > > > + data_size = rounddown(SKB_WITH_OVERHEAD(q->buf_size),
> > > > > +
> > > > > dev->usb.in_ep[MT_EP_IN_PKT_RX].max_packet);
> > > > > for (i = 0; i < nsgs; i++) {
> > > > > struct page *page;
> > > > > void *data;
> > > > > @@ -302,7 +304,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct
> > > > > mt76_queue *q, struct urb *urb,
> > > > >
> > > > > page = virt_to_head_page(data);
> > > > > offset = data - page_address(page);
> > > > > - sg_set_page(&urb->sg[i], page, q->buf_size, offset);
> > > > > + sg_set_page(&urb->sg[i], page, data_size, offset);
> > > >
> > > > > - q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
> > > > > q->ndesc = MT_NUM_RX_ENTRIES;
> > > > > + q->buf_size = PAGE_SIZE;
> > > > > +
> > > >
> > > > This should be associated with decrease of MT_SG_MAX_SIZE to value that
> > > > is actually needed and currently this is 2 for 4k AMSDU.
> > >
> > > MT_SG_MAX_SIZE is used even on tx side and I do not think we will end up
> > > with a
> > > huge difference here
> >
> > So use different value as argument for mt76u_fill_rx_sg() in
> > mt76u_rx_urb_alloc(). After changing buf_size to PAGE_SIZE we will
> > allocate 8 pages per rx queue entry, but only 2 pages will be used
> > (with data_size change, 1 without data_size change). Or I'm wrong?
>
> yes, it is right (we will use two pages with data_size change). Maybe better
> to
> use 4 pages for each rx queue entry? (otherwise we will probably change it in
> the future)
We should not allocate more than is required. If support for bigger
rx AMSDUs will be added and announced in vht/ht capabilities to remote
stations, then increase of number of segments will be needed.
> > > > However I don't think allocating 2 pages to avoid ieee80211 header and
> > > > SNAP
> > > > copy is worth to do. For me best approach would be allocate 1 page for
> > > > 4k AMSDU, 2 for 8k and 3 for 12k (still using sg, but without data_size
> > > > change to avoid 32B copying).
> > >
> > > From my point of view it is better to avoid copying if it is possible.
> > > Are you
> > > sure there is no difference?
> >
> > I do not understand what you mean by difference here.
>
> tpt differences, not sure if there are any
I would not expect any measurable difference in tpt nor in cpu usage
either way.
But I think, if some AMSDU subframe will be spited into two fragments,
data most likely will need to be linearised/copied, at some point before
passed to application, what will overcome any benefit of avoiding coping
802.11 header. Thought, I don't think this somehow will be visible in
benchmarking.
Stanislaw
Re: [PATCH v3 wireless-drivers 1/3] mt76: usb: fix rx A-MSDU support
On Fri, Jun 14, 2019 at 12:20:59PM +0200, Johannes Berg wrote: > On Fri, 2019-06-14 at 12:11 +0200, Lorenzo Bianconi wrote: > > > Looking at __ieee80211_amsdu_copy() now I got why other drivers copy hdrlen > > + > > 8, thx :) > > In our case reuse_frag is true in __ieee80211_amsdu_copy, so we will end up > > copying 32B + ether_len. Anyway I think 32 is a little bit too low and we > > could get > > better performances increasing it a little bit. > > A typical use case (e.g IPv6 + TCP): > > > > IPv6 = 40B, TCP = 32B --> so 72B..I guess 128B is a good value :) > > @Felix, Johannes: what do you think? > > I think while we might *allocate* more, I don't think we should *copy* > more, since then the TCP payload will no longer be in pages. > > It'd probably be better to implement leaving enough tailroom (allocate > 128), but copying nothing, unless the *entire* packet fits. iwl4965 put entire packet in fragment in il4965_pass_packet_to_mac80211() . Initially I thought this is a bug, since mac80211 require header be in the linear area, but looks like ieee80211_rx_monitor() copy header before rest of mac80211 check it, so 4965 is fine. Anyway I think the driver should put ieee80211 header in linear area and iwlwifi & mt7601u implementation is somewhat optimal. Stanislaw
Re: [PATCH v3 wireless-drivers 1/3] mt76: usb: fix rx A-MSDU support
On Fri, Jun 14, 2019 at 12:11:17PM +0200, Lorenzo Bianconi wrote:
> > On Thu, Jun 13, 2019 at 11:43:11PM +0200, Lorenzo Bianconi wrote:
> > > Commit f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes
> > > for rx") breaks A-MSDU support. When A-MSDU is enable the device can
> > > receive frames up to q->buf_size but they will be discarded in
> > > mt76u_process_rx_entry since there is no enough room for
> > > skb_shared_info. Fix the issue reallocating the skb and copying in the
> > > linear area the first 128B of the received frames and in the frag_list
> > > the remaining part.
> > >
> > > Fixes: f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes
> > > for rx")
> > > Signed-off-by: Lorenzo Bianconi
> > > ---
> > > drivers/net/wireless/mediatek/mt76/mt76.h | 1 +
> > > drivers/net/wireless/mediatek/mt76/usb.c | 49 ++-
> > > 2 files changed, 41 insertions(+), 9 deletions(-)
> > >
> > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h
> > > b/drivers/net/wireless/mediatek/mt76/mt76.h
> > > index 8ecbf81a906f..889b76deb703 100644
> > > --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> > > +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> > > @@ -30,6 +30,7 @@
> > > #define MT_TX_RING_SIZE 256
> > > #define MT_MCU_RING_SIZE32
> > > #define MT_RX_BUF_SIZE 2048
> > > +#define MT_SKB_HEAD_LEN 128
> > >
> > > struct mt76_dev;
> > > struct mt76_wcid;
> > > diff --git a/drivers/net/wireless/mediatek/mt76/usb.c
> > > b/drivers/net/wireless/mediatek/mt76/usb.c
> > > index bbaa1365bbda..12d60d31cb51 100644
> > > --- a/drivers/net/wireless/mediatek/mt76/usb.c
> > > +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> > > @@ -429,6 +429,45 @@ static int mt76u_get_rx_entry_len(u8 *data, u32
> > > data_len)
> > > return dma_len;
> > > }
> > >
> > > +static struct sk_buff *
> > > +mt76u_build_rx_skb(u8 *data, int len, int buf_size)
> > > +{
> > > + struct sk_buff *skb;
> > > +
> > > + if (SKB_WITH_OVERHEAD(buf_size) < MT_DMA_HDR_LEN + len) {
> > > + struct page *page;
> > > + int offset;
> > > +
> > > + /* slow path, not enough space for data and
> > > + * skb_shared_info
> > > + */
> > > + skb = alloc_skb(MT_SKB_HEAD_LEN, GFP_ATOMIC);
> > > + if (!skb)
> > > + return NULL;
> > > +
> > > + skb_put_data(skb, data + MT_DMA_HDR_LEN, MT_SKB_HEAD_LEN);
> >
> > I looked how rx amsdu is processed in mac80211 and it is decomposed and
> > copied into newly allocated individual skb's, see ieee80211_amsdu_to_8023s()
> >
> > So copy L3 & L4 headers doesn't do anything good here, actually seems to
> > be better to have them in frag as __ieee80211_amsdu_copy_frag() do some
> > magic to avid copy.
>
> Looking at __ieee80211_amsdu_copy() now I got why other drivers copy hdrlen +
> 8, thx :)
> In our case reuse_frag is true in __ieee80211_amsdu_copy, so we will end up
I don't think reuse_frag is true in our case since skb->head_frag is
not set when we use alloc_skb().
Stanislaw
Re: [PATCH v3 wireless-drivers 3/3] mt76: usb: do not always copy the first part of received frames
On Fri, Jun 14, 2019 at 12:22:48PM +0200, Lorenzo Bianconi wrote:
> > On Thu, Jun 13, 2019 at 11:43:13PM +0200, Lorenzo Bianconi wrote:
> > > Set usb buffer size taking into account skb_shared_info in order to
> > > not always copy the first part of received frames if A-MSDU is enabled
> > > for SG capable devices. Moreover align usb buffer size to max_ep
> > > boundaries and set buf_size to PAGE_SIZE even for sg case
> >
> > I think this should not be applied to wirless-drivers, only first patch
> > that fix the bug and optimizations should be done in -next.
>
> ack, right. I think patch 2/3 and 3/3 can go directly in Felix's tree
>
> >
> > > + int i, data_size;
> > >
> > > + data_size = rounddown(SKB_WITH_OVERHEAD(q->buf_size),
> > > + dev->usb.in_ep[MT_EP_IN_PKT_RX].max_packet);
> > > for (i = 0; i < nsgs; i++) {
> > > struct page *page;
> > > void *data;
> > > @@ -302,7 +304,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct
> > > mt76_queue *q, struct urb *urb,
> > >
> > > page = virt_to_head_page(data);
> > > offset = data - page_address(page);
> > > - sg_set_page(&urb->sg[i], page, q->buf_size, offset);
> > > + sg_set_page(&urb->sg[i], page, data_size, offset);
> >
> > > - q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
> > > q->ndesc = MT_NUM_RX_ENTRIES;
> > > + q->buf_size = PAGE_SIZE;
> > > +
> >
> > This should be associated with decrease of MT_SG_MAX_SIZE to value that
> > is actually needed and currently this is 2 for 4k AMSDU.
>
> MT_SG_MAX_SIZE is used even on tx side and I do not think we will end up with
> a
> huge difference here
So use different value as argument for mt76u_fill_rx_sg() in
mt76u_rx_urb_alloc(). After changing buf_size to PAGE_SIZE we will
allocate 8 pages per rx queue entry, but only 2 pages will be used
(with data_size change, 1 without data_size change). Or I'm wrong?
> > However I don't think allocating 2 pages to avoid ieee80211 header and SNAP
> > copy is worth to do. For me best approach would be allocate 1 page for
> > 4k AMSDU, 2 for 8k and 3 for 12k (still using sg, but without data_size
> > change to avoid 32B copying).
>
> From my point of view it is better to avoid copying if it is possible. Are you
> sure there is no difference?
I do not understand what you mean by difference here.
Stanislaw
Re: [PATCH v3 wireless-drivers 3/3] mt76: usb: do not always copy the first part of received frames
On Thu, Jun 13, 2019 at 11:43:13PM +0200, Lorenzo Bianconi wrote:
> Set usb buffer size taking into account skb_shared_info in order to
> not always copy the first part of received frames if A-MSDU is enabled
> for SG capable devices. Moreover align usb buffer size to max_ep
> boundaries and set buf_size to PAGE_SIZE even for sg case
I think this should not be applied to wirless-drivers, only first patch
that fix the bug and optimizations should be done in -next.
> + int i, data_size;
>
> + data_size = rounddown(SKB_WITH_OVERHEAD(q->buf_size),
> + dev->usb.in_ep[MT_EP_IN_PKT_RX].max_packet);
> for (i = 0; i < nsgs; i++) {
> struct page *page;
> void *data;
> @@ -302,7 +304,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue
> *q, struct urb *urb,
>
> page = virt_to_head_page(data);
> offset = data - page_address(page);
> - sg_set_page(&urb->sg[i], page, q->buf_size, offset);
> + sg_set_page(&urb->sg[i], page, data_size, offset);
> - q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
> q->ndesc = MT_NUM_RX_ENTRIES;
> + q->buf_size = PAGE_SIZE;
> +
This should be associated with decrease of MT_SG_MAX_SIZE to value that
is actually needed and currently this is 2 for 4k AMSDU.
However I don't think allocating 2 pages to avoid ieee80211 header and SNAP
copy is worth to do. For me best approach would be allocate 1 page for
4k AMSDU, 2 for 8k and 3 for 12k (still using sg, but without data_size
change to avoid 32B copying).
Stanislaw
Re: [PATCH v3 wireless-drivers 1/3] mt76: usb: fix rx A-MSDU support
On Thu, Jun 13, 2019 at 11:43:11PM +0200, Lorenzo Bianconi wrote:
> Commit f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes
> for rx") breaks A-MSDU support. When A-MSDU is enable the device can
> receive frames up to q->buf_size but they will be discarded in
> mt76u_process_rx_entry since there is no enough room for
> skb_shared_info. Fix the issue reallocating the skb and copying in the
> linear area the first 128B of the received frames and in the frag_list
> the remaining part.
>
> Fixes: f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes for
> rx")
> Signed-off-by: Lorenzo Bianconi
> ---
> drivers/net/wireless/mediatek/mt76/mt76.h | 1 +
> drivers/net/wireless/mediatek/mt76/usb.c | 49 ++-
> 2 files changed, 41 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h
> b/drivers/net/wireless/mediatek/mt76/mt76.h
> index 8ecbf81a906f..889b76deb703 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> @@ -30,6 +30,7 @@
> #define MT_TX_RING_SIZE 256
> #define MT_MCU_RING_SIZE32
> #define MT_RX_BUF_SIZE 2048
> +#define MT_SKB_HEAD_LEN 128
>
> struct mt76_dev;
> struct mt76_wcid;
> diff --git a/drivers/net/wireless/mediatek/mt76/usb.c
> b/drivers/net/wireless/mediatek/mt76/usb.c
> index bbaa1365bbda..12d60d31cb51 100644
> --- a/drivers/net/wireless/mediatek/mt76/usb.c
> +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> @@ -429,6 +429,45 @@ static int mt76u_get_rx_entry_len(u8 *data, u32 data_len)
> return dma_len;
> }
>
> +static struct sk_buff *
> +mt76u_build_rx_skb(u8 *data, int len, int buf_size)
> +{
> + struct sk_buff *skb;
> +
> + if (SKB_WITH_OVERHEAD(buf_size) < MT_DMA_HDR_LEN + len) {
> + struct page *page;
> + int offset;
> +
> + /* slow path, not enough space for data and
> + * skb_shared_info
> + */
> + skb = alloc_skb(MT_SKB_HEAD_LEN, GFP_ATOMIC);
> + if (!skb)
> + return NULL;
> +
> + skb_put_data(skb, data + MT_DMA_HDR_LEN, MT_SKB_HEAD_LEN);
I looked how rx amsdu is processed in mac80211 and it is decomposed and
copied into newly allocated individual skb's, see ieee80211_amsdu_to_8023s()
So copy L3 & L4 headers doesn't do anything good here, actually seems to
be better to have them in frag as __ieee80211_amsdu_copy_frag() do some
magic to avid copy.
Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
On Wed, Jun 12, 2019 at 04:44:01PM +0200, Lorenzo Bianconi wrote: > On Jun 12, Stanislaw Gruszka wrote: > > On Wed, Jun 12, 2019 at 02:59:05PM +0200, Stanislaw Gruszka wrote: > > > > > If max RX AMSDU size is 3839B I do not see reason why we allocate > > > > > MT_SG_MAX_SIZE=8 of MT_RX_BUF_SIZE=2k buffers for sg_en case. > > > > > I thought the reason is that max AMSDU size is 16kB so it fit into > > > > > 8 sg buffers of 2k. > > > > > > > > > > In other words, for me, looks like either > > > > > - we can not handle AMSDU for non sg case because we do not > > > > > allocate big enough buffer > > > > > > > > I think AMSDU is mandatory and we currently support it even for non-sg > > > > case > > > > (since max rx AMSDU is 3839B) > > > > > > > > > or > > > > > - we can just use one PAGE_SIZE buffer for rx and remove sg > > > > > buffers for rx completely > > > > > > > > using sg buffers we can support bigger rx AMSDU size in the future > > > > without using > > > > huge buffers (e.g. we can try to use IEEE80211_MAX_MPDU_LEN_HT_7935 with > > > > mt76x2u) > > > > > > I think it would be simpler just to allocate 2 pages for 7935B . > > > > And if we could determine that there is no true need to use sg for rx, > > I think best approach here would be revert f8f527b16db5 in v5.2 to fix > > regression and remove rx sg in -next. That would make code simpler, > > allocate 4k instead 16k per packet, allow to use build_skb (4096 - 3839 > > give enough space for shared info) and not use usb hcd bounce buffer. > > I do not think we should drop sg support since: > - it allow us to rx huge amsdu frames (e.g. IEEE80211_MAX_MPDU_LEN_VHT_11454) > using multiple one page buffer. I think there will be new usb devices where > we can > increase amsdu size (we can increase it even on mt76x2u usb 3.0 devices) I looked at intel wifi drivers and this is handled by amsdu_size module parameter, supported values are 4k, 8k and 12k. RX allocation size and proper values in vht_cap & ht_cap are set accordingly. Assuming (some) mt76 HW and FW can handle bigger AMSDUs I think we should do similar thing. Otherwise looks for me, we just waste memory and have not needed code for no true reason. > space needed for skb_shared_info is 320B on a x86_64 device Uhh, I haven't expected that sk_shared_info() is that big, so indeed build_skb could not used and 128B copy fallback will be necessary. Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
On Wed, Jun 12, 2019 at 04:27:42PM +0200, Lorenzo Bianconi wrote: > > On Wed, Jun 12, 2019 at 02:28:48PM +0200, Lorenzo Bianconi wrote: > > [...] > > > > > > > using sg buffers we can support bigger rx AMSDU size in the future > > > without using > > > huge buffers (e.g. we can try to use IEEE80211_MAX_MPDU_LEN_HT_7935 with > > > mt76x2u) > > I think it would be simpler just to allocate 2 pages for 7935B . > > > > We should avoid increasing buffer size to more than PAGE_SIZE for > performance reasons. Interesting, at what place and how this affect performance negatively ? > As suggested by Felix what about of setting buf_size to > PAGE_SIZE for both sg and non-sg cases and for sg setting usb data size to > > SKB_WITH_OVERHEAD(q->buf_size) & (usb_endpoint_maxp() - 1); Increasing to PAGE_SIZE for sg looks reasonable to me. Not sure if understand data_size logic. If this mean 'PAGE_SIZE - usb_endpint_maxp()', looks ok to me as well (for first segment), but would require one extra segment to avoid coping (i.e. 2 pages for 3839 , 3 pages for 7935 ...) If we would like to stay with 128B copy fallback, we can have 1 page for 3839 , 2 for 7935 ... It would be interesting how frequently AMSDU of max size is sent by remote station. If this is rare situation I would be opting for smaller allocation and 128B copy fallback. If this is frequent for bigger allocation to assure we can always use build_skb(). Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
Hi On Thu, Jun 13, 2019 at 10:26:38AM +0200, Lorenzo Bianconi wrote: > [...] > > > I looked at intel wifi drivers and this is handled by amsdu_size module > > parameter, supported values are 4k, 8k and 12k. RX allocation size and > > proper values in vht_cap & ht_cap are set accordingly. Assuming (some) > > mt76 HW and FW can handle bigger AMSDUs I think we should do similar > > thing. > > > > Otherwise looks for me, we just waste memory and have not needed code > > for no true reason. > > > > > space needed for skb_shared_info is 320B on a x86_64 device > > > > Uhh, I haven't expected that sk_shared_info() is that big, so indeed > > build_skb > > could not used and 128B copy fallback will be necessary. > > Hi Stanislaw, > > reviewing the original patch I think we can't trigger any IOMMU bug since the > usb buffer length is actually 2048 and not 2048 + skb_shared_info_size: I'm concerned about alignment and crossing pages boundaries. If you allocate via page_frag_alloc() buffers, except first one, will have 'not natural' alignment and every second will be spanned across two pages. Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
On Wed, Jun 12, 2019 at 02:59:05PM +0200, Stanislaw Gruszka wrote: > > > If max RX AMSDU size is 3839B I do not see reason why we allocate > > > MT_SG_MAX_SIZE=8 of MT_RX_BUF_SIZE=2k buffers for sg_en case. > > > I thought the reason is that max AMSDU size is 16kB so it fit into > > > 8 sg buffers of 2k. > > > > > > In other words, for me, looks like either > > > - we can not handle AMSDU for non sg case because we do not > > > allocate big enough buffer > > > > I think AMSDU is mandatory and we currently support it even for non-sg case > > (since max rx AMSDU is 3839B) > > > > > or > > > - we can just use one PAGE_SIZE buffer for rx and remove sg > > > buffers for rx completely > > > > using sg buffers we can support bigger rx AMSDU size in the future without > > using > > huge buffers (e.g. we can try to use IEEE80211_MAX_MPDU_LEN_HT_7935 with > > mt76x2u) > > I think it would be simpler just to allocate 2 pages for 7935B . And if we could determine that there is no true need to use sg for rx, I think best approach here would be revert f8f527b16db5 in v5.2 to fix regression and remove rx sg in -next. That would make code simpler, allocate 4k instead 16k per packet, allow to use build_skb (4096 - 3839 give enough space for shared info) and not use usb hcd bounce buffer. Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
On Wed, Jun 12, 2019 at 02:28:48PM +0200, Lorenzo Bianconi wrote:
> [...]
> >
> > My sense of humor declined quite drastically lastly ;-/
> >
> > > > > but we can be more cautious since probably copying
> > > > > the first 128B will not make any difference
> > > >
> > > > Not sure if I understand what you mean.
> > >
> > > Please correct me if I am wrong but I think max amsdu rx size is 3839B for
> > > mt76. For the sg_en case this frame will span over multiple sg buffers
> > > since
> > > sg buffer size is 2048B (2 sg buffers). Moreover if we do not take into
> > > account
> > > skb_shared_info when configuring the sg buffer size we will need to
> > > always copy
> > > the first 128B of the first buffer since received len will be set to 2048
> > > and
> > > the following if condition will always fail:
> > >
> > > if (SKB_WITH_OVERHEAD(buf_size) >= MT_DMA_HDR_LEN + len) {
> > > }
> >
> > Ok, that I understand.
> >
> > > > > > And skb_shered_info is needed only in first buffer IIUC.
> > > > > >
> > > > > > Also this patch seems to make first patch unnecessary except for
> > > > > > non sg_en case (in which I think rx AMSDU is broken anyway),
> > > > > > so I would prefer just to apply first patch.
> > > > >
> > > > > I do not think rx AMSDU is broken for non sg_en case since the max rx
> > > > > value
> > > > > allowed should be 3839 IIRC and we alloc one page in this case
> > > >
> > > > If that's the case we should be fine, but then I do not understand
> > > > why we allocate 8*2k buffers for sg_en case, isn't that AP can
> > > > sent AMSDU frame 16k big?
> > >
> > > Sorry I did not get what you mean here, could you please explain?
> >
> > If max RX AMSDU size is 3839B I do not see reason why we allocate
> > MT_SG_MAX_SIZE=8 of MT_RX_BUF_SIZE=2k buffers for sg_en case.
> > I thought the reason is that max AMSDU size is 16kB so it fit into
> > 8 sg buffers of 2k.
> >
> > In other words, for me, looks like either
> > - we can not handle AMSDU for non sg case because we do not
> > allocate big enough buffer
>
> I think AMSDU is mandatory and we currently support it even for non-sg case
> (since max rx AMSDU is 3839B)
>
> > or
> > - we can just use one PAGE_SIZE buffer for rx and remove sg
> > buffers for rx completely
>
> using sg buffers we can support bigger rx AMSDU size in the future without
> using
> huge buffers (e.g. we can try to use IEEE80211_MAX_MPDU_LEN_HT_7935 with
> mt76x2u)
I think it would be simpler just to allocate 2 pages for 7935B .
Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
On Wed, Jun 12, 2019 at 12:49:22PM +0200, Lorenzo Bianconi wrote:
> > On Wed, Jun 12, 2019 at 11:53:03AM +0200, Lorenzo Bianconi wrote:
> > > > On Fri, May 31, 2019 at 11:38:23AM +0200, Lorenzo Bianconi wrote:
> > >
> > > [...]
> > >
> > > > > }
> > > > >
> > > > > urb->num_sgs = max_t(int, i, urb->num_sgs);
> > > > > - urb->transfer_buffer_length = urb->num_sgs * q->buf_size,
> > > > > + urb->transfer_buffer_length = urb->num_sgs * data_size;
> > > > > sg_init_marker(urb->sg, urb->num_sgs);
> > > > >
> > > > > return i ? : -ENOMEM;
> > > > > @@ -611,8 +611,12 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
> > > > > if (!q->entry)
> > > > > return -ENOMEM;
> > > > >
> > > > > - q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
> > > > > + if (dev->usb.sg_en)
> > > > > + q->buf_size = MT_BUF_WITH_OVERHEAD(MT_RX_BUF_SIZE);
> > > >
> > > > I strongly recommend to not doing this. While this should work
> > > > in theory creating buffer with size of 2k + some bytes might
> > > > trigger various bugs in dma mapping or other low level code.
> > >
> > > even in practice actually :)
> >
> > I wouldn't be sure about this. It's not common to have buffers of
> > such size and crossing pages boundaries. It really can trigger
> > nasty bugs on various IOMMU drivers.
>
> I was just joking, I mean that it worked in the tests I carried out, but I
> agree it can trigger some issues in buggy IOMMU drivers
My sense of humor declined quite drastically lastly ;-/
> > > but we can be more cautious since probably copying
> > > the first 128B will not make any difference
> >
> > Not sure if I understand what you mean.
>
> Please correct me if I am wrong but I think max amsdu rx size is 3839B for
> mt76. For the sg_en case this frame will span over multiple sg buffers since
> sg buffer size is 2048B (2 sg buffers). Moreover if we do not take into
> account
> skb_shared_info when configuring the sg buffer size we will need to always
> copy
> the first 128B of the first buffer since received len will be set to 2048 and
> the following if condition will always fail:
>
> if (SKB_WITH_OVERHEAD(buf_size) >= MT_DMA_HDR_LEN + len) {
> }
Ok, that I understand.
> > > > And skb_shered_info is needed only in first buffer IIUC.
> > > >
> > > > Also this patch seems to make first patch unnecessary except for
> > > > non sg_en case (in which I think rx AMSDU is broken anyway),
> > > > so I would prefer just to apply first patch.
> > >
> > > I do not think rx AMSDU is broken for non sg_en case since the max rx
> > > value
> > > allowed should be 3839 IIRC and we alloc one page in this case
> >
> > If that's the case we should be fine, but then I do not understand
> > why we allocate 8*2k buffers for sg_en case, isn't that AP can
> > sent AMSDU frame 16k big?
>
> Sorry I did not get what you mean here, could you please explain?
If max RX AMSDU size is 3839B I do not see reason why we allocate
MT_SG_MAX_SIZE=8 of MT_RX_BUF_SIZE=2k buffers for sg_en case.
I thought the reason is that max AMSDU size is 16kB so it fit into
8 sg buffers of 2k.
In other words, for me, looks like either
- we can not handle AMSDU for non sg case because we do not
allocate big enough buffer
or
- we can just use one PAGE_SIZE buffer for rx and remove sg
buffers for rx completely
Do I miss something ?
Stanislaw
Re: [PATCH v2 1/2] mt76: usb: fix rx A-MSDU support
On Wed, Jun 12, 2019 at 12:21:34PM +0200, Lorenzo Bianconi wrote:
> On Jun 12, Stanislaw Gruszka wrote:
> > On Wed, Jun 12, 2019 at 11:45:21AM +0200, Lorenzo Bianconi wrote:
> > > > > +mt76u_build_rx_skb(u8 *data, int len, int buf_size,
> > > > > +int *nsgs)
> > > > > +{
> > > > > + int data_len = min(len, MT_SKB_HEAD_LEN);
> >
> > Oh, and this looks unneeded as well as for len < MT_SKB_HEAD_LEN=128
> > we will go through fast path.
>
> I guess if we remove data_len = min(len, MT_SKB_HEAD_LEN) and even *nsgs = 0
> at
> the end we are making some assumptions on the value of MT_SKB_HEAD_LEN and
> buf_size. In the patch I just avoided them but maybe we can just assume that
> MT_SKB_HEAD_LEN and buf_size will not changed in the future. What do you
> think?
Yes, sure. Other drivers just use 128 value directly and don't even
create a macro for that. And if somebody will decide to change
buf_size it will not be small value.
> > > > mt7601u and iwlmvm just copy hdrlen + 8 and put the rest
> > > > of the buffer in fragment, which supose to be more efficient,
> > > > see comment in iwl_mvm_pass_packet_to_mac80211().
> > >
> > > Right here we copy 128B instead of 32 but I think it is good to have L3
> > > and L4
> > > header in the linear area of the skb since otherwise the stack will need
> > > to
> > > align them
> >
> > Not sure if understand, I think aliment of L3 & L4 headers will be
> > the same, assuming ieee80211 header is aligned the same in fragment
> > buffer and in linear area. But if you think this is better to copy those
> > to linear area I'm ok with that.
>
> Sorry I have not been so clear. I mean in the stack before accessing a given
> header we will run pskb_may_pull() that can end up copying the skb if there is
> not enough space in the skb->head
Ok, so L3 and L4 headers should be in linear area of skb and if not
network stack will copy them from fragment. But I wonder why other
drivers just copy ieee80211_hdr and SNAP ? Isn't that if we copy
128B then is possible that part of the payload will be in linear
area and part in fragment, whereas is expected that payload
will not be broken into two parts?
Stanislaw
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
On Wed, Jun 12, 2019 at 11:53:03AM +0200, Lorenzo Bianconi wrote: > > On Fri, May 31, 2019 at 11:38:23AM +0200, Lorenzo Bianconi wrote: > > [...] > > > > } > > > > > > urb->num_sgs = max_t(int, i, urb->num_sgs); > > > - urb->transfer_buffer_length = urb->num_sgs * q->buf_size, > > > + urb->transfer_buffer_length = urb->num_sgs * data_size; > > > sg_init_marker(urb->sg, urb->num_sgs); > > > > > > return i ? : -ENOMEM; > > > @@ -611,8 +611,12 @@ static int mt76u_alloc_rx(struct mt76_dev *dev) > > > if (!q->entry) > > > return -ENOMEM; > > > > > > - q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE; > > > + if (dev->usb.sg_en) > > > + q->buf_size = MT_BUF_WITH_OVERHEAD(MT_RX_BUF_SIZE); > > > > I strongly recommend to not doing this. While this should work > > in theory creating buffer with size of 2k + some bytes might > > trigger various bugs in dma mapping or other low level code. > > even in practice actually :) I wouldn't be sure about this. It's not common to have buffers of such size and crossing pages boundaries. It really can trigger nasty bugs on various IOMMU drivers. > but we can be more cautious since probably copying > the first 128B will not make any difference Not sure if I understand what you mean. > > And skb_shered_info is needed only in first buffer IIUC. > > > > Also this patch seems to make first patch unnecessary except for > > non sg_en case (in which I think rx AMSDU is broken anyway), > > so I would prefer just to apply first patch. > > I do not think rx AMSDU is broken for non sg_en case since the max rx value > allowed should be 3839 IIRC and we alloc one page in this case If that's the case we should be fine, but then I do not understand why we allocate 8*2k buffers for sg_en case, isn't that AP can sent AMSDU frame 16k big? Stanislaw
Re: [PATCH v2 1/2] mt76: usb: fix rx A-MSDU support
On Wed, Jun 12, 2019 at 11:45:21AM +0200, Lorenzo Bianconi wrote:
> > > +mt76u_build_rx_skb(u8 *data, int len, int buf_size,
> > > +int *nsgs)
> > > +{
> > > + int data_len = min(len, MT_SKB_HEAD_LEN);
Oh, and this looks unneeded as well as for len < MT_SKB_HEAD_LEN=128
we will go through fast path.
> > mt7601u and iwlmvm just copy hdrlen + 8 and put the rest
> > of the buffer in fragment, which supose to be more efficient,
> > see comment in iwl_mvm_pass_packet_to_mac80211().
>
> Right here we copy 128B instead of 32 but I think it is good to have L3 and L4
> header in the linear area of the skb since otherwise the stack will need to
> align them
Not sure if understand, I think aliment of L3 & L4 headers will be
the same, assuming ieee80211 header is aligned the same in fragment
buffer and in linear area. But if you think this is better to copy those
to linear area I'm ok with that.
Stanislaw
Re: rtw88: M.2 RTL8822BE not working - rfe 3 isn't supported
Cc Tony On Sat, Jun 08, 2019 at 02:26:51PM +0200, g.schlmm wrote: > my RTL8822BE M.2 card is not working with linux 5.2rc3 > > the staging r8822be driver in linux 5.1 was working for this card > > from dmesg: > > [8.001186] rtw_pci :04:00.0: rfe 3 isn't supported > > > > [8.003870] rtw_pci :04:00.0: failed to setup chip efuse info > > > > [8.006405] rtw_pci :04:00.0: failed to setup chip information > > lspci: > > 04:00.0 Unassigned class [ff00]: Realtek Semiconductor Co., Ltd. RTL8822BE > > 802.11a/b/g/n/ac WiFi adapter > > Subsystem: Lenovo RTL8822BE 802.11a/b/g/n/ac WiFi adapter > > Flags: fast devsel, IRQ 19 > > I/O ports at c000 [size=256] > > Memory at 8120 (64-bit, non-prefetchable) [size=64K] > > Capabilities: [40] Power Management version 3 > > Capabilities: [50] MSI: Enable- Count=1/1 Maskable- 64bit+ > > Capabilities: [70] Express Endpoint, MSI 00 > > Capabilities: [100] Advanced Error Reporting > > Capabilities: [148] Device Serial Number 00-e0-4c-ff-fe-b8-22-01 > > Capabilities: [158] Latency Tolerance Reporting > > Capabilities: [160] L1 PM Substates > > Kernel modules: rtwpci
Re: [PATCH v2 2/2] mt76: usb: do not always copy the first part of received frames
On Fri, May 31, 2019 at 11:38:23AM +0200, Lorenzo Bianconi wrote:
> Set usb buffer size taking into account skb_shared_info in order to
> not always copy the first part of received frames if A-MSDU is enabled
> for SG capable devices
>
> Signed-off-by: Lorenzo Bianconi
> ---
> drivers/net/wireless/mediatek/mt76/mt76.h | 3 +++
> drivers/net/wireless/mediatek/mt76/usb.c | 12
> 2 files changed, 11 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h
> b/drivers/net/wireless/mediatek/mt76/mt76.h
> index 74d4edf941d6..7899e9b88b54 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> @@ -32,6 +32,9 @@
> #define MT_RX_BUF_SIZE 2048
> #define MT_SKB_HEAD_LEN 128
>
> +#define MT_BUF_WITH_OVERHEAD(x) \
> + ((x) + SKB_DATA_ALIGN(sizeof(struct skb_shared_info)))
> +
> struct mt76_dev;
> struct mt76_wcid;
>
> diff --git a/drivers/net/wireless/mediatek/mt76/usb.c
> b/drivers/net/wireless/mediatek/mt76/usb.c
> index 2bfc8214c0d8..5081643ce701 100644
> --- a/drivers/net/wireless/mediatek/mt76/usb.c
> +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> @@ -286,7 +286,7 @@ static int
> mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue *q, struct urb *urb,
>int nsgs, gfp_t gfp)
> {
> - int i;
> + int i, data_size = SKB_WITH_OVERHEAD(q->buf_size);
>
> for (i = 0; i < nsgs; i++) {
> struct page *page;
> @@ -299,7 +299,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue
> *q, struct urb *urb,
>
> page = virt_to_head_page(data);
> offset = data - page_address(page);
> - sg_set_page(&urb->sg[i], page, q->buf_size, offset);
> + sg_set_page(&urb->sg[i], page, data_size, offset);
> }
>
> if (i < nsgs) {
> @@ -311,7 +311,7 @@ mt76u_fill_rx_sg(struct mt76_dev *dev, struct mt76_queue
> *q, struct urb *urb,
> }
>
> urb->num_sgs = max_t(int, i, urb->num_sgs);
> - urb->transfer_buffer_length = urb->num_sgs * q->buf_size,
> + urb->transfer_buffer_length = urb->num_sgs * data_size;
> sg_init_marker(urb->sg, urb->num_sgs);
>
> return i ? : -ENOMEM;
> @@ -611,8 +611,12 @@ static int mt76u_alloc_rx(struct mt76_dev *dev)
> if (!q->entry)
> return -ENOMEM;
>
> - q->buf_size = dev->usb.sg_en ? MT_RX_BUF_SIZE : PAGE_SIZE;
> + if (dev->usb.sg_en)
> + q->buf_size = MT_BUF_WITH_OVERHEAD(MT_RX_BUF_SIZE);
I strongly recommend to not doing this. While this should work
in theory creating buffer with size of 2k + some bytes might
trigger various bugs in dma mapping or other low level code.
And skb_shered_info is needed only in first buffer IIUC.
Also this patch seems to make first patch unnecessary except for
non sg_en case (in which I think rx AMSDU is broken anyway),
so I would prefer just to apply first patch.
Stanislaw
Re: [PATCH v2 1/2] mt76: usb: fix rx A-MSDU support
Hi and sorry for late comment.
On Fri, May 31, 2019 at 11:38:22AM +0200, Lorenzo Bianconi wrote:
> Commit f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes
> for rx") breaks A-MSDU support. When A-MSDU is enable the device can
> receive frames up to q->buf_size but they will be discarded in
> mt76u_process_rx_entry since there is no enough room for
> skb_shared_info. Fix the issue reallocating the skb and copying in the
> linear area the first 128B of the received frames and in the frag_list
> the remaining part.
>
> Fixes: f8f527b16db5 ("mt76: usb: use EP max packet aligned buffer sizes for
> rx")
> Signed-off-by: Lorenzo Bianconi
> ---
> drivers/net/wireless/mediatek/mt76/mt76.h | 1 +
> drivers/net/wireless/mediatek/mt76/usb.c | 52 +++
> 2 files changed, 44 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/net/wireless/mediatek/mt76/mt76.h
> b/drivers/net/wireless/mediatek/mt76/mt76.h
> index 97a1296562d0..74d4edf941d6 100644
> --- a/drivers/net/wireless/mediatek/mt76/mt76.h
> +++ b/drivers/net/wireless/mediatek/mt76/mt76.h
> @@ -30,6 +30,7 @@
> #define MT_TX_RING_SIZE 256
> #define MT_MCU_RING_SIZE32
> #define MT_RX_BUF_SIZE 2048
> +#define MT_SKB_HEAD_LEN 128
>
> struct mt76_dev;
> struct mt76_wcid;
> diff --git a/drivers/net/wireless/mediatek/mt76/usb.c
> b/drivers/net/wireless/mediatek/mt76/usb.c
> index bbaa1365bbda..2bfc8214c0d8 100644
> --- a/drivers/net/wireless/mediatek/mt76/usb.c
> +++ b/drivers/net/wireless/mediatek/mt76/usb.c
> @@ -429,6 +429,48 @@ static int mt76u_get_rx_entry_len(u8 *data, u32 data_len)
> return dma_len;
> }
>
> +static struct sk_buff *
> +mt76u_build_rx_skb(u8 *data, int len, int buf_size,
> +int *nsgs)
> +{
> + int data_len = min(len, MT_SKB_HEAD_LEN);
> + struct sk_buff *skb;
> +
> + if (SKB_WITH_OVERHEAD(buf_size) >= MT_DMA_HDR_LEN + len) {
> + /* fast path */
> + skb = build_skb(data, buf_size);
> + if (!skb)
> + return NULL;
> +
> + skb_reserve(skb, MT_DMA_HDR_LEN);
> + __skb_put(skb, len);
> +
> + return skb;
> + }
> +
> + /* slow path, not enough space for data and
> + * skb_shared_info
> + */
> + skb = alloc_skb(data_len, GFP_ATOMIC);
> + if (!skb)
> + return NULL;
> +
> + skb_put_data(skb, data + MT_DMA_HDR_LEN, data_len);
mt7601u and iwlmvm just copy hdrlen + 8 and put the rest
of the buffer in fragment, which supose to be more efficient,
see comment in iwl_mvm_pass_packet_to_mac80211().
> + data += (data_len + MT_DMA_HDR_LEN);
> + len -= data_len;
> + if (len > 0) {
> + struct page *page = virt_to_head_page(data);
> + int offset = data - (u8 *)page_address(page);
u8 cast not needed.
> + skb_add_rx_frag(skb, skb_shinfo(skb)->nr_frags,
> + page, offset, len, buf_size);
> + } else {
> + *nsgs = 0;
This seems to be not necessary or a bug (use first buffer 2 times).
Stanislaw
Re: [PATCH] mt76: mt7603: add debugfs knob to enable/disable edcca
On Wed, May 15, 2019 at 03:48:29PM +0200, Stanislaw Gruszka wrote: > > Tx hangs occur in very particular conditions (e.g 200Mbps bidirectional > > traffic) and moreover they do not always occur so I am not convinced they > > are always EDCCA related and so I am not confident to remove the watchdog > > I'm not opting for watchdog removal, but for full EDCCA removal. On mt76x02, on other chips/firmwares it can work just fine. Stanislaw
Re: [PATCH] mt76: mt7603: add debugfs knob to enable/disable edcca
On Wed, May 15, 2019 at 02:07:42PM +0200, Lorenzo Bianconi wrote: > > On Wed, May 15, 2019 at 01:13:49PM +0200, Lorenzo Bianconi wrote: > > > > On Wed, May 15, 2019 at 12:03:44PM +0200, Lorenzo Bianconi wrote: > > > > > > On Wed, May 15, 2019 at 11:43:55AM +0200, Lorenzo Bianconi wrote: > > > > > > > > On Mon, May 13, 2019 at 11:48:37AM +0200, Stanislaw Gruszka > > > > > > > > wrote: > > > > > > > > > On Mon, May 13, 2019 at 10:41:28AM +0200, Lorenzo Bianconi > > > > > > > > > wrote: > > > > > > > > > > > Lorenzo Bianconi writes: > > > > > > > > > > > > > > > > > > > > > > > Introduce a knob in mt7603 debugfs in order to > > > > > > > > > > > > enable/disable > > > > > > > > > > > > edcca processing > > > > > > > > > > > > > > > > > > > > > > > > Signed-off-by: Lorenzo Bianconi > > > > > > > > > > > > > > > > > > > > > > It's good to explain what edcca does and how the file is > > > > > > > > > > > used supposed > > > > > > > > > > > to be used. In other words, have a small introduction for > > > > > > > > > > > the user. > > > > > > > > > > > > > > > > > > > > Hi Kalle, > > > > > > > > > > > > > > > > > > > > edcca is used for adjusting energy detect based on CCA > > > > > > > > > > thresholds. > > > > > > > > > > The code was already there so I just reported the acronym. > > > > > > > > > > > > > > > > > > What for it is needed ? > > > > > > > > > > > > > > > > Care to comment why EDCCA is needed at all ? > > > > > > > > > > > > > > > > Taking that debugfs file that enable it is read-only, it looks > > > > > > > > like > > > > > > > > feature that nobody needs nor tests. > > > > > > > > > > > > > > already fixed in v2 > > > > > > > https://patchwork.kernel.org/patch/10940645/ > > > > > > > > > > > > I'm aware of this patch and other one for mt76x02. But so far in the > > > > > > sources EDCCA is disabled for mt76x02 without possibility to enable > > > > > > it > > > > > > (and this permission file issue was pointed by Kalle during review, > > > > > > not > > > > > > by someone who want to test EDCCA). So again, what for EDCCA is > > > > > > needed ? > > > > > > > > > > As I have already written in a previous email, ED/CCA is used to > > > > > control tx power > > > > > according to the CCA MIB counters (e.g do not transmit if the channel > > > > > busy time > > > > > is higher than 90% for given amount of time in a row). I guess it is > > > > > required > > > > > by ETSI regulatory. > > > > But what is user case for that, i.e. who need this (it wasn't > > > > implemented in > > > > mt76x2 since you added it on Dec 2018). What will happen if it will be > > > > removed? > > > > > > > > > Regarding file permission for mt76x02 debugfs edcca node is a typo. > > > > Typo or not, effectively disable the feature and show nobody is > > > > testing it. > > > > > > > > The reason I'm asking is that seems EDCCA is the main reason to > > > > implement watchod for mt76x2, it wasn't necessary to have a watchdog > > > > as seems devices did not hung before EDCCA was added. > > > > > > IIRC I added the first watchdog implementation to fix tx hangs that occur > > > under heavy load even using FCC regulatory (so when EDCCA processing is > > > disabled) > > > > There was changes in various registers programming introduced by EDCCA > > support, even with EDCCA disabled. It's rally not convenient that > > watchdog and EDCCA are not related, since you added tx hung watchdog > > 2 weeks after adding EDCCA. > > > > You can look at this report: > > https://github.com/openwrt/mt76/issues/246 > > Before mt76x2e worked without hungs & watchodg. Now, even with EDCCA > > disabled watchdog and HW restarts are required to fix hungs on runtime. > > Tx hangs occur in very particular conditions (e.g 200Mbps bidirectional > traffic) and moreover they do not always occur so I am not convinced they > are always EDCCA related and so I am not confident to remove the watchdog I'm not opting for watchdog removal, but for full EDCCA removal. Apparently nobody cares, if it can be enabled or not. Stanislaw
Re: [PATCH] mt76: mt7603: add debugfs knob to enable/disable edcca
On Wed, May 15, 2019 at 01:13:49PM +0200, Lorenzo Bianconi wrote: > > On Wed, May 15, 2019 at 12:03:44PM +0200, Lorenzo Bianconi wrote: > > > > On Wed, May 15, 2019 at 11:43:55AM +0200, Lorenzo Bianconi wrote: > > > > > > On Mon, May 13, 2019 at 11:48:37AM +0200, Stanislaw Gruszka wrote: > > > > > > > On Mon, May 13, 2019 at 10:41:28AM +0200, Lorenzo Bianconi wrote: > > > > > > > > > Lorenzo Bianconi writes: > > > > > > > > > > > > > > > > > > > Introduce a knob in mt7603 debugfs in order to > > > > > > > > > > enable/disable > > > > > > > > > > edcca processing > > > > > > > > > > > > > > > > > > > > Signed-off-by: Lorenzo Bianconi > > > > > > > > > > > > > > > > > > It's good to explain what edcca does and how the file is used > > > > > > > > > supposed > > > > > > > > > to be used. In other words, have a small introduction for the > > > > > > > > > user. > > > > > > > > > > > > > > > > Hi Kalle, > > > > > > > > > > > > > > > > edcca is used for adjusting energy detect based on CCA > > > > > > > > thresholds. > > > > > > > > The code was already there so I just reported the acronym. > > > > > > > > > > > > > > What for it is needed ? > > > > > > > > > > > > Care to comment why EDCCA is needed at all ? > > > > > > > > > > > > Taking that debugfs file that enable it is read-only, it looks like > > > > > > feature that nobody needs nor tests. > > > > > > > > > > already fixed in v2 > > > > > https://patchwork.kernel.org/patch/10940645/ > > > > > > > > I'm aware of this patch and other one for mt76x02. But so far in the > > > > sources EDCCA is disabled for mt76x02 without possibility to enable it > > > > (and this permission file issue was pointed by Kalle during review, not > > > > by someone who want to test EDCCA). So again, what for EDCCA is needed ? > > > > > > As I have already written in a previous email, ED/CCA is used to control > > > tx power > > > according to the CCA MIB counters (e.g do not transmit if the channel > > > busy time > > > is higher than 90% for given amount of time in a row). I guess it is > > > required > > > by ETSI regulatory. > > But what is user case for that, i.e. who need this (it wasn't implemented in > > mt76x2 since you added it on Dec 2018). What will happen if it will be > > removed? > > > > > Regarding file permission for mt76x02 debugfs edcca node is a typo. > > Typo or not, effectively disable the feature and show nobody is > > testing it. > > > > The reason I'm asking is that seems EDCCA is the main reason to > > implement watchod for mt76x2, it wasn't necessary to have a watchdog > > as seems devices did not hung before EDCCA was added. > > IIRC I added the first watchdog implementation to fix tx hangs that occur > under heavy load even using FCC regulatory (so when EDCCA processing is > disabled) There was changes in various registers programming introduced by EDCCA support, even with EDCCA disabled. It's rally not convenient that watchdog and EDCCA are not related, since you added tx hung watchdog 2 weeks after adding EDCCA. You can look at this report: https://github.com/openwrt/mt76/issues/246 Before mt76x2e worked without hungs & watchodg. Now, even with EDCCA disabled watchdog and HW restarts are required to fix hungs on runtime. Stanislaw
Re: [PATCH] mt76: mt7603: add debugfs knob to enable/disable edcca
On Wed, May 15, 2019 at 12:03:44PM +0200, Lorenzo Bianconi wrote: > > On Wed, May 15, 2019 at 11:43:55AM +0200, Lorenzo Bianconi wrote: > > > > On Mon, May 13, 2019 at 11:48:37AM +0200, Stanislaw Gruszka wrote: > > > > > On Mon, May 13, 2019 at 10:41:28AM +0200, Lorenzo Bianconi wrote: > > > > > > > Lorenzo Bianconi writes: > > > > > > > > > > > > > > > Introduce a knob in mt7603 debugfs in order to enable/disable > > > > > > > > edcca processing > > > > > > > > > > > > > > > > Signed-off-by: Lorenzo Bianconi > > > > > > > > > > > > > > It's good to explain what edcca does and how the file is used > > > > > > > supposed > > > > > > > to be used. In other words, have a small introduction for the > > > > > > > user. > > > > > > > > > > > > Hi Kalle, > > > > > > > > > > > > edcca is used for adjusting energy detect based on CCA thresholds. > > > > > > The code was already there so I just reported the acronym. > > > > > > > > > > What for it is needed ? > > > > > > > > Care to comment why EDCCA is needed at all ? > > > > > > > > Taking that debugfs file that enable it is read-only, it looks like > > > > feature that nobody needs nor tests. > > > > > > already fixed in v2 > > > https://patchwork.kernel.org/patch/10940645/ > > > > I'm aware of this patch and other one for mt76x02. But so far in the > > sources EDCCA is disabled for mt76x02 without possibility to enable it > > (and this permission file issue was pointed by Kalle during review, not > > by someone who want to test EDCCA). So again, what for EDCCA is needed ? > > As I have already written in a previous email, ED/CCA is used to control tx > power > according to the CCA MIB counters (e.g do not transmit if the channel busy > time > is higher than 90% for given amount of time in a row). I guess it is required > by ETSI regulatory. But what is user case for that, i.e. who need this (it wasn't implemented in mt76x2 since you added it on Dec 2018). What will happen if it will be removed? > Regarding file permission for mt76x02 debugfs edcca node is a typo. Typo or not, effectively disable the feature and show nobody is testing it. The reason I'm asking is that seems EDCCA is the main reason to implement watchod for mt76x2, it wasn't necessary to have a watchdog as seems devices did not hung before EDCCA was added. Stanislaw
Re: [PATCH] mt76: mt7603: add debugfs knob to enable/disable edcca
On Wed, May 15, 2019 at 11:43:55AM +0200, Lorenzo Bianconi wrote: > > On Mon, May 13, 2019 at 11:48:37AM +0200, Stanislaw Gruszka wrote: > > > On Mon, May 13, 2019 at 10:41:28AM +0200, Lorenzo Bianconi wrote: > > > > > Lorenzo Bianconi writes: > > > > > > > > > > > Introduce a knob in mt7603 debugfs in order to enable/disable > > > > > > edcca processing > > > > > > > > > > > > Signed-off-by: Lorenzo Bianconi > > > > > > > > > > It's good to explain what edcca does and how the file is used supposed > > > > > to be used. In other words, have a small introduction for the user. > > > > > > > > Hi Kalle, > > > > > > > > edcca is used for adjusting energy detect based on CCA thresholds. > > > > The code was already there so I just reported the acronym. > > > > > > What for it is needed ? > > > > Care to comment why EDCCA is needed at all ? > > > > Taking that debugfs file that enable it is read-only, it looks like > > feature that nobody needs nor tests. > > already fixed in v2 > https://patchwork.kernel.org/patch/10940645/ I'm aware of this patch and other one for mt76x02. But so far in the sources EDCCA is disabled for mt76x02 without possibility to enable it (and this permission file issue was pointed by Kalle during review, not by someone who want to test EDCCA). So again, what for EDCCA is needed ? Stanislaw
Re: [PATCH] mt76: mt7603: add debugfs knob to enable/disable edcca
On Mon, May 13, 2019 at 11:48:37AM +0200, Stanislaw Gruszka wrote: > On Mon, May 13, 2019 at 10:41:28AM +0200, Lorenzo Bianconi wrote: > > > Lorenzo Bianconi writes: > > > > > > > Introduce a knob in mt7603 debugfs in order to enable/disable > > > > edcca processing > > > > > > > > Signed-off-by: Lorenzo Bianconi > > > > > > It's good to explain what edcca does and how the file is used supposed > > > to be used. In other words, have a small introduction for the user. > > > > Hi Kalle, > > > > edcca is used for adjusting energy detect based on CCA thresholds. > > The code was already there so I just reported the acronym. > > What for it is needed ? Care to comment why EDCCA is needed at all ? Taking that debugfs file that enable it is read-only, it looks like feature that nobody needs nor tests. Stanislaw
Re: [PATCH] mt76: mt7603: add debugfs knob to enable/disable edcca
On Mon, May 13, 2019 at 10:41:28AM +0200, Lorenzo Bianconi wrote: > > Lorenzo Bianconi writes: > > > > > Introduce a knob in mt7603 debugfs in order to enable/disable > > > edcca processing > > > > > > Signed-off-by: Lorenzo Bianconi > > > > It's good to explain what edcca does and how the file is used supposed > > to be used. In other words, have a small introduction for the user. > > Hi Kalle, > > edcca is used for adjusting energy detect based on CCA thresholds. > The code was already there so I just reported the acronym. What for it is needed ? Stanislaw
Re: [PATCH 3/4] mt76: mt76x2: move mutex_lock inside mt76x2_set_channel
On Mon, May 13, 2019 at 11:19:06AM +0200, Lorenzo Bianconi wrote: > > On Sat, May 11, 2019 at 12:17:53PM +0200, Lorenzo Bianconi wrote: > > > This is a preliminary patch to run mt76x02_edcca_init atomically > > > > > > Signed-off-by: Lorenzo Bianconi > > > --- > > > .../wireless/mediatek/mt76/mt76x2/pci_main.c | 16 -- > > > .../wireless/mediatek/mt76/mt76x2/usb_main.c | 22 ++- > > > 2 files changed, 21 insertions(+), 17 deletions(-) > > > > > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c > > > b/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c > > > index e416eee6a306..3a1467326f4d 100644 > > > --- a/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c > > > +++ b/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c > > > @@ -54,14 +54,14 @@ mt76x2_set_channel(struct mt76x02_dev *dev, struct > > > cfg80211_chan_def *chandef) > > > int ret; > > > > > > cancel_delayed_work_sync(&dev->cal_work); > > > > Since now you use mutex in mt76x2_phy_calibrate() you can remove > > cancel_delayed_work_sync() and drop other changes from this patch > > as releasing mutex just to acquire it in almost next step make > > no sense. > > I agree with you, the only difference is in that way we will perform phy > calibration even during scanning. If the there are no > objections I will post a v3 removing cancel_delayed_work_sync and > reworking patch 3/4 Oh, calibration work should not be done during scanning, so cancel cal_work should be added to .sw_scan_start() callback or this patch should stay unchanged. Stanislaw
Re: [PATCH 3/4] mt76: mt76x2: move mutex_lock inside mt76x2_set_channel
On Sat, May 11, 2019 at 12:17:53PM +0200, Lorenzo Bianconi wrote: > This is a preliminary patch to run mt76x02_edcca_init atomically > > Signed-off-by: Lorenzo Bianconi > --- > .../wireless/mediatek/mt76/mt76x2/pci_main.c | 16 -- > .../wireless/mediatek/mt76/mt76x2/usb_main.c | 22 ++- > 2 files changed, 21 insertions(+), 17 deletions(-) > > diff --git a/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c > b/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c > index e416eee6a306..3a1467326f4d 100644 > --- a/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c > +++ b/drivers/net/wireless/mediatek/mt76/mt76x2/pci_main.c > @@ -54,14 +54,14 @@ mt76x2_set_channel(struct mt76x02_dev *dev, struct > cfg80211_chan_def *chandef) > int ret; > > cancel_delayed_work_sync(&dev->cal_work); Since now you use mutex in mt76x2_phy_calibrate() you can remove cancel_delayed_work_sync() and drop other changes from this patch as releasing mutex just to acquire it in almost next step make no sense. Stanislaw
