* Re: [PATCH v4] brcmfmac: Decrease 8021x_cnt for dropped packets
From: Per Förlin @ 2016-07-13 18:52 UTC (permalink / raw)
To: Arend Van Spriel; +Cc: linux-wireless, arend
In-Reply-To: <cdb613ba-d2b1-d9eb-9beb-2934cff96aa6@broadcom.com>
2016-07-13 13:20 GMT+02:00 Arend Van Spriel <arend.vanspriel@broadcom.com>:
> On 12-7-2016 12:23, Per Förlin wrote:
>> 2016-07-12 11:48 GMT+02:00 Arend Van Spriel <arend.vanspriel@broadcom.com>:
>>>
>>>
>>> On 12-7-2016 10:35, Per Förlin wrote:
>>>> 2016-07-06 11:53 GMT+02:00 Per Förlin <per.forlin@gmail.com>:
>>>>> I have now verified this patch on backports 4.4.
>>>>>
>>>>> 2016-04-12 23:55 GMT+02:00 <per.forlin@gmail.com>:
>>>>>> From: Per Forlin <per.forlin@gmail.com>
>>>>>>
>>>>>> This patch resolves an issue where pend_8021x_cnt was not decreased
>>>>>> on txfinalize. This caused brcmf_netdev_wait_pend8021x to timeout
>>>>>> because the counter indicated pending packets.
>>>>>>
>>>>>> WARNING: at .../brcmfmac/core.c:1289 brcmf_netdev_wait_pend8021x
>>>>>> (warn_slowpath_common)
>>>>>> (warn_slowpath_null)
>>>>>> (brcmf_netdev_wait_pend8021x [brcmfmac])
>>>>>> (send_key_to_dongle [brcmfmac])
>>>>>> (brcmf_cfg80211_del_key [brcmfmac])
>>>>>> (nl80211_del_key [cfg80211])
>>>>>> (genl_rcv_msg)
>>>>>> (netlink_rcv_skb)
>>>>>> (genl_rcv)
>>>>>> (netlink_unicast)
>>>>>> (netlink_sendmsg)
>>>>>> (sock_sendmsg)
>>>>>> (___sys_sendmsg)
>>>>>> (__sys_sendmsg)
>>>>>> (SyS_sendmsg)
>>>>>>
>>>>>> The solution is to pull back the header offset in case
>>>>>> of an error in txdata(), which may happen in case of
>>>> Clarification:
>>>>
>>>> txdata=brcmf_proto_bcdc_txdata()
>>>> brcmf_proto_bcdc_txdata(): Calls brcmf_proto_bcdc_hdrpush()
>>>>
>>>> The header needs to be pulled back in case of error otherwise
>>>> the error handling and cleanup up will fail to decrease the counter
>>>> of pending packets.
>>>
>>> Yes, this part of the patch is clear to me.
>>>
>> Thanks, I wasn't sure.
>>
>>>>>> packet overload in brcmf_sdio_bus_txdata.
>>>>>>
>>>>>> Overloading an WLAN interface is not an unlikely scenario.
>>>
>>> So here is where things start to look suspicious and I have mentioned
>>> this before. My thought here was "How the hell can you end up with a
>>> 2048 packets on the sdio queue", which I mentioned to you before. There
>>> is a high watermark on the queue upon which we do a netif_stop_queue()
>>> so network layer does not keep pushing tx packets our way. Looking
>>> further into this I found that we introduced a bug with commit
>>> 9cd18359d31e ("brcmfmac: Make FWS queueing configurable.") so we ended
>>> up doing nothing except increasing as statistics debug counter :-(
>>>
>> Is there a fix available for the high watermark issue or is it
>> something you will look into?
>>
>> To produce a load on the wlan interface I run
>> iperf -c 239.255.1.3 -u -b 10m -f m -i 60 -t 3000
>> and this is enough in my case to fill up the 2048 queue.
>>
>>>>>> In case of packet overload the error print "out of bus->txq"
>>>>>> is very verbose. To reduce SPAM degrade it to a debug print.
>>>>>>
>>>>>> Signed-off-by: Per Forlin <per.forlin@gmail.com>
>>>>>> ---
>>>>>> Change log:
>>>>>> v2 - Add variable to know whether the counter is increased or not
>>>>>> v3 - txfinalize should decrease the counter. Adjust skb header offset
>>>>>> v4 - Fix build error
>>>>>>
>>>>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 4 ++++
>>>>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c | 4 +++-
>>>>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
>>>>>> 3 files changed, 8 insertions(+), 2 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>>>>> index ed9998b..f342f7c 100644
>>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>>>>> @@ -541,6 +541,9 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success)
>>>>>> struct ethhdr *eh;
>>>>>> u16 type;
>>>>>>
>>>>>> + if (!ifp)
>>>>>> + goto free;
>>>>>> +
>>>
>>> This may not be needed.
>>>
>> This is not strictly needed. I can remove it.
>>
>>>>>> eh = (struct ethhdr *)(txp->data);
>>>>>> type = ntohs(eh->h_proto);
>>>>>>
>>>>>> @@ -553,6 +556,7 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success)
>>>>>> if (!success)
>>>>>> ifp->stats.tx_errors++;
>>>>>>
>>>>>> +free:
>>>>>> brcmu_pkt_buf_free_skb(txp);
>>>>>> }
>>>>>>
>>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>>>>>> index f82c9ab..98cb83f 100644
>>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>>>>>> @@ -1899,8 +1899,10 @@ int brcmf_fws_process_skb(struct brcmf_if *ifp, struct sk_buff *skb)
>>>>>>
>>>>>> if (fws->avoid_queueing) {
>>>>>> rc = brcmf_proto_txdata(drvr, ifp->ifidx, 0, skb);
>>>>>> - if (rc < 0)
>>>>>> + if (rc < 0) {
>>>>>> + (void)brcmf_proto_hdrpull(drvr, false, skb, &ifp);
>>>
>>> Could it be that the ifp is NULL pointer after brcmf_proto_hdrpull().
>>> Can you check. Better use tmp_ifp variable in this call as you have a
>>> valid ifp before this call for sure.
>>>
>> To be on the safe side I can use NULL as in param like you propose,
>> and use the available ifp.
>>
>>>>>> brcmf_txfinalize(ifp, skb, false);
>>>>>> + }
>>>>>> return rc;
>>>>>> }
>>>>>>
>>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>> index a14d9d9d..485e2ad 100644
>>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>> @@ -2721,7 +2721,7 @@ static int brcmf_sdio_bus_txdata(struct device *dev, struct sk_buff *pkt)
>>>>>> *(u16 *)(pkt->cb) = 0;
>>>>>> if (!brcmf_sdio_prec_enq(&bus->txq, pkt, prec)) {
>>>>>> skb_pull(pkt, bus->tx_hdrlen);
>>>>>> - brcmf_err("out of bus->txq !!!\n");
>>>>>> + brcmf_dbg(INFO, "out of bus->txq !!!\n");
>>>
>>> Now that I understand the issue I want to keep this as error print as it
>>> should be very unlikely.
>> I would like to test this patch with the watermark fix to confirm this.
>
> Can you try this?
>
> Regards,
> Arend
>
> ---
> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
> b/drive
> index cd221ab..9f9024a 100644
> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
> @@ -2469,10 +2469,22 @@ void brcmf_fws_bustxfail(struct brcmf_fws_info
> *fws, str
> void brcmf_fws_bus_blocked(struct brcmf_pub *drvr, bool flow_blocked)
> {
> struct brcmf_fws_info *fws = drvr->fws;
> + struct brcmf_if *ifp;
> + int i;
>
> - fws->bus_flow_blocked = flow_blocked;
> - if (!flow_blocked)
> - brcmf_fws_schedule_deq(fws);
> - else
> - fws->stats.bus_flow_block++;
> + if (fws->avoid_queueing) {
> + for (i = 0; i < BRCMF_MAX_IFS; i++) {
> + ifp = drvr->iflist[i];
> + if (!ifp || !ifp->ndev)
> + continue;
> + brcmf_txflowblock_if(ifp,
> BRCMF_NETIF_STOP_REASON_FLOW,
> + flow_blocked);
> + }
> + } else {
> + fws->bus_flow_blocked = flow_blocked;
> + if (!flow_blocked)
> + brcmf_fws_schedule_deq(fws);
> + else
> + fws->stats.bus_flow_block++;
> + }
> }
>
Thanks for the code. I run a quick test and it looks fine.
I added some prints to check the progress:
len - is number of items in the queue
max - is max number of entries in the queue
[ 89.407856] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1789 max 2048
[ 89.414682] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1790 max 2048
[ 89.421497] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1791 max 2048
[ 93.970466] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1524 max 2048
[ 93.977520] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1525 max 2048
[ 93.984572] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1526 max 2048
I will some run more WLAN tests tomorrow to make sure.
When I'm done testing I will update my patch as well and let you know.
I came across this issue when I tried to connect to my WLAN access
point. The bug was triggered due to a lot of background traffic
(broadcast and multicast) in the network filling up the queue.
It's now fixed so that the queue is not flooded. Now I move on to the
next issue. It's still difficult to connect because the background
eats up all the bandwidth (this is not a constant issue but it happens
from time to time depending on the background load).
For queueing-mode there seems to exist logic for handling multicast
traffic separately but for the SDIO case (no-queueing) normal traffic
gets same precedence as multicast traffic.
I'm considering something like this:
--- a/drivers/net/wireless/brcm80211/brcmfmac/fwsignal.c
+++ b/drivers/net/wireless/brcm80211/brcmfmac/fwsignal.c
@@ -1900,6 +1902,8 @@ int brcmf_fws_process_skb(struct brcmf_if *ifp,
struct sk_buff *skb)
drvr->tx_multicast += !!multicast;
if (fws->avoid_queueing) {
+ if (multicast)
+ skb->priority = PRIO_8021D_NONE;
rc = brcmf_proto_txdata(drvr, ifp->ifidx, 0, skb);
if (rc < 0) {
(void)brcmf_proto_hdrpull(drvr, false, skb, NULL);
It feels a little hacky.
I would like to use the drvr->tx_multicast instead, if possible.
The problem is that the "drvr" struct is not passed down to the sdio layer.
One could update bcdc.c : brcmf_proto_bcdc_txdata() to pass
"drcr->multicast" to the SDIO layer but not all bus implementations
need this information (USB for instance)
Any suggestions?
BR
Per Forlin
^ permalink raw reply
* RE: [PATCH 1/4] mac80211: mesh: flush stations before beacons are stopped
From: Machani, Yaniv @ 2016-07-13 19:54 UTC (permalink / raw)
To: Bob Copeland
Cc: Johannes Berg, linux-kernel@vger.kernel.org, David S . Miller,
linux-wireless@vger.kernel.org, netdev@vger.kernel.org,
Hahn, Maital
In-Reply-To: <20160713133338.GD7819@localhost>
On Wed, Jul 13, 2016 at 16:33:38, Bob Copeland wrote:
> linux- wireless@vger.kernel.org; netdev@vger.kernel.org; Hahn, Maital
> Subject: Re: [PATCH 1/4] mac80211: mesh: flush stations before beacons
> are stopped
>
> On Wed, Jul 13, 2016 at 10:11:25AM +0000, Machani, Yaniv wrote:
> > > > Some drivers (e.g. wl18xx) expect that the last stage in the
> > > > de-initialization process will be stopping the beacons, similar to ap.
> > > > Update ieee80211_stop_mesh() flow accordingly.
> > > >
> > > How well have you tested that with other drivers?
> > >
> >
> > Sorry for the delayed response (I've been out) and thanks for your
> > comments, I have tested it with RT3572 as well, and didn't see any issue.
> > I'll update the comment to reflect that.
>
> I'll give this a test on ath10k and wcn36xx as they are the ones most
> likely to care about ordering.
>
Thank you,
Yaniv
> --
> Bob Copeland %% http://bobcopeland.com/
^ permalink raw reply
* [PATCH v3 3/3] mac80211: mesh: fixed HT ies in beacon template
From: Yaniv Machani @ 2016-07-13 20:07 UTC (permalink / raw)
To: linux-kernel
Cc: Yaniv Machani, Meirav Kama, Johannes Berg, David S. Miller,
linux-wireless, netdev
The HT capab info field inside the HT capab IE of the mesh beacon
is incorrect (in the case of 20MHz channel width).
To fix this driver will check configuration from cfg and
will build it accordingly.
Signed-off-by: Meirav Kama <meiravk@ti.com>
Signed-off-by: Yaniv Machani <yanivma@ti.com>
---
V3 - Fixes redundant spaces,empty lines and added FALLTHROUGH note.
net/mac80211/mesh.c | 33 ++++++++++++++++++++++++++++++++-
net/mac80211/util.c | 3 ---
2 files changed, 32 insertions(+), 4 deletions(-)
diff --git a/net/mac80211/mesh.c b/net/mac80211/mesh.c
index 9214bc1..6a67049 100644
--- a/net/mac80211/mesh.c
+++ b/net/mac80211/mesh.c
@@ -422,6 +422,7 @@ int mesh_add_ht_cap_ie(struct ieee80211_sub_if_data *sdata,
enum nl80211_band band = ieee80211_get_sdata_band(sdata);
struct ieee80211_supported_band *sband;
u8 *pos;
+ u16 cap;
sband = local->hw.wiphy->bands[band];
if (!sband->ht_cap.ht_supported ||
@@ -430,11 +431,41 @@ int mesh_add_ht_cap_ie(struct ieee80211_sub_if_data *sdata,
sdata->vif.bss_conf.chandef.width == NL80211_CHAN_WIDTH_10)
return 0;
+ /* determine capability flags */
+ cap = sband->ht_cap.cap;
+
+ /* if channel width is 20MHz - configure HT capab accordingly*/
+ if (sdata->vif.bss_conf.chandef.width == NL80211_CHAN_WIDTH_20) {
+ cap &= ~IEEE80211_HT_CAP_SUP_WIDTH_20_40;
+ cap &= ~IEEE80211_HT_CAP_DSSSCCK40;
+ }
+
+ /* set SM PS mode properly */
+ cap &= ~IEEE80211_HT_CAP_SM_PS;
+ switch (sdata->smps_mode) {
+ case IEEE80211_SMPS_AUTOMATIC:
+ case IEEE80211_SMPS_NUM_MODES:
+ WARN_ON(1);
+ /* FALLTHROUGH */
+ case IEEE80211_SMPS_OFF:
+ cap |= WLAN_HT_CAP_SM_PS_DISABLED <<
+ IEEE80211_HT_CAP_SM_PS_SHIFT;
+ break;
+ case IEEE80211_SMPS_STATIC:
+ cap |= WLAN_HT_CAP_SM_PS_STATIC <<
+ IEEE80211_HT_CAP_SM_PS_SHIFT;
+ break;
+ case IEEE80211_SMPS_DYNAMIC:
+ cap |= WLAN_HT_CAP_SM_PS_DYNAMIC <<
+ IEEE80211_HT_CAP_SM_PS_SHIFT;
+ break;
+ }
+
if (skb_tailroom(skb) < 2 + sizeof(struct ieee80211_ht_cap))
return -ENOMEM;
pos = skb_put(skb, 2 + sizeof(struct ieee80211_ht_cap));
- ieee80211_ie_build_ht_cap(pos, &sband->ht_cap, sband->ht_cap.cap);
+ ieee80211_ie_build_ht_cap(pos, &sband->ht_cap, cap);
return 0;
}
diff --git a/net/mac80211/util.c b/net/mac80211/util.c
index 42bf0b6..5375a82 100644
--- a/net/mac80211/util.c
+++ b/net/mac80211/util.c
@@ -2349,10 +2349,7 @@ u8 *ieee80211_ie_build_ht_oper(u8 *pos, struct ieee80211_sta_ht_cap *ht_cap,
ht_oper->operation_mode = cpu_to_le16(prot_mode);
ht_oper->stbc_param = 0x0000;
- /* It seems that Basic MCS set and Supported MCS set
- are identical for the first 10 bytes */
memset(&ht_oper->basic_set, 0, 16);
- memcpy(&ht_oper->basic_set, &ht_cap->mcs, 10);
return pos + sizeof(struct ieee80211_ht_operation);
}
--
2.9.0
^ permalink raw reply related
* [PATCH 2/4 v1] even better readability
From: Alex Briskin @ 2016-07-13 20:19 UTC (permalink / raw)
To: linux-wireless, johannes; +Cc: Alex Briskin
In-Reply-To: <1468441196-23503-1-git-send-email-br.shurik@gmail.com>
Further improve readability by replacing if else with switch case
Signed-off-by: Alex Briskin <br.shurik@gmail.com>
---
net/mac80211/iface.c | 15 ++++++++++-----
1 file changed, 10 insertions(+), 5 deletions(-)
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index a68cbac..1793ad2 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1207,13 +1207,16 @@ static bool ieee80211_is_skb_handled_by_pkt_type(struct sk_buff *skb,
struct ieee80211_local *local = sdata->local;
struct sta_info *sta;
- if (skb->pkt_type == IEEE80211_SDATA_QUEUE_AGG_START) {
+ switch (skb->pkt_type) {
+ case IEEE80211_SDATA_QUEUE_AGG_START:
ra_tid = (void *)&skb->cb;
ieee80211_start_tx_ba_cb(&sdata->vif, ra_tid->ra, ra_tid->tid);
- } else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_AGG_STOP) {
+ break;
+ case IEEE80211_SDATA_QUEUE_AGG_STOP:
ra_tid = (void *)&skb->cb;
ieee80211_stop_tx_ba_cb(&sdata->vif, ra_tid->ra, ra_tid->tid);
- } else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_RX_AGG_START) {
+ break;
+ case IEEE80211_SDATA_QUEUE_RX_AGG_START:
rx_agg = (void *)&skb->cb;
mutex_lock(&local->sta_mtx);
sta = sta_info_get_bss(sdata, rx_agg->addr);
@@ -1223,7 +1226,8 @@ static bool ieee80211_is_skb_handled_by_pkt_type(struct sk_buff *skb,
IEEE80211_MAX_AMPDU_BUF,
false, true);
mutex_unlock(&local->sta_mtx);
- } else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_RX_AGG_STOP) {
+ break;
+ case IEEE80211_SDATA_QUEUE_RX_AGG_STOP:
rx_agg = (void *)&skb->cb;
mutex_lock(&local->sta_mtx);
sta = sta_info_get_bss(sdata, rx_agg->addr);
@@ -1233,7 +1237,8 @@ static bool ieee80211_is_skb_handled_by_pkt_type(struct sk_buff *skb,
WLAN_BACK_RECIPIENT, 0,
false);
mutex_unlock(&local->sta_mtx);
- } else {
+ break;
+ default:
return false;
}
/*will return true if pkt_type found and handled */
--
2.5.0
^ permalink raw reply related
* [PATCH 3/4 v1] Continue breaking down to smaller functions
From: Alex Briskin @ 2016-07-13 20:19 UTC (permalink / raw)
To: linux-wireless, johannes; +Cc: Alex Briskin
In-Reply-To: <1468441196-23503-1-git-send-email-br.shurik@gmail.com>
This commit contains unhandled checkpatch warnings in code moved from
ieee80211_iface_work
Signed-off-by: Alex Briskin <br.shurik@gmail.com>
---
net/mac80211/iface.c | 150 +++++++++++++++++++++++++++------------------------
1 file changed, 81 insertions(+), 69 deletions(-)
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 1793ad2..41c3bd0 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1245,6 +1245,85 @@ static bool ieee80211_is_skb_handled_by_pkt_type(struct sk_buff *skb,
return true;
}
+static bool ieee80211_is_handled_by_frame_control(struct sk_buff *skb,
+ struct ieee80211_sub_if_data
+ *sdata)
+{
+ struct ieee80211_local *local = sdata->local;
+ struct sta_info *sta;
+ struct ieee80211_mgmt *mgmt = (void *)skb->data;
+
+ if (ieee80211_is_action(mgmt->frame_control) &&
+ mgmt->u.action.category == WLAN_CATEGORY_BACK) {
+ int len = skb->len;
+
+ mutex_lock(&local->sta_mtx);
+ sta = sta_info_get_bss(sdata, mgmt->sa);
+ if (sta) {
+ switch (mgmt->u.action.u.addba_req.action_code) {
+ case WLAN_ACTION_ADDBA_REQ:
+ ieee80211_process_addba_request(local, sta,
+ mgmt, len);
+ break;
+ case WLAN_ACTION_ADDBA_RESP:
+ ieee80211_process_addba_resp(local, sta,
+ mgmt, len);
+ break;
+ case WLAN_ACTION_DELBA:
+ ieee80211_process_delba(sdata, sta, mgmt, len);
+ break;
+ default:
+ WARN_ON(1);
+ break;
+ }
+ }
+ mutex_unlock(&local->sta_mtx);
+ } else if (ieee80211_is_action(mgmt->frame_control) &&
+ mgmt->u.action.category == WLAN_CATEGORY_VHT) {
+ switch (mgmt->u.action.u.vht_group_notif.action_code) {
+ case WLAN_VHT_ACTION_GROUPID_MGMT:
+ ieee80211_process_mu_groups(sdata, mgmt);
+ break;
+ default:
+ WARN_ON(1);
+ break;
+ }
+ } else if (ieee80211_is_data_qos(mgmt->frame_control)) {
+ struct ieee80211_hdr *hdr = (void *)mgmt;
+ /*
+ * So the frame isn't mgmt, but frame_control
+ * is at the right place anyway, of course, so
+ * the if statement is correct.
+ *
+ * Warn if we have other data frame types here,
+ * they must not get here.
+ */
+ WARN_ON(hdr->frame_control &
+ cpu_to_le16(IEEE80211_STYPE_NULLFUNC));
+ WARN_ON(!(hdr->seq_ctrl & cpu_to_le16(IEEE80211_SCTL_FRAG)));
+ /*
+ * This was a fragment of a frame, received while
+ * a block-ack session was active. That cannot be
+ * right, so terminate the session.
+ */
+ mutex_lock(&local->sta_mtx);
+ sta = sta_info_get_bss(sdata, mgmt->sa);
+ if (sta) {
+ u16 tid = *ieee80211_get_qos_ctl(hdr) &
+ IEEE80211_QOS_CTL_TID_MASK;
+
+ __ieee80211_stop_rx_ba_session(sta, tid,
+ WLAN_BACK_RECIPIENT,
+ WLAN_REASON_QSTA_REQUIRE_SETUP,
+ true);
+ }
+ mutex_unlock(&local->sta_mtx);
+ } else {
+ return false;
+ }
+ return true;
+}
+
static void ieee80211_iface_work(struct work_struct *work)
{
struct ieee80211_sub_if_data *sdata =
@@ -1264,77 +1343,10 @@ static void ieee80211_iface_work(struct work_struct *work)
/* first process frames */
while ((skb = skb_dequeue(&sdata->skb_queue))) {
- struct ieee80211_mgmt *mgmt = (void *)skb->data;
-
if (ieee80211_is_skb_handled_by_pkt_type(skb, sdata)) {
goto free_skb;
- } else if (ieee80211_is_action(mgmt->frame_control) &&
- mgmt->u.action.category == WLAN_CATEGORY_BACK) {
- int len = skb->len;
-
- mutex_lock(&local->sta_mtx);
- sta = sta_info_get_bss(sdata, mgmt->sa);
- if (sta) {
- switch (mgmt->u.action.u.addba_req.action_code) {
- case WLAN_ACTION_ADDBA_REQ:
- ieee80211_process_addba_request(
- local, sta, mgmt, len);
- break;
- case WLAN_ACTION_ADDBA_RESP:
- ieee80211_process_addba_resp(local, sta,
- mgmt, len);
- break;
- case WLAN_ACTION_DELBA:
- ieee80211_process_delba(sdata, sta,
- mgmt, len);
- break;
- default:
- WARN_ON(1);
- break;
- }
- }
- mutex_unlock(&local->sta_mtx);
- } else if (ieee80211_is_action(mgmt->frame_control) &&
- mgmt->u.action.category == WLAN_CATEGORY_VHT) {
- switch (mgmt->u.action.u.vht_group_notif.action_code) {
- case WLAN_VHT_ACTION_GROUPID_MGMT:
- ieee80211_process_mu_groups(sdata, mgmt);
- break;
- default:
- WARN_ON(1);
- break;
- }
- } else if (ieee80211_is_data_qos(mgmt->frame_control)) {
- struct ieee80211_hdr *hdr = (void *)mgmt;
- /*
- * So the frame isn't mgmt, but frame_control
- * is at the right place anyway, of course, so
- * the if statement is correct.
- *
- * Warn if we have other data frame types here,
- * they must not get here.
- */
- WARN_ON(hdr->frame_control &
- cpu_to_le16(IEEE80211_STYPE_NULLFUNC));
- WARN_ON(!(hdr->seq_ctrl &
- cpu_to_le16(IEEE80211_SCTL_FRAG)));
- /*
- * This was a fragment of a frame, received while
- * a block-ack session was active. That cannot be
- * right, so terminate the session.
- */
- mutex_lock(&local->sta_mtx);
- sta = sta_info_get_bss(sdata, mgmt->sa);
- if (sta) {
- u16 tid = *ieee80211_get_qos_ctl(hdr) &
- IEEE80211_QOS_CTL_TID_MASK;
-
- __ieee80211_stop_rx_ba_session(
- sta, tid, WLAN_BACK_RECIPIENT,
- WLAN_REASON_QSTA_REQUIRE_SETUP,
- true);
- }
- mutex_unlock(&local->sta_mtx);
+ } else if (ieee80211_is_handled_by_frame_control(skb, sdata)) {
+ goto free_skb;
} else switch (sdata->vif.type) {
case NL80211_IFTYPE_STATION:
ieee80211_sta_rx_queued_mgmt(sdata, skb);
--
2.5.0
^ permalink raw reply related
* [PATCH 1/4 v1] Refactoring ieee80211_iface_work to reduce complexity and improve readability.
From: Alex Briskin @ 2016-07-13 20:19 UTC (permalink / raw)
To: linux-wireless, johannes; +Cc: Alex Briskin
In-Reply-To: <1468441196-23503-1-git-send-email-br.shurik@gmail.com>
No performance impact/overhead expected. The (gcc) compiler optimizes the
added function out.
Signed-off-by: Alex Briskin <br.shurik@gmail.com>
---
net/mac80211/iface.c | 75 +++++++++++++++++++++++++++++++---------------------
1 file changed, 45 insertions(+), 30 deletions(-)
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index c59af3e..a68cbac 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1198,6 +1198,48 @@ static void ieee80211_if_setup(struct net_device *dev)
dev->destructor = ieee80211_if_free;
}
+static bool ieee80211_is_skb_handled_by_pkt_type(struct sk_buff *skb,
+ struct ieee80211_sub_if_data
+ *sdata)
+{
+ struct ieee80211_ra_tid *ra_tid;
+ struct ieee80211_rx_agg *rx_agg;
+ struct ieee80211_local *local = sdata->local;
+ struct sta_info *sta;
+
+ if (skb->pkt_type == IEEE80211_SDATA_QUEUE_AGG_START) {
+ ra_tid = (void *)&skb->cb;
+ ieee80211_start_tx_ba_cb(&sdata->vif, ra_tid->ra, ra_tid->tid);
+ } else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_AGG_STOP) {
+ ra_tid = (void *)&skb->cb;
+ ieee80211_stop_tx_ba_cb(&sdata->vif, ra_tid->ra, ra_tid->tid);
+ } else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_RX_AGG_START) {
+ rx_agg = (void *)&skb->cb;
+ mutex_lock(&local->sta_mtx);
+ sta = sta_info_get_bss(sdata, rx_agg->addr);
+ if (sta)
+ __ieee80211_start_rx_ba_session(sta,
+ 0, 0, 0, 1, rx_agg->tid,
+ IEEE80211_MAX_AMPDU_BUF,
+ false, true);
+ mutex_unlock(&local->sta_mtx);
+ } else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_RX_AGG_STOP) {
+ rx_agg = (void *)&skb->cb;
+ mutex_lock(&local->sta_mtx);
+ sta = sta_info_get_bss(sdata, rx_agg->addr);
+ if (sta)
+ __ieee80211_stop_rx_ba_session(sta,
+ rx_agg->tid,
+ WLAN_BACK_RECIPIENT, 0,
+ false);
+ mutex_unlock(&local->sta_mtx);
+ } else {
+ return false;
+ }
+ /*will return true if pkt_type found and handled */
+ return true;
+}
+
static void ieee80211_iface_work(struct work_struct *work)
{
struct ieee80211_sub_if_data *sdata =
@@ -1205,8 +1247,6 @@ static void ieee80211_iface_work(struct work_struct *work)
struct ieee80211_local *local = sdata->local;
struct sk_buff *skb;
struct sta_info *sta;
- struct ieee80211_ra_tid *ra_tid;
- struct ieee80211_rx_agg *rx_agg;
if (!ieee80211_sdata_running(sdata))
return;
@@ -1221,34 +1261,8 @@ static void ieee80211_iface_work(struct work_struct *work)
while ((skb = skb_dequeue(&sdata->skb_queue))) {
struct ieee80211_mgmt *mgmt = (void *)skb->data;
- if (skb->pkt_type == IEEE80211_SDATA_QUEUE_AGG_START) {
- ra_tid = (void *)&skb->cb;
- ieee80211_start_tx_ba_cb(&sdata->vif, ra_tid->ra,
- ra_tid->tid);
- } else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_AGG_STOP) {
- ra_tid = (void *)&skb->cb;
- ieee80211_stop_tx_ba_cb(&sdata->vif, ra_tid->ra,
- ra_tid->tid);
- } else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_RX_AGG_START) {
- rx_agg = (void *)&skb->cb;
- mutex_lock(&local->sta_mtx);
- sta = sta_info_get_bss(sdata, rx_agg->addr);
- if (sta)
- __ieee80211_start_rx_ba_session(sta,
- 0, 0, 0, 1, rx_agg->tid,
- IEEE80211_MAX_AMPDU_BUF,
- false, true);
- mutex_unlock(&local->sta_mtx);
- } else if (skb->pkt_type == IEEE80211_SDATA_QUEUE_RX_AGG_STOP) {
- rx_agg = (void *)&skb->cb;
- mutex_lock(&local->sta_mtx);
- sta = sta_info_get_bss(sdata, rx_agg->addr);
- if (sta)
- __ieee80211_stop_rx_ba_session(sta,
- rx_agg->tid,
- WLAN_BACK_RECIPIENT, 0,
- false);
- mutex_unlock(&local->sta_mtx);
+ if (ieee80211_is_skb_handled_by_pkt_type(skb, sdata)) {
+ goto free_skb;
} else if (ieee80211_is_action(mgmt->frame_control) &&
mgmt->u.action.category == WLAN_CATEGORY_BACK) {
int len = skb->len;
@@ -1333,6 +1347,7 @@ static void ieee80211_iface_work(struct work_struct *work)
break;
}
+free_skb:
kfree_skb(skb);
}
--
2.5.0
^ permalink raw reply related
* [PATCH 0/4 v1] Refactoring ieee80211_iface_work
From: Alex Briskin @ 2016-07-13 20:19 UTC (permalink / raw)
To: linux-wireless, johannes; +Cc: Alex Briskin
Hi All,
This is my first patch(s).
I've decided to refactor ieee80211_iface_work function and break it down
to smaller better defined function.
I think these changes make the code much more readable and do not impose
no overhead.
I've tested these patches with sparse and checkpatch.pl
Function names might not be descriptive enough.
Hope you find this useful.
Alex Briskin (4):
0) [28e464b19aaaba90c8946fb979b58709d55dffcf]
Added new function ieee80211_is_skb_handled_by_pkt_type and moved
some code from ieee80211_iface_work to reduce complexity and
improve readability
1) [486e3d5abb4dc6361cdd923254a2b68d43dcdaba]
Refactored code in ieee80211_is_skb_handled_by_pkt_type.
"if () {} else if ()" replaced by switch case.
2) [9ef2eab8e831420bc6748a4466ffa6b7a99bf447]
Added new function ieee80211_is_handled_by_frame_control and moved
some code from ieee80211_iface_work to it.
3) [1de8cdf9a0c05c6a21d9e43e5b55862f6efcf450]
Added new function ieee80211_handle_by_vif_type with code from
ieee80211_iface_work.
At this point ieee80211_iface_work seems to me much more readable
and better understood.
net/mac80211/iface.c | 264 +++++++++++++++++++++++++++++----------------------
1 file changed, 150 insertions(+), 114 deletions(-)
--
2.5.0
^ permalink raw reply
* [PATCH 4/4 v1] Simple and well understood logic
From: Alex Briskin @ 2016-07-13 20:19 UTC (permalink / raw)
To: linux-wireless, johannes; +Cc: Alex Briskin
In-Reply-To: <1468441196-23503-1-git-send-email-br.shurik@gmail.com>
Now the logic is obvious - if skb not handled by pkt type and not handled
by frame control it is handled by vif type and then released
Signed-off-by: Alex Briskin <br.shurik@gmail.com>
---
net/mac80211/iface.c | 44 ++++++++++++++++++++++++--------------------
1 file changed, 24 insertions(+), 20 deletions(-)
diff --git a/net/mac80211/iface.c b/net/mac80211/iface.c
index 41c3bd0..76e8c6a 100644
--- a/net/mac80211/iface.c
+++ b/net/mac80211/iface.c
@@ -1324,6 +1324,27 @@ static bool ieee80211_is_handled_by_frame_control(struct sk_buff *skb,
return true;
}
+static void ieee80211_handle_by_vif_type(struct sk_buff *skb,
+ struct ieee80211_sub_if_data *sdata)
+{
+ switch (sdata->vif.type) {
+ case NL80211_IFTYPE_STATION:
+ ieee80211_sta_rx_queued_mgmt(sdata, skb);
+ break;
+ case NL80211_IFTYPE_ADHOC:
+ ieee80211_ibss_rx_queued_mgmt(sdata, skb);
+ break;
+ case NL80211_IFTYPE_MESH_POINT:
+ if (!ieee80211_vif_is_mesh(&sdata->vif))
+ break;
+ ieee80211_mesh_rx_queued_mgmt(sdata, skb);
+ break;
+ default:
+ WARN(1, "frame for unexpected interface type");
+ break;
+ }
+}
+
static void ieee80211_iface_work(struct work_struct *work)
{
struct ieee80211_sub_if_data *sdata =
@@ -1343,28 +1364,11 @@ static void ieee80211_iface_work(struct work_struct *work)
/* first process frames */
while ((skb = skb_dequeue(&sdata->skb_queue))) {
- if (ieee80211_is_skb_handled_by_pkt_type(skb, sdata)) {
- goto free_skb;
- } else if (ieee80211_is_handled_by_frame_control(skb, sdata)) {
- goto free_skb;
- } else switch (sdata->vif.type) {
- case NL80211_IFTYPE_STATION:
- ieee80211_sta_rx_queued_mgmt(sdata, skb);
- break;
- case NL80211_IFTYPE_ADHOC:
- ieee80211_ibss_rx_queued_mgmt(sdata, skb);
- break;
- case NL80211_IFTYPE_MESH_POINT:
- if (!ieee80211_vif_is_mesh(&sdata->vif))
- break;
- ieee80211_mesh_rx_queued_mgmt(sdata, skb);
- break;
- default:
- WARN(1, "frame for unexpected interface type");
- break;
+ if (!ieee80211_is_skb_handled_by_pkt_type(skb, sdata) &&
+ !ieee80211_is_handled_by_frame_control(skb, sdata)) {
+ ieee80211_handle_by_vif_type(skb, sdata);
}
-free_skb:
kfree_skb(skb);
}
--
2.5.0
^ permalink raw reply related
* Re: [PATCH 3/4 v1] Continue breaking down to smaller functions
From: kbuild test robot @ 2016-07-13 20:41 UTC (permalink / raw)
To: Alex Briskin; +Cc: kbuild-all, linux-wireless, johannes, Alex Briskin
In-Reply-To: <1468441196-23503-4-git-send-email-br.shurik@gmail.com>
[-- Attachment #1: Type: text/plain, Size: 1963 bytes --]
Hi,
[auto build test WARNING on mac80211/master]
[also build test WARNING on v4.7-rc7]
[cannot apply to mac80211-next/master next-20160712]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Alex-Briskin/Refactoring-ieee80211_iface_work/20160714-042301
base: https://git.kernel.org/pub/scm/linux/kernel/git/jberg/mac80211.git master
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 5.3.1-8) 5.3.1 20160205
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc64
All warnings (new ones prefixed by >>):
net/mac80211/iface.c: In function 'ieee80211_iface_work':
>> net/mac80211/iface.c:1333:19: warning: unused variable 'sta' [-Wunused-variable]
struct sta_info *sta;
^
vim +/sta +1333 net/mac80211/iface.c
1317 WLAN_REASON_QSTA_REQUIRE_SETUP,
1318 true);
1319 }
1320 mutex_unlock(&local->sta_mtx);
1321 } else {
1322 return false;
1323 }
1324 return true;
1325 }
1326
1327 static void ieee80211_iface_work(struct work_struct *work)
1328 {
1329 struct ieee80211_sub_if_data *sdata =
1330 container_of(work, struct ieee80211_sub_if_data, work);
1331 struct ieee80211_local *local = sdata->local;
1332 struct sk_buff *skb;
> 1333 struct sta_info *sta;
1334
1335 if (!ieee80211_sdata_running(sdata))
1336 return;
1337
1338 if (test_bit(SCAN_SW_SCANNING, &local->scanning))
1339 return;
1340
1341 if (!ieee80211_can_run_worker(local))
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 46212 bytes --]
^ permalink raw reply
* Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded
From: Kalle Valo @ 2016-07-14 7:13 UTC (permalink / raw)
To: Prarit Bhargava
Cc: Grumbach, Emmanuel, linux-kernel@vger.kernel.org, linuxwifi,
Coelho, Luciano, Berg, Johannes, Ivgi, Chaya Rachel,
netdev@vger.kernel.org, Sharon, Sara,
linux-wireless@vger.kernel.org
In-Reply-To: <57861172.30103@redhat.com>
Prarit Bhargava <prarit@redhat.com> writes:
> On 07/13/2016 02:50 AM, Kalle Valo wrote:
>> Prarit Bhargava <prarit@redhat.com> writes:
>>
>>>> We implement thermal zone because we do support it, but the problem is
>>>> that we need the firmware to be loaded for that. So you can argue that
>>>> we should register *later* when the firmware is loaded. But this is
>>>> really not helping all that much because the firmware can also be
>>>> stopped at any time. So you'd want us to register / unregister the
>>>> thermal zone anytime the firmware is loaded / unloaded?
>>>
>>> You might have to do that. I think that if the firmware enables a feature then
>>> the act of loading the firmware should run the code that enables the feature.
>>> IMO of course.
>>
>> But I suspect that the iwlwifi firmware is loaded during interface up
>> (and unloaded during interface down) and in that case
>> register/unregister would be happening all the time.
>
> You make it sound like the interface is coming and going a 1000 times a second.
> Maybe this happens once during runtime & during suspend/resume
> cycles?
Of course it doesn't happen 1000 times a second but it depends on user
space behaviour. In some cases, when the wlan interface is always up,
the firmware is loaded only once. But in some cases the user space might
change the interface state more frequently.
More so registering services like thermal zone should happen during
driver probe time, not during interface up event.
> What about the cases when the firmware isn't present (and that's what
> lead me to this bug)?
In that case the kernel could return a predefined error value like
-EGAIN or -ENOTDOWN so that the user space knows that a value is not
available at this time (but might be available later).
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH 0/4 v1] Refactoring ieee80211_iface_work
From: Arend Van Spriel @ 2016-07-14 7:56 UTC (permalink / raw)
To: Alex Briskin, linux-wireless, johannes
In-Reply-To: <1468441196-23503-1-git-send-email-br.shurik@gmail.com>
On 13-7-2016 22:19, Alex Briskin wrote:
> Hi All,
> This is my first patch(s).
Hi Alex,
As these patches are touching mac80211 it would be good to use
'mac80211: ' prefix.
> I've decided to refactor ieee80211_iface_work function and break it down
> to smaller better defined function.
>
> I think these changes make the code much more readable and do not impose
> no overhead.
>
> I've tested these patches with sparse and checkpatch.pl
>
> Function names might not be descriptive enough.
> Hope you find this useful.
>
> Alex Briskin (4):
> 0) [28e464b19aaaba90c8946fb979b58709d55dffcf]
> Added new function ieee80211_is_skb_handled_by_pkt_type and moved
> some code from ieee80211_iface_work to reduce complexity and
> improve readability
>
> 1) [486e3d5abb4dc6361cdd923254a2b68d43dcdaba]
> Refactored code in ieee80211_is_skb_handled_by_pkt_type.
> "if () {} else if ()" replaced by switch case.
I would collapse these two patches in one patch.
> 2) [9ef2eab8e831420bc6748a4466ffa6b7a99bf447]
> Added new function ieee80211_is_handled_by_frame_control and moved
> some code from ieee80211_iface_work to it.
>
> 3) [1de8cdf9a0c05c6a21d9e43e5b55862f6efcf450]
> Added new function ieee80211_handle_by_vif_type with code from
> ieee80211_iface_work.
>
> At this point ieee80211_iface_work seems to me much more readable
> and better understood.
You are allowed to have an opinion :-) The function naming of the three
functions could be more consistent as you seem to drop a bit in every
patch, ie. is_skb_handled_by -> is_handled_by -> handle_by.
Regards,
Arend
> net/mac80211/iface.c | 264 +++++++++++++++++++++++++++++----------------------
> 1 file changed, 150 insertions(+), 114 deletions(-)
>
^ permalink raw reply
* Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded
From: Kalle Valo @ 2016-07-14 8:01 UTC (permalink / raw)
To: Prarit Bhargava
Cc: Luca Coelho, Grumbach, Emmanuel, linux-kernel@vger.kernel.org,
linuxwifi, Berg, Johannes, Ivgi, Chaya Rachel,
netdev@vger.kernel.org, Sharon, Sara,
linux-wireless@vger.kernel.org
In-Reply-To: <578615EF.9010305@redhat.com>
Prarit Bhargava <prarit@redhat.com> writes:
> On 07/13/2016 03:24 AM, Luca Coelho wrote:
>
>> I totally agree with Emmanuel and Kalle. We should not change this.
>> It is a design decision to return an error when the interface is
>> down, this is very common with other subsystems as well.
>
> Please show me another subsystem or driver that does this. I've looked around
> the kernel but cannot find one that updates the firmware and implements new
> features on the fly like this. I have come across several drivers that allow
> for an update, but they do not implement new features based on the
> firmware.
>
> Additionally, what happens when someone back revs firmware versions (which
> happens far more than you and I would expect)? Does that mean I now go from a
> functional system to a non-functional system wrt to userspace?
I'm not following, what do you mean exactly? Why are you talking
updating the firmware?
So when we talk about "loading firmware" we mean that the driver pushes
the firmware image to to the chipset. And then the interface is down the
chipset is powered down and the RAM on it will be erased. That's the
general idea anyway, I haven't checked how iwlwifi exactly works in this
case but Luca or Emmanuel can correct me.
>> The userspace should be able to handle errors and report something
>> like "unavailable" when this kind of error is returned.
>
> I myself have made the same arguments wrt to cpufreq code & bad userspace
> choices. I just went through this a few months back with what went from a
> simple patch and turned out to be a hideous patch in cpufreq. You cannot break
> userspace like this.
Don't get me wrong, I'm a strong supporter of stable user space
interfaces and I always try to adher to that. But there's a limit for
everything. If I'm understanding correctly, what you mean is that the
kernel should never return an error because an application doesn't
handle errors gracefully. Sorry, but that doesn't make sense to me.
> See commit 51443fbf3d2c ("cpufreq: intel_pstate: Fix intel_pstate powersave
> min_perf_pct value"). What should have been a trivial change resulted in a
> massive change because of broken userspace.
In that cpufreq case I understand, it was about a combination of
configuration values which broke the user space. But here we are just
dealing with a simple error value, nothing fancy.
>> I'm not sure EIO is the best we can have, but for me that's exactly
>> what it is. The thermal zone *is* there, but cannot be accessed
>> because the firmware is not available. I'm okay to change it to EBUSY,
>> if that would help userspace, but I think that's a bit misleading. The
>> device is not busy, on the contrary, it's not even running at all.
>>
>
> I understand that, but by returning -EIO we end up with an error.
>
>> Furthermore, I don't think this is "breaking userspace" in the sense of
>> being a regression.
>
> I run (let's say 4.5 kernel). sensors works. I update to 4.7. sensors doesn't
> work. How is that not a regression? That's _exactly_ what it should be
> reported as.
Sure, it's a regression in a way. But that's how the user space app you
are using is implemented, the same problem would happen with any driver
returning errors.
>> The userspace API has always been implemented with
>> the possibility of returning errors. It's not a good design if a
>> single device returning an error causes all the other devices to also
>> fail.
>>
>
> If that were the case we would never have to worry about "breaking userspace"?
> For any kernel change I could just say that the userspace design was bad and be
> done with it. Why fix anything then?
Because we are talking about a simple error value.
> I don't see any harm in waiting to register the sysfs files for hwmon until the
> firmware has been validated.
I'm against of that because it's bad software design. It's standard
practise in Linux that drivers register their capabilities during driver
probe time so that user space can query them whenever needed. I assume a
properly behaving user space app would want to know about all the
available sensors once the driver is initialised and your suggestion
would break that.
> IIUC, the up/down'ing of the device doesn't happen that often (during
> initial boot, and suspend/resume, switching wifi connections,
> shutdown?).
Basically it can happen anytime, this is fully controlled by user space.
There's no point of trying to make any assumptions as they won't hold
anyway.
> This would make the iwlwifi community happy (IMO) and
> sensors would still work. At the same time I could write a patch for
> lm-sensors to fix this issue if it comes up in future versions.
> [Aside: I'm going to have the reproducing system available today and
> will test this out. It looks like just moving some code around.]
Another option, but still a bad one I don't like, is that you change the
kernel interface to ignore all errors from drivers (like iwlwifi). This
way drivers don't need to make ugly workarounds.
> The bottom line is that lm-sensors is currently broken with this change in
> iwlwifi. AFAICT, no other thermal device returns an error this way, and IMO
> that means the iwlwifi driver is doing something new and unexpected wrt to
> userspace.
I haven't checked but I suspect ath10k has a similar problem when
interface is down.
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH v4] brcmfmac: Decrease 8021x_cnt for dropped packets
From: Arend Van Spriel @ 2016-07-14 8:57 UTC (permalink / raw)
To: Per Förlin; +Cc: linux-wireless, arend
In-Reply-To: <CAC0pXTKk1AA5k-i10PKTxHujPG2jPGbWMnNp6y6=v0ngK5+L4Q@mail.gmail.com>
On 13-7-2016 20:52, Per Förlin wrote:
> 2016-07-13 13:20 GMT+02:00 Arend Van Spriel <arend.vanspriel@broadcom.com>:
>> On 12-7-2016 12:23, Per Förlin wrote:
>>> 2016-07-12 11:48 GMT+02:00 Arend Van Spriel <arend.vanspriel@broadcom.com>:
>>>>
>>>>
>>>> On 12-7-2016 10:35, Per Förlin wrote:
>>>>> 2016-07-06 11:53 GMT+02:00 Per Förlin <per.forlin@gmail.com>:
>>>>>> I have now verified this patch on backports 4.4.
>>>>>>
>>>>>> 2016-04-12 23:55 GMT+02:00 <per.forlin@gmail.com>:
>>>>>>> From: Per Forlin <per.forlin@gmail.com>
>>>>>>>
>>>>>>> This patch resolves an issue where pend_8021x_cnt was not decreased
>>>>>>> on txfinalize. This caused brcmf_netdev_wait_pend8021x to timeout
>>>>>>> because the counter indicated pending packets.
>>>>>>>
>>>>>>> WARNING: at .../brcmfmac/core.c:1289 brcmf_netdev_wait_pend8021x
>>>>>>> (warn_slowpath_common)
>>>>>>> (warn_slowpath_null)
>>>>>>> (brcmf_netdev_wait_pend8021x [brcmfmac])
>>>>>>> (send_key_to_dongle [brcmfmac])
>>>>>>> (brcmf_cfg80211_del_key [brcmfmac])
>>>>>>> (nl80211_del_key [cfg80211])
>>>>>>> (genl_rcv_msg)
>>>>>>> (netlink_rcv_skb)
>>>>>>> (genl_rcv)
>>>>>>> (netlink_unicast)
>>>>>>> (netlink_sendmsg)
>>>>>>> (sock_sendmsg)
>>>>>>> (___sys_sendmsg)
>>>>>>> (__sys_sendmsg)
>>>>>>> (SyS_sendmsg)
>>>>>>>
>>>>>>> The solution is to pull back the header offset in case
>>>>>>> of an error in txdata(), which may happen in case of
>>>>> Clarification:
>>>>>
>>>>> txdata=brcmf_proto_bcdc_txdata()
>>>>> brcmf_proto_bcdc_txdata(): Calls brcmf_proto_bcdc_hdrpush()
>>>>>
>>>>> The header needs to be pulled back in case of error otherwise
>>>>> the error handling and cleanup up will fail to decrease the counter
>>>>> of pending packets.
>>>>
>>>> Yes, this part of the patch is clear to me.
>>>>
>>> Thanks, I wasn't sure.
>>>
>>>>>>> packet overload in brcmf_sdio_bus_txdata.
>>>>>>>
>>>>>>> Overloading an WLAN interface is not an unlikely scenario.
>>>>
>>>> So here is where things start to look suspicious and I have mentioned
>>>> this before. My thought here was "How the hell can you end up with a
>>>> 2048 packets on the sdio queue", which I mentioned to you before. There
>>>> is a high watermark on the queue upon which we do a netif_stop_queue()
>>>> so network layer does not keep pushing tx packets our way. Looking
>>>> further into this I found that we introduced a bug with commit
>>>> 9cd18359d31e ("brcmfmac: Make FWS queueing configurable.") so we ended
>>>> up doing nothing except increasing as statistics debug counter :-(
>>>>
>>> Is there a fix available for the high watermark issue or is it
>>> something you will look into?
>>>
>>> To produce a load on the wlan interface I run
>>> iperf -c 239.255.1.3 -u -b 10m -f m -i 60 -t 3000
>>> and this is enough in my case to fill up the 2048 queue.
>>>
>>>>>>> In case of packet overload the error print "out of bus->txq"
>>>>>>> is very verbose. To reduce SPAM degrade it to a debug print.
>>>>>>>
>>>>>>> Signed-off-by: Per Forlin <per.forlin@gmail.com>
>>>>>>> ---
>>>>>>> Change log:
>>>>>>> v2 - Add variable to know whether the counter is increased or not
>>>>>>> v3 - txfinalize should decrease the counter. Adjust skb header offset
>>>>>>> v4 - Fix build error
>>>>>>>
>>>>>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 4 ++++
>>>>>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c | 4 +++-
>>>>>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
>>>>>>> 3 files changed, 8 insertions(+), 2 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>>>>>> index ed9998b..f342f7c 100644
>>>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>>>>>> @@ -541,6 +541,9 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success)
>>>>>>> struct ethhdr *eh;
>>>>>>> u16 type;
>>>>>>>
>>>>>>> + if (!ifp)
>>>>>>> + goto free;
>>>>>>> +
>>>>
>>>> This may not be needed.
>>>>
>>> This is not strictly needed. I can remove it.
>>>
>>>>>>> eh = (struct ethhdr *)(txp->data);
>>>>>>> type = ntohs(eh->h_proto);
>>>>>>>
>>>>>>> @@ -553,6 +556,7 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success)
>>>>>>> if (!success)
>>>>>>> ifp->stats.tx_errors++;
>>>>>>>
>>>>>>> +free:
>>>>>>> brcmu_pkt_buf_free_skb(txp);
>>>>>>> }
>>>>>>>
>>>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>>>>>>> index f82c9ab..98cb83f 100644
>>>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>>>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>>>>>>> @@ -1899,8 +1899,10 @@ int brcmf_fws_process_skb(struct brcmf_if *ifp, struct sk_buff *skb)
>>>>>>>
>>>>>>> if (fws->avoid_queueing) {
>>>>>>> rc = brcmf_proto_txdata(drvr, ifp->ifidx, 0, skb);
>>>>>>> - if (rc < 0)
>>>>>>> + if (rc < 0) {
>>>>>>> + (void)brcmf_proto_hdrpull(drvr, false, skb, &ifp);
>>>>
>>>> Could it be that the ifp is NULL pointer after brcmf_proto_hdrpull().
>>>> Can you check. Better use tmp_ifp variable in this call as you have a
>>>> valid ifp before this call for sure.
>>>>
>>> To be on the safe side I can use NULL as in param like you propose,
>>> and use the available ifp.
>>>
>>>>>>> brcmf_txfinalize(ifp, skb, false);
>>>>>>> + }
>>>>>>> return rc;
>>>>>>> }
>>>>>>>
>>>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>>> index a14d9d9d..485e2ad 100644
>>>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>>> @@ -2721,7 +2721,7 @@ static int brcmf_sdio_bus_txdata(struct device *dev, struct sk_buff *pkt)
>>>>>>> *(u16 *)(pkt->cb) = 0;
>>>>>>> if (!brcmf_sdio_prec_enq(&bus->txq, pkt, prec)) {
>>>>>>> skb_pull(pkt, bus->tx_hdrlen);
>>>>>>> - brcmf_err("out of bus->txq !!!\n");
>>>>>>> + brcmf_dbg(INFO, "out of bus->txq !!!\n");
>>>>
>>>> Now that I understand the issue I want to keep this as error print as it
>>>> should be very unlikely.
>>> I would like to test this patch with the watermark fix to confirm this.
>>
>> Can you try this?
>>
>> Regards,
>> Arend
>>
>> ---
>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>> b/drive
>> index cd221ab..9f9024a 100644
>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>> @@ -2469,10 +2469,22 @@ void brcmf_fws_bustxfail(struct brcmf_fws_info
>> *fws, str
>> void brcmf_fws_bus_blocked(struct brcmf_pub *drvr, bool flow_blocked)
>> {
>> struct brcmf_fws_info *fws = drvr->fws;
>> + struct brcmf_if *ifp;
>> + int i;
>>
>> - fws->bus_flow_blocked = flow_blocked;
>> - if (!flow_blocked)
>> - brcmf_fws_schedule_deq(fws);
>> - else
>> - fws->stats.bus_flow_block++;
>> + if (fws->avoid_queueing) {
>> + for (i = 0; i < BRCMF_MAX_IFS; i++) {
>> + ifp = drvr->iflist[i];
>> + if (!ifp || !ifp->ndev)
>> + continue;
>> + brcmf_txflowblock_if(ifp,
>> BRCMF_NETIF_STOP_REASON_FLOW,
>> + flow_blocked);
>> + }
>> + } else {
>> + fws->bus_flow_blocked = flow_blocked;
>> + if (!flow_blocked)
>> + brcmf_fws_schedule_deq(fws);
>> + else
>> + fws->stats.bus_flow_block++;
>> + }
>> }
>>
> Thanks for the code. I run a quick test and it looks fine.
> I added some prints to check the progress:
> len - is number of items in the queue
> max - is max number of entries in the queue
>
> [ 89.407856] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1789 max 2048
> [ 89.414682] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1790 max 2048
> [ 89.421497] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1791 max 2048
> [ 93.970466] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1524 max 2048
> [ 93.977520] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1525 max 2048
> [ 93.984572] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1526 max 2048
>
> I will some run more WLAN tests tomorrow to make sure.
> When I'm done testing I will update my patch as well and let you know.
>
> I came across this issue when I tried to connect to my WLAN access
> point. The bug was triggered due to a lot of background traffic
> (broadcast and multicast) in the network filling up the queue.
> It's now fixed so that the queue is not flooded. Now I move on to the
> next issue. It's still difficult to connect because the background
> eats up all the bandwidth (this is not a constant issue but it happens
> from time to time depending on the background load).
> For queueing-mode there seems to exist logic for handling multicast
> traffic separately but for the SDIO case (no-queueing) normal traffic
> gets same precedence as multicast traffic.
I am a bit confused by the term "background traffic". There is a
specific WMM-AC_BK that I would refer to as background traffic. Most
traffic is set to AC_BE regardless of it being unicast or bc/mc.
Changing the priority like that for multicast is a very bad idea.
> I'm considering something like this:
> --- a/drivers/net/wireless/brcm80211/brcmfmac/fwsignal.c
> +++ b/drivers/net/wireless/brcm80211/brcmfmac/fwsignal.c
> @@ -1900,6 +1902,8 @@ int brcmf_fws_process_skb(struct brcmf_if *ifp,
> struct sk_buff *skb)
> drvr->tx_multicast += !!multicast;
>
> if (fws->avoid_queueing) {
> + if (multicast)
> + skb->priority = PRIO_8021D_NONE;
> rc = brcmf_proto_txdata(drvr, ifp->ifidx, 0, skb);
> if (rc < 0) {
> (void)brcmf_proto_hdrpull(drvr, false, skb, NULL);
>
> It feels a little hacky.
>
> I would like to use the drvr->tx_multicast instead, if possible.
> The problem is that the "drvr" struct is not passed down to the sdio layer.
> One could update bcdc.c : brcmf_proto_bcdc_txdata() to pass
> "drcr->multicast" to the SDIO layer but not all bus implementations
> need this information (USB for instance)
>
> Any suggestions?
I suspect that we fixed your issue with commit 92121e69de8a ("brcmfmac:
Properly set carrier state of netdev."). That is if your issue was
caused on an older kernel.
Regards,
Arend
^ permalink raw reply
* RE: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded
From: Grumbach, Emmanuel @ 2016-07-14 9:08 UTC (permalink / raw)
To: Kalle Valo, Prarit Bhargava
Cc: Luca Coelho, linux-kernel@vger.kernel.org, linuxwifi,
Berg, Johannes, Ivgi, Chaya Rachel, netdev@vger.kernel.org,
Sharon, Sara, linux-wireless@vger.kernel.org
In-Reply-To: <87oa60y7ff.fsf@kamboji.qca.qualcomm.com>
>
> Prarit Bhargava <prarit@redhat.com> writes:
>
> > On 07/13/2016 03:24 AM, Luca Coelho wrote:
> >
> >> I totally agree with Emmanuel and Kalle. We should not change this.
> >> It is a design decision to return an error when the interface is
> >> down, this is very common with other subsystems as well.
> >
> > Please show me another subsystem or driver that does this. I've
> > looked around the kernel but cannot find one that updates the firmware
> > and implements new features on the fly like this. I have come across
> > several drivers that allow for an update, but they do not implement
> > new features based on the firmware.
> >
> > Additionally, what happens when someone back revs firmware versions
> > (which happens far more than you and I would expect)? Does that mean
> > I now go from a functional system to a non-functional system wrt to
> userspace?
>
> I'm not following, what do you mean exactly? Why are you talking updating
> the firmware?
>
> So when we talk about "loading firmware" we mean that the driver pushes
> the firmware image to to the chipset. And then the interface is down the
> chipset is powered down and the RAM on it will be erased. That's the general
> idea anyway, I haven't checked how iwlwifi exactly works in this case but
> Luca or Emmanuel can correct me.
This is correct.
>
> >> The userspace should be able to handle errors and report something
> >> like "unavailable" when this kind of error is returned.
> >
> > I myself have made the same arguments wrt to cpufreq code & bad
> > userspace choices. I just went through this a few months back with
> > what went from a simple patch and turned out to be a hideous patch in
> > cpufreq. You cannot break userspace like this.
>
> Don't get me wrong, I'm a strong supporter of stable user space interfaces
> and I always try to adher to that. But there's a limit for everything. If I'm
> understanding correctly, what you mean is that the kernel should never
> return an error because an application doesn't handle errors gracefully.
> Sorry, but that doesn't make sense to me.
>
> > See commit 51443fbf3d2c ("cpufreq: intel_pstate: Fix intel_pstate
> > powersave min_perf_pct value"). What should have been a trivial
> > change resulted in a massive change because of broken userspace.
>
> In that cpufreq case I understand, it was about a combination of
> configuration values which broke the user space. But here we are just
> dealing with a simple error value, nothing fancy.
>
> >> I'm not sure EIO is the best we can have, but for me that's exactly
> >> what it is. The thermal zone *is* there, but cannot be accessed
> >> because the firmware is not available. I'm okay to change it to
> >> EBUSY, if that would help userspace, but I think that's a bit
> >> misleading. The device is not busy, on the contrary, it's not even running
> at all.
> >>
> >
> > I understand that, but by returning -EIO we end up with an error.
> >
> >> Furthermore, I don't think this is "breaking userspace" in the sense
> >> of being a regression.
> >
> > I run (let's say 4.5 kernel). sensors works. I update to 4.7.
> > sensors doesn't work. How is that not a regression? That's _exactly_
> > what it should be reported as.
>
> Sure, it's a regression in a way. But that's how the user space app you are
> using is implemented, the same problem would happen with any driver
> returning errors.
>
> >> The userspace API has always been implemented with the possibility of
> >> returning errors. It's not a good design if a single device
> >> returning an error causes all the other devices to also fail.
> >>
> >
> > If that were the case we would never have to worry about "breaking
> userspace"?
> > For any kernel change I could just say that the userspace design was
> > bad and be done with it. Why fix anything then?
>
> Because we are talking about a simple error value.
>
> > I don't see any harm in waiting to register the sysfs files for hwmon
> > until the firmware has been validated.
>
> I'm against of that because it's bad software design. It's standard practise in
> Linux that drivers register their capabilities during driver probe time so that
> user space can query them whenever needed. I assume a properly behaving
> user space app would want to know about all the available sensors once the
> driver is initialised and your suggestion would break that.
>
> > IIUC, the up/down'ing of the device doesn't happen that often (during
> > initial boot, and suspend/resume, switching wifi connections,
> > shutdown?).
>
> Basically it can happen anytime, this is fully controlled by user space.
> There's no point of trying to make any assumptions as they won't hold
> anyway.
>
> > This would make the iwlwifi community happy (IMO) and sensors would
> > still work. At the same time I could write a patch for lm-sensors to
> > fix this issue if it comes up in future versions.
> > [Aside: I'm going to have the reproducing system available today and
> > will test this out. It looks like just moving some code around.]
>
> Another option, but still a bad one I don't like, is that you change the kernel
> interface to ignore all errors from drivers (like iwlwifi). This way drivers don't
> need to make ugly workarounds.
>
> > The bottom line is that lm-sensors is currently broken with this
> > change in iwlwifi. AFAICT, no other thermal device returns an error
> > this way, and IMO that means the iwlwifi driver is doing something new
> > and unexpected wrt to userspace.
>
> I haven't checked but I suspect ath10k has a similar problem when interface
> is down.
>
> --
> Kalle Valo
^ permalink raw reply
* Re: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded
From: Stanislaw Gruszka @ 2016-07-14 9:24 UTC (permalink / raw)
To: Grumbach, Emmanuel
Cc: prarit@redhat.com, linux-kernel@vger.kernel.org, linuxwifi,
Coelho, Luciano, Berg, Johannes, kvalo@codeaurora.org,
Ivgi, Chaya Rachel, netdev@vger.kernel.org, Sharon, Sara,
linux-wireless@vger.kernel.org
In-Reply-To: <1468261650.20877.14.camel@intel.com>
On Mon, Jul 11, 2016 at 06:27:30PM +0000, Grumbach, Emmanuel wrote:
> I guess that works, but it seems wrong to me. Usually, registration
> should happen only upon INIT, and yes, at that time the firmware is not
> ready to provide the information yet.
<snip>
> >
> > As can be seen in the current code base, iwl_mvm_tzone_get_temp()
> > will return
> > -EIO 100% of the time when the firmware doesn't support reading the
If I understad correctly this error happen 100% of the time, not only
during init. Hence seems there is an issue here, i.e. cur_ucode is not
marked correctly as IWL_UCODE_REGULAR or iwl_mvm_get_temp() fail
100% of the time (iwl_mvm_is_tt_in_fw() incorrecly return true on
Prarit device ? ).
BTW, you implement thermal_zone device, but do you also need hwmon
device? Perhaps using theramal_zone_params no_hwmon option would be
proper here?
Stanislaw
^ permalink raw reply
* RE: [PATCH RESEND] iwlwifi, Do not implement thermal zone unless ucode is loaded
From: Grumbach, Emmanuel @ 2016-07-14 9:44 UTC (permalink / raw)
To: Stanislaw Gruszka
Cc: prarit@redhat.com, linux-kernel@vger.kernel.org, linuxwifi,
Coelho, Luciano, Berg, Johannes, kvalo@codeaurora.org,
Ivgi, Chaya Rachel, netdev@vger.kernel.org, Sharon, Sara,
linux-wireless@vger.kernel.org
In-Reply-To: <20160714092408.GA27271@redhat.com>
>
> On Mon, Jul 11, 2016 at 06:27:30PM +0000, Grumbach, Emmanuel wrote:
> > I guess that works, but it seems wrong to me. Usually, registration
> > should happen only upon INIT, and yes, at that time the firmware is
> > not ready to provide the information yet.
> <snip>
> > >
> > > As can be seen in the current code base, iwl_mvm_tzone_get_temp()
> > > will return -EIO 100% of the time when the firmware doesn't support
> > > reading the
>
> If I understad correctly this error happen 100% of the time, not only during
> init. Hence seems there is an issue here, i.e. cur_ucode is not marked
> correctly as IWL_UCODE_REGULAR or iwl_mvm_get_temp() fail 100% of the
> time (iwl_mvm_is_tt_in_fw() incorrecly return true on Prarit device ? ).
Cur_ucode will not be IWL_UCODE_REGULAR until you load the firmware which
will happen upon ifup.
>
> BTW, you implement thermal_zone device, but do you also need hwmon
> device? Perhaps using theramal_zone_params no_hwmon option would be
> proper here?
That's an interesting direction. I'd have to check, but TBH, I am not familiar with
that code. Luca was very involved during the development but he is not available
right now. I will be back more the less when the merge window will close :)
>
> Stanislaw
^ permalink raw reply
* Re: iwlwifi + wpa2-leap + multiple AP's = call trace
From: Emmanuel Grumbach @ 2016-07-14 9:50 UTC (permalink / raw)
To: Ismael Farfán, linuxwifi; +Cc: Arend Van Spriel, linux-wireless
In-Reply-To: <CANXECd4T6xohJMhLZzXN8jphowQjWyc7DmxsRhfQJRi5u69mmQ@mail.gmail.com>
On Wed, Jul 13, 2016 at 9:33 PM, Ismael Farfán <sulfurfff@gmail.com> wrote:
> Hi
>
> I'm using wicd, so changed from wext to nl80211 and I don't see the
> trace anymore... it still refuses to connect though, so I'll check
> what's wrong with my configuration.
>
> Do you want me to collect some information regarding wpa_supplicant
> and wext from my system?
> If so, how?
I am very busy right now, so I doubt I will have the time to look at
it. So let's drop it unless it still doesn't work for you.
>
>
> 2016-07-13 11:31 GMT-05:00 Emmanuel Grumbach <egrumbach@gmail.com>:
>> On Jul 13, 2016 9:23 AM, "Arend Van Spriel" <arend.vanspriel@broadcom.com>
>> wrote:
>>>
>>>
>>>
>>> On 13-7-2016 2:29, Ismael Farfán wrote:
>>> > Hello list
>>> >
>>> > I searched this error around and didn't find anything, so here it goes.
>>> >
>>> > Today I tried to connect to an enterprise network, which means,
>>> > literally, tens of AP's sharing the same name... the network requieres
>>> > user/password authentication (wpa2-leap).
>>> >
>>> > I'm using Arch
>>> > $ uname -a
>>> > Linux 4.6.3-1-ARCH #1 SMP PREEMPT Fri Jun 24 21:19:13 CEST 2016 x86_64
>>> > GNU/Linux
>>> >
>>> > Since the thing just didn't connect, I checked dmesg and found this:
>>> >
>>> > Any ideas?
>>>
>>> From the stack trace it seems wpa_supplicant on Arch is using WEXT API.
>>> You could try and change it to use NL80211 API. Personally, I have not
>>> used Arch Linux so no idea where to change that.
>>>
>>> The warning is here [1]:
>>>
>>> 503 IWL_DEBUG_TE(mvm, "Add new TE, duration %d TU\n",
>>> 504 le32_to_cpu(te_cmd->duration));
>>> 505
>>> 506 spin_lock_bh(&mvm->time_event_lock);
>>> 507 if (WARN_ON(te_data->id != TE_MAX)) {
>>> 508 spin_unlock_bh(&mvm->time_event_lock);
>>> 509 return -EIO;
>>> 510 }
>>>
>>> It seems this function is called with te_data that is already in use,
>>> but that is my uneducated guess so I may be wrong.
>>>
>>
>> This is right :)
>>
>> Can you please record tracing of this?
>>
>> Thank you.
>>
>>> Regards,
>>> Arend
>>>
>>> [1]
>>>
>>> http://lxr.free-electrons.com/source/drivers/net/wireless/intel/iwlwifi/mvm/time-event.c#L507
>>> >
>>> > [ 83.030072] wifi0: aborting authentication with xx:xx:xx:xx:xx:xx
>>> > by local choice (Reason: 3=DEAUTH_LEAVING)
>>> > [ 83.030073] ------------[ cut here ]------------
>>> > [ 83.030082] WARNING: CPU: 2 PID: 1087 at
>>> > drivers/net/wireless/intel/iwlwifi/mvm/time-event.c:507 iwl_mvm_tim
>>> > e_event_send_add+0x1c6/0x200 [iwlmvm]
>>> > [ 83.030157] CPU: 2 PID: 1087 Comm: wpa_supplicant Tainted: G
>>> > O 4.6.3-1-ARCH #1
>>> > [ 83.030158] Hardware name: Notebook
>>> > P65_P67SA /P65_P67SA
>>> > , BIOS 1.03.01 07/22/2015
>>> > [ 83.030160] 0000000000000286 0000000009e998bc ffff880403a5b940
>>> > ffffffff812e54c2
>>> > [ 83.030162] 0000000000000000 0000000000000000 ffff880403a5b980
>>> > ffffffff8107a6bb
>>> > [ 83.030164] 000001fb81a8a180 ffff88041b443580 ffff88041c329548
>>> > 00000000fffffffb
>>> > [ 83.030167] Call Trace:
>>> > [ 83.030172] [<ffffffff812e54c2>] dump_stack+0x63/0x81
>>> > [ 83.030174] [<ffffffff8107a6bb>] __warn+0xcb/0xf0
>>> > [ 83.030176] [<ffffffff8107a7ed>] warn_slowpath_null+0x1d/0x20
>>> > [ 83.030181] [<ffffffffa0b068d6>]
>>> > iwl_mvm_time_event_send_add+0x1c6/0x200 [iwlmvm]
>>> > [ 83.030184] [<ffffffff810c4772>] ? up+0x32/0x50
>>> > [ 83.030187] [<ffffffff810d371b>] ? wake_up_klogd+0x3b/0x50
>>> > [ 83.030189] [<ffffffff810d3c19>] ? console_unlock+0x4e9/0x590
>>> > [ 83.030193] [<ffffffffa0b07520>]
>>> > iwl_mvm_protect_session+0x220/0x280 [iwlmvm]
>>> > [ 83.030196] [<ffffffffa0af100e>] ? iwl_mvm_ref_sync+0x2e/0x140
>>> > [iwlmvm]
>>> > [ 83.030199] [<ffffffff810d423f>] ? vprintk_default+0x1f/0x30
>>> > [ 83.030202] [<ffffffffa0af154a>]
>>> > iwl_mvm_mac_mgd_prepare_tx+0x5a/0xa0 [iwlmvm]
>>> > [ 83.030216] [<ffffffffa0c8e198>] ieee80211_mgd_deauth+0x338/0x4d0
>>> > [mac80211]
>>> > [ 83.030219] [<ffffffff810b2333>] ? enqueue_entity+0x323/0xd70
>>> > [ 83.030228] [<ffffffffa0c579b8>] ieee80211_deauth+0x18/0x20
>>> > [mac80211]
>>> > [ 83.030237] [<ffffffffa05a1b8f>] cfg80211_mlme_deauth+0x9f/0x1a0
>>> > [cfg80211]
>>> > [ 83.030242] [<ffffffffa05a62da>] cfg80211_disconnect+0x9a/0x200
>>> > [cfg80211]
>>> > [ 83.030248] [<ffffffffa05c4529>]
>>> > cfg80211_mgd_wext_siwessid+0xa9/0x170 [cfg80211]
>>> > [ 83.030255] [<ffffffffa05c3512>] cfg80211_wext_siwessid+0x22/0x40
>>> > [cfg80211]
>>> > [ 83.030258] [<ffffffff815af803>] ioctl_standard_iw_point+0x133/0x350
>>> > [ 83.030264] [<ffffffffa05c34f0>] ?
>>> > cfg80211_wext_giwessid+0x50/0x50 [cfg80211]
>>> > [ 83.030266] [<ffffffff810a4a82>] ? wake_up_q+0x32/0x70
>>> > [ 83.030268] [<ffffffff815b0930>] ? iw_handler_get_private+0x60/0x60
>>> > [ 83.030271] [<ffffffff815afd07>] ioctl_standard_call+0x87/0xd0
>>> > [ 83.030273] [<ffffffff815afc80>] ?
>>> > call_commit_handler.part.4+0x30/0x30
>>> > [ 83.030275] [<ffffffff815afc10>] wireless_process_ioctl+0x1f0/0x230
>>> > [ 83.030278] [<ffffffff814b52fe>] ? dev_get_by_name_rcu+0x5e/0x80
>>> > [ 83.030280] [<ffffffff815aff58>] wext_handle_ioctl+0x78/0xd0
>>> > [ 83.030283] [<ffffffff814d7777>] dev_ioctl+0x2a7/0x5a0
>>> > [ 83.030285] [<ffffffff8149dc86>] sock_ioctl+0x126/0x290
>>> > [ 83.030287] [<ffffffff81209be3>] do_vfs_ioctl+0xa3/0x5d0
>>> > [ 83.030290] [<ffffffff811f7362>] ? vfs_write+0x142/0x190
>>> > [ 83.030292] [<ffffffff8120a189>] SyS_ioctl+0x79/0x90
>>> > [ 83.030294] [<ffffffff815c71b2>] entry_SYSCALL_64_fastpath+0x1a/0xa4
>>> > [ 83.030296] ---[ end trace 8247b235a66a1a21 ]---
>>> >
>>> >
>>> >
>>> >
>>> >
>>> --
>>> To unsubscribe from this list: send the line "unsubscribe linux-wireless"
>>> in
>>> the body of a message to majordomo@vger.kernel.org
>>> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
>
>
> --
> Do not let me induce you to satisfy my curiosity, from an expectation,
> that I shall gratify yours. What I may judge proper to conceal, does
> not concern myself alone.
^ permalink raw reply
* [PATCH] staging: rtl8723au: hal: check BT_Active and BT_State with correct bit pattern
From: Colin King @ 2016-07-14 10:21 UTC (permalink / raw)
To: Larry Finger, Jes Sorensen, Greg Kroah-Hartman,
Bhaktipriya Shridhar, Bhumika Goyal, Amitoj Kaur Chawla,
Shivani Bhardwaj, Daniil Leshchev, Ksenija Stanojevic,
linux-wireless, devel
Cc: linux-kernel
From: Colin Ian King <colin.king@canonical.com>
BT_Active and BT_State are being masked with 0x00ffffff so it the subsequent
comparisons with 0xffffffff are therefore a buggy check. Instead, check them
against 0x00ffffff.
Unfortunately I couldn't find a datasheet or hardware to see if 0xffffffff
is an expected invalid bit pattern that should be checked before BT_Active and
BT_State are masked with 0x00ffffff, so for now, this fix seems like the least
risky approach.
Signed-off-by: Colin Ian King <colin.king@canonical.com>
---
drivers/staging/rtl8723au/hal/rtl8723a_bt-coexist.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/staging/rtl8723au/hal/rtl8723a_bt-coexist.c b/drivers/staging/rtl8723au/hal/rtl8723a_bt-coexist.c
index bfcbd7a..6989580 100644
--- a/drivers/staging/rtl8723au/hal/rtl8723a_bt-coexist.c
+++ b/drivers/staging/rtl8723au/hal/rtl8723a_bt-coexist.c
@@ -9824,7 +9824,7 @@ void BTDM_CheckBTIdleChange1Ant(struct rtw_adapter *padapter)
BT_Polling = rtl8723au_read32(padapter, regBTPolling);
RTPRINT(FBT, BT_TRACE, ("[DM][BT], BT_Polling(0x%x) =%x\n", regBTPolling, BT_Polling));
- if (BT_Active == 0xffffffff && BT_State == 0xffffffff && BT_Polling == 0xffffffff)
+ if (BT_Active == 0x00ffffff && BT_State == 0x00ffffff && BT_Polling == 0xffffffff)
return;
if (BT_Polling == 0)
return;
--
2.8.1
^ permalink raw reply related
* Re: [PATCH] wlcore/wl18xx: mesh: added initial mesh support for wl8
From: Kalle Valo @ 2016-07-14 12:04 UTC (permalink / raw)
To: Machani, Yaniv
Cc: linux-kernel@vger.kernel.org, Hahn, Maital, Eliad Peller,
Mishol, Guy, Johannes Berg, Arik Nemtsov,
linux-wireless@vger.kernel.org, netdev@vger.kernel.org
In-Reply-To: <AE1C82FB3D0EC64DB1F752C81CBD1101391FC336@DFRE01.ent.ti.com>
"Machani, Yaniv" <yanivma@ti.com> writes:
> On Tue, Jun 28, 2016 at 13:41:35, Machani, Yaniv wrote:
>> Guy; Johannes Berg; Arik Nemtsov; linux-wireless@vger.kernel.org;
>> netdev@vger.kernel.org
>> Subject: [PATCH] wlcore/wl18xx: mesh: added initial mesh support for
>> wl8
>>
>> From: Maital Hahn <maitalm@ti.com>
>>
>> 1. Added support for interface and role of mesh type.
>> 2. Enabled enable/start of mesh-point role,
>> and opening and closing a connection with a mesh peer.
>> 3. Added multirole combination of mesh and ap
>> under the same limits of dual ap mode.
>> 4. Add support for 'sta_rc_update' opcode for mesh IF.
>> The 'sta_rc_update' opcode is being used in mesh_plink.c.
>> Add support in wlcore to handle this opcode correctly for mesh (as
>> opposed to current implementation that handles STA only).
>> 5. Bumped the firmware version to support new Mesh functionality
>>
>> Signed-off-by: Maital Hahn <maitalm@ti.com>
>> Signed-off-by: Yaniv Machani <yanivma@ti.com>
>> ---
>
> Any comments on this patch ? Can this be pulled ?
I'm away this week, will look at it next week. But it's on my queue:
https://patchwork.kernel.org/patch/9202707/
--
Kalle Valo
^ permalink raw reply
* Re: [PATCH v4 1/3] Documentation: dt: net: add ath9k wireless device binding
From: Arnd Bergmann @ 2016-07-14 12:33 UTC (permalink / raw)
To: Martin Blumenstingl
Cc: ath9k-devel, devicetree, linux-wireless, ath9k-devel, mcgrof,
galak, ijc+devicetree, mark.rutland, pawel.moll, robh+dt, kvalo,
chunkeey, arend.vanspriel, julian.calaby
In-Reply-To: <CAFBinCBYmpBVph9vmWcPT512o2ObgmYn4naS2cTrpL61q6u1xw@mail.gmail.com>
On Monday, July 11, 2016 11:21:26 PM CEST Martin Blumenstingl wrote:
> On Mon, Jul 11, 2016 at 12:01 AM, Arnd Bergmann <arnd@arndb.de> wrote:
> >> ath9k reads the data from the EEPROM into memory. With that property
> >> disabled ath9k simply assumes that the endianness of the values in the
> >> EEPROM are having the correct endianness for the host system (in other
> >> words: no swap is being applied).
> >> I am not sure I understand you correctly, but isn't what you are
> >> explaining an issue in the ath9k code, rather than in this
> >> documentation?
> >
> > I looked at the code more to find that out now, but I'm more confused
> > now, as the eeprom seems to be read as a byte stream, and the endianess
> > conversion that the driver performs is not on the data values in it,
> > but seems to instead swap the bytes in each 16-bit word, regardless
> > of the contents (the values inside of the byte stream are always
> > interpreted as big-endian). Is that a correct observation?
> that seems to be the case for the ar9003 eeprom. Other implementations
> are doing it different, look at ath9k_hw_ar9287_check_eeprom for
> example: first ath9k_hw_nvram_swap_data checks the two magic bytes at
> the beginning of the data and swaps the bytes in each 16-bit word if
> the magic bytes don't match the magic bytes for the "native system
> endianness" (see AR5416_EEPROM_MAGIC). Then more swapping is applied.
> I asked for more details about the EEPROM format (specifically the
> endianness part) here [0] as I don't have access to the datasheets
> (all I have is the ath9k code)
Ok.
> > What I see in ath_pci_eeprom_read() is that the 16-bit words are taken
> > from the lower 16 bit of the little-endian AR_EEPROM_STATUS_DATA
> > register. As this is coming from a PCI register, it must have a device
> > specific endianess that is identical on all CPUs, so in the description
> > above, mentioning CPU endianess is a bit confusing. I could not find
> > the code that does the conditional byteswap, instead this function
> >
> > static bool ar9300_eeprom_read_byte(struct ath_hw *ah, int address,
> > u8 *buffer)
> > {
> > u16 val;
> >
> > if (unlikely(!ath9k_hw_nvram_read(ah, address / 2, &val)))
> > return false;
> >
> > *buffer = (val >> (8 * (address % 2))) & 0xff;
> > return true;
> > }
> >
> > evidently assumes that the lower 8 bit of the 16-bit data from PCI
> > come first, i.e. it byteswaps on big-endian CPUs to get the bytestream
> > back into the order in which it is stored in the EEPROM.
> Please have a look at the ath9k_hw_nvram_swap_data function and
> eeprom_ops.check_eeprom (for example ath9k_hw_ar9287_check_eeprom).
> These are performing the conditional swapping (in addition to whatever
> previous swapping there was).
> The basic code works like this: read the full EEPROM data into memory
> (either from PCI bus, ath9k_platform_data or request_firmware), then
> eeprom_ops.check_eeprom will call ath9k_hw_nvram_swap_data for 16-bit
> word swapping and afterwards the check_eeprom implementation will doe
> further swapping.
> Apart from that your findings seem correct (at least this is identical
> to how I would interpret the code).
Ok, so my interpretation of what this is done for is that the
swap in ath9k_hw_nvram_swap_data() is done to compensate for
the data that is read byte-reversed from the PCI bus and it
is does not swap when the data is read from a file. The result
is a structure with big-endian 16-bit and 32-bit members but
all fields in the right place.
The swapping in ath9k_hw_ar9287_check_eeprom() then turns the
big-endian fields into little-endian fields so it can be used
on little-endian CPUs without going through le16_to_cpu().
However, the whole thing still looks fragile to me as it
doesn't seem to handle the case where we want to swap the
values but not the bus.
My guess is that we still want to fix the driver to handle
this more consistently in order to decide whether a DT property
is needed or not.
> > Interestingly, this also seems to happen for ath_ahb_eeprom_read()
> > even though on that one the value does not get swapped by the bus
> > accessor, so presumably big-endian machines with a ahb based ath9k
> > store their eeprom byte-reversed?
> on AHB the eeprom data has to be provided via ath9k_platform_data /
> request_firmware mechanism. Thus there is no bus specific swapping,
> only the ath9k_hw_nvram_swap_data / eeprom_ops.check_eeprom swapping
> is applied in this case.
I guess the header then indicates that none of the swapping is
performed.
Arnd
^ permalink raw reply
* Re: [PATCH 0/4 v1] Refactoring ieee80211_iface_work
From: Alex Briskin @ 2016-07-14 13:04 UTC (permalink / raw)
To: Arend Van Spriel, linux-wireless; +Cc: johannes
In-Reply-To: <ecb3e937-7fa9-f530-1b23-b793b86b1514@broadcom.com>
Hi Arend,
Thank you for suggestions!
I will definitely collapse the two commits you suggested.
Could you please elaborate on your naming suggestion.
In your opinion, should it be something like
mac80211_is handled_by_pkt_type? Or something entirely different?
Also, I have two more concerns about the suggested changes:
1. Necessity - whether these changes really improve readability of the code.
2. Precision - whether the code was split in logical manner.
On Thursday, July 14, 2016 09:56:19 AM Arend Van Spriel wrote:
> On 13-7-2016 22:19, Alex Briskin wrote:
> > Hi All,
> > This is my first patch(s).
>
> Hi Alex,
>
> As these patches are touching mac80211 it would be good to use
> 'mac80211: ' prefix.
>
> > I've decided to refactor ieee80211_iface_work function and break it down
> > to smaller better defined function.
> >
> > I think these changes make the code much more readable and do not impose
> > no overhead.
> >
> > I've tested these patches with sparse and checkpatch.pl
> >
> > Function names might not be descriptive enough.
> > Hope you find this useful.
> >
> > Alex Briskin (4):
> > 0) [28e464b19aaaba90c8946fb979b58709d55dffcf]
> >
> > Added new function ieee80211_is_skb_handled_by_pkt_type and moved
> > some code from ieee80211_iface_work to reduce complexity and
> > improve readability
> >
> > 1) [486e3d5abb4dc6361cdd923254a2b68d43dcdaba]
> >
> > Refactored code in ieee80211_is_skb_handled_by_pkt_type.
> > "if () {} else if ()" replaced by switch case.
>
> I would collapse these two patches in one patch.
>
> > 2) [9ef2eab8e831420bc6748a4466ffa6b7a99bf447]
> >
> > Added new function ieee80211_is_handled_by_frame_control and moved
> > some code from ieee80211_iface_work to it.
> >
> > 3) [1de8cdf9a0c05c6a21d9e43e5b55862f6efcf450]
> >
> > Added new function ieee80211_handle_by_vif_type with code from
> > ieee80211_iface_work.
> >
> > At this point ieee80211_iface_work seems to me much more readable
> > and better understood.
>
> You are allowed to have an opinion :-) The function naming of the three
> functions could be more consistent as you seem to drop a bit in every
> patch, ie. is_skb_handled_by -> is_handled_by -> handle_by.
>
> Regards,
> Arend
>
> > net/mac80211/iface.c | 264
> > +++++++++++++++++++++++++++++---------------------- 1 file changed, 150
> > insertions(+), 114 deletions(-)
^ permalink raw reply
* Re: [PATCH v4] brcmfmac: Decrease 8021x_cnt for dropped packets
From: Per Förlin @ 2016-07-14 14:31 UTC (permalink / raw)
To: Arend Van Spriel; +Cc: linux-wireless, arend
In-Reply-To: <b8e300a0-b3a5-7a5b-c954-5b55a4426287@broadcom.com>
2016-07-14 10:57 GMT+02:00 Arend Van Spriel <arend.vanspriel@broadcom.com>:
> On 13-7-2016 20:52, Per Förlin wrote:
>> 2016-07-13 13:20 GMT+02:00 Arend Van Spriel <arend.vanspriel@broadcom.com>:
>>> On 12-7-2016 12:23, Per Förlin wrote:
>>>> 2016-07-12 11:48 GMT+02:00 Arend Van Spriel <arend.vanspriel@broadcom.com>:
>>>>>
>>>>>
>>>>> On 12-7-2016 10:35, Per Förlin wrote:
>>>>>> 2016-07-06 11:53 GMT+02:00 Per Förlin <per.forlin@gmail.com>:
>>>>>>> I have now verified this patch on backports 4.4.
>>>>>>>
>>>>>>> 2016-04-12 23:55 GMT+02:00 <per.forlin@gmail.com>:
>>>>>>>> From: Per Forlin <per.forlin@gmail.com>
>>>>>>>>
>>>>>>>> This patch resolves an issue where pend_8021x_cnt was not decreased
>>>>>>>> on txfinalize. This caused brcmf_netdev_wait_pend8021x to timeout
>>>>>>>> because the counter indicated pending packets.
>>>>>>>>
>>>>>>>> WARNING: at .../brcmfmac/core.c:1289 brcmf_netdev_wait_pend8021x
>>>>>>>> (warn_slowpath_common)
>>>>>>>> (warn_slowpath_null)
>>>>>>>> (brcmf_netdev_wait_pend8021x [brcmfmac])
>>>>>>>> (send_key_to_dongle [brcmfmac])
>>>>>>>> (brcmf_cfg80211_del_key [brcmfmac])
>>>>>>>> (nl80211_del_key [cfg80211])
>>>>>>>> (genl_rcv_msg)
>>>>>>>> (netlink_rcv_skb)
>>>>>>>> (genl_rcv)
>>>>>>>> (netlink_unicast)
>>>>>>>> (netlink_sendmsg)
>>>>>>>> (sock_sendmsg)
>>>>>>>> (___sys_sendmsg)
>>>>>>>> (__sys_sendmsg)
>>>>>>>> (SyS_sendmsg)
>>>>>>>>
>>>>>>>> The solution is to pull back the header offset in case
>>>>>>>> of an error in txdata(), which may happen in case of
>>>>>> Clarification:
>>>>>>
>>>>>> txdata=brcmf_proto_bcdc_txdata()
>>>>>> brcmf_proto_bcdc_txdata(): Calls brcmf_proto_bcdc_hdrpush()
>>>>>>
>>>>>> The header needs to be pulled back in case of error otherwise
>>>>>> the error handling and cleanup up will fail to decrease the counter
>>>>>> of pending packets.
>>>>>
>>>>> Yes, this part of the patch is clear to me.
>>>>>
>>>> Thanks, I wasn't sure.
>>>>
>>>>>>>> packet overload in brcmf_sdio_bus_txdata.
>>>>>>>>
>>>>>>>> Overloading an WLAN interface is not an unlikely scenario.
>>>>>
>>>>> So here is where things start to look suspicious and I have mentioned
>>>>> this before. My thought here was "How the hell can you end up with a
>>>>> 2048 packets on the sdio queue", which I mentioned to you before. There
>>>>> is a high watermark on the queue upon which we do a netif_stop_queue()
>>>>> so network layer does not keep pushing tx packets our way. Looking
>>>>> further into this I found that we introduced a bug with commit
>>>>> 9cd18359d31e ("brcmfmac: Make FWS queueing configurable.") so we ended
>>>>> up doing nothing except increasing as statistics debug counter :-(
>>>>>
>>>> Is there a fix available for the high watermark issue or is it
>>>> something you will look into?
>>>>
>>>> To produce a load on the wlan interface I run
>>>> iperf -c 239.255.1.3 -u -b 10m -f m -i 60 -t 3000
>>>> and this is enough in my case to fill up the 2048 queue.
>>>>
>>>>>>>> In case of packet overload the error print "out of bus->txq"
>>>>>>>> is very verbose. To reduce SPAM degrade it to a debug print.
>>>>>>>>
>>>>>>>> Signed-off-by: Per Forlin <per.forlin@gmail.com>
>>>>>>>> ---
>>>>>>>> Change log:
>>>>>>>> v2 - Add variable to know whether the counter is increased or not
>>>>>>>> v3 - txfinalize should decrease the counter. Adjust skb header offset
>>>>>>>> v4 - Fix build error
>>>>>>>>
>>>>>>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c | 4 ++++
>>>>>>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c | 4 +++-
>>>>>>>> drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c | 2 +-
>>>>>>>> 3 files changed, 8 insertions(+), 2 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>>>>>>> index ed9998b..f342f7c 100644
>>>>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>>>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/core.c
>>>>>>>> @@ -541,6 +541,9 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success)
>>>>>>>> struct ethhdr *eh;
>>>>>>>> u16 type;
>>>>>>>>
>>>>>>>> + if (!ifp)
>>>>>>>> + goto free;
>>>>>>>> +
>>>>>
>>>>> This may not be needed.
>>>>>
>>>> This is not strictly needed. I can remove it.
>>>>
>>>>>>>> eh = (struct ethhdr *)(txp->data);
>>>>>>>> type = ntohs(eh->h_proto);
>>>>>>>>
>>>>>>>> @@ -553,6 +556,7 @@ void brcmf_txfinalize(struct brcmf_if *ifp, struct sk_buff *txp, bool success)
>>>>>>>> if (!success)
>>>>>>>> ifp->stats.tx_errors++;
>>>>>>>>
>>>>>>>> +free:
>>>>>>>> brcmu_pkt_buf_free_skb(txp);
>>>>>>>> }
>>>>>>>>
>>>>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>>>>>>>> index f82c9ab..98cb83f 100644
>>>>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>>>>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>>>>>>>> @@ -1899,8 +1899,10 @@ int brcmf_fws_process_skb(struct brcmf_if *ifp, struct sk_buff *skb)
>>>>>>>>
>>>>>>>> if (fws->avoid_queueing) {
>>>>>>>> rc = brcmf_proto_txdata(drvr, ifp->ifidx, 0, skb);
>>>>>>>> - if (rc < 0)
>>>>>>>> + if (rc < 0) {
>>>>>>>> + (void)brcmf_proto_hdrpull(drvr, false, skb, &ifp);
>>>>>
>>>>> Could it be that the ifp is NULL pointer after brcmf_proto_hdrpull().
>>>>> Can you check. Better use tmp_ifp variable in this call as you have a
>>>>> valid ifp before this call for sure.
>>>>>
>>>> To be on the safe side I can use NULL as in param like you propose,
>>>> and use the available ifp.
>>>>
>>>>>>>> brcmf_txfinalize(ifp, skb, false);
>>>>>>>> + }
>>>>>>>> return rc;
>>>>>>>> }
>>>>>>>>
>>>>>>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>>>> index a14d9d9d..485e2ad 100644
>>>>>>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/sdio.c
>>>>>>>> @@ -2721,7 +2721,7 @@ static int brcmf_sdio_bus_txdata(struct device *dev, struct sk_buff *pkt)
>>>>>>>> *(u16 *)(pkt->cb) = 0;
>>>>>>>> if (!brcmf_sdio_prec_enq(&bus->txq, pkt, prec)) {
>>>>>>>> skb_pull(pkt, bus->tx_hdrlen);
>>>>>>>> - brcmf_err("out of bus->txq !!!\n");
>>>>>>>> + brcmf_dbg(INFO, "out of bus->txq !!!\n");
>>>>>
>>>>> Now that I understand the issue I want to keep this as error print as it
>>>>> should be very unlikely.
>>>> I would like to test this patch with the watermark fix to confirm this.
>>>
>>> Can you try this?
>>>
>>> Regards,
>>> Arend
>>>
>>> ---
>>> diff --git a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>>> b/drive
>>> index cd221ab..9f9024a 100644
>>> --- a/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>>> +++ b/drivers/net/wireless/broadcom/brcm80211/brcmfmac/fwsignal.c
>>> @@ -2469,10 +2469,22 @@ void brcmf_fws_bustxfail(struct brcmf_fws_info
>>> *fws, str
>>> void brcmf_fws_bus_blocked(struct brcmf_pub *drvr, bool flow_blocked)
>>> {
>>> struct brcmf_fws_info *fws = drvr->fws;
>>> + struct brcmf_if *ifp;
>>> + int i;
>>>
>>> - fws->bus_flow_blocked = flow_blocked;
>>> - if (!flow_blocked)
>>> - brcmf_fws_schedule_deq(fws);
>>> - else
>>> - fws->stats.bus_flow_block++;
>>> + if (fws->avoid_queueing) {
>>> + for (i = 0; i < BRCMF_MAX_IFS; i++) {
>>> + ifp = drvr->iflist[i];
>>> + if (!ifp || !ifp->ndev)
>>> + continue;
>>> + brcmf_txflowblock_if(ifp,
>>> BRCMF_NETIF_STOP_REASON_FLOW,
>>> + flow_blocked);
>>> + }
>>> + } else {
>>> + fws->bus_flow_blocked = flow_blocked;
>>> + if (!flow_blocked)
>>> + brcmf_fws_schedule_deq(fws);
>>> + else
>>> + fws->stats.bus_flow_block++;
>>> + }
>>> }
>>>
>> Thanks for the code. I run a quick test and it looks fine.
>> I added some prints to check the progress:
>> len - is number of items in the queue
>> max - is max number of entries in the queue
>>
>> [ 89.407856] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1789 max 2048
>> [ 89.414682] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1790 max 2048
>> [ 89.421497] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1791 max 2048
>> [ 93.970466] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1524 max 2048
>> [ 93.977520] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1525 max 2048
>> [ 93.984572] [brcmf_sdio_prec_enq] prec 2 prio 0 len 1526 max 2048
>>
>> I will some run more WLAN tests tomorrow to make sure.
>> When I'm done testing I will update my patch as well and let you know.
>>
>> I came across this issue when I tried to connect to my WLAN access
>> point. The bug was triggered due to a lot of background traffic
>> (broadcast and multicast) in the network filling up the queue.
>> It's now fixed so that the queue is not flooded. Now I move on to the
>> next issue. It's still difficult to connect because the background
>> eats up all the bandwidth (this is not a constant issue but it happens
>> from time to time depending on the background load).
>> For queueing-mode there seems to exist logic for handling multicast
>> traffic separately but for the SDIO case (no-queueing) normal traffic
>> gets same precedence as multicast traffic.
>
> I am a bit confused by the term "background traffic". There is a
> specific WMM-AC_BK that I would refer to as background traffic. Most
> traffic is set to AC_BE regardless of it being unicast or bc/mc.
> Changing the priority like that for multicast is a very bad idea.
>
Sorry for using "background traffic", I simply mean broadcast and
multicast traffic.
Normally broadcast traffic and multicast traffic are less time
critical. In my case
the broadcast traffic prevents clients to connect to the access point.
The connection traffic and the broadcast traffic have the same prio.
I will consider other options to improve this. I going away for a
couple of weeks now
and I hope to pick up this work again in mid August.
>> I'm considering something like this:
>> --- a/drivers/net/wireless/brcm80211/brcmfmac/fwsignal.c
>> +++ b/drivers/net/wireless/brcm80211/brcmfmac/fwsignal.c
>> @@ -1900,6 +1902,8 @@ int brcmf_fws_process_skb(struct brcmf_if *ifp,
>> struct sk_buff *skb)
>> drvr->tx_multicast += !!multicast;
>>
>> if (fws->avoid_queueing) {
>> + if (multicast)
>> + skb->priority = PRIO_8021D_NONE;
>> rc = brcmf_proto_txdata(drvr, ifp->ifidx, 0, skb);
>> if (rc < 0) {
>> (void)brcmf_proto_hdrpull(drvr, false, skb, NULL);
>>
>> It feels a little hacky.
>>
>> I would like to use the drvr->tx_multicast instead, if possible.
>> The problem is that the "drvr" struct is not passed down to the sdio layer.
>> One could update bcdc.c : brcmf_proto_bcdc_txdata() to pass
>> "drcr->multicast" to the SDIO layer but not all bus implementations
>> need this information (USB for instance)
>>
>> Any suggestions?
>
> I suspect that we fixed your issue with commit 92121e69de8a ("brcmfmac:
> Properly set carrier state of netdev."). That is if your issue was
> caused on an older kernel.
I'm running on 4.4.6 so I should have this patch in place already.
About the watermark fix.
It looks like the fix makes the eviction handling in sdio.c
brcmf_sdio_prec_enq()
obsolete. The function evicts packets with lower prio to make way for
packets with
higher prio. This only kicks in when the queue is full.
if (pktq_pfull(q, prec) || pktq_full(q))
// Give precedence
If pktq_full(q) is not full, pktq_pfull(q, prec) will not get full either
With the watermark-fix in place the queue never gets full.
>
> Regards,
> Arend
^ permalink raw reply
* Re: pull-request: wireless-drivers-next 2016-07-13
From: David Miller @ 2016-07-15 0:41 UTC (permalink / raw)
To: kvalo; +Cc: linux-wireless, netdev, linux-kernel
In-Reply-To: <87a8hlz91i.fsf@kamboji.qca.qualcomm.com>
From: Kalle Valo <kvalo@codeaurora.org>
Date: Wed, 13 Jul 2016 21:29:13 +0300
> here's a pull request for net-next. This time there are few conflicts
> due to the cfg80211 scan API changes, and one of them is easy to miss,
> so please pay extra attention to them. Otherwise there's not nothing
> really out of ordinary. Please note that I also pulled wireless-drivers
> to wireless-drivers-next to reduce the amount of conflicts.
>
> So about the conflicts, the obvious are notified by git:
...
> I have attached the output from git diff as an example how to resolve
> this, hopefully that helps. Please let me know if there are any problems
> or if you want to handle these differently.
Thanks for all of the conflict resolution info, it really helped.
I pushed this all out, please double check my work.
Thanks.
^ permalink raw reply
* Re: pull-request: wireless-drivers-next 2016-07-13
From: kbuild test robot @ 2016-07-15 3:09 UTC (permalink / raw)
To: Kalle Valo; +Cc: kbuild-all, David Miller, linux-wireless, netdev, linux-kernel
In-Reply-To: <87a8hlz91i.fsf@kamboji.qca.qualcomm.com>
[-- Attachment #1: Type: text/plain, Size: 2366 bytes --]
Hi,
[auto build test ERROR on wireless-drivers-next/master]
[cannot apply to v4.7-rc7 next-20160714]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
url: https://github.com/0day-ci/linux/commits/Kalle-Valo/pull-request-wireless-drivers-next-2016-07-13/20160714-023750
base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
config: sparc64-allmodconfig (attached as .config)
compiler: sparc64-linux-gnu-gcc (Debian 5.3.1-8) 5.3.1 20160205
reproduce:
wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# save the attached .config to linux build tree
make.cross ARCH=sparc64
All error/warnings (new ones prefixed by >>):
drivers/net/wireless/marvell/mwifiex/scan.c: In function 'mwifiex_cancel_scan':
>> drivers/net/wireless/marvell/mwifiex/scan.c:2024:12: error: variable 'info' has initializer but incomplete type
struct cfg80211_scan_info info = {
^
>> drivers/net/wireless/marvell/mwifiex/scan.c:2025:6: error: unknown field 'aborted' specified in initializer
.aborted = true,
^
>> drivers/net/wireless/marvell/mwifiex/scan.c:2025:17: warning: excess elements in struct initializer
.aborted = true,
^
drivers/net/wireless/marvell/mwifiex/scan.c:2025:17: note: (near initialization for 'info')
>> drivers/net/wireless/marvell/mwifiex/scan.c:2024:31: error: storage size of 'info' isn't known
struct cfg80211_scan_info info = {
^
>> drivers/net/wireless/marvell/mwifiex/scan.c:2024:31: warning: unused variable 'info' [-Wunused-variable]
vim +/info +2024 drivers/net/wireless/marvell/mwifiex/scan.c
2018 spin_unlock_irqrestore(&adapter->mwifiex_cmd_lock, cmd_flags);
2019 for (i = 0; i < adapter->priv_num; i++) {
2020 priv = adapter->priv[i];
2021 if (!priv)
2022 continue;
2023 if (priv->scan_request) {
> 2024 struct cfg80211_scan_info info = {
> 2025 .aborted = true,
2026 };
2027
2028 mwifiex_dbg(adapter, INFO,
---
0-DAY kernel test infrastructure Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all Intel Corporation
[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 46352 bytes --]
^ permalink raw reply
* Re: pull-request: wireless-drivers-next 2016-07-13
From: Kalle Valo @ 2016-07-15 7:21 UTC (permalink / raw)
To: kbuild test robot
Cc: kbuild-all, David Miller, linux-wireless, netdev, linux-kernel
In-Reply-To: <201607151143.oxJDgbga%fengguang.wu@intel.com>
kbuild test robot <lkp@intel.com> writes:
> Hi,
>
> [auto build test ERROR on wireless-drivers-next/master]
> [cannot apply to v4.7-rc7 next-20160714]
> [if your patch is applied to the wrong git tree, please drop us a note to help improve the system]
>
> url: https://github.com/0day-ci/linux/commits/Kalle-Valo/pull-request-wireless-drivers-next-2016-07-13/20160714-023750
> base: https://git.kernel.org/pub/scm/linux/kernel/git/kvalo/wireless-drivers-next.git master
> config: sparc64-allmodconfig (attached as .config)
> compiler: sparc64-linux-gnu-gcc (Debian 5.3.1-8) 5.3.1 20160205
> reproduce:
> wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
> chmod +x ~/bin/make.cross
> # save the attached .config to linux build tree
> make.cross ARCH=sparc64
>
> All error/warnings (new ones prefixed by >>):
>
> drivers/net/wireless/marvell/mwifiex/scan.c: In function 'mwifiex_cancel_scan':
>>> drivers/net/wireless/marvell/mwifiex/scan.c:2024:12: error: variable 'info' has initializer but incomplete type
> struct cfg80211_scan_info info = {
The diff was just an example to help Dave fix few conflicts in my pull
request, it was not meant to be applied directly.
--
Kalle Valo
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox