There are few cases where the tx skb is dropped but netstats is
not updated, fix this.
Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
drivers/net/wireless/ath/ath6kl/txrx.c | 12 ++++--------
1 files changed, 4 insertions(+), 8 deletions(-)
diff --git a/drivers/net/wireless/ath/ath6kl/txrx.c b/drivers/net/wireless/ath/ath6kl/txrx.c
index 82f2f5c..67206ae 100644
--- a/drivers/net/wireless/ath/ath6kl/txrx.c
+++ b/drivers/net/wireless/ath/ath6kl/txrx.c
@@ -362,15 +362,11 @@ int ath6kl_data_tx(struct sk_buff *skb, struct net_device *dev)
skb, skb->data, skb->len);
/* If target is not associated */
- if (!test_bit(CONNECTED, &vif->flags)) {
- dev_kfree_skb(skb);
- return 0;
- }
+ if (!test_bit(CONNECTED, &vif->flags))
+ goto fail_tx;
- if (WARN_ON_ONCE(ar->state != ATH6KL_STATE_ON)) {
- dev_kfree_skb(skb);
- return 0;
- }
+ if (WARN_ON_ONCE(ar->state != ATH6KL_STATE_ON))
+ goto fail_tx;
if (!test_bit(WMI_READY, &ar->flag))
goto fail_tx;
--
1.7.0.4
On 04/26/2012 05:56 PM, Vasanthakumar Thiagarajan wrote:
> There are few cases where the tx skb is dropped but netstats is
> not updated, fix this.
>
> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
Thanks, both patches applied.
Kalle
On 04/27/2012 07:09 AM, Vasanthakumar Thiagarajan wrote:
>>> bus_req = ath6kl_sdio_alloc_busreq(ar_sdio);
>>>
>>> - if (!bus_req)
>>> + if (!bus_req) {
>>> + ath6kl_err("Ran out of bus request buffer for tx\n");
>>> return -ENOMEM;
>>> + }
>>
>> I'm not sure about this one. There's a risk that this will spam the log.
>> Should it be a debug message instead? Or should we have instead
>> ath6kl_err_ratelimit()?
>
> This condition is very rare, I found it only through code review. I'm
> pretty sure we don't hit this very often.
I'm not worried how often it happens, I'm just worried that _when_ it
happens the warning might make things worse. For example, I personally
saw a case where flood of warnings prevented watchdog heartbeat from
happening which caused the whole system to reboot. Without the warnings
system would have worked just fine, just a bit more slowly.
Is it ok for you if I change the ath6kl_err() to WARN_ON_ONCE() (or
WARN_ONCE() if you prefer to keep the warning message)? This should be a
rare event anyway.
Kalle
Return status of ath6kl_htc_tx_issue() is ignored in
ath6kl_htc_tx_from_queue(), but failed tx packet is
is not cleaned up. To fix memory leak in this case, call
completion with error. Also, throw an error debug message
when tx fails in ath6kl_sdio_write_async() due to shortage
in bus request buffer.
Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
---
drivers/net/wireless/ath/ath6kl/htc_mbox.c | 8 +++++++-
drivers/net/wireless/ath/ath6kl/sdio.c | 4 +++-
2 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/drivers/net/wireless/ath/ath6kl/htc_mbox.c b/drivers/net/wireless/ath/ath6kl/htc_mbox.c
index 65310d5..8729803 100644
--- a/drivers/net/wireless/ath/ath6kl/htc_mbox.c
+++ b/drivers/net/wireless/ath/ath6kl/htc_mbox.c
@@ -850,6 +850,7 @@ static void ath6kl_htc_tx_from_queue(struct htc_target *target,
int bundle_sent;
int n_pkts_bundle;
u8 ac = WMM_NUM_AC;
+ int status;
spin_lock_bh(&target->tx_lock);
@@ -911,7 +912,12 @@ static void ath6kl_htc_tx_from_queue(struct htc_target *target,
ath6kl_htc_tx_prep_pkt(packet, packet->info.tx.flags,
0, packet->info.tx.seqno);
- ath6kl_htc_tx_issue(target, packet);
+ status = ath6kl_htc_tx_issue(target, packet);
+
+ if (status) {
+ packet->status = status;
+ packet->completion(packet->context, packet);
+ }
}
spin_lock_bh(&target->tx_lock);
diff --git a/drivers/net/wireless/ath/ath6kl/sdio.c b/drivers/net/wireless/ath/ath6kl/sdio.c
index 0384a0f..efe083f 100644
--- a/drivers/net/wireless/ath/ath6kl/sdio.c
+++ b/drivers/net/wireless/ath/ath6kl/sdio.c
@@ -552,8 +552,10 @@ static int ath6kl_sdio_write_async(struct ath6kl *ar, u32 address, u8 *buffer,
bus_req = ath6kl_sdio_alloc_busreq(ar_sdio);
- if (!bus_req)
+ if (!bus_req) {
+ ath6kl_err("Ran out of bus request buffer for tx\n");
return -ENOMEM;
+ }
bus_req->address = address;
bus_req->buffer = buffer;
--
1.7.0.4
On 04/26/2012 05:56 PM, Vasanthakumar Thiagarajan wrote:
> Return status of ath6kl_htc_tx_issue() is ignored in
> ath6kl_htc_tx_from_queue(), but failed tx packet is
> is not cleaned up. To fix memory leak in this case, call
> completion with error. Also, throw an error debug message
> when tx fails in ath6kl_sdio_write_async() due to shortage
> in bus request buffer.
>
> Signed-off-by: Vasanthakumar Thiagarajan <[email protected]>
[...]
> bus_req = ath6kl_sdio_alloc_busreq(ar_sdio);
>
> - if (!bus_req)
> + if (!bus_req) {
> + ath6kl_err("Ran out of bus request buffer for tx\n");
> return -ENOMEM;
> + }
I'm not sure about this one. There's a risk that this will spam the log.
Should it be a debug message instead? Or should we have instead
ath6kl_err_ratelimit()?
Kalle
On Thursday 26 April 2012 11:51 PM, Kalle Valo wrote:
> On 04/26/2012 05:56 PM, Vasanthakumar Thiagarajan wrote:
>> Return status of ath6kl_htc_tx_issue() is ignored in
>> ath6kl_htc_tx_from_queue(), but failed tx packet is
>> is not cleaned up. To fix memory leak in this case, call
>> completion with error. Also, throw an error debug message
>> when tx fails in ath6kl_sdio_write_async() due to shortage
>> in bus request buffer.
>>
>> Signed-off-by: Vasanthakumar Thiagarajan<[email protected]>
>
> [...]
>
>> bus_req = ath6kl_sdio_alloc_busreq(ar_sdio);
>>
>> - if (!bus_req)
>> + if (!bus_req) {
>> + ath6kl_err("Ran out of bus request buffer for tx\n");
>> return -ENOMEM;
>> + }
>
> I'm not sure about this one. There's a risk that this will spam the log.
> Should it be a debug message instead? Or should we have instead
> ath6kl_err_ratelimit()?
This condition is very rare, I found it only through code review. I'm
pretty sure we don't hit this very often.
Vasanth
On Friday 27 April 2012 12:04 PM, Kalle Valo wrote:
> On 04/27/2012 07:09 AM, Vasanthakumar Thiagarajan wrote:
>
>>>> bus_req = ath6kl_sdio_alloc_busreq(ar_sdio);
>>>>
>>>> - if (!bus_req)
>>>> + if (!bus_req) {
>>>> + ath6kl_err("Ran out of bus request buffer for tx\n");
>>>> return -ENOMEM;
>>>> + }
>>>
>>> I'm not sure about this one. There's a risk that this will spam the log.
>>> Should it be a debug message instead? Or should we have instead
>>> ath6kl_err_ratelimit()?
>>
>> This condition is very rare, I found it only through code review. I'm
>> pretty sure we don't hit this very often.
>
> I'm not worried how often it happens, I'm just worried that _when_ it
> happens the warning might make things worse. For example, I personally
> saw a case where flood of warnings prevented watchdog heartbeat from
> happening which caused the whole system to reboot. Without the warnings
> system would have worked just fine, just a bit more slowly.
Fair enough.
>
> Is it ok for you if I change the ath6kl_err() to WARN_ON_ONCE() (or
> WARN_ONCE() if you prefer to keep the warning message)? This should be a
> rare event anyway.
I'm fine with that, thanks.
Vasanth
On 05/22/2012 10:25 AM, Vasanthakumar Thiagarajan wrote:
>
> Oops, please forget this crap, mistakenly sent the older patches.
> I'm really sorry.
No worries :)
I will ignore the patches.
Kalle