| 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
| ||
|
Message-ID: <CA+h21hphrc0+H24zAjTNsrq4u-LgVBw630vpDgwoEXF96KrPkQ@mail.gmail.com>
Date: Sun, 14 Apr 2019 00:55:52 +0300
From: Vladimir Oltean <olteanv@...il.com>
To: Jiri Pirko <jiri@...nulli.us>
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
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
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
Powered by blists - more mailing lists