[PATCH] ath9k: Use GFP_ATOMIC when allocating private area

[PATCH] ath9k: Use GFP_ATOMIC when allocating private area

[Sujith]

Using GFP_KERNEL was wrong and produces a 'scheduling while atomic'
bug. Also, check for proper return values now, in case allocation
fails.

Signed-off-by: Sujith <Sujith.Manoharan@atheros.com>
Signed-off-by: Senthil Balasubramanian <senthilkumar@atheros.com>
---
 drivers/net/wireless/ath9k/xmit.c |   21 ++++++++++++++++-----
 1 files changed, 16 insertions(+), 5 deletions(-)

diff --git a/drivers/net/wireless/ath9k/xmit.c b/drivers/net/wireless/ath9k/xmit.c
index fc52f61..021cc56 100644
--- a/drivers/net/wireless/ath9k/xmit.c
+++ b/drivers/net/wireless/ath9k/xmit.c
@@ -1641,9 +1641,9 @@ static void ath_txq_drain_pending_buffers(struct ath_softc *sc,
 	}
 }
 
-static void ath_tx_setup_buffer(struct ath_softc *sc, struct ath_buf *bf,
-				struct sk_buff *skb,
-				struct ath_tx_control *txctl)
+static int ath_tx_setup_buffer(struct ath_softc *sc, struct ath_buf *bf,
+			       struct sk_buff *skb,
+			       struct ath_tx_control *txctl)
 {
 	struct ieee80211_tx_info *tx_info = IEEE80211_SKB_CB(skb);
 	struct ieee80211_hdr *hdr = (struct ieee80211_hdr *)skb->data;
@@ -1651,7 +1651,10 @@ static void ath_tx_setup_buffer(struct ath_softc *sc, struct ath_buf *bf,
 	int hdrlen;
 	__le16 fc;
 
-	tx_info_priv = kzalloc(sizeof(*tx_info_priv), GFP_KERNEL);
+	tx_info_priv = kzalloc(sizeof(*tx_info_priv), GFP_ATOMIC);
+	if (!tx_info_priv)
+		return -ENOMEM;
+
 	tx_info->rate_driver_data[0] = tx_info_priv;
 	hdrlen = ieee80211_get_hdrlen_from_skb(skb);
 	fc = hdr->frame_control;
@@ -1703,6 +1706,8 @@ static void ath_tx_setup_buffer(struct ath_softc *sc, struct ath_buf *bf,
 	bf->bf_dmacontext = pci_map_single(sc->pdev, skb->data,
 					   skb->len, PCI_DMA_TODEVICE);
 	bf->bf_buf_addr = bf->bf_dmacontext;
+
+	return 0;
 }
 
 /* FIXME: tx power */
@@ -1777,6 +1782,7 @@ static void ath_tx_start_dma(struct ath_softc *sc, struct ath_buf *bf,
 int ath_tx_start(struct ath_softc *sc, struct sk_buff *skb,
 		 struct ath_tx_control *txctl)
 {
+	int ret = 0;
 	struct ath_buf *bf;
 
 	/* Check if a tx buffer is available */
@@ -1787,7 +1793,12 @@ int ath_tx_start(struct ath_softc *sc, struct sk_buff *skb,
 		return -1;
 	}
 
-	ath_tx_setup_buffer(sc, bf, skb, txctl);
+	ret = ath_tx_setup_buffer(sc, bf, skb, txctl);
+	if (ret) {
+		DPRINTF(sc, ATH_DBG_FATAL, "TX mem alloc failure\n");
+		return ret;
+	}
+
 	ath_tx_start_dma(sc, bf, txctl);
 
 	return 0;
-- 
1.6.0.3

Re: [PATCH] ath9k: Use GFP_ATOMIC when allocating private area

[Luis R. Rodriguez]

On Tue, Dec 2, 2008 at 5:06 AM, Sujith <Sujith.Manoharan@atheros.com> wrote:

> --- a/drivers/net/wireless/ath9k/xmit.c
> +++ b/drivers/net/wireless/ath9k/xmit.c
> @@ -1787,7 +1793,12 @@ int ath_tx_start(struct ath_softc *sc, struct sk_buff *skb,
>                return -1;
>        }
>
> -       ath_tx_setup_buffer(sc, bf, skb, txctl);
> +       ret = ath_tx_setup_buffer(sc, bf, skb, txctl);
> +       if (ret) {
> +               DPRINTF(sc, ATH_DBG_FATAL, "TX mem alloc failure\n");
> +               return ret;

Hm, this doesn't add the bf back to the txq, so we'd run out of bf's
completely when on low memory, eventually leaving the list always
empty. I'll resend with that added and a few more changes.

  Luis

Re: [PATCH] ath9k: Use GFP_ATOMIC when allocating private area

[Sujith]

Luis R. Rodriguez wrote:
> On Tue, Dec 2, 2008 at 5:06 AM, Sujith <Sujith.Manoharan@atheros.com> wrote:
> 
> > --- a/drivers/net/wireless/ath9k/xmit.c
> > +++ b/drivers/net/wireless/ath9k/xmit.c
> > @@ -1787,7 +1793,12 @@ int ath_tx_start(struct ath_softc *sc, struct sk_buff *skb,
> >                return -1;
> >        }
> >
> > -       ath_tx_setup_buffer(sc, bf, skb, txctl);
> > +       ret = ath_tx_setup_buffer(sc, bf, skb, txctl);
> > +       if (ret) {
> > +               DPRINTF(sc, ATH_DBG_FATAL, "TX mem alloc failure\n");
> > +               return ret;
> 
> Hm, this doesn't add the bf back to the txq, so we'd run out of bf's
> completely when on low memory, eventually leaving the list always
> empty. I'll resend with that added and a few more changes.
> 

Right.

I think the queue has to be stopped too (ieee80211_stop_queue()),
and resumed when memory is available again.

What do you think ?

Sujith

Re: [PATCH] ath9k: Use GFP_ATOMIC when allocating private area

[Luis R. Rodriguez]

On Tue, Dec 2, 2008 at 5:58 PM, Sujith <m.sujith@gmail.com> wrote:
> Luis R. Rodriguez wrote:
>> On Tue, Dec 2, 2008 at 5:06 AM, Sujith <Sujith.Manoharan@atheros.com> wrote:
>>
>> > --- a/drivers/net/wireless/ath9k/xmit.c
>> > +++ b/drivers/net/wireless/ath9k/xmit.c
>> > @@ -1787,7 +1793,12 @@ int ath_tx_start(struct ath_softc *sc, struct sk_buff *skb,
>> >                return -1;
>> >        }
>> >
>> > -       ath_tx_setup_buffer(sc, bf, skb, txctl);
>> > +       ret = ath_tx_setup_buffer(sc, bf, skb, txctl);
>> > +       if (ret) {
>> > +               DPRINTF(sc, ATH_DBG_FATAL, "TX mem alloc failure\n");
>> > +               return ret;
>>
>> Hm, this doesn't add the bf back to the txq, so we'd run out of bf's
>> completely when on low memory, eventually leaving the list always
>> empty. I'll resend with that added and a few more changes.
>>
>
> Right.
>
> I think the queue has to be stopped too (ieee80211_stop_queue()),
> and resumed when memory is available again.
>
> What do you think ?

Interesting, I hadn't thought about stopping the queues, when would
they be resumed then though, or rather who? I'll a rework of this
ontop of more DMA fun, let me know what you think. I just tested this.

  Luis

Re: [PATCH] ath9k: Use GFP_ATOMIC when allocating private area

[Sujith]

Luis R. Rodriguez wrote:
> Interesting, I hadn't thought about stopping the queues, when would
> they be resumed then though, or rather who? I'll a rework of this
> ontop of more DMA fun, let me know what you think. I just tested this.
> 

In TX completion, when we see that a queue is stopped, we wake up that queue
if the depth is less than a threshold.

Sujith

Re: [PATCH] ath9k: Use GFP_ATOMIC when allocating private area

[Luis R. Rodriguez]

On Tue, Dec 2, 2008 at 6:12 PM, Sujith <m.sujith@gmail.com> wrote:
> Luis R. Rodriguez wrote:
>> Interesting, I hadn't thought about stopping the queues, when would
>> they be resumed then though, or rather who? I'll a rework of this
>> ontop of more DMA fun, let me know what you think. I just tested this.
>>
>
> In TX completion, when we see that a queue is stopped, we wake up that queue
> if the depth is less than a threshold.

Ah yeah I see that now, so this will wake it up but only if we're
processing the txq on completion. So technically speaking if we ran
out of memory on the first attempted skb, wouldn't we never resume it?

  Luis

Re: [PATCH] ath9k: Use GFP_ATOMIC when allocating private area

[Luis R. Rodriguez]

On Tue, Dec 2, 2008 at 6:26 PM, Luis R. Rodriguez <mcgrof@gmail.com> wrote:
> On Tue, Dec 2, 2008 at 6:12 PM, Sujith <m.sujith@gmail.com> wrote:
>> Luis R. Rodriguez wrote:
>>> Interesting, I hadn't thought about stopping the queues, when would
>>> they be resumed then though, or rather who? I'll a rework of this
>>> ontop of more DMA fun, let me know what you think. I just tested this.
>>>
>>
>> In TX completion, when we see that a queue is stopped, we wake up that queue
>> if the depth is less than a threshold.
>
> Ah yeah I see that now, so this will wake it up but only if we're
> processing the txq on completion. So technically speaking if we ran
> out of memory on the first attempted skb, wouldn't we never resume it?

OK I think I have a way to handle this too. Let me know what you think.

  Luis