[v2] ether1: Use net_device_stats from struct net_device

Submitted by Tobias Klauser on Aug. 18, 2010, 7:04 a.m.

Details

Message ID 1282115064-3701-1-git-send-email-tklauser@distanz.ch
State Superseded
Delegated to: David Miller
Headers show

Commit Message

Tobias Klauser Aug. 18, 2010, 7:04 a.m.
struct net_device has its own struct net_device_stats member, so use
this one instead of a private copy in the ether1_priv struct.

Cc: Dan Carpenter <error27@gmail.com>
Signed-off-by: Tobias Klauser <tklauser@distanz.ch>
---
 drivers/net/arm/ether1.c |   28 ++++++++++++++--------------
 drivers/net/arm/ether1.h |    1 -
 2 files changed, 14 insertions(+), 15 deletions(-)

Comments

Russell King - ARM Linux Aug. 18, 2010, 8:13 a.m.
On Wed, Aug 18, 2010 at 09:04:24AM +0200, Tobias Klauser wrote:
> @@ -965,7 +965,7 @@ ether1_close (struct net_device *dev)
>  static struct net_device_stats *
>  ether1_getstats (struct net_device *dev)
>  {
> -	return &priv(dev)->stats;
> +	return &dev->stats;
>  }

Doesn't the core do this for you already if you omit this function?

Same comment for ether3.c
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Eric Dumazet Aug. 18, 2010, 8:27 a.m.
Le mercredi 18 août 2010 à 09:13 +0100, Russell King - ARM Linux a
écrit :
> On Wed, Aug 18, 2010 at 09:04:24AM +0200, Tobias Klauser wrote:
> > @@ -965,7 +965,7 @@ ether1_close (struct net_device *dev)
> >  static struct net_device_stats *
> >  ether1_getstats (struct net_device *dev)
> >  {
> > -	return &priv(dev)->stats;
> > +	return &dev->stats;
> >  }
> 
> Doesn't the core do this for you already if you omit this function?
> 
> Same comment for ether3.c

Yes, thats right, no need to declare ndo_get_stats() methods that only
returns &dev->stats



--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tobias Klauser Aug. 18, 2010, 8:33 a.m.
On 2010-08-18 at 10:27:53 +0200, Eric Dumazet <eric.dumazet@gmail.com> wrote:
> Le mercredi 18 août 2010 à 09:13 +0100, Russell King - ARM Linux a
> écrit :
> > On Wed, Aug 18, 2010 at 09:04:24AM +0200, Tobias Klauser wrote:
> > > @@ -965,7 +965,7 @@ ether1_close (struct net_device *dev)
> > >  static struct net_device_stats *
> > >  ether1_getstats (struct net_device *dev)
> > >  {
> > > -	return &priv(dev)->stats;
> > > +	return &dev->stats;
> > >  }
> > 
> > Doesn't the core do this for you already if you omit this function?
> > 
> > Same comment for ether3.c
> 
> Yes, thats right, no need to declare ndo_get_stats() methods that only
> returns &dev->stats

Thanks. I'll send another updated patch, omitting the ether1_getstats
(same for ether3). Sorry for the mess.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
stephen hemminger Aug. 18, 2010, 3:33 p.m.
On Wed, 18 Aug 2010 09:04:24 +0200
Tobias Klauser <tklauser@distanz.ch> wrote:

>  
> -	memset (&priv(dev)->stats, 0, sizeof (struct net_device_stats));
> +	memset(&dev->stats, 0, sizeof(struct net_device_stats));

This is incorrect, just remove the memset.  The stats are initialized
when device is created.  The Linux device driver convention is to
keep stats when device is set down and brought back up; that is what
the majority of other drivers do.
David Miller Aug. 18, 2010, 8:10 p.m.
From: Stephen Hemminger <shemminger@vyatta.com>
Date: Wed, 18 Aug 2010 08:33:04 -0700

> On Wed, 18 Aug 2010 09:04:24 +0200
> Tobias Klauser <tklauser@distanz.ch> wrote:
> 
>>  
>> -	memset (&priv(dev)->stats, 0, sizeof (struct net_device_stats));
>> +	memset(&dev->stats, 0, sizeof(struct net_device_stats));
> 
> This is incorrect, just remove the memset.  The stats are initialized
> when device is created.  The Linux device driver convention is to
> keep stats when device is set down and brought back up; that is what
> the majority of other drivers do.

Yep, both the ether1 and ether3 patch have this problem.  Looks
like we'll see v4 coming some :-)
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Tobias Klauser Aug. 19, 2010, 6:14 a.m.
On 2010-08-18 at 22:10:43 +0200, David Miller <davem@davemloft.net> wrote:
> From: Stephen Hemminger <shemminger@vyatta.com>
> Date: Wed, 18 Aug 2010 08:33:04 -0700
> 
> > On Wed, 18 Aug 2010 09:04:24 +0200
> > Tobias Klauser <tklauser@distanz.ch> wrote:
> > 
> >>  
> >> -	memset (&priv(dev)->stats, 0, sizeof (struct net_device_stats));
> >> +	memset(&dev->stats, 0, sizeof(struct net_device_stats));
> > 
> > This is incorrect, just remove the memset.  The stats are initialized
> > when device is created.  The Linux device driver convention is to
> > keep stats when device is set down and brought back up; that is what
> > the majority of other drivers do.
> 
> Yep, both the ether1 and ether3 patch have this problem.  Looks
> like we'll see v4 coming some :-)

Thanks a lot. I'll send another updated patch. Hope I get it right this
time :-)

AFAICS there are a few other network drivers having the same problem,
I'll send patches for those too.
--
To unsubscribe from this list: send the line "unsubscribe netdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch hide | download patch | download mbox

diff --git a/drivers/net/arm/ether1.c b/drivers/net/arm/ether1.c
index b17ab51..f08b6b2 100644
--- a/drivers/net/arm/ether1.c
+++ b/drivers/net/arm/ether1.c
@@ -649,7 +649,7 @@  ether1_open (struct net_device *dev)
 	if (request_irq(dev->irq, ether1_interrupt, 0, "ether1", dev))
 		return -EAGAIN;
 
-	memset (&priv(dev)->stats, 0, sizeof (struct net_device_stats));
+	memset(&dev->stats, 0, sizeof(struct net_device_stats));
 
 	if (ether1_init_for_open (dev)) {
 		free_irq (dev->irq, dev);
@@ -673,7 +673,7 @@  ether1_timeout(struct net_device *dev)
 	if (ether1_init_for_open (dev))
 		printk (KERN_ERR "%s: unable to restart interface\n", dev->name);
 
-	priv(dev)->stats.tx_errors++;
+	dev->stats.tx_errors++;
 	netif_wake_queue(dev);
 }
 
@@ -802,21 +802,21 @@  again:
 
 	while (nop.nop_status & STAT_COMPLETE) {
 		if (nop.nop_status & STAT_OK) {
-			priv(dev)->stats.tx_packets ++;
-			priv(dev)->stats.collisions += (nop.nop_status & STAT_COLLISIONS);
+			dev->stats.tx_packets++;
+			dev->stats.collisions += (nop.nop_status & STAT_COLLISIONS);
 		} else {
-			priv(dev)->stats.tx_errors ++;
+			dev->stats.tx_errors++;
 
 			if (nop.nop_status & STAT_COLLAFTERTX)
-				priv(dev)->stats.collisions ++;
+				dev->stats.collisions++;
 			if (nop.nop_status & STAT_NOCARRIER)
-				priv(dev)->stats.tx_carrier_errors ++;
+				dev->stats.tx_carrier_errors++;
 			if (nop.nop_status & STAT_TXLOSTCTS)
 				printk (KERN_WARNING "%s: cts lost\n", dev->name);
 			if (nop.nop_status & STAT_TXSLOWDMA)
-				priv(dev)->stats.tx_fifo_errors ++;
+				dev->stats.tx_fifo_errors++;
 			if (nop.nop_status & STAT_COLLEXCESSIVE)
-				priv(dev)->stats.collisions += 16;
+				dev->stats.collisions += 16;
 		}
 
 		if (nop.nop_link == caddr) {
@@ -879,13 +879,13 @@  ether1_recv_done (struct net_device *dev)
 
 				skb->protocol = eth_type_trans (skb, dev);
 				netif_rx (skb);
-				priv(dev)->stats.rx_packets ++;
+				dev->stats.rx_packets++;
 			} else
-				priv(dev)->stats.rx_dropped ++;
+				dev->stats.rx_dropped++;
 		} else {
 			printk(KERN_WARNING "%s: %s\n", dev->name,
 				(rbd.rbd_status & RBD_EOF) ? "oversized packet" : "acnt not valid");
-			priv(dev)->stats.rx_dropped ++;
+			dev->stats.rx_dropped++;
 		}
 
 		nexttail = ether1_readw(dev, priv(dev)->rx_tail, rfd_t, rfd_link, NORMALIRQS);
@@ -939,7 +939,7 @@  ether1_interrupt (int irq, void *dev_id)
 				printk (KERN_WARNING "%s: RU went not ready: RU suspended\n", dev->name);
 				ether1_writew(dev, SCB_CMDRXRESUME, SCB_ADDR, scb_t, scb_command, NORMALIRQS);
 				writeb(CTRL_CA, REG_CONTROL);
-				priv(dev)->stats.rx_dropped ++;	/* we suspended due to lack of buffer space */
+				dev->stats.rx_dropped++;	/* we suspended due to lack of buffer space */
 			} else
 				printk(KERN_WARNING "%s: RU went not ready: %04X\n", dev->name,
 					ether1_readw(dev, SCB_ADDR, scb_t, scb_status, NORMALIRQS));
@@ -965,7 +965,7 @@  ether1_close (struct net_device *dev)
 static struct net_device_stats *
 ether1_getstats (struct net_device *dev)
 {
-	return &priv(dev)->stats;
+	return &dev->stats;
 }
 
 /*
diff --git a/drivers/net/arm/ether1.h b/drivers/net/arm/ether1.h
index c8a4b23..3a5830a 100644
--- a/drivers/net/arm/ether1.h
+++ b/drivers/net/arm/ether1.h
@@ -38,7 +38,6 @@ 
 
 struct ether1_priv {
 	void __iomem *base;
-	struct net_device_stats stats;
 	unsigned int tx_link;
 	unsigned int tx_head;
 	volatile unsigned int tx_tail;