diff mbox

sh_eth: merge sh_eth_free_dma_buffer() into sh_eth_ring_free()

Message ID 6672379.kyQ5eFYq8y@wasted.cogentembedded.com (mailing list archive)
State Awaiting Upstream
Headers show

Commit Message

Sergei Shtylyov Nov. 3, 2015, 9:55 p.m. UTC
While the ring allocation is done by a single function, sh_eth_ring_init(),
the ring deallocation was split into two functions (almost always called
one after the other) for no good reason. Merge  sh_eth_free_dma_buffer()
into sh_eth_ring_free() which allows us  to save space not only on the
direct calls  of the former function but also on the sh_eth_ring_init()'s
simplified error path...

Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

---
The patch is against DaveM's 'net-next.git' repo plus the patch just posted.

 drivers/net/ethernet/renesas/sh_eth.c |   60 ++++++++++++----------------------
 1 file changed, 22 insertions(+), 38 deletions(-)


--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Comments

David Miller Nov. 5, 2015, 1:59 a.m. UTC | #1
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Wed, 04 Nov 2015 00:55:13 +0300

> While the ring allocation is done by a single function, sh_eth_ring_init(),
> the ring deallocation was split into two functions (almost always called
> one after the other) for no good reason. Merge  sh_eth_free_dma_buffer()
> into sh_eth_ring_free() which allows us  to save space not only on the
> direct calls  of the former function but also on the sh_eth_ring_init()'s
> simplified error path...
> 
> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>

Applied.
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov Nov. 5, 2015, 12:53 p.m. UTC | #2
Hello.

On 11/5/2015 4:59 AM, David Miller wrote:

>> While the ring allocation is done by a single function, sh_eth_ring_init(),
>> the ring deallocation was split into two functions (almost always called
>> one after the other) for no good reason. Merge  sh_eth_free_dma_buffer()
>> into sh_eth_ring_free() which allows us  to save space not only on the
>> direct calls  of the former function but also on the sh_eth_ring_init()'s
>> simplified error path...
>>
>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>
> Applied.

    Hum, I'm seeing both patches in the net.git repo, while they were  clearly 
targeted to net-next.git... Did you really consider these 2 patches fixes?

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Nov. 5, 2015, 4:13 p.m. UTC | #3
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Thu, 5 Nov 2015 15:53:24 +0300

> Hello.
> 
> On 11/5/2015 4:59 AM, David Miller wrote:
> 
>>> While the ring allocation is done by a single function,
>>> sh_eth_ring_init(),
>>> the ring deallocation was split into two functions (almost always
>>> called
>>> one after the other) for no good reason. Merge
>>> sh_eth_free_dma_buffer()
>>> into sh_eth_ring_free() which allows us  to save space not only on the
>>> direct calls of the former function but also on the
>>> sh_eth_ring_init()'s
>>> simplified error path...
>>>
>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>
>> Applied.
> 
>    Hum, I'm seeing both patches in the net.git repo, while they were
>    clearly targeted to net-next.git... Did you really consider these 2
>    patches fixes?

It was more work for me to let the patches rot in patchwork until I openned
net-next back up than to simply just apply them to net.

You guys really make an enormous amount of work and stress for me when you
submit net-next patches when I _CLEARLY_ and _EXPLICITLY_ state that the
tree is closed right now.
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov Nov. 5, 2015, 5:19 p.m. UTC | #4
Hello.

On 11/05/2015 07:13 PM, David Miller wrote:

>>>> While the ring allocation is done by a single function,
>>>> sh_eth_ring_init(),
>>>> the ring deallocation was split into two functions (almost always
>>>> called
>>>> one after the other) for no good reason. Merge
>>>> sh_eth_free_dma_buffer()
>>>> into sh_eth_ring_free() which allows us  to save space not only on the
>>>> direct calls of the former function but also on the
>>>> sh_eth_ring_init()'s
>>>> simplified error path...
>>>>
>>>> Signed-off-by: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
>>>
>>> Applied.
>>
>>     Hum, I'm seeing both patches in the net.git repo, while they were
>>     clearly targeted to net-next.git... Did you really consider these 2
>>     patches fixes?
>
> It was more work for me to let the patches rot in patchwork until I openned
> net-next back up than to simply just apply them to net.

    OK, thank you!

> You guys really make an enormous amount of work and stress for me when you
> submit net-next patches when I _CLEARLY_ and _EXPLICITLY_ state that the
> tree is closed right now.

    Hmm, I hadn't seen your announcement, else I would have refrained from 
sending. Will look for it now...

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov Nov. 5, 2015, 5:42 p.m. UTC | #5
On 11/05/2015 08:19 PM, Sergei Shtylyov wrote:

>> You guys really make an enormous amount of work and stress for me when you
>> submit net-next patches when I _CLEARLY_ and _EXPLICITLY_ state that the
>> tree is closed right now.
>
>     Hmm, I hadn't seen your announcement, else I would have refrained from
> sending. Will look for it now...

    Nay, still not seeing anything...

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Miller Nov. 5, 2015, 6:29 p.m. UTC | #6
From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
Date: Thu, 5 Nov 2015 20:19:17 +0300

>    Hmm, I hadn't seen your announcement, else I would have refrained from
>    sending. Will look for it now...

I really don't know how to better get people's attention than this:

	http://marc.info/?l=linux-netdev&m=144652382428132&w=2

Do I have to use all CAPS?  Flashing LEDs in the Subject?

Just tell me, I'll do it...
--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Sergei Shtylyov Nov. 5, 2015, 6:38 p.m. UTC | #7
On 11/05/2015 09:29 PM, David Miller wrote:

>>     Hmm, I hadn't seen your announcement, else I would have refrained from
>>     sending. Will look for it now...
>
> I really don't know how to better get people's attention than this:
>
> 	http://marc.info/?l=linux-netdev&m=144652382428132&w=2

    Umm, I'm seeing it now, it's been even marked as read but I for the life 
of me couldn't remember reading it. Now I'll have to say I'm really sorry for 
sending out my last 2 or 3 patches, I was under full impression your tree is 
still open... :-<

> Do I have to use all CAPS?

    Well, perhaps that would have drawn more attention (it has in the past). :-)

>  Flashing LEDs in the Subject?

    :-)

MBR, Sergei

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Dave Jones Nov. 5, 2015, 6:58 p.m. UTC | #8
On Thu, Nov 05, 2015 at 01:29:15PM -0500, David Miller wrote:
 > From: Sergei Shtylyov <sergei.shtylyov@cogentembedded.com>
 > Date: Thu, 5 Nov 2015 20:19:17 +0300
 > 
 > >    Hmm, I hadn't seen your announcement, else I would have refrained from
 > >    sending. Will look for it now...
 > 
 > I really don't know how to better get people's attention than this:
 > 
 > 	http://marc.info/?l=linux-netdev&m=144652382428132&w=2
 > 
 > Do I have to use all CAPS?  Flashing LEDs in the Subject?
 > 
 > Just tell me, I'll do it...

web page on www.isnetnextopen.com, with a giant word saying YES or NO :)

	Dave

--
To unsubscribe from this list: send the line "unsubscribe linux-sh" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

Index: net-next/drivers/net/ethernet/renesas/sh_eth.c
===================================================================
--- net-next.orig/drivers/net/ethernet/renesas/sh_eth.c
+++ net-next/drivers/net/ethernet/renesas/sh_eth.c
@@ -1098,7 +1098,7 @@  static struct mdiobb_ops bb_ops = {
 static void sh_eth_ring_free(struct net_device *ndev)
 {
 	struct sh_eth_private *mdp = netdev_priv(ndev);
-	int i;
+	int ringsize, i;
 
 	/* Free Rx skb ringbuffer */
 	if (mdp->rx_skbuff) {
@@ -1115,6 +1115,20 @@  static void sh_eth_ring_free(struct net_
 	}
 	kfree(mdp->tx_skbuff);
 	mdp->tx_skbuff = NULL;
+
+	if (mdp->rx_ring) {
+		ringsize = sizeof(struct sh_eth_rxdesc) * mdp->num_rx_ring;
+		dma_free_coherent(NULL, ringsize, mdp->rx_ring,
+				  mdp->rx_desc_dma);
+		mdp->rx_ring = NULL;
+	}
+
+	if (mdp->tx_ring) {
+		ringsize = sizeof(struct sh_eth_txdesc) * mdp->num_tx_ring;
+		dma_free_coherent(NULL, ringsize, mdp->tx_ring,
+				  mdp->tx_desc_dma);
+		mdp->tx_ring = NULL;
+	}
 }
 
 /* format skb and descriptor buffer */
@@ -1220,14 +1234,14 @@  static int sh_eth_ring_init(struct net_d
 	mdp->tx_skbuff = kcalloc(mdp->num_tx_ring, sizeof(*mdp->tx_skbuff),
 				 GFP_KERNEL);
 	if (!mdp->tx_skbuff)
-		goto skb_ring_free;
+		goto ring_free;
 
 	/* Allocate all Rx descriptors. */
 	rx_ringsize = sizeof(struct sh_eth_rxdesc) * mdp->num_rx_ring;
 	mdp->rx_ring = dma_alloc_coherent(NULL, rx_ringsize, &mdp->rx_desc_dma,
 					  GFP_KERNEL);
 	if (!mdp->rx_ring)
-		goto skb_ring_free;
+		goto ring_free;
 
 	mdp->dirty_rx = 0;
 
@@ -1236,41 +1250,16 @@  static int sh_eth_ring_init(struct net_d
 	mdp->tx_ring = dma_alloc_coherent(NULL, tx_ringsize, &mdp->tx_desc_dma,
 					  GFP_KERNEL);
 	if (!mdp->tx_ring)
-		goto desc_ring_free;
+		goto ring_free;
 	return 0;
 
-desc_ring_free:
-	/* free DMA buffer */
-	dma_free_coherent(NULL, rx_ringsize, mdp->rx_ring, mdp->rx_desc_dma);
-
-skb_ring_free:
-	/* Free Rx and Tx skb ring buffer */
+ring_free:
+	/* Free Rx and Tx skb ring buffer and DMA buffer */
 	sh_eth_ring_free(ndev);
-	mdp->tx_ring = NULL;
-	mdp->rx_ring = NULL;
 
 	return -ENOMEM;
 }
 
-static void sh_eth_free_dma_buffer(struct sh_eth_private *mdp)
-{
-	int ringsize;
-
-	if (mdp->rx_ring) {
-		ringsize = sizeof(struct sh_eth_rxdesc) * mdp->num_rx_ring;
-		dma_free_coherent(NULL, ringsize, mdp->rx_ring,
-				  mdp->rx_desc_dma);
-		mdp->rx_ring = NULL;
-	}
-
-	if (mdp->tx_ring) {
-		ringsize = sizeof(struct sh_eth_txdesc) * mdp->num_tx_ring;
-		dma_free_coherent(NULL, ringsize, mdp->tx_ring,
-				  mdp->tx_desc_dma);
-		mdp->tx_ring = NULL;
-	}
-}
-
 static int sh_eth_dev_init(struct net_device *ndev, bool start)
 {
 	int ret = 0;
@@ -2231,10 +2220,8 @@  static int sh_eth_set_ringparam(struct n
 
 		sh_eth_dev_exit(ndev);
 
-		/* Free all the skbuffs in the Rx queue. */
+		/* Free all the skbuffs in the Rx queue and the DMA buffers. */
 		sh_eth_ring_free(ndev);
-		/* Free DMA buffer */
-		sh_eth_free_dma_buffer(mdp);
 	}
 
 	/* Set new parameters */
@@ -2479,12 +2466,9 @@  static int sh_eth_close(struct net_devic
 
 	free_irq(ndev->irq, ndev);
 
-	/* Free all the skbuffs in the Rx queue. */
+	/* Free all the skbuffs in the Rx queue and the DMA buffer. */
 	sh_eth_ring_free(ndev);
 
-	/* free DMA buffer */
-	sh_eth_free_dma_buffer(mdp);
-
 	pm_runtime_put_sync(&mdp->pdev->dev);
 
 	mdp->is_opened = 0;