diff mbox

r8169: Add values missing in @get_stats64 from HW counters

Message ID 20150819192414.GC30904@calimero.vinschen.de
State RFC, archived
Delegated to: David Miller
Headers show

Commit Message

Corinna Vinschen Aug. 19, 2015, 7:24 p.m. UTC 
On Aug 19 16:24, Corinna Vinschen wrote:
> On Aug 19 15:07, Corinna Vinschen wrote:
> > On Aug 19 09:31, Hayes Wang wrote:
> > > Corinna Vinschen [mailto:vinschen@redhat.com]
> > > > Sent: Wednesday, August 19, 2015 5:13 PM
> > > [...]
> > > > > It could be cleared by setting bit 0, such as rtl_tally_reset() of r8152.
> > > > 
> > > > Is it safe to assume that this is implemented in all NICs covered by r8169? 
> > > 
> > > It is supported from RTL8111C. That is, RTL_GIGA_MAC_VER_19 and later.
> > 
> > Thanks.  In that case I would prefer the same generic method for all
> > chip versions, so I'd opt for storing the offset values at rtl_open
> > time as my patch is doing right now.  Is that acceptable?
> > 
> > If so, wouldn't it make even more sense to use the hardware collected
> > information in @get_stats64 throughout, except for the numbers collected
> > *only* in software?  
> > 
> > I would be willing to propose a matching patch.
> 
> It just occured to me that the combination of resetting the counters on
> post-RTL_GIGA_MAC_VER_19 chips plus offset handling would be quite
> nice, because it would reset also the small 16 and 32 bit counters.
> 
> So I'd like to propose a patch which combines both techniques, if that's
> an acceptable way to go forward.
> 
> Btw., does setting the reset bit in CounterAddrLow work the same way as
> setting the CounterDump flag?  I.e, does the driver have to wait for the
> hardware to set the bit to 0 again to be sure the reset is finished?

Here's a preliminary implementation of the counter reset code.  It's
based on my previous patch.  It calls the new rtl8169_reset_counters
prior to rtl8169_update_counters from rtl_open.

I tested the patch successfully on a RTL8111f NIC (RTL_GIGA_MAC_VER_35).

This isn't an official patch submission, I'm just sending this for
further discussion.  I'll make a full patch submission if the code is
acceptable.  



Thanks,
Corinna

Comments

Hayes Wang Aug. 20, 2015, 2:43 a.m. UTC | #1 
Corinna Vinschen [mailto:vinschen@redhat.com]
> Sent: Thursday, August 20, 2015 3:24 AM

[...]
> +	/*

> +	 * Versions prior to RTL_GIGA_MAC_VER_19 don't support resetting the

> +	 * tally counters.

> +	 */

> +	if (tp->mac_version >= RTL_GIGA_MAC_VER_19) {

> +		RTL_W32(CounterAddrHigh, 0);

> +		RTL_W32(CounterAddrLow, CounterReset);


I check these with our engineers, and they say the bit 6 ~ 63 should be the
valid 64 byte alignment memory address. Although you don’t want to dump
the counters, the hw may also clear the data in the memory which is indicated
by bit 6 ~ 63, when you reset the counters.

Best Regards,
Hayes
Corinna Vinschen Aug. 20, 2015, 9:41 a.m. UTC | #2 
On Aug 20 02:43, Hayes Wang wrote:
> Corinna Vinschen [mailto:vinschen@redhat.com]
> > Sent: Thursday, August 20, 2015 3:24 AM
> [...]
> > +	/*
> > +	 * Versions prior to RTL_GIGA_MAC_VER_19 don't support resetting the
> > +	 * tally counters.
> > +	 */
> > +	if (tp->mac_version >= RTL_GIGA_MAC_VER_19) {
> > +		RTL_W32(CounterAddrHigh, 0);
> > +		RTL_W32(CounterAddrLow, CounterReset);
> 
> I check these with our engineers, and they say the bit 6 ~ 63 should be the
> valid 64 byte alignment memory address. Although you don’t want to dump
> the counters, the hw may also clear the data in the memory which is indicated
> by bit 6 ~ 63, when you reset the counters.

Ok, that's easy enough to implement.  What about CmdRxEnb?  Are there
chips which need this flag set to perform the counter reset?


Thanks,
Corinna
diff mbox

Patch

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index e7c7955..204f7e7 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -637,6 +637,9 @@  enum rtl_register_content {
 	/* _TBICSRBit */
 	TBILinkOK	= 0x02000000,
 
+	/* ResetCounterCommand */
+	CounterReset	= 0x1,
+
 	/* DumpCounterCommand */
 	CounterDump	= 0x8,
 
@@ -2188,6 +2191,31 @@  static int rtl8169_get_sset_count(struct net_device *dev, int sset)
 	}
 }
 
+DECLARE_RTL_COND(rtl_reset_counters_cond)
+{
+	void __iomem *ioaddr = tp->mmio_addr;
+
+	return RTL_R32(CounterAddrLow) & CounterReset;
+}
+
+static void rtl8169_reset_counters(struct net_device *dev)
+{
+	struct rtl8169_private *tp = netdev_priv(dev);
+	void __iomem *ioaddr = tp->mmio_addr;
+
+	/*
+	 * Versions prior to RTL_GIGA_MAC_VER_19 don't support resetting the
+	 * tally counters.
+	 */
+	if (tp->mac_version >= RTL_GIGA_MAC_VER_19) {
+		RTL_W32(CounterAddrHigh, 0);
+		RTL_W32(CounterAddrLow, CounterReset);
+		if (!rtl_udelay_loop_wait_low(tp, &rtl_reset_counters_cond,
+					      10, 1000))
+			netif_warn(tp, hw, dev, "counter reset failed\n");
+	}
+}
+
 DECLARE_RTL_COND(rtl_counters_cond)
 {
 	void __iomem *ioaddr = tp->mmio_addr;
@@ -2234,11 +2262,10 @@  static void rtl8169_init_tc_counter_offset(struct net_device *dev)
 	struct rtl8169_private *tp = netdev_priv(dev);
 
 	/*
-	 * rtl8169_init_tc_counter_offset is called from rtl_open.  The tally
-	 * counters are only reset by a power cycle, while the counter values
-	 * collected by the driver are reset at every driver unload/load cycle.
-	 * There's also no (documented?) way to reset the tally counters
-	 * programatically.
+	 * rtl8169_init_tc_counter_offset is called from rtl_open.  On chip
+	 * versions prior to RTL_GIGA_MAC_VER_19 the tally counters are only
+	 * reset by a power cycle, while the counter values collected by the
+	 * driver are reset at every driver unload/load cycle.
 	 *
 	 * To make sure the HW values returned by @get_stats64 match the SW
 	 * values, we collect the initial values at first open(*) and use them
@@ -2252,6 +2279,8 @@  static void rtl8169_init_tc_counter_offset(struct net_device *dev)
 	if (tp->tc_offset.inited)
 		return;
 
+	rtl8169_reset_counters(dev);
+
 	rtl8169_update_counters(dev);
 
 	tp->tc_offset.tx_errors = tp->counters.tx_errors;