On 12-7-2016 10:35, Per Förlin wrote:
> 2016-07-06 11:53 GMT+02:00 Per Förlin <per.forlin@xxxxxxxxx>:
>> I have now verified this patch on backports 4.4.
>>
>> 2016-04-12 23:55 GMT+02:00 <per.forlin@xxxxxxxxx>:
>>> From: Per Forlin <per.forlin@xxxxxxxxx>
>>>
>>> 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.
>>> 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 :-(
>>> 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@xxxxxxxxx>
>>> ---
>>> 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.
>>> 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.
>>> 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.
Regards,
Arend
>>> ret = -ENOSR;
>>> } else {
>>> ret = 0;
>>> --
>>> 2.1.4
>>>
--
To unsubscribe from this list: send the line "unsubscribe linux-wireless" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
