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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sun, 14 Apr 2019 10:34:26 +0200
From:   Jiri Pirko <jiri@...nulli.us>
To:     Vladimir Oltean <olteanv@...il.com>
Cc:     Florian Fainelli <f.fainelli@...il.com>, vivien.didelot@...il.com,
        Andrew Lunn <andrew@...n.ch>, davem@...emloft.net,
        netdev <netdev@...r.kernel.org>, linux-kernel@...r.kernel.org,
        Georg Waibel <georg.waibel@...sor-technik.de>
Subject: Re: [PATCH v3 net-next 17/24] net: dsa: sja1105: Add support for
 ethtool port counters

Sat, Apr 13, 2019 at 11:55:52PM CEST, olteanv@...il.com wrote:
>On Sat, 13 Apr 2019 at 23:53, Jiri Pirko <jiri@...nulli.us> wrote:
>>
>> Sat, Apr 13, 2019 at 03:28:15AM CEST, olteanv@...il.com wrote:
>> >Signed-off-by: Vladimir Oltean <olteanv@...il.com>
>> >Reviewed-by: Florian Fainelli <f.fainelli@...il.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@...il.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

Yep


>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.

It's a tradeoff. But most of the time, it is doable. Then the code is
easier to read.


>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