Re: [PATCH] r8169: Add values missing in @get_stats64 from HW counters

[Corinna Vinschen]

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:vinsc...@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.  

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;


Thanks,
Corinna

pgplEX60t_D0q.pgp
Description: PGP signature