The cleanup mostly handles the statistics code path - some issues
regarding understandability became apparent after the series
"Fix trainwreck with Ocelot switch statistics counters":
https://lore.kernel.org/netdev/[email protected]/
There is also one patch which cleans up a misleading comment
in the DSA felix_setup().
Vladimir Oltean (8):
net: mscc: ocelot: strengthen type of "u32 reg" in I/O accessors
net: mscc: ocelot: refactor enum ocelot_reg decoding to helper
net: mscc: ocelot: debugging print for statistics regions
net: mscc: ocelot: remove blank line at the end of ocelot_stats.c
net: dsa: felix: remove confusing/incorrect comment from felix_setup()
net: mscc: ocelot: strengthen type of "u32 reg" and "u32 base" in
ocelot_stats.c
net: mscc: ocelot: strengthen type of "int i" in ocelot_stats.c
net: mscc: ocelot: fix ineffective WARN_ON() in ocelot_stats.c
drivers/net/dsa/ocelot/felix.c | 5 ---
drivers/net/ethernet/mscc/ocelot.h | 9 +++++
drivers/net/ethernet/mscc/ocelot_io.c | 50 +++++++++++++-----------
drivers/net/ethernet/mscc/ocelot_stats.c | 42 +++++++++++++-------
include/soc/mscc/ocelot.h | 20 +++++-----
5 files changed, 75 insertions(+), 51 deletions(-)
--
2.34.1
The "u32 reg" argument that is passed to these functions is not a plain
address, but rather a driver-specific encoding of another enum
ocelot_target target in the upper bits, and an index into the
u32 ocelot->map[target][] array in the lower bits. That encoded value
takes the type "enum ocelot_reg" and is what is passed to these I/O
functions, so let's actually use that to prevent type confusion.
Signed-off-by: Vladimir Oltean <[email protected]>
---
drivers/net/ethernet/mscc/ocelot_io.c | 20 +++++++++++---------
include/soc/mscc/ocelot.h | 20 +++++++++++---------
2 files changed, 22 insertions(+), 18 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot_io.c b/drivers/net/ethernet/mscc/ocelot_io.c
index 2067382d0ee1..ddb96f32830d 100644
--- a/drivers/net/ethernet/mscc/ocelot_io.c
+++ b/drivers/net/ethernet/mscc/ocelot_io.c
@@ -10,8 +10,8 @@
#include "ocelot.h"
-int __ocelot_bulk_read_ix(struct ocelot *ocelot, u32 reg, u32 offset, void *buf,
- int count)
+int __ocelot_bulk_read_ix(struct ocelot *ocelot, enum ocelot_reg reg,
+ u32 offset, void *buf, int count)
{
u16 target = reg >> TARGET_OFFSET;
@@ -23,7 +23,7 @@ int __ocelot_bulk_read_ix(struct ocelot *ocelot, u32 reg, u32 offset, void *buf,
}
EXPORT_SYMBOL_GPL(__ocelot_bulk_read_ix);
-u32 __ocelot_read_ix(struct ocelot *ocelot, u32 reg, u32 offset)
+u32 __ocelot_read_ix(struct ocelot *ocelot, enum ocelot_reg reg, u32 offset)
{
u16 target = reg >> TARGET_OFFSET;
u32 val;
@@ -36,7 +36,8 @@ u32 __ocelot_read_ix(struct ocelot *ocelot, u32 reg, u32 offset)
}
EXPORT_SYMBOL_GPL(__ocelot_read_ix);
-void __ocelot_write_ix(struct ocelot *ocelot, u32 val, u32 reg, u32 offset)
+void __ocelot_write_ix(struct ocelot *ocelot, u32 val, enum ocelot_reg reg,
+ u32 offset)
{
u16 target = reg >> TARGET_OFFSET;
@@ -47,8 +48,8 @@ void __ocelot_write_ix(struct ocelot *ocelot, u32 val, u32 reg, u32 offset)
}
EXPORT_SYMBOL_GPL(__ocelot_write_ix);
-void __ocelot_rmw_ix(struct ocelot *ocelot, u32 val, u32 mask, u32 reg,
- u32 offset)
+void __ocelot_rmw_ix(struct ocelot *ocelot, u32 val, u32 mask,
+ enum ocelot_reg reg, u32 offset)
{
u16 target = reg >> TARGET_OFFSET;
@@ -60,7 +61,7 @@ void __ocelot_rmw_ix(struct ocelot *ocelot, u32 val, u32 mask, u32 reg,
}
EXPORT_SYMBOL_GPL(__ocelot_rmw_ix);
-u32 ocelot_port_readl(struct ocelot_port *port, u32 reg)
+u32 ocelot_port_readl(struct ocelot_port *port, enum ocelot_reg reg)
{
struct ocelot *ocelot = port->ocelot;
u16 target = reg >> TARGET_OFFSET;
@@ -73,7 +74,7 @@ u32 ocelot_port_readl(struct ocelot_port *port, u32 reg)
}
EXPORT_SYMBOL_GPL(ocelot_port_readl);
-void ocelot_port_writel(struct ocelot_port *port, u32 val, u32 reg)
+void ocelot_port_writel(struct ocelot_port *port, u32 val, enum ocelot_reg reg)
{
struct ocelot *ocelot = port->ocelot;
u16 target = reg >> TARGET_OFFSET;
@@ -84,7 +85,8 @@ void ocelot_port_writel(struct ocelot_port *port, u32 val, u32 reg)
}
EXPORT_SYMBOL_GPL(ocelot_port_writel);
-void ocelot_port_rmwl(struct ocelot_port *port, u32 val, u32 mask, u32 reg)
+void ocelot_port_rmwl(struct ocelot_port *port, u32 val, u32 mask,
+ enum ocelot_reg reg)
{
u32 cur = ocelot_port_readl(port, reg);
diff --git a/include/soc/mscc/ocelot.h b/include/soc/mscc/ocelot.h
index c0e40ceba50c..85505ac5e63e 100644
--- a/include/soc/mscc/ocelot.h
+++ b/include/soc/mscc/ocelot.h
@@ -943,15 +943,17 @@ struct ocelot_policer {
__ocelot_target_write_ix(ocelot, target, val, reg, 0)
/* I/O */
-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,
- u32 offset);
+u32 ocelot_port_readl(struct ocelot_port *port, enum ocelot_reg reg);
+void ocelot_port_writel(struct ocelot_port *port, u32 val, enum ocelot_reg reg);
+void ocelot_port_rmwl(struct ocelot_port *port, u32 val, u32 mask,
+ enum ocelot_reg reg);
+int __ocelot_bulk_read_ix(struct ocelot *ocelot, enum ocelot_reg reg,
+ u32 offset, void *buf, int count);
+u32 __ocelot_read_ix(struct ocelot *ocelot, enum ocelot_reg reg, u32 offset);
+void __ocelot_write_ix(struct ocelot *ocelot, u32 val, enum ocelot_reg reg,
+ u32 offset);
+void __ocelot_rmw_ix(struct ocelot *ocelot, u32 val, u32 mask,
+ enum ocelot_reg reg, u32 offset);
u32 __ocelot_target_read_ix(struct ocelot *ocelot, enum ocelot_target target,
u32 reg, u32 offset);
void __ocelot_target_write_ix(struct ocelot *ocelot, enum ocelot_target target,
--
2.34.1
ocelot_io.c duplicates the decoding of an enum ocelot_reg (which holds
an enum ocelot_target in the upper bits and an index into a regmap array
in the lower bits) 4 times.
We'd like to reuse that logic once more, from ocelot.c. In order to do
that, let's consolidate the existing 4 instances into a header
accessible both by ocelot.c as well as by ocelot_io.c.
Signed-off-by: Vladimir Oltean <[email protected]>
---
drivers/net/ethernet/mscc/ocelot.h | 9 ++++++++
drivers/net/ethernet/mscc/ocelot_io.c | 30 ++++++++++++++-------------
2 files changed, 25 insertions(+), 14 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
index 9e0f2e4ed556..14440a3b04c3 100644
--- a/drivers/net/ethernet/mscc/ocelot.h
+++ b/drivers/net/ethernet/mscc/ocelot.h
@@ -74,6 +74,15 @@ struct ocelot_multicast {
struct ocelot_pgid *pgid;
};
+static inline void ocelot_reg_to_target_addr(struct ocelot *ocelot,
+ enum ocelot_reg reg,
+ enum ocelot_target *target,
+ u32 *addr)
+{
+ *target = reg >> TARGET_OFFSET;
+ *addr = ocelot->map[*target][reg & REG_MASK];
+}
+
int ocelot_bridge_num_find(struct ocelot *ocelot,
const struct net_device *bridge);
diff --git a/drivers/net/ethernet/mscc/ocelot_io.c b/drivers/net/ethernet/mscc/ocelot_io.c
index ddb96f32830d..3aa7dc29ebe1 100644
--- a/drivers/net/ethernet/mscc/ocelot_io.c
+++ b/drivers/net/ethernet/mscc/ocelot_io.c
@@ -13,25 +13,26 @@
int __ocelot_bulk_read_ix(struct ocelot *ocelot, enum ocelot_reg reg,
u32 offset, void *buf, int count)
{
- u16 target = reg >> TARGET_OFFSET;
+ enum ocelot_target target;
+ u32 addr;
+ ocelot_reg_to_target_addr(ocelot, reg, &target, &addr);
WARN_ON(!target);
- return regmap_bulk_read(ocelot->targets[target],
- ocelot->map[target][reg & REG_MASK] + offset,
+ return regmap_bulk_read(ocelot->targets[target], addr + offset,
buf, count);
}
EXPORT_SYMBOL_GPL(__ocelot_bulk_read_ix);
u32 __ocelot_read_ix(struct ocelot *ocelot, enum ocelot_reg reg, u32 offset)
{
- u16 target = reg >> TARGET_OFFSET;
- u32 val;
+ enum ocelot_target target;
+ u32 addr, val;
+ ocelot_reg_to_target_addr(ocelot, reg, &target, &addr);
WARN_ON(!target);
- regmap_read(ocelot->targets[target],
- ocelot->map[target][reg & REG_MASK] + offset, &val);
+ regmap_read(ocelot->targets[target], addr + offset, &val);
return val;
}
EXPORT_SYMBOL_GPL(__ocelot_read_ix);
@@ -39,25 +40,26 @@ EXPORT_SYMBOL_GPL(__ocelot_read_ix);
void __ocelot_write_ix(struct ocelot *ocelot, u32 val, enum ocelot_reg reg,
u32 offset)
{
- u16 target = reg >> TARGET_OFFSET;
+ enum ocelot_target target;
+ u32 addr;
+ ocelot_reg_to_target_addr(ocelot, reg, &target, &addr);
WARN_ON(!target);
- regmap_write(ocelot->targets[target],
- ocelot->map[target][reg & REG_MASK] + offset, val);
+ regmap_write(ocelot->targets[target], addr + offset, val);
}
EXPORT_SYMBOL_GPL(__ocelot_write_ix);
void __ocelot_rmw_ix(struct ocelot *ocelot, u32 val, u32 mask,
enum ocelot_reg reg, u32 offset)
{
- u16 target = reg >> TARGET_OFFSET;
+ enum ocelot_target target;
+ u32 addr;
+ ocelot_reg_to_target_addr(ocelot, reg, &target, &addr);
WARN_ON(!target);
- regmap_update_bits(ocelot->targets[target],
- ocelot->map[target][reg & REG_MASK] + offset,
- mask, val);
+ regmap_update_bits(ocelot->targets[target], addr + offset, mask, val);
}
EXPORT_SYMBOL_GPL(__ocelot_rmw_ix);
--
2.34.1
Commit a3bb8f521fd8 ("net: mscc: ocelot: remove unnecessary exposure of
stats structures") made an unnecessary change which was to add a new
line at the end of ocelot_stats.c. Remove it.
Signed-off-by: Vladimir Oltean <[email protected]>
---
drivers/net/ethernet/mscc/ocelot_stats.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/net/ethernet/mscc/ocelot_stats.c b/drivers/net/ethernet/mscc/ocelot_stats.c
index b50d9d9f8023..99a14a942600 100644
--- a/drivers/net/ethernet/mscc/ocelot_stats.c
+++ b/drivers/net/ethernet/mscc/ocelot_stats.c
@@ -981,4 +981,3 @@ void ocelot_stats_deinit(struct ocelot *ocelot)
cancel_delayed_work(&ocelot->stats_work);
destroy_workqueue(ocelot->stats_queue);
}
-
--
2.34.1
Since it is hopefully now clear that, since "last" and "layout[i].reg"
are enum types and not addresses, the existing WARN_ON() is ineffective
in checking that the _addresses_ are sorted in the proper order.
Signed-off-by: Vladimir Oltean <[email protected]>
---
drivers/net/ethernet/mscc/ocelot_stats.c | 17 +++++++++++------
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot_stats.c b/drivers/net/ethernet/mscc/ocelot_stats.c
index e82c9d9d0ad3..5c55197c7327 100644
--- a/drivers/net/ethernet/mscc/ocelot_stats.c
+++ b/drivers/net/ethernet/mscc/ocelot_stats.c
@@ -901,6 +901,17 @@ static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
if (!layout[i].reg)
continue;
+ /* enum ocelot_stat must be kept sorted in the same order
+ * as the addresses behind layout[i].reg in order to have
+ * efficient bulking
+ */
+ if (last) {
+ WARN(ocelot->map[SYS][last & REG_MASK] >= ocelot->map[SYS][layout[i].reg & REG_MASK],
+ "reg 0x%x had address 0x%x but reg 0x%x has address 0x%x, bulking broken!",
+ last, ocelot->map[SYS][last & REG_MASK],
+ layout[i].reg, ocelot->map[SYS][layout[i].reg & REG_MASK]);
+ }
+
if (region && ocelot->map[SYS][layout[i].reg & REG_MASK] ==
ocelot->map[SYS][last & REG_MASK] + 4) {
region->count++;
@@ -910,12 +921,6 @@ static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
if (!region)
return -ENOMEM;
- /* enum ocelot_stat must be kept sorted in the same
- * order as layout[i].reg in order to have efficient
- * bulking
- */
- WARN_ON(last >= layout[i].reg);
-
region->base = layout[i].reg;
region->first_stat = i;
region->count = 1;
--
2.34.1
That comment was written prior to knowing that what I was actually
seeing was a manifestation of the bug fixed in commit b4024c9e5c57
("felix: Fix initialization of ioremap resources").
There isn't any particular reason now why the hardware initialization is
done in felix_setup(), so just delete that comment to avoid spreading
misinformation.
Signed-off-by: Vladimir Oltean <[email protected]>
---
drivers/net/dsa/ocelot/felix.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
index 6dcebcfd71e7..80861ac090ae 100644
--- a/drivers/net/dsa/ocelot/felix.c
+++ b/drivers/net/dsa/ocelot/felix.c
@@ -1550,11 +1550,6 @@ static int felix_connect_tag_protocol(struct dsa_switch *ds,
}
}
-/* Hardware initialization done here so that we can allocate structures with
- * devm without fear of dsa_register_switch returning -EPROBE_DEFER and causing
- * us to allocate structures twice (leak memory) and map PCI memory twice
- * (which will not work).
- */
static int felix_setup(struct dsa_switch *ds)
{
struct ocelot *ocelot = ds->priv;
--
2.34.1
The "int i" used to index the struct ocelot_stat_layout array actually
has a specific type: enum ocelot_stat. Use it, so that the WARN()
comment from ocelot_prepare_stats_regions() makes more sense.
Signed-off-by: Vladimir Oltean <[email protected]>
---
drivers/net/ethernet/mscc/ocelot_stats.c | 9 +++++----
1 file changed, 5 insertions(+), 4 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot_stats.c b/drivers/net/ethernet/mscc/ocelot_stats.c
index a381e326cb2b..e82c9d9d0ad3 100644
--- a/drivers/net/ethernet/mscc/ocelot_stats.c
+++ b/drivers/net/ethernet/mscc/ocelot_stats.c
@@ -395,7 +395,7 @@ static void ocelot_check_stats_work(struct work_struct *work)
void ocelot_get_strings(struct ocelot *ocelot, int port, u32 sset, u8 *data)
{
const struct ocelot_stat_layout *layout;
- int i;
+ enum ocelot_stat i;
if (sset != ETH_SS_STATS)
return;
@@ -442,7 +442,8 @@ static void ocelot_port_stats_run(struct ocelot *ocelot, int port, void *priv,
int ocelot_get_sset_count(struct ocelot *ocelot, int port, int sset)
{
const struct ocelot_stat_layout *layout;
- int i, num_stats = 0;
+ enum ocelot_stat i;
+ int num_stats = 0;
if (sset != ETH_SS_STATS)
return -EOPNOTSUPP;
@@ -461,8 +462,8 @@ static void ocelot_port_ethtool_stats_cb(struct ocelot *ocelot, int port,
void *priv)
{
const struct ocelot_stat_layout *layout;
+ enum ocelot_stat i;
u64 *data = priv;
- int i;
layout = ocelot_get_stats_layout(ocelot);
@@ -890,7 +891,7 @@ static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
struct ocelot_stats_region *region = NULL;
const struct ocelot_stat_layout *layout;
enum ocelot_reg last = 0;
- int i;
+ enum ocelot_stat i;
INIT_LIST_HEAD(&ocelot->stats_regions);
--
2.34.1
To make it easier to debug future issues with statistics counters not
getting aggregated properly into regions, like what happened in commit
6acc72a43eac ("net: mscc: ocelot: fix stats region batching"), add some
dev_dbg() prints which show the regions that were dynamically
determined.
Signed-off-by: Vladimir Oltean <[email protected]>
---
drivers/net/ethernet/mscc/ocelot_stats.c | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/drivers/net/ethernet/mscc/ocelot_stats.c b/drivers/net/ethernet/mscc/ocelot_stats.c
index d0e6cd8dbe5c..b50d9d9f8023 100644
--- a/drivers/net/ethernet/mscc/ocelot_stats.c
+++ b/drivers/net/ethernet/mscc/ocelot_stats.c
@@ -925,6 +925,15 @@ static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
}
list_for_each_entry(region, &ocelot->stats_regions, node) {
+ enum ocelot_target target;
+ u32 addr;
+
+ ocelot_reg_to_target_addr(ocelot, region->base, &target,
+ &addr);
+
+ dev_dbg(ocelot->dev,
+ "region of %d contiguous counters starting with SYS:STAT:CNT[0x%03x]\n",
+ region->count, addr / 4);
region->buf = devm_kcalloc(ocelot->dev, region->count,
sizeof(*region->buf), GFP_KERNEL);
if (!region->buf)
--
2.34.1
Use the specific enum ocelot_reg to make it clear that the region
registers are encoded and not plain addresses.
Signed-off-by: Vladimir Oltean <[email protected]>
---
drivers/net/ethernet/mscc/ocelot_stats.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/drivers/net/ethernet/mscc/ocelot_stats.c b/drivers/net/ethernet/mscc/ocelot_stats.c
index 99a14a942600..a381e326cb2b 100644
--- a/drivers/net/ethernet/mscc/ocelot_stats.c
+++ b/drivers/net/ethernet/mscc/ocelot_stats.c
@@ -145,7 +145,7 @@ enum ocelot_stat {
};
struct ocelot_stat_layout {
- u32 reg;
+ enum ocelot_reg reg;
char name[ETH_GSTRING_LEN];
};
@@ -257,7 +257,7 @@ struct ocelot_stat_layout {
struct ocelot_stats_region {
struct list_head node;
- u32 base;
+ enum ocelot_reg base;
enum ocelot_stat first_stat;
int count;
u32 *buf;
@@ -889,7 +889,7 @@ static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
{
struct ocelot_stats_region *region = NULL;
const struct ocelot_stat_layout *layout;
- unsigned int last = 0;
+ enum ocelot_reg last = 0;
int i;
INIT_LIST_HEAD(&ocelot->stats_regions);
--
2.34.1
On Wed, Apr 12, 2023 at 03:47:33PM +0300, Vladimir Oltean wrote:
> Commit a3bb8f521fd8 ("net: mscc: ocelot: remove unnecessary exposure of
> stats structures") made an unnecessary change which was to add a new
> line at the end of ocelot_stats.c. Remove it.
Yes it did. Apologies.
Acked-by: Colin Foster <[email protected]>
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
> drivers/net/ethernet/mscc/ocelot_stats.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mscc/ocelot_stats.c b/drivers/net/ethernet/mscc/ocelot_stats.c
> index b50d9d9f8023..99a14a942600 100644
> --- a/drivers/net/ethernet/mscc/ocelot_stats.c
> +++ b/drivers/net/ethernet/mscc/ocelot_stats.c
> @@ -981,4 +981,3 @@ void ocelot_stats_deinit(struct ocelot *ocelot)
> cancel_delayed_work(&ocelot->stats_work);
> destroy_workqueue(ocelot->stats_queue);
> }
> -
> --
> 2.34.1
>
On 4/12/2023 5:47 AM, Vladimir Oltean wrote:
> The "u32 reg" argument that is passed to these functions is not a plain
> address, but rather a driver-specific encoding of another enum
> ocelot_target target in the upper bits, and an index into the
> u32 ocelot->map[target][] array in the lower bits. That encoded value
> takes the type "enum ocelot_reg" and is what is passed to these I/O
> functions, so let's actually use that to prevent type confusion.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
It does make the prototypes a bit longer, but clarity of the type of
value you need to pass is good.
Reviewed-by: Jacob Keller <[email protected]>
On 4/12/2023 5:47 AM, Vladimir Oltean wrote:
> ocelot_io.c duplicates the decoding of an enum ocelot_reg (which holds
> an enum ocelot_target in the upper bits and an index into a regmap array
> in the lower bits) 4 times.
>
> We'd like to reuse that logic once more, from ocelot.c. In order to do
> that, let's consolidate the existing 4 instances into a header
> accessible both by ocelot.c as well as by ocelot_io.c.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
> drivers/net/ethernet/mscc/ocelot.h | 9 ++++++++
> drivers/net/ethernet/mscc/ocelot_io.c | 30 ++++++++++++++-------------
> 2 files changed, 25 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/net/ethernet/mscc/ocelot.h b/drivers/net/ethernet/mscc/ocelot.h
> index 9e0f2e4ed556..14440a3b04c3 100644
> --- a/drivers/net/ethernet/mscc/ocelot.h
> +++ b/drivers/net/ethernet/mscc/ocelot.h
> @@ -74,6 +74,15 @@ struct ocelot_multicast {
> struct ocelot_pgid *pgid;
> };
>
> +static inline void ocelot_reg_to_target_addr(struct ocelot *ocelot,
> + enum ocelot_reg reg,
> + enum ocelot_target *target,
> + u32 *addr)
> +{
> + *target = reg >> TARGET_OFFSET;
> + *addr = ocelot->map[*target][reg & REG_MASK];
> +}
> +
Ok this takes a reg and returns it split into target and address, so you
can't just directly return the value.
You could do this with two separate functions, but thats not really any
better. I do wish it was easier to return tuples from a C function, but
alas...
Reviewed-by: Jacob Keller <[email protected]>
On 4/12/2023 5:47 AM, Vladimir Oltean wrote:
> To make it easier to debug future issues with statistics counters not
> getting aggregated properly into regions, like what happened in commit
> 6acc72a43eac ("net: mscc: ocelot: fix stats region batching"), add some
> dev_dbg() prints which show the regions that were dynamically
> determined.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
> drivers/net/ethernet/mscc/ocelot_stats.c | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/drivers/net/ethernet/mscc/ocelot_stats.c b/drivers/net/ethernet/mscc/ocelot_stats.c
> index d0e6cd8dbe5c..b50d9d9f8023 100644
> --- a/drivers/net/ethernet/mscc/ocelot_stats.c
> +++ b/drivers/net/ethernet/mscc/ocelot_stats.c
> @@ -925,6 +925,15 @@ static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
> }
>
> list_for_each_entry(region, &ocelot->stats_regions, node) {
> + enum ocelot_target target;
> + u32 addr;
> +
> + ocelot_reg_to_target_addr(ocelot, region->base, &target,
> + &addr);
> +
> + dev_dbg(ocelot->dev,
> + "region of %d contiguous counters starting with SYS:STAT:CNT[0x%03x]\n",
> + region->count, addr / 4);
This will load the target and addr every time even when dev_dbg isn't
activated, but thats not a huge cost.
Reviewed-by: Jacob Keller <[email protected]>
> region->buf = devm_kcalloc(ocelot->dev, region->count,
> sizeof(*region->buf), GFP_KERNEL);
> if (!region->buf)
On 4/12/2023 5:47 AM, Vladimir Oltean wrote:
> Commit a3bb8f521fd8 ("net: mscc: ocelot: remove unnecessary exposure of
> stats structures") made an unnecessary change which was to add a new
> line at the end of ocelot_stats.c. Remove it.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
> drivers/net/ethernet/mscc/ocelot_stats.c | 1 -
> 1 file changed, 1 deletion(-)
>
> diff --git a/drivers/net/ethernet/mscc/ocelot_stats.c b/drivers/net/ethernet/mscc/ocelot_stats.c
> index b50d9d9f8023..99a14a942600 100644
> --- a/drivers/net/ethernet/mscc/ocelot_stats.c
> +++ b/drivers/net/ethernet/mscc/ocelot_stats.c
> @@ -981,4 +981,3 @@ void ocelot_stats_deinit(struct ocelot *ocelot)
> cancel_delayed_work(&ocelot->stats_work);
> destroy_workqueue(ocelot->stats_queue);
> }
> -
Reviewed-by: Jacob Keller <[email protected]>
On 4/12/2023 5:47 AM, Vladimir Oltean wrote:
> That comment was written prior to knowing that what I was actually
> seeing was a manifestation of the bug fixed in commit b4024c9e5c57
> ("felix: Fix initialization of ioremap resources").
>
> There isn't any particular reason now why the hardware initialization is
> done in felix_setup(), so just delete that comment to avoid spreading
> misinformation.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
> drivers/net/dsa/ocelot/felix.c | 5 -----
> 1 file changed, 5 deletions(-)
>
> diff --git a/drivers/net/dsa/ocelot/felix.c b/drivers/net/dsa/ocelot/felix.c
> index 6dcebcfd71e7..80861ac090ae 100644
> --- a/drivers/net/dsa/ocelot/felix.c
> +++ b/drivers/net/dsa/ocelot/felix.c
> @@ -1550,11 +1550,6 @@ static int felix_connect_tag_protocol(struct dsa_switch *ds,
> }
> }
>
> -/* Hardware initialization done here so that we can allocate structures with
> - * devm without fear of dsa_register_switch returning -EPROBE_DEFER and causing
> - * us to allocate structures twice (leak memory) and map PCI memory twice
> - * (which will not work).
> - */
> static int felix_setup(struct dsa_switch *ds)
> {
> struct ocelot *ocelot = ds->priv;
Reviewed-by: Jacob Keller <[email protected]>
On 4/12/2023 5:47 AM, Vladimir Oltean wrote:
> Use the specific enum ocelot_reg to make it clear that the region
> registers are encoded and not plain addresses.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
> drivers/net/ethernet/mscc/ocelot_stats.c | 6 +++---
> 1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/net/ethernet/mscc/ocelot_stats.c b/drivers/net/ethernet/mscc/ocelot_stats.c
> index 99a14a942600..a381e326cb2b 100644
> --- a/drivers/net/ethernet/mscc/ocelot_stats.c
> +++ b/drivers/net/ethernet/mscc/ocelot_stats.c
> @@ -145,7 +145,7 @@ enum ocelot_stat {
> };
>
> struct ocelot_stat_layout {
> - u32 reg;
> + enum ocelot_reg reg;
> char name[ETH_GSTRING_LEN];
> };
>
> @@ -257,7 +257,7 @@ struct ocelot_stat_layout {
>
> struct ocelot_stats_region {
> struct list_head node;
> - u32 base;
> + enum ocelot_reg base;
> enum ocelot_stat first_stat;
> int count;
> u32 *buf;
> @@ -889,7 +889,7 @@ static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
> {
> struct ocelot_stats_region *region = NULL;
> const struct ocelot_stat_layout *layout;
> - unsigned int last = 0;
> + enum ocelot_reg last = 0;
> int i;
>
> INIT_LIST_HEAD(&ocelot->stats_regions);
Reviewed-by: Jacob Keller <[email protected]>
On 4/12/2023 5:47 AM, Vladimir Oltean wrote:
> The "int i" used to index the struct ocelot_stat_layout array actually
> has a specific type: enum ocelot_stat. Use it, so that the WARN()
> comment from ocelot_prepare_stats_regions() makes more sense.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
> drivers/net/ethernet/mscc/ocelot_stats.c | 9 +++++----
> 1 file changed, 5 insertions(+), 4 deletions(-)
>
For the lazy readers who didn't dig up the source outside the patch
context, the WARN in question is:
>
> /* enum ocelot_stat must be kept sorted in the same
> * order as layout[i].reg in order to have efficient
> * bulking
> */
> WARN_ON(last >= layout[i].reg);
Reviewed-by: Jacob Keller <[email protected]>
On 4/12/2023 5:47 AM, Vladimir Oltean wrote:
> Since it is hopefully now clear that, since "last" and "layout[i].reg"
> are enum types and not addresses, the existing WARN_ON() is ineffective
> in checking that the _addresses_ are sorted in the proper order.
>
> Signed-off-by: Vladimir Oltean <[email protected]>
> ---
This is definitely more clear after reviewing the other code dealing
with these encoded register addresses.
Nice fix.
Reviewed-by: Jacob Keller <[email protected]>
Thanks,
Jake
> drivers/net/ethernet/mscc/ocelot_stats.c | 17 +++++++++++------
> 1 file changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/net/ethernet/mscc/ocelot_stats.c b/drivers/net/ethernet/mscc/ocelot_stats.c
> index e82c9d9d0ad3..5c55197c7327 100644
> --- a/drivers/net/ethernet/mscc/ocelot_stats.c
> +++ b/drivers/net/ethernet/mscc/ocelot_stats.c
> @@ -901,6 +901,17 @@ static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
> if (!layout[i].reg)
> continue;
>
> + /* enum ocelot_stat must be kept sorted in the same order
> + * as the addresses behind layout[i].reg in order to have
> + * efficient bulking
> + */
> + if (last) {
> + WARN(ocelot->map[SYS][last & REG_MASK] >= ocelot->map[SYS][layout[i].reg & REG_MASK],
> + "reg 0x%x had address 0x%x but reg 0x%x has address 0x%x, bulking broken!",
> + last, ocelot->map[SYS][last & REG_MASK],
> + layout[i].reg, ocelot->map[SYS][layout[i].reg & REG_MASK]);
> + }
> +
> if (region && ocelot->map[SYS][layout[i].reg & REG_MASK] ==
> ocelot->map[SYS][last & REG_MASK] + 4) {
> region->count++;
> @@ -910,12 +921,6 @@ static int ocelot_prepare_stats_regions(struct ocelot *ocelot)
> if (!region)
> return -ENOMEM;
>
> - /* enum ocelot_stat must be kept sorted in the same
> - * order as layout[i].reg in order to have efficient
> - * bulking
> - */
> - WARN_ON(last >= layout[i].reg);
> -
> region->base = layout[i].reg;
> region->first_stat = i;
> region->count = 1;
Hi Vladimir,
On Wed, Apr 12, 2023 at 03:47:29PM +0300, Vladimir Oltean wrote:
> The cleanup mostly handles the statistics code path - some issues
> regarding understandability became apparent after the series
> "Fix trainwreck with Ocelot switch statistics counters":
> https://lore.kernel.org/netdev/[email protected]/
>
> There is also one patch which cleans up a misleading comment
> in the DSA felix_setup().
Sorry I won't have access to hardware until next week, so I can't add
any tested-bys. But this whole set is straightforward, it probably
isn't too necessary. Let me know if there's anything you want from me on
this set.
Colin
Hi Colin,
On Wed, Apr 12, 2023 at 05:45:34PM -0700, Colin Foster wrote:
> Sorry I won't have access to hardware until next week, so I can't add
> any tested-bys. But this whole set is straightforward, it probably
> isn't too necessary. Let me know if there's anything you want from me on
> this set.
I've tested the patches on Felix; hopefully there is no reason why they
would introduce regressions.
I copied you just to make sure you're aware of the changes, because it's
all code you've visited, contributed to, and which has confused you (and
me too).
Hello:
This series was applied to netdev/net-next.git (main)
by Jakub Kicinski <[email protected]>:
On Wed, 12 Apr 2023 15:47:29 +0300 you wrote:
> The cleanup mostly handles the statistics code path - some issues
> regarding understandability became apparent after the series
> "Fix trainwreck with Ocelot switch statistics counters":
> https://lore.kernel.org/netdev/[email protected]/
>
> There is also one patch which cleans up a misleading comment
> in the DSA felix_setup().
>
> [...]
Here is the summary with links:
- [net-next,1/8] net: mscc: ocelot: strengthen type of "u32 reg" in I/O accessors
https://git.kernel.org/netdev/net-next/c/9ecd05794b8d
- [net-next,2/8] net: mscc: ocelot: refactor enum ocelot_reg decoding to helper
https://git.kernel.org/netdev/net-next/c/40cd07cb4261
- [net-next,3/8] net: mscc: ocelot: debugging print for statistics regions
https://git.kernel.org/netdev/net-next/c/07de32655bb4
- [net-next,4/8] net: mscc: ocelot: remove blank line at the end of ocelot_stats.c
https://git.kernel.org/netdev/net-next/c/93f0f93bbdb9
- [net-next,5/8] net: dsa: felix: remove confusing/incorrect comment from felix_setup()
https://git.kernel.org/netdev/net-next/c/a9afc3e41c61
- [net-next,6/8] net: mscc: ocelot: strengthen type of "u32 reg" and "u32 base" in ocelot_stats.c
https://git.kernel.org/netdev/net-next/c/eae0b9d15ba6
- [net-next,7/8] net: mscc: ocelot: strengthen type of "int i" in ocelot_stats.c
https://git.kernel.org/netdev/net-next/c/6663c01eca1a
- [net-next,8/8] net: mscc: ocelot: fix ineffective WARN_ON() in ocelot_stats.c
https://git.kernel.org/netdev/net-next/c/a291399e6354
You are awesome, thank you!
--
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html