[PATCH v5 net-next 0/3] use bulk reads for ocelot statistics

[PATCH v5 net-next 0/3] use bulk reads for ocelot statistics

[Colin Foster]

Ocelot loops over memory regions to gather stats on different ports.
These regions are mostly continuous, and are ordered. This patch set
uses that information to break the stats reads into regions that can get
read in bulk.

The motiviation is for general cleanup, but also for SPI. Performing two
back-to-back reads on a SPI bus require toggling the CS line, holding,
re-toggling the CS line, sending 3 address bytes, sending N padding
bytes, then actually performing the read. Bulk reads could reduce almost
all of that overhead, but require that the reads are performed via
regmap_bulk_read.

v1 > v2: reword commit messages
v2 > v3: correctly mark this for net-next when sending
v3 > v4: calloc array instead of zalloc per review
v4 > v5:
    Apply CR suggestions for whitespace
    Fix calloc / zalloc mixup
    Properly destroy workqueues
    Add third commit to split long macros


Colin Foster (3):
  net: ocelot: align macros for consistency
  net: mscc: ocelot: add ability to perform bulk reads
  net: mscc: ocelot: use bulk reads for stats

 drivers/net/ethernet/mscc/ocelot.c    | 78 ++++++++++++++++++++++-----
 drivers/net/ethernet/mscc/ocelot_io.c | 13 +++++
 include/soc/mscc/ocelot.h             | 57 ++++++++++++++------
 3 files changed, 120 insertions(+), 28 deletions(-)

-- 
2.25.1

[PATCH v5 net-next 1/3] net: ocelot: align macros for consistency

[Colin Foster]

In the ocelot.h file, several read / write macros were split across
multiple lines, while others weren't. Split all macros that exceed the 80
character column width and match the style of the rest of the file.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 include/soc/mscc/ocelot.h | 44 ++++++++++++++++++++++++++-------------
 1 file changed, 29 insertions(+), 15 deletions(-)

diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 62cd61d4142e..14a6f4de8e1f 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -744,25 +744,39 @@ struct ocelot_policer {
 	u32 burst; /* bytes */
 };
 
-#define ocelot_read_ix(ocelot, reg, gi, ri) __ocelot_read_ix(ocelot, reg, reg##_GSZ * (gi) + reg##_RSZ * (ri))
-#define ocelot_read_gix(ocelot, reg, gi) __ocelot_read_ix(ocelot, reg, reg##_GSZ * (gi))
-#define ocelot_read_rix(ocelot, reg, ri) __ocelot_read_ix(ocelot, reg, reg##_RSZ * (ri))
-#define ocelot_read(ocelot, reg) __ocelot_read_ix(ocelot, reg, 0)
-
-#define ocelot_write_ix(ocelot, val, reg, gi, ri) __ocelot_write_ix(ocelot, val, reg, reg##_GSZ * (gi) + reg##_RSZ * (ri))
-#define ocelot_write_gix(ocelot, val, reg, gi) __ocelot_write_ix(ocelot, val, reg, reg##_GSZ * (gi))
-#define ocelot_write_rix(ocelot, val, reg, ri) __ocelot_write_ix(ocelot, val, reg, reg##_RSZ * (ri))
+#define ocelot_read_ix(ocelot, reg, gi, ri) \
+	__ocelot_read_ix(ocelot, reg, reg##_GSZ * (gi) + reg##_RSZ * (ri))
+#define ocelot_read_gix(ocelot, reg, gi) \
+	__ocelot_read_ix(ocelot, reg, reg##_GSZ * (gi))
+#define ocelot_read_rix(ocelot, reg, ri) \
+	__ocelot_read_ix(ocelot, reg, reg##_RSZ * (ri))
+#define ocelot_read(ocelot, reg) \
+	__ocelot_read_ix(ocelot, reg, 0)
+
+#define ocelot_write_ix(ocelot, val, reg, gi, ri) \
+	__ocelot_write_ix(ocelot, val, reg, reg##_GSZ * (gi) + reg##_RSZ * (ri))
+#define ocelot_write_gix(ocelot, val, reg, gi) \
+	__ocelot_write_ix(ocelot, val, reg, reg##_GSZ * (gi))
+#define ocelot_write_rix(ocelot, val, reg, ri) \
+	__ocelot_write_ix(ocelot, val, reg, reg##_RSZ * (ri))
 #define ocelot_write(ocelot, val, reg) __ocelot_write_ix(ocelot, val, reg, 0)
 
-#define ocelot_rmw_ix(ocelot, val, m, reg, gi, ri) __ocelot_rmw_ix(ocelot, val, m, reg, reg##_GSZ * (gi) + reg##_RSZ * (ri))
-#define ocelot_rmw_gix(ocelot, val, m, reg, gi) __ocelot_rmw_ix(ocelot, val, m, reg, reg##_GSZ * (gi))
-#define ocelot_rmw_rix(ocelot, val, m, reg, ri) __ocelot_rmw_ix(ocelot, val, m, reg, reg##_RSZ * (ri))
+#define ocelot_rmw_ix(ocelot, val, m, reg, gi, ri) \
+	__ocelot_rmw_ix(ocelot, val, m, reg, reg##_GSZ * (gi) + reg##_RSZ * (ri))
+#define ocelot_rmw_gix(ocelot, val, m, reg, gi) \
+	__ocelot_rmw_ix(ocelot, val, m, reg, reg##_GSZ * (gi))
+#define ocelot_rmw_rix(ocelot, val, m, reg, ri) \
+	__ocelot_rmw_ix(ocelot, val, m, reg, reg##_RSZ * (ri))
 #define ocelot_rmw(ocelot, val, m, reg) __ocelot_rmw_ix(ocelot, val, m, reg, 0)
 
-#define ocelot_field_write(ocelot, reg, val) regmap_field_write((ocelot)->regfields[(reg)], (val))
-#define ocelot_field_read(ocelot, reg, val) regmap_field_read((ocelot)->regfields[(reg)], (val))
-#define ocelot_fields_write(ocelot, id, reg, val) regmap_fields_write((ocelot)->regfields[(reg)], (id), (val))
-#define ocelot_fields_read(ocelot, id, reg, val) regmap_fields_read((ocelot)->regfields[(reg)], (id), (val))
+#define ocelot_field_write(ocelot, reg, val) \
+	regmap_field_write((ocelot)->regfields[(reg)], (val))
+#define ocelot_field_read(ocelot, reg, val) \
+	regmap_field_read((ocelot)->regfields[(reg)], (val))
+#define ocelot_fields_write(ocelot, id, reg, val) \
+	regmap_fields_write((ocelot)->regfields[(reg)], (id), (val))
+#define ocelot_fields_read(ocelot, id, reg, val) \
+	regmap_fields_read((ocelot)->regfields[(reg)], (id), (val))
 
 #define ocelot_target_read_ix(ocelot, target, reg, gi, ri) \
 	__ocelot_target_read_ix(ocelot, target, reg, reg##_GSZ * (gi) + reg##_RSZ * (ri))
-- 
2.25.1

[PATCH v5 net-next 2/3] net: mscc: ocelot: add ability to perform bulk reads

[Colin Foster]

Regmap supports bulk register reads. Ocelot does not. This patch adds
support for Ocelot to invoke bulk regmap reads. That will allow any driver
that performs consecutive reads over memory regions to optimize that
access.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/net/ethernet/mscc/ocelot_io.c | 13 +++++++++++++
 include/soc/mscc/ocelot.h             |  5 +++++
 2 files changed, 18 insertions(+)

diff --git a/drivers/net/ethernet/mscc/ocelot_io.c b/drivers/net/ethernet/mscc/ocelot_io.c
index 7390fa3980ec..2067382d0ee1 100644
--- a/drivers/net/ethernet/mscc/ocelot_io.c
+++ b/drivers/net/ethernet/mscc/ocelot_io.c
@@ -10,6 +10,19 @@
 
 #include "ocelot.h"
 
+int __ocelot_bulk_read_ix(struct ocelot *ocelot, u32 reg, u32 offset, void *buf,
+			  int count)
+{
+	u16 target = reg >> TARGET_OFFSET;
+
+	WARN_ON(!target);
+
+	return regmap_bulk_read(ocelot->targets[target],
+				ocelot->map[target][reg & REG_MASK] + offset,
+				buf, count);
+}
+EXPORT_SYMBOL_GPL(__ocelot_bulk_read_ix);
+
 u32 __ocelot_read_ix(struct ocelot *ocelot, u32 reg, u32 offset)
 {
 	u16 target = reg >> TARGET_OFFSET;
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 14a6f4de8e1f..312b72558659 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -744,6 +744,9 @@ struct ocelot_policer {
 	u32 burst; /* bytes */
 };
 
+#define ocelot_bulk_read_rix(ocelot, reg, ri, buf, count) \
+	__ocelot_bulk_read_ix(ocelot, reg, reg##_RSZ * (ri), buf, count)
+
 #define ocelot_read_ix(ocelot, reg, gi, ri) \
 	__ocelot_read_ix(ocelot, reg, reg##_GSZ * (gi) + reg##_RSZ * (ri))
 #define ocelot_read_gix(ocelot, reg, gi) \
@@ -800,6 +803,8 @@ struct ocelot_policer {
 u32 ocelot_port_readl(struct ocelot_port *port, u32 reg);
 void ocelot_port_writel(struct ocelot_port *port, u32 val, u32 reg);
 void ocelot_port_rmwl(struct ocelot_port *port, u32 val, u32 mask, u32 reg);
+int __ocelot_bulk_read_ix(struct ocelot *ocelot, u32 reg, u32 offset, void *buf,
+			  int count);
 u32 __ocelot_read_ix(struct ocelot *ocelot, u32 reg, u32 offset);
 void __ocelot_write_ix(struct ocelot *ocelot, u32 val, u32 reg, u32 offset);
 void __ocelot_rmw_ix(struct ocelot *ocelot, u32 val, u32 mask, u32 reg,
-- 
2.25.1

[PATCH v5 net-next 3/3] net: mscc: ocelot: use bulk reads for stats

[Colin Foster]

Create and utilize bulk regmap reads instead of single access for gathering
stats. The background reading of statistics happens frequently, and over
a few contiguous memory regions.

High speed PCIe buses and MMIO access will probably see negligible
performance increase. Lower speed buses like SPI and I2C could see
significant performance increase, since the bus configuration and register
access times account for a large percentage of data transfer time.

Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
---
 drivers/net/ethernet/mscc/ocelot.c | 78 +++++++++++++++++++++++++-----
 include/soc/mscc/ocelot.h          |  8 +++
 2 files changed, 73 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
index 455293aa6343..5efb1f3a1410 100644
--- a/drivers/net/ethernet/mscc/ocelot.c
+++ b/drivers/net/ethernet/mscc/ocelot.c
@@ -1737,32 +1737,41 @@ void ocelot_get_strings(struct ocelot *ocelot, int port, u32 sset, u8 *data)
 }
 EXPORT_SYMBOL(ocelot_get_strings);
 
-static void ocelot_update_stats(struct ocelot *ocelot)
+static int ocelot_update_stats(struct ocelot *ocelot)
 {
-	int i, j;
+	struct ocelot_stats_region *region;
+	int i, j, err = 0;
 
 	mutex_lock(&ocelot->stats_lock);
 
 	for (i = 0; i < ocelot->num_phys_ports; i++) {
+		unsigned int idx = 0;
+
 		/* Configure the port to read the stats from */
 		ocelot_write(ocelot, SYS_STAT_CFG_STAT_VIEW(i), SYS_STAT_CFG);
 
-		for (j = 0; j < ocelot->num_stats; j++) {
-			u32 val;
-			unsigned int idx = i * ocelot->num_stats + j;
+		list_for_each_entry(region, &ocelot->stats_regions, node) {
+			err = ocelot_bulk_read_rix(ocelot, SYS_COUNT_RX_OCTETS,
+						   region->offset, region->buf,
+						   region->count);
+			if (err)
+				goto out;
 
-			val = ocelot_read_rix(ocelot, SYS_COUNT_RX_OCTETS,
-					      ocelot->stats_layout[j].offset);
+			for (j = 0; j < region->count; j++) {
+				if (region->buf[j] < (ocelot->stats[idx + j] & U32_MAX))
+					ocelot->stats[idx + j] += (u64)1 << 32;
 
-			if (val < (ocelot->stats[idx] & U32_MAX))
-				ocelot->stats[idx] += (u64)1 << 32;
+				ocelot->stats[idx + j] = (ocelot->stats[idx + j] &
+							~(u64)U32_MAX) + region->buf[j];
+			}
 
-			ocelot->stats[idx] = (ocelot->stats[idx] &
-					      ~(u64)U32_MAX) + val;
+			idx += region->count;
 		}
 	}
 
+out:
 	mutex_unlock(&ocelot->stats_lock);
+	return err;
 }
 
 static void ocelot_check_stats_work(struct work_struct *work)
@@ -1779,10 +1788,11 @@ static void ocelot_check_stats_work(struct work_struct *work)
 
 void ocelot_get_ethtool_stats(struct ocelot *ocelot, int port, u64 *data)
 {
-	int i;
+	int i, err;
 
 	/* check and update now */
-	ocelot_update_stats(ocelot);
+	err = ocelot_update_stats(ocelot);
+	WARN_ONCE(err, "Error %d updating ethtool stats\n", err);
 
 	/* Copy all counters */
 	for (i = 0; i < ocelot->num_stats; i++)
@@ -1799,6 +1809,41 @@ int ocelot_get_sset_count(struct ocelot *ocelot, int port, int sset)
 }
 EXPORT_SYMBOL(ocelot_get_sset_count);
 
+static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
+{
+	struct ocelot_stats_region *region = NULL;
+	unsigned int last;
+	int i;
+
+	INIT_LIST_HEAD(&ocelot->stats_regions);
+
+	for (i = 0; i < ocelot->num_stats; i++) {
+		if (region && ocelot->stats_layout[i].offset == last + 1) {
+			region->count++;
+		} else {
+			region = devm_kzalloc(ocelot->dev, sizeof(*region),
+					      GFP_KERNEL);
+			if (!region)
+				return -ENOMEM;
+
+			region->offset = ocelot->stats_layout[i].offset;
+			region->count = 1;
+			list_add_tail(&region->node, &ocelot->stats_regions);
+		}
+
+		last = ocelot->stats_layout[i].offset;
+	}
+
+	list_for_each_entry(region, &ocelot->stats_regions, node) {
+		region->buf = devm_kcalloc(ocelot->dev, region->count,
+					   sizeof(*region->buf), GFP_KERNEL);
+		if (!region->buf)
+			return -ENOMEM;
+	}
+
+	return 0;
+}
+
 int ocelot_get_ts_info(struct ocelot *ocelot, int port,
 		       struct ethtool_ts_info *info)
 {
@@ -2799,6 +2844,13 @@ int ocelot_init(struct ocelot *ocelot)
 				 ANA_CPUQ_8021_CFG_CPUQ_BPDU_VAL(6),
 				 ANA_CPUQ_8021_CFG, i);
 
+	ret = ocelot_prepare_stats_regions(ocelot);
+	if (ret) {
+		destroy_workqueue(ocelot->stats_queue);
+		destroy_workqueue(ocelot->owq);
+		return ret;
+	}
+
 	INIT_DELAYED_WORK(&ocelot->stats_work, ocelot_check_stats_work);
 	queue_delayed_work(ocelot->stats_queue, &ocelot->stats_work,
 			   OCELOT_STATS_CHECK_DELAY);
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index 312b72558659..d3291a5f7e88 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -542,6 +542,13 @@ struct ocelot_stat_layout {
 	char name[ETH_GSTRING_LEN];
 };
 
+struct ocelot_stats_region {
+	struct list_head node;
+	u32 offset;
+	int count;
+	u32 *buf;
+};
+
 enum ocelot_tag_prefix {
 	OCELOT_TAG_PREFIX_DISABLED	= 0,
 	OCELOT_TAG_PREFIX_NONE,
@@ -673,6 +680,7 @@ struct ocelot {
 	struct regmap_field		*regfields[REGFIELD_MAX];
 	const u32 *const		*map;
 	const struct ocelot_stat_layout	*stats_layout;
+	struct list_head		stats_regions;
 	unsigned int			num_stats;
 
 	u32				pool_size[OCELOT_SB_NUM][OCELOT_SB_POOL_NUM];
-- 
2.25.1

Re: [PATCH v5 net-next 1/3] net: ocelot: align macros for consistency

[Vladimir Oltean]

On Mon, Feb 07, 2022 at 08:46:42PM -0800, Colin Foster wrote:
> In the ocelot.h file, several read / write macros were split across
> multiple lines, while others weren't. Split all macros that exceed the 80
> character column width and match the style of the rest of the file.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH v5 net-next 2/3] net: mscc: ocelot: add ability to perform bulk reads

[Vladimir Oltean]

On Mon, Feb 07, 2022 at 08:46:43PM -0800, Colin Foster wrote:
> Regmap supports bulk register reads. Ocelot does not. This patch adds
> support for Ocelot to invoke bulk regmap reads. That will allow any driver
> that performs consecutive reads over memory regions to optimize that
> access.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Re: [PATCH v5 net-next 3/3] net: mscc: ocelot: use bulk reads for stats

[Vladimir Oltean]

On Mon, Feb 07, 2022 at 08:46:44PM -0800, Colin Foster wrote:
> Create and utilize bulk regmap reads instead of single access for gathering
> stats. The background reading of statistics happens frequently, and over
> a few contiguous memory regions.
> 
> High speed PCIe buses and MMIO access will probably see negligible
> performance increase. Lower speed buses like SPI and I2C could see
> significant performance increase, since the bus configuration and register
> access times account for a large percentage of data transfer time.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---

Reviewed-by: Vladimir Oltean <vladimir.oltean@nxp.com>

Just one minor comment below, but all in all it's fine as is, too.

>  drivers/net/ethernet/mscc/ocelot.c | 78 +++++++++++++++++++++++++-----
>  include/soc/mscc/ocelot.h          |  8 +++
>  2 files changed, 73 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 455293aa6343..5efb1f3a1410 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -1737,32 +1737,41 @@ void ocelot_get_strings(struct ocelot *ocelot, int port, u32 sset, u8 *data)
>  }
>  EXPORT_SYMBOL(ocelot_get_strings);
>  
> -static void ocelot_update_stats(struct ocelot *ocelot)
> +static int ocelot_update_stats(struct ocelot *ocelot)
>  {
> -	int i, j;
> +	struct ocelot_stats_region *region;
> +	int i, j, err = 0;
>  
>  	mutex_lock(&ocelot->stats_lock);
>  
>  	for (i = 0; i < ocelot->num_phys_ports; i++) {
> +		unsigned int idx = 0;
> +
>  		/* Configure the port to read the stats from */
>  		ocelot_write(ocelot, SYS_STAT_CFG_STAT_VIEW(i), SYS_STAT_CFG);
>  
> -		for (j = 0; j < ocelot->num_stats; j++) {
> -			u32 val;
> -			unsigned int idx = i * ocelot->num_stats + j;
> +		list_for_each_entry(region, &ocelot->stats_regions, node) {
> +			err = ocelot_bulk_read_rix(ocelot, SYS_COUNT_RX_OCTETS,
> +						   region->offset, region->buf,
> +						   region->count);
> +			if (err)
> +				goto out;
>  
> -			val = ocelot_read_rix(ocelot, SYS_COUNT_RX_OCTETS,
> -					      ocelot->stats_layout[j].offset);
> +			for (j = 0; j < region->count; j++) {
> +				if (region->buf[j] < (ocelot->stats[idx + j] & U32_MAX))
> +					ocelot->stats[idx + j] += (u64)1 << 32;
>  
> -			if (val < (ocelot->stats[idx] & U32_MAX))
> -				ocelot->stats[idx] += (u64)1 << 32;
> +				ocelot->stats[idx + j] = (ocelot->stats[idx + j] &
> +							~(u64)U32_MAX) + region->buf[j];
> +			}
>  
> -			ocelot->stats[idx] = (ocelot->stats[idx] &
> -					      ~(u64)U32_MAX) + val;
> +			idx += region->count;
>  		}
>  	}
>  
> +out:
>  	mutex_unlock(&ocelot->stats_lock);
> +	return err;
>  }
>  
>  static void ocelot_check_stats_work(struct work_struct *work)
> @@ -1779,10 +1788,11 @@ static void ocelot_check_stats_work(struct work_struct *work)
>  
>  void ocelot_get_ethtool_stats(struct ocelot *ocelot, int port, u64 *data)
>  {
> -	int i;
> +	int i, err;
>  
>  	/* check and update now */
> -	ocelot_update_stats(ocelot);
> +	err = ocelot_update_stats(ocelot);
> +	WARN_ONCE(err, "Error %d updating ethtool stats\n", err);

I think a dev_err(ocelot->dev, ...) would be more appropriate here.
WARN_ON() should be used for truly critical errors (things that should
never happen unless bugs are present). When the assertion holds true, a
WARN_ON() will print a stack trace, and when "panic_on_warn" is enabled,
a WARN() is effectively a BUG() and crashes the kernel.

include/asm-generic/bug.h says:

/*
 * WARN(), WARN_ON(), WARN_ON_ONCE, and so on can be used to report
 * significant kernel issues that need prompt attention if they should ever
 * appear at runtime.
 *
 * Do not use these macros when checking for invalid external inputs
 * (e.g. invalid system call arguments, or invalid data coming from
 * network/devices), and on transient conditions like ENOMEM or EAGAIN.
 * These macros should be used for recoverable kernel issues only.
 * For invalid external inputs, transient conditions, etc use
 * pr_err[_once/_ratelimited]() followed by dump_stack(), if necessary.
 * Do not include "BUG"/"WARNING" in format strings manually to make these
 * conditions distinguishable from kernel issues.
 *
 * Use the versions with printk format strings to provide better diagnostics.
 */

>  
>  	/* Copy all counters */
>  	for (i = 0; i < ocelot->num_stats; i++)
> @@ -1799,6 +1809,41 @@ int ocelot_get_sset_count(struct ocelot *ocelot, int port, int sset)
>  }
>  EXPORT_SYMBOL(ocelot_get_sset_count);
>  
> +static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
> +{
> +	struct ocelot_stats_region *region = NULL;
> +	unsigned int last;
> +	int i;
> +
> +	INIT_LIST_HEAD(&ocelot->stats_regions);
> +
> +	for (i = 0; i < ocelot->num_stats; i++) {
> +		if (region && ocelot->stats_layout[i].offset == last + 1) {
> +			region->count++;
> +		} else {
> +			region = devm_kzalloc(ocelot->dev, sizeof(*region),
> +					      GFP_KERNEL);
> +			if (!region)
> +				return -ENOMEM;
> +
> +			region->offset = ocelot->stats_layout[i].offset;
> +			region->count = 1;
> +			list_add_tail(&region->node, &ocelot->stats_regions);
> +		}
> +
> +		last = ocelot->stats_layout[i].offset;
> +	}
> +
> +	list_for_each_entry(region, &ocelot->stats_regions, node) {
> +		region->buf = devm_kcalloc(ocelot->dev, region->count,
> +					   sizeof(*region->buf), GFP_KERNEL);
> +		if (!region->buf)
> +			return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
>  int ocelot_get_ts_info(struct ocelot *ocelot, int port,
>  		       struct ethtool_ts_info *info)
>  {
> @@ -2799,6 +2844,13 @@ int ocelot_init(struct ocelot *ocelot)
>  				 ANA_CPUQ_8021_CFG_CPUQ_BPDU_VAL(6),
>  				 ANA_CPUQ_8021_CFG, i);
>  
> +	ret = ocelot_prepare_stats_regions(ocelot);
> +	if (ret) {
> +		destroy_workqueue(ocelot->stats_queue);
> +		destroy_workqueue(ocelot->owq);
> +		return ret;
> +	}
> +
>  	INIT_DELAYED_WORK(&ocelot->stats_work, ocelot_check_stats_work);
>  	queue_delayed_work(ocelot->stats_queue, &ocelot->stats_work,
>  			   OCELOT_STATS_CHECK_DELAY);
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index 312b72558659..d3291a5f7e88 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -542,6 +542,13 @@ struct ocelot_stat_layout {
>  	char name[ETH_GSTRING_LEN];
>  };
>  
> +struct ocelot_stats_region {
> +	struct list_head node;
> +	u32 offset;
> +	int count;
> +	u32 *buf;
> +};
> +
>  enum ocelot_tag_prefix {
>  	OCELOT_TAG_PREFIX_DISABLED	= 0,
>  	OCELOT_TAG_PREFIX_NONE,
> @@ -673,6 +680,7 @@ struct ocelot {
>  	struct regmap_field		*regfields[REGFIELD_MAX];
>  	const u32 *const		*map;
>  	const struct ocelot_stat_layout	*stats_layout;
> +	struct list_head		stats_regions;
>  	unsigned int			num_stats;
>  
>  	u32				pool_size[OCELOT_SB_NUM][OCELOT_SB_POOL_NUM];
> -- 
> 2.25.1
>

Re: [PATCH v5 net-next 0/3] use bulk reads for ocelot statistics

[Vladimir Oltean]

On Mon, Feb 07, 2022 at 08:46:41PM -0800, Colin Foster wrote:
> Ocelot loops over memory regions to gather stats on different ports.
> These regions are mostly continuous, and are ordered. This patch set
> uses that information to break the stats reads into regions that can get
> read in bulk.
>
> The motiviation is for general cleanup, but also for SPI. Performing two
> back-to-back reads on a SPI bus require toggling the CS line, holding,
> re-toggling the CS line, sending 3 address bytes, sending N padding
> bytes, then actually performing the read. Bulk reads could reduce almost
> all of that overhead, but require that the reads are performed via
> regmap_bulk_read.
>
> v1 > v2: reword commit messages
> v2 > v3: correctly mark this for net-next when sending
> v3 > v4: calloc array instead of zalloc per review
> v4 > v5:
>     Apply CR suggestions for whitespace
>     Fix calloc / zalloc mixup
>     Properly destroy workqueues
>     Add third commit to split long macros
>
>
> Colin Foster (3):
>   net: ocelot: align macros for consistency
>   net: mscc: ocelot: add ability to perform bulk reads
>   net: mscc: ocelot: use bulk reads for stats
>
>  drivers/net/ethernet/mscc/ocelot.c    | 78 ++++++++++++++++++++++-----
>  drivers/net/ethernet/mscc/ocelot_io.c | 13 +++++
>  include/soc/mscc/ocelot.h             | 57 ++++++++++++++------
>  3 files changed, 120 insertions(+), 28 deletions(-)
>
> --
> 2.25.1
>

Please do not merge these yet. I gave them a run on my board and the
kernel crashed on boot.

[    8.043170] mscc_felix 0000:00:00.5: Found PCS at internal MDIO address 0
[    8.050241] mscc_felix 0000:00:00.5: Found PCS at internal MDIO address 1
[    8.057142] mscc_felix 0000:00:00.5: Found PCS at internal MDIO address 2
[    8.064021] mscc_felix 0000:00:00.5: Found PCS at internal MDIO address 3
[    8.128668] ------------[ cut here ]------------
[    8.133315] WARNING: CPU: 1 PID: 44 at drivers/net/dsa/ocelot/felix_vsc9959.c:1007 vsc9959_wm_enc+0x2c/0x40
[    8.143107] Modules linked in:
[    8.146181] CPU: 1 PID: 44 Comm: kworker/u4:2 Not tainted 5.17.0-rc2-07010-ga9b9500ffaac-dirty #2104
[    8.155355] Hardware name: LS1028A RDB Board (DT)
[    8.160079] Workqueue: events_unbound deferred_probe_work_func
[    8.165945] pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    8.172940] pc : vsc9959_wm_enc+0x2c/0x40
[    8.176967] lr : ocelot_setup_sharing_watermarks+0x94/0x1fc
[    8.182568] sp : ffff800008863810
[    8.185896] x29: ffff800008863810 x28: 0000000000000308 x27: 0000000000000001
[    8.193079] x26: 000000000300001a x25: 0000000000000008 x24: 0000000000000080
[    8.200261] x23: 0000000088888889 x22: 0000000000000000 x21: 0000000000000000
[    8.207441] x20: 00000000ffff2d17 x19: ffff2d17039d8010 x18: ffffd8e2afa23b28
[    8.214623] x17: 0000000000000015 x16: 0000000000000041 x15: 0000000000000000
[    8.221803] x14: ffffd8e2afa49228 x13: 0000000000025700 x12: 0000000000000009
[    8.228984] x11: ffff2d1703a96c18 x10: 0000000000000004 x9 : ffffd8e2ad6ce0f8
[    8.236165] x8 : ffff2d1700be4240 x7 : ffffd8e2af981000 x6 : 0000000000000001
[    8.243345] x5 : ffffd8e2ad1e1440 x4 : 0000000000000000 x3 : 0000000000000000
[    8.250525] x2 : 0000000000000000 x1 : ffffd8e2ad3d2810 x0 : 00000000000040c0
[    8.257706] Call trace:
[    8.260162]  vsc9959_wm_enc+0x2c/0x40
[    8.263841]  ocelot_devlink_sb_register+0x33c/0x380
[    8.268742]  felix_setup+0x438/0x750
[    8.272334]  dsa_register_switch+0x988/0x114c
[    8.276713]  felix_pci_probe+0x138/0x1fc
[    8.280654]  local_pci_probe+0x4c/0xc0
[    8.284423]  pci_device_probe+0x1b0/0x1f0
[    8.288451]  really_probe.part.0+0xa4/0x310
[    8.292654]  __driver_probe_device+0xa0/0x150
[    8.297030]  driver_probe_device+0xcc/0x164
[    8.301231]  __device_attach_driver+0xc4/0x130
[    8.305695]  bus_for_each_drv+0x84/0xe0
[    8.309547]  __device_attach+0xe4/0x190
[    8.313400]  device_initial_probe+0x20/0x30
[    8.317601]  bus_probe_device+0xac/0xb4
[    8.321454]  deferred_probe_work_func+0x98/0xd4
[    8.326004]  process_one_work+0x294/0x700
[    8.330037]  worker_thread+0x80/0x480
[    8.333717]  kthread+0x10c/0x120
[    8.336961]  ret_from_fork+0x10/0x20
[    8.340554] irq event stamp: 50432
[    8.343968] hardirqs last  enabled at (50431): [<ffffd8e2ade442b0>] _raw_spin_unlock_irqrestore+0x90/0xb0
[    8.353581] hardirqs last disabled at (50432): [<ffffd8e2ade36e44>] el1_dbg+0x24/0x90
[    8.361448] softirqs last  enabled at (50148): [<ffffd8e2ac6908f0>] __do_softirq+0x480/0x5f8
[    8.369923] softirqs last disabled at (50143): [<ffffd8e2ac728e3c>] __irq_exit_rcu+0x17c/0x1b0
[    8.378577] ---[ end trace 0000000000000000 ]---
[    8.383304] ------------[ cut here ]------------
[    8.387942] WARNING: CPU: 1 PID: 44 at drivers/net/dsa/ocelot/felix_vsc9959.c:1007 vsc9959_wm_enc+0x2c/0x40
[    8.397729] Modules linked in:
[    8.400800] CPU: 1 PID: 44 Comm: kworker/u4:2 Tainted: G        W         5.17.0-rc2-07010-ga9b9500ffaac-dirty #2104
[    8.411369] Hardware name: LS1028A RDB Board (DT)
[    8.416092] Workqueue: events_unbound deferred_probe_work_func
[    8.421955] pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    8.428948] pc : vsc9959_wm_enc+0x2c/0x40
[    8.432975] lr : ocelot_setup_sharing_watermarks+0xc0/0x1fc
[    8.438573] sp : ffff800008863810
[    8.441900] x29: ffff800008863810 x28: 0000000000000308 x27: 0000000000000001
[    8.449081] x26: 000000000300001a x25: 0000000000000008 x24: 0000000000000080
[    8.456262] x23: 0000000088888889 x22: 0000000000000000 x21: 0000000000000000
[    8.463443] x20: 00000000ffff2d17 x19: ffff2d17039d8010 x18: ffffd8e2afa23b28
[    8.470623] x17: 0000000000000015 x16: 0000000000000041 x15: 0000000000000000
[    8.477804] x14: ffffd8e2afa49228 x13: 0000000000025700 x12: 0000000000000009
[    8.484984] x11: ffff2d1703a96c18 x10: 0000000000000004 x9 : ffffd8e2ad6ce124
[    8.492165] x8 : ffff2d1700be4240 x7 : ffffd8e2af981000 x6 : 0000000000000001
[    8.499345] x5 : ffffd8e2ad1e1440 x4 : 0000000000000000 x3 : 0000000000000001
[    8.506525] x2 : 0000000000000000 x1 : ffffd8e2ad3d2810 x0 : 00000000000040c0
[    8.513705] Call trace:
[    8.516161]  vsc9959_wm_enc+0x2c/0x40
[    8.519840]  ocelot_devlink_sb_register+0x33c/0x380
[    8.524740]  felix_setup+0x438/0x750
[    8.528331]  dsa_register_switch+0x988/0x114c
[    8.532708]  felix_pci_probe+0x138/0x1fc
[    8.536648]  local_pci_probe+0x4c/0xc0
[    8.540415]  pci_device_probe+0x1b0/0x1f0
[    8.544443]  really_probe.part.0+0xa4/0x310
[    8.548646]  __driver_probe_device+0xa0/0x150
[    8.553022]  driver_probe_device+0xcc/0x164
[    8.557225]  __device_attach_driver+0xc4/0x130
[    8.561688]  bus_for_each_drv+0x84/0xe0
[    8.565540]  __device_attach+0xe4/0x190
[    8.569393]  device_initial_probe+0x20/0x30
[    8.573594]  bus_probe_device+0xac/0xb4
[    8.577447]  deferred_probe_work_func+0x98/0xd4
[    8.581997]  process_one_work+0x294/0x700
[    8.586026]  worker_thread+0x80/0x480
[    8.589706]  kthread+0x10c/0x120
[    8.592949]  ret_from_fork+0x10/0x20
[    8.596541] irq event stamp: 50450
[    8.599955] hardirqs last  enabled at (50449): [<ffffd8e2ade442b0>] _raw_spin_unlock_irqrestore+0x90/0xb0                                                                                                
[    8.609566] hardirqs last disabled at (50450): [<ffffd8e2ade36e44>] el1_dbg+0x24/0x90
[    8.617431] softirqs last  enabled at (50446): [<ffffd8e2ac6908f0>] __do_softirq+0x480/0x5f8
[    8.625907] softirqs last disabled at (50435): [<ffffd8e2ac728e3c>] __irq_exit_rcu+0x17c/0x1b0
[    8.634559] ---[ end trace 0000000000000000 ]---
[    8.640586] device eth1 entered promiscuous mode
[    8.645499] Unable to handle kernel paging request at virtual address 00000010400020bc
[    8.653496] Mem abort info:
[    8.656340]   ESR = 0x96000044
[    8.659413]   EC = 0x25: DABT (current EL), IL = 32 bits
[    8.664784]   SET = 0, FnV = 0
[    8.667855]   EA = 0, S1PTW = 0
[    8.671044]   FSC = 0x04: level 0 translation fault
[    8.675979] Data abort info:
[    8.678875]   ISV = 0, ISS = 0x00000044
[    8.682762]   CM = 0, WnR = 1
[    8.685795] [00000010400020bc] user address but active_mm is swapper
[    8.692272] Internal error: Oops: 96000044 [#1] PREEMPT SMP
[    8.697865] Modules linked in:
[    8.700928] CPU: 1 PID: 44 Comm: kworker/u4:2 Tainted: G        W         5.17.0-rc2-07010-ga9b9500ffaac-dirty #2104
[    8.711490] Hardware name: LS1028A RDB Board (DT)
[    8.716206] Workqueue: events_unbound deferred_probe_work_func
[    8.722065] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    8.729051] pc : ocelot_phylink_mac_link_down+0x70/0x314
[    8.734381] lr : felix_phylink_mac_link_down+0x24/0x30
[    8.739536] sp : ffff800008863840
[    8.742856] x29: ffff800008863840 x28: 0000000000000000 x27: ffffd8e2af8ef180
[    8.750022] x26: 0000000000000001 x25: 0000001040002000 x24: 0000000000000000
[    8.757187] x23: 0000000000000180 x22: ffff2d1703a92000 x21: 0000000000000004
[    8.764352] x20: 0000000000000004 x19: ffff2d17039d8010 x18: ffffd8e2afa23b28
[    8.771516] x17: 0000000000040006 x16: 0000000500030008 x15: ffffffffffffffff
[    8.778680] x14: ffffffffffff0000 x13: ffffffffffffffff x12: 0000000000000010
[    8.785845] x11: 0101010101010101 x10: 0000000000000004 x9 : ffffd8e2ad3d0814
[    8.793009] x8 : fefefefefeff6a6d x7 : 0000ffffffffffff x6 : 0000000000000000
[    8.800173] x5 : 00000000ffffffff x4 : 0000000000000001 x3 : 000000000d000007
[    8.807338] x2 : 0000000000000010 x1 : 0000000000000000 x0 : 0000001040002000
[    8.814502] Call trace:
[    8.816949]  ocelot_phylink_mac_link_down+0x70/0x314
[    8.821929]  felix_phylink_mac_link_down+0x24/0x30
[    8.826734]  dsa_port_link_register_of+0xa8/0x240
[    8.831454]  dsa_port_setup+0xb4/0x180
[    8.835212]  dsa_register_switch+0xdb4/0x114c
[    8.839581]  felix_pci_probe+0x138/0x1fc
[    8.843515]  local_pci_probe+0x4c/0xc0
[    8.847275]  pci_device_probe+0x1b0/0x1f0
[    8.851296]  really_probe.part.0+0xa4/0x310
[    8.855490]  __driver_probe_device+0xa0/0x150
[    8.859860]  driver_probe_device+0xcc/0x164
[    8.864054]  __device_attach_driver+0xc4/0x130
[    8.868510]  bus_for_each_drv+0x84/0xe0
[    8.872355]  __device_attach+0xe4/0x190
[    8.876200]  device_initial_probe+0x20/0x30
[    8.880395]  bus_probe_device+0xac/0xb4
[    8.884240]  deferred_probe_work_func+0x98/0xd4
[    8.888783]  process_one_work+0x294/0x700
[    8.892808]  worker_thread+0x80/0x480
[    8.896480]  kthread+0x10c/0x120
[    8.899715]  ret_from_fork+0x10/0x20
[    8.903303] Code: 52800202 f874d839 52800001 aa1903e0 (b900bf25)
[    8.909417] ---[ end trace 0000000000000000 ]---

Investigating...

Re: [PATCH v5 net-next 0/3] use bulk reads for ocelot statistics

[Colin Foster]

On Tue, Feb 08, 2022 at 01:30:16PM +0000, Vladimir Oltean wrote:
> On Mon, Feb 07, 2022 at 08:46:41PM -0800, Colin Foster wrote:
> > Ocelot loops over memory regions to gather stats on different ports.
> > These regions are mostly continuous, and are ordered. This patch set
> > uses that information to break the stats reads into regions that can get
> > read in bulk.
> >
> > The motiviation is for general cleanup, but also for SPI. Performing two
> > back-to-back reads on a SPI bus require toggling the CS line, holding,
> > re-toggling the CS line, sending 3 address bytes, sending N padding
> > bytes, then actually performing the read. Bulk reads could reduce almost
> > all of that overhead, but require that the reads are performed via
> > regmap_bulk_read.
> >
> > v1 > v2: reword commit messages
> > v2 > v3: correctly mark this for net-next when sending
> > v3 > v4: calloc array instead of zalloc per review
> > v4 > v5:
> >     Apply CR suggestions for whitespace
> >     Fix calloc / zalloc mixup
> >     Properly destroy workqueues
> >     Add third commit to split long macros
> >
> >
> > Colin Foster (3):
> >   net: ocelot: align macros for consistency
> >   net: mscc: ocelot: add ability to perform bulk reads
> >   net: mscc: ocelot: use bulk reads for stats
> >
> >  drivers/net/ethernet/mscc/ocelot.c    | 78 ++++++++++++++++++++++-----
> >  drivers/net/ethernet/mscc/ocelot_io.c | 13 +++++
> >  include/soc/mscc/ocelot.h             | 57 ++++++++++++++------
> >  3 files changed, 120 insertions(+), 28 deletions(-)
> >
> > --
> > 2.25.1
> >
> 
> Please do not merge these yet. I gave them a run on my board and the
> kernel crashed on boot.
> 
> [    8.043170] mscc_felix 0000:00:00.5: Found PCS at internal MDIO address 0
> [    8.050241] mscc_felix 0000:00:00.5: Found PCS at internal MDIO address 1
> [    8.057142] mscc_felix 0000:00:00.5: Found PCS at internal MDIO address 2
> [    8.064021] mscc_felix 0000:00:00.5: Found PCS at internal MDIO address 3
> [    8.128668] ------------[ cut here ]------------
> [    8.133315] WARNING: CPU: 1 PID: 44 at drivers/net/dsa/ocelot/felix_vsc9959.c:1007 vsc9959_wm_enc+0x2c/0x40
> [    8.143107] Modules linked in:
> [    8.146181] CPU: 1 PID: 44 Comm: kworker/u4:2 Not tainted 5.17.0-rc2-07010-ga9b9500ffaac-dirty #2104
> [    8.155355] Hardware name: LS1028A RDB Board (DT)
> [    8.160079] Workqueue: events_unbound deferred_probe_work_func
> [    8.165945] pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    8.172940] pc : vsc9959_wm_enc+0x2c/0x40
> [    8.176967] lr : ocelot_setup_sharing_watermarks+0x94/0x1fc
> [    8.182568] sp : ffff800008863810
> [    8.185896] x29: ffff800008863810 x28: 0000000000000308 x27: 0000000000000001
> [    8.193079] x26: 000000000300001a x25: 0000000000000008 x24: 0000000000000080
> [    8.200261] x23: 0000000088888889 x22: 0000000000000000 x21: 0000000000000000
> [    8.207441] x20: 00000000ffff2d17 x19: ffff2d17039d8010 x18: ffffd8e2afa23b28
> [    8.214623] x17: 0000000000000015 x16: 0000000000000041 x15: 0000000000000000
> [    8.221803] x14: ffffd8e2afa49228 x13: 0000000000025700 x12: 0000000000000009
> [    8.228984] x11: ffff2d1703a96c18 x10: 0000000000000004 x9 : ffffd8e2ad6ce0f8
> [    8.236165] x8 : ffff2d1700be4240 x7 : ffffd8e2af981000 x6 : 0000000000000001
> [    8.243345] x5 : ffffd8e2ad1e1440 x4 : 0000000000000000 x3 : 0000000000000000
> [    8.250525] x2 : 0000000000000000 x1 : ffffd8e2ad3d2810 x0 : 00000000000040c0
> [    8.257706] Call trace:
> [    8.260162]  vsc9959_wm_enc+0x2c/0x40
> [    8.263841]  ocelot_devlink_sb_register+0x33c/0x380
> [    8.268742]  felix_setup+0x438/0x750
> [    8.272334]  dsa_register_switch+0x988/0x114c
> [    8.276713]  felix_pci_probe+0x138/0x1fc
> [    8.280654]  local_pci_probe+0x4c/0xc0
> [    8.284423]  pci_device_probe+0x1b0/0x1f0
> [    8.288451]  really_probe.part.0+0xa4/0x310
> [    8.292654]  __driver_probe_device+0xa0/0x150
> [    8.297030]  driver_probe_device+0xcc/0x164
> [    8.301231]  __device_attach_driver+0xc4/0x130
> [    8.305695]  bus_for_each_drv+0x84/0xe0
> [    8.309547]  __device_attach+0xe4/0x190
> [    8.313400]  device_initial_probe+0x20/0x30
> [    8.317601]  bus_probe_device+0xac/0xb4
> [    8.321454]  deferred_probe_work_func+0x98/0xd4
> [    8.326004]  process_one_work+0x294/0x700
> [    8.330037]  worker_thread+0x80/0x480
> [    8.333717]  kthread+0x10c/0x120
> [    8.336961]  ret_from_fork+0x10/0x20
> [    8.340554] irq event stamp: 50432
> [    8.343968] hardirqs last  enabled at (50431): [<ffffd8e2ade442b0>] _raw_spin_unlock_irqrestore+0x90/0xb0
> [    8.353581] hardirqs last disabled at (50432): [<ffffd8e2ade36e44>] el1_dbg+0x24/0x90
> [    8.361448] softirqs last  enabled at (50148): [<ffffd8e2ac6908f0>] __do_softirq+0x480/0x5f8
> [    8.369923] softirqs last disabled at (50143): [<ffffd8e2ac728e3c>] __irq_exit_rcu+0x17c/0x1b0
> [    8.378577] ---[ end trace 0000000000000000 ]---
> [    8.383304] ------------[ cut here ]------------
> [    8.387942] WARNING: CPU: 1 PID: 44 at drivers/net/dsa/ocelot/felix_vsc9959.c:1007 vsc9959_wm_enc+0x2c/0x40
> [    8.397729] Modules linked in:
> [    8.400800] CPU: 1 PID: 44 Comm: kworker/u4:2 Tainted: G        W         5.17.0-rc2-07010-ga9b9500ffaac-dirty #2104
> [    8.411369] Hardware name: LS1028A RDB Board (DT)
> [    8.416092] Workqueue: events_unbound deferred_probe_work_func
> [    8.421955] pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    8.428948] pc : vsc9959_wm_enc+0x2c/0x40
> [    8.432975] lr : ocelot_setup_sharing_watermarks+0xc0/0x1fc
> [    8.438573] sp : ffff800008863810
> [    8.441900] x29: ffff800008863810 x28: 0000000000000308 x27: 0000000000000001
> [    8.449081] x26: 000000000300001a x25: 0000000000000008 x24: 0000000000000080
> [    8.456262] x23: 0000000088888889 x22: 0000000000000000 x21: 0000000000000000
> [    8.463443] x20: 00000000ffff2d17 x19: ffff2d17039d8010 x18: ffffd8e2afa23b28
> [    8.470623] x17: 0000000000000015 x16: 0000000000000041 x15: 0000000000000000
> [    8.477804] x14: ffffd8e2afa49228 x13: 0000000000025700 x12: 0000000000000009
> [    8.484984] x11: ffff2d1703a96c18 x10: 0000000000000004 x9 : ffffd8e2ad6ce124
> [    8.492165] x8 : ffff2d1700be4240 x7 : ffffd8e2af981000 x6 : 0000000000000001
> [    8.499345] x5 : ffffd8e2ad1e1440 x4 : 0000000000000000 x3 : 0000000000000001
> [    8.506525] x2 : 0000000000000000 x1 : ffffd8e2ad3d2810 x0 : 00000000000040c0
> [    8.513705] Call trace:
> [    8.516161]  vsc9959_wm_enc+0x2c/0x40
> [    8.519840]  ocelot_devlink_sb_register+0x33c/0x380
> [    8.524740]  felix_setup+0x438/0x750
> [    8.528331]  dsa_register_switch+0x988/0x114c
> [    8.532708]  felix_pci_probe+0x138/0x1fc
> [    8.536648]  local_pci_probe+0x4c/0xc0
> [    8.540415]  pci_device_probe+0x1b0/0x1f0
> [    8.544443]  really_probe.part.0+0xa4/0x310
> [    8.548646]  __driver_probe_device+0xa0/0x150
> [    8.553022]  driver_probe_device+0xcc/0x164
> [    8.557225]  __device_attach_driver+0xc4/0x130
> [    8.561688]  bus_for_each_drv+0x84/0xe0
> [    8.565540]  __device_attach+0xe4/0x190
> [    8.569393]  device_initial_probe+0x20/0x30
> [    8.573594]  bus_probe_device+0xac/0xb4
> [    8.577447]  deferred_probe_work_func+0x98/0xd4
> [    8.581997]  process_one_work+0x294/0x700
> [    8.586026]  worker_thread+0x80/0x480
> [    8.589706]  kthread+0x10c/0x120
> [    8.592949]  ret_from_fork+0x10/0x20
> [    8.596541] irq event stamp: 50450
> [    8.599955] hardirqs last  enabled at (50449): [<ffffd8e2ade442b0>] _raw_spin_unlock_irqrestore+0x90/0xb0                                                                                                
> [    8.609566] hardirqs last disabled at (50450): [<ffffd8e2ade36e44>] el1_dbg+0x24/0x90
> [    8.617431] softirqs last  enabled at (50446): [<ffffd8e2ac6908f0>] __do_softirq+0x480/0x5f8
> [    8.625907] softirqs last disabled at (50435): [<ffffd8e2ac728e3c>] __irq_exit_rcu+0x17c/0x1b0
> [    8.634559] ---[ end trace 0000000000000000 ]---
> [    8.640586] device eth1 entered promiscuous mode
> [    8.645499] Unable to handle kernel paging request at virtual address 00000010400020bc
> [    8.653496] Mem abort info:
> [    8.656340]   ESR = 0x96000044
> [    8.659413]   EC = 0x25: DABT (current EL), IL = 32 bits
> [    8.664784]   SET = 0, FnV = 0
> [    8.667855]   EA = 0, S1PTW = 0
> [    8.671044]   FSC = 0x04: level 0 translation fault
> [    8.675979] Data abort info:
> [    8.678875]   ISV = 0, ISS = 0x00000044
> [    8.682762]   CM = 0, WnR = 1
> [    8.685795] [00000010400020bc] user address but active_mm is swapper
> [    8.692272] Internal error: Oops: 96000044 [#1] PREEMPT SMP
> [    8.697865] Modules linked in:
> [    8.700928] CPU: 1 PID: 44 Comm: kworker/u4:2 Tainted: G        W         5.17.0-rc2-07010-ga9b9500ffaac-dirty #2104
> [    8.711490] Hardware name: LS1028A RDB Board (DT)
> [    8.716206] Workqueue: events_unbound deferred_probe_work_func
> [    8.722065] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> [    8.729051] pc : ocelot_phylink_mac_link_down+0x70/0x314
> [    8.734381] lr : felix_phylink_mac_link_down+0x24/0x30
> [    8.739536] sp : ffff800008863840
> [    8.742856] x29: ffff800008863840 x28: 0000000000000000 x27: ffffd8e2af8ef180
> [    8.750022] x26: 0000000000000001 x25: 0000001040002000 x24: 0000000000000000
> [    8.757187] x23: 0000000000000180 x22: ffff2d1703a92000 x21: 0000000000000004
> [    8.764352] x20: 0000000000000004 x19: ffff2d17039d8010 x18: ffffd8e2afa23b28
> [    8.771516] x17: 0000000000040006 x16: 0000000500030008 x15: ffffffffffffffff
> [    8.778680] x14: ffffffffffff0000 x13: ffffffffffffffff x12: 0000000000000010
> [    8.785845] x11: 0101010101010101 x10: 0000000000000004 x9 : ffffd8e2ad3d0814
> [    8.793009] x8 : fefefefefeff6a6d x7 : 0000ffffffffffff x6 : 0000000000000000
> [    8.800173] x5 : 00000000ffffffff x4 : 0000000000000001 x3 : 000000000d000007
> [    8.807338] x2 : 0000000000000010 x1 : 0000000000000000 x0 : 0000001040002000
> [    8.814502] Call trace:
> [    8.816949]  ocelot_phylink_mac_link_down+0x70/0x314
> [    8.821929]  felix_phylink_mac_link_down+0x24/0x30
> [    8.826734]  dsa_port_link_register_of+0xa8/0x240
> [    8.831454]  dsa_port_setup+0xb4/0x180
> [    8.835212]  dsa_register_switch+0xdb4/0x114c
> [    8.839581]  felix_pci_probe+0x138/0x1fc
> [    8.843515]  local_pci_probe+0x4c/0xc0
> [    8.847275]  pci_device_probe+0x1b0/0x1f0
> [    8.851296]  really_probe.part.0+0xa4/0x310
> [    8.855490]  __driver_probe_device+0xa0/0x150
> [    8.859860]  driver_probe_device+0xcc/0x164
> [    8.864054]  __device_attach_driver+0xc4/0x130
> [    8.868510]  bus_for_each_drv+0x84/0xe0
> [    8.872355]  __device_attach+0xe4/0x190
> [    8.876200]  device_initial_probe+0x20/0x30
> [    8.880395]  bus_probe_device+0xac/0xb4
> [    8.884240]  deferred_probe_work_func+0x98/0xd4
> [    8.888783]  process_one_work+0x294/0x700
> [    8.892808]  worker_thread+0x80/0x480
> [    8.896480]  kthread+0x10c/0x120
> [    8.899715]  ret_from_fork+0x10/0x20
> [    8.903303] Code: 52800202 f874d839 52800001 aa1903e0 (b900bf25)
> [    8.909417] ---[ end trace 0000000000000000 ]---
> 
> Investigating...

I just did a sanity check on my latest tree and it doesn't crash. I'll
keep an eye out here as well.

Gives me an opportunity to fix up your other suggestion.

Thanks for reviewing and testing Vladimir!

Re: [PATCH v5 net-next 0/3] use bulk reads for ocelot statistics

[Vladimir Oltean]

On Tue, Feb 08, 2022 at 05:55:32AM -0800, Colin Foster wrote:
> On Tue, Feb 08, 2022 at 01:30:16PM +0000, Vladimir Oltean wrote:
> > On Mon, Feb 07, 2022 at 08:46:41PM -0800, Colin Foster wrote:
> > > Ocelot loops over memory regions to gather stats on different ports.
> > > These regions are mostly continuous, and are ordered. This patch set
> > > uses that information to break the stats reads into regions that can get
> > > read in bulk.
> > >
> > > The motiviation is for general cleanup, but also for SPI. Performing two
> > > back-to-back reads on a SPI bus require toggling the CS line, holding,
> > > re-toggling the CS line, sending 3 address bytes, sending N padding
> > > bytes, then actually performing the read. Bulk reads could reduce almost
> > > all of that overhead, but require that the reads are performed via
> > > regmap_bulk_read.
> > >
> > > v1 > v2: reword commit messages
> > > v2 > v3: correctly mark this for net-next when sending
> > > v3 > v4: calloc array instead of zalloc per review
> > > v4 > v5:
> > >     Apply CR suggestions for whitespace
> > >     Fix calloc / zalloc mixup
> > >     Properly destroy workqueues
> > >     Add third commit to split long macros
> > >
> > >
> > > Colin Foster (3):
> > >   net: ocelot: align macros for consistency
> > >   net: mscc: ocelot: add ability to perform bulk reads
> > >   net: mscc: ocelot: use bulk reads for stats
> > >
> > >  drivers/net/ethernet/mscc/ocelot.c    | 78 ++++++++++++++++++++++-----
> > >  drivers/net/ethernet/mscc/ocelot_io.c | 13 +++++
> > >  include/soc/mscc/ocelot.h             | 57 ++++++++++++++------
> > >  3 files changed, 120 insertions(+), 28 deletions(-)
> > >
> > > --
> > > 2.25.1
> > >
> > 
> > Please do not merge these yet. I gave them a run on my board and the
> > kernel crashed on boot.
> > 
> > [    8.043170] mscc_felix 0000:00:00.5: Found PCS at internal MDIO address 0
> > [    8.050241] mscc_felix 0000:00:00.5: Found PCS at internal MDIO address 1
> > [    8.057142] mscc_felix 0000:00:00.5: Found PCS at internal MDIO address 2
> > [    8.064021] mscc_felix 0000:00:00.5: Found PCS at internal MDIO address 3
> > [    8.128668] ------------[ cut here ]------------
> > [    8.133315] WARNING: CPU: 1 PID: 44 at drivers/net/dsa/ocelot/felix_vsc9959.c:1007 vsc9959_wm_enc+0x2c/0x40
> > [    8.143107] Modules linked in:
> > [    8.146181] CPU: 1 PID: 44 Comm: kworker/u4:2 Not tainted 5.17.0-rc2-07010-ga9b9500ffaac-dirty #2104
> > [    8.155355] Hardware name: LS1028A RDB Board (DT)
> > [    8.160079] Workqueue: events_unbound deferred_probe_work_func
> > [    8.165945] pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [    8.172940] pc : vsc9959_wm_enc+0x2c/0x40
> > [    8.176967] lr : ocelot_setup_sharing_watermarks+0x94/0x1fc
> > [    8.182568] sp : ffff800008863810
> > [    8.185896] x29: ffff800008863810 x28: 0000000000000308 x27: 0000000000000001
> > [    8.193079] x26: 000000000300001a x25: 0000000000000008 x24: 0000000000000080
> > [    8.200261] x23: 0000000088888889 x22: 0000000000000000 x21: 0000000000000000
> > [    8.207441] x20: 00000000ffff2d17 x19: ffff2d17039d8010 x18: ffffd8e2afa23b28
> > [    8.214623] x17: 0000000000000015 x16: 0000000000000041 x15: 0000000000000000
> > [    8.221803] x14: ffffd8e2afa49228 x13: 0000000000025700 x12: 0000000000000009
> > [    8.228984] x11: ffff2d1703a96c18 x10: 0000000000000004 x9 : ffffd8e2ad6ce0f8
> > [    8.236165] x8 : ffff2d1700be4240 x7 : ffffd8e2af981000 x6 : 0000000000000001
> > [    8.243345] x5 : ffffd8e2ad1e1440 x4 : 0000000000000000 x3 : 0000000000000000
> > [    8.250525] x2 : 0000000000000000 x1 : ffffd8e2ad3d2810 x0 : 00000000000040c0
> > [    8.257706] Call trace:
> > [    8.260162]  vsc9959_wm_enc+0x2c/0x40
> > [    8.263841]  ocelot_devlink_sb_register+0x33c/0x380
> > [    8.268742]  felix_setup+0x438/0x750
> > [    8.272334]  dsa_register_switch+0x988/0x114c
> > [    8.276713]  felix_pci_probe+0x138/0x1fc
> > [    8.280654]  local_pci_probe+0x4c/0xc0
> > [    8.284423]  pci_device_probe+0x1b0/0x1f0
> > [    8.288451]  really_probe.part.0+0xa4/0x310
> > [    8.292654]  __driver_probe_device+0xa0/0x150
> > [    8.297030]  driver_probe_device+0xcc/0x164
> > [    8.301231]  __device_attach_driver+0xc4/0x130
> > [    8.305695]  bus_for_each_drv+0x84/0xe0
> > [    8.309547]  __device_attach+0xe4/0x190
> > [    8.313400]  device_initial_probe+0x20/0x30
> > [    8.317601]  bus_probe_device+0xac/0xb4
> > [    8.321454]  deferred_probe_work_func+0x98/0xd4
> > [    8.326004]  process_one_work+0x294/0x700
> > [    8.330037]  worker_thread+0x80/0x480
> > [    8.333717]  kthread+0x10c/0x120
> > [    8.336961]  ret_from_fork+0x10/0x20
> > [    8.340554] irq event stamp: 50432
> > [    8.343968] hardirqs last  enabled at (50431): [<ffffd8e2ade442b0>] _raw_spin_unlock_irqrestore+0x90/0xb0
> > [    8.353581] hardirqs last disabled at (50432): [<ffffd8e2ade36e44>] el1_dbg+0x24/0x90
> > [    8.361448] softirqs last  enabled at (50148): [<ffffd8e2ac6908f0>] __do_softirq+0x480/0x5f8
> > [    8.369923] softirqs last disabled at (50143): [<ffffd8e2ac728e3c>] __irq_exit_rcu+0x17c/0x1b0
> > [    8.378577] ---[ end trace 0000000000000000 ]---
> > [    8.383304] ------------[ cut here ]------------
> > [    8.387942] WARNING: CPU: 1 PID: 44 at drivers/net/dsa/ocelot/felix_vsc9959.c:1007 vsc9959_wm_enc+0x2c/0x40
> > [    8.397729] Modules linked in:
> > [    8.400800] CPU: 1 PID: 44 Comm: kworker/u4:2 Tainted: G        W         5.17.0-rc2-07010-ga9b9500ffaac-dirty #2104
> > [    8.411369] Hardware name: LS1028A RDB Board (DT)
> > [    8.416092] Workqueue: events_unbound deferred_probe_work_func
> > [    8.421955] pstate: 20000005 (nzCv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [    8.428948] pc : vsc9959_wm_enc+0x2c/0x40
> > [    8.432975] lr : ocelot_setup_sharing_watermarks+0xc0/0x1fc
> > [    8.438573] sp : ffff800008863810
> > [    8.441900] x29: ffff800008863810 x28: 0000000000000308 x27: 0000000000000001
> > [    8.449081] x26: 000000000300001a x25: 0000000000000008 x24: 0000000000000080
> > [    8.456262] x23: 0000000088888889 x22: 0000000000000000 x21: 0000000000000000
> > [    8.463443] x20: 00000000ffff2d17 x19: ffff2d17039d8010 x18: ffffd8e2afa23b28
> > [    8.470623] x17: 0000000000000015 x16: 0000000000000041 x15: 0000000000000000
> > [    8.477804] x14: ffffd8e2afa49228 x13: 0000000000025700 x12: 0000000000000009
> > [    8.484984] x11: ffff2d1703a96c18 x10: 0000000000000004 x9 : ffffd8e2ad6ce124
> > [    8.492165] x8 : ffff2d1700be4240 x7 : ffffd8e2af981000 x6 : 0000000000000001
> > [    8.499345] x5 : ffffd8e2ad1e1440 x4 : 0000000000000000 x3 : 0000000000000001
> > [    8.506525] x2 : 0000000000000000 x1 : ffffd8e2ad3d2810 x0 : 00000000000040c0
> > [    8.513705] Call trace:
> > [    8.516161]  vsc9959_wm_enc+0x2c/0x40
> > [    8.519840]  ocelot_devlink_sb_register+0x33c/0x380
> > [    8.524740]  felix_setup+0x438/0x750
> > [    8.528331]  dsa_register_switch+0x988/0x114c
> > [    8.532708]  felix_pci_probe+0x138/0x1fc
> > [    8.536648]  local_pci_probe+0x4c/0xc0
> > [    8.540415]  pci_device_probe+0x1b0/0x1f0
> > [    8.544443]  really_probe.part.0+0xa4/0x310
> > [    8.548646]  __driver_probe_device+0xa0/0x150
> > [    8.553022]  driver_probe_device+0xcc/0x164
> > [    8.557225]  __device_attach_driver+0xc4/0x130
> > [    8.561688]  bus_for_each_drv+0x84/0xe0
> > [    8.565540]  __device_attach+0xe4/0x190
> > [    8.569393]  device_initial_probe+0x20/0x30
> > [    8.573594]  bus_probe_device+0xac/0xb4
> > [    8.577447]  deferred_probe_work_func+0x98/0xd4
> > [    8.581997]  process_one_work+0x294/0x700
> > [    8.586026]  worker_thread+0x80/0x480
> > [    8.589706]  kthread+0x10c/0x120
> > [    8.592949]  ret_from_fork+0x10/0x20
> > [    8.596541] irq event stamp: 50450
> > [    8.599955] hardirqs last  enabled at (50449): [<ffffd8e2ade442b0>] _raw_spin_unlock_irqrestore+0x90/0xb0                                                                                                
> > [    8.609566] hardirqs last disabled at (50450): [<ffffd8e2ade36e44>] el1_dbg+0x24/0x90
> > [    8.617431] softirqs last  enabled at (50446): [<ffffd8e2ac6908f0>] __do_softirq+0x480/0x5f8
> > [    8.625907] softirqs last disabled at (50435): [<ffffd8e2ac728e3c>] __irq_exit_rcu+0x17c/0x1b0
> > [    8.634559] ---[ end trace 0000000000000000 ]---
> > [    8.640586] device eth1 entered promiscuous mode
> > [    8.645499] Unable to handle kernel paging request at virtual address 00000010400020bc
> > [    8.653496] Mem abort info:
> > [    8.656340]   ESR = 0x96000044
> > [    8.659413]   EC = 0x25: DABT (current EL), IL = 32 bits
> > [    8.664784]   SET = 0, FnV = 0
> > [    8.667855]   EA = 0, S1PTW = 0
> > [    8.671044]   FSC = 0x04: level 0 translation fault
> > [    8.675979] Data abort info:
> > [    8.678875]   ISV = 0, ISS = 0x00000044
> > [    8.682762]   CM = 0, WnR = 1
> > [    8.685795] [00000010400020bc] user address but active_mm is swapper
> > [    8.692272] Internal error: Oops: 96000044 [#1] PREEMPT SMP
> > [    8.697865] Modules linked in:
> > [    8.700928] CPU: 1 PID: 44 Comm: kworker/u4:2 Tainted: G        W         5.17.0-rc2-07010-ga9b9500ffaac-dirty #2104
> > [    8.711490] Hardware name: LS1028A RDB Board (DT)
> > [    8.716206] Workqueue: events_unbound deferred_probe_work_func
> > [    8.722065] pstate: 00000005 (nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
> > [    8.729051] pc : ocelot_phylink_mac_link_down+0x70/0x314
> > [    8.734381] lr : felix_phylink_mac_link_down+0x24/0x30
> > [    8.739536] sp : ffff800008863840
> > [    8.742856] x29: ffff800008863840 x28: 0000000000000000 x27: ffffd8e2af8ef180
> > [    8.750022] x26: 0000000000000001 x25: 0000001040002000 x24: 0000000000000000
> > [    8.757187] x23: 0000000000000180 x22: ffff2d1703a92000 x21: 0000000000000004
> > [    8.764352] x20: 0000000000000004 x19: ffff2d17039d8010 x18: ffffd8e2afa23b28
> > [    8.771516] x17: 0000000000040006 x16: 0000000500030008 x15: ffffffffffffffff
> > [    8.778680] x14: ffffffffffff0000 x13: ffffffffffffffff x12: 0000000000000010
> > [    8.785845] x11: 0101010101010101 x10: 0000000000000004 x9 : ffffd8e2ad3d0814
> > [    8.793009] x8 : fefefefefeff6a6d x7 : 0000ffffffffffff x6 : 0000000000000000
> > [    8.800173] x5 : 00000000ffffffff x4 : 0000000000000001 x3 : 000000000d000007
> > [    8.807338] x2 : 0000000000000010 x1 : 0000000000000000 x0 : 0000001040002000
> > [    8.814502] Call trace:
> > [    8.816949]  ocelot_phylink_mac_link_down+0x70/0x314
> > [    8.821929]  felix_phylink_mac_link_down+0x24/0x30
> > [    8.826734]  dsa_port_link_register_of+0xa8/0x240
> > [    8.831454]  dsa_port_setup+0xb4/0x180
> > [    8.835212]  dsa_register_switch+0xdb4/0x114c
> > [    8.839581]  felix_pci_probe+0x138/0x1fc
> > [    8.843515]  local_pci_probe+0x4c/0xc0
> > [    8.847275]  pci_device_probe+0x1b0/0x1f0
> > [    8.851296]  really_probe.part.0+0xa4/0x310
> > [    8.855490]  __driver_probe_device+0xa0/0x150
> > [    8.859860]  driver_probe_device+0xcc/0x164
> > [    8.864054]  __device_attach_driver+0xc4/0x130
> > [    8.868510]  bus_for_each_drv+0x84/0xe0
> > [    8.872355]  __device_attach+0xe4/0x190
> > [    8.876200]  device_initial_probe+0x20/0x30
> > [    8.880395]  bus_probe_device+0xac/0xb4
> > [    8.884240]  deferred_probe_work_func+0x98/0xd4
> > [    8.888783]  process_one_work+0x294/0x700
> > [    8.892808]  worker_thread+0x80/0x480
> > [    8.896480]  kthread+0x10c/0x120
> > [    8.899715]  ret_from_fork+0x10/0x20
> > [    8.903303] Code: 52800202 f874d839 52800001 aa1903e0 (b900bf25)
> > [    8.909417] ---[ end trace 0000000000000000 ]---
> > 
> > Investigating...
> 
> I just did a sanity check on my latest tree and it doesn't crash. I'll
> keep an eye out here as well.
> 
> Gives me an opportunity to fix up your other suggestion.
> 
> Thanks for reviewing and testing Vladimir!

I don't know why this glitch happened. I couldn't reproduce this a
second time. Booting patch by patch with KASAN enabled also showed
nothing. And the stack traces don't make much sense either. I'll just
ignore it for now.

But anyway, I found some actual bugs in the code while testing, I'll
reply on the individual patch.

Re: [PATCH v5 net-next 3/3] net: mscc: ocelot: use bulk reads for stats

[Vladimir Oltean]

On Mon, Feb 07, 2022 at 08:46:44PM -0800, Colin Foster wrote:
> Create and utilize bulk regmap reads instead of single access for gathering
> stats. The background reading of statistics happens frequently, and over
> a few contiguous memory regions.
> 
> High speed PCIe buses and MMIO access will probably see negligible
> performance increase. Lower speed buses like SPI and I2C could see
> significant performance increase, since the bus configuration and register
> access times account for a large percentage of data transfer time.
> 
> Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> ---
>  drivers/net/ethernet/mscc/ocelot.c | 78 +++++++++++++++++++++++++-----
>  include/soc/mscc/ocelot.h          |  8 +++
>  2 files changed, 73 insertions(+), 13 deletions(-)
> 
> diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> index 455293aa6343..5efb1f3a1410 100644
> --- a/drivers/net/ethernet/mscc/ocelot.c
> +++ b/drivers/net/ethernet/mscc/ocelot.c
> @@ -1737,32 +1737,41 @@ void ocelot_get_strings(struct ocelot *ocelot, int port, u32 sset, u8 *data)
>  }
>  EXPORT_SYMBOL(ocelot_get_strings);
>  
> -static void ocelot_update_stats(struct ocelot *ocelot)
> +static int ocelot_update_stats(struct ocelot *ocelot)
>  {
> -	int i, j;
> +	struct ocelot_stats_region *region;
> +	int i, j, err = 0;
>  
>  	mutex_lock(&ocelot->stats_lock);
>  
>  	for (i = 0; i < ocelot->num_phys_ports; i++) {
> +		unsigned int idx = 0;
> +

This is a bug which causes ocelot->stats to be overwritten with the
statistics of port 0, for all ports. Either move the variable
declaration and initialization with 0 in the larger scope (outside the
"for" loop), or initialize idx with i * ocelot->num_stats.

>  		/* Configure the port to read the stats from */
>  		ocelot_write(ocelot, SYS_STAT_CFG_STAT_VIEW(i), SYS_STAT_CFG);
>  
> -		for (j = 0; j < ocelot->num_stats; j++) {
> -			u32 val;
> -			unsigned int idx = i * ocelot->num_stats + j;
> +		list_for_each_entry(region, &ocelot->stats_regions, node) {
> +			err = ocelot_bulk_read_rix(ocelot, SYS_COUNT_RX_OCTETS,
> +						   region->offset, region->buf,
> +						   region->count);
> +			if (err)
> +				goto out;
>  
> -			val = ocelot_read_rix(ocelot, SYS_COUNT_RX_OCTETS,
> -					      ocelot->stats_layout[j].offset);
> +			for (j = 0; j < region->count; j++) {
> +				if (region->buf[j] < (ocelot->stats[idx + j] & U32_MAX))
> +					ocelot->stats[idx + j] += (u64)1 << 32;

I'd prefer if you reduce the apparent complexity of this logic by
creating some temporary variables:

	u64 *stat = &ocelot->stats[idx + j];
	u64 val = region->buf[j];

>  
> -			if (val < (ocelot->stats[idx] & U32_MAX))
> -				ocelot->stats[idx] += (u64)1 << 32;
> +				ocelot->stats[idx + j] = (ocelot->stats[idx + j] &
> +							~(u64)U32_MAX) + region->buf[j];
> +			}
>  
> -			ocelot->stats[idx] = (ocelot->stats[idx] &
> -					      ~(u64)U32_MAX) + val;
> +			idx += region->count;
>  		}
>  	}
>  
> +out:
>  	mutex_unlock(&ocelot->stats_lock);
> +	return err;
>  }
>  
>  static void ocelot_check_stats_work(struct work_struct *work)
> @@ -1779,10 +1788,11 @@ static void ocelot_check_stats_work(struct work_struct *work)
>  
>  void ocelot_get_ethtool_stats(struct ocelot *ocelot, int port, u64 *data)
>  {
> -	int i;
> +	int i, err;
>  
>  	/* check and update now */
> -	ocelot_update_stats(ocelot);
> +	err = ocelot_update_stats(ocelot);

Please, as a separate change, introduce a function that reads the
statistics for a single port, and make ethtool call that and not the
entire port array, it's pointless.

> +	WARN_ONCE(err, "Error %d updating ethtool stats\n", err);
>  
>  	/* Copy all counters */
>  	for (i = 0; i < ocelot->num_stats; i++)

and here, in the unseen part of the context, lies:

	/* Copy all counters */
	for (i = 0; i < ocelot->num_stats; i++)
		*data++ = ocelot->stats[port * ocelot->num_stats + i];

I think this is buggy, because this is a reader of ocelot->stats which
is not protected by ocelot->stats_lock (it was taken and dropped by
ocelot_update_stats). But a second ocelot_update_stats() can run
concurrently with ethtool and ruin the day, modifying the array at the
same time as it's being read out.

The new function that you introduce, for reading the stats of a single
port, should require that ocelot->stats_lock is already held, and you
should hold it from top-level (ocelot_get_ethtool_stats).

> @@ -1799,6 +1809,41 @@ int ocelot_get_sset_count(struct ocelot *ocelot, int port, int sset)
>  }
>  EXPORT_SYMBOL(ocelot_get_sset_count);
>  
> +static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
> +{
> +	struct ocelot_stats_region *region = NULL;
> +	unsigned int last;
> +	int i;
> +
> +	INIT_LIST_HEAD(&ocelot->stats_regions);
> +
> +	for (i = 0; i < ocelot->num_stats; i++) {
> +		if (region && ocelot->stats_layout[i].offset == last + 1) {
> +			region->count++;
> +		} else {
> +			region = devm_kzalloc(ocelot->dev, sizeof(*region),
> +					      GFP_KERNEL);
> +			if (!region)
> +				return -ENOMEM;
> +
> +			region->offset = ocelot->stats_layout[i].offset;
> +			region->count = 1;
> +			list_add_tail(&region->node, &ocelot->stats_regions);
> +		}
> +
> +		last = ocelot->stats_layout[i].offset;
> +	}
> +
> +	list_for_each_entry(region, &ocelot->stats_regions, node) {
> +		region->buf = devm_kcalloc(ocelot->dev, region->count,
> +					   sizeof(*region->buf), GFP_KERNEL);
> +		if (!region->buf)
> +			return -ENOMEM;
> +	}
> +
> +	return 0;
> +}
> +
>  int ocelot_get_ts_info(struct ocelot *ocelot, int port,
>  		       struct ethtool_ts_info *info)
>  {
> @@ -2799,6 +2844,13 @@ int ocelot_init(struct ocelot *ocelot)
>  				 ANA_CPUQ_8021_CFG_CPUQ_BPDU_VAL(6),
>  				 ANA_CPUQ_8021_CFG, i);
>  
> +	ret = ocelot_prepare_stats_regions(ocelot);
> +	if (ret) {
> +		destroy_workqueue(ocelot->stats_queue);
> +		destroy_workqueue(ocelot->owq);
> +		return ret;
> +	}
> +
>  	INIT_DELAYED_WORK(&ocelot->stats_work, ocelot_check_stats_work);
>  	queue_delayed_work(ocelot->stats_queue, &ocelot->stats_work,
>  			   OCELOT_STATS_CHECK_DELAY);
> diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> index 312b72558659..d3291a5f7e88 100644
> --- a/include/soc/mscc/ocelot.h
> +++ b/include/soc/mscc/ocelot.h
> @@ -542,6 +542,13 @@ struct ocelot_stat_layout {
>  	char name[ETH_GSTRING_LEN];
>  };
>  
> +struct ocelot_stats_region {
> +	struct list_head node;
> +	u32 offset;
> +	int count;
> +	u32 *buf;
> +};
> +
>  enum ocelot_tag_prefix {
>  	OCELOT_TAG_PREFIX_DISABLED	= 0,
>  	OCELOT_TAG_PREFIX_NONE,
> @@ -673,6 +680,7 @@ struct ocelot {
>  	struct regmap_field		*regfields[REGFIELD_MAX];
>  	const u32 *const		*map;
>  	const struct ocelot_stat_layout	*stats_layout;
> +	struct list_head		stats_regions;
>  	unsigned int			num_stats;
>  
>  	u32				pool_size[OCELOT_SB_NUM][OCELOT_SB_POOL_NUM];
> -- 
> 2.25.1
>

Re: [PATCH v5 net-next 3/3] net: mscc: ocelot: use bulk reads for stats

[Vladimir Oltean]

On Tue, Feb 08, 2022 at 05:03:03PM +0200, Vladimir Oltean wrote:
> >  	for (i = 0; i < ocelot->num_phys_ports; i++) {
> > +		unsigned int idx = 0;
> > +
> 
> This is a bug which causes ocelot->stats to be overwritten with the
> statistics of port 0, for all ports. Either move the variable
> declaration and initialization with 0 in the larger scope (outside the
> "for" loop), or initialize idx with i * ocelot->num_stats.

My analysis was slightly incorrect. Somehow I managed to fool myself
into thinking that you had tested this in a limited scenario, hence the
reason you didn't notice it's not working. But apparently you didn't
test with traffic at all.

So ocelot->stats isn't overwritten with the stats of port 0 for all
ports. But rather, all ports write into the ocelot->stats space
dedicated for port 0, effectively overwriting the stats of port 0 with
the stats of the last port. And no one populates the ocelot->stats space
for ports [1 .. last]. So no port has good statistics, I don't see a
circumstance where testing could have misled you.

Re: [PATCH v5 net-next 3/3] net: mscc: ocelot: use bulk reads for stats

[Colin Foster]

Hi Vladimir,

On Tue, Feb 08, 2022 at 03:03:04PM +0000, Vladimir Oltean wrote:
> On Mon, Feb 07, 2022 at 08:46:44PM -0800, Colin Foster wrote:
> > Create and utilize bulk regmap reads instead of single access for gathering
> > stats. The background reading of statistics happens frequently, and over
> > a few contiguous memory regions.
> > 
> > High speed PCIe buses and MMIO access will probably see negligible
> > performance increase. Lower speed buses like SPI and I2C could see
> > significant performance increase, since the bus configuration and register
> > access times account for a large percentage of data transfer time.
> > 
> > Signed-off-by: Colin Foster <colin.foster@in-advantage.com>
> > ---
> >  drivers/net/ethernet/mscc/ocelot.c | 78 +++++++++++++++++++++++++-----
> >  include/soc/mscc/ocelot.h          |  8 +++
> >  2 files changed, 73 insertions(+), 13 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/mscc/ocelot.c b/drivers/net/ethernet/mscc/ocelot.c
> > index 455293aa6343..5efb1f3a1410 100644
> > --- a/drivers/net/ethernet/mscc/ocelot.c
> > +++ b/drivers/net/ethernet/mscc/ocelot.c
> > @@ -1737,32 +1737,41 @@ void ocelot_get_strings(struct ocelot *ocelot, int port, u32 sset, u8 *data)
> >  }
> >  EXPORT_SYMBOL(ocelot_get_strings);
> >  
> > -static void ocelot_update_stats(struct ocelot *ocelot)
> > +static int ocelot_update_stats(struct ocelot *ocelot)
> >  {
> > -	int i, j;
> > +	struct ocelot_stats_region *region;
> > +	int i, j, err = 0;
> >  
> >  	mutex_lock(&ocelot->stats_lock);
> >  
> >  	for (i = 0; i < ocelot->num_phys_ports; i++) {
> > +		unsigned int idx = 0;
> > +
> 
> This is a bug which causes ocelot->stats to be overwritten with the
> statistics of port 0, for all ports. Either move the variable
> declaration and initialization with 0 in the larger scope (outside the
> "for" loop), or initialize idx with i * ocelot->num_stats.

I see that now. It is confusing and I'll clear it up. I never caught
this because I'm testing in a setup where port 0 is the CPU port, so I
can't get ethtool stats. Thanks!

> 
> >  		/* Configure the port to read the stats from */
> >  		ocelot_write(ocelot, SYS_STAT_CFG_STAT_VIEW(i), SYS_STAT_CFG);
> >  
> > -		for (j = 0; j < ocelot->num_stats; j++) {
> > -			u32 val;
> > -			unsigned int idx = i * ocelot->num_stats + j;
> > +		list_for_each_entry(region, &ocelot->stats_regions, node) {
> > +			err = ocelot_bulk_read_rix(ocelot, SYS_COUNT_RX_OCTETS,
> > +						   region->offset, region->buf,
> > +						   region->count);
> > +			if (err)
> > +				goto out;
> >  
> > -			val = ocelot_read_rix(ocelot, SYS_COUNT_RX_OCTETS,
> > -					      ocelot->stats_layout[j].offset);
> > +			for (j = 0; j < region->count; j++) {
> > +				if (region->buf[j] < (ocelot->stats[idx + j] & U32_MAX))
> > +					ocelot->stats[idx + j] += (u64)1 << 32;
> 
> I'd prefer if you reduce the apparent complexity of this logic by
> creating some temporary variables:
> 
> 	u64 *stat = &ocelot->stats[idx + j];
> 	u64 val = region->buf[j];

Can do. Thanks for the suggestion.

> 
> >  
> > -			if (val < (ocelot->stats[idx] & U32_MAX))
> > -				ocelot->stats[idx] += (u64)1 << 32;
> > +				ocelot->stats[idx + j] = (ocelot->stats[idx + j] &
> > +							~(u64)U32_MAX) + region->buf[j];
> > +			}
> >  
> > -			ocelot->stats[idx] = (ocelot->stats[idx] &
> > -					      ~(u64)U32_MAX) + val;
> > +			idx += region->count;
> >  		}
> >  	}
> >  
> > +out:
> >  	mutex_unlock(&ocelot->stats_lock);
> > +	return err;
> >  }
> >  
> >  static void ocelot_check_stats_work(struct work_struct *work)
> > @@ -1779,10 +1788,11 @@ static void ocelot_check_stats_work(struct work_struct *work)
> >  
> >  void ocelot_get_ethtool_stats(struct ocelot *ocelot, int port, u64 *data)
> >  {
> > -	int i;
> > +	int i, err;
> >  
> >  	/* check and update now */
> > -	ocelot_update_stats(ocelot);
> > +	err = ocelot_update_stats(ocelot);
> 
> Please, as a separate change, introduce a function that reads the
> statistics for a single port, and make ethtool call that and not the
> entire port array, it's pointless.
> 
> > +	WARN_ONCE(err, "Error %d updating ethtool stats\n", err);
> >  
> >  	/* Copy all counters */
> >  	for (i = 0; i < ocelot->num_stats; i++)
> 
> and here, in the unseen part of the context, lies:
> 
> 	/* Copy all counters */
> 	for (i = 0; i < ocelot->num_stats; i++)
> 		*data++ = ocelot->stats[port * ocelot->num_stats + i];
> 
> I think this is buggy, because this is a reader of ocelot->stats which
> is not protected by ocelot->stats_lock (it was taken and dropped by
> ocelot_update_stats). But a second ocelot_update_stats() can run
> concurrently with ethtool and ruin the day, modifying the array at the
> same time as it's being read out.
> 
> The new function that you introduce, for reading the stats of a single
> port, should require that ocelot->stats_lock is already held, and you
> should hold it from top-level (ocelot_get_ethtool_stats).

I'll add this fix as a separate patch. Thanks as always for the
feedback!

> 
> > @@ -1799,6 +1809,41 @@ int ocelot_get_sset_count(struct ocelot *ocelot, int port, int sset)
> >  }
> >  EXPORT_SYMBOL(ocelot_get_sset_count);
> >  
> > +static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
> > +{
> > +	struct ocelot_stats_region *region = NULL;
> > +	unsigned int last;
> > +	int i;
> > +
> > +	INIT_LIST_HEAD(&ocelot->stats_regions);
> > +
> > +	for (i = 0; i < ocelot->num_stats; i++) {
> > +		if (region && ocelot->stats_layout[i].offset == last + 1) {
> > +			region->count++;
> > +		} else {
> > +			region = devm_kzalloc(ocelot->dev, sizeof(*region),
> > +					      GFP_KERNEL);
> > +			if (!region)
> > +				return -ENOMEM;
> > +
> > +			region->offset = ocelot->stats_layout[i].offset;
> > +			region->count = 1;
> > +			list_add_tail(&region->node, &ocelot->stats_regions);
> > +		}
> > +
> > +		last = ocelot->stats_layout[i].offset;
> > +	}
> > +
> > +	list_for_each_entry(region, &ocelot->stats_regions, node) {
> > +		region->buf = devm_kcalloc(ocelot->dev, region->count,
> > +					   sizeof(*region->buf), GFP_KERNEL);
> > +		if (!region->buf)
> > +			return -ENOMEM;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  int ocelot_get_ts_info(struct ocelot *ocelot, int port,
> >  		       struct ethtool_ts_info *info)
> >  {
> > @@ -2799,6 +2844,13 @@ int ocelot_init(struct ocelot *ocelot)
> >  				 ANA_CPUQ_8021_CFG_CPUQ_BPDU_VAL(6),
> >  				 ANA_CPUQ_8021_CFG, i);
> >  
> > +	ret = ocelot_prepare_stats_regions(ocelot);
> > +	if (ret) {
> > +		destroy_workqueue(ocelot->stats_queue);
> > +		destroy_workqueue(ocelot->owq);
> > +		return ret;
> > +	}
> > +
> >  	INIT_DELAYED_WORK(&ocelot->stats_work, ocelot_check_stats_work);
> >  	queue_delayed_work(ocelot->stats_queue, &ocelot->stats_work,
> >  			   OCELOT_STATS_CHECK_DELAY);
> > diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
> > index 312b72558659..d3291a5f7e88 100644
> > --- a/include/soc/mscc/ocelot.h
> > +++ b/include/soc/mscc/ocelot.h
> > @@ -542,6 +542,13 @@ struct ocelot_stat_layout {
> >  	char name[ETH_GSTRING_LEN];
> >  };
> >  
> > +struct ocelot_stats_region {
> > +	struct list_head node;
> > +	u32 offset;
> > +	int count;
> > +	u32 *buf;
> > +};
> > +
> >  enum ocelot_tag_prefix {
> >  	OCELOT_TAG_PREFIX_DISABLED	= 0,
> >  	OCELOT_TAG_PREFIX_NONE,
> > @@ -673,6 +680,7 @@ struct ocelot {
> >  	struct regmap_field		*regfields[REGFIELD_MAX];
> >  	const u32 *const		*map;
> >  	const struct ocelot_stat_layout	*stats_layout;
> > +	struct list_head		stats_regions;
> >  	unsigned int			num_stats;
> >  
> >  	u32				pool_size[OCELOT_SB_NUM][OCELOT_SB_POOL_NUM];
> > -- 
> > 2.25.1
> >

Re: [PATCH v5 net-next 3/3] net: mscc: ocelot: use bulk reads for stats

[Vladimir Oltean]

On Tue, Feb 08, 2022 at 07:41:56AM -0800, Colin Foster wrote:
> I see that now. It is confusing and I'll clear it up. I never caught
> this because I'm testing in a setup where port 0 is the CPU port, so I
> can't get ethtool stats. Thanks!

You can retrieve the stats on the CPU port, as they are appended to the
ethtool stats of the DSA master, prefixed with "p%02d_", cpu_dp->index.

ethtool -S eth0 | grep -v ': 0' # where eth0 is the DSA master

Re: [PATCH v5 net-next 3/3] net: mscc: ocelot: use bulk reads for stats

[Colin Foster]

On Tue, Feb 08, 2022 at 03:34:49PM +0000, Vladimir Oltean wrote:
> On Tue, Feb 08, 2022 at 05:03:03PM +0200, Vladimir Oltean wrote:
> > >  	for (i = 0; i < ocelot->num_phys_ports; i++) {
> > > +		unsigned int idx = 0;
> > > +
> > 
> > This is a bug which causes ocelot->stats to be overwritten with the
> > statistics of port 0, for all ports. Either move the variable
> > declaration and initialization with 0 in the larger scope (outside the
> > "for" loop), or initialize idx with i * ocelot->num_stats.
> 
> My analysis was slightly incorrect. Somehow I managed to fool myself
> into thinking that you had tested this in a limited scenario, hence the
> reason you didn't notice it's not working. But apparently you didn't
> test with traffic at all.
> 
> So ocelot->stats isn't overwritten with the stats of port 0 for all
> ports. But rather, all ports write into the ocelot->stats space
> dedicated for port 0, effectively overwriting the stats of port 0 with
> the stats of the last port. And no one populates the ocelot->stats space
> for ports [1 .. last]. So no port has good statistics, I don't see a
> circumstance where testing could have misled you.

Both ethtool -S and debugfs show statistics that I'd expect in my test
setup. But yes, the port 0 stats would be especially curious.

EDIT: Just saw your message about these. Yes, port 0 is all messed up:
# ethtool -S eth0 | grep p00 | head
     p00_rx_octets: 13602161426432
     p00_rx_unicast: 4539780431872
     p00_rx_multicast: 9028021256192
     p00_rx_broadcast: 8980776615936
     p00_rx_shorts: 0
     p00_rx_fragments: 0
     p00_rx_jabbers: 0
     p00_rx_crc_align_errs: 0
     p00_rx_sym_errs: 0
     p00_rx_frames_below_65_octets: 4539780431872


(configuration - swp2 plugged into swp3, bridged with stp. swp1 into my
development machine, swp4-7 not up)

# pwd
/sys/class/net
# cat swp[123]/statistics/tx_bytes
44616
44668
52
# cat swp[123]/statistics/rx_bytes
34574
46
73272

# ethtool -S swp1 | head -n 5
NIC statistics:
     tx_packets: 942
     tx_bytes: 48984
     rx_packets: 764
     rx_bytes: 37808
# ethtool -S swp2 | head -n 5
NIC statistics:
     tx_packets: 946
     tx_bytes: 49192
     rx_packets: 1
     rx_bytes: 46
# ethtool -S swp3 | head -n 5
NIC statistics:
     tx_packets: 1
     tx_bytes: 52
     rx_packets: 1699
     rx_bytes: 80658
# ethtool -S swp4 | head -n 5
NIC statistics:
     tx_packets: 0
     tx_bytes: 0
     rx_packets: 0
     rx_bytes: 0
# ip link
1: lo: <LOOPBACK,UP,LOWER_UP> mtu 65536 qdisc noqueue state UNKNOWN mode DEFAULT group default qlen 1000
    link/loopback 00:00:00:00:00:00 brd 00:00:00:00:00:00
2: eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1520 qdisc mq state UP mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
3: swp1@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master br0 state UP mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
4: swp2@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master br0 state UP mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
5: swp3@eth0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue master br0 state UP mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
6: swp4@eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
7: swp5@eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
8: swp6@eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
9: swp7@eth0: <BROADCAST,MULTICAST> mtu 1500 qdisc noop state DOWN mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff
10: br0: <BROADCAST,MULTICAST,UP,LOWER_UP> mtu 1500 qdisc noqueue state UP mode DEFAULT group default qlen 1000
    link/ether 24:76:25:76:35:37 brd ff:ff:ff:ff:ff:ff


Clearly my testing wasn't sufficient as there were still issues, but
there were reasonable signs of things working as expected on ports 1-4.

Re: [PATCH v5 net-next 3/3] net: mscc: ocelot: use bulk reads for stats

[Vladimir Oltean]

On Tue, Feb 08, 2022 at 08:07:42AM -0800, Colin Foster wrote:
> # ethtool -S swp1 | head -n 5
> NIC statistics:
>      tx_packets: 942
>      tx_bytes: 48984
>      rx_packets: 764
>      rx_bytes: 37808
> # ethtool -S swp2 | head -n 5
> NIC statistics:
>      tx_packets: 946
>      tx_bytes: 49192
>      rx_packets: 1
>      rx_bytes: 46
> # ethtool -S swp3 | head -n 5
> NIC statistics:
>      tx_packets: 1
>      tx_bytes: 52
>      rx_packets: 1699
>      rx_bytes: 80658
> # ethtool -S swp4 | head -n 5
> NIC statistics:
>      tx_packets: 0
>      tx_bytes: 0
>      rx_packets: 0
>      rx_bytes: 0

I understand what confused you now. These are software counters patched
in by dsa_slave_get_ethtool_stats(). They are in no way the counters
from felix_info :: stats_layout, please compare the strings!

Re: [PATCH v5 net-next 3/3] net: mscc: ocelot: use bulk reads for stats

[Colin Foster]

On Tue, Feb 08, 2022 at 03:45:15PM +0000, Vladimir Oltean wrote:
> On Tue, Feb 08, 2022 at 07:41:56AM -0800, Colin Foster wrote:
> > I see that now. It is confusing and I'll clear it up. I never caught
> > this because I'm testing in a setup where port 0 is the CPU port, so I
> > can't get ethtool stats. Thanks!
> 
> You can retrieve the stats on the CPU port, as they are appended to the
> ethtool stats of the DSA master, prefixed with "p%02d_", cpu_dp->index.
> 
> ethtool -S eth0 | grep -v ': 0' # where eth0 is the DSA master

Thanks for this hint. I didn't even think to go looking for it since the
DSA Documenation mentions an "inability to fetch CPU port statistics
counters using ethtool." I see now that this bullet point is from 2015,
and probably should be removed entirely.

Re: [PATCH v5 net-next 3/3] net: mscc: ocelot: use bulk reads for stats

[Vladimir Oltean]

On Tue, Feb 08, 2022 at 08:49:52AM -0800, Colin Foster wrote:
> On Tue, Feb 08, 2022 at 03:45:15PM +0000, Vladimir Oltean wrote:
> > On Tue, Feb 08, 2022 at 07:41:56AM -0800, Colin Foster wrote:
> > > I see that now. It is confusing and I'll clear it up. I never caught
> > > this because I'm testing in a setup where port 0 is the CPU port, so I
> > > can't get ethtool stats. Thanks!
> > 
> > You can retrieve the stats on the CPU port, as they are appended to the
> > ethtool stats of the DSA master, prefixed with "p%02d_", cpu_dp->index.
> > 
> > ethtool -S eth0 | grep -v ': 0' # where eth0 is the DSA master
> 
> Thanks for this hint. I didn't even think to go looking for it since the
> DSA Documenation mentions an "inability to fetch CPU port statistics
> counters using ethtool." I see now that this bullet point is from 2015,
> and probably should be removed entirely.

Yes, that paragraph did not age well, although it is still true in some
sense, just not in that phrasing. You still cannot collect ethtool
statistics for cascade (DSA) ports. Maybe it should be updated.