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

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

[Corinna Vinschen]

The r8169 driver collects statistical information returned by
@get_stats64 by counting them in the driver itself, even though many
(but not all) of the values are already collected by tally counters
(TCs) in the NIC.  Some of these TC values are not returned by
@get_stats64.  Especially the received multicast packages are missing
from /proc/net/dev.

Rectify this by fetching the TCs and returning them from
rtl8169_get_stats64.

The counters collected in the driver obviously disappear as soon as the
driver is unloaded so after a driver is loaded the counters always start
at 0.  The TCs are only reset by a power cycle and there's no known way
to reset them programatically.  Without further considerations the values
collected by the driver would not match up against the TC values.

Therefore introduce an addition to the rtl8169_private struct and a
function rtl8169_init_tc_counter_offset to store the TCs at first
rtl_open.  Use these values as offsets in rtl8169_get_stats64.

Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---
 drivers/net/ethernet/realtek/r8169.c | 62 ++++++++++++++++++++++++++++++++++++
 1 file changed, 62 insertions(+)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index f790f61..e7c7955 100644
--- a/drivers/net/ethernet/realtek/r8169.c
+++ b/drivers/net/ethernet/realtek/r8169.c
@@ -747,6 +747,14 @@ struct rtl8169_counters {
 	__le16	tx_underun;
 };
 
+struct rtl8169_tc_offsets {
+	bool	inited;
+	__le64	tx_errors;
+	__le32	tx_multi_collision;
+	__le32	rx_multicast;
+	__le16	tx_aborted;
+};
+
 enum rtl_flag {
 	RTL_FLAG_TASK_ENABLED,
 	RTL_FLAG_TASK_SLOW_PENDING,
@@ -824,6 +832,7 @@ struct rtl8169_private {
 
 	struct mii_if_info mii;
 	struct rtl8169_counters counters;
+	struct rtl8169_tc_offsets tc_offset;
 	u32 saved_wolopts;
 	u32 opts1_mask;
 
@@ -2220,6 +2229,38 @@ static void rtl8169_update_counters(struct net_device *dev)
 	dma_free_coherent(d, sizeof(*counters), counters, paddr);
 }
 
+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.
+	 *
+	 * 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
+	 * as offsets to normalize the values returned by @get_stats64.
+	 *
+	 * (*) We can't call rtl8169_init_tc_counter_offset from rtl_init_one
+	 * for the reason stated in rtl8169_update_counters; CmdRxEnb is only
+	 * set at open time by rtl_hw_start.
+	 */
+
+	if (tp->tc_offset.inited)
+		return;
+
+	rtl8169_update_counters(dev);
+
+	tp->tc_offset.tx_errors = tp->counters.tx_errors;
+	tp->tc_offset.tx_multi_collision = tp->counters.tx_multi_collision;
+	tp->tc_offset.rx_multicast = tp->counters.rx_multicast;
+	tp->tc_offset.tx_aborted = tp->counters.tx_aborted;
+	tp->tc_offset.inited = true;
+}
+
 static void rtl8169_get_ethtool_stats(struct net_device *dev,
 				      struct ethtool_stats *stats, u64 *data)
 {
@@ -7631,6 +7672,8 @@ static int rtl_open(struct net_device *dev)
 
 	rtl_hw_start(dev);
 
+	rtl8169_init_tc_counter_offset(dev);
+
 	netif_start_queue(dev);
 
 	rtl_unlock_work(tp);
@@ -7689,6 +7732,25 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
 	stats->rx_fifo_errors	= dev->stats.rx_fifo_errors;
 	stats->rx_missed_errors = dev->stats.rx_missed_errors;
 
+	/*
+	 * Fetch additonal counter values missing in stats collected by driver
+	 * from tally counters.
+	 */
+	rtl8169_update_counters(dev);
+
+	/*
+	 * Subtract values fetched during initalization.
+	 * See rtl8169_init_tc_counter_offset for a description why we do that.
+	 */
+	stats->tx_errors = le64_to_cpu(tp->counters.tx_errors) -
+		le64_to_cpu(tp->tc_offset.tx_errors);
+	stats->collisions = le32_to_cpu(tp->counters.tx_multi_collision) -
+		le32_to_cpu(tp->tc_offset.tx_multi_collision);
+	stats->multicast = le32_to_cpu(tp->counters.rx_multicast) -
+		le32_to_cpu(tp->tc_offset.rx_multicast);
+	stats->tx_aborted_errors = le16_to_cpu(tp->counters.tx_aborted) -
+		le16_to_cpu(tp->tc_offset.tx_aborted);
+
 	return stats;
 }
 
-- 
2.1.0

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

[Francois Romieu]

Corinna Vinschen <vinschen@redhat.com> :
> The r8169 driver collects statistical information returned by
> @get_stats64 by counting them in the driver itself, even though many
> (but not all) of the values are already collected by tally counters
> (TCs) in the NIC.  Some of these TC values are not returned by
> @get_stats64.  Especially the received multicast packages are missing
> from /proc/net/dev.
> Rectify this by fetching the TCs and returning them from
> rtl8169_get_stats64.

It is not exactly a some / many / not all / confused [*] picture
- get_stats64 provides software managed values
- ethtool provides hardware only values

[*] ok, rx_missed is kinda special.

> The counters collected in the driver obviously disappear as soon as the
> driver is unloaded so after a driver is loaded the counters always start
> at 0.  The TCs are only reset by a power cycle and there's no known way
> to reset them programatically. Without further considerations the values
> collected by the driver would not match up against the TC values.

I'd rather:
1. keep software and hardware stats separated
2. push to userspace the responsibility to offset device specific hardware
   values jumps

My 0.02 €.

-- 
Ueimor

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

[David Miller]

From: Francois Romieu <romieu@fr.zoreil.com>
Date: Tue, 18 Aug 2015 23:40:17 +0200

> Corinna Vinschen <vinschen@redhat.com> :
>> The r8169 driver collects statistical information returned by
>> @get_stats64 by counting them in the driver itself, even though many
>> (but not all) of the values are already collected by tally counters
>> (TCs) in the NIC.  Some of these TC values are not returned by
>> @get_stats64.  Especially the received multicast packages are missing
>> from /proc/net/dev.
>> Rectify this by fetching the TCs and returning them from
>> rtl8169_get_stats64.
> 
> It is not exactly a some / many / not all / confused [*] picture
> - get_stats64 provides software managed values
> - ethtool provides hardware only values
> 
> [*] ok, rx_missed is kinda special.
> 
>> The counters collected in the driver obviously disappear as soon as the
>> driver is unloaded so after a driver is loaded the counters always start
>> at 0.  The TCs are only reset by a power cycle and there's no known way
>> to reset them programatically. Without further considerations the values
>> collected by the driver would not match up against the TC values.
> 
> I'd rather:
> 1. keep software and hardware stats separated
> 2. push to userspace the responsibility to offset device specific hardware
>    values jumps
> 
> My 0.02 €.

Any value that is provided by ->get_stats64() should be provided by whatever
mechanism is available and determined to be "prudent" for the device in
question.  If the device is not tracking the event, this means pure software
counters.

If the device tracks the event, you should attempt to use the hardware
facility to provide the ->get_stats64() values.

If the hardware provides counters not starting at zero on driver load,
one option to handle that is to load the initial values of all of
those counters and calculate diffs at ->get_stats64() time.

Otherwise, if you don't want to calculate diffs, you must use software
computed counters.

Ethtool stats are for values provided by the hardware which are outside of
the collection defined by ->get_stats64().  Some drivers also use it for
providing per-RX-queue and per-TX-queue statistics.

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

[Hayes Wang]

[...]
> > The TCs are only reset by a power cycle and there's no known way
> > to reset them programatically.

It could be cleared by setting bit 0, such as rtl_tally_reset() of r8152.

Best Regards,
Hayes

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2429 bytes --]

On Aug 18 19:05, David Miller wrote:
> From: Francois Romieu <romieu@fr.zoreil.com>
> Date: Tue, 18 Aug 2015 23:40:17 +0200
> 
> > Corinna Vinschen <vinschen@redhat.com> :
> >> The r8169 driver collects statistical information returned by
> >> @get_stats64 by counting them in the driver itself, even though many
> >> (but not all) of the values are already collected by tally counters
> >> (TCs) in the NIC.  Some of these TC values are not returned by
> >> @get_stats64.  Especially the received multicast packages are missing
> >> from /proc/net/dev.
> >> Rectify this by fetching the TCs and returning them from
> >> rtl8169_get_stats64.
> > 
> > It is not exactly a some / many / not all / confused [*] picture
> > - get_stats64 provides software managed values
> > - ethtool provides hardware only values
> > 
> > [*] ok, rx_missed is kinda special.
> > 
> >> The counters collected in the driver obviously disappear as soon as the
> >> driver is unloaded so after a driver is loaded the counters always start
> >> at 0.  The TCs are only reset by a power cycle and there's no known way
> >> to reset them programatically. Without further considerations the values
> >> collected by the driver would not match up against the TC values.
> > 
> > I'd rather:
> > 1. keep software and hardware stats separated
> > 2. push to userspace the responsibility to offset device specific hardware
> >    values jumps
> > 
> > My 0.02 €.
> 
> Any value that is provided by ->get_stats64() should be provided by whatever
> mechanism is available and determined to be "prudent" for the device in
> question.  If the device is not tracking the event, this means pure software
> counters.
> 
> If the device tracks the event, you should attempt to use the hardware
> facility to provide the ->get_stats64() values.
> 
> If the hardware provides counters not starting at zero on driver load,
> one option to handle that is to load the initial values of all of
> those counters and calculate diffs at ->get_stats64() time.

That's what my code is doing, just for a limited set of values.

I was already wondering why some values available in the hardware tally
counters are additionally counted in software.  I was planning to
propose to use the hardware counters in @get_stats64 in the first place
and only count values in the driver which are not provided by the
hardware


Corinna

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 455 bytes --]

On Aug 19 02:51, Hayes Wang wrote:
> [...]
> > > The TCs are only reset by a power cycle and there's no known way
> > > to reset them programatically.
> 
> It could be cleared by setting bit 0, such as rtl_tally_reset() of r8152.

Thanks for this hint.  I'll give it a try.  Is it safe to assume that
this is implemented in all NICs covered by r8169?  Otherwise it's more
safe to use offset values as in my patch, I think.


Thanks,
Corinna

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Hayes Wang]

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.

Best Regards,
Hayes

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 830 bytes --]

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.


Thanks,
Corinna

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1462 bytes --]

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?


Thanks in advance,
Corinna

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 4424 bytes --]

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.  

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Hayes Wang]

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

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

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 844 bytes --]

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

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

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

[Hayes Wang]

Corinna Vinschen [mailto:vinschen@redhat.com]
> Sent: Thursday, August 20, 2015 5:42 PM
[...]
> What about CmdRxEnb?  Are there
> chips which need this flag set to perform the counter reset?

No. CmdRxEnb is used to enable/disable the rx, and you could
reset the counters without changing CmdRxEnb.

Best Regards,
Hayes

[PATCH v2 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Corinna Vinschen]

The r8169 driver collects statistical information returned by
@get_stats64 by counting them in the driver itself, even though many
(but not all) of the values are already collected by tally counters
(TCs) in the NIC.  Some of these TC values are not returned by
@get_stats64.  Especially the received multicast packages are missing
from /proc/net/dev.

Rectify this by fetching the TCs and returning them from
rtl8169_get_stats64.

The counters collected in the driver obviously disappear as soon as the
driver is unloaded so after a driver is loaded the counters always start
at 0. The TCs on the other hand are only reset by a power cycle.  Without
further considerations the values collected by the driver would not match
up against the TC values.

This patch introduces a new function rtl8169_reset_counters which
resets the TCs.

Unfortunately chip versions prior to RTL_GIGA_MAC_VER_19 don't allow
to reset the TCs programatically.  Therefore introduce an addition to
the rtl8169_private struct and a function rtl8169_init_counter_offsets
to store the TCs at first rtl_open.  Use these values as offsets in
rtl8169_get_stats64.

Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---
 drivers/net/ethernet/realtek/r8169.c | 107 +++++++++++++++++++++++++++++++++++
 1 file changed, 107 insertions(+)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index f790f61..f26a48d 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,
 
@@ -747,6 +750,14 @@ struct rtl8169_counters {
 	__le16	tx_underun;
 };
 
+struct rtl8169_tc_offsets {
+	bool	inited;
+	__le64	tx_errors;
+	__le32	tx_multi_collision;
+	__le32	rx_multicast;
+	__le16	tx_aborted;
+};
+
 enum rtl_flag {
 	RTL_FLAG_TASK_ENABLED,
 	RTL_FLAG_TASK_SLOW_PENDING,
@@ -824,6 +835,7 @@ struct rtl8169_private {
 
 	struct mii_if_info mii;
 	struct rtl8169_counters counters;
+	struct rtl8169_tc_offsets tc_offset;
 	u32 saved_wolopts;
 	u32 opts1_mask;
 
@@ -2179,6 +2191,47 @@ 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;
+	struct device *d = &tp->pci_dev->dev;
+	struct rtl8169_counters *counters;
+	dma_addr_t paddr;
+	u32 cmd;
+
+	/*
+	 * Versions prior to RTL_GIGA_MAC_VER_19 don't support resetting the
+	 * tally counters.
+	 */
+	if (tp->mac_version < RTL_GIGA_MAC_VER_19)
+		return;
+
+	counters = dma_alloc_coherent(d, sizeof(*counters), &paddr, GFP_KERNEL);
+	if (!counters)
+		return;
+
+	RTL_W32(CounterAddrHigh, (u64)paddr >> 32);
+	cmd = (u64)paddr & DMA_BIT_MASK(32);
+	RTL_W32(CounterAddrLow, cmd);
+	RTL_W32(CounterAddrLow, cmd | CounterReset);
+
+	if (!rtl_udelay_loop_wait_low(tp, &rtl_reset_counters_cond, 10, 1000))
+		netif_warn(tp, hw, dev, "counter reset failed\n");
+
+	RTL_W32(CounterAddrLow, 0);
+	RTL_W32(CounterAddrHigh, 0);
+
+	dma_free_coherent(d, sizeof(*counters), counters, paddr);
+}
+
 DECLARE_RTL_COND(rtl_counters_cond)
 {
 	void __iomem *ioaddr = tp->mmio_addr;
@@ -2220,6 +2273,39 @@ static void rtl8169_update_counters(struct net_device *dev)
 	dma_free_coherent(d, sizeof(*counters), counters, paddr);
 }
 
+static void rtl8169_init_counter_offsets(struct net_device *dev)
+{
+	struct rtl8169_private *tp = netdev_priv(dev);
+
+	/*
+	 * rtl8169_init_counter_offsets 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
+	 * as offsets to normalize the values returned by @get_stats64.
+	 *
+	 * (*) We can't call rtl8169_init_counter_offsets from rtl_init_one
+	 * for the reason stated in rtl8169_update_counters; CmdRxEnb is only
+	 * set at open time by rtl_hw_start.
+	 */
+
+	if (tp->tc_offset.inited)
+		return;
+
+	rtl8169_reset_counters(dev);
+
+	rtl8169_update_counters(dev);
+
+	tp->tc_offset.tx_errors = tp->counters.tx_errors;
+	tp->tc_offset.tx_multi_collision = tp->counters.tx_multi_collision;
+	tp->tc_offset.rx_multicast = tp->counters.rx_multicast;
+	tp->tc_offset.tx_aborted = tp->counters.tx_aborted;
+	tp->tc_offset.inited = true;
+}
+
 static void rtl8169_get_ethtool_stats(struct net_device *dev,
 				      struct ethtool_stats *stats, u64 *data)
 {
@@ -7631,6 +7717,8 @@ static int rtl_open(struct net_device *dev)
 
 	rtl_hw_start(dev);
 
+	rtl8169_init_counter_offsets(dev);
+
 	netif_start_queue(dev);
 
 	rtl_unlock_work(tp);
@@ -7689,6 +7777,25 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
 	stats->rx_fifo_errors	= dev->stats.rx_fifo_errors;
 	stats->rx_missed_errors = dev->stats.rx_missed_errors;
 
+	/*
+	 * Fetch additonal counter values missing in stats collected by driver
+	 * from tally counters.
+	 */
+	rtl8169_update_counters(dev);
+
+	/*
+	 * Subtract values fetched during initalization.
+	 * See rtl8169_init_counter_offsets for a description why we do that.
+	 */
+	stats->tx_errors = le64_to_cpu(tp->counters.tx_errors) -
+		le64_to_cpu(tp->tc_offset.tx_errors);
+	stats->collisions = le32_to_cpu(tp->counters.tx_multi_collision) -
+		le32_to_cpu(tp->tc_offset.tx_multi_collision);
+	stats->multicast = le32_to_cpu(tp->counters.rx_multicast) -
+		le32_to_cpu(tp->tc_offset.rx_multicast);
+	stats->tx_aborted_errors = le16_to_cpu(tp->counters.tx_aborted) -
+		le16_to_cpu(tp->tc_offset.tx_aborted);
+
 	return stats;
 }
 
-- 
2.1.0

Re: [PATCH v2 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 7244 bytes --]

Sorry, I forgot to mention that I tested this patch on three different
chip versions, RTL_GIGA_MAC_VER_23, RTL_GIGA_MAC_VER_33 and
RTL_GIGA_MAC_VER_35.  I couldn't test on pre-RTL_GIGA_MAC_VER_19, but
the offset handling without counter reset already worked as expected on
later chip versions, so I'm pretty confident that older chip versions
should work accordingly.

On Aug 21 12:09, Corinna Vinschen wrote:
> The r8169 driver collects statistical information returned by
> @get_stats64 by counting them in the driver itself, even though many
> (but not all) of the values are already collected by tally counters
> (TCs) in the NIC.  Some of these TC values are not returned by
> @get_stats64.  Especially the received multicast packages are missing
> from /proc/net/dev.
> 
> Rectify this by fetching the TCs and returning them from
> rtl8169_get_stats64.
> 
> The counters collected in the driver obviously disappear as soon as the
> driver is unloaded so after a driver is loaded the counters always start
> at 0. The TCs on the other hand are only reset by a power cycle.  Without
> further considerations the values collected by the driver would not match
> up against the TC values.
> 
> This patch introduces a new function rtl8169_reset_counters which
> resets the TCs.
> 
> Unfortunately chip versions prior to RTL_GIGA_MAC_VER_19 don't allow
> to reset the TCs programatically.  Therefore introduce an addition to
> the rtl8169_private struct and a function rtl8169_init_counter_offsets
> to store the TCs at first rtl_open.  Use these values as offsets in
> rtl8169_get_stats64.
> 
> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
> ---
>  drivers/net/ethernet/realtek/r8169.c | 107 +++++++++++++++++++++++++++++++++++
>  1 file changed, 107 insertions(+)
> 
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index f790f61..f26a48d 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,
>  
> @@ -747,6 +750,14 @@ struct rtl8169_counters {
>  	__le16	tx_underun;
>  };
>  
> +struct rtl8169_tc_offsets {
> +	bool	inited;
> +	__le64	tx_errors;
> +	__le32	tx_multi_collision;
> +	__le32	rx_multicast;
> +	__le16	tx_aborted;
> +};
> +
>  enum rtl_flag {
>  	RTL_FLAG_TASK_ENABLED,
>  	RTL_FLAG_TASK_SLOW_PENDING,
> @@ -824,6 +835,7 @@ struct rtl8169_private {
>  
>  	struct mii_if_info mii;
>  	struct rtl8169_counters counters;
> +	struct rtl8169_tc_offsets tc_offset;
>  	u32 saved_wolopts;
>  	u32 opts1_mask;
>  
> @@ -2179,6 +2191,47 @@ 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;
> +	struct device *d = &tp->pci_dev->dev;
> +	struct rtl8169_counters *counters;
> +	dma_addr_t paddr;
> +	u32 cmd;
> +
> +	/*
> +	 * Versions prior to RTL_GIGA_MAC_VER_19 don't support resetting the
> +	 * tally counters.
> +	 */
> +	if (tp->mac_version < RTL_GIGA_MAC_VER_19)
> +		return;
> +
> +	counters = dma_alloc_coherent(d, sizeof(*counters), &paddr, GFP_KERNEL);
> +	if (!counters)
> +		return;
> +
> +	RTL_W32(CounterAddrHigh, (u64)paddr >> 32);
> +	cmd = (u64)paddr & DMA_BIT_MASK(32);
> +	RTL_W32(CounterAddrLow, cmd);
> +	RTL_W32(CounterAddrLow, cmd | CounterReset);
> +
> +	if (!rtl_udelay_loop_wait_low(tp, &rtl_reset_counters_cond, 10, 1000))
> +		netif_warn(tp, hw, dev, "counter reset failed\n");
> +
> +	RTL_W32(CounterAddrLow, 0);
> +	RTL_W32(CounterAddrHigh, 0);
> +
> +	dma_free_coherent(d, sizeof(*counters), counters, paddr);
> +}
> +
>  DECLARE_RTL_COND(rtl_counters_cond)
>  {
>  	void __iomem *ioaddr = tp->mmio_addr;
> @@ -2220,6 +2273,39 @@ static void rtl8169_update_counters(struct net_device *dev)
>  	dma_free_coherent(d, sizeof(*counters), counters, paddr);
>  }
>  
> +static void rtl8169_init_counter_offsets(struct net_device *dev)
> +{
> +	struct rtl8169_private *tp = netdev_priv(dev);
> +
> +	/*
> +	 * rtl8169_init_counter_offsets 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
> +	 * as offsets to normalize the values returned by @get_stats64.
> +	 *
> +	 * (*) We can't call rtl8169_init_counter_offsets from rtl_init_one
> +	 * for the reason stated in rtl8169_update_counters; CmdRxEnb is only
> +	 * set at open time by rtl_hw_start.
> +	 */
> +
> +	if (tp->tc_offset.inited)
> +		return;
> +
> +	rtl8169_reset_counters(dev);
> +
> +	rtl8169_update_counters(dev);
> +
> +	tp->tc_offset.tx_errors = tp->counters.tx_errors;
> +	tp->tc_offset.tx_multi_collision = tp->counters.tx_multi_collision;
> +	tp->tc_offset.rx_multicast = tp->counters.rx_multicast;
> +	tp->tc_offset.tx_aborted = tp->counters.tx_aborted;
> +	tp->tc_offset.inited = true;
> +}
> +
>  static void rtl8169_get_ethtool_stats(struct net_device *dev,
>  				      struct ethtool_stats *stats, u64 *data)
>  {
> @@ -7631,6 +7717,8 @@ static int rtl_open(struct net_device *dev)
>  
>  	rtl_hw_start(dev);
>  
> +	rtl8169_init_counter_offsets(dev);
> +
>  	netif_start_queue(dev);
>  
>  	rtl_unlock_work(tp);
> @@ -7689,6 +7777,25 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
>  	stats->rx_fifo_errors	= dev->stats.rx_fifo_errors;
>  	stats->rx_missed_errors = dev->stats.rx_missed_errors;
>  
> +	/*
> +	 * Fetch additonal counter values missing in stats collected by driver
> +	 * from tally counters.
> +	 */
> +	rtl8169_update_counters(dev);
> +
> +	/*
> +	 * Subtract values fetched during initalization.
> +	 * See rtl8169_init_counter_offsets for a description why we do that.
> +	 */
> +	stats->tx_errors = le64_to_cpu(tp->counters.tx_errors) -
> +		le64_to_cpu(tp->tc_offset.tx_errors);
> +	stats->collisions = le32_to_cpu(tp->counters.tx_multi_collision) -
> +		le32_to_cpu(tp->tc_offset.tx_multi_collision);
> +	stats->multicast = le32_to_cpu(tp->counters.rx_multicast) -
> +		le32_to_cpu(tp->tc_offset.rx_multicast);
> +	stats->tx_aborted_errors = le16_to_cpu(tp->counters.tx_aborted) -
> +		le16_to_cpu(tp->tc_offset.tx_aborted);
> +
>  	return stats;
>  }
>  
> -- 
> 2.1.0
> 
> --
> To unsubscribe from this list: send the line "unsubscribe netdev" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Francois Romieu]

Corinna Vinschen <vinschen@redhat.com> :
[...]
> diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> index f790f61..f26a48d 100644
> --- a/drivers/net/ethernet/realtek/r8169.c
> +++ b/drivers/net/ethernet/realtek/r8169.c
[...]
> @@ -2179,6 +2191,47 @@ 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)
> +{

rtl8169_reset_counters duplicates most of rtl8169_update_counters. Please
factor out the dma_alloc + parametrized CounterAddrLow write + cleanup.

[...]
> @@ -2220,6 +2273,39 @@ static void rtl8169_update_counters(struct net_device *dev)
>  	dma_free_coherent(d, sizeof(*counters), counters, paddr);
>  }
>  
> +static void rtl8169_init_counter_offsets(struct net_device *dev)
> +{
> +	struct rtl8169_private *tp = netdev_priv(dev);
> +
> +	/*
> +	 * rtl8169_init_counter_offsets 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
> +	 * as offsets to normalize the values returned by @get_stats64.
> +	 *
> +	 * (*) We can't call rtl8169_init_counter_offsets from rtl_init_one
> +	 * for the reason stated in rtl8169_update_counters; CmdRxEnb is only
> +	 * set at open time by rtl_hw_start.
> +	 */
> +
> +	if (tp->tc_offset.inited)
> +		return;
> +
> +	rtl8169_reset_counters(dev);
> +
> +	rtl8169_update_counters(dev);


The code should propagate failure when both rtl8169_reset_counters and
rtl8169_update_counters fail.

--
Ueimor

Re: [PATCH v2 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1393 bytes --]

On Aug 21 21:39, Francois Romieu wrote:
> Corinna Vinschen <vinschen@redhat.com> :
> [...]
> > diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
> > index f790f61..f26a48d 100644
> > --- a/drivers/net/ethernet/realtek/r8169.c
> > +++ b/drivers/net/ethernet/realtek/r8169.c
> [...]
> > @@ -2179,6 +2191,47 @@ 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)
> > +{
> 
> rtl8169_reset_counters duplicates most of rtl8169_update_counters. Please
> factor out the dma_alloc + parametrized CounterAddrLow write + cleanup.

Ok, will do.

> > +	rtl8169_reset_counters(dev);
> > +
> > +	rtl8169_update_counters(dev);
> 
> 
> The code should propagate failure when both rtl8169_reset_counters and
> rtl8169_update_counters fail.

This one I don't understand.  Neither failing to reset the counters nor
failing to update the counters is fatal for the driver.  So far the
(unchanged) rtl8169_update_counters doesn't even print a log message,
while a failing reset in rtl8169_reset_counters now does.

Why is that not sufficent?


Thanks,
Corinna

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Francois Romieu]

Corinna Vinschen <vinschen@redhat.com> :
> On Aug 21 21:39, Francois Romieu wrote:
[...]
> > The code should propagate failure when both rtl8169_reset_counters and
> > rtl8169_update_counters fail.
> 
> This one I don't understand.  Neither failing to reset the counters nor
> failing to update the counters is fatal for the driver.  So far the
> (unchanged) rtl8169_update_counters doesn't even print a log message,

I wouldn't overestimate the value of log messages vs real status return.
Users can be quite unhappy with default settings that spam their logs
(it isn't a problem in open(), it's marginaly murphy plausible from
a periodic get_stats context).

The driver can't propagate errors from the current get_stats context
where rtl8169_update_counters is used. However it can be done in
open().

> while a failing reset in rtl8169_reset_counters now does.
> 
> Why is that not sufficent?

Because of the same reason(s) why this patch wants to improve things.

It isn't a showstopper.

-- 
Ueimor

Re: [PATCH v2 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2293 bytes --]

On Aug 22 01:59, Francois Romieu wrote:
> Corinna Vinschen <vinschen@redhat.com> :
> > On Aug 21 21:39, Francois Romieu wrote:
> [...]
> > > The code should propagate failure when both rtl8169_reset_counters and
> > > rtl8169_update_counters fail.
> > 
> > This one I don't understand.  Neither failing to reset the counters nor
> > failing to update the counters is fatal for the driver.  So far the
> > (unchanged) rtl8169_update_counters doesn't even print a log message,
> 
> I wouldn't overestimate the value of log messages vs real status return.
> Users can be quite unhappy with default settings that spam their logs
> (it isn't a problem in open(), it's marginaly murphy plausible from
> a periodic get_stats context).

That won't happen with the current patch because only
rtl8169_reset_counters would print a log message, it's only called from
open, and open occurs rather seldom.  Atop of that the code only tries
to reset counters on HW supporting it, and only if resetting on the HW
fails, there will be a log message at all.  There's no reasonable chance
that failing to reset the counters will lead to log flooding.

> The driver can't propagate errors from the current get_stats context
> where rtl8169_update_counters is used. However it can be done in
> open().
> 
> > while a failing reset in rtl8169_reset_counters now does.
> > 
> > Why is that not sufficent?
> 
> Because of the same reason(s) why this patch wants to improve things.
> 
> It isn't a showstopper.

I'm not trying to avoid work, I'm trying to understand.

As far as I see it failing to reset the counters has no impact on the
viability of the code.  It's still working with offsets and if the
offset is 0 or non-0, the user space won't see the difference in the
values returned by @get_stats64.  Successful resetting the counters is
just a bonus.

Considering that failing rtl8169_reset_counters or failing
rtl8169_update_counters has no negative impact on open at all, the
problem I have here is:

What do you do with a return value from either one of these functions?

rtl_open () {
  [...]

  if (!rtl8169_reset_counters (dev)) {
    /* ??? */
  }

  if (!rtl8169_update_counters (dev)) {
    /* ??? */
  }

  [...]
}


Thanks,
Corinna

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Francois Romieu]

Corinna Vinschen <vinschen@redhat.com> :
[...]
> That won't happen with the current patch because only
> rtl8169_reset_counters would print a log message, it's only called from
> open, and open occurs rather seldom.  Atop of that the code only tries
> to reset counters on HW supporting it, and only if resetting on the HW
> fails, there will be a log message at all.  There's no reasonable chance
> that failing to reset the counters will lead to log flooding.

Thanks for reformulating it. We are in violent agreement here.

[...]
> I'm not trying to avoid work, I'm trying to understand.
> 
> As far as I see it failing to reset the counters has no impact on the
> viability of the code.  It's still working with offsets and if the
> offset is 0 or non-0, the user space won't see the difference in the
> values returned by @get_stats64.  Successful resetting the counters is
> just a bonus.

Sorry, my english was really bad:

the code should propagate failure when rtl8169_reset_counters and
rtl8169_update_counters *simultaneously* fail.

-- 
Ueimor

Re: [PATCH v2 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 1820 bytes --]

On Aug 22 13:23, Francois Romieu wrote:
> Corinna Vinschen <vinschen@redhat.com> :
> [...]
> > That won't happen with the current patch because only
> > rtl8169_reset_counters would print a log message, it's only called from
> > open, and open occurs rather seldom.  Atop of that the code only tries
> > to reset counters on HW supporting it, and only if resetting on the HW
> > fails, there will be a log message at all.  There's no reasonable chance
> > that failing to reset the counters will lead to log flooding.
> 
> Thanks for reformulating it. We are in violent agreement here.
> 
> [...]
> > I'm not trying to avoid work, I'm trying to understand.
> > 
> > As far as I see it failing to reset the counters has no impact on the
> > viability of the code.  It's still working with offsets and if the
> > offset is 0 or non-0, the user space won't see the difference in the
> > values returned by @get_stats64.  Successful resetting the counters is
> > just a bonus.
> 
> Sorry, my english was really bad:
> 
> the code should propagate failure when rtl8169_reset_counters and
> rtl8169_update_counters *simultaneously* fail.

Uhm... sorry, but that still doesn't answer the question.  As you can
see in my patch, the initalization at open time is already encapsulated
in a function rtl8169_init_counter_offsets. 

Assuming rtl8169_init_counter_offsets returns -1 if both functions,
rtl8169_reset_counters and rtl8169_update_counters fail.

Then... what?

Not being able to reset or update the counters is still not at all fatal
for the operation of the NIC as a whole and rtl_open in particular:

  rtl_open()
  {
    [...]

    /* This is non-fatal. */
    if (!rtl8169_init_counter_offsets ()) {

	  /* What to do here??? */

    }

    [...]
  }


Corinna

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v3 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Corinna Vinschen]

The r8169 driver collects statistical information returned by
@get_stats64 by counting them in the driver itself, even though many
(but not all) of the values are already collected by tally counters
(TCs) in the NIC.  Some of these TC values are not returned by
@get_stats64.  Especially the received multicast packages are missing
from /proc/net/dev.

Rectify this by fetching the TCs and returning them from
rtl8169_get_stats64.

The counters collected in the driver obviously disappear as soon as the
driver is unloaded so after a driver is loaded the counters always start
at 0. The TCs on the other hand are only reset by a power cycle.  Without
further considerations the values collected by the driver would not match
up against the TC values.

This patch introduces a new function rtl8169_reset_counters which
resets the TCs.  Also, since rtl8169_reset_counters shares most of
its code with rtl8169_update_counters, refactor the shared code into
two new functions  rtl8169_map_counters and rtl8169_unmap_counters.

Unfortunately chip versions prior to RTL_GIGA_MAC_VER_19 don't allow
to reset the TCs programatically.  Therefore introduce an addition to
the rtl8169_private struct and a function rtl8169_init_counter_offsets
to store the TCs at first rtl_open.  Use these values as offsets in
rtl8169_get_stats64.  Propagate a failure to reset *and* update the
counters up to rtl_open and emit a warning message, if so.

Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---
 drivers/net/ethernet/realtek/r8169.c | 165 +++++++++++++++++++++++++++++++----
 1 file changed, 150 insertions(+), 15 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index f790f61..1e4322c 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,
 
@@ -747,6 +750,14 @@ struct rtl8169_counters {
 	__le16	tx_underun;
 };
 
+struct rtl8169_tc_offsets {
+	bool	inited;
+	__le64	tx_errors;
+	__le32	tx_multi_collision;
+	__le32	rx_multicast;
+	__le16	tx_aborted;
+};
+
 enum rtl_flag {
 	RTL_FLAG_TASK_ENABLED,
 	RTL_FLAG_TASK_SLOW_PENDING,
@@ -824,6 +835,7 @@ struct rtl8169_private {
 
 	struct mii_if_info mii;
 	struct rtl8169_counters counters;
+	struct rtl8169_tc_offsets tc_offset;
 	u32 saved_wolopts;
 	u32 opts1_mask;
 
@@ -2179,6 +2191,73 @@ static int rtl8169_get_sset_count(struct net_device *dev, int sset)
 	}
 }
 
+static struct rtl8169_counters *rtl8169_map_counters(struct net_device *dev,
+						     dma_addr_t *paddr,
+						     u32 counter_cmd)
+{
+	struct rtl8169_private *tp = netdev_priv(dev);
+	void __iomem *ioaddr = tp->mmio_addr;
+	struct device *d = &tp->pci_dev->dev;
+	struct rtl8169_counters *counters;
+	u32 cmd;
+
+	counters = dma_alloc_coherent(d, sizeof(*counters), paddr, GFP_KERNEL);
+	if (counters) {
+		RTL_W32(CounterAddrHigh, (u64)*paddr >> 32);
+		cmd = (u64)*paddr & DMA_BIT_MASK(32);
+		RTL_W32(CounterAddrLow, cmd);
+		RTL_W32(CounterAddrLow, cmd | counter_cmd);
+	}
+	return counters;
+}
+
+static void rtl8169_unmap_counters (struct net_device *dev,
+				    dma_addr_t paddr,
+				    struct rtl8169_counters *counters)
+{
+	struct rtl8169_private *tp = netdev_priv(dev);
+	void __iomem *ioaddr = tp->mmio_addr;
+	struct device *d = &tp->pci_dev->dev;
+
+	RTL_W32(CounterAddrLow, 0);
+	RTL_W32(CounterAddrHigh, 0);
+
+	dma_free_coherent(d, sizeof(*counters), counters, paddr);
+}
+
+DECLARE_RTL_COND(rtl_reset_counters_cond)
+{
+	void __iomem *ioaddr = tp->mmio_addr;
+
+	return RTL_R32(CounterAddrLow) & CounterReset;
+}
+
+static bool rtl8169_reset_counters(struct net_device *dev)
+{
+	struct rtl8169_private *tp = netdev_priv(dev);
+	struct rtl8169_counters *counters;
+	dma_addr_t paddr;
+	bool ret = true;
+
+	/*
+	 * Versions prior to RTL_GIGA_MAC_VER_19 don't support resetting the
+	 * tally counters.
+	 */
+	if (tp->mac_version < RTL_GIGA_MAC_VER_19)
+		return true;
+
+	counters = rtl8169_map_counters(dev, &paddr, CounterReset);
+	if (!counters)
+		return false;
+
+	if (!rtl_udelay_loop_wait_low(tp, &rtl_reset_counters_cond, 10, 1000))
+		ret = false;
+
+	rtl8169_unmap_counters(dev, paddr, counters);
+
+	return ret;
+}
+
 DECLARE_RTL_COND(rtl_counters_cond)
 {
 	void __iomem *ioaddr = tp->mmio_addr;
@@ -2186,38 +2265,72 @@ DECLARE_RTL_COND(rtl_counters_cond)
 	return RTL_R32(CounterAddrLow) & CounterDump;
 }
 
-static void rtl8169_update_counters(struct net_device *dev)
+static bool rtl8169_update_counters(struct net_device *dev)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
 	void __iomem *ioaddr = tp->mmio_addr;
-	struct device *d = &tp->pci_dev->dev;
 	struct rtl8169_counters *counters;
 	dma_addr_t paddr;
-	u32 cmd;
+	bool ret = true;
 
 	/*
 	 * Some chips are unable to dump tally counters when the receiver
 	 * is disabled.
 	 */
 	if ((RTL_R8(ChipCmd) & CmdRxEnb) == 0)
-		return;
+		return true;
 
-	counters = dma_alloc_coherent(d, sizeof(*counters), &paddr, GFP_KERNEL);
+	counters = rtl8169_map_counters(dev, &paddr, CounterDump);
 	if (!counters)
-		return;
-
-	RTL_W32(CounterAddrHigh, (u64)paddr >> 32);
-	cmd = (u64)paddr & DMA_BIT_MASK(32);
-	RTL_W32(CounterAddrLow, cmd);
-	RTL_W32(CounterAddrLow, cmd | CounterDump);
+		return false;
 
 	if (rtl_udelay_loop_wait_low(tp, &rtl_counters_cond, 10, 1000))
 		memcpy(&tp->counters, counters, sizeof(*counters));
+	else
+		ret = false;
 
-	RTL_W32(CounterAddrLow, 0);
-	RTL_W32(CounterAddrHigh, 0);
+	rtl8169_unmap_counters(dev, paddr, counters);
 
-	dma_free_coherent(d, sizeof(*counters), counters, paddr);
+	return ret;
+}
+
+static bool rtl8169_init_counter_offsets(struct net_device *dev)
+{
+	struct rtl8169_private *tp = netdev_priv(dev);
+	bool ret = false;
+
+	/*
+	 * rtl8169_init_counter_offsets 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
+	 * as offsets to normalize the values returned by @get_stats64.
+	 *
+	 * (*) We can't call rtl8169_init_counter_offsets from rtl_init_one
+	 * for the reason stated in rtl8169_update_counters; CmdRxEnb is only
+	 * set at open time by rtl_hw_start.
+	 */
+
+	if (tp->tc_offset.inited)
+		return true;
+
+	/* If both, reset and update fail, propagate to caller. */
+	if (rtl8169_reset_counters(dev))
+		ret = true;
+
+	if (rtl8169_update_counters(dev))
+		ret = true;
+
+	tp->tc_offset.tx_errors = tp->counters.tx_errors;
+	tp->tc_offset.tx_multi_collision = tp->counters.tx_multi_collision;
+	tp->tc_offset.rx_multicast = tp->counters.rx_multicast;
+	tp->tc_offset.tx_aborted = tp->counters.tx_aborted;
+	tp->tc_offset.inited = true;
+
+	return ret;
 }
 
 static void rtl8169_get_ethtool_stats(struct net_device *dev,
@@ -7544,7 +7657,7 @@ static int rtl8169_close(struct net_device *dev)
 
 	pm_runtime_get_sync(&pdev->dev);
 
-	/* Update counters before going down */
+	/* update counters before going down */
 	rtl8169_update_counters(dev);
 
 	rtl_lock_work(tp);
@@ -7631,6 +7744,9 @@ static int rtl_open(struct net_device *dev)
 
 	rtl_hw_start(dev);
 
+	if (!rtl8169_init_counter_offsets(dev))
+		netif_warn(tp, hw, dev, "counter reset/update failed\n");
+
 	netif_start_queue(dev);
 
 	rtl_unlock_work(tp);
@@ -7689,6 +7805,25 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
 	stats->rx_fifo_errors	= dev->stats.rx_fifo_errors;
 	stats->rx_missed_errors = dev->stats.rx_missed_errors;
 
+	/*
+	 * Fetch additonal counter values missing in stats collected by driver
+	 * from tally counters.
+	 */
+	rtl8169_update_counters(dev);
+
+	/*
+	 * Subtract values fetched during initalization.
+	 * See rtl8169_init_counter_offsets for a description why we do that.
+	 */
+	stats->tx_errors = le64_to_cpu(tp->counters.tx_errors) -
+		le64_to_cpu(tp->tc_offset.tx_errors);
+	stats->collisions = le32_to_cpu(tp->counters.tx_multi_collision) -
+		le32_to_cpu(tp->tc_offset.tx_multi_collision);
+	stats->multicast = le32_to_cpu(tp->counters.rx_multicast) -
+		le32_to_cpu(tp->tc_offset.rx_multicast);
+	stats->tx_aborted_errors = le16_to_cpu(tp->counters.tx_aborted) -
+		le16_to_cpu(tp->tc_offset.tx_aborted);
+
 	return stats;
 }
 
-- 
2.1.0

Re: [PATCH v2 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 2151 bytes --]

On Aug 24 09:33, Corinna Vinschen wrote:
> On Aug 22 13:23, Francois Romieu wrote:
> > Corinna Vinschen <vinschen@redhat.com> :
> > [...]
> > > That won't happen with the current patch because only
> > > rtl8169_reset_counters would print a log message, it's only called from
> > > open, and open occurs rather seldom.  Atop of that the code only tries
> > > to reset counters on HW supporting it, and only if resetting on the HW
> > > fails, there will be a log message at all.  There's no reasonable chance
> > > that failing to reset the counters will lead to log flooding.
> > 
> > Thanks for reformulating it. We are in violent agreement here.
> > 
> > [...]
> > > I'm not trying to avoid work, I'm trying to understand.
> > > 
> > > As far as I see it failing to reset the counters has no impact on the
> > > viability of the code.  It's still working with offsets and if the
> > > offset is 0 or non-0, the user space won't see the difference in the
> > > values returned by @get_stats64.  Successful resetting the counters is
> > > just a bonus.
> > 
> > Sorry, my english was really bad:
> > 
> > the code should propagate failure when rtl8169_reset_counters and
> > rtl8169_update_counters *simultaneously* fail.
> 
> Uhm... sorry, but that still doesn't answer the question.  As you can
> see in my patch, the initalization at open time is already encapsulated
> in a function rtl8169_init_counter_offsets. 
> 
> Assuming rtl8169_init_counter_offsets returns -1 if both functions,
> rtl8169_reset_counters and rtl8169_update_counters fail.
> 
> Then... what?
> 
> Not being able to reset or update the counters is still not at all fatal
> for the operation of the NIC as a whole and rtl_open in particular:
> 
>   rtl_open()
>   {
>     [...]
> 
>     /* This is non-fatal. */
>     if (!rtl8169_init_counter_offsets ()) {
> 
> 	  /* What to do here??? */
> 
>     }
> 
>     [...]
>   }

Never mind, I rearranged the code so that the warning is now printed
in rtl_open.  All functions related to the counter reset/update are
now bool functions.  I just sent a v3 patch.


Corinna

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

[PATCH v4 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Corinna Vinschen]

The r8169 driver collects statistical information returned by
@get_stats64 by counting them in the driver itself, even though many
(but not all) of the values are already collected by tally counters
(TCs) in the NIC.  Some of these TC values are not returned by
@get_stats64.  Especially the received multicast packages are missing
from /proc/net/dev.

Rectify this by fetching the TCs and returning them from
rtl8169_get_stats64.

The counters collected in the driver obviously disappear as soon as the
driver is unloaded so after a driver is loaded the counters always start
at 0. The TCs on the other hand are only reset by a power cycle.  Without
further considerations the values collected by the driver would not match
up against the TC values.

This patch introduces a new function rtl8169_reset_counters which
resets the TCs.  Also, since rtl8169_reset_counters shares most of
its code with rtl8169_update_counters, refactor the shared code into
two new functions  rtl8169_map_counters and rtl8169_unmap_counters.

Unfortunately chip versions prior to RTL_GIGA_MAC_VER_19 don't allow
to reset the TCs programatically.  Therefore introduce an addition to
the rtl8169_private struct and a function rtl8169_init_counter_offsets
to store the TCs at first rtl_open.  Use these values as offsets in
rtl8169_get_stats64.  Propagate a failure to reset *and* update the
counters up to rtl_open and emit a warning message, if so.

Signed-off-by: Corinna Vinschen <vinschen@redhat.com>
---

[v3 -> v4]: Remove useless rtl8169_close chunk changing case of comment


 drivers/net/ethernet/realtek/r8169.c | 163 ++++++++++++++++++++++++++++++++---
 1 file changed, 149 insertions(+), 14 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169.c b/drivers/net/ethernet/realtek/r8169.c
index f790f61..d6d39df 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,
 
@@ -747,6 +750,14 @@ struct rtl8169_counters {
 	__le16	tx_underun;
 };
 
+struct rtl8169_tc_offsets {
+	bool	inited;
+	__le64	tx_errors;
+	__le32	tx_multi_collision;
+	__le32	rx_multicast;
+	__le16	tx_aborted;
+};
+
 enum rtl_flag {
 	RTL_FLAG_TASK_ENABLED,
 	RTL_FLAG_TASK_SLOW_PENDING,
@@ -824,6 +835,7 @@ struct rtl8169_private {
 
 	struct mii_if_info mii;
 	struct rtl8169_counters counters;
+	struct rtl8169_tc_offsets tc_offset;
 	u32 saved_wolopts;
 	u32 opts1_mask;
 
@@ -2179,6 +2191,73 @@ static int rtl8169_get_sset_count(struct net_device *dev, int sset)
 	}
 }
 
+static struct rtl8169_counters *rtl8169_map_counters(struct net_device *dev,
+						     dma_addr_t *paddr,
+						     u32 counter_cmd)
+{
+	struct rtl8169_private *tp = netdev_priv(dev);
+	void __iomem *ioaddr = tp->mmio_addr;
+	struct device *d = &tp->pci_dev->dev;
+	struct rtl8169_counters *counters;
+	u32 cmd;
+
+	counters = dma_alloc_coherent(d, sizeof(*counters), paddr, GFP_KERNEL);
+	if (counters) {
+		RTL_W32(CounterAddrHigh, (u64)*paddr >> 32);
+		cmd = (u64)*paddr & DMA_BIT_MASK(32);
+		RTL_W32(CounterAddrLow, cmd);
+		RTL_W32(CounterAddrLow, cmd | counter_cmd);
+	}
+	return counters;
+}
+
+static void rtl8169_unmap_counters (struct net_device *dev,
+				    dma_addr_t paddr,
+				    struct rtl8169_counters *counters)
+{
+	struct rtl8169_private *tp = netdev_priv(dev);
+	void __iomem *ioaddr = tp->mmio_addr;
+	struct device *d = &tp->pci_dev->dev;
+
+	RTL_W32(CounterAddrLow, 0);
+	RTL_W32(CounterAddrHigh, 0);
+
+	dma_free_coherent(d, sizeof(*counters), counters, paddr);
+}
+
+DECLARE_RTL_COND(rtl_reset_counters_cond)
+{
+	void __iomem *ioaddr = tp->mmio_addr;
+
+	return RTL_R32(CounterAddrLow) & CounterReset;
+}
+
+static bool rtl8169_reset_counters(struct net_device *dev)
+{
+	struct rtl8169_private *tp = netdev_priv(dev);
+	struct rtl8169_counters *counters;
+	dma_addr_t paddr;
+	bool ret = true;
+
+	/*
+	 * Versions prior to RTL_GIGA_MAC_VER_19 don't support resetting the
+	 * tally counters.
+	 */
+	if (tp->mac_version < RTL_GIGA_MAC_VER_19)
+		return true;
+
+	counters = rtl8169_map_counters(dev, &paddr, CounterReset);
+	if (!counters)
+		return false;
+
+	if (!rtl_udelay_loop_wait_low(tp, &rtl_reset_counters_cond, 10, 1000))
+		ret = false;
+
+	rtl8169_unmap_counters(dev, paddr, counters);
+
+	return ret;
+}
+
 DECLARE_RTL_COND(rtl_counters_cond)
 {
 	void __iomem *ioaddr = tp->mmio_addr;
@@ -2186,38 +2265,72 @@ DECLARE_RTL_COND(rtl_counters_cond)
 	return RTL_R32(CounterAddrLow) & CounterDump;
 }
 
-static void rtl8169_update_counters(struct net_device *dev)
+static bool rtl8169_update_counters(struct net_device *dev)
 {
 	struct rtl8169_private *tp = netdev_priv(dev);
 	void __iomem *ioaddr = tp->mmio_addr;
-	struct device *d = &tp->pci_dev->dev;
 	struct rtl8169_counters *counters;
 	dma_addr_t paddr;
-	u32 cmd;
+	bool ret = true;
 
 	/*
 	 * Some chips are unable to dump tally counters when the receiver
 	 * is disabled.
 	 */
 	if ((RTL_R8(ChipCmd) & CmdRxEnb) == 0)
-		return;
+		return true;
 
-	counters = dma_alloc_coherent(d, sizeof(*counters), &paddr, GFP_KERNEL);
+	counters = rtl8169_map_counters(dev, &paddr, CounterDump);
 	if (!counters)
-		return;
-
-	RTL_W32(CounterAddrHigh, (u64)paddr >> 32);
-	cmd = (u64)paddr & DMA_BIT_MASK(32);
-	RTL_W32(CounterAddrLow, cmd);
-	RTL_W32(CounterAddrLow, cmd | CounterDump);
+		return false;
 
 	if (rtl_udelay_loop_wait_low(tp, &rtl_counters_cond, 10, 1000))
 		memcpy(&tp->counters, counters, sizeof(*counters));
+	else
+		ret = false;
 
-	RTL_W32(CounterAddrLow, 0);
-	RTL_W32(CounterAddrHigh, 0);
+	rtl8169_unmap_counters(dev, paddr, counters);
 
-	dma_free_coherent(d, sizeof(*counters), counters, paddr);
+	return ret;
+}
+
+static bool rtl8169_init_counter_offsets(struct net_device *dev)
+{
+	struct rtl8169_private *tp = netdev_priv(dev);
+	bool ret = false;
+
+	/*
+	 * rtl8169_init_counter_offsets 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
+	 * as offsets to normalize the values returned by @get_stats64.
+	 *
+	 * (*) We can't call rtl8169_init_counter_offsets from rtl_init_one
+	 * for the reason stated in rtl8169_update_counters; CmdRxEnb is only
+	 * set at open time by rtl_hw_start.
+	 */
+
+	if (tp->tc_offset.inited)
+		return true;
+
+	/* If both, reset and update fail, propagate to caller. */
+	if (rtl8169_reset_counters(dev))
+		ret = true;
+
+	if (rtl8169_update_counters(dev))
+		ret = true;
+
+	tp->tc_offset.tx_errors = tp->counters.tx_errors;
+	tp->tc_offset.tx_multi_collision = tp->counters.tx_multi_collision;
+	tp->tc_offset.rx_multicast = tp->counters.rx_multicast;
+	tp->tc_offset.tx_aborted = tp->counters.tx_aborted;
+	tp->tc_offset.inited = true;
+
+	return ret;
 }
 
 static void rtl8169_get_ethtool_stats(struct net_device *dev,
@@ -7631,6 +7744,9 @@ static int rtl_open(struct net_device *dev)
 
 	rtl_hw_start(dev);
 
+	if (!rtl8169_init_counter_offsets(dev))
+		netif_warn(tp, hw, dev, "counter reset/update failed\n");
+
 	netif_start_queue(dev);
 
 	rtl_unlock_work(tp);
@@ -7689,6 +7805,25 @@ rtl8169_get_stats64(struct net_device *dev, struct rtnl_link_stats64 *stats)
 	stats->rx_fifo_errors	= dev->stats.rx_fifo_errors;
 	stats->rx_missed_errors = dev->stats.rx_missed_errors;
 
+	/*
+	 * Fetch additonal counter values missing in stats collected by driver
+	 * from tally counters.
+	 */
+	rtl8169_update_counters(dev);
+
+	/*
+	 * Subtract values fetched during initalization.
+	 * See rtl8169_init_counter_offsets for a description why we do that.
+	 */
+	stats->tx_errors = le64_to_cpu(tp->counters.tx_errors) -
+		le64_to_cpu(tp->tc_offset.tx_errors);
+	stats->collisions = le32_to_cpu(tp->counters.tx_multi_collision) -
+		le32_to_cpu(tp->tc_offset.tx_multi_collision);
+	stats->multicast = le32_to_cpu(tp->counters.rx_multicast) -
+		le32_to_cpu(tp->tc_offset.rx_multicast);
+	stats->tx_aborted_errors = le16_to_cpu(tp->counters.tx_aborted) -
+		le16_to_cpu(tp->tc_offset.tx_aborted);
+
 	return stats;
 }
 
-- 
2.1.0

Re: [PATCH v2 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Francois Romieu]

Corinna Vinschen <vinschen@redhat.com> :
> On Aug 22 13:23, Francois Romieu wrote:
[...]
> > Sorry, my english was really bad:
> > 
> > the code should propagate failure when rtl8169_reset_counters and
> > rtl8169_update_counters *simultaneously* fail.
> 
> Uhm... sorry, but that still doesn't answer the question.  As you can
> see in my patch, the initalization at open time is already encapsulated
> in a function rtl8169_init_counter_offsets. 

I have read your patch, I have already answered the question and I have
already said that it wasn't a showstopper.

-- 
Ueimor

Re: [PATCH v2 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 708 bytes --]

On Aug 25 01:33, Francois Romieu wrote:
> Corinna Vinschen <vinschen@redhat.com> :
> > On Aug 22 13:23, Francois Romieu wrote:
> [...]
> > > Sorry, my english was really bad:
> > > 
> > > the code should propagate failure when rtl8169_reset_counters and
> > > rtl8169_update_counters *simultaneously* fail.
> > 
> > Uhm... sorry, but that still doesn't answer the question.  As you can
> > see in my patch, the initalization at open time is already encapsulated
> > in a function rtl8169_init_counter_offsets. 
> 
> I have read your patch, I have already answered the question and I have
> already said that it wasn't a showstopper.

Ok.  I hope v4 of the patch is ok?


Thanks,
Corinna

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v2 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Francois Romieu]

Corinna Vinschen <vinschen@redhat.com> :
[...]
> Ok.  I hope v4 of the patch is ok?

The whole map...unmap sequence - memcpy included - could be in a single
function and the comments in the code are imho a bit verbose but it's
good enough as is.

-- 
Ueimor

Re: [PATCH v4 net-next] r8169: Add values missing in @get_stats64 from HW counters

[David Miller]

From: Corinna Vinschen <vinschen@redhat.com>
Date: Mon, 24 Aug 2015 12:52:39 +0200

> The r8169 driver collects statistical information returned by
> @get_stats64 by counting them in the driver itself, even though many
> (but not all) of the values are already collected by tally counters
> (TCs) in the NIC.  Some of these TC values are not returned by
> @get_stats64.  Especially the received multicast packages are missing
> from /proc/net/dev.
> 
> Rectify this by fetching the TCs and returning them from
> rtl8169_get_stats64.
> 
> The counters collected in the driver obviously disappear as soon as the
> driver is unloaded so after a driver is loaded the counters always start
> at 0. The TCs on the other hand are only reset by a power cycle.  Without
> further considerations the values collected by the driver would not match
> up against the TC values.
> 
> This patch introduces a new function rtl8169_reset_counters which
> resets the TCs.  Also, since rtl8169_reset_counters shares most of
> its code with rtl8169_update_counters, refactor the shared code into
> two new functions  rtl8169_map_counters and rtl8169_unmap_counters.
> 
> Unfortunately chip versions prior to RTL_GIGA_MAC_VER_19 don't allow
> to reset the TCs programatically.  Therefore introduce an addition to
> the rtl8169_private struct and a function rtl8169_init_counter_offsets
> to store the TCs at first rtl_open.  Use these values as offsets in
> rtl8169_get_stats64.  Propagate a failure to reset *and* update the
> counters up to rtl_open and emit a warning message, if so.
> 
> Signed-off-by: Corinna Vinschen <vinschen@redhat.com>

Your counter offsets should be read at probe time, not open time.

Bringing the interface is brought down/up should not reset the
counters.  Look at how the software counters behave in such a
situation.

Re: [PATCH v4 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Francois Romieu]

David Miller <davem@davemloft.net> :
[...]
> Your counter offsets should be read at probe time, not open time.

It can be done but the "CmdRxEnb / rx traffic must be enabled" constraint
will make it a major pita. 

Reading counter offsets at the end of open() naturally solves this
constraint (retentive error unwinding in opne() stops being completely
trivial though :o/ ).

> Bringing the interface is brought down/up should not reset the
> counters.

Afaiks rtl8169_tc_offsets.inited in rtl8169_init_counter_offsets
takes care of it: it's set during the first open() after probe().

Looking at it again, the patch directly stores 16 and 32 bit values
in rtnl_link_stats64. Nobody should care about exact exceedingly high
error count but rx_multicast ought to be accumulated.

-- 
Ueimor

Re: [PATCH v4 net-next] r8169: Add values missing in @get_stats64 from HW counters

[David Miller]

From: Francois Romieu <romieu@fr.zoreil.com>
Date: Wed, 26 Aug 2015 00:54:06 +0200

> David Miller <davem@davemloft.net> :
> [...]
>> Your counter offsets should be read at probe time, not open time.
> 
> It can be done but the "CmdRxEnb / rx traffic must be enabled" constraint
> will make it a major pita. 
> 
> Reading counter offsets at the end of open() naturally solves this
> constraint (retentive error unwinding in opne() stops being completely
> trivial though :o/ ).

So you can manage whether you've done the "once per device probe"
counter reset, and act upon it at the end of ->open().

>> Bringing the interface is brought down/up should not reset the
>> counters.
> 
> Afaiks rtl8169_tc_offsets.inited in rtl8169_init_counter_offsets
> takes care of it: it's set during the first open() after probe().

Ok, then it's fine.

Re: [PATCH v4 net-next] r8169: Add values missing in @get_stats64 from HW counters

[David Miller]

From: David Miller <davem@davemloft.net>
Date: Tue, 25 Aug 2015 15:59:21 -0700 (PDT)

> From: Francois Romieu <romieu@fr.zoreil.com>
> Date: Wed, 26 Aug 2015 00:54:06 +0200
> 
>>> Bringing the interface is brought down/up should not reset the
>>> counters.
>> 
>> Afaiks rtl8169_tc_offsets.inited in rtl8169_init_counter_offsets
>> takes care of it: it's set during the first open() after probe().
> 
> Ok, then it's fine.

And as such I've applied this patch, thanks.

Re: [PATCH v4 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 953 bytes --]

On Aug 26 00:54, Francois Romieu wrote:
> David Miller <davem@davemloft.net> :
> [...]
> > Your counter offsets should be read at probe time, not open time.
> 
> It can be done but the "CmdRxEnb / rx traffic must be enabled" constraint
> will make it a major pita. 
> 
> Reading counter offsets at the end of open() naturally solves this
> constraint (retentive error unwinding in opne() stops being completely
> trivial though :o/ ).
> 
> > Bringing the interface is brought down/up should not reset the
> > counters.
> 
> Afaiks rtl8169_tc_offsets.inited in rtl8169_init_counter_offsets
> takes care of it: it's set during the first open() after probe().
> 
> Looking at it again, the patch directly stores 16 and 32 bit values
> in rtnl_link_stats64. Nobody should care about exact exceedingly high
> error count but rx_multicast ought to be accumulated.

I'll have a look into that for a followup patch.


Thanks,
Corinna

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v4 net-next] r8169: Add values missing in @get_stats64 from HW counters

[Corinna Vinschen]

[-- Attachment #1: Type: text/plain, Size: 572 bytes --]

On Aug 25 16:03, David Miller wrote:
> From: David Miller <davem@davemloft.net>
> Date: Tue, 25 Aug 2015 15:59:21 -0700 (PDT)
> 
> > From: Francois Romieu <romieu@fr.zoreil.com>
> > Date: Wed, 26 Aug 2015 00:54:06 +0200
> > 
> >>> Bringing the interface is brought down/up should not reset the
> >>> counters.
> >> 
> >> Afaiks rtl8169_tc_offsets.inited in rtl8169_init_counter_offsets
> >> takes care of it: it's set during the first open() after probe().
> > 
> > Ok, then it's fine.
> 
> And as such I've applied this patch, thanks.

Thanks,
Corinna

[-- Attachment #2: Type: application/pgp-signature, Size: 819 bytes --]