[PATCH 1/2] ath6kl: Update netstats for some of the tx failrues in ath6kl_data_tx()

[PATCH 1/2] ath6kl: Update netstats for some of the tx failrues in ath6kl_data_tx()

[Vasanthakumar Thiagarajan]

There are few cases where the tx skb is dropped but netstats is
not updated, fix this.

Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
---
 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

[PATCH 2/2] ath6kl: Complete failed tx packet in ath6kl_htc_tx_from_queue()

[Vasanthakumar Thiagarajan]

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 <vthiagar@qca.qualcomm.com>
---
 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

Re: [PATCH 2/2] ath6kl: Complete failed tx packet in ath6kl_htc_tx_from_queue()

[Kalle Valo]

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 <vthiagar@qca.qualcomm.com>

[...]

>  	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

Re: [PATCH 2/2] ath6kl: Complete failed tx packet in ath6kl_htc_tx_from_queue()

[Vasanthakumar Thiagarajan]

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<vthiagar@qca.qualcomm.com>
>
> [...]
>
>>   	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

Re: [PATCH 2/2] ath6kl: Complete failed tx packet in ath6kl_htc_tx_from_queue()

[Kalle Valo]

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

Re: [PATCH 2/2] ath6kl: Complete failed tx packet in ath6kl_htc_tx_from_queue()

[Vasanthakumar Thiagarajan]

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

Re: [PATCH 1/2] ath6kl: Update netstats for some of the tx failrues in ath6kl_data_tx()

[Kalle Valo]

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 <vthiagar@qca.qualcomm.com>

Thanks, both patches applied.

Kalle

Re: [PATCH 2/2] ath6kl: Complete failed tx packet in ath6kl_htc_tx_from_queue()

[Kalle Valo]

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

Re: [PATCH 2/2] ath6kl: Complete failed tx packet in ath6kl_htc_tx_from_queue()

[Vasanthakumar Thiagarajan]

Oops, please forget this crap, mistakenly sent the older patches.
I'm really sorry.

Vasanth

On Tuesday 22 May 2012 12:52 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<vthiagar@qca.qualcomm.com>
> ---
>   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;

[PATCH 2/2] ath6kl: Complete failed tx packet in ath6kl_htc_tx_from_queue()

[Vasanthakumar Thiagarajan]

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 <vthiagar@qca.qualcomm.com>
---
 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