Re: [PATCH v4] brcmfmac: Decrease 8021x_cnt for dropped packets

2016-07-14 Thread Per Förlin 
2016-07-14 10:57 GMT+02:00 Arend Van Spriel :
> On 13-7-2016 20:52, Per Förlin wrote:
>> 2016-07-13 13:20 GMT+02:00 Arend Van Spriel :
>>> On 12-7-2016 12:23, Per Förlin wrote:
 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

Re: [PATCH v4] brcmfmac: Decrease 8021x_cnt for dropped packets

2016-07-14 Thread Arend Van Spriel 
On 13-7-2016 20:52, Per Förlin wrote:
> 2016-07-13 13:20 GMT+02:00 Arend Van Spriel :
>> On 12-7-2016 12:23, Per Förlin wrote:
>>> 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

Re: [PATCH v4] brcmfmac: Decrease 8021x_cnt for dropped packets

2016-07-13 Thread Per Förlin 
2016-07-13 13:20 GMT+02:00 Arend Van Spriel :
> On 12-7-2016 12:23, Per Förlin wrote:
>> 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,

Re: [PATCH v4] brcmfmac: Decrease 8021x_cnt for dropped packets

2016-07-13 Thread Arend Van Spriel 
On 12-7-2016 12:23, Per Förlin wrote:
> 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, );
>>
>> Could it be that the ifp is

Re: [PATCH v4] brcmfmac: Decrease 8021x_cnt for dropped packets

2016-07-12 Thread Per Förlin 
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, );
>
> 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

Re: [PATCH v4] brcmfmac: Decrease 8021x_cnt for dropped packets

2016-07-12 Thread 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.

>>> 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 
>>> ---
>>> 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, );

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)
>>>

Re: [PATCH v4] brcmfmac: Decrease 8021x_cnt for dropped packets

2016-07-12 Thread Per Förlin 
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.

>> packet overload in brcmf_sdio_bus_txdata.
>>
>> Overloading an WLAN interface is not an unlikely scenario.
>> 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;
>> +
>> 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, );
>> 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(>txq, pkt, prec)) {
>> skb_pull(pkt, bus->tx_hdrlen);
>> -   brcmf_err("out of bus->txq !!!\n");
>> +   brcmf_dbg(INFO, "out of bus->txq !!!\n");
>> 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] brcmfmac: Decrease 8021x_cnt for dropped packets

2016-07-06 Thread 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
> packet overload in brcmf_sdio_bus_txdata.
>
> Overloading an WLAN interface is not an unlikely scenario.
> 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;
> +
> 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, );
> 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(>txq, pkt, prec)) {
> skb_pull(pkt, bus->tx_hdrlen);
> -   brcmf_err("out of bus->txq !!!\n");
> +   brcmf_dbg(INFO, "out of bus->txq !!!\n");
> 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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4] brcmfmac: Decrease 8021x_cnt for dropped packets

2016-04-12 Thread per . forlin 
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
packet overload in brcmf_sdio_bus_txdata.

Overloading an WLAN interface is not an unlikely scenario.
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;
+
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, );
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(>txq, pkt, prec)) {
skb_pull(pkt, bus->tx_hdrlen);
-   brcmf_err("out of bus->txq !!!\n");
+   brcmf_dbg(INFO, "out of bus->txq !!!\n");
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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html