[PATCH 1/2] ath6kl: Make sure to allocate rx buffers after the endpoint connection

[PATCH 1/2] ath6kl: Make sure to allocate rx buffers after the endpoint connection

[Vasanthakumar Thiagarajan]

Rx buffers should be allocated for control and best effort endpoints only
after the enpoints connection is esablished. But this is done before the
endpoint connection is complete, we don't even the control and BE endpoints
that time. Move the buffer allocation after endpoint connection is over,
after ath6kl_init_hw_start(). Found in review, never seen any real issue
with this.

Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath6kl/core.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/core.c b/drivers/net/wireless/ath/ath6kl/core.c
index c4926cf..ce7d9b5 100644
--- a/drivers/net/wireless/ath/ath6kl/core.c
+++ b/drivers/net/wireless/ath/ath6kl/core.c
@@ -136,10 +136,6 @@ int ath6kl_core_init(struct ath6kl *ar)
 	ar->ac_stream_pri_map[WMM_AC_VI] = 2;
 	ar->ac_stream_pri_map[WMM_AC_VO] = 3; /* highest */
 
-	/* give our connected endpoints some buffers */
-	ath6kl_rx_refill(ar->htc_target, ar->ctrl_ep);
-	ath6kl_rx_refill(ar->htc_target, ar->ac2ep_map[WMM_AC_BE]);
-
 	/* allocate some buffers that handle larger AMSDU frames */
 	ath6kl_refill_amsdu_rxbufs(ar, ATH6KL_MAX_AMSDU_RX_BUFFERS);
 
@@ -182,6 +178,10 @@ int ath6kl_core_init(struct ath6kl *ar)
 		goto err_rxbuf_cleanup;
 	}
 
+	/* give our connected endpoints some buffers */
+	ath6kl_rx_refill(ar->htc_target, ar->ctrl_ep);
+	ath6kl_rx_refill(ar->htc_target, ar->ac2ep_map[WMM_AC_BE]);
+
 	/*
 	 * Set mac address which is received in ready event
 	 * FIXME: Move to ath6kl_interface_add()
-- 
1.7.0.4

[PATCH 2/2] ath6kl: Fix memory leak of rx packets in endpoint 0

[Vasanthakumar Thiagarajan]

htc_packet and htc_packet->buf_start are separately allocated
for endpoint 0. This is different for other endpoints where
packets are allocated as skb where htc_packet is skb->head
and they are freed properly. Free htc_packet and htc_packet->buf_start
separatly for endpoint 0.

Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
---
 drivers/net/wireless/ath/ath6kl/htc.c |   16 +++++++++++++++-
 1 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/drivers/net/wireless/ath/ath6kl/htc.c b/drivers/net/wireless/ath/ath6kl/htc.c
index c703ef9..e50cc8e 100644
--- a/drivers/net/wireless/ath/ath6kl/htc.c
+++ b/drivers/net/wireless/ath/ath6kl/htc.c
@@ -2372,7 +2372,21 @@ void ath6kl_htc_flush_rx_buf(struct htc_target *target)
 				   "htc rx flush pkt 0x%p  len %d  ep %d\n",
 				   packet, packet->buf_len,
 				   packet->endpoint);
-			dev_kfree_skb(packet->pkt_cntxt);
+			/*
+			 * packets in rx_bufq of endpoint 0 have originally
+			 * been queued from target->free_ctrl_rxbuf where
+			 * packet and packet->buf_start are allocated
+			 * separately using kmalloc(). For other endpoint
+			 * rx_bufq, it is allocated as skb where packet is
+			 * skb->head. Take care of this difference while freeing
+			 * the memory.
+			 */
+			if (packet->endpoint == ENDPOINT_0) {
+				kfree(packet->buf_start);
+				kfree(packet);
+			} else {
+				dev_kfree_skb(packet->pkt_cntxt);
+			}
 			spin_lock_bh(&target->rx_lock);
 		}
 		spin_unlock_bh(&target->rx_lock);
-- 
1.7.0.4

Re: [PATCH 2/2] ath6kl: Fix memory leak of rx packets in endpoint 0

[Raja Mani]

On Friday 10 February 2012 08:40 PM, Vasanthakumar Thiagarajan wrote:
> htc_packet and htc_packet->buf_start are separately allocated
> for endpoint 0. This is different for other endpoints where
> packets are allocated as skb where htc_packet is skb->head
> and they are freed properly. Free htc_packet and htc_packet->buf_start
> separatly for endpoint 0.
>
> Signed-off-by: Vasanthakumar Thiagarajan<vthiagar@qca.qualcomm.com>
> ---
>   drivers/net/wireless/ath/ath6kl/htc.c |   16 +++++++++++++++-
>   1 files changed, 15 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/net/wireless/ath/ath6kl/htc.c b/drivers/net/wireless/ath/ath6kl/htc.c
> index c703ef9..e50cc8e 100644
> --- a/drivers/net/wireless/ath/ath6kl/htc.c
> +++ b/drivers/net/wireless/ath/ath6kl/htc.c
> @@ -2372,7 +2372,21 @@ void ath6kl_htc_flush_rx_buf(struct htc_target *target)
>   				   "htc rx flush pkt 0x%p  len %d  ep %d\n",
>   				   packet, packet->buf_len,
>   				   packet->endpoint);
> -			dev_kfree_skb(packet->pkt_cntxt);
> +			/*
> +			 * packets in rx_bufq of endpoint 0 have originally
> +			 * been queued from target->free_ctrl_rxbuf where
> +			 * packet and packet->buf_start are allocated
> +			 * separately using kmalloc(). For other endpoint
> +			 * rx_bufq, it is allocated as skb where packet is
> +			 * skb->head. Take care of this difference while freeing
> +			 * the memory.
> +			 */
> +			if (packet->endpoint == ENDPOINT_0) {
> +				kfree(packet->buf_start);
> +				kfree(packet);
> +			} else {
> +				dev_kfree_skb(packet->pkt_cntxt);
> +			}

Vasanth,

My system is freezing with this patch when i tried to unload
ath6kl_sdio.ko. Could you double check this change once again?

>   			spin_lock_bh(&target->rx_lock);
>   		}
>   		spin_unlock_bh(&target->rx_lock);

Re: [PATCH 2/2] ath6kl: Fix memory leak of rx packets in endpoint 0

[Vasanthakumar Thiagarajan]

On Tuesday 14 February 2012 12:53 PM, Raja Mani wrote:
>
> Vasanth,
>
> My system is freezing with this patch when i tried to unload
> ath6kl_sdio.ko. Could you double check this change once again?

Thanks for reporting this. Kalle, please drop this, i'll send the
revised patch.

-- 
Vasanth

Re: [PATCH 2/2] ath6kl: Fix memory leak of rx packets in endpoint 0

[Vasanthakumar Thiagarajan]

On Tuesday 14 February 2012 12:57 PM, Vasanthakumar Thiagarajan wrote:
> On Tuesday 14 February 2012 12:53 PM, Raja Mani wrote:
>>
>> Vasanth,
>>
>> My system is freezing with this patch when i tried to unload
>> ath6kl_sdio.ko. Could you double check this change once again?
>
> Thanks for reporting this. Kalle, please drop this, i'll send the
> revised patch.
>
Actually the first patch also needs to be applied to avoid invalid memory
free.

Kalle, when used with PATCH[1/2] this patch does not cause any
system freeze. This patch can be taken if there is no any review commands.

-- 
Vasanth

Re: [PATCH 2/2] ath6kl: Fix memory leak of rx packets in endpoint 0

[Kalle Valo]

On 02/10/2012 05:10 PM, Vasanthakumar Thiagarajan wrote:
> htc_packet and htc_packet->buf_start are separately allocated
> for endpoint 0. This is different for other endpoints where
> packets are allocated as skb where htc_packet is skb->head
> and they are freed properly. Free htc_packet and htc_packet->buf_start
> separatly for endpoint 0.
> 
> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
> ---
>  drivers/net/wireless/ath/ath6kl/htc.c |   16 +++++++++++++++-
>  1 files changed, 15 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/net/wireless/ath/ath6kl/htc.c b/drivers/net/wireless/ath/ath6kl/htc.c
> index c703ef9..e50cc8e 100644
> --- a/drivers/net/wireless/ath/ath6kl/htc.c
> +++ b/drivers/net/wireless/ath/ath6kl/htc.c
> @@ -2372,7 +2372,21 @@ void ath6kl_htc_flush_rx_buf(struct htc_target *target)
>  				   "htc rx flush pkt 0x%p  len %d  ep %d\n",
>  				   packet, packet->buf_len,
>  				   packet->endpoint);
> -			dev_kfree_skb(packet->pkt_cntxt);
> +			/*
> +			 * packets in rx_bufq of endpoint 0 have originally
> +			 * been queued from target->free_ctrl_rxbuf where
> +			 * packet and packet->buf_start are allocated
> +			 * separately using kmalloc(). For other endpoint
> +			 * rx_bufq, it is allocated as skb where packet is
> +			 * skb->head. Take care of this difference while freeing
> +			 * the memory.
> +			 */
> +			if (packet->endpoint == ENDPOINT_0) {
> +				kfree(packet->buf_start);
> +				kfree(packet);
> +			} else {
> +				dev_kfree_skb(packet->pkt_cntxt);
> +			}

I didn't look at the code, but my question would it be possible to use
skbs also with endpoint 0? That would be more consistent aproach than
testing for a particular endpoint.

And in the future we should get rid of that buf_start hack and use skbs
directly anyway.

Kalle

Re: [PATCH 2/2] ath6kl: Fix memory leak of rx packets in endpoint 0

[Vasanthakumar Thiagarajan]

On Mon, Feb 27, 2012 at 07:22:45PM +0200, Kalle Valo wrote:
> On 02/10/2012 05:10 PM, Vasanthakumar Thiagarajan wrote:
> > htc_packet and htc_packet->buf_start are separately allocated
> > for endpoint 0. This is different for other endpoints where
> > packets are allocated as skb where htc_packet is skb->head
> > and they are freed properly. Free htc_packet and htc_packet->buf_start
> > separatly for endpoint 0.
> > 
> > Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>
> > ---
> >  drivers/net/wireless/ath/ath6kl/htc.c |   16 +++++++++++++++-
> >  1 files changed, 15 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/net/wireless/ath/ath6kl/htc.c b/drivers/net/wireless/ath/ath6kl/htc.c
> > index c703ef9..e50cc8e 100644
> > --- a/drivers/net/wireless/ath/ath6kl/htc.c
> > +++ b/drivers/net/wireless/ath/ath6kl/htc.c
> > @@ -2372,7 +2372,21 @@ void ath6kl_htc_flush_rx_buf(struct htc_target *target)
> >  				   "htc rx flush pkt 0x%p  len %d  ep %d\n",
> >  				   packet, packet->buf_len,
> >  				   packet->endpoint);
> > -			dev_kfree_skb(packet->pkt_cntxt);
> > +			/*
> > +			 * packets in rx_bufq of endpoint 0 have originally
> > +			 * been queued from target->free_ctrl_rxbuf where
> > +			 * packet and packet->buf_start are allocated
> > +			 * separately using kmalloc(). For other endpoint
> > +			 * rx_bufq, it is allocated as skb where packet is
> > +			 * skb->head. Take care of this difference while freeing
> > +			 * the memory.
> > +			 */
> > +			if (packet->endpoint == ENDPOINT_0) {
> > +				kfree(packet->buf_start);
> > +				kfree(packet);
> > +			} else {
> > +				dev_kfree_skb(packet->pkt_cntxt);
> > +			}
> 
> I didn't look at the code, but my question would it be possible to use
> skbs also with endpoint 0? That would be more consistent aproach than
> testing for a particular endpoint.

Yeah, using skbs for control buffer is the right thing, but for the time being
we can have this fix in.

> 
> And in the future we should get rid of that buf_start hack and use skbs
> directly anyway.

Agree.

Vasanth

Re: [PATCH 2/2] ath6kl: Fix memory leak of rx packets in endpoint 0

[Kalle Valo]

On 02/28/2012 06:26 AM, Vasanthakumar Thiagarajan wrote:
> On Mon, Feb 27, 2012 at 07:22:45PM +0200, Kalle Valo wrote:
>> On 02/10/2012 05:10 PM, Vasanthakumar Thiagarajan wrote:
>>> htc_packet and htc_packet->buf_start are separately allocated
>>> for endpoint 0. This is different for other endpoints where
>>> packets are allocated as skb where htc_packet is skb->head
>>> and they are freed properly. Free htc_packet and htc_packet->buf_start
>>> separatly for endpoint 0.
>>>
>>> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>

[...]

>>> +			/*
>>> +			 * packets in rx_bufq of endpoint 0 have originally
>>> +			 * been queued from target->free_ctrl_rxbuf where
>>> +			 * packet and packet->buf_start are allocated
>>> +			 * separately using kmalloc(). For other endpoint
>>> +			 * rx_bufq, it is allocated as skb where packet is
>>> +			 * skb->head. Take care of this difference while freeing
>>> +			 * the memory.
>>> +			 */
>>> +			if (packet->endpoint == ENDPOINT_0) {
>>> +				kfree(packet->buf_start);
>>> +				kfree(packet);
>>> +			} else {
>>> +				dev_kfree_skb(packet->pkt_cntxt);
>>> +			}
>>
>> I didn't look at the code, but my question would it be possible to use
>> skbs also with endpoint 0? That would be more consistent aproach than
>> testing for a particular endpoint.
> 
> Yeah, using skbs for control buffer is the right thing, but for the time being
> we can have this fix in.

Ok, fair enough. Let's do it like this.

Kalle

Re: [PATCH 1/2] ath6kl: Make sure to allocate rx buffers after the endpoint connection

[Kalle Valo]

On 02/10/2012 05:10 PM, Vasanthakumar Thiagarajan wrote:
> Rx buffers should be allocated for control and best effort endpoints only
> after the enpoints connection is esablished. But this is done before the
> endpoint connection is complete, we don't even the control and BE endpoints
> that time. Move the buffer allocation after endpoint connection is over,
> after ath6kl_init_hw_start(). Found in review, never seen any real issue
> with this.
> 
> Signed-off-by: Vasanthakumar Thiagarajan <vthiagar@qca.qualcomm.com>

Thanks, both patches applied.

Kalle