Return-path: Received: from mail-vk0-f66.google.com ([209.85.213.66]:36752 "EHLO mail-vk0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932085AbcGLKXb convert rfc822-to-8bit (ORCPT ); Tue, 12 Jul 2016 06:23:31 -0400 Received: by mail-vk0-f66.google.com with SMTP id r135so671747vkf.3 for ; Tue, 12 Jul 2016 03:23:30 -0700 (PDT) MIME-Version: 1.0 In-Reply-To: <38869535-4f77-f6d8-d8d0-cb01b3f9c65b@broadcom.com> References: <1460498140-18874-1-git-send-email-per.forlin@gmail.com> <38869535-4f77-f6d8-d8d0-cb01b3f9c65b@broadcom.com> From: =?UTF-8?Q?Per_F=C3=B6rlin?= Date: Tue, 12 Jul 2016 12:23:29 +0200 Message-ID: (sfid-20160712_122336_281701_656546AB) Subject: Re: [PATCH v4] brcmfmac: Decrease 8021x_cnt for dropped packets To: Arend Van Spriel Cc: linux-wireless@vger.kernel.org, arend@broadcom.com Content-Type: text/plain; charset=UTF-8 Sender: linux-wireless-owner@vger.kernel.org List-ID: 2016-07-12 11:48 GMT+02:00 Arend Van Spriel : > > > On 12-7-2016 10:35, Per Förlin wrote: >> 2016-07-06 11:53 GMT+02:00 Per Förlin : >>> I have now verified this patch on backports 4.4. >>> >>> 2016-04-12 23:55 GMT+02:00 : >>>> From: Per Forlin >>>> >>>> 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 >>>> --- >>>> 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. > > Regards, > Arend Thanks for the review! > >>>> ret = -ENOSR; >>>> } else { >>>> ret = 0; >>>> -- >>>> 2.1.4 >>>>