Skip to contentSkip to conversation

rt2x00: Remove duplicate deinitialization

PATCH In review Ivo van Doorn 2 participants 3 messages Jun 20, 2008 – Jun 21, 2008 Networking drivers (wireless)

lore
Share & tools
lore
#0 lore raw

PATCH 1/1 rt2x00: Remove duplicate deinitialization

When rt2x00queue_alloc_rxskbs() fails rt2x00queue_unitialize() will be called which will free all rxskb. So we don't need to do this in the rt2x00queue_alloc_rxskb() function as well.

rt2x00queue_free_skb() unmaps the DMA but doesn't clear the allocation flag. Since the code is copied from rt2x00queue_unmap_skb() anyway (and that function does clear the flag) we might as well use that function directly.

---

This patch depends on patches from the pull request of June 16

 drivers/net/wireless/rt2x00/rt2x00queue.c |   20 ++------------------
 1 files changed, 2 insertions(+), 18 deletions(-)
drivers/net/wireless/rt2x00/rt2x00queue.c +2 -18
index 49d3bb8..8e86611 100644
@@ -107,18 +107,7 @@ void rt2x00queue_unmap_skb(struct rt2x00_dev *rt2x00dev, struct sk_buff *skb)
 
 void rt2x00queue_free_skb(struct rt2x00_dev *rt2x00dev, struct sk_buff *skb)
 {
-	struct skb_frame_desc *skbdesc = get_skb_frame_desc(skb);
-
-	if (skbdesc->flags & SKBDESC_DMA_MAPPED_RX) {
-		dma_unmap_single(rt2x00dev->dev, skbdesc->skb_dma, skb->len,
-				 DMA_FROM_DEVICE);
-	}
-
-	if (skbdesc->flags & SKBDESC_DMA_MAPPED_TX) {
-		dma_unmap_single(rt2x00dev->dev, skbdesc->skb_dma, skb->len,
-				 DMA_TO_DEVICE);
-	}
-
+	rt2x00queue_unmap_skb(rt2x00dev, skb);
 	dev_kfree_skb_any(skb);
 }
 
@@ -509,16 +498,11 @@ static int rt2x00queue_alloc_rxskbs(struct rt2x00_dev *rt2x00dev,
 	for (i = 0; i < queue->limit; i++) {
 		skb = rt2x00queue_alloc_rxskb(rt2x00dev, &queue->entries[i]);
 		if (!skb)
-			goto exit;
+			return -ENOMEM;
 		queue->entries[i].skb = skb;
 	}
 
 	return 0;
-
-exit:
-	rt2x00queue_free_skbs(rt2x00dev, queue);
-
-	return -ENOMEM;
 }
 
 int rt2x00queue_initialize(struct rt2x00_dev *rt2x00dev)
Signature · 1 line
1.5.5.4
#1 lore raw

Hi Ivo,

Quoted 50 lines · Ivo van Doorn

When rt2x00queue_alloc_rxskbs() fails rt2x00queue_unitialize() will be called which will free all rxskb. So we don't need to do this in the rt2x00queue_alloc_rxskb() function as well.

rt2x00queue_free_skb() unmaps the DMA but doesn't clear the allocation flag. Since the code is copied from rt2x00queue_unmap_skb() anyway (and that function does clear the flag) we might as well use that function directly.

---

This patch depends on patches from the pull request of June 16

 drivers/net/wireless/rt2x00/rt2x00queue.c |   20 ++------------------
 1 files changed, 2 insertions(+), 18 deletions(-)
drivers/net/wireless/rt2x00/rt2x00queue.c +2 -13
index 49d3bb8..8e86611 100644
@@ -107,18 +107,7 @@ void rt2x00queue_unmap_skb(struct rt2x00_dev *rt2x00dev, struct sk_buff *skb)
 
 void rt2x00queue_free_skb(struct rt2x00_dev *rt2x00dev, struct sk_buff *skb)
 {
-	struct skb_frame_desc *skbdesc = get_skb_frame_desc(skb);
-
-	if (skbdesc->flags & SKBDESC_DMA_MAPPED_RX) {
-		dma_unmap_single(rt2x00dev->dev, skbdesc->skb_dma, skb->len,
-				 DMA_FROM_DEVICE);
-	}
-
-	if (skbdesc->flags & SKBDESC_DMA_MAPPED_TX) {
-		dma_unmap_single(rt2x00dev->dev, skbdesc->skb_dma, skb->len,
-				 DMA_TO_DEVICE);
-	}
-
+	rt2x00queue_unmap_skb(rt2x00dev, skb);
 	dev_kfree_skb_any(skb);
 }
 
@@ -509,16 +498,11 @@ static int rt2x00queue_alloc_rxskbs(struct rt2x00_dev *rt2x00dev,
 	for (i = 0; i < queue->limit; i++) {
 		skb = rt2x00queue_alloc_rxskb(rt2x00dev, &queue->entries[i]);
 		if (!skb)
-			goto exit;
+			return -ENOMEM;
 		queue->entries[i].skb = skb;
 	}

drivers/net/wireless/rt2x00/rt2x00queue.c static int rt2x00queue_alloc_rxskbs(struct rt2x00_dev *rt2x00dev, · line 508

Quoted 10 lines
drivers/net/wireless/rt2x00/rt2x00queue.c +0 -5
 
 	return 0;
-
-exit:
-	rt2x00queue_free_skbs(rt2x00dev, queue);
-
-	return -ENOMEM;
 }
 
 int rt2x00queue_initialize(struct rt2x00_dev *rt2x00dev)

I find this last hunk of the patch a bit dubious. IMHO a function should never rely on letting his caller do the right thing and clean up for it, in case of failures; it should always clean up after itself. It is true that in this case the caller will also attempt to clean up these structures, but that doesn't mean that any potential future users of such a function should be bothered with doing the same thing.

---
Gertjan

#2 lore raw

Hi

Quoted 17 lines
Quoted 17 lines
drivers/net/wireless/rt2x00/rt2x00queue.c +1 -6
@@ -509,16 +498,11 @@ static int rt2x00queue_alloc_rxskbs(struct rt2x00_dev *rt2x00dev,
 	for (i = 0; i < queue->limit; i++) {
 		skb = rt2x00queue_alloc_rxskb(rt2x00dev, &queue->entries[i]);
 		if (!skb)
-			goto exit;
+			return -ENOMEM;
 		queue->entries[i].skb = skb;
 	}
 
 	return 0;
-
-exit:
-	rt2x00queue_free_skbs(rt2x00dev, queue);
-
-	return -ENOMEM;
 }
 
Quoted 10 lines

drivers/net/wireless/rt2x00/rt2x00queue.c static int rt2x00queue_alloc_rxskbs(struct rt2x00_dev *rt2x00dev, · line 508

Quoted 2 lines
drivers/net/wireless/rt2x00/rt2x00queue.c +0 -0
 int rt2x00queue_initialize(struct rt2x00_dev *rt2x00dev)

I find this last hunk of the patch a bit dubious. IMHO a function should never rely on letting his caller do the right thing and clean up for it, in case of failures; it should always clean up after itself. It is true that in this case the caller will also attempt to clean up these structures, but that doesn't mean that any potential future users of such a function should be bothered with doing the same thing.

True but rt2x00queue_alloc_rxskbs and rt2x00queue_free_rxskbs are only called once. And there isn't any reason why they should be called anywhere else other then initialize() and uninitialize().

And this approach is the same as other allocation/free paths like in rt2x00pci/usb and rt2x00lib.

Ivo