All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH -next] sundance: Add initial ethtool stats support
@ 2010-10-09  9:53 Denis Kirjanov
  2010-10-09 12:17 ` [PATCH net-next] sundance: get_stats proper locking Eric Dumazet
                   ` (2 more replies)
  0 siblings, 3 replies; 18+ messages in thread
From: Denis Kirjanov @ 2010-10-09  9:53 UTC (permalink / raw)
  To: davem; +Cc: netdev

Add initial ethtool statistics support 

Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org>
---
 drivers/net/sundance.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/drivers/net/sundance.c b/drivers/net/sundance.c
index 27d69aa..685845b 100644
--- a/drivers/net/sundance.c
+++ b/drivers/net/sundance.c
@@ -1564,6 +1564,18 @@ static int __set_mac_addr(struct net_device *dev)
 	return 0;
 }
 
+static const struct {
+	const char name[ETH_GSTRING_LEN];
+} sundance_stats[] = {
+	{ "tx_packets" },
+	{ "tx_bytes" },
+	{ "rx_packets" },
+	{ "rx_bytes" },
+	{ "tx_errors" },
+	{ "tx_dropped" },
+	{ "rx_errors" },
+};
+
 static int check_if_running(struct net_device *dev)
 {
 	if (!netif_running(dev))
@@ -1622,6 +1634,37 @@ static void set_msglevel(struct net_device *dev, u32 val)
 	np->msg_enable = val;
 }
 
+static void get_strings(struct net_device *dev, u32 stringset,
+		u8 *data)
+{
+	memcpy(data, sundance_stats, sizeof(sundance_stats));
+}
+
+static int get_sset_count(struct net_device *dev, int sset)
+{
+	switch (sset) {
+	case ETH_SS_STATS:
+		return ARRAY_SIZE(sundance_stats);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static void get_ethtool_stats(struct net_device *dev,
+		struct ethtool_stats *stats, u64 *data)
+{
+	struct net_device_stats *netdev_stats = get_stats(dev);
+	int i = 0;
+
+	data[i++] = netdev_stats->tx_packets;
+	data[i++] = netdev_stats->tx_bytes;
+	data[i++] = netdev_stats->rx_packets;
+	data[i++] = netdev_stats->rx_bytes;
+	data[i++] = netdev_stats->tx_errors;
+	data[i++] = netdev_stats->tx_dropped;
+	data[i++] = netdev_stats->rx_errors;
+}
+
 static const struct ethtool_ops ethtool_ops = {
 	.begin = check_if_running,
 	.get_drvinfo = get_drvinfo,
@@ -1631,6 +1674,9 @@ static const struct ethtool_ops ethtool_ops = {
 	.get_link = get_link,
 	.get_msglevel = get_msglevel,
 	.set_msglevel = set_msglevel,
+	.get_strings = get_strings,
+	.get_sset_count = get_sset_count,
+	.get_ethtool_stats = get_ethtool_stats,
 };
 
 static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
-- 
1.7.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* [PATCH net-next] sundance: get_stats proper locking
  2010-10-09  9:53 [PATCH -next] sundance: Add initial ethtool stats support Denis Kirjanov
@ 2010-10-09 12:17 ` Eric Dumazet
  2010-10-09 16:24   ` David Miller
  2010-10-09 12:33 ` [PATCH -next] sundance: Add initial ethtool stats support Eric Dumazet
  2010-10-09 13:27 ` Ben Hutchings
  2 siblings, 1 reply; 18+ messages in thread
From: Eric Dumazet @ 2010-10-09 12:17 UTC (permalink / raw)
  To: Denis Kirjanov, David Miller; +Cc: netdev

Le samedi 09 octobre 2010 à 09:53 +0000, Denis Kirjanov a écrit :
> Add initial ethtool statistics support 

OK, I guess its time to add proper locking into sundance after all ;)

[PATCH net-next] sundance: get_stats proper locking

sundance get_stats() should not be run concurrently, add a lock to avoid
potential losses.

Note: Remove unused rx_lock field

Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>
---
 drivers/net/sundance.c |   10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/drivers/net/sundance.c b/drivers/net/sundance.c
index 27d69aa..4283cc5 100644
--- a/drivers/net/sundance.c
+++ b/drivers/net/sundance.c
@@ -365,7 +365,6 @@ struct netdev_private {
 	struct timer_list timer;		/* Media monitoring timer. */
 	/* Frequently used values: keep some adjacent for cache effect. */
 	spinlock_t lock;
-	spinlock_t rx_lock;			/* Group with Tx control cache line. */
 	int msg_enable;
 	int chip_id;
 	unsigned int cur_rx, dirty_rx;		/* Producer/consumer ring indices */
@@ -390,6 +389,7 @@ struct netdev_private {
 	unsigned char phys[MII_CNT];		/* MII device addresses, only first one used. */
 	struct pci_dev *pci_dev;
 	void __iomem *base;
+	spinlock_t statlock;
 };
 
 /* The station address location in the EEPROM. */
@@ -514,6 +514,7 @@ static int __devinit sundance_probe1 (struct pci_dev *pdev,
 	np->chip_id = chip_idx;
 	np->msg_enable = (1 << debug) - 1;
 	spin_lock_init(&np->lock);
+	spin_lock_init(&np->statlock);
 	tasklet_init(&np->rx_tasklet, rx_poll, (unsigned long)dev);
 	tasklet_init(&np->tx_tasklet, tx_poll, (unsigned long)dev);
 
@@ -1486,10 +1487,9 @@ static struct net_device_stats *get_stats(struct net_device *dev)
 	struct netdev_private *np = netdev_priv(dev);
 	void __iomem *ioaddr = np->base;
 	int i;
+	unsigned long flags;
 
-	/* We should lock this segment of code for SMP eventually, although
-	   the vulnerability window is very small and statistics are
-	   non-critical. */
+	spin_lock_irqsave(&np->statlock, flags);
 	/* The chip only need report frame silently dropped. */
 	dev->stats.rx_missed_errors	+= ioread8(ioaddr + RxMissed);
 	dev->stats.tx_packets += ioread16(ioaddr + TxFramesOK);
@@ -1506,6 +1506,8 @@ static struct net_device_stats *get_stats(struct net_device *dev)
 	dev->stats.rx_bytes += ioread16(ioaddr + RxOctetsLow);
 	dev->stats.rx_bytes += ioread16(ioaddr + RxOctetsHigh) << 16;
 
+	spin_unlock_irqrestore(&np->statlock, flags);
+
 	return &dev->stats;
 }
 



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH -next] sundance: Add initial ethtool stats support
  2010-10-09  9:53 [PATCH -next] sundance: Add initial ethtool stats support Denis Kirjanov
  2010-10-09 12:17 ` [PATCH net-next] sundance: get_stats proper locking Eric Dumazet
@ 2010-10-09 12:33 ` Eric Dumazet
  2010-10-09 13:27 ` Ben Hutchings
  2 siblings, 0 replies; 18+ messages in thread
From: Eric Dumazet @ 2010-10-09 12:33 UTC (permalink / raw)
  To: Denis Kirjanov; +Cc: davem, netdev

Le samedi 09 octobre 2010 à 09:53 +0000, Denis Kirjanov a écrit :
> Add initial ethtool statistics support 
> 
> Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org>
> ---
>  drivers/net/sundance.c |   46 ++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 46 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/net/sundance.c b/drivers/net/sundance.c
> index 27d69aa..685845b 100644
> --- a/drivers/net/sundance.c
> +++ b/drivers/net/sundance.c
> @@ -1564,6 +1564,18 @@ static int __set_mac_addr(struct net_device *dev)
>  	return 0;
>  }
>  
> +static const struct {
> +	const char name[ETH_GSTRING_LEN];
> +} sundance_stats[] = {
> +	{ "tx_packets" },
> +	{ "tx_bytes" },
> +	{ "rx_packets" },
> +	{ "rx_bytes" },
> +	{ "tx_errors" },
> +	{ "tx_dropped" },
> +	{ "rx_errors" },
> +};
> +
>  static int check_if_running(struct net_device *dev)
>  {
>  	if (!netif_running(dev))
> @@ -1622,6 +1634,37 @@ static void set_msglevel(struct net_device *dev, u32 val)
>  	np->msg_enable = val;
>  }
>  
> +static void get_strings(struct net_device *dev, u32 stringset,
> +		u8 *data)
> +{

	if (stringset != ETH_SS_STATS)
		return;

> +	memcpy(data, sundance_stats, sizeof(sundance_stats));
> +}
> +
> +static int get_sset_count(struct net_device *dev, int sset)
> +{
> +	switch (sset) {
> +	case ETH_SS_STATS:
> +		return ARRAY_SIZE(sundance_stats);
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +}
> +
> +static void get_ethtool_stats(struct net_device *dev,
> +		struct ethtool_stats *stats, u64 *data)
> +{
> +	struct net_device_stats *netdev_stats = get_stats(dev);
> +	int i = 0;
> +
> +	data[i++] = netdev_stats->tx_packets;
> +	data[i++] = netdev_stats->tx_bytes;
> +	data[i++] = netdev_stats->rx_packets;
> +	data[i++] = netdev_stats->rx_bytes;
> +	data[i++] = netdev_stats->tx_errors;
> +	data[i++] = netdev_stats->tx_dropped;
> +	data[i++] = netdev_stats->rx_errors;
> +}
> +
>  static const struct ethtool_ops ethtool_ops = {
>  	.begin = check_if_running,
>  	.get_drvinfo = get_drvinfo,
> @@ -1631,6 +1674,9 @@ static const struct ethtool_ops ethtool_ops = {
>  	.get_link = get_link,
>  	.get_msglevel = get_msglevel,
>  	.set_msglevel = set_msglevel,
> +	.get_strings = get_strings,
> +	.get_sset_count = get_sset_count,
> +	.get_ethtool_stats = get_ethtool_stats,
>  };
>  
>  static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH -next] sundance: Add initial ethtool stats support
  2010-10-09  9:53 [PATCH -next] sundance: Add initial ethtool stats support Denis Kirjanov
  2010-10-09 12:17 ` [PATCH net-next] sundance: get_stats proper locking Eric Dumazet
  2010-10-09 12:33 ` [PATCH -next] sundance: Add initial ethtool stats support Eric Dumazet
@ 2010-10-09 13:27 ` Ben Hutchings
  2010-10-09 14:40   ` Denis Kirjanov
  2010-10-09 19:41   ` Denis Kirjanov
  2 siblings, 2 replies; 18+ messages in thread
From: Ben Hutchings @ 2010-10-09 13:27 UTC (permalink / raw)
  To: Denis Kirjanov; +Cc: davem, netdev

Denis Kirjanov wrote:
> Add initial ethtool statistics support 
[...]
> +static void get_ethtool_stats(struct net_device *dev,
> +		struct ethtool_stats *stats, u64 *data)
> +{
> +	struct net_device_stats *netdev_stats = get_stats(dev);
> +	int i = 0;
> +
> +	data[i++] = netdev_stats->tx_packets;
> +	data[i++] = netdev_stats->tx_bytes;
> +	data[i++] = netdev_stats->rx_packets;
> +	data[i++] = netdev_stats->rx_bytes;
> +	data[i++] = netdev_stats->tx_errors;
> +	data[i++] = netdev_stats->tx_dropped;
> +	data[i++] = netdev_stats->rx_errors;
> +}
[...]

There is no point in adding ethtool stats that merely mirror the baseline
net device stats.

Ben.

-- 
Ben Hutchings, Senior Software Engineer, Solarflare Communications
Not speaking for my employer; that's the marketing department's job.
They asked us to note that Solarflare product names are trademarked.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH -next] sundance: Add initial ethtool stats support
  2010-10-09 13:27 ` Ben Hutchings
@ 2010-10-09 14:40   ` Denis Kirjanov
  2010-10-09 19:41   ` Denis Kirjanov
  1 sibling, 0 replies; 18+ messages in thread
From: Denis Kirjanov @ 2010-10-09 14:40 UTC (permalink / raw)
  To: Ben Hutchings; +Cc: davem, netdev, Eric Dumazet

On 10/09/2010 05:27 PM, Ben Hutchings wrote:
> Denis Kirjanov wrote:
>> Add initial ethtool statistics support 
> [...]
>> +static void get_ethtool_stats(struct net_device *dev,
>> +		struct ethtool_stats *stats, u64 *data)
>> +{
>> +	struct net_device_stats *netdev_stats = get_stats(dev);
>> +	int i = 0;
>> +
>> +	data[i++] = netdev_stats->tx_packets;
>> +	data[i++] = netdev_stats->tx_bytes;
>> +	data[i++] = netdev_stats->rx_packets;
>> +	data[i++] = netdev_stats->rx_bytes;
>> +	data[i++] = netdev_stats->tx_errors;
>> +	data[i++] = netdev_stats->tx_dropped;
>> +	data[i++] = netdev_stats->rx_errors;
>> +}
> [...]
> 
> There is no point in adding ethtool stats that merely mirror the baseline
> net device stats.
Fair enough,
I'll add extra stats shortly
> Ben.
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH net-next] sundance: get_stats proper locking
  2010-10-09 12:17 ` [PATCH net-next] sundance: get_stats proper locking Eric Dumazet
@ 2010-10-09 16:24   ` David Miller
  0 siblings, 0 replies; 18+ messages in thread
From: David Miller @ 2010-10-09 16:24 UTC (permalink / raw)
  To: eric.dumazet; +Cc: dkirjanov, netdev

From: Eric Dumazet <eric.dumazet@gmail.com>
Date: Sat, 09 Oct 2010 14:17:01 +0200

> Le samedi 09 octobre 2010 à 09:53 +0000, Denis Kirjanov a écrit :
>> Add initial ethtool statistics support 
> 
> OK, I guess its time to add proper locking into sundance after all ;)
> 
> [PATCH net-next] sundance: get_stats proper locking
> 
> sundance get_stats() should not be run concurrently, add a lock to avoid
> potential losses.
> 
> Note: Remove unused rx_lock field
> 
> Signed-off-by: Eric Dumazet <eric.dumazet@gmail.com>

Applied.

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH -next] sundance: Add initial ethtool stats support
  2010-10-09 13:27 ` Ben Hutchings
  2010-10-09 14:40   ` Denis Kirjanov
@ 2010-10-09 19:41   ` Denis Kirjanov
  2010-10-09 21:48     ` Jeff Garzik
  2010-10-12 18:51     ` David Miller
  1 sibling, 2 replies; 18+ messages in thread
From: Denis Kirjanov @ 2010-10-09 19:41 UTC (permalink / raw)
  To: netdev; +Cc: davem, Eric Dumazet, Ben Hutchings

On 10/09/2010 05:27 PM, Ben Hutchings wrote:
> Denis Kirjanov wrote:
>> Add initial ethtool statistics support 
> [...]
>> +static void get_ethtool_stats(struct net_device *dev,
>> +		struct ethtool_stats *stats, u64 *data)
>> +{
>> +	struct net_device_stats *netdev_stats = get_stats(dev);
>> +	int i = 0;
>> +
>> +	data[i++] = netdev_stats->tx_packets;
>> +	data[i++] = netdev_stats->tx_bytes;
>> +	data[i++] = netdev_stats->rx_packets;
>> +	data[i++] = netdev_stats->rx_bytes;
>> +	data[i++] = netdev_stats->tx_errors;
>> +	data[i++] = netdev_stats->tx_dropped;
>> +	data[i++] = netdev_stats->rx_errors;
>> +}
> [...]
> 
> There is no point in adding ethtool stats that merely mirror the baseline
> net device stats.
> 
> Ben.
> 

[PATCH -next v2] sundance: Add ethtool stats support

Add ethtool stats support

Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org>
---
V2: 
 check for the ETH_SS_STATS in get_string()
 use xstats struct for ethtool stats

drivers/net/sundance.c |   90 ++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 83 insertions(+), 7 deletions(-)

diff --git a/drivers/net/sundance.c b/drivers/net/sundance.c
index 4283cc5..4e3ff71 100644
--- a/drivers/net/sundance.c
+++ b/drivers/net/sundance.c
@@ -363,6 +363,19 @@ struct netdev_private {
         dma_addr_t tx_ring_dma;
         dma_addr_t rx_ring_dma;
 	struct timer_list timer;		/* Media monitoring timer. */
+	/* ethtool extra stats */
+	struct {
+		unsigned long tx_multiple_collisions;
+		unsigned long tx_single_collisions;
+		unsigned long tx_late_collisions;
+		unsigned long tx_deffered;
+		unsigned long tx_deffered_excessive;
+		unsigned long tx_aborted;
+		unsigned long tx_bcasts;
+		unsigned long rx_bcasts;
+		unsigned long tx_mcasts;
+		unsigned long rx_mcasts;
+	} xstats;
 	/* Frequently used values: keep some adjacent for cache effect. */
 	spinlock_t lock;
 	int msg_enable;
@@ -1486,7 +1499,6 @@ static struct net_device_stats *get_stats(struct net_device *dev)
 {
 	struct netdev_private *np = netdev_priv(dev);
 	void __iomem *ioaddr = np->base;
-	int i;
 	unsigned long flags;
 
 	spin_lock_irqsave(&np->statlock, flags);
@@ -1494,13 +1506,23 @@ static struct net_device_stats *get_stats(struct net_device *dev)
 	dev->stats.rx_missed_errors	+= ioread8(ioaddr + RxMissed);
 	dev->stats.tx_packets += ioread16(ioaddr + TxFramesOK);
 	dev->stats.rx_packets += ioread16(ioaddr + RxFramesOK);
-	dev->stats.collisions += ioread8(ioaddr + StatsLateColl);
-	dev->stats.collisions += ioread8(ioaddr + StatsMultiColl);
-	dev->stats.collisions += ioread8(ioaddr + StatsOneColl);
 	dev->stats.tx_carrier_errors += ioread8(ioaddr + StatsCarrierError);
-	ioread8(ioaddr + StatsTxDefer);
-	for (i = StatsTxDefer; i <= StatsMcastRx; i++)
-		ioread8(ioaddr + i);
+
+	np->xstats.tx_multiple_collisions += ioread8(ioaddr + StatsMultiColl);
+	np->xstats.tx_single_collisions += ioread8(ioaddr + StatsOneColl);
+	np->xstats.tx_late_collisions += ioread8(ioaddr + StatsLateColl);
+	dev->stats.collisions += np->xstats.tx_multiple_collisions
+		+ np->xstats.tx_single_collisions
+		+ np->xstats.tx_late_collisions;
+
+	np->xstats.tx_deffered += ioread8(ioaddr + StatsTxDefer);
+	np->xstats.tx_deffered_excessive += ioread8(ioaddr + StatsTxXSDefer);
+	np->xstats.tx_aborted += ioread8(ioaddr + StatsTxAbort);
+	np->xstats.tx_bcasts += ioread8(ioaddr + StatsBcastTx);
+	np->xstats.rx_bcasts += ioread8(ioaddr + StatsBcastRx);
+	np->xstats.tx_mcasts += ioread8(ioaddr + StatsMcastTx);
+	np->xstats.rx_mcasts += ioread8(ioaddr + StatsMcastRx);
+
 	dev->stats.tx_bytes += ioread16(ioaddr + TxOctetsLow);
 	dev->stats.tx_bytes += ioread16(ioaddr + TxOctetsHigh) << 16;
 	dev->stats.rx_bytes += ioread16(ioaddr + RxOctetsLow);
@@ -1566,6 +1588,21 @@ static int __set_mac_addr(struct net_device *dev)
 	return 0;
 }
 
+static const struct {
+	const char name[ETH_GSTRING_LEN];
+} sundance_stats[] = {
+	{ "tx_multiple_collisions" },
+	{ "tx_single_collisions" },
+	{ "tx_late_collisions" },
+	{ "tx_deffered" },
+	{ "tx_deffered_excessive" },
+	{ "tx_aborted" },
+	{ "tx_bcasts" },
+	{ "rx_bcasts" },
+	{ "tx_mcasts" },
+	{ "rx_mcasts" },
+};
+
 static int check_if_running(struct net_device *dev)
 {
 	if (!netif_running(dev))
@@ -1624,6 +1661,42 @@ static void set_msglevel(struct net_device *dev, u32 val)
 	np->msg_enable = val;
 }
 
+static void get_strings(struct net_device *dev, u32 stringset,
+		u8 *data)
+{
+	if (stringset == ETH_SS_STATS)
+		memcpy(data, sundance_stats, sizeof(sundance_stats));
+}
+
+static int get_sset_count(struct net_device *dev, int sset)
+{
+	switch (sset) {
+	case ETH_SS_STATS:
+		return ARRAY_SIZE(sundance_stats);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static void get_ethtool_stats(struct net_device *dev,
+		struct ethtool_stats *stats, u64 *data)
+{
+	struct netdev_private *np = netdev_priv(dev);
+	int i = 0;
+
+	get_stats(dev);
+	data[i++] = np->xstats.tx_multiple_collisions;
+	data[i++] = np->xstats.tx_single_collisions;
+	data[i++] = np->xstats.tx_late_collisions;
+	data[i++] = np->xstats.tx_deffered;
+	data[i++] = np->xstats.tx_deffered_excessive;
+	data[i++] = np->xstats.tx_aborted;
+	data[i++] = np->xstats.tx_bcasts;
+	data[i++] = np->xstats.rx_bcasts;
+	data[i++] = np->xstats.tx_mcasts;
+	data[i++] = np->xstats.rx_mcasts;
+}
+
 static const struct ethtool_ops ethtool_ops = {
 	.begin = check_if_running,
 	.get_drvinfo = get_drvinfo,
@@ -1633,6 +1706,9 @@ static const struct ethtool_ops ethtool_ops = {
 	.get_link = get_link,
 	.get_msglevel = get_msglevel,
 	.set_msglevel = set_msglevel,
+	.get_strings = get_strings,
+	.get_sset_count = get_sset_count,
+	.get_ethtool_stats = get_ethtool_stats,
 };
 
 static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
-- 
1.7.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH -next] sundance: Add initial ethtool stats support
  2010-10-09 19:41   ` Denis Kirjanov
@ 2010-10-09 21:48     ` Jeff Garzik
  2010-10-12 18:51     ` David Miller
  1 sibling, 0 replies; 18+ messages in thread
From: Jeff Garzik @ 2010-10-09 21:48 UTC (permalink / raw)
  To: Denis Kirjanov; +Cc: netdev, davem, Eric Dumazet, Ben Hutchings

On 10/09/2010 03:41 PM, Denis Kirjanov wrote:
> On 10/09/2010 05:27 PM, Ben Hutchings wrote:
>> Denis Kirjanov wrote:
>>> Add initial ethtool statistics support
>> [...]
>>> +static void get_ethtool_stats(struct net_device *dev,
>>> +		struct ethtool_stats *stats, u64 *data)
>>> +{
>>> +	struct net_device_stats *netdev_stats = get_stats(dev);
>>> +	int i = 0;
>>> +
>>> +	data[i++] = netdev_stats->tx_packets;
>>> +	data[i++] = netdev_stats->tx_bytes;
>>> +	data[i++] = netdev_stats->rx_packets;
>>> +	data[i++] = netdev_stats->rx_bytes;
>>> +	data[i++] = netdev_stats->tx_errors;
>>> +	data[i++] = netdev_stats->tx_dropped;
>>> +	data[i++] = netdev_stats->rx_errors;
>>> +}
>> [...]
>>
>> There is no point in adding ethtool stats that merely mirror the baseline
>> net device stats.
>>
>> Ben.
>>
>
> [PATCH -next v2] sundance: Add ethtool stats support
>
> Add ethtool stats support
>
> Signed-off-by: Denis Kirjanov<dkirjanov@kernel.org>
> ---
> V2:
>   check for the ETH_SS_STATS in get_string()
>   use xstats struct for ethtool stats
>
> drivers/net/sundance.c |   90 ++++++++++++++++++++++++++++++++++++++++++++----
>   1 files changed, 83 insertions(+), 7 deletions(-)

Acked-by: Jeff Garzik <jgarzik@redhat.com>

glad somebody tackled this...



^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH -next] sundance: Add initial ethtool stats support
  2010-10-09 19:41   ` Denis Kirjanov
  2010-10-09 21:48     ` Jeff Garzik
@ 2010-10-12 18:51     ` David Miller
  2010-10-12 20:36       ` Denis Kirjanov
  1 sibling, 1 reply; 18+ messages in thread
From: David Miller @ 2010-10-12 18:51 UTC (permalink / raw)
  To: dkirjanov; +Cc: netdev, eric.dumazet, bhutchings

From: Denis Kirjanov <dkirjanov@kernel.org>
Date: Sat, 09 Oct 2010 23:41:32 +0400

> +	/* ethtool extra stats */
> +	struct {
> +		unsigned long tx_multiple_collisions;
> +		unsigned long tx_single_collisions;
> +		unsigned long tx_late_collisions;
> +		unsigned long tx_deffered;
> +		unsigned long tx_deffered_excessive;
> +		unsigned long tx_aborted;
> +		unsigned long tx_bcasts;
> +		unsigned long rx_bcasts;
> +		unsigned long tx_mcasts;
> +		unsigned long rx_mcasts;
> +	} xstats;

I think these should be "u64".

^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH -next] sundance: Add initial ethtool stats support
  2010-10-12 18:51     ` David Miller
@ 2010-10-12 20:36       ` Denis Kirjanov
  2010-10-12 20:57         ` Joe Perches
  0 siblings, 1 reply; 18+ messages in thread
From: Denis Kirjanov @ 2010-10-12 20:36 UTC (permalink / raw)
  To: David Miller; +Cc: netdev, eric.dumazet, Ben Hutchings, Jeff Garzik

On 10/12/2010 10:51 PM, David Miller wrote:
> From: Denis Kirjanov <dkirjanov@kernel.org>
> Date: Sat, 09 Oct 2010 23:41:32 +0400
> 
>> +	/* ethtool extra stats */
>> +	struct {
>> +		unsigned long tx_multiple_collisions;
>> +		unsigned long tx_single_collisions;
>> +		unsigned long tx_late_collisions;
>> +		unsigned long tx_deffered;
>> +		unsigned long tx_deffered_excessive;
>> +		unsigned long tx_aborted;
>> +		unsigned long tx_bcasts;
>> +		unsigned long rx_bcasts;
>> +		unsigned long tx_mcasts;
>> +		unsigned long rx_mcasts;
>> +	} xstats;
> 
> I think these should be "u64".
> 

[PATCH -next v3] sundance: Add ethtool stats support
Add ethtool stats support

Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org>
---
V2: 
 check for the ETH_SS_STATS in get_string()
 use xstats struct for ethtool stats
V3:
 make counters 64-bits wide
 
 drivers/net/sundance.c |   90 ++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 83 insertions(+), 7 deletions(-)

diff --git a/drivers/net/sundance.c b/drivers/net/sundance.c
index 4283cc5..159f7e7 100644
--- a/drivers/net/sundance.c
+++ b/drivers/net/sundance.c
@@ -363,6 +363,19 @@ struct netdev_private {
         dma_addr_t tx_ring_dma;
         dma_addr_t rx_ring_dma;
 	struct timer_list timer;		/* Media monitoring timer. */
+	/* ethtool extra stats */
+	struct {
+		u64 tx_multiple_collisions;
+		u64 tx_single_collisions;
+		u64 tx_late_collisions;
+		u64 tx_deffered;
+		u64 tx_deffered_excessive;
+		u64 tx_aborted;
+		u64 tx_bcasts;
+		u64 rx_bcasts;
+		u64 tx_mcasts;
+		u64 rx_mcasts;
+	} xstats;
 	/* Frequently used values: keep some adjacent for cache effect. */
 	spinlock_t lock;
 	int msg_enable;
@@ -1486,7 +1499,6 @@ static struct net_device_stats *get_stats(struct net_device *dev)
 {
 	struct netdev_private *np = netdev_priv(dev);
 	void __iomem *ioaddr = np->base;
-	int i;
 	unsigned long flags;
 
 	spin_lock_irqsave(&np->statlock, flags);
@@ -1494,13 +1506,23 @@ static struct net_device_stats *get_stats(struct net_device *dev)
 	dev->stats.rx_missed_errors	+= ioread8(ioaddr + RxMissed);
 	dev->stats.tx_packets += ioread16(ioaddr + TxFramesOK);
 	dev->stats.rx_packets += ioread16(ioaddr + RxFramesOK);
-	dev->stats.collisions += ioread8(ioaddr + StatsLateColl);
-	dev->stats.collisions += ioread8(ioaddr + StatsMultiColl);
-	dev->stats.collisions += ioread8(ioaddr + StatsOneColl);
 	dev->stats.tx_carrier_errors += ioread8(ioaddr + StatsCarrierError);
-	ioread8(ioaddr + StatsTxDefer);
-	for (i = StatsTxDefer; i <= StatsMcastRx; i++)
-		ioread8(ioaddr + i);
+
+	np->xstats.tx_multiple_collisions += ioread8(ioaddr + StatsMultiColl);
+	np->xstats.tx_single_collisions += ioread8(ioaddr + StatsOneColl);
+	np->xstats.tx_late_collisions += ioread8(ioaddr + StatsLateColl);
+	dev->stats.collisions += np->xstats.tx_multiple_collisions
+		+ np->xstats.tx_single_collisions
+		+ np->xstats.tx_late_collisions;
+
+	np->xstats.tx_deffered += ioread8(ioaddr + StatsTxDefer);
+	np->xstats.tx_deffered_excessive += ioread8(ioaddr + StatsTxXSDefer);
+	np->xstats.tx_aborted += ioread8(ioaddr + StatsTxAbort);
+	np->xstats.tx_bcasts += ioread8(ioaddr + StatsBcastTx);
+	np->xstats.rx_bcasts += ioread8(ioaddr + StatsBcastRx);
+	np->xstats.tx_mcasts += ioread8(ioaddr + StatsMcastTx);
+	np->xstats.rx_mcasts += ioread8(ioaddr + StatsMcastRx);
+
 	dev->stats.tx_bytes += ioread16(ioaddr + TxOctetsLow);
 	dev->stats.tx_bytes += ioread16(ioaddr + TxOctetsHigh) << 16;
 	dev->stats.rx_bytes += ioread16(ioaddr + RxOctetsLow);
@@ -1566,6 +1588,21 @@ static int __set_mac_addr(struct net_device *dev)
 	return 0;
 }
 
+static const struct {
+	const char name[ETH_GSTRING_LEN];
+} sundance_stats[] = {
+	{ "tx_multiple_collisions" },
+	{ "tx_single_collisions" },
+	{ "tx_late_collisions" },
+	{ "tx_deffered" },
+	{ "tx_deffered_excessive" },
+	{ "tx_aborted" },
+	{ "tx_bcasts" },
+	{ "rx_bcasts" },
+	{ "tx_mcasts" },
+	{ "rx_mcasts" },
+};
+
 static int check_if_running(struct net_device *dev)
 {
 	if (!netif_running(dev))
@@ -1624,6 +1661,42 @@ static void set_msglevel(struct net_device *dev, u32 val)
 	np->msg_enable = val;
 }
 
+static void get_strings(struct net_device *dev, u32 stringset,
+		u8 *data)
+{
+	if (stringset == ETH_SS_STATS)
+		memcpy(data, sundance_stats, sizeof(sundance_stats));
+}
+
+static int get_sset_count(struct net_device *dev, int sset)
+{
+	switch (sset) {
+	case ETH_SS_STATS:
+		return ARRAY_SIZE(sundance_stats);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static void get_ethtool_stats(struct net_device *dev,
+		struct ethtool_stats *stats, u64 *data)
+{
+	struct netdev_private *np = netdev_priv(dev);
+	int i = 0;
+
+	get_stats(dev);
+	data[i++] = np->xstats.tx_multiple_collisions;
+	data[i++] = np->xstats.tx_single_collisions;
+	data[i++] = np->xstats.tx_late_collisions;
+	data[i++] = np->xstats.tx_deffered;
+	data[i++] = np->xstats.tx_deffered_excessive;
+	data[i++] = np->xstats.tx_aborted;
+	data[i++] = np->xstats.tx_bcasts;
+	data[i++] = np->xstats.rx_bcasts;
+	data[i++] = np->xstats.tx_mcasts;
+	data[i++] = np->xstats.rx_mcasts;
+}
+
 static const struct ethtool_ops ethtool_ops = {
 	.begin = check_if_running,
 	.get_drvinfo = get_drvinfo,
@@ -1633,6 +1706,9 @@ static const struct ethtool_ops ethtool_ops = {
 	.get_link = get_link,
 	.get_msglevel = get_msglevel,
 	.set_msglevel = set_msglevel,
+	.get_strings = get_strings,
+	.get_sset_count = get_sset_count,
+	.get_ethtool_stats = get_ethtool_stats,
 };
 
 static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
-- 
1.7.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH -next] sundance: Add initial ethtool stats support
  2010-10-12 20:36       ` Denis Kirjanov
@ 2010-10-12 20:57         ` Joe Perches
  2010-10-13  6:06           ` Denis Kirjanov
  0 siblings, 1 reply; 18+ messages in thread
From: Joe Perches @ 2010-10-12 20:57 UTC (permalink / raw)
  To: Denis Kirjanov
  Cc: David Miller, netdev, eric.dumazet, Ben Hutchings, Jeff Garzik

On Wed, 2010-10-13 at 00:36 +0400, Denis Kirjanov wrote:

> diff --git a/drivers/net/sundance.c b/drivers/net/sundance.c

Hi Denis.
 
Just a few trivial misspellings of deffered -> deferred

> index 4283cc5..159f7e7 100644
> --- a/drivers/net/sundance.c
> +++ b/drivers/net/sundance.c
> @@ -363,6 +363,19 @@ struct netdev_private {
>          dma_addr_t tx_ring_dma;
>          dma_addr_t rx_ring_dma;
>  	struct timer_list timer;		/* Media monitoring timer. */
> +	/* ethtool extra stats */
> +	struct {
> +		u64 tx_multiple_collisions;
> +		u64 tx_single_collisions;
> +		u64 tx_late_collisions;
> +		u64 tx_deffered;
> +		u64 tx_deffered_excessive;

1

> +	np->xstats.tx_deffered += ioread8(ioaddr + StatsTxDefer);
> +	np->xstats.tx_deffered_excessive += ioread8(ioaddr + StatsTxXSDefer);

2

> +static const struct {
> +	const char name[ETH_GSTRING_LEN];
> +} sundance_stats[] = {
> +	{ "tx_multiple_collisions" },
> +	{ "tx_single_collisions" },
> +	{ "tx_late_collisions" },
> +	{ "tx_deffered" },
> +	{ "tx_deffered_excessive" },

3

> +	data[i++] = np->xstats.tx_deffered;
> +	data[i++] = np->xstats.tx_deffered_excessive;

4


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH -next] sundance: Add initial ethtool stats support
  2010-10-12 20:57         ` Joe Perches
@ 2010-10-13  6:06           ` Denis Kirjanov
  2010-10-13  6:33             ` Eric Dumazet
  0 siblings, 1 reply; 18+ messages in thread
From: Denis Kirjanov @ 2010-10-13  6:06 UTC (permalink / raw)
  To: netdev
  Cc: David Miller, Joe Perches, eric.dumazet, Ben Hutchings, Jeff Garzik

On 10/13/2010 12:57 AM, Joe Perches wrote:
> On Wed, 2010-10-13 at 00:36 +0400, Denis Kirjanov wrote:
> 
>> diff --git a/drivers/net/sundance.c b/drivers/net/sundance.c
> 
> Hi Denis.
>  
> Just a few trivial misspellings of deffered -> deferred

Hi Joe,
thanks for pointing that out.

[PATCH -next v4] sundance: Add ethtool stats support

Add ethtool stats support

Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org>
---
V2:
 check for the ETH_SS_STATS in get_string()
 use xstats struct for ethtool stats
V3:
 make counters 64-bits wide
V4:
 fixed some misspellings 

 drivers/net/sundance.c |   90 ++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 83 insertions(+), 7 deletions(-)

diff --git a/drivers/net/sundance.c b/drivers/net/sundance.c
index 4283cc5..406ca45 100644
--- a/drivers/net/sundance.c
+++ b/drivers/net/sundance.c
@@ -363,6 +363,19 @@ struct netdev_private {
         dma_addr_t tx_ring_dma;
         dma_addr_t rx_ring_dma;
 	struct timer_list timer;		/* Media monitoring timer. */
+	/* ethtool extra stats */
+	struct {
+		u64 tx_multiple_collisions;
+		u64 tx_single_collisions;
+		u64 tx_late_collisions;
+		u64 tx_defered;
+		u64 tx_defered_excessive;
+		u64 tx_aborted;
+		u64 tx_bcasts;
+		u64 rx_bcasts;
+		u64 tx_mcasts;
+		u64 rx_mcasts;
+	} xstats;
 	/* Frequently used values: keep some adjacent for cache effect. */
 	spinlock_t lock;
 	int msg_enable;
@@ -1486,7 +1499,6 @@ static struct net_device_stats *get_stats(struct net_device *dev)
 {
 	struct netdev_private *np = netdev_priv(dev);
 	void __iomem *ioaddr = np->base;
-	int i;
 	unsigned long flags;
 
 	spin_lock_irqsave(&np->statlock, flags);
@@ -1494,13 +1506,23 @@ static struct net_device_stats *get_stats(struct net_device *dev)
 	dev->stats.rx_missed_errors	+= ioread8(ioaddr + RxMissed);
 	dev->stats.tx_packets += ioread16(ioaddr + TxFramesOK);
 	dev->stats.rx_packets += ioread16(ioaddr + RxFramesOK);
-	dev->stats.collisions += ioread8(ioaddr + StatsLateColl);
-	dev->stats.collisions += ioread8(ioaddr + StatsMultiColl);
-	dev->stats.collisions += ioread8(ioaddr + StatsOneColl);
 	dev->stats.tx_carrier_errors += ioread8(ioaddr + StatsCarrierError);
-	ioread8(ioaddr + StatsTxDefer);
-	for (i = StatsTxDefer; i <= StatsMcastRx; i++)
-		ioread8(ioaddr + i);
+
+	np->xstats.tx_multiple_collisions += ioread8(ioaddr + StatsMultiColl);
+	np->xstats.tx_single_collisions += ioread8(ioaddr + StatsOneColl);
+	np->xstats.tx_late_collisions += ioread8(ioaddr + StatsLateColl);
+	dev->stats.collisions += np->xstats.tx_multiple_collisions
+		+ np->xstats.tx_single_collisions
+		+ np->xstats.tx_late_collisions;
+
+	np->xstats.tx_defered += ioread8(ioaddr + StatsTxDefer);
+	np->xstats.tx_defered_excessive += ioread8(ioaddr + StatsTxXSDefer);
+	np->xstats.tx_aborted += ioread8(ioaddr + StatsTxAbort);
+	np->xstats.tx_bcasts += ioread8(ioaddr + StatsBcastTx);
+	np->xstats.rx_bcasts += ioread8(ioaddr + StatsBcastRx);
+	np->xstats.tx_mcasts += ioread8(ioaddr + StatsMcastTx);
+	np->xstats.rx_mcasts += ioread8(ioaddr + StatsMcastRx);
+
 	dev->stats.tx_bytes += ioread16(ioaddr + TxOctetsLow);
 	dev->stats.tx_bytes += ioread16(ioaddr + TxOctetsHigh) << 16;
 	dev->stats.rx_bytes += ioread16(ioaddr + RxOctetsLow);
@@ -1566,6 +1588,21 @@ static int __set_mac_addr(struct net_device *dev)
 	return 0;
 }
 
+static const struct {
+	const char name[ETH_GSTRING_LEN];
+} sundance_stats[] = {
+	{ "tx_multiple_collisions" },
+	{ "tx_single_collisions" },
+	{ "tx_late_collisions" },
+	{ "tx_defered" },
+	{ "tx_defered_excessive" },
+	{ "tx_aborted" },
+	{ "tx_bcasts" },
+	{ "rx_bcasts" },
+	{ "tx_mcasts" },
+	{ "rx_mcasts" },
+};
+
 static int check_if_running(struct net_device *dev)
 {
 	if (!netif_running(dev))
@@ -1624,6 +1661,42 @@ static void set_msglevel(struct net_device *dev, u32 val)
 	np->msg_enable = val;
 }
 
+static void get_strings(struct net_device *dev, u32 stringset,
+		u8 *data)
+{
+	if (stringset == ETH_SS_STATS)
+		memcpy(data, sundance_stats, sizeof(sundance_stats));
+}
+
+static int get_sset_count(struct net_device *dev, int sset)
+{
+	switch (sset) {
+	case ETH_SS_STATS:
+		return ARRAY_SIZE(sundance_stats);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static void get_ethtool_stats(struct net_device *dev,
+		struct ethtool_stats *stats, u64 *data)
+{
+	struct netdev_private *np = netdev_priv(dev);
+	int i = 0;
+
+	get_stats(dev);
+	data[i++] = np->xstats.tx_multiple_collisions;
+	data[i++] = np->xstats.tx_single_collisions;
+	data[i++] = np->xstats.tx_late_collisions;
+	data[i++] = np->xstats.tx_defered;
+	data[i++] = np->xstats.tx_defered_excessive;
+	data[i++] = np->xstats.tx_aborted;
+	data[i++] = np->xstats.tx_bcasts;
+	data[i++] = np->xstats.rx_bcasts;
+	data[i++] = np->xstats.tx_mcasts;
+	data[i++] = np->xstats.rx_mcasts;
+}
+
 static const struct ethtool_ops ethtool_ops = {
 	.begin = check_if_running,
 	.get_drvinfo = get_drvinfo,
@@ -1633,6 +1706,9 @@ static const struct ethtool_ops ethtool_ops = {
 	.get_link = get_link,
 	.get_msglevel = get_msglevel,
 	.set_msglevel = set_msglevel,
+	.get_strings = get_strings,
+	.get_sset_count = get_sset_count,
+	.get_ethtool_stats = get_ethtool_stats,
 };
 
 static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
-- 
1.7.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH -next] sundance: Add initial ethtool stats support
  2010-10-13  6:06           ` Denis Kirjanov
@ 2010-10-13  6:33             ` Eric Dumazet
  2010-10-13  7:02               ` Denis Kirjanov
  2010-10-13 10:28               ` Denis Kirjanov
  0 siblings, 2 replies; 18+ messages in thread
From: Eric Dumazet @ 2010-10-13  6:33 UTC (permalink / raw)
  To: Denis Kirjanov
  Cc: netdev, David Miller, Joe Perches, Ben Hutchings, Jeff Garzik

Le mercredi 13 octobre 2010 à 10:06 +0400, Denis Kirjanov a écrit :

> @@ -1494,13 +1506,23 @@ static struct net_device_stats *get_stats(struct net_device *dev)
>  	dev->stats.rx_missed_errors	+= ioread8(ioaddr + RxMissed);
>  	dev->stats.tx_packets += ioread16(ioaddr + TxFramesOK);
>  	dev->stats.rx_packets += ioread16(ioaddr + RxFramesOK);
> -	dev->stats.collisions += ioread8(ioaddr + StatsLateColl);
> -	dev->stats.collisions += ioread8(ioaddr + StatsMultiColl);
> -	dev->stats.collisions += ioread8(ioaddr + StatsOneColl);
>  	dev->stats.tx_carrier_errors += ioread8(ioaddr + StatsCarrierError);
> -	ioread8(ioaddr + StatsTxDefer);
> -	for (i = StatsTxDefer; i <= StatsMcastRx; i++)
> -		ioread8(ioaddr + i);
> +
> +	np->xstats.tx_multiple_collisions += ioread8(ioaddr + StatsMultiColl);
> +	np->xstats.tx_single_collisions += ioread8(ioaddr + StatsOneColl);
> +	np->xstats.tx_late_collisions += ioread8(ioaddr + StatsLateColl);
> +	dev->stats.collisions += np->xstats.tx_multiple_collisions
> +		+ np->xstats.tx_single_collisions
> +		+ np->xstats.tx_late_collisions;


Oh well..., really ?

Each time we are calling get_stats(), you do

dev->stats.collisions += (number of accumulated collisions since device
is up)

instead of

dev->stats.collisions += (number of accumulated collisions since last
time we read counters)


Can you see the problem ?




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH -next] sundance: Add initial ethtool stats support
  2010-10-13  6:33             ` Eric Dumazet
@ 2010-10-13  7:02               ` Denis Kirjanov
  2010-10-13 10:28               ` Denis Kirjanov
  1 sibling, 0 replies; 18+ messages in thread
From: Denis Kirjanov @ 2010-10-13  7:02 UTC (permalink / raw)
  To: Eric Dumazet
  Cc: netdev, David Miller, Joe Perches, Ben Hutchings, Jeff Garzik

On 10/13/2010 10:33 AM, Eric Dumazet wrote:
> Le mercredi 13 octobre 2010 à 10:06 +0400, Denis Kirjanov a écrit :
> 
>> @@ -1494,13 +1506,23 @@ static struct net_device_stats *get_stats(struct net_device *dev)
>>  	dev->stats.rx_missed_errors	+= ioread8(ioaddr + RxMissed);
>>  	dev->stats.tx_packets += ioread16(ioaddr + TxFramesOK);
>>  	dev->stats.rx_packets += ioread16(ioaddr + RxFramesOK);
>> -	dev->stats.collisions += ioread8(ioaddr + StatsLateColl);
>> -	dev->stats.collisions += ioread8(ioaddr + StatsMultiColl);
>> -	dev->stats.collisions += ioread8(ioaddr + StatsOneColl);
>>  	dev->stats.tx_carrier_errors += ioread8(ioaddr + StatsCarrierError);
>> -	ioread8(ioaddr + StatsTxDefer);
>> -	for (i = StatsTxDefer; i <= StatsMcastRx; i++)
>> -		ioread8(ioaddr + i);
>> +
>> +	np->xstats.tx_multiple_collisions += ioread8(ioaddr + StatsMultiColl);
>> +	np->xstats.tx_single_collisions += ioread8(ioaddr + StatsOneColl);
>> +	np->xstats.tx_late_collisions += ioread8(ioaddr + StatsLateColl);
>> +	dev->stats.collisions += np->xstats.tx_multiple_collisions
>> +		+ np->xstats.tx_single_collisions
>> +		+ np->xstats.tx_late_collisions;
> 
> 
> Oh well..., really ?
> 
> Each time we are calling get_stats(), you do
> 
> dev->stats.collisions += (number of accumulated collisions since device
> is up)
> 
> instead of
> 
> dev->stats.collisions += (number of accumulated collisions since last
> time we read counters)
> 
> 
> Can you see the problem ?

Oops, thanks Eric!
I'll fix it up and resend
> 
> 


^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH -next] sundance: Add initial ethtool stats support
  2010-10-13  6:33             ` Eric Dumazet
  2010-10-13  7:02               ` Denis Kirjanov
@ 2010-10-13 10:28               ` Denis Kirjanov
  2010-10-13 10:36                 ` Eric Dumazet
  1 sibling, 1 reply; 18+ messages in thread
From: Denis Kirjanov @ 2010-10-13 10:28 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev, David Miller, Ben Hutchings, Jeff Garzik

On 10/13/2010 10:33 AM, Eric Dumazet wrote:
> Le mercredi 13 octobre 2010 à 10:06 +0400, Denis Kirjanov a écrit :
> 
>> @@ -1494,13 +1506,23 @@ static struct net_device_stats *get_stats(struct net_device *dev)
>>  	dev->stats.rx_missed_errors	+= ioread8(ioaddr + RxMissed);
>>  	dev->stats.tx_packets += ioread16(ioaddr + TxFramesOK);
>>  	dev->stats.rx_packets += ioread16(ioaddr + RxFramesOK);
>> -	dev->stats.collisions += ioread8(ioaddr + StatsLateColl);
>> -	dev->stats.collisions += ioread8(ioaddr + StatsMultiColl);
>> -	dev->stats.collisions += ioread8(ioaddr + StatsOneColl);
>>  	dev->stats.tx_carrier_errors += ioread8(ioaddr + StatsCarrierError);
>> -	ioread8(ioaddr + StatsTxDefer);
>> -	for (i = StatsTxDefer; i <= StatsMcastRx; i++)
>> -		ioread8(ioaddr + i);
>> +
>> +	np->xstats.tx_multiple_collisions += ioread8(ioaddr + StatsMultiColl);
>> +	np->xstats.tx_single_collisions += ioread8(ioaddr + StatsOneColl);
>> +	np->xstats.tx_late_collisions += ioread8(ioaddr + StatsLateColl);
>> +	dev->stats.collisions += np->xstats.tx_multiple_collisions
>> +		+ np->xstats.tx_single_collisions
>> +		+ np->xstats.tx_late_collisions;
> 
> 
> Oh well..., really ?
> 
> Each time we are calling get_stats(), you do
> 
> dev->stats.collisions += (number of accumulated collisions since device
> is up)
> 
> instead of
> 
> dev->stats.collisions += (number of accumulated collisions since last
> time we read counters)

Ok, this is a fixed patch.
Thanks.

Subject: [PATCH -next V5] sundance: Add ethtool stats support

Add ethtool stats support

Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org>
---
V2:
 check for the ETH_SS_STATS in get_string()
 use xstats struct for ethtool stats
V3:
 make counters 64-bits wide
V4:
 fixed some misspellings
V5:
 fixed issue with collisions counting

 drivers/net/sundance.c |   94 ++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 87 insertions(+), 7 deletions(-)

diff --git a/drivers/net/sundance.c b/drivers/net/sundance.c
index 4283cc5..bfa2a01 100644
--- a/drivers/net/sundance.c
+++ b/drivers/net/sundance.c
@@ -363,6 +363,19 @@ struct netdev_private {
         dma_addr_t tx_ring_dma;
         dma_addr_t rx_ring_dma;
 	struct timer_list timer;		/* Media monitoring timer. */
+	/* ethtool extra stats */
+	struct {
+		u64 tx_multiple_collisions;
+		u64 tx_single_collisions;
+		u64 tx_late_collisions;
+		u64 tx_defered;
+		u64 tx_defered_excessive;
+		u64 tx_aborted;
+		u64 tx_bcasts;
+		u64 rx_bcasts;
+		u64 tx_mcasts;
+		u64 rx_mcasts;
+	} xstats;
 	/* Frequently used values: keep some adjacent for cache effect. */
 	spinlock_t lock;
 	int msg_enable;
@@ -1486,21 +1499,34 @@ static struct net_device_stats *get_stats(struct net_device *dev)
 {
 	struct netdev_private *np = netdev_priv(dev);
 	void __iomem *ioaddr = np->base;
-	int i;
 	unsigned long flags;
+	u8 late_coll, single_coll, mult_coll;
 
 	spin_lock_irqsave(&np->statlock, flags);
 	/* The chip only need report frame silently dropped. */
 	dev->stats.rx_missed_errors	+= ioread8(ioaddr + RxMissed);
 	dev->stats.tx_packets += ioread16(ioaddr + TxFramesOK);
 	dev->stats.rx_packets += ioread16(ioaddr + RxFramesOK);
-	dev->stats.collisions += ioread8(ioaddr + StatsLateColl);
-	dev->stats.collisions += ioread8(ioaddr + StatsMultiColl);
-	dev->stats.collisions += ioread8(ioaddr + StatsOneColl);
 	dev->stats.tx_carrier_errors += ioread8(ioaddr + StatsCarrierError);
-	ioread8(ioaddr + StatsTxDefer);
-	for (i = StatsTxDefer; i <= StatsMcastRx; i++)
-		ioread8(ioaddr + i);
+
+	mult_coll = ioread8(ioaddr + StatsMultiColl);
+	np->xstats.tx_multiple_collisions += mult_coll;
+	single_coll = ioread8(ioaddr + StatsOneColl);
+	np->xstats.tx_single_collisions += single_coll;
+	late_coll = ioread8(ioaddr + StatsLateColl);
+	np->xstats.tx_late_collisions += late_coll;
+	dev->stats.collisions += mult_coll
+		+ single_coll
+		+ late_coll;
+
+	np->xstats.tx_defered += ioread8(ioaddr + StatsTxDefer);
+	np->xstats.tx_defered_excessive += ioread8(ioaddr + StatsTxXSDefer);
+	np->xstats.tx_aborted += ioread8(ioaddr + StatsTxAbort);
+	np->xstats.tx_bcasts += ioread8(ioaddr + StatsBcastTx);
+	np->xstats.rx_bcasts += ioread8(ioaddr + StatsBcastRx);
+	np->xstats.tx_mcasts += ioread8(ioaddr + StatsMcastTx);
+	np->xstats.rx_mcasts += ioread8(ioaddr + StatsMcastRx);
+
 	dev->stats.tx_bytes += ioread16(ioaddr + TxOctetsLow);
 	dev->stats.tx_bytes += ioread16(ioaddr + TxOctetsHigh) << 16;
 	dev->stats.rx_bytes += ioread16(ioaddr + RxOctetsLow);
@@ -1566,6 +1592,21 @@ static int __set_mac_addr(struct net_device *dev)
 	return 0;
 }
 
+static const struct {
+	const char name[ETH_GSTRING_LEN];
+} sundance_stats[] = {
+	{ "tx_multiple_collisions" },
+	{ "tx_single_collisions" },
+	{ "tx_late_collisions" },
+	{ "tx_defered" },
+	{ "tx_defered_excessive" },
+	{ "tx_aborted" },
+	{ "tx_bcasts" },
+	{ "rx_bcasts" },
+	{ "tx_mcasts" },
+	{ "rx_mcasts" },
+};
+
 static int check_if_running(struct net_device *dev)
 {
 	if (!netif_running(dev))
@@ -1624,6 +1665,42 @@ static void set_msglevel(struct net_device *dev, u32 val)
 	np->msg_enable = val;
 }
 
+static void get_strings(struct net_device *dev, u32 stringset,
+		u8 *data)
+{
+	if (stringset == ETH_SS_STATS)
+		memcpy(data, sundance_stats, sizeof(sundance_stats));
+}
+
+static int get_sset_count(struct net_device *dev, int sset)
+{
+	switch (sset) {
+	case ETH_SS_STATS:
+		return ARRAY_SIZE(sundance_stats);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static void get_ethtool_stats(struct net_device *dev,
+		struct ethtool_stats *stats, u64 *data)
+{
+	struct netdev_private *np = netdev_priv(dev);
+	int i = 0;
+
+	get_stats(dev);
+	data[i++] = np->xstats.tx_multiple_collisions;
+	data[i++] = np->xstats.tx_single_collisions;
+	data[i++] = np->xstats.tx_late_collisions;
+	data[i++] = np->xstats.tx_defered;
+	data[i++] = np->xstats.tx_defered_excessive;
+	data[i++] = np->xstats.tx_aborted;
+	data[i++] = np->xstats.tx_bcasts;
+	data[i++] = np->xstats.rx_bcasts;
+	data[i++] = np->xstats.tx_mcasts;
+	data[i++] = np->xstats.rx_mcasts;
+}
+
 static const struct ethtool_ops ethtool_ops = {
 	.begin = check_if_running,
 	.get_drvinfo = get_drvinfo,
@@ -1633,6 +1710,9 @@ static const struct ethtool_ops ethtool_ops = {
 	.get_link = get_link,
 	.get_msglevel = get_msglevel,
 	.set_msglevel = set_msglevel,
+	.get_strings = get_strings,
+	.get_sset_count = get_sset_count,
+	.get_ethtool_stats = get_ethtool_stats,
 };
 
 static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
-- 
1.7.0



^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH -next] sundance: Add initial ethtool stats support
  2010-10-13 10:28               ` Denis Kirjanov
@ 2010-10-13 10:36                 ` Eric Dumazet
  2010-10-13 10:56                   ` Denis Kirjanov
  0 siblings, 1 reply; 18+ messages in thread
From: Eric Dumazet @ 2010-10-13 10:36 UTC (permalink / raw)
  To: Denis Kirjanov; +Cc: netdev, David Miller, Ben Hutchings, Jeff Garzik

Le mercredi 13 octobre 2010 à 14:28 +0400, Denis Kirjanov a écrit :

> +		u64 tx_defered;
> +		u64 tx_defered_excessive;

Joe said correct english word is "deferred", not "defered"




^ permalink raw reply	[flat|nested] 18+ messages in thread

* Re: [PATCH -next] sundance: Add initial ethtool stats support
  2010-10-13 10:36                 ` Eric Dumazet
@ 2010-10-13 10:56                   ` Denis Kirjanov
  2010-10-13 19:44                     ` Eric Dumazet
  0 siblings, 1 reply; 18+ messages in thread
From: Denis Kirjanov @ 2010-10-13 10:56 UTC (permalink / raw)
  To: Eric Dumazet; +Cc: netdev, David Miller, Ben Hutchings, Jeff Garzik

On 10/13/2010 02:36 PM, Eric Dumazet wrote:
> Le mercredi 13 octobre 2010 à 14:28 +0400, Denis Kirjanov a écrit :
> 
>> +		u64 tx_defered;
>> +		u64 tx_defered_excessive;
> 
PATCH -next v5] sundance: Add ethtool stats support

Add ethtool stats support.

Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org>
---
V2:
 check for the ETH_SS_STATS in get_string()
 use xstats struct for ethtool stats
V3:
 make counters 64-bits wide
V4:
 fixed some misspellings
V5:
 fixed issue with collisions counting

drivers/net/sundance.c |   94 ++++++++++++++++++++++++++++++++++++++++++++----
 1 files changed, 87 insertions(+), 7 deletions(-)

diff --git a/drivers/net/sundance.c b/drivers/net/sundance.c
index 4283cc5..3ed2a67 100644
--- a/drivers/net/sundance.c
+++ b/drivers/net/sundance.c
@@ -363,6 +363,19 @@ struct netdev_private {
         dma_addr_t tx_ring_dma;
         dma_addr_t rx_ring_dma;
 	struct timer_list timer;		/* Media monitoring timer. */
+	/* ethtool extra stats */
+	struct {
+		u64 tx_multiple_collisions;
+		u64 tx_single_collisions;
+		u64 tx_late_collisions;
+		u64 tx_deferred;
+		u64 tx_deferred_excessive;
+		u64 tx_aborted;
+		u64 tx_bcasts;
+		u64 rx_bcasts;
+		u64 tx_mcasts;
+		u64 rx_mcasts;
+	} xstats;
 	/* Frequently used values: keep some adjacent for cache effect. */
 	spinlock_t lock;
 	int msg_enable;
@@ -1486,21 +1499,34 @@ static struct net_device_stats *get_stats(struct net_device *dev)
 {
 	struct netdev_private *np = netdev_priv(dev);
 	void __iomem *ioaddr = np->base;
-	int i;
 	unsigned long flags;
+	u8 late_coll, single_coll, mult_coll;
 
 	spin_lock_irqsave(&np->statlock, flags);
 	/* The chip only need report frame silently dropped. */
 	dev->stats.rx_missed_errors	+= ioread8(ioaddr + RxMissed);
 	dev->stats.tx_packets += ioread16(ioaddr + TxFramesOK);
 	dev->stats.rx_packets += ioread16(ioaddr + RxFramesOK);
-	dev->stats.collisions += ioread8(ioaddr + StatsLateColl);
-	dev->stats.collisions += ioread8(ioaddr + StatsMultiColl);
-	dev->stats.collisions += ioread8(ioaddr + StatsOneColl);
 	dev->stats.tx_carrier_errors += ioread8(ioaddr + StatsCarrierError);
-	ioread8(ioaddr + StatsTxDefer);
-	for (i = StatsTxDefer; i <= StatsMcastRx; i++)
-		ioread8(ioaddr + i);
+
+	mult_coll = ioread8(ioaddr + StatsMultiColl);
+	np->xstats.tx_multiple_collisions += mult_coll;
+	single_coll = ioread8(ioaddr + StatsOneColl);
+	np->xstats.tx_single_collisions += single_coll;
+	late_coll = ioread8(ioaddr + StatsLateColl);
+	np->xstats.tx_late_collisions += late_coll;
+	dev->stats.collisions += mult_coll
+		+ single_coll
+		+ late_coll;
+
+	np->xstats.tx_deferred += ioread8(ioaddr + StatsTxDefer);
+	np->xstats.tx_deferred_excessive += ioread8(ioaddr + StatsTxXSDefer);
+	np->xstats.tx_aborted += ioread8(ioaddr + StatsTxAbort);
+	np->xstats.tx_bcasts += ioread8(ioaddr + StatsBcastTx);
+	np->xstats.rx_bcasts += ioread8(ioaddr + StatsBcastRx);
+	np->xstats.tx_mcasts += ioread8(ioaddr + StatsMcastTx);
+	np->xstats.rx_mcasts += ioread8(ioaddr + StatsMcastRx);
+
 	dev->stats.tx_bytes += ioread16(ioaddr + TxOctetsLow);
 	dev->stats.tx_bytes += ioread16(ioaddr + TxOctetsHigh) << 16;
 	dev->stats.rx_bytes += ioread16(ioaddr + RxOctetsLow);
@@ -1566,6 +1592,21 @@ static int __set_mac_addr(struct net_device *dev)
 	return 0;
 }
 
+static const struct {
+	const char name[ETH_GSTRING_LEN];
+} sundance_stats[] = {
+	{ "tx_multiple_collisions" },
+	{ "tx_single_collisions" },
+	{ "tx_late_collisions" },
+	{ "tx_deferred" },
+	{ "tx_deferred_excessive" },
+	{ "tx_aborted" },
+	{ "tx_bcasts" },
+	{ "rx_bcasts" },
+	{ "tx_mcasts" },
+	{ "rx_mcasts" },
+};
+
 static int check_if_running(struct net_device *dev)
 {
 	if (!netif_running(dev))
@@ -1624,6 +1665,42 @@ static void set_msglevel(struct net_device *dev, u32 val)
 	np->msg_enable = val;
 }
 
+static void get_strings(struct net_device *dev, u32 stringset,
+		u8 *data)
+{
+	if (stringset == ETH_SS_STATS)
+		memcpy(data, sundance_stats, sizeof(sundance_stats));
+}
+
+static int get_sset_count(struct net_device *dev, int sset)
+{
+	switch (sset) {
+	case ETH_SS_STATS:
+		return ARRAY_SIZE(sundance_stats);
+	default:
+		return -EOPNOTSUPP;
+	}
+}
+
+static void get_ethtool_stats(struct net_device *dev,
+		struct ethtool_stats *stats, u64 *data)
+{
+	struct netdev_private *np = netdev_priv(dev);
+	int i = 0;
+
+	get_stats(dev);
+	data[i++] = np->xstats.tx_multiple_collisions;
+	data[i++] = np->xstats.tx_single_collisions;
+	data[i++] = np->xstats.tx_late_collisions;
+	data[i++] = np->xstats.tx_deferred;
+	data[i++] = np->xstats.tx_deferred_excessive;
+	data[i++] = np->xstats.tx_aborted;
+	data[i++] = np->xstats.tx_bcasts;
+	data[i++] = np->xstats.rx_bcasts;
+	data[i++] = np->xstats.tx_mcasts;
+	data[i++] = np->xstats.rx_mcasts;
+}
+
 static const struct ethtool_ops ethtool_ops = {
 	.begin = check_if_running,
 	.get_drvinfo = get_drvinfo,
@@ -1633,6 +1710,9 @@ static const struct ethtool_ops ethtool_ops = {
 	.get_link = get_link,
 	.get_msglevel = get_msglevel,
 	.set_msglevel = set_msglevel,
+	.get_strings = get_strings,
+	.get_sset_count = get_sset_count,
+	.get_ethtool_stats = get_ethtool_stats,
 };
 
 static int netdev_ioctl(struct net_device *dev, struct ifreq *rq, int cmd)
-- 
1.7.0


^ permalink raw reply related	[flat|nested] 18+ messages in thread

* Re: [PATCH -next] sundance: Add initial ethtool stats support
  2010-10-13 10:56                   ` Denis Kirjanov
@ 2010-10-13 19:44                     ` Eric Dumazet
  0 siblings, 0 replies; 18+ messages in thread
From: Eric Dumazet @ 2010-10-13 19:44 UTC (permalink / raw)
  To: Denis Kirjanov; +Cc: netdev, David Miller, Ben Hutchings, Jeff Garzik

Le mercredi 13 octobre 2010 à 14:56 +0400, Denis Kirjanov a écrit :
> On 10/13/2010 02:36 PM, Eric Dumazet wrote:
> > Le mercredi 13 octobre 2010 à 14:28 +0400, Denis Kirjanov a écrit :
> > 
> >> +		u64 tx_defered;
> >> +		u64 tx_defered_excessive;
> > 
> PATCH -next v5] sundance: Add ethtool stats support
> 
> Add ethtool stats support.
> 
> Signed-off-by: Denis Kirjanov <dkirjanov@kernel.org>
> ---

Acked-by: Eric Dumazet <eric.dumazet@gmail.com>



^ permalink raw reply	[flat|nested] 18+ messages in thread

end of thread, other threads:[~2010-10-13 19:45 UTC | newest]

Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-10-09  9:53 [PATCH -next] sundance: Add initial ethtool stats support Denis Kirjanov
2010-10-09 12:17 ` [PATCH net-next] sundance: get_stats proper locking Eric Dumazet
2010-10-09 16:24   ` David Miller
2010-10-09 12:33 ` [PATCH -next] sundance: Add initial ethtool stats support Eric Dumazet
2010-10-09 13:27 ` Ben Hutchings
2010-10-09 14:40   ` Denis Kirjanov
2010-10-09 19:41   ` Denis Kirjanov
2010-10-09 21:48     ` Jeff Garzik
2010-10-12 18:51     ` David Miller
2010-10-12 20:36       ` Denis Kirjanov
2010-10-12 20:57         ` Joe Perches
2010-10-13  6:06           ` Denis Kirjanov
2010-10-13  6:33             ` Eric Dumazet
2010-10-13  7:02               ` Denis Kirjanov
2010-10-13 10:28               ` Denis Kirjanov
2010-10-13 10:36                 ` Eric Dumazet
2010-10-13 10:56                   ` Denis Kirjanov
2010-10-13 19:44                     ` Eric Dumazet

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.