Re: [PATCH] 3c59x: support more ethtool_ops

Re: [PATCH] 3c59x: support more ethtool_ops

[Jeff Garzik]

Linux Kernel Mailing List wrote:
> +static void vortex_get_ethtool_stats(struct net_device *dev,
> +	struct ethtool_stats *stats, u64 *data)
> +{
> +	struct vortex_private *vp = netdev_priv(dev);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vp->lock, flags);
> +	update_stats(dev->base_addr, dev);
> +	spin_unlock_irqrestore(&vp->lock, flags);
> +
> +	data[0] = vp->stats.rx_packets;
> +	data[1] = vp->stats.tx_packets;
> +	data[2] = vp->stats.rx_bytes;
> +	data[3] = vp->stats.tx_bytes;
> +	data[4] = vp->stats.collisions;
> +	data[5] = vp->stats.tx_carrier_errors;
> +	data[6] = vp->stats.tx_heartbeat_errors;
> +	data[7] = vp->stats.tx_window_errors;
> +}

Everything in the patch is correct except for the above.

This is very wrong -- get_ethtool_stats() is for NIC-specific stats. 
The above stats are already available through the generic net stack.

	Jeff

Re: [PATCH] 3c59x: support more ethtool_ops

[Steffen Klassert]

On Tue, Jan 11, 2005 at 02:42:48PM -0500, Jeff Garzik wrote:
> Linux Kernel Mailing List wrote:
> >+static void vortex_get_ethtool_stats(struct net_device *dev,
> >+	struct ethtool_stats *stats, u64 *data)
> >+{
> >+	struct vortex_private *vp = netdev_priv(dev);
> >+	unsigned long flags;
> >+
> >+	spin_lock_irqsave(&vp->lock, flags);
> >+	update_stats(dev->base_addr, dev);
> >+	spin_unlock_irqrestore(&vp->lock, flags);
> >+
> >+	data[0] = vp->stats.rx_packets;
> >+	data[1] = vp->stats.tx_packets;
> >+	data[2] = vp->stats.rx_bytes;
> >+	data[3] = vp->stats.tx_bytes;
> >+	data[4] = vp->stats.collisions;
> >+	data[5] = vp->stats.tx_carrier_errors;
> >+	data[6] = vp->stats.tx_heartbeat_errors;
> >+	data[7] = vp->stats.tx_window_errors;
> >+}
> 
> Everything in the patch is correct except for the above.
> 
> This is very wrong -- get_ethtool_stats() is for NIC-specific stats. 
> The above stats are already available through the generic net stack.
> 

When I did this I took the e100 driver as an example, here get_ethtool_stats()
provides the generic and the NIC specific stats. Thus I added all in vp->stats
counted stats (in this case just the generic) to get_ethtool_stats().   

But anyway, I will rework this part. 
What is the expected behavior of get_ethtool_stats()?
Provide just the NIC specific stats or all stats as the e100 driver does it?

	Steffen

Re: [PATCH] 3c59x: support more ethtool_ops

[Jeff Garzik]

Steffen Klassert wrote:
> But anyway, I will rework this part. 
> What is the expected behavior of get_ethtool_stats()?
> Provide just the NIC specific stats or all stats as the e100 driver does it?


Just the NIC-specific stats.

	Jeff