[PATCH RFC net-next] net: dsa: Provide CPU port statistics to master netdev

[PATCH RFC net-next] net: dsa: Provide CPU port statistics to master netdev

[Florian Fainelli]

This patch overloads the DSA master netdev, aka CPU Ethernet MAC to also
include switch-side statistics, which is useful for debugging purposes,
when the switch is not properly connected to the Ethernet MAC (duplex
mismatch, (RG)MII electrical issues etc.).

We accomplish this by retaining the original copy of the master netdev's
ethtool_ops, and just overload the 3 operations we care about:
get_sset_count, get_strings and get_ethtool_stats so as to intercept
these calls and call into the original master_netdev ethtool_ops, plus
our own.

We take this approach as opposed to providing a set of DSA helper
functions that would retrive the CPU port's statistics, because the
entire purpose of DSA is to allow unmodified Ethernet MAC drivers to be
used as CPU conduit interfaces, therefore, statistics overlay in such
drivers would simply not scale.

Signed-off-by: Florian Fainelli <f.fainelli@gmail.com>
---
 include/net/dsa.h |  5 ++++
 net/dsa/slave.c   | 69 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 74 insertions(+)

diff --git a/include/net/dsa.h b/include/net/dsa.h
index c4bc42bd3538..67f811f00339 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -111,6 +111,11 @@ struct dsa_switch_tree {
 	enum dsa_tag_protocol	tag_protocol;
 
 	/*
+	 * Original copy of the master netdev ethtool_ops
+	 */
+	struct ethtool_ops	master_ethtool_ops;
+
+	/*
 	 * The switch and port to which the CPU is attached.
 	 */
 	s8			cpu_switch;
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index 2dae0d064359..41283c6f725a 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -666,6 +666,59 @@ static void dsa_slave_get_strings(struct net_device *dev,
 	}
 }
 
+static void dsa_cpu_port_get_ethtool_stats(struct net_device *dev,
+					   struct ethtool_stats *stats,
+					   uint64_t *data)
+{
+	struct dsa_switch_tree *dst = dev->dsa_ptr;
+	struct dsa_switch *ds = dst->ds[0];
+	s8 cpu_port = dst->cpu_port;
+	int count = 0;
+
+	if (dst->master_ethtool_ops.get_sset_count) {
+		count = dst->master_ethtool_ops.get_sset_count(dev,
+							       ETH_SS_STATS);
+		dst->master_ethtool_ops.get_ethtool_stats(dev, stats, data);
+	}
+
+	if (ds->drv->get_ethtool_stats)
+		ds->drv->get_ethtool_stats(ds, cpu_port, data + count);
+}
+
+static int dsa_cpu_port_get_sset_count(struct net_device *dev, int sset)
+{
+	struct dsa_switch_tree *dst = dev->dsa_ptr;
+	struct dsa_switch *ds = dst->ds[0];
+	int count = 0;
+
+	if (dst->master_ethtool_ops.get_sset_count)
+		count += dst->master_ethtool_ops.get_sset_count(dev, sset);
+
+	if (sset == ETH_SS_STATS && ds->drv->get_sset_count)
+		count += ds->drv->get_sset_count(ds);
+
+	return count;
+}
+
+static void dsa_cpu_port_get_strings(struct net_device *dev,
+				     uint32_t stringset, uint8_t *data)
+{
+	struct dsa_switch_tree *dst = dev->dsa_ptr;
+	struct dsa_switch *ds = dst->ds[0];
+	s8 cpu_port = dst->cpu_port;
+	int len = ETH_GSTRING_LEN;
+	int count = 0;
+
+	if (dst->master_ethtool_ops.get_sset_count) {
+		count = dst->master_ethtool_ops.get_sset_count(dev,
+							       ETH_SS_STATS);
+		dst->master_ethtool_ops.get_strings(dev, stringset, data);
+	}
+
+	if (stringset == ETH_SS_STATS && ds->drv->get_strings)
+		ds->drv->get_strings(ds, cpu_port, data + count * len);
+}
+
 static void dsa_slave_get_ethtool_stats(struct net_device *dev,
 					struct ethtool_stats *stats,
 					uint64_t *data)
@@ -821,6 +874,8 @@ static const struct ethtool_ops dsa_slave_ethtool_ops = {
 	.get_eee		= dsa_slave_get_eee,
 };
 
+static struct ethtool_ops dsa_cpu_port_ethtool_ops;
+
 static const struct net_device_ops dsa_slave_netdev_ops = {
 	.ndo_open	 	= dsa_slave_open,
 	.ndo_stop		= dsa_slave_close,
@@ -1038,6 +1093,7 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 		     int port, char *name)
 {
 	struct net_device *master = ds->dst->master_netdev;
+	struct dsa_switch_tree *dst = ds->dst;
 	struct net_device *slave_dev;
 	struct dsa_slave_priv *p;
 	int ret;
@@ -1049,6 +1105,19 @@ int dsa_slave_create(struct dsa_switch *ds, struct device *parent,
 
 	slave_dev->features = master->vlan_features;
 	slave_dev->ethtool_ops = &dsa_slave_ethtool_ops;
+	if (master->ethtool_ops != &dsa_cpu_port_ethtool_ops) {
+		memcpy(&dst->master_ethtool_ops, master->ethtool_ops,
+		       sizeof(struct ethtool_ops));
+		memcpy(&dsa_cpu_port_ethtool_ops, &dst->master_ethtool_ops,
+		       sizeof(struct ethtool_ops));
+		dsa_cpu_port_ethtool_ops.get_sset_count =
+					dsa_cpu_port_get_sset_count;
+		dsa_cpu_port_ethtool_ops.get_ethtool_stats =
+					dsa_cpu_port_get_ethtool_stats;
+		dsa_cpu_port_ethtool_ops.get_strings =
+					dsa_cpu_port_get_strings;
+		master->ethtool_ops = &dsa_cpu_port_ethtool_ops;
+	}
 	eth_hw_addr_inherit(slave_dev, master);
 	slave_dev->priv_flags |= IFF_NO_QUEUE;
 	slave_dev->netdev_ops = &dsa_slave_netdev_ops;
-- 
2.1.0

Re: [PATCH RFC net-next] net: dsa: Provide CPU port statistics to master netdev

[Andrew Lunn]

On Wed, Apr 20, 2016 at 10:58:21AM -0700, Florian Fainelli wrote:
> This patch overloads the DSA master netdev, aka CPU Ethernet MAC to also
> include switch-side statistics, which is useful for debugging purposes,
> when the switch is not properly connected to the Ethernet MAC (duplex
> mismatch, (RG)MII electrical issues etc.).
> 
> We accomplish this by retaining the original copy of the master netdev's
> ethtool_ops, and just overload the 3 operations we care about:
> get_sset_count, get_strings and get_ethtool_stats so as to intercept
> these calls and call into the original master_netdev ethtool_ops, plus
> our own.

Hi Florian

Interesting concept. My one concern is that by concatenating the two
sets of statistics, we get a name clash. I'm not sure the Marvell
switch statistics counters have different names to the Marvell
Ethernet driver statistics counters. ethtool does not care, but maybe
an SNMP agent using these statistics might not be too happy seeing the
same name twice?

     Andrew

Re: [PATCH RFC net-next] net: dsa: Provide CPU port statistics to master netdev

[Florian Fainelli]

On 25/04/16 14:43, Andrew Lunn wrote:
> On Wed, Apr 20, 2016 at 10:58:21AM -0700, Florian Fainelli wrote:
>> This patch overloads the DSA master netdev, aka CPU Ethernet MAC to also
>> include switch-side statistics, which is useful for debugging purposes,
>> when the switch is not properly connected to the Ethernet MAC (duplex
>> mismatch, (RG)MII electrical issues etc.).
>>
>> We accomplish this by retaining the original copy of the master netdev's
>> ethtool_ops, and just overload the 3 operations we care about:
>> get_sset_count, get_strings and get_ethtool_stats so as to intercept
>> these calls and call into the original master_netdev ethtool_ops, plus
>> our own.
> 
> Hi Florian
> 
> Interesting concept. My one concern is that by concatenating the two
> sets of statistics, we get a name clash. I'm not sure the Marvell
> switch statistics counters have different names to the Marvell
> Ethernet driver statistics counters. ethtool does not care, but maybe
> an SNMP agent using these statistics might not be too happy seeing the
> same name twice?

That's a very good point, would you agree if we were prefixing the DSA
CPU port statistics with some kind of name, e.g: cpu_port_<name> or
something more compact?
-- 
Florian

Re: [PATCH RFC net-next] net: dsa: Provide CPU port statistics to master netdev

[Andrew Lunn]

> > Hi Florian
> > 
> > Interesting concept. My one concern is that by concatenating the two
> > sets of statistics, we get a name clash. I'm not sure the Marvell
> > switch statistics counters have different names to the Marvell
> > Ethernet driver statistics counters. ethtool does not care, but maybe
> > an SNMP agent using these statistics might not be too happy seeing the
> > same name twice?
> 
> That's a very good point, would you agree if we were prefixing the DSA
> CPU port statistics with some kind of name, e.g: cpu_port_<name> or
> something more compact?

Yes, that would be O.K.

It cannot be too long a prefix. The maximum length of the name is
32. The header file is unclear if there must be a NULL at the end. It
looks like the longest switch statistic we have is
RxPkts1024toMaxPktsOctets, i.e. 25 characters. So we only have 6 or 7
characters for the prefix.

	   Andrew