[PATCH net-next 0/5] Export SERDES stats via ethtool -S

[PATCH net-next 0/5] Export SERDES stats via ethtool -S

[Andrew Lunn]

The mv88e6352 family has a SERDES interface which can be used for
example to connect to SFF/SFP modules. This interface has a couple of
statistics counters. Add support for including these counters in the
output of ethtool -S.

Andrew Lunn (5):
  dsa: Pass the port to get_sset_count()
  net: dsa: mv88e6xxx: Hold mutex while doing stats operations
  net: dsa: mv88e6xxx: Allow the SERDES interfaces to have statistics
  net: dsa: mv88e6xxx: Add helper to determining if port has SERDES
  net: dsa: mv88e6xxx: Get mv88e6352 SERDES statistics

 drivers/net/dsa/b53/b53_common.c       |   2 +-
 drivers/net/dsa/b53/b53_priv.h         |   2 +-
 drivers/net/dsa/dsa_loop.c             |   2 +-
 drivers/net/dsa/lan9303-core.c         |   2 +-
 drivers/net/dsa/microchip/ksz_common.c |   2 +-
 drivers/net/dsa/mt7530.c               |   2 +-
 drivers/net/dsa/mv88e6xxx/chip.c       |  94 +++++++++++++++++++++--------
 drivers/net/dsa/mv88e6xxx/chip.h       |  20 ++++++-
 drivers/net/dsa/mv88e6xxx/serdes.c     | 106 +++++++++++++++++++++++++++++++--
 drivers/net/dsa/mv88e6xxx/serdes.h     |   6 +-
 drivers/net/dsa/qca8k.c                |   2 +-
 include/net/dsa.h                      |   2 +-
 net/dsa/master.c                       |   4 +-
 net/dsa/slave.c                        |   2 +-
 14 files changed, 204 insertions(+), 44 deletions(-)

-- 
2.15.1

[PATCH net-next 1/5] dsa: Pass the port to get_sset_count()

[Andrew Lunn]

By passing the port, we allow different ports to have different
statistics. This is useful since some ports have SERDES interfaces
with their own statistic counters.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/b53/b53_common.c       | 2 +-
 drivers/net/dsa/b53/b53_priv.h         | 2 +-
 drivers/net/dsa/dsa_loop.c             | 2 +-
 drivers/net/dsa/lan9303-core.c         | 2 +-
 drivers/net/dsa/microchip/ksz_common.c | 2 +-
 drivers/net/dsa/mt7530.c               | 2 +-
 drivers/net/dsa/mv88e6xxx/chip.c       | 2 +-
 drivers/net/dsa/qca8k.c                | 2 +-
 include/net/dsa.h                      | 2 +-
 net/dsa/master.c                       | 4 ++--
 net/dsa/slave.c                        | 2 +-
 11 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/drivers/net/dsa/b53/b53_common.c b/drivers/net/dsa/b53/b53_common.c
index 561b05089cb6..0c1d814573b3 100644
--- a/drivers/net/dsa/b53/b53_common.c
+++ b/drivers/net/dsa/b53/b53_common.c
@@ -852,7 +852,7 @@ void b53_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data)
 }
 EXPORT_SYMBOL(b53_get_ethtool_stats);
 
-int b53_get_sset_count(struct dsa_switch *ds)
+int b53_get_sset_count(struct dsa_switch *ds, int port)
 {
 	struct b53_device *dev = ds->priv;
 
diff --git a/drivers/net/dsa/b53/b53_priv.h b/drivers/net/dsa/b53/b53_priv.h
index d954cf36ecd8..1187ebd79287 100644
--- a/drivers/net/dsa/b53/b53_priv.h
+++ b/drivers/net/dsa/b53/b53_priv.h
@@ -288,7 +288,7 @@ void b53_imp_vlan_setup(struct dsa_switch *ds, int cpu_port);
 int b53_configure_vlan(struct dsa_switch *ds);
 void b53_get_strings(struct dsa_switch *ds, int port, uint8_t *data);
 void b53_get_ethtool_stats(struct dsa_switch *ds, int port, uint64_t *data);
-int b53_get_sset_count(struct dsa_switch *ds);
+int b53_get_sset_count(struct dsa_switch *ds, int port);
 int b53_br_join(struct dsa_switch *ds, int port, struct net_device *bridge);
 void b53_br_leave(struct dsa_switch *ds, int port, struct net_device *bridge);
 void b53_br_set_stp_state(struct dsa_switch *ds, int port, u8 state);
diff --git a/drivers/net/dsa/dsa_loop.c b/drivers/net/dsa/dsa_loop.c
index 7aa84ee4e771..f77be9f85cb3 100644
--- a/drivers/net/dsa/dsa_loop.c
+++ b/drivers/net/dsa/dsa_loop.c
@@ -86,7 +86,7 @@ static int dsa_loop_setup(struct dsa_switch *ds)
 	return 0;
 }
 
-static int dsa_loop_get_sset_count(struct dsa_switch *ds)
+static int dsa_loop_get_sset_count(struct dsa_switch *ds, int port)
 {
 	return __DSA_LOOP_CNT_MAX;
 }
diff --git a/drivers/net/dsa/lan9303-core.c b/drivers/net/dsa/lan9303-core.c
index 944901f03f8b..ba46dd319b10 100644
--- a/drivers/net/dsa/lan9303-core.c
+++ b/drivers/net/dsa/lan9303-core.c
@@ -1011,7 +1011,7 @@ static void lan9303_get_ethtool_stats(struct dsa_switch *ds, int port,
 	}
 }
 
-static int lan9303_get_sset_count(struct dsa_switch *ds)
+static int lan9303_get_sset_count(struct dsa_switch *ds, int port)
 {
 	return ARRAY_SIZE(lan9303_mib);
 }
diff --git a/drivers/net/dsa/microchip/ksz_common.c b/drivers/net/dsa/microchip/ksz_common.c
index 663b0d5b982b..bcb3e6c734f2 100644
--- a/drivers/net/dsa/microchip/ksz_common.c
+++ b/drivers/net/dsa/microchip/ksz_common.c
@@ -439,7 +439,7 @@ static void ksz_disable_port(struct dsa_switch *ds, int port,
 	ksz_port_cfg(dev, port, REG_PORT_CTRL_0, PORT_MAC_LOOPBACK, true);
 }
 
-static int ksz_sset_count(struct dsa_switch *ds)
+static int ksz_sset_count(struct dsa_switch *ds, int port)
 {
 	return TOTAL_SWITCH_COUNTER_NUM;
 }
diff --git a/drivers/net/dsa/mt7530.c b/drivers/net/dsa/mt7530.c
index 8a0bb000d056..511ca134f13f 100644
--- a/drivers/net/dsa/mt7530.c
+++ b/drivers/net/dsa/mt7530.c
@@ -604,7 +604,7 @@ mt7530_get_ethtool_stats(struct dsa_switch *ds, int port,
 }
 
 static int
-mt7530_get_sset_count(struct dsa_switch *ds)
+mt7530_get_sset_count(struct dsa_switch *ds, int port)
 {
 	return ARRAY_SIZE(mt7530_mib);
 }
diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index fc512c98f2f8..504407adc7aa 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -692,7 +692,7 @@ static int mv88e6320_stats_get_sset_count(struct mv88e6xxx_chip *chip)
 					      STATS_TYPE_BANK1);
 }
 
-static int mv88e6xxx_get_sset_count(struct dsa_switch *ds)
+static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 
diff --git a/drivers/net/dsa/qca8k.c b/drivers/net/dsa/qca8k.c
index 9df22ebee822..600d5ad1fbde 100644
--- a/drivers/net/dsa/qca8k.c
+++ b/drivers/net/dsa/qca8k.c
@@ -631,7 +631,7 @@ qca8k_get_ethtool_stats(struct dsa_switch *ds, int port,
 }
 
 static int
-qca8k_get_sset_count(struct dsa_switch *ds)
+qca8k_get_sset_count(struct dsa_switch *ds, int port)
 {
 	return ARRAY_SIZE(ar8327_mib);
 }
diff --git a/include/net/dsa.h b/include/net/dsa.h
index 6cb602dd970c..35433386c314 100644
--- a/include/net/dsa.h
+++ b/include/net/dsa.h
@@ -357,7 +357,7 @@ struct dsa_switch_ops {
 	void	(*get_strings)(struct dsa_switch *ds, int port, uint8_t *data);
 	void	(*get_ethtool_stats)(struct dsa_switch *ds,
 				     int port, uint64_t *data);
-	int	(*get_sset_count)(struct dsa_switch *ds);
+	int	(*get_sset_count)(struct dsa_switch *ds, int port);
 
 	/*
 	 * ethtool Wake-on-LAN
diff --git a/net/dsa/master.c b/net/dsa/master.c
index 00589147f042..f20a9600318f 100644
--- a/net/dsa/master.c
+++ b/net/dsa/master.c
@@ -42,7 +42,7 @@ static int dsa_master_get_sset_count(struct net_device *dev, int sset)
 		count += ops->get_sset_count(dev, sset);
 
 	if (sset == ETH_SS_STATS && ds->ops->get_sset_count)
-		count += ds->ops->get_sset_count(ds);
+		count += ds->ops->get_sset_count(ds, cpu_dp->index);
 
 	return count;
 }
@@ -76,7 +76,7 @@ static void dsa_master_get_strings(struct net_device *dev, uint32_t stringset,
 		 * constructed earlier
 		 */
 		ds->ops->get_strings(ds, port, ndata);
-		count = ds->ops->get_sset_count(ds);
+		count = ds->ops->get_sset_count(ds, cpu_dp->index);
 		for (i = 0; i < count; i++) {
 			memmove(ndata + (i * len + sizeof(pfx)),
 				ndata + i * len, len - sizeof(pfx));
diff --git a/net/dsa/slave.c b/net/dsa/slave.c
index f52307296de4..0d07004d59d4 100644
--- a/net/dsa/slave.c
+++ b/net/dsa/slave.c
@@ -559,7 +559,7 @@ static int dsa_slave_get_sset_count(struct net_device *dev, int sset)
 
 		count = 4;
 		if (ds->ops->get_sset_count)
-			count += ds->ops->get_sset_count(ds);
+			count += ds->ops->get_sset_count(ds, dp->index);
 
 		return count;
 	}
-- 
2.15.1

Re: [PATCH net-next 1/5] dsa: Pass the port to get_sset_count()

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> --- a/net/dsa/master.c
> +++ b/net/dsa/master.c
> @@ -42,7 +42,7 @@ static int dsa_master_get_sset_count(struct net_device *dev, int sset)
>  		count += ops->get_sset_count(dev, sset);
>  
>  	if (sset == ETH_SS_STATS && ds->ops->get_sset_count)
> -		count += ds->ops->get_sset_count(ds);
> +		count += ds->ops->get_sset_count(ds, cpu_dp->index);
>  
>  	return count;
>  }
> @@ -76,7 +76,7 @@ static void dsa_master_get_strings(struct net_device *dev, uint32_t stringset,
>  		 * constructed earlier
>  		 */
>  		ds->ops->get_strings(ds, port, ndata);
> -		count = ds->ops->get_sset_count(ds);
> +		count = ds->ops->get_sset_count(ds, cpu_dp->index);

You could reuse the 'port' variable already assigned to cpu_dp->index.

>  		for (i = 0; i < count; i++) {
>  			memmove(ndata + (i * len + sizeof(pfx)),
>  				ndata + i * len, len - sizeof(pfx));
> diff --git a/net/dsa/slave.c b/net/dsa/slave.c
> index f52307296de4..0d07004d59d4 100644
> --- a/net/dsa/slave.c
> +++ b/net/dsa/slave.c
> @@ -559,7 +559,7 @@ static int dsa_slave_get_sset_count(struct net_device *dev, int sset)
>  
>  		count = 4;
>  		if (ds->ops->get_sset_count)
> -			count += ds->ops->get_sset_count(ds);
> +			count += ds->ops->get_sset_count(ds, dp->index);
>  
>  		return count;
>  	}

Otherwise:

Reviewed-by: Vivien Didelot <vivien.didelot@savoirfairelinux.com>


Thanks,

        Vivien

[PATCH net-next 2/5] net: dsa: mv88e6xxx: Hold mutex while doing stats operations

[Andrew Lunn]

Until now, there has been no need to hold the reg mutex while getting
the count of statistics, or the strings, because the hardware was not
accessed. When adding support for SERDES statistics, it is necessary
to access the hardware, to determine if a port is using the SERDES
interface. So add mutex lock/unlocks.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 504407adc7aa..12e274a3ff24 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -662,8 +662,12 @@ static void mv88e6xxx_get_strings(struct dsa_switch *ds, int port,
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 
+	mutex_lock(&chip->reg_lock);
+
 	if (chip->info->ops->stats_get_strings)
 		chip->info->ops->stats_get_strings(chip, data);
+
+	mutex_unlock(&chip->reg_lock);
 }
 
 static int mv88e6xxx_stats_get_sset_count(struct mv88e6xxx_chip *chip,
@@ -692,7 +696,7 @@ static int mv88e6320_stats_get_sset_count(struct mv88e6xxx_chip *chip)
 					      STATS_TYPE_BANK1);
 }
 
-static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
+static int _mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
 
@@ -702,6 +706,19 @@ static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
 	return 0;
 }
 
+static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
+{
+	struct mv88e6xxx_chip *chip = ds->priv;
+	int ret;
+
+	mutex_lock(&chip->reg_lock);
+	ret = _mv88e6xxx_get_sset_count(ds, port);
+	mutex_unlock(&chip->reg_lock);
+
+	return ret;
+}
+
+
 static void mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
 				      uint64_t *data, int types,
 				      u16 bank1_select, u16 histogram)
-- 
2.15.1

Re: [PATCH net-next 2/5] net: dsa: mv88e6xxx: Hold mutex while doing stats operations

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> -static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
> +static int _mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
>  {
>  	struct mv88e6xxx_chip *chip = ds->priv;
>  
> @@ -702,6 +706,19 @@ static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
>  	return 0;
>  }

We worked to remove the old underscore prefix convention. Please don't
add it back... Simply rework the return statements of
mv88e6xxx_get_sset_count to lock/unlock there.

>  
> +static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
> +{
> +	struct mv88e6xxx_chip *chip = ds->priv;
> +	int ret;
> +
> +	mutex_lock(&chip->reg_lock);
> +	ret = _mv88e6xxx_get_sset_count(ds, port);
> +	mutex_unlock(&chip->reg_lock);
> +
> +	return ret;
> +}
> +
> +

Extra newline.

>  static void mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
>  				      uint64_t *data, int types,
>  				      u16 bank1_select, u16 histogram)

Thanks,

        Vivien

Re: [PATCH net-next 2/5] net: dsa: mv88e6xxx: Hold mutex while doing stats operations

[Andrew Lunn]

On Wed, Jan 03, 2018 at 09:32:42AM -0500, Vivien Didelot wrote:
> Hi Andrew,
> 
> Andrew Lunn <andrew@lunn.ch> writes:
> 
> > -static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
> > +static int _mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
> >  {
> >  	struct mv88e6xxx_chip *chip = ds->priv;
> >  
> > @@ -702,6 +706,19 @@ static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
> >  	return 0;
> >  }
> 
> We worked to remove the old underscore prefix convention. Please don't
> add it back... Simply rework the return statements of
> mv88e6xxx_get_sset_count to lock/unlock there.

Hi Vivien

That makes mv88e6xxx_get_sset_count quite complex, making it error
prone. Doing the locking in a separate function makes is very clear
the lock is held and then correctly released. So i will just rename
_mv88e6xxx_get_sset_count() to mv88e6xxx_get_sset_count_locked()

    Andrew

Re: [PATCH net-next 2/5] net: dsa: mv88e6xxx: Hold mutex while doing stats operations

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> On Wed, Jan 03, 2018 at 09:32:42AM -0500, Vivien Didelot wrote:
>> Hi Andrew,
>> 
>> Andrew Lunn <andrew@lunn.ch> writes:
>> 
>> > -static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
>> > +static int _mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
>> >  {
>> >  	struct mv88e6xxx_chip *chip = ds->priv;
>> >  
>> > @@ -702,6 +706,19 @@ static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
>> >  	return 0;
>> >  }
>> 
>> We worked to remove the old underscore prefix convention. Please don't
>> add it back... Simply rework the return statements of
>> mv88e6xxx_get_sset_count to lock/unlock there.
>
> That makes mv88e6xxx_get_sset_count quite complex, making it error
> prone. Doing the locking in a separate function makes is very clear
> the lock is held and then correctly released. So i will just rename
> _mv88e6xxx_get_sset_count() to mv88e6xxx_get_sset_count_locked()

     static int mv88e6xxx_get_sset_count(struct dsa_switch *ds)
     {
            struct mv88e6xxx_chip *chip = ds->priv;
    +       int err;
     
    -       if (chip->info->ops->stats_get_sset_count)
    -               return chip->info->ops->stats_get_sset_count(chip);
    +       if (!chip->info->ops->stats_get_sset_count)
    +               return 0;
     
    -       return 0;
    +       mutex_lock(&chip->reg_lock);
    +       err = chip->info->ops->stats_get_sset_count(chip);
    +       mutex_unlock(&chip->reg_lock);
    +
    +       return err;
     }


This is quite complex and error prone, seriously?


     Vivien

[PATCH net-next 3/5] net: dsa: mv88e6xxx: Allow the SERDES interfaces to have statistics

[Andrew Lunn]

When gettting the number of statistics, the strings and the actual
statistics, call the SERDES ops if implemented. This means the stats
code needs to return the number of strings/stats they have placed into
the data, so that the SERDES strings/stats can follow on.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c | 72 +++++++++++++++++++++++++++-------------
 drivers/net/dsa/mv88e6xxx/chip.h | 13 ++++++--
 2 files changed, 59 insertions(+), 26 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 12e274a3ff24..5274e8292451 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -627,8 +627,8 @@ static uint64_t _mv88e6xxx_get_ethtool_stat(struct mv88e6xxx_chip *chip,
 	return value;
 }
 
-static void mv88e6xxx_stats_get_strings(struct mv88e6xxx_chip *chip,
-					uint8_t *data, int types)
+static int mv88e6xxx_stats_get_strings(struct mv88e6xxx_chip *chip,
+				       uint8_t *data, int types)
 {
 	struct mv88e6xxx_hw_stat *stat;
 	int i, j;
@@ -641,31 +641,39 @@ static void mv88e6xxx_stats_get_strings(struct mv88e6xxx_chip *chip,
 			j++;
 		}
 	}
+
+	return j;
 }
 
-static void mv88e6095_stats_get_strings(struct mv88e6xxx_chip *chip,
-					uint8_t *data)
+static int mv88e6095_stats_get_strings(struct mv88e6xxx_chip *chip,
+				       uint8_t *data)
 {
-	mv88e6xxx_stats_get_strings(chip, data,
-				    STATS_TYPE_BANK0 | STATS_TYPE_PORT);
+	return mv88e6xxx_stats_get_strings(chip, data,
+					   STATS_TYPE_BANK0 | STATS_TYPE_PORT);
 }
 
-static void mv88e6320_stats_get_strings(struct mv88e6xxx_chip *chip,
-					uint8_t *data)
+static int mv88e6320_stats_get_strings(struct mv88e6xxx_chip *chip,
+				       uint8_t *data)
 {
-	mv88e6xxx_stats_get_strings(chip, data,
-				    STATS_TYPE_BANK0 | STATS_TYPE_BANK1);
+	return mv88e6xxx_stats_get_strings(chip, data,
+					   STATS_TYPE_BANK0 | STATS_TYPE_BANK1);
 }
 
 static void mv88e6xxx_get_strings(struct dsa_switch *ds, int port,
 				  uint8_t *data)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
+	int count = 0;
 
 	mutex_lock(&chip->reg_lock);
 
 	if (chip->info->ops->stats_get_strings)
-		chip->info->ops->stats_get_strings(chip, data);
+		count = chip->info->ops->stats_get_strings(chip, data);
+
+	if (chip->info->ops->serdes_get_strings) {
+		data += count * ETH_GSTRING_LEN;
+		chip->info->ops->serdes_get_strings(chip, port, data);
+	}
 
 	mutex_unlock(&chip->reg_lock);
 }
@@ -699,11 +707,21 @@ static int mv88e6320_stats_get_sset_count(struct mv88e6xxx_chip *chip)
 static int _mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
 {
 	struct mv88e6xxx_chip *chip = ds->priv;
+	int serdes_count = 0;
+	int count = 0;
 
 	if (chip->info->ops->stats_get_sset_count)
-		return chip->info->ops->stats_get_sset_count(chip);
+		count = chip->info->ops->stats_get_sset_count(chip);
+	if (count < 0)
+		return count;
 
-	return 0;
+	if (chip->info->ops->serdes_get_sset_count)
+		serdes_count = chip->info->ops->serdes_get_sset_count(chip,
+								      port);
+	if (serdes_count < 0)
+		return serdes_count;
+
+	return count + serdes_count;
 }
 
 static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
@@ -719,9 +737,9 @@ static int mv88e6xxx_get_sset_count(struct dsa_switch *ds, int port)
 }
 
 
-static void mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
-				      uint64_t *data, int types,
-				      u16 bank1_select, u16 histogram)
+static int mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
+				     uint64_t *data, int types,
+				     u16 bank1_select, u16 histogram)
 {
 	struct mv88e6xxx_hw_stat *stat;
 	int i, j;
@@ -735,18 +753,19 @@ static void mv88e6xxx_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
 			j++;
 		}
 	}
+	return j;
 }
 
-static void mv88e6095_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
-				      uint64_t *data)
+static int mv88e6095_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
+				     uint64_t *data)
 {
 	return mv88e6xxx_stats_get_stats(chip, port, data,
 					 STATS_TYPE_BANK0 | STATS_TYPE_PORT,
 					 0, MV88E6XXX_G1_STATS_OP_HIST_RX_TX);
 }
 
-static void mv88e6320_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
-				      uint64_t *data)
+static int mv88e6320_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
+				     uint64_t *data)
 {
 	return mv88e6xxx_stats_get_stats(chip, port, data,
 					 STATS_TYPE_BANK0 | STATS_TYPE_BANK1,
@@ -754,8 +773,8 @@ static void mv88e6320_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
 					 MV88E6XXX_G1_STATS_OP_HIST_RX_TX);
 }
 
-static void mv88e6390_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
-				      uint64_t *data)
+static int mv88e6390_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
+				     uint64_t *data)
 {
 	return mv88e6xxx_stats_get_stats(chip, port, data,
 					 STATS_TYPE_BANK0 | STATS_TYPE_BANK1,
@@ -766,8 +785,15 @@ static void mv88e6390_stats_get_stats(struct mv88e6xxx_chip *chip, int port,
 static void mv88e6xxx_get_stats(struct mv88e6xxx_chip *chip, int port,
 				uint64_t *data)
 {
+	int count = 0;
+
 	if (chip->info->ops->stats_get_stats)
-		chip->info->ops->stats_get_stats(chip, port, data);
+		count = chip->info->ops->stats_get_stats(chip, port, data);
+
+	if (chip->info->ops->serdes_get_stats) {
+		data += count;
+		chip->info->ops->serdes_get_stats(chip, port, data);
+	}
 }
 
 static void mv88e6xxx_get_ethtool_stats(struct dsa_switch *ds, int port,
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 334f6f7544ba..1787fc43167d 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -325,9 +325,9 @@ struct mv88e6xxx_ops {
 
 	/* Return the number of strings describing statistics */
 	int (*stats_get_sset_count)(struct mv88e6xxx_chip *chip);
-	void (*stats_get_strings)(struct mv88e6xxx_chip *chip,  uint8_t *data);
-	void (*stats_get_stats)(struct mv88e6xxx_chip *chip,  int port,
-				uint64_t *data);
+	int (*stats_get_strings)(struct mv88e6xxx_chip *chip,  uint8_t *data);
+	int (*stats_get_stats)(struct mv88e6xxx_chip *chip,  int port,
+			       uint64_t *data);
 	int (*set_cpu_port)(struct mv88e6xxx_chip *chip, int port);
 	int (*set_egress_port)(struct mv88e6xxx_chip *chip, int port);
 	const struct mv88e6xxx_irq_ops *watchdog_ops;
@@ -337,6 +337,13 @@ struct mv88e6xxx_ops {
 	/* Power on/off a SERDES interface */
 	int (*serdes_power)(struct mv88e6xxx_chip *chip, int port, bool on);
 
+	/* Statistics from the SERDES interface */
+	int (*serdes_get_sset_count)(struct mv88e6xxx_chip *chip, int port);
+	void (*serdes_get_strings)(struct mv88e6xxx_chip *chip,  int port,
+				   uint8_t *data);
+	void (*serdes_get_stats)(struct mv88e6xxx_chip *chip,  int port,
+				 uint64_t *data);
+
 	/* VLAN Translation Unit operations */
 	int (*vtu_getnext)(struct mv88e6xxx_chip *chip,
 			   struct mv88e6xxx_vtu_entry *entry);
-- 
2.15.1

Re: [PATCH net-next 3/5] net: dsa: mv88e6xxx: Allow the SERDES interfaces to have statistics

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> When gettting the number of statistics, the strings and the actual

       getting
       
> statistics, call the SERDES ops if implemented. This means the stats
> code needs to return the number of strings/stats they have placed into
> the data, so that the SERDES strings/stats can follow on.
>  	/* Return the number of strings describing statistics */
>  	int (*stats_get_sset_count)(struct mv88e6xxx_chip *chip);
> -	void (*stats_get_strings)(struct mv88e6xxx_chip *chip,  uint8_t *data);
> -	void (*stats_get_stats)(struct mv88e6xxx_chip *chip,  int port,
> -				uint64_t *data);
> +	int (*stats_get_strings)(struct mv88e6xxx_chip *chip,  uint8_t *data);
> +	int (*stats_get_stats)(struct mv88e6xxx_chip *chip,  int port,
> +			       uint64_t *data);
>  	int (*set_cpu_port)(struct mv88e6xxx_chip *chip, int port);
>  	int (*set_egress_port)(struct mv88e6xxx_chip *chip, int port);
>  	const struct mv88e6xxx_irq_ops *watchdog_ops;
> @@ -337,6 +337,13 @@ struct mv88e6xxx_ops {
>  	/* Power on/off a SERDES interface */
>  	int (*serdes_power)(struct mv88e6xxx_chip *chip, int port, bool on);
>  
> +	/* Statistics from the SERDES interface */
> +	int (*serdes_get_sset_count)(struct mv88e6xxx_chip *chip, int port);
> +	void (*serdes_get_strings)(struct mv88e6xxx_chip *chip,  int port,
> +				   uint8_t *data);
> +	void (*serdes_get_stats)(struct mv88e6xxx_chip *chip,  int port,
> +				 uint64_t *data);
> +

Shouldn't serdes_get_{strings,stats} be symmetrical with
stats_get_{strings,stats} and return the count as well?


Thanks,

        Vivien

[PATCH net-next 4/5] net: dsa: mv88e6xxx: Add helper to determining if port has SERDES

[Andrew Lunn]

Refactor the existing code. This helper will be used for SERDES
statistics.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/serdes.c | 22 +++++++++++++++++-----
 1 file changed, 17 insertions(+), 5 deletions(-)

diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index f3c01119b3d1..d32522276fea 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -55,18 +55,30 @@ static int mv88e6352_serdes_power_set(struct mv88e6xxx_chip *chip, bool on)
 	return err;
 }
 
-int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
+static int mv88e6352_port_has_serdes(struct mv88e6xxx_chip *chip, int port)
 {
-	int err;
 	u8 cmode;
+	int err;
 
 	err = mv88e6xxx_port_get_cmode(chip, port, &cmode);
-	if (err)
-		return err;
+	if (err) {
+		dev_err(chip->dev, "failed to read cmode\n");
+		return 0;
+	}
 
 	if ((cmode == MV88E6XXX_PORT_STS_CMODE_100BASE_X) ||
 	    (cmode == MV88E6XXX_PORT_STS_CMODE_1000BASE_X) ||
-	    (cmode == MV88E6XXX_PORT_STS_CMODE_SGMII)) {
+	    (cmode == MV88E6XXX_PORT_STS_CMODE_SGMII))
+		return 1;
+
+	return 0;
+}
+
+int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
+{
+	int err;
+
+	if (mv88e6352_port_has_serdes(chip, port)) {
 		err = mv88e6352_serdes_power_set(chip, on);
 		if (err < 0)
 			return err;
-- 
2.15.1

Re: [PATCH net-next 4/5] net: dsa: mv88e6xxx: Add helper to determining if port has SERDES

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> -int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
> +static int mv88e6352_port_has_serdes(struct mv88e6xxx_chip *chip, int port)
>  {
> -	int err;
>  	u8 cmode;
> +	int err;
>  
>  	err = mv88e6xxx_port_get_cmode(chip, port, &cmode);
> -	if (err)
> -		return err;
> +	if (err) {
> +		dev_err(chip->dev, "failed to read cmode\n");
> +		return 0;
> +	}
>  
>  	if ((cmode == MV88E6XXX_PORT_STS_CMODE_100BASE_X) ||
>  	    (cmode == MV88E6XXX_PORT_STS_CMODE_1000BASE_X) ||
> -	    (cmode == MV88E6XXX_PORT_STS_CMODE_SGMII)) {
> +	    (cmode == MV88E6XXX_PORT_STS_CMODE_SGMII))
> +		return 1;
> +
> +	return 0;
> +}

Please use a bool for such helpers.


Thanks,

        Vivien

[PATCH net-next 5/5] net: dsa: mv88e6xxx: Get mv88e6352 SERDES statistics

[Andrew Lunn]

Add support for reading the SERDES statistics of the mv88e8352, using
the standard ethtool -S option. The SERDES interface can be mapped to
either port 4 or 5, so only return statistics on those ports, if the
SERDES interface is in use.

The counters are reset on read, so need to be accumulated. Add a per
port structure to hold the stats counters. The 6352 only has a single
SERDES interface and so only one port will using the newly added
array. However the 6390 family has as many SERDES interfaces as ports,
each with statistics counters. Also, PTP has a number of counters per
port which will also need accumulating.

Signed-off-by: Andrew Lunn <andrew@lunn.ch>
Tested-by: Florian Fainelli <f.fainelli@gmail.com>
---
 drivers/net/dsa/mv88e6xxx/chip.c   |  3 ++
 drivers/net/dsa/mv88e6xxx/chip.h   |  7 ++++
 drivers/net/dsa/mv88e6xxx/serdes.c | 84 ++++++++++++++++++++++++++++++++++++++
 drivers/net/dsa/mv88e6xxx/serdes.h |  6 ++-
 4 files changed, 99 insertions(+), 1 deletion(-)

diff --git a/drivers/net/dsa/mv88e6xxx/chip.c b/drivers/net/dsa/mv88e6xxx/chip.c
index 5274e8292451..a335ef2f1087 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.c
+++ b/drivers/net/dsa/mv88e6xxx/chip.c
@@ -3129,6 +3129,9 @@ static const struct mv88e6xxx_ops mv88e6352_ops = {
 	.vtu_getnext = mv88e6352_g1_vtu_getnext,
 	.vtu_loadpurge = mv88e6352_g1_vtu_loadpurge,
 	.serdes_power = mv88e6352_serdes_power,
+	.serdes_get_sset_count = mv88e6352_serdes_get_sset_count,
+	.serdes_get_strings = mv88e6352_serdes_get_strings,
+	.serdes_get_stats = mv88e6352_serdes_get_stats,
 };
 
 static const struct mv88e6xxx_ops mv88e6390_ops = {
diff --git a/drivers/net/dsa/mv88e6xxx/chip.h b/drivers/net/dsa/mv88e6xxx/chip.h
index 1787fc43167d..c5accbb84eea 100644
--- a/drivers/net/dsa/mv88e6xxx/chip.h
+++ b/drivers/net/dsa/mv88e6xxx/chip.h
@@ -154,6 +154,10 @@ struct mv88e6xxx_irq {
 	unsigned int nirqs;
 };
 
+struct mv88e6xxx_port {
+	u64 serdes_stats[2];
+};
+
 struct mv88e6xxx_chip {
 	const struct mv88e6xxx_info *info;
 
@@ -207,6 +211,9 @@ struct mv88e6xxx_chip {
 	int irq;
 	int device_irq;
 	int watchdog_irq;
+
+	/* Array of port structures. */
+	struct mv88e6xxx_port ports[DSA_MAX_PORTS];
 };
 
 struct mv88e6xxx_bus_ops {
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
index d32522276fea..3b911b80da33 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.c
+++ b/drivers/net/dsa/mv88e6xxx/serdes.c
@@ -87,6 +87,90 @@ int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
 	return 0;
 }
 
+struct mv88e6352_serdes_hw_stat {
+	char string[ETH_GSTRING_LEN];
+	int sizeof_stat;
+	int reg;
+};
+
+static struct mv88e6352_serdes_hw_stat mv88e6352_serdes_hw_stats[] = {
+	{ "serdes_fibre_rx_error", 16, 21 },
+	{ "serdes_PRBS_error", 32, 24 },
+};
+
+int mv88e6352_serdes_get_sset_count(struct mv88e6xxx_chip *chip, int port)
+{
+	if (mv88e6352_port_has_serdes(chip, port))
+		return ARRAY_SIZE(mv88e6352_serdes_hw_stats);
+
+	return 0;
+}
+
+void mv88e6352_serdes_get_strings(struct mv88e6xxx_chip *chip,
+				  int port, uint8_t *data)
+{
+	struct mv88e6352_serdes_hw_stat *stat;
+	int i;
+
+	if (!mv88e6352_port_has_serdes(chip, port))
+		return;
+
+	for (i = 0; i < ARRAY_SIZE(mv88e6352_serdes_hw_stats); i++) {
+		stat = &mv88e6352_serdes_hw_stats[i];
+		memcpy(data + i * ETH_GSTRING_LEN, stat->string,
+		       ETH_GSTRING_LEN);
+	}
+}
+
+static uint64_t mv88e6352_serdes_get_stat(struct mv88e6xxx_chip *chip,
+					  struct mv88e6352_serdes_hw_stat *stat)
+{
+	u64 val = 0;
+	u16 reg;
+	int err;
+
+	err = mv88e6352_serdes_read(chip, stat->reg, &reg);
+	if (err) {
+		dev_err(chip->dev, "failed to read statistic\n");
+		return 0;
+	}
+
+	val = reg;
+
+	if (stat->sizeof_stat == 32) {
+		err = mv88e6352_serdes_read(chip, stat->reg + 1, &reg);
+		if (err) {
+			dev_err(chip->dev, "failed to read statistic\n");
+			return 0;
+		}
+		val = val << 16 | reg;
+	}
+
+	return val;
+}
+
+void mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
+				uint64_t *data)
+{
+	struct mv88e6xxx_port *mv88e6xxx_port = &chip->ports[port];
+	struct mv88e6352_serdes_hw_stat *stat;
+	u64 value;
+	int i;
+
+	if (!mv88e6352_port_has_serdes(chip, port))
+		return;
+
+	BUILD_BUG_ON(ARRAY_SIZE(mv88e6352_serdes_hw_stats) >
+		     ARRAY_SIZE(mv88e6xxx_port->serdes_stats));
+
+	for (i = 0; i < ARRAY_SIZE(mv88e6352_serdes_hw_stats); i++) {
+		stat = &mv88e6352_serdes_hw_stats[i];
+		value  = mv88e6352_serdes_get_stat(chip, stat);
+		mv88e6xxx_port->serdes_stats[i] += value;
+		data[i] = mv88e6xxx_port->serdes_stats[i];
+	}
+}
+
 /* Set the power on/off for 10GBASE-R and 10GBASE-X4/X2 */
 static int mv88e6390_serdes_10g(struct mv88e6xxx_chip *chip, int addr, bool on)
 {
diff --git a/drivers/net/dsa/mv88e6xxx/serdes.h b/drivers/net/dsa/mv88e6xxx/serdes.h
index 5c1cd6d8e9a5..641baa75f910 100644
--- a/drivers/net/dsa/mv88e6xxx/serdes.h
+++ b/drivers/net/dsa/mv88e6xxx/serdes.h
@@ -44,5 +44,9 @@
 
 int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on);
 int mv88e6390_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on);
-
+int mv88e6352_serdes_get_sset_count(struct mv88e6xxx_chip *chip, int port);
+void mv88e6352_serdes_get_strings(struct mv88e6xxx_chip *chip,
+				  int port, uint8_t *data);
+void mv88e6352_serdes_get_stats(struct mv88e6xxx_chip *chip, int port,
+				uint64_t *data);
 #endif
-- 
2.15.1

Re: [PATCH net-next 5/5] net: dsa: mv88e6xxx: Get mv88e6352 SERDES statistics

[Vivien Didelot]

Hi Andrew,

Andrew Lunn <andrew@lunn.ch> writes:

> +struct mv88e6xxx_port {
> +	u64 serdes_stats[2];
> +};
> +
>  struct mv88e6xxx_chip {
>  	const struct mv88e6xxx_info *info;
>  
> @@ -207,6 +211,9 @@ struct mv88e6xxx_chip {
>  	int irq;
>  	int device_irq;
>  	int watchdog_irq;
> +
> +	/* Array of port structures. */
> +	struct mv88e6xxx_port ports[DSA_MAX_PORTS];
>  };

We are trying to get rid of these global DSA limitations for ports and
switches and support dynamic values. It wasn't mentioned but I assume
you couldn't use a zero length array here because we allocate before
detecting the switch model. Please define MV88E6XXX_MAX_PORTS to 16
instead, which is indeed a Marvell limitation for SOHO devices.

>  
>  struct mv88e6xxx_bus_ops {
> diff --git a/drivers/net/dsa/mv88e6xxx/serdes.c b/drivers/net/dsa/mv88e6xxx/serdes.c
> index d32522276fea..3b911b80da33 100644
> --- a/drivers/net/dsa/mv88e6xxx/serdes.c
> +++ b/drivers/net/dsa/mv88e6xxx/serdes.c
> @@ -87,6 +87,90 @@ int mv88e6352_serdes_power(struct mv88e6xxx_chip *chip, int port, bool on)
>  	return 0;
>  }
>  
> +struct mv88e6352_serdes_hw_stat {
> +	char string[ETH_GSTRING_LEN];
> +	int sizeof_stat;

You can simply define size_t size.

> +	int reg;
> +};
> +
> +static struct mv88e6352_serdes_hw_stat mv88e6352_serdes_hw_stats[] = {
> +	{ "serdes_fibre_rx_error", 16, 21 },
> +	{ "serdes_PRBS_error", 32, 24 },
> +};

<...>

> +	BUILD_BUG_ON(ARRAY_SIZE(mv88e6352_serdes_hw_stats) >
> +		     ARRAY_SIZE(mv88e6xxx_port->serdes_stats));
> +
> +	for (i = 0; i < ARRAY_SIZE(mv88e6352_serdes_hw_stats); i++) {
> +		stat = &mv88e6352_serdes_hw_stats[i];
> +		value  = mv88e6352_serdes_get_stat(chip, stat);

                      extra space

> +		mv88e6xxx_port->serdes_stats[i] += value;
> +		data[i] = mv88e6xxx_port->serdes_stats[i];
> +	}
> +}

Re: [PATCH net-next 0/5] Export SERDES stats via ethtool -S

[Russell King - ARM Linux]

On Wed, Jan 03, 2018 at 03:09:17PM +0100, Andrew Lunn wrote:
> The mv88e6352 family has a SERDES interface which can be used for
> example to connect to SFF/SFP modules. This interface has a couple of
> statistics counters. Add support for including these counters in the
> output of ethtool -S.

Hi Andrew,

Isn't the serdes interface just a normal PHY - the register set in
Viviens debugfs patch certainly looks like an 88e1545 in one of the
switches I've here (I think on the dev rev B).  Is there a reason why
this PHY isn't visible as a normal PHY, just like we export the other
internal PHYs?

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up
According to speedtest.net: 8.21Mbps down 510kbps up

Re: [PATCH net-next 0/5] Export SERDES stats via ethtool -S

[Andrew Lunn]

> Hi Andrew,
> 
> Isn't the serdes interface just a normal PHY - the register set in
> Viviens debugfs patch certainly looks like an 88e1545 in one of the
> switches I've here (I think on the dev rev B).  Is there a reason why
> this PHY isn't visible as a normal PHY, just like we export the other
> internal PHYs?

Humm, interesting idea. Yes, it looks like a normal PHY. It is however
not contiguous to the internal PHYs on the 6352. You probably can
however see it, if you look on the MDIO bus, at address 0xf.

Depends on what mode the ports are in, the SERDES can be connected to
either port 4 or 5. Port 5 does not have an internal PHY, but port 4
does. We could determine what port is using the SERDES, and redirect
PHY read/writes to the SERDES interface. But it gets a bit complex for
port 4, since it can use either copper or SERDES depending on which
comes up first. So suppose you need to be able to see both the copper
PHY and the SERDES, so you can initialize them both, in order for one
to actually come up.

   Andrew