ath9k: few memory fixes

PATCH Superseded Luis R. Rodriguez 3 participants 6 messages Dec 3, 2008 Networking drivers (wireless)

lore

Share & tools

lore

Luis R. Rodriguez

author

2008-12-03 03:15 UTC #0 lore raw

v2 cover ath9k: few memory fixes

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

Luis

Luis R. Rodriguez

author

2008-12-03 03:15 UTC #1 lore raw

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

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 <[email protected]>

---
 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(-)

drivers/net/wireless/ath9k/beacon.c +15 -1

index e80d9b9..bc75f64 100644
@@ -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;
 }

drivers/net/wireless/ath9k/recv.c +17 -0

index ba6194a..35ab3bb 100644
@@ -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;
 
 		/*

drivers/net/wireless/ath9k/xmit.c +21 -2

index 413fbdd..ef209b5 100644
@@ -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;

Signature · 1 line

1.5.6.rc2.15.g457bb.dirty

Luis R. Rodriguez

author

2008-12-03 03:15 UTC #2 lore raw

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

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 <[email protected]>

Signed-off-by Sujith <[email protected]>

Signed-off-by Luis R. Rodriguez <[email protected]>

---
 drivers/net/wireless/ath9k/xmit.c |   26 ++++++++++++++++++++++++--
 1 files changed, 24 insertions(+), 2 deletions(-)

drivers/net/wireless/ath9k/xmit.c +24 -2

index ef209b5..435af61 100644
@@ -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;
 	}

Signature · 1 line

1.5.6.rc2.15.g457bb.dirty

Sujith

2008-12-03 03:46 UTC #3 lore raw

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

Quoted 19 lines · Luis R. Rodriguez

drivers/net/wireless/ath9k/xmit.c +13 -1

@@ -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) {

drivers/net/wireless/ath9k/xmit.c int ath_tx_start(struct ath_softc *sc, struct sk_buff *skb, · line 1841

Quoted 10 lines

drivers/net/wireless/ath9k/xmit.c +8 -0

+			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.

Sujith

Luis R. Rodriguez

author

2008-12-03 04:12 UTC #4 lore raw

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

Quoted 34 lines · Sujith

Quoted 19 lines · Luis R. Rodriguez

drivers/net/wireless/ath9k/xmit.c +13 -1

@@ -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) {

drivers/net/wireless/ath9k/xmit.c int ath_tx_start(struct ath_softc *sc, struct sk_buff *skb, · line 1841

Quoted 10 lines

drivers/net/wireless/ath9k/xmit.c +8 -0

+                     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.

Quoted 1 line

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

Sujith

2008-12-03 04:50 UTC #5 lore raw

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

Quoted 9 lines · Luis Rodriguez

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