Re: mt76x0 bug report
On Tue, Sep 4, 2018 at 5:11 PM Lorenzo Bianconi wrote: > > > > > I have one mt76x2u (Alfa AWUS036ACM) and a few mt76x0. > > > > I've noticed two additional issues in my testing. > > > > First issue, is that it appears the mt76x0 devices don't work properly > > in monitor mode. Sometimes they seem to monitor one channel properly, > > but nothing else. The mt76x2u works great, channel control, lots of > > packets, etc. > > Could you elaborate a little bit please? how can you reproduce the issue? > just add an interface in monitor mode and run a scan? Correct, standard stuff, use iw to create a monitor mode interface, use iw to remove managed mode interface, run some tool such as kismet or airodump-ng or even wireshark. > > > > > Second issue, and frankly a very minor one, is that the TP-Link T1U > > (which is a 5GHz only device) still thinks it has support for 2.4GHz, > > both in managed and monitor mode. > > I guess it depends on eeprom values. Could you please enable debug > messages a paste > syslog output? I don't see a mediatek specific debug near the driver selection in menuconfig, what debug messages do you want me to enable and how? Thanks, Zero > > > > > Lastly, I already mentioned in the other thread, but it would be > > awesome if firmware version was available to ethtool. > > > > Ack, I will take care of it > Regards, > > Lorenzo > > > Thanks for all your hard work on this, > > Zero
[bug report] mt76: unify wait_for_mac
Hello Stanislaw Gruszka,
The patch 2735a6dd7df3: "mt76: unify wait_for_mac" from Aug 29, 2018,
leads to the following static checker warning:
drivers/net/wireless/mediatek/mt76/mt76x02_mac.h:60 mt76x02_wait_for_mac()
warn: signedness bug returning '(-5)'
drivers/net/wireless/mediatek/mt76/mt76x02_mac.h
53 static inline bool mt76x02_wait_for_mac(struct mt76_dev *dev)
54 {
55 const u32 MAC_CSR0 = 0x1000;
56 int i;
57
58 for (i = 0; i < 500; i++) {
59 if (test_bit(MT76_REMOVED, >state))
60 return -EIO;
This is supposed to be true or false.
61
62 switch (dev->bus->rr(dev, MAC_CSR0)) {
63 case 0:
64 case ~0:
65 break;
66 default:
67 return true;
68 }
69 usleep_range(5000, 1);
70 }
71 return false;
72 }
regards,
dan carpenter
[PATCH] cfg80211: Address some corner cases in scan result channel updating
cfg80211_get_bss_channel() is used to update the RX channel based on the
available frame payload information (channel number from DSSS Parameter
Set element or HT Operation element). This is needed on 2.4 GHz channels
where frames may be received on neighboring channels due to overlapping
frequency range.
This might of some use on the 5 GHz band in some corner cases, but
things are more complex there since there is no n:1 or 1:n mapping
between channel numbers and frequencies due to multiple different
starting frequencies in different operating classes. This could result
in ieee80211_channel_to_frequency() returning incorrect frequency and
ieee80211_get_channel() returning incorrect channel information (or
indication of no match). In the previous implementation, this could
result in some scan results being dropped completely, e.g., for the 4.9
GHz channels. That prevented connection to such BSSs.
Fix this by using the driver-provided channel pointer if
ieee80211_get_channel() does not find matching channel data for the
channel number in the frame payload and if the scan is done with 5 MHz
or 10 MHz channel bandwidth. While doing this, also add comments
describing what the function is trying to achieve to make it easier to
understand what happens here and why.
Signed-off-by: Jouni Malinen
---
net/wireless/scan.c | 58 -
1 file changed, 49 insertions(+), 9 deletions(-)
diff --git a/net/wireless/scan.c b/net/wireless/scan.c
index d36c3eb..d0e7472 100644
--- a/net/wireless/scan.c
+++ b/net/wireless/scan.c
@@ -1058,13 +1058,23 @@ cfg80211_bss_update(struct cfg80211_registered_device
*rdev,
return NULL;
}
+/*
+ * Update RX channel information based on the available frame payload
+ * information. This is mainly for the 2.4 GHz band where frames can be
received
+ * from neighboring channels and the Beacon frames use the DSSS Parameter Set
+ * element to indicate the current (transmitting) channel, but this might also
+ * be needed on other bands if RX frequency does not match with the actual
+ * operating channel of a BSS.
+ */
static struct ieee80211_channel *
cfg80211_get_bss_channel(struct wiphy *wiphy, const u8 *ie, size_t ielen,
-struct ieee80211_channel *channel)
+struct ieee80211_channel *channel,
+enum nl80211_bss_scan_width scan_width)
{
const u8 *tmp;
u32 freq;
int channel_number = -1;
+ struct ieee80211_channel *alt_channel;
tmp = cfg80211_find_ie(WLAN_EID_DS_PARAMS, ie, ielen);
if (tmp && tmp[1] == 1) {
@@ -1078,16 +1088,45 @@ cfg80211_get_bss_channel(struct wiphy *wiphy, const u8
*ie, size_t ielen,
}
}
- if (channel_number < 0)
+ if (channel_number < 0) {
+ /* No channel information in frame payload */
return channel;
+ }
freq = ieee80211_channel_to_frequency(channel_number, channel->band);
- channel = ieee80211_get_channel(wiphy, freq);
- if (!channel)
- return NULL;
- if (channel->flags & IEEE80211_CHAN_DISABLED)
+ alt_channel = ieee80211_get_channel(wiphy, freq);
+ if (!alt_channel) {
+ if (channel->band == NL80211_BAND_2GHZ) {
+ /*
+* Better not allow unexpected channels when that could
+* be going beyond the 1-11 range (e.g., discovering
+* BSS on channel 12 when radio is configured for
+* channel 11.
+*/
+ return NULL;
+ }
+
+ /* No match for the payload channel number - ignore it */
+ return channel;
+ }
+
+ if (scan_width == NL80211_BSS_CHAN_WIDTH_10 ||
+ scan_width == NL80211_BSS_CHAN_WIDTH_5) {
+ /*
+* Ignore channel number in 5 and 10 MHz channels where there
+* may not be an n:1 or 1:n mapping between frequencies and
+* channel numbers.
+*/
+ return channel;
+ }
+
+ /*
+* Use the channel determined through the payload channel number
+* instead of the RX channel reported by the driver.
+*/
+ if (alt_channel->flags & IEEE80211_CHAN_DISABLED)
return NULL;
- return channel;
+ return alt_channel;
}
/* Returned bss is reference counted and must be cleaned up appropriately. */
@@ -1112,7 +1151,8 @@ cfg80211_inform_bss_data(struct wiphy *wiphy,
(data->signal < 0 || data->signal > 100)))
return NULL;
- channel = cfg80211_get_bss_channel(wiphy, ie, ielen, data->chan);
+ channel = cfg80211_get_bss_channel(wiphy, ie, ielen, data->chan,
+ data->scan_width);
if (!channel)
Re: [PATCH 0/6] brcmfmac: fix 160MHz support and monitor mode
On Wed, 5 Sep 2018 at 13:06, Arend van Spriel wrote: > On 8/29/2018 10:17 PM, Rafał Miłecki wrote: > > Hi Arend, > > > > On Mon, 25 Jun 2018 at 10:31, Arend van Spriel > > wrote: > >> On 6/25/2018 6:40 AM, Rafał Miłecki wrote: > >>> On Sun, 24 Jun 2018 at 13:48, Rafał Miłecki wrote: > On Fri, 22 Jun 2018 at 20:45, Arend van Spriel > wrote: > > Here a couple of patches in preparation of monitor mode support. It > > is mostly about querying firmware for support. While at it I stumbled > > upon the fact that 160MHz was not completely implemented in the driver > > so there is a patch for that as well. > > > > The first two patches are actually some changes to the patches that > > Rafal submitted. So this series depend on: > > > > [V3,2/2] brcmfmac: handle monitor mode marked msgbuf packets [1] > > > > These apply to the master branch of the wireless-drivers-next > > repository. > > > > [1] https://patchwork.kernel.org/patch/10474799/ > > I find it pointless to submit fixes for patches that weren't accepted > yet. Let's work on improving original patches while they are still > pending. > >>> > >>> I sent V4 with changes pointed our by Arend. That obsoletes all (?) > >>> monitor patches from this patchset. I believe it was cleaner to > >>> improve original patchset (not pushed yet). > >>> > >>> My suggestion is to apply patches 3/6 and 4/6 (they aren't monitor > >>> related) and drop the others. > >> > >> Well. Given that Kalle prefers applying "all-or-nothing" I will resubmit > >> the series addressing the issues you found. > > > > Would you have a moment to resubmit these 2 patches or do you mind if I do > > that? > > Found a moment. Should be in patchwork now. Thank you! :) -- Rafał
Re: [PATCH] mac80211: TDLS: fix skb queue/priority assignment
Johannes Berg writes: > On Wed, 2018-09-05 at 14:32 +0200, Toke Høiland-Jørgensen wrote: >> Johannes Berg writes: >> >> > On Wed, 2018-09-05 at 13:41 +0200, Johannes Berg wrote: >> > > On Wed, 2018-09-05 at 13:40 +0200, Toke Høiland-Jørgensen wrote: >> > > > >> > > > Guess we'll have to deal with everything else if we ever move >> > > > management >> > > > frames onto the TXQ path as well... >> > > >> > > Depends on whether we care for management frame priorities or not ... so >> > > far we haven't really. >> > >> > Actually, for the most part we have implemented that properly. Except >> > for the TXQ I added for bufferable management ... oh well, I think we're >> > the only user thereof now. >> > >> > I'm not sure we want to have a TXQ per TID for management, that seems >> > overkill. But I'm also not sure how to solve this otherwise ... >> >> Graft it to an existing TXQ, similar to how the fragments queue is used >> now? Saves a TXQ at the expense of having to special-case it... > > The problem isn't so much how we handle it in mac80211 for the queueing, > but how we deal with things like A-MSDU and how we present it to the > driver ... for iwlwifi at least we'd really like to have only data > frames so we can map it directly to the hardware queue ... Ah, I see. No, then just putting them at the head of a different TXQ probably won't work... Are you mapping TXQs to hardware queues dynamically as they empty and re-fill? Presumably you'll have cases where you don't have enough HWQs? -Toke
Re: [PATCH] mac80211: TDLS: fix skb queue/priority assignment
On Wed, 2018-09-05 at 14:32 +0200, Toke Høiland-Jørgensen wrote: > Johannes Berg writes: > > > On Wed, 2018-09-05 at 13:41 +0200, Johannes Berg wrote: > > > On Wed, 2018-09-05 at 13:40 +0200, Toke Høiland-Jørgensen wrote: > > > > > > > > Guess we'll have to deal with everything else if we ever move management > > > > frames onto the TXQ path as well... > > > > > > Depends on whether we care for management frame priorities or not ... so > > > far we haven't really. > > > > Actually, for the most part we have implemented that properly. Except > > for the TXQ I added for bufferable management ... oh well, I think we're > > the only user thereof now. > > > > I'm not sure we want to have a TXQ per TID for management, that seems > > overkill. But I'm also not sure how to solve this otherwise ... > > Graft it to an existing TXQ, similar to how the fragments queue is used > now? Saves a TXQ at the expense of having to special-case it... The problem isn't so much how we handle it in mac80211 for the queueing, but how we deal with things like A-MSDU and how we present it to the driver ... for iwlwifi at least we'd really like to have only data frames so we can map it directly to the hardware queue ... johannes
Re: [PATCH] mac80211: TDLS: fix skb queue/priority assignment
On Wed, 2018-09-05 at 13:41 +0200, Johannes Berg wrote: > On Wed, 2018-09-05 at 13:40 +0200, Toke Høiland-Jørgensen wrote: > > > > Guess we'll have to deal with everything else if we ever move management > > frames onto the TXQ path as well... > > Depends on whether we care for management frame priorities or not ... so > far we haven't really. Actually, for the most part we have implemented that properly. Except for the TXQ I added for bufferable management ... oh well, I think we're the only user thereof now. I'm not sure we want to have a TXQ per TID for management, that seems overkill. But I'm also not sure how to solve this otherwise ... johannes
Re: [PATCH] mac80211: TDLS: fix skb queue/priority assignment
On Wed, 2018-09-05 at 13:40 +0200, Toke Høiland-Jørgensen wrote: > > Guess we'll have to deal with everything else if we ever move management > frames onto the TXQ path as well... Depends on whether we care for management frame priorities or not ... so far we haven't really. johannes
Re: [PATCH] mac80211: TDLS: fix skb queue/priority assignment
Johannes Berg writes: > On Wed, 2018-09-05 at 13:34 +0200, Johannes Berg wrote: >> From: Johannes Berg >> >> If the TDLS setup happens over a connection to an AP that >> doesn't have QoS, we nevertheless assign a non-zero TID >> (skb->priority) and queue mapping, which may confuse us or >> drivers later. >> >> Fix it by just assigning the special skb->priority and then >> using ieee80211_select_queue() just like other data frames >> would go through. >> >> Signed-off-by: Johannes Berg >> --- >> OK, this addresses the case I was worried about - perhaps >> better take this than the txq patch? (This is just you talking to yourself, right? :)) > I really only found this. All the other cases aren't doing real data > frames, only null-data and management. So I guess for those we're OK, > and this was the only special case after all. SGTM. Guess we'll have to deal with everything else if we ever move management frames onto the TXQ path as well... -Toke
[PATCH] mac80211: TDLS: fix skb queue/priority assignment
From: Johannes Berg
If the TDLS setup happens over a connection to an AP that
doesn't have QoS, we nevertheless assign a non-zero TID
(skb->priority) and queue mapping, which may confuse us or
drivers later.
Fix it by just assigning the special skb->priority and then
using ieee80211_select_queue() just like other data frames
would go through.
Signed-off-by: Johannes Berg
---
OK, this addresses the case I was worried about - perhaps
better take this than the txq patch?
---
net/mac80211/tdls.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/net/mac80211/tdls.c b/net/mac80211/tdls.c
index 5cd5e6e5834e..6c647f425e05 100644
--- a/net/mac80211/tdls.c
+++ b/net/mac80211/tdls.c
@@ -16,6 +16,7 @@
#include "ieee80211_i.h"
#include "driver-ops.h"
#include "rate.h"
+#include "wme.h"
/* give usermode some time for retries in setting up the TDLS session */
#define TDLS_PEER_SETUP_TIMEOUT(15 * HZ)
@@ -1010,14 +1011,13 @@ ieee80211_tdls_prep_mgmt_packet(struct wiphy *wiphy,
struct net_device *dev,
switch (action_code) {
case WLAN_TDLS_SETUP_REQUEST:
case WLAN_TDLS_SETUP_RESPONSE:
- skb_set_queue_mapping(skb, IEEE80211_AC_BK);
- skb->priority = 2;
+ skb->priority = 256 + 2;
break;
default:
- skb_set_queue_mapping(skb, IEEE80211_AC_VI);
- skb->priority = 5;
+ skb->priority = 256 + 5;
break;
}
+ skb_set_queue_mapping(skb, ieee80211_select_queue(sdata, skb));
/*
* Set the WLAN_TDLS_TEARDOWN flag to indicate a teardown in progress.
--
2.14.4
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
Johannes Berg writes: > On Wed, 2018-09-05 at 13:07 +0200, Toke Høiland-Jørgensen wrote: > >> > Felix wasn't really convinced, I think. He also pointed out some drivers >> > use skb->priority without checking anything, but I'm not sure we can >> > really squash all the cases of setting skb priority easily? >> >> ~/build/linux/drivers/net/wireless $ git grep 'skb->priority = ' >> ath/ath9k/channel.c: skb->priority = 7; >> broadcom/brcm80211/brcmfmac/core.c: skb->priority = >> cfg80211_classify8021d(skb, NULL); >> broadcom/brcm80211/brcmutil/utils.c: skb->priority = 0; >> intel/ipw2x00/libipw_tx.c: skb->priority = libipw_classify(skb); >> marvell/mwifiex/cfg80211.c: skb->priority = LOW_PRIO_TID; >> marvell/mwifiex/main.c: skb->priority = cfg80211_classify8021d(skb, >> NULL); >> marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_BK; >> marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_VI; >> marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_VI; >> rsi/rsi_91x_core.c: skb->priority = q_num; >> rsi/rsi_91x_core.c: skb->priority = TID_TO_WME_AC(tid); >> rsi/rsi_91x_core.c: skb->priority = BE_Q; >> rsi/rsi_91x_core.c: skb->priority = q_num; >> rsi/rsi_91x_hal.c: skb->priority = VO_Q; >> rsi/rsi_91x_mgmt.c: skb->priority = MGMT_SOFT_Q; >> ti/wlcore/main.c:skb->priority = WL1271_TID_MGMT; >> >> Doesn't seem *that* excessive? Obviously there could be other cases, and >> I haven't looked closer at any of those... > > That's assignments. For assignments, I guess you'd have to look at > net/mac80211/. It's not that excessive either, but it's not in all > places trivial to determine ... Ah, sorry, I read that as "some drivers *set* skb->priority without checking"... -Toke
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
On Wed, 2018-09-05 at 13:07 +0200, Toke Høiland-Jørgensen wrote: > > Felix wasn't really convinced, I think. He also pointed out some drivers > > use skb->priority without checking anything, but I'm not sure we can > > really squash all the cases of setting skb priority easily? > > ~/build/linux/drivers/net/wireless $ git grep 'skb->priority = ' > ath/ath9k/channel.c: skb->priority = 7; > broadcom/brcm80211/brcmfmac/core.c: skb->priority = > cfg80211_classify8021d(skb, NULL); > broadcom/brcm80211/brcmutil/utils.c: skb->priority = 0; > intel/ipw2x00/libipw_tx.c:skb->priority = libipw_classify(skb); > marvell/mwifiex/cfg80211.c: skb->priority = LOW_PRIO_TID; > marvell/mwifiex/main.c: skb->priority = cfg80211_classify8021d(skb, > NULL); > marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_BK; > marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_VI; > marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_VI; > rsi/rsi_91x_core.c: skb->priority = q_num; > rsi/rsi_91x_core.c: skb->priority = TID_TO_WME_AC(tid); > rsi/rsi_91x_core.c: skb->priority = BE_Q; > rsi/rsi_91x_core.c: skb->priority = q_num; > rsi/rsi_91x_hal.c:skb->priority = VO_Q; > rsi/rsi_91x_mgmt.c: skb->priority = MGMT_SOFT_Q; > ti/wlcore/main.c: skb->priority = WL1271_TID_MGMT; > > Doesn't seem *that* excessive? Obviously there could be other cases, and > I haven't looked closer at any of those... That's assignments. For assignments, I guess you'd have to look at net/mac80211/. It's not that excessive either, but it's not in all places trivial to determine ... Whatever, I'll just try, give me a minute :) > Does it matter for the drivers that don't use TXQs? Probably. johannes
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
Johannes Berg writes: > On Wed, 2018-09-05 at 11:56 +0200, Toke Høiland-Jørgensen wrote: > >> > So basically this gets rid of a corner case that we shouldn't have. >> > Either we should decide that using different TXQs is *always* correct >> > for non-QoS, or - what I thought - that this isn't worth it, and then we >> > should *never* do it. >> >> Yeah, I agree that this is not worth it. The queue is already >> FQ-CoDel'ed, which gives us most of the benefit of QoS anyway :) > > So do I read that as a tentative ack? :) Yeah, guess so :) > Felix wasn't really convinced, I think. He also pointed out some drivers > use skb->priority without checking anything, but I'm not sure we can > really squash all the cases of setting skb priority easily? ~/build/linux/drivers/net/wireless $ git grep 'skb->priority = ' ath/ath9k/channel.c:skb->priority = 7; broadcom/brcm80211/brcmfmac/core.c: skb->priority = cfg80211_classify8021d(skb, NULL); broadcom/brcm80211/brcmutil/utils.c:skb->priority = 0; intel/ipw2x00/libipw_tx.c: skb->priority = libipw_classify(skb); marvell/mwifiex/cfg80211.c: skb->priority = LOW_PRIO_TID; marvell/mwifiex/main.c: skb->priority = cfg80211_classify8021d(skb, NULL); marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_BK; marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_VI; marvell/mwifiex/tdls.c: skb->priority = MWIFIEX_PRIO_VI; rsi/rsi_91x_core.c: skb->priority = q_num; rsi/rsi_91x_core.c: skb->priority = TID_TO_WME_AC(tid); rsi/rsi_91x_core.c: skb->priority = BE_Q; rsi/rsi_91x_core.c: skb->priority = q_num; rsi/rsi_91x_hal.c: skb->priority = VO_Q; rsi/rsi_91x_mgmt.c: skb->priority = MGMT_SOFT_Q; ti/wlcore/main.c: skb->priority = WL1271_TID_MGMT; Doesn't seem *that* excessive? Obviously there could be other cases, and I haven't looked closer at any of those... Does it matter for the drivers that don't use TXQs? -Toke
Re: [PATCH 0/6] brcmfmac: fix 160MHz support and monitor mode
On 8/29/2018 10:17 PM, Rafał Miłecki wrote: Hi Arend, On Mon, 25 Jun 2018 at 10:31, Arend van Spriel wrote: On 6/25/2018 6:40 AM, Rafał Miłecki wrote: On Sun, 24 Jun 2018 at 13:48, Rafał Miłecki wrote: On Fri, 22 Jun 2018 at 20:45, Arend van Spriel wrote: Here a couple of patches in preparation of monitor mode support. It is mostly about querying firmware for support. While at it I stumbled upon the fact that 160MHz was not completely implemented in the driver so there is a patch for that as well. The first two patches are actually some changes to the patches that Rafal submitted. So this series depend on: [V3,2/2] brcmfmac: handle monitor mode marked msgbuf packets [1] These apply to the master branch of the wireless-drivers-next repository. [1] https://patchwork.kernel.org/patch/10474799/ I find it pointless to submit fixes for patches that weren't accepted yet. Let's work on improving original patches while they are still pending. I sent V4 with changes pointed our by Arend. That obsoletes all (?) monitor patches from this patchset. I believe it was cleaner to improve original patchset (not pushed yet). My suggestion is to apply patches 3/6 and 4/6 (they aren't monitor related) and drop the others. Well. Given that Kalle prefers applying "all-or-nothing" I will resubmit the series addressing the issues you found. Would you have a moment to resubmit these 2 patches or do you mind if I do that? Found a moment. Should be in patchwork now. Regards, Arend
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
On Wed, 2018-09-05 at 11:56 +0200, Toke Høiland-Jørgensen wrote: > > So basically this gets rid of a corner case that we shouldn't have. > > Either we should decide that using different TXQs is *always* correct > > for non-QoS, or - what I thought - that this isn't worth it, and then we > > should *never* do it. > > Yeah, I agree that this is not worth it. The queue is already > FQ-CoDel'ed, which gives us most of the benefit of QoS anyway :) So do I read that as a tentative ack? :) Felix wasn't really convinced, I think. He also pointed out some drivers use skb->priority without checking anything, but I'm not sure we can really squash all the cases of setting skb priority easily? johannes
Re: [PATCH] mac80211: fix pending queue hang due to TX_DROP
Bob Copeland writes: > In our environment running lots of mesh nodes, we are seeing the > pending queue hang periodically, with the debugfs queues file showing > lines such as: > > 00: 0x/348 > > i.e. there are a large number of frames but no stop reason set. > > One way this could happen is if queue processing from the pending > tasklet exited early without processing all frames, and without having > some future event (incoming frame, stop reason flag, ...) to reschedule > it. > > Exactly this can occur today if ieee80211_tx() returns false due to > packet drops or power-save buffering in the tx handlers. In the > past, this function would return true in such cases, and the change > to false doesn't seem to be intentional. Can confirm that this was not intentional; nice catch! :) Acked-by: Toke Høiland-Jørgensen -Toke
[PATCH] mac80211: fix pending queue hang due to TX_DROP
In our environment running lots of mesh nodes, we are seeing the
pending queue hang periodically, with the debugfs queues file showing
lines such as:
00: 0x/348
i.e. there are a large number of frames but no stop reason set.
One way this could happen is if queue processing from the pending
tasklet exited early without processing all frames, and without having
some future event (incoming frame, stop reason flag, ...) to reschedule
it.
Exactly this can occur today if ieee80211_tx() returns false due to
packet drops or power-save buffering in the tx handlers. In the
past, this function would return true in such cases, and the change
to false doesn't seem to be intentional. Fix this case by reverting
to the previous behavior.
Fixes: bb42f2d13ffc ("mac80211: Move reorder-sensitive TX handlers to after TXQ
dequeue")
Signed-off-by: Bob Copeland
---
net/mac80211/tx.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index e88547842239..6b83dc397c3e 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1907,7 +1907,7 @@ static bool ieee80211_tx(struct ieee80211_sub_if_data
*sdata,
sdata->vif.hw_queue[skb_get_queue_mapping(skb)];
if (invoke_tx_handlers_early())
- return false;
+ return true;
if (ieee80211_queue_skb(local, sdata, tx.sta, tx.skb))
return true;
--
2.11.0
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
Johannes Berg writes: > On Wed, 2018-09-05 at 11:47 +0200, Toke Høiland-Jørgensen wrote: >> Johannes Berg writes: >> >> > From: Johannes Berg >> > >> > Some frames may have a non-zero skb->priority assigned by >> > mac80211 internally, e.g. TDLS setup frames, regardless of >> > support for QoS. >> > >> > Currently, we set skb->priority to 0 for all data frames. >> > Note that there's a comment that this is "required for >> > correct WPA/11i MIC", but that doesn't seem true as we use >> > >> > if (ieee80211_is_data_qos(hdr->frame_control)) >> > qos_tid = ieee80211_get_tid(hdr); >> > else >> > qos_tid = 0; >> > >> > in the code there. We could therefore reconsider this, but >> > it seems like unnecessary complexity for the unlikely (and >> > not very useful) case of not having QoS on the connection. >> > >> > This situation then causes something strange - most data >> > frames will go on TXQ for TID 0 for non-QoS connections, >> > but very few exceptions that are internally generated will >> > go on another TXQ, possibly causing confusion. >> >> What kind of confusion are you seeing? Reordering issues, or something >> else? > > I haven't actually been able to test this... > > But with the iwlwifi work we're doing, at the very least we'd waste a > hardware queue for the case that basically never happens, since you'd > end up putting these frames (that are very few) on a separate TXQ and > thus hardware queue. Ah, right, you're doing 1-to-1 TXQ-to-HWQ mapping. Gotcha. > You could argue we should explicitly _not_ do this, but then we should > also set skb->priority to be non-zero for non-QoS stations. Then we > could benefit from some form of QoS (between the TXQs) for non-QoS > connections, but that seems pretty complex and doesn't seem worth it > since all connections that want anything from HT/11n and newer need QoS > anyway. > > So basically this gets rid of a corner case that we shouldn't have. > Either we should decide that using different TXQs is *always* correct > for non-QoS, or - what I thought - that this isn't worth it, and then we > should *never* do it. Yeah, I agree that this is not worth it. The queue is already FQ-CoDel'ed, which gives us most of the benefit of QoS anyway :) -Toke
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
On Wed, 2018-09-05 at 11:47 +0200, Toke Høiland-Jørgensen wrote: > Johannes Berg writes: > > > From: Johannes Berg > > > > Some frames may have a non-zero skb->priority assigned by > > mac80211 internally, e.g. TDLS setup frames, regardless of > > support for QoS. > > > > Currently, we set skb->priority to 0 for all data frames. > > Note that there's a comment that this is "required for > > correct WPA/11i MIC", but that doesn't seem true as we use > > > > if (ieee80211_is_data_qos(hdr->frame_control)) > > qos_tid = ieee80211_get_tid(hdr); > > else > > qos_tid = 0; > > > > in the code there. We could therefore reconsider this, but > > it seems like unnecessary complexity for the unlikely (and > > not very useful) case of not having QoS on the connection. > > > > This situation then causes something strange - most data > > frames will go on TXQ for TID 0 for non-QoS connections, > > but very few exceptions that are internally generated will > > go on another TXQ, possibly causing confusion. > > What kind of confusion are you seeing? Reordering issues, or something > else? I haven't actually been able to test this... But with the iwlwifi work we're doing, at the very least we'd waste a hardware queue for the case that basically never happens, since you'd end up putting these frames (that are very few) on a separate TXQ and thus hardware queue. You could argue we should explicitly _not_ do this, but then we should also set skb->priority to be non-zero for non-QoS stations. Then we could benefit from some form of QoS (between the TXQs) for non-QoS connections, but that seems pretty complex and doesn't seem worth it since all connections that want anything from HT/11n and newer need QoS anyway. So basically this gets rid of a corner case that we shouldn't have. Either we should decide that using different TXQs is *always* correct for non-QoS, or - what I thought - that this isn't worth it, and then we should *never* do it. johannes
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
Johannes Berg writes: > From: Johannes Berg > > Some frames may have a non-zero skb->priority assigned by > mac80211 internally, e.g. TDLS setup frames, regardless of > support for QoS. > > Currently, we set skb->priority to 0 for all data frames. > Note that there's a comment that this is "required for > correct WPA/11i MIC", but that doesn't seem true as we use > > if (ieee80211_is_data_qos(hdr->frame_control)) > qos_tid = ieee80211_get_tid(hdr); > else > qos_tid = 0; > > in the code there. We could therefore reconsider this, but > it seems like unnecessary complexity for the unlikely (and > not very useful) case of not having QoS on the connection. > > This situation then causes something strange - most data > frames will go on TXQ for TID 0 for non-QoS connections, > but very few exceptions that are internally generated will > go on another TXQ, possibly causing confusion. What kind of confusion are you seeing? Reordering issues, or something else? -Toke
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
On Wed, 2018-09-05 at 10:06 +0200, Arend van Spriel wrote:
>
> > +++ b/net/mac80211/tx.c
> > @@ -1260,7 +1260,10 @@ static struct txq_info *ieee80211_get_txq(struct
> > ieee80211_local *local,
> > txq = sta->sta.txq[IEEE80211_NUM_TIDS];
> > }
> > } else if (sta) {
> > - u8 tid = skb->priority & IEEE80211_QOS_CTL_TID_MASK;
> > + u8 tid = 0;
> > +
> > + if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA))
> > + tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
>
> Is the use of different mask intentional here? Just a quick glance so
> did not look into it further.
Ah, I forgot to mention that in the commit log. That just aligns it with
most of the other code, but since we never have values other than 0-7 it
doesn't actually matter.
johannes
Re: [PATCH] mac80211: use non-zero TID only for QoS frames
On 9/5/2018 10:00 AM, Johannes Berg wrote:
From: Johannes Berg
Some frames may have a non-zero skb->priority assigned by
mac80211 internally, e.g. TDLS setup frames, regardless of
support for QoS.
Currently, we set skb->priority to 0 for all data frames.
Note that there's a comment that this is "required for
correct WPA/11i MIC", but that doesn't seem true as we use
if (ieee80211_is_data_qos(hdr->frame_control))
qos_tid = ieee80211_get_tid(hdr);
else
qos_tid = 0;
in the code there. We could therefore reconsider this, but
it seems like unnecessary complexity for the unlikely (and
not very useful) case of not having QoS on the connection.
This situation then causes something strange - most data
frames will go on TXQ for TID 0 for non-QoS connections,
but very few exceptions that are internally generated will
go on another TXQ, possibly causing confusion.
Avoid this confusion by putting non-QoS data frames into
TID 0 regardless of the skb->priority.
Signed-off-by: Johannes Berg
---
net/mac80211/tx.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 5b6e06ad35ff..6540595addd1 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1260,7 +1260,10 @@ static struct txq_info *ieee80211_get_txq(struct
ieee80211_local *local,
txq = sta->sta.txq[IEEE80211_NUM_TIDS];
}
} else if (sta) {
- u8 tid = skb->priority & IEEE80211_QOS_CTL_TID_MASK;
+ u8 tid = 0;
+
+ if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA))
+ tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
Is the use of different mask intentional here? Just a quick glance so
did not look into it further.
Gr. AvS
[PATCH] mac80211: use non-zero TID only for QoS frames
From: Johannes Berg
Some frames may have a non-zero skb->priority assigned by
mac80211 internally, e.g. TDLS setup frames, regardless of
support for QoS.
Currently, we set skb->priority to 0 for all data frames.
Note that there's a comment that this is "required for
correct WPA/11i MIC", but that doesn't seem true as we use
if (ieee80211_is_data_qos(hdr->frame_control))
qos_tid = ieee80211_get_tid(hdr);
else
qos_tid = 0;
in the code there. We could therefore reconsider this, but
it seems like unnecessary complexity for the unlikely (and
not very useful) case of not having QoS on the connection.
This situation then causes something strange - most data
frames will go on TXQ for TID 0 for non-QoS connections,
but very few exceptions that are internally generated will
go on another TXQ, possibly causing confusion.
Avoid this confusion by putting non-QoS data frames into
TID 0 regardless of the skb->priority.
Signed-off-by: Johannes Berg
---
net/mac80211/tx.c | 5 -
1 file changed, 4 insertions(+), 1 deletion(-)
diff --git a/net/mac80211/tx.c b/net/mac80211/tx.c
index 5b6e06ad35ff..6540595addd1 100644
--- a/net/mac80211/tx.c
+++ b/net/mac80211/tx.c
@@ -1260,7 +1260,10 @@ static struct txq_info *ieee80211_get_txq(struct
ieee80211_local *local,
txq = sta->sta.txq[IEEE80211_NUM_TIDS];
}
} else if (sta) {
- u8 tid = skb->priority & IEEE80211_QOS_CTL_TID_MASK;
+ u8 tid = 0;
+
+ if (hdr->frame_control & cpu_to_le16(IEEE80211_STYPE_QOS_DATA))
+ tid = skb->priority & IEEE80211_QOS_CTL_TAG1D_MASK;
if (!sta->uploaded)
return NULL;
--
2.14.4
[PATCH V2 1/2] brcmfmac: fix for proper support of 160MHz bandwidth
Decoding of firmware channel information was not complete for 160MHz
support. This resulted in the following warning:
WARNING: CPU: 2 PID: at .../broadcom/brcm80211/brcmutil/d11.c:196
brcmu_d11ac_decchspec+0x2e/0x100 [brcmutil]
Modules linked in: brcmfmac(O) brcmutil(O) sha256_generic cfg80211 ...
CPU: 2 PID: Comm: kworker/2:0 Tainted: G O
4.17.0-wt-testing-x64-2-gf1bed50 #1
Hardware name: Dell Inc. Latitude E6410/07XJP9, BIOS A07 02/15/2011
Workqueue: events request_firmware_work_func
RIP: 0010:brcmu_d11ac_decchspec+0x2e/0x100 [brcmutil]
RSP: 0018:c9047bd0 EFLAGS: 00010206
RAX: e832 RBX: 8801146fe910 RCX: 8801146fd3c0
RDX: 2800 RSI: 0070 RDI: c9047c30
RBP: c9047bd0 R08: R09: a0798c80
R10: 88012bca55e0 R11: 880110a4ea00 R12: 8801146f8000
R13: c9047c30 R14: 8801146fe930 R15: 8801138e02e0
FS: () GS:88012bc8() knlGS:
CS: 0010 DS: ES: CR0: 80050033
CR2: 7f18ce8b8070 CR3: 0200a003 CR4: 000206e0
Call Trace:
brcmf_setup_wiphybands+0x212/0x780 [brcmfmac]
brcmf_cfg80211_attach+0xae2/0x11a0 [brcmfmac]
brcmf_attach+0x1fc/0x4b0 [brcmfmac]
? __kmalloc+0x13c/0x1c0
brcmf_pcie_setup+0x99b/0xe00 [brcmfmac]
brcmf_fw_request_done+0x16a/0x1f0 [brcmfmac]
request_firmware_work_func+0x36/0x60
process_one_work+0x146/0x350
worker_thread+0x4a/0x3b0
kthread+0x102/0x140
? process_one_work+0x350/0x350
? kthread_bind+0x20/0x20
ret_from_fork+0x35/0x40
Code: 66 90 0f b7 07 55 48 89 e5 89 c2 88 47 02 88 47 03 66 81 e2 00 38
66 81 fa 00 18 74 6e 66 81 fa 00 20 74 39 66 81 fa 00 10 74 14 <0f>
0b 66 25 00 c0 74 20 66 3d 00 c0 75 20 c6 47 04 01 5d c3 66
---[ end trace 550c46682415b26d ]---
brcmfmac: brcmf_construct_chaninfo: Ignoring unexpected firmware channel 50
This patch adds the missing stuff to properly handle this.
Reviewed-by: Hante Meuleman
Reviewed-by: Pieter-Paul Giesberts
Reviewed-by: Franky Lin
Signed-off-by: Arend van Spriel
---
.../net/wireless/broadcom/brcm80211/brcmutil/d11.c | 34 +-
.../broadcom/brcm80211/include/brcmu_wifi.h| 2 ++
2 files changed, 35 insertions(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c
b/drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c
index d8b79cb..e7584b8 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmutil/d11.c
@@ -77,6 +77,8 @@ static u16 d11ac_bw(enum brcmu_chan_bw bw)
return BRCMU_CHSPEC_D11AC_BW_40;
case BRCMU_CHAN_BW_80:
return BRCMU_CHSPEC_D11AC_BW_80;
+ case BRCMU_CHAN_BW_160:
+ return BRCMU_CHSPEC_D11AC_BW_160;
default:
WARN_ON(1);
}
@@ -190,8 +192,38 @@ static void brcmu_d11ac_decchspec(struct brcmu_chan *ch)
break;
}
break;
- case BRCMU_CHSPEC_D11AC_BW_8080:
case BRCMU_CHSPEC_D11AC_BW_160:
+ switch (ch->sb) {
+ case BRCMU_CHAN_SB_LLL:
+ ch->control_ch_num -= CH_70MHZ_APART;
+ break;
+ case BRCMU_CHAN_SB_LLU:
+ ch->control_ch_num -= CH_50MHZ_APART;
+ break;
+ case BRCMU_CHAN_SB_LUL:
+ ch->control_ch_num -= CH_30MHZ_APART;
+ break;
+ case BRCMU_CHAN_SB_LUU:
+ ch->control_ch_num -= CH_10MHZ_APART;
+ break;
+ case BRCMU_CHAN_SB_ULL:
+ ch->control_ch_num += CH_10MHZ_APART;
+ break;
+ case BRCMU_CHAN_SB_ULU:
+ ch->control_ch_num += CH_30MHZ_APART;
+ break;
+ case BRCMU_CHAN_SB_UUL:
+ ch->control_ch_num += CH_50MHZ_APART;
+ break;
+ case BRCMU_CHAN_SB_UUU:
+ ch->control_ch_num += CH_70MHZ_APART;
+ break;
+ default:
+ WARN_ON_ONCE(1);
+ break;
+ }
+ break;
+ case BRCMU_CHSPEC_D11AC_BW_8080:
default:
WARN_ON_ONCE(1);
break;
diff --git a/drivers/net/wireless/broadcom/brcm80211/include/brcmu_wifi.h
b/drivers/net/wireless/broadcom/brcm80211/include/brcmu_wifi.h
index 91fca79..dddebaa 100644
--- a/drivers/net/wireless/broadcom/brcm80211/include/brcmu_wifi.h
+++ b/drivers/net/wireless/broadcom/brcm80211/include/brcmu_wifi.h
@@ -29,6 +29,8 @@
#define CH_UPPER_SB0x01
#define CH_LOWER_SB0x02
#define CH_EWA_VALID
[PATCH V2 0/2] brcmfmac: fix for 160MHz and firmware capabilities
Resending two patches which apply to the master branch of the wireless-drivers-next repository. Arend van Spriel (2): brcmfmac: fix for proper support of 160MHz bandwidth brcmfmac: increase buffer for obtaining firmware capabilities .../wireless/broadcom/brcm80211/brcmfmac/feature.c | 2 +- .../net/wireless/broadcom/brcm80211/brcmutil/d11.c | 34 +- .../broadcom/brcm80211/include/brcmu_wifi.h| 2 ++ 3 files changed, 36 insertions(+), 2 deletions(-) -- 1.9.1
[PATCH V2 2/2] brcmfmac: increase buffer for obtaining firmware capabilities
When obtaining the firmware capability a buffer is provided of 512
bytes. However, if all features in firmware are supported the buffer
needs to be 565 bytes as otherwise truncated information is retrieved
from firmware. Increasing the buffer to 768 bytes on stack.
Reviewed-by: Hante Meuleman
Reviewed-by: Pieter-Paul Giesberts
Reviewed-by: Franky Lin
Signed-off-by: Arend van Spriel
---
drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
index 8347da6..4c5a399 100644
--- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
+++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/feature.c
@@ -178,7 +178,7 @@ static void brcmf_feat_iovar_data_set(struct brcmf_if *ifp,
ifp->fwil_fwerr = false;
}
-#define MAX_CAPS_BUFFER_SIZE 512
+#define MAX_CAPS_BUFFER_SIZE 768
static void brcmf_feat_firmware_capabilities(struct brcmf_if *ifp)
{
char caps[MAX_CAPS_BUFFER_SIZE];
--
1.9.1
