| Message ID | 20221207233954.3619276-3-vladimir.oltean@nxp.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | Trace points for mv88e6xxx | expand |
On 08 Dec 01:39, Vladimir Oltean wrote: >In applications where the switch ports must perform 802.1X based >authentication and are therefore locked, ATU violation interrupts are >quite to be expected as part of normal operation. The problem is that >they currently spam the kernel log, even if rate limited. > +1 >Create a series of trace points, all derived from the same event class, >which log these violations to the kernel's trace buffer, which is both >much faster and much easier to ignore than printing to a serial console. > >I've deliberately stopped reporting the portvec, since in my experience >it contains redundant information with the spid (port) field: portvec == >1 << spid. > >New usage model: > >$ trace-cmd list | grep mv88e6xxx >mv88e6xxx >mv88e6xxx:mv88e6xxx_atu_full_violation >mv88e6xxx:mv88e6xxx_atu_miss_violation >mv88e6xxx:mv88e6xxx_atu_member_violation >mv88e6xxx:mv88e6xxx_atu_age_out_violation >$ trace-cmd record -e mv88e6xxx sleep 10 > >Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> >--- > drivers/net/dsa/mv88e6xxx/Makefile | 4 ++ > drivers/net/dsa/mv88e6xxx/global1_atu.c | 21 ++++---- > drivers/net/dsa/mv88e6xxx/trace.c | 6 +++ > drivers/net/dsa/mv88e6xxx/trace.h | 68 +++++++++++++++++++++++++ > 4 files changed, 87 insertions(+), 12 deletions(-) > create mode 100644 drivers/net/dsa/mv88e6xxx/trace.c > create mode 100644 drivers/net/dsa/mv88e6xxx/trace.h > >diff --git a/drivers/net/dsa/mv88e6xxx/Makefile b/drivers/net/dsa/mv88e6xxx/Makefile >index c8eca2b6f959..49bf358b9c4f 100644 >--- a/drivers/net/dsa/mv88e6xxx/Makefile >+++ b/drivers/net/dsa/mv88e6xxx/Makefile >@@ -15,3 +15,7 @@ mv88e6xxx-objs += port_hidden.o > mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_PTP) += ptp.o > mv88e6xxx-objs += serdes.o > mv88e6xxx-objs += smi.o >+mv88e6xxx-objs += trace.o >+ >+# for tracing framework to find trace.h >+CFLAGS_trace.o := -I$(src) >diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c >index a9e2ff7d0e52..6ba65b723b42 100644 >--- a/drivers/net/dsa/mv88e6xxx/global1_atu.c >+++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c >@@ -12,6 +12,7 @@ > > #include "chip.h" > #include "global1.h" >+#include "trace.h" > > /* Offset 0x01: ATU FID Register */ > >@@ -429,29 +430,25 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id) > spid = entry.state; > > if (val & MV88E6XXX_G1_ATU_OP_AGE_OUT_VIOLATION) { >- dev_err_ratelimited(chip->dev, >- "ATU age out violation for %pM fid %u\n", >- entry.mac, fid); >+ trace_mv88e6xxx_atu_age_out_violation(chip->dev, spid, >+ entry.mac, fid); no stats here? tracepoints are disabled by default and this event will go unnoticed, users usually monitor light weight indicators such as stats, then turn on tracepoints to see what's actually happening.. > } >
On Wed, Dec 07, 2022 at 04:14:16PM -0800, Saeed Mahameed wrote: > > if (val & MV88E6XXX_G1_ATU_OP_AGE_OUT_VIOLATION) { > > - dev_err_ratelimited(chip->dev, > > - "ATU age out violation for %pM fid %u\n", > > - entry.mac, fid); > > + trace_mv88e6xxx_atu_age_out_violation(chip->dev, spid, > > + entry.mac, fid); > > no stats here? tracepoints are disabled by default and this event will go > unnoticed, users usually monitor light weight indicators such as stats, then > turn on tracepoints to see what's actually happening.. I believe that the ATU age out violation handler is dead code currently. The driver does not enable the MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT bit (interrupt on age out). I just converted the existing debugging prints to trace points. Open to more suggestions, but I believe that if I introduce a counter, it would always return 0.
On 2022-12-08 15:49, Vladimir Oltean wrote: > On Wed, Dec 07, 2022 at 04:14:16PM -0800, Saeed Mahameed wrote: >> > if (val & MV88E6XXX_G1_ATU_OP_AGE_OUT_VIOLATION) { >> > - dev_err_ratelimited(chip->dev, >> > - "ATU age out violation for %pM fid %u\n", >> > - entry.mac, fid); >> > + trace_mv88e6xxx_atu_age_out_violation(chip->dev, spid, >> > + entry.mac, fid); >> >> no stats here? tracepoints are disabled by default and this event will >> go >> unnoticed, users usually monitor light weight indicators such as >> stats, then >> turn on tracepoints to see what's actually happening.. > > I believe that the ATU age out violation handler is dead code > currently. > The driver does not enable the MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT > bit > (interrupt on age out). > > I just converted the existing debugging prints to trace points. Open to > more suggestions, but I believe that if I introduce a counter, it would > always return 0. The follow-up patch set to the MAB patch set I have, will make use of the age out violation.
On Thu, Dec 08, 2022 at 04:22:53PM +0100, netdev@kapio-technology.com wrote: > The follow-up patch set to the MAB patch set I have, will make use of the age > out violation. Ok, so for v2 I can delete the debugging print, since it's currently dead code, and you can add the counter and the trace point when the code will be actually exercised, how does that sound?
On 2022-12-08 16:27, Vladimir Oltean wrote: > On Thu, Dec 08, 2022 at 04:22:53PM +0100, netdev@kapio-technology.com > wrote: >> The follow-up patch set to the MAB patch set I have, will make use of >> the age >> out violation. > > Ok, so for v2 I can delete the debugging print, since it's currently > dead code, and you can add the counter and the trace point when the > code > will be actually exercised, how does that sound? Ok, sounds like a plan.
On 2022-12-08 15:49, Vladimir Oltean wrote: > On Wed, Dec 07, 2022 at 04:14:16PM -0800, Saeed Mahameed wrote: >> > if (val & MV88E6XXX_G1_ATU_OP_AGE_OUT_VIOLATION) { >> > - dev_err_ratelimited(chip->dev, >> > - "ATU age out violation for %pM fid %u\n", >> > - entry.mac, fid); >> > + trace_mv88e6xxx_atu_age_out_violation(chip->dev, spid, >> > + entry.mac, fid); >> >> no stats here? tracepoints are disabled by default and this event will >> go >> unnoticed, users usually monitor light weight indicators such as >> stats, then >> turn on tracepoints to see what's actually happening.. > > I believe that the ATU age out violation handler is dead code > currently. > The driver does not enable the MV88E6XXX_PORT_ASSOC_VECTOR_INT_AGE_OUT > bit > (interrupt on age out). > > I just converted the existing debugging prints to trace points. Open to > more suggestions, but I believe that if I introduce a counter, it would > always return 0. If I am not mistaken, I will have to wait for your patch set to be accepted before I can send the next version of the MAB patch set.
On Thu, Dec 08, 2022 at 07:33:50PM +0100, netdev@kapio-technology.com wrote: > If I am not mistaken, I will have to wait for your patch set to be accepted before > I can send the next version of the MAB patch set. Yes, you are not mistaken.
On 08 Dec 16:35, netdev@kapio-technology.com wrote: >On 2022-12-08 16:27, Vladimir Oltean wrote: >>On Thu, Dec 08, 2022 at 04:22:53PM +0100, >>netdev@kapio-technology.com wrote: >>>The follow-up patch set to the MAB patch set I have, will make use >>>of the age >>>out violation. >> >>Ok, so for v2 I can delete the debugging print, since it's currently >>dead code, and you can add the counter and the trace point when the >>code >>will be actually exercised, how does that sound? > >Ok, sounds like a plan. FWIW +1 :)
diff --git a/drivers/net/dsa/mv88e6xxx/Makefile b/drivers/net/dsa/mv88e6xxx/Makefile index c8eca2b6f959..49bf358b9c4f 100644 --- a/drivers/net/dsa/mv88e6xxx/Makefile +++ b/drivers/net/dsa/mv88e6xxx/Makefile @@ -15,3 +15,7 @@ mv88e6xxx-objs += port_hidden.o mv88e6xxx-$(CONFIG_NET_DSA_MV88E6XXX_PTP) += ptp.o mv88e6xxx-objs += serdes.o mv88e6xxx-objs += smi.o +mv88e6xxx-objs += trace.o + +# for tracing framework to find trace.h +CFLAGS_trace.o := -I$(src) diff --git a/drivers/net/dsa/mv88e6xxx/global1_atu.c b/drivers/net/dsa/mv88e6xxx/global1_atu.c index a9e2ff7d0e52..6ba65b723b42 100644 --- a/drivers/net/dsa/mv88e6xxx/global1_atu.c +++ b/drivers/net/dsa/mv88e6xxx/global1_atu.c @@ -12,6 +12,7 @@ #include "chip.h" #include "global1.h" +#include "trace.h" /* Offset 0x01: ATU FID Register */ @@ -429,29 +430,25 @@ static irqreturn_t mv88e6xxx_g1_atu_prob_irq_thread_fn(int irq, void *dev_id) spid = entry.state; if (val & MV88E6XXX_G1_ATU_OP_AGE_OUT_VIOLATION) { - dev_err_ratelimited(chip->dev, - "ATU age out violation for %pM fid %u\n", - entry.mac, fid); + trace_mv88e6xxx_atu_age_out_violation(chip->dev, spid, + entry.mac, fid); } if (val & MV88E6XXX_G1_ATU_OP_MEMBER_VIOLATION) { - dev_err_ratelimited(chip->dev, - "ATU member violation for %pM fid %u portvec %x spid %d\n", - entry.mac, fid, entry.portvec, spid); + trace_mv88e6xxx_atu_member_violation(chip->dev, spid, + entry.mac, fid); chip->ports[spid].atu_member_violation++; } if (val & MV88E6XXX_G1_ATU_OP_MISS_VIOLATION) { - dev_err_ratelimited(chip->dev, - "ATU miss violation for %pM fid %u portvec %x spid %d\n", - entry.mac, fid, entry.portvec, spid); + trace_mv88e6xxx_atu_miss_violation(chip->dev, spid, + entry.mac, fid); chip->ports[spid].atu_miss_violation++; } if (val & MV88E6XXX_G1_ATU_OP_FULL_VIOLATION) { - dev_err_ratelimited(chip->dev, - "ATU full violation for %pM fid %u portvec %x spid %d\n", - entry.mac, fid, entry.portvec, spid); + trace_mv88e6xxx_atu_full_violation(chip->dev, spid, + entry.mac, fid); chip->ports[spid].atu_full_violation++; } mv88e6xxx_reg_unlock(chip); diff --git a/drivers/net/dsa/mv88e6xxx/trace.c b/drivers/net/dsa/mv88e6xxx/trace.c new file mode 100644 index 000000000000..7833cb50ca5d --- /dev/null +++ b/drivers/net/dsa/mv88e6xxx/trace.c @@ -0,0 +1,6 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* Copyright 2022 NXP + */ + +#define CREATE_TRACE_POINTS +#include "trace.h" diff --git a/drivers/net/dsa/mv88e6xxx/trace.h b/drivers/net/dsa/mv88e6xxx/trace.h new file mode 100644 index 000000000000..dc24dbd77f77 --- /dev/null +++ b/drivers/net/dsa/mv88e6xxx/trace.h @@ -0,0 +1,68 @@ +/* SPDX-License-Identifier: GPL-2.0-or-later */ +/* Copyright 2022 NXP + */ + +#undef TRACE_SYSTEM +#define TRACE_SYSTEM mv88e6xxx + +#if !defined(_MV88E6XXX_TRACE_H) || defined(TRACE_HEADER_MULTI_READ) +#define _MV88E6XXX_TRACE_H + +#include <linux/device.h> +#include <linux/if_ether.h> +#include <linux/tracepoint.h> + +DECLARE_EVENT_CLASS(mv88e6xxx_atu_violation, + + TP_PROTO(const struct device *dev, int port, + const unsigned char *addr, u16 fid), + + TP_ARGS(dev, port, addr, fid), + + TP_STRUCT__entry( + __string(name, dev_name(dev)) + __field(int, port) + __array(unsigned char, addr, ETH_ALEN) + __field(u16, fid) + ), + + TP_fast_assign( + __assign_str(name, dev_name(dev)); + __entry->port = port; + memcpy(__entry->addr, addr, ETH_ALEN); + __entry->fid = fid; + ), + + TP_printk("dev %s port %d addr %pM fid %u", + __get_str(name), __entry->port, __entry->addr, __entry->fid) +); + +DEFINE_EVENT(mv88e6xxx_atu_violation, mv88e6xxx_atu_age_out_violation, + TP_PROTO(const struct device *dev, int port, + const unsigned char *addr, u16 fid), + TP_ARGS(dev, port, addr, fid)); + +DEFINE_EVENT(mv88e6xxx_atu_violation, mv88e6xxx_atu_member_violation, + TP_PROTO(const struct device *dev, int port, + const unsigned char *addr, u16 fid), + TP_ARGS(dev, port, addr, fid)); + +DEFINE_EVENT(mv88e6xxx_atu_violation, mv88e6xxx_atu_miss_violation, + TP_PROTO(const struct device *dev, int port, + const unsigned char *addr, u16 fid), + TP_ARGS(dev, port, addr, fid)); + +DEFINE_EVENT(mv88e6xxx_atu_violation, mv88e6xxx_atu_full_violation, + TP_PROTO(const struct device *dev, int port, + const unsigned char *addr, u16 fid), + TP_ARGS(dev, port, addr, fid)); + +#endif /* _MV88E6XXX_TRACE_H */ + +/* We don't want to use include/trace/events */ +#undef TRACE_INCLUDE_PATH +#define TRACE_INCLUDE_PATH . +#undef TRACE_INCLUDE_FILE +#define TRACE_INCLUDE_FILE trace +/* This part must be outside protection */ +#include <trace/define_trace.h>
In applications where the switch ports must perform 802.1X based authentication and are therefore locked, ATU violation interrupts are quite to be expected as part of normal operation. The problem is that they currently spam the kernel log, even if rate limited. Create a series of trace points, all derived from the same event class, which log these violations to the kernel's trace buffer, which is both much faster and much easier to ignore than printing to a serial console. I've deliberately stopped reporting the portvec, since in my experience it contains redundant information with the spid (port) field: portvec == 1 << spid. New usage model: $ trace-cmd list | grep mv88e6xxx mv88e6xxx mv88e6xxx:mv88e6xxx_atu_full_violation mv88e6xxx:mv88e6xxx_atu_miss_violation mv88e6xxx:mv88e6xxx_atu_member_violation mv88e6xxx:mv88e6xxx_atu_age_out_violation $ trace-cmd record -e mv88e6xxx sleep 10 Signed-off-by: Vladimir Oltean <vladimir.oltean@nxp.com> --- drivers/net/dsa/mv88e6xxx/Makefile | 4 ++ drivers/net/dsa/mv88e6xxx/global1_atu.c | 21 ++++---- drivers/net/dsa/mv88e6xxx/trace.c | 6 +++ drivers/net/dsa/mv88e6xxx/trace.h | 68 +++++++++++++++++++++++++ 4 files changed, 87 insertions(+), 12 deletions(-) create mode 100644 drivers/net/dsa/mv88e6xxx/trace.c create mode 100644 drivers/net/dsa/mv88e6xxx/trace.h