Re: [PATCH v3 net-next 17/24] net: dsa: sja1105: Add support for ethtool port counters

[Vladimir Oltean <olteanv@gmail.com>]

On Sat, 13 Apr 2019 at 23:53, Jiri Pirko <jiri@resnulli.us> wrote:
>
> Sat, Apr 13, 2019 at 03:28:15AM CEST, olteanv@gmail.com wrote:
> >Signed-off-by: Vladimir Oltean <olteanv@gmail.com>
> >Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
> >---
> >Changes in v3:
> >None.
> >
> >Changes in v2:
> >None functional. Moved the IS_ET() and IS_PQRS() device identification
> >macros here since they are not used in earlier patches.
> >
> > drivers/net/dsa/sja1105/Makefile              |   1 +
> > drivers/net/dsa/sja1105/sja1105.h             |   7 +-
> > drivers/net/dsa/sja1105/sja1105_ethtool.c     | 414 ++++++++++++++++++
> > drivers/net/dsa/sja1105/sja1105_main.c        |   3 +
> > .../net/dsa/sja1105/sja1105_static_config.h   |  21 +
> > 5 files changed, 445 insertions(+), 1 deletion(-)
> > create mode 100644 drivers/net/dsa/sja1105/sja1105_ethtool.c
> >
> >diff --git a/drivers/net/dsa/sja1105/Makefile b/drivers/net/dsa/sja1105/Makefile
> >index ed00840802f4..bb4404c79eb2 100644
> >--- a/drivers/net/dsa/sja1105/Makefile
> >+++ b/drivers/net/dsa/sja1105/Makefile
> >@@ -3,6 +3,7 @@ obj-$(CONFIG_NET_DSA_SJA1105) += sja1105.o
> > sja1105-objs := \
> >     sja1105_spi.o \
> >     sja1105_main.o \
> >+    sja1105_ethtool.o \
> >     sja1105_clocking.o \
> >     sja1105_static_config.o \
> >     sja1105_dynamic_config.o \
> >diff --git a/drivers/net/dsa/sja1105/sja1105.h b/drivers/net/dsa/sja1105/sja1105.h
> >index 4c9df44a4478..80b20bdd8f9c 100644
> >--- a/drivers/net/dsa/sja1105/sja1105.h
> >+++ b/drivers/net/dsa/sja1105/sja1105.h
> >@@ -120,8 +120,13 @@ typedef enum {
> > int sja1105_clocking_setup_port(struct sja1105_private *priv, int port);
> > int sja1105_clocking_setup(struct sja1105_private *priv);
> >
> >-/* From sja1105_dynamic_config.c */
> >+/* From sja1105_ethtool.c */
> >+void sja1105_get_ethtool_stats(struct dsa_switch *ds, int port, u64 *data);
> >+void sja1105_get_strings(struct dsa_switch *ds, int port,
> >+                       u32 stringset, u8 *data);
> >+int sja1105_get_sset_count(struct dsa_switch *ds, int port, int sset);
> >
> >+/* From sja1105_dynamic_config.c */
> > int sja1105_dynamic_config_read(struct sja1105_private *priv,
> >                               enum sja1105_blk_idx blk_idx,
> >                               int index, void *entry);
> >diff --git a/drivers/net/dsa/sja1105/sja1105_ethtool.c b/drivers/net/dsa/sja1105/sja1105_ethtool.c
> >new file mode 100644
> >index 000000000000..c082599702bd
> >--- /dev/null
> >+++ b/drivers/net/dsa/sja1105/sja1105_ethtool.c
> >@@ -0,0 +1,414 @@
> >+// SPDX-License-Identifier: GPL-2.0
> >+/* Copyright (c) 2018-2019, Vladimir Oltean <olteanv@gmail.com>
> >+ */
> >+#include "sja1105.h"
> >+
> >+#define SIZE_MAC_AREA         (0x02 * 4)
> >+#define SIZE_HL1_AREA         (0x10 * 4)
> >+#define SIZE_HL2_AREA         (0x4 * 4)
> >+#define SIZE_QLEVEL_AREA      (0x8 * 4) /* 0x4 to 0xB */
>
> Please use prefixes for defines like this. For example "SIZE_MAC_AREA"
> sounds way too generic.
>

What you propose sounds nice but then consistency would be a concern,
so I'd have to redo the entire driver. And then there are tables named
like "L2 Address Lookup Parameters Table", and as if
SIZE_L2_LOOKUP_PARAMS_DYN_CMD_ET wasn't long enough, a prefix would
top it off.
I don't think it's as much of an issue for the reader (for the
compiler it clearly isn't, as it's restricted to this C file only) as
it is for tools like ctags?

> [...]
>
>
> >+static void
> >+sja1105_port_status_hl1_unpack(void *buf,
> >+                             struct sja1105_port_status_hl1 *status)
> >+{
> >+      /* Make pointer arithmetic work on 4 bytes */
> >+      u32 *p = (u32 *)buf;
>
> You don't need to cast void *. Please avoid it in the whole patchset.
>

Ok.

> [...]
>
>
> >+      if (!IS_PQRS(priv->info->device_id))
> >+              return;
> >+
> >+      memset(data + k, 0, ARRAY_SIZE(sja1105pqrs_extra_port_stats) *
> >+                      sizeof(u64));
> >+      for (i = 0; i < 8; i++) {
>
> Array size instead of "8"?
>

Perhaps SJA1105_NUM_TC, since the egress queue occupancy levels are
per traffic class. The size of the array is 16.

>
> >+              data[k++] = status.hl2.qlevel_hwm[i];
> >+              data[k++] = status.hl2.qlevel[i];
> >+      }
>
> [...]
>
>
> >
> >+#define IS_PQRS(device_id) \
> >+      (((device_id) == SJA1105PR_DEVICE_ID) || \
> >+       ((device_id) == SJA1105QS_DEVICE_ID))
> >+#define IS_ET(device_id) \
> >+      (((device_id) == SJA1105E_DEVICE_ID) || \
> >+       ((device_id) == SJA1105T_DEVICE_ID))
> >+/* P and R have same Device ID, and differ by Part Number */
> >+#define IS_P(device_id, part_nr) \
> >+      (((device_id) == SJA1105PR_DEVICE_ID) && \
> >+       ((part_nr) == SJA1105P_PART_NR))
> >+#define IS_R(device_id, part_nr) \
> >+      (((device_id) == SJA1105PR_DEVICE_ID) && \
> >+       ((part_nr) == SJA1105R_PART_NR))
> >+/* Same do Q and S */
> >+#define IS_Q(device_id, part_nr) \
> >+      (((device_id) == SJA1105QS_DEVICE_ID) && \
> >+       ((part_nr) == SJA1105Q_PART_NR))
> >+#define IS_S(device_id, part_nr) \
>
> Please have a prefix for macros like this. "IS_S" sounds way too
> generic...
>

Ok, I can think of a more descriptive name, since there are under 5
occurrences of these macros after the reorg, now that I have the
sja1105_info structure which also holds per-revision function
pointers.

>
> >+      (((device_id) == SJA1105QS_DEVICE_ID) && \
> >+       ((part_nr) == SJA1105S_PART_NR))
> >+
> > struct sja1105_general_params_entry {
> >       u64 vllupformat;
> >       u64 mirr_ptacu;
> >--
> >2.17.1
> >


Thanks!
-Vladimir