[PATCH v2 0/2] ath9k: few memory fixes

[PATCH v2 0/2] ath9k: few memory fixes

[Luis R. Rodriguez]

This series addresses some memory issues present on ath9k
regarding allocation and not handling failed DMA maps.

  Luis

[PATCH v2 1/2] ath9k: Check for pci_map_single() errors

[Luis R. Rodriguez]

pci_map_single() can fail so detect those errors with
pci_dma_mapping_error() and deal with them accordingly.

Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath9k/beacon.c |   16 +++++++++++++++-
 drivers/net/wireless/ath9k/recv.c   |   17 +++++++++++++++++
 drivers/net/wireless/ath9k/xmit.c   |   23 +++++++++++++++++++++--
 3 files changed, 53 insertions(+), 3 deletions(-)

diff --git a/drivers/net/wireless/ath9k/beacon.c b/drivers/net/wireless/ath9k/beacon.c
index e80d9b9..bc75f64 100644
--- a/drivers/net/wireless/ath9k/beacon.c
+++ b/drivers/net/wireless/ath9k/beacon.c
@@ -192,6 +192,13 @@ static struct ath_buf *ath_beacon_generate(struct ath_softc *sc, int if_id)
 		pci_map_single(sc->pdev, skb->data,
 			       skb->len,
 			       PCI_DMA_TODEVICE);
+	if (unlikely(pci_dma_mapping_error(sc->pdev, bf->bf_buf_addr))) {
+		dev_kfree_skb_any(skb);
+		bf->bf_mpdu = NULL;
+		DPRINTF(sc, ATH_DBG_CONFIG,
+			"pci_dma_mapping_error() on beaconing\n");
+		return NULL;
+	}
 
 	skb = ieee80211_get_buffered_bc(sc->hw, vif);
 
@@ -396,11 +403,18 @@ int ath_beacon_alloc(struct ath_softc *sc, int if_id)
 		memcpy(&hdr[1], &val, sizeof(val));
 	}
 
+	bf->bf_mpdu = skb;
 	bf->bf_buf_addr = bf->bf_dmacontext =
 		pci_map_single(sc->pdev, skb->data,
 			       skb->len,
 			       PCI_DMA_TODEVICE);
-	bf->bf_mpdu = skb;
+	if (unlikely(pci_dma_mapping_error(sc->pdev, bf->bf_buf_addr))) {
+		dev_kfree_skb_any(skb);
+		bf->bf_mpdu = NULL;
+		DPRINTF(sc, ATH_DBG_CONFIG,
+			"pci_dma_mapping_error() on beacon alloc\n");
+		return -ENOMEM;
+	}
 
 	return 0;
 }
diff --git a/drivers/net/wireless/ath9k/recv.c b/drivers/net/wireless/ath9k/recv.c
index ba6194a..35ab3bb 100644
--- a/drivers/net/wireless/ath9k/recv.c
+++ b/drivers/net/wireless/ath9k/recv.c
@@ -309,6 +309,15 @@ int ath_rx_init(struct ath_softc *sc, int nbufs)
 			bf->bf_buf_addr = pci_map_single(sc->pdev, skb->data,
 					 sc->sc_rxbufsize,
 					 PCI_DMA_FROMDEVICE);
+			if (unlikely(pci_dma_mapping_error(sc->pdev,
+				  bf->bf_buf_addr))) {
+				dev_kfree_skb_any(skb);
+				bf->bf_mpdu = NULL;
+				DPRINTF(sc, ATH_DBG_CONFIG,
+					"pci_dma_mapping_error() on RX init\n");
+				error = -ENOMEM;
+				break;
+			}
 			bf->bf_dmacontext = bf->bf_buf_addr;
 		}
 		sc->sc_rxlink = NULL;
@@ -594,6 +603,14 @@ int ath_rx_tasklet(struct ath_softc *sc, int flush)
 		bf->bf_buf_addr = pci_map_single(sc->pdev, requeue_skb->data,
 					 sc->sc_rxbufsize,
 					 PCI_DMA_FROMDEVICE);
+		if (unlikely(pci_dma_mapping_error(sc->pdev,
+			  bf->bf_buf_addr))) {
+			dev_kfree_skb_any(requeue_skb);
+			bf->bf_mpdu = NULL;
+			DPRINTF(sc, ATH_DBG_CONFIG,
+				"pci_dma_mapping_error() on RX\n");
+			break;
+		}
 		bf->bf_dmacontext = bf->bf_buf_addr;
 
 		/*
diff --git a/drivers/net/wireless/ath9k/xmit.c b/drivers/net/wireless/ath9k/xmit.c
index 413fbdd..ef209b5 100644
--- a/drivers/net/wireless/ath9k/xmit.c
+++ b/drivers/net/wireless/ath9k/xmit.c
@@ -1653,7 +1653,7 @@ 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,
+static int ath_tx_setup_buffer(struct ath_softc *sc, struct ath_buf *bf,
 				struct sk_buff *skb,
 				struct ath_tx_control *txctl)
 {
@@ -1712,9 +1712,18 @@ static void ath_tx_setup_buffer(struct ath_softc *sc, struct ath_buf *bf,
 	/* DMA setup */
 
 	bf->bf_mpdu = skb;
+
 	bf->bf_dmacontext = pci_map_single(sc->pdev, skb->data,
 					   skb->len, PCI_DMA_TODEVICE);
+	if (unlikely(pci_dma_mapping_error(sc->pdev, bf->bf_dmacontext))) {
+		bf->bf_mpdu = NULL;
+		DPRINTF(sc, ATH_DBG_CONFIG,
+			"pci_dma_mapping_error() on TX\n");
+		return -ENOMEM;
+	}
+
 	bf->bf_buf_addr = bf->bf_dmacontext;
+	return 0;
 }
 
 /* FIXME: tx power */
@@ -1786,10 +1795,12 @@ static void ath_tx_start_dma(struct ath_softc *sc, struct ath_buf *bf,
 	spin_unlock_bh(&txctl->txq->axq_lock);
 }
 
+/* Upon failure caller should free skb */
 int ath_tx_start(struct ath_softc *sc, struct sk_buff *skb,
 		 struct ath_tx_control *txctl)
 {
 	struct ath_buf *bf;
+	int r;
 
 	/* Check if a tx buffer is available */
 
@@ -1800,7 +1811,15 @@ int ath_tx_start(struct ath_softc *sc, struct sk_buff *skb,
 		return -1;
 	}
 
-	ath_tx_setup_buffer(sc, bf, skb, txctl);
+	r = ath_tx_setup_buffer(sc, bf, skb, txctl);
+	if (r) {
+		spin_lock_bh(&sc->sc_txbuflock);
+		DPRINTF(sc, ATH_DBG_FATAL, "TX mem alloc failure\n");
+		list_add_tail(&bf->list, &sc->sc_txbuf);
+		spin_unlock_bh(&sc->sc_txbuflock);
+		return r;
+	}
+
 	ath_tx_start_dma(sc, bf, txctl);
 
 	return 0;
-- 
1.5.6.rc2.15.g457bb.dirty

[PATCH v2 2/2] ath9k: Use GFP_ATOMIC when allocating TX private area

[Luis R. Rodriguez]

Using GFP_KERNEL was wrong and produces a 'scheduling while atomic'
bug as we're in a tasklet. Also, check for proper return values
now, in case allocation fails and be sure to stop the TX queue
in case of memory issues but gaurantee the TX queue will
eventually be woken up.

Signed-off-by: Senthil Balasubramanian <senthilkumar@atheros.com>
Signed-off-by: Sujith <Sujith.Manoharan@atheros.com>
Signed-off-by: Luis R. Rodriguez <lrodriguez@atheros.com>
---
 drivers/net/wireless/ath9k/xmit.c |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

diff --git a/drivers/net/wireless/ath9k/xmit.c b/drivers/net/wireless/ath9k/xmit.c
index ef209b5..435af61 100644
--- a/drivers/net/wireless/ath9k/xmit.c
+++ b/drivers/net/wireless/ath9k/xmit.c
@@ -1663,7 +1663,9 @@ static int 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;
@@ -1813,10 +1815,30 @@ int ath_tx_start(struct ath_softc *sc, struct sk_buff *skb,
 
 	r = ath_tx_setup_buffer(sc, bf, skb, txctl);
 	if (r) {
-		spin_lock_bh(&sc->sc_txbuflock);
+		struct ath_txq *txq = NULL;
+		int qnum;
+
+		qnum = ath_get_hal_qnum(skb_get_queue_mapping(skb), sc);
+		txq = &sc->sc_txq[qnum];
+
 		DPRINTF(sc, ATH_DBG_FATAL, "TX mem alloc failure\n");
+
+		/* upon ath_tx_processq() this TX queue will be resumed, we
+		 * guarantee this will happen by knowing beforehand that
+		 * we will at least have to run TX completionon one buffer
+		 * on the queue */
+		spin_lock_bh(&txq->axq_lock);
+		if (ath_txq_depth(sc, txq->axq_qnum) > 1) {
+			ieee80211_stop_queue(sc->hw,
+				skb_get_queue_mapping(skb));
+			txq->stopped = 1;
+		}
+		spin_unlock_bh(&txq->axq_lock);
+
+		spin_lock_bh(&sc->sc_txbuflock);
 		list_add_tail(&bf->list, &sc->sc_txbuf);
 		spin_unlock_bh(&sc->sc_txbuflock);
+
 		return r;
 	}
 
-- 
1.5.6.rc2.15.g457bb.dirty

Re: [ath9k-devel] [PATCH v2 2/2] ath9k: Use GFP_ATOMIC when allocating TX private area

[Luis R. Rodriguez]

On Tue, Dec 2, 2008 at 7:46 PM, Sujith <m.sujith@gmail.com> wrote:
> Luis R. Rodriguez wrote:
>> @@ -1813,10 +1815,30 @@ int ath_tx_start(struct ath_softc *sc, struct sk_buff *skb,
>>
>>       r = ath_tx_setup_buffer(sc, bf, skb, txctl);
>>       if (r) {
>> -             spin_lock_bh(&sc->sc_txbuflock);
>> +             struct ath_txq *txq = NULL;
>> +             int qnum;
>> +
>> +             qnum = ath_get_hal_qnum(skb_get_queue_mapping(skb), sc);
>> +             txq = &sc->sc_txq[qnum];
>> +
>>               DPRINTF(sc, ATH_DBG_FATAL, "TX mem alloc failure\n");
>> +
>> +             /* upon ath_tx_processq() this TX queue will be resumed, we
>> +              * guarantee this will happen by knowing beforehand that
>> +              * we will at least have to run TX completionon one buffer
>> +              * on the queue */
>> +             spin_lock_bh(&txq->axq_lock);
>> +             if (ath_txq_depth(sc, txq->axq_qnum) > 1) {
>> +                     ieee80211_stop_queue(sc->hw,
>> +                             skb_get_queue_mapping(skb));
>> +                     txq->stopped = 1;
>> +             }
>> +             spin_unlock_bh(&txq->axq_lock);
>> +
>> +             spin_lock_bh(&sc->sc_txbuflock);
>>               list_add_tail(&bf->list, &sc->sc_txbuf);
>>               spin_unlock_bh(&sc->sc_txbuflock);
>> +
>
> The queue is already available, we make sure of that before calling ath_tx_start().
> And the queue is sent through txctl.txq, you don't have to calculate the queue number again.
>
> So, just stopping the queue and moving the buffer back to the free list would be enough.

ath_test_get_txq() will not return the queue only if the queue is
getting full. What the check above guarantees is we have at least one
buffer to run through procesq before disabling the TX queue, otherwise
the queue won't be be woken up. Or am I missing something? That is,
say you just have to TX one skb and we hit -ENOMEM on
ath_tx_setup_buffer() and you disable the queue. How will this queue
be awoken if this is true in ath_tx_processq():

list_empty(&txq->axq_q)

  Luis

Re: [ath9k-devel] [PATCH v2 2/2] ath9k: Use GFP_ATOMIC when allocating TX private area

[Sujith]

Luis Rodriguez wrote:
> ath_test_get_txq() will not return the queue only if the queue is
> getting full. What the check above guarantees is we have at least one
> buffer to run through procesq before disabling the TX queue, otherwise
> the queue won't be be woken up. Or am I missing something? That is,
> say you just have to TX one skb and we hit -ENOMEM on
> ath_tx_setup_buffer() and you disable the queue. How will this queue
> be awoken if this is true in ath_tx_processq():
> 
> list_empty(&txq->axq_q)

Ok, makes sense.
But the queue can be obtained from txctl, so that code can be removed.

Sujith