netdev - Re: [PATCH] r8169: Add values missing in @get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Wed, 19 Aug 2015 21:24:14 +0200
From:	Corinna Vinschen <vinschen@...hat.com>
To:	"netdev@...r.kernel.org" <netdev@...r.kernel.org>
Cc:	Hayes Wang <hayeswang@...ltek.com>,
	Francois Romieu <romieu@...zoreil.com>,
	nic_swsd <nic_swsd@...ltek.com>,
	Ivan Vecera <ivecera@...hat.com>,
	David Miller <davem@...emloft.net>
Subject: Re: [PATCH] r8169: Add values missing in @get_stats64 from HW
 counters

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@...hat.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

Content of type "application/pgp-signature" skipped