diff mbox

[v4] brcmfmac: Decrease 8021x_cnt for dropped packets

Message ID 1460498140-18874-1-git-send-email-per.forlin@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Kalle Valo
Headers show

Commit Message

per.forlin@gmail.com April 12, 2016, 9:55 p.m. UTC
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
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 <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(-)

Comments

per.forlin@gmail.com July 6, 2016, 9:53 a.m. UTC | #1
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
> 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 <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;
> +
>         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);
>                         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");
>                 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
per.forlin@gmail.com July 12, 2016, 8:35 a.m. UTC | #2
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.

>> 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 <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;
>> +
>>         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);
>>                         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");
>>                 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Arend van Spriel July 12, 2016, 9:48 a.m. UTC | #3
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.

>>> 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@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.

>>>         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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
per.forlin@gmail.com July 12, 2016, 10:23 a.m. UTC | #4
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.
>
> Regards,
> Arend

Thanks for the review!

>
>>>>                 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@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

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, &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");
 		ret = -ENOSR;
 	} else {
 		ret = 0;