Two tracepoints for now:
* `phy_interrupt` Pretty self-explanatory.
* `phy_state_change` Whenever the PHY's state machine is run, trace
the old and the new state.
Signed-off-by: Tobias Waldekranz <[email protected]>
---
drivers/net/phy/phy.c | 4 +++
include/trace/events/phy.h | 68 ++++++++++++++++++++++++++++++++++++++
2 files changed, 72 insertions(+)
create mode 100644 include/trace/events/phy.h
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 9aabfa1a455a..8d22926f3962 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -38,6 +38,9 @@
#include <asm/irq.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/phy.h>
+
#define PHY_STATE_STR(_state) \
case PHY_##_state: \
return __stringify(_state); \
@@ -1039,6 +1042,7 @@ void phy_state_machine(struct work_struct *work)
if (err < 0)
phy_error(phydev);
+ trace_phy_state_change(phydev, old_state);
if (old_state != phydev->state)
phydev_dbg(phydev, "PHY state change %s -> %s\n",
phy_state_to_str(old_state),
diff --git a/include/trace/events/phy.h b/include/trace/events/phy.h
new file mode 100644
index 000000000000..7ba6c0dda47e
--- /dev/null
+++ b/include/trace/events/phy.h
@@ -0,0 +1,68 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM phy
+
+#if !defined(_TRACE_PHY_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_PHY_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(phy_interrupt,
+ TP_PROTO(int irq, struct phy_device *phydev),
+ TP_ARGS(irq, phydev),
+ TP_STRUCT__entry(
+ __field(int, irq)
+ __field(int, addr)
+ __field(int, state)
+ __array(char, ifname, IFNAMSIZ)
+ ),
+ TP_fast_assign(
+ __entry->irq = irq;
+ __entry->addr = phydev->mdio.addr;
+ __entry->state = phydev->state;
+ if (phydev->attached_dev)
+ memcpy(__entry->ifname,
+ netdev_name(phydev->attached_dev),
+ IFNAMSIZ);
+ else
+ memset(__entry->ifname, 0, IFNAMSIZ);
+ ),
+ TP_printk("phy-%d-irq irq=%d ifname=%16s state=%d",
+ __entry->addr,
+ __entry->irq,
+ __entry->ifname,
+ __entry->state
+ )
+ );
+
+TRACE_EVENT(phy_state_change,
+ TP_PROTO(struct phy_device *phydev, enum phy_state old_state),
+ TP_ARGS(phydev, old_state),
+ TP_STRUCT__entry(
+ __field(int, addr)
+ __field(int, state)
+ __field(int, old_state)
+ __array(char, ifname, IFNAMSIZ)
+ ),
+ TP_fast_assign(
+ __entry->addr = phydev->mdio.addr;
+ __entry->state = phydev->state;
+ __entry->old_state = old_state;
+ if (phydev->attached_dev)
+ memcpy(__entry->ifname,
+ netdev_name(phydev->attached_dev),
+ IFNAMSIZ);
+ else
+ memset(__entry->ifname, 0, IFNAMSIZ);
+ ),
+ TP_printk("phy-%d-change ifname=%16s old_state=%d state=%d",
+ __entry->addr,
+ __entry->ifname,
+ __entry->old_state,
+ __entry->state
+ )
+ );
+
+#endif /* _TRACE_PHY_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
--
2.17.1
On Thu, 16 Aug 2018 11:30:53 +0200
Tobias Waldekranz <[email protected]> wrote:
> Two tracepoints for now:
>
> * `phy_interrupt` Pretty self-explanatory.
>
> * `phy_state_change` Whenever the PHY's state machine is run, trace
> the old and the new state.
From a tracing perspective, this patch looks fine,
Acked-by: Steven Rostedt (VMware) <[email protected]>
But below I have a possible improvement.
>
> Signed-off-by: Tobias Waldekranz <[email protected]>
> ---
> drivers/net/phy/phy.c | 4 +++
> include/trace/events/phy.h | 68 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 72 insertions(+)
> create mode 100644 include/trace/events/phy.h
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 9aabfa1a455a..8d22926f3962 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -38,6 +38,9 @@
>
> #include <asm/irq.h>
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/phy.h>
> +
> #define PHY_STATE_STR(_state) \
> case PHY_##_state: \
> return __stringify(_state); \
> @@ -1039,6 +1042,7 @@ void phy_state_machine(struct work_struct *work)
> if (err < 0)
> phy_error(phydev);
>
> + trace_phy_state_change(phydev, old_state);
I see that you are passing in a enum phy_state.
> if (old_state != phydev->state)
> phydev_dbg(phydev, "PHY state change %s -> %s\n",
> phy_state_to_str(old_state),
> diff --git a/include/trace/events/phy.h b/include/trace/events/phy.h
> new file mode 100644
> index 000000000000..7ba6c0dda47e
> --- /dev/null
> +++ b/include/trace/events/phy.h
> @@ -0,0 +1,68 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM phy
You can add this:
#define PHY_STATE_ENUMS \
EM( DOWN ) \
EM( STARTING ) \
EM( READY ) \
EM( PENDING ) \
EM( UP ) \
EM( AN ) \
EM( RUNNING ) \
EM( NOLINK ) \
EM( FORCING ) \
EM( CHANGELINK )\
EM( HALTED ) \
EMe(RESUMING)
#undef EM
#undef EMe
#define EM(a) TRACE_DEFINE_ENUM( PHY_##a );
#define EMe(a) TRACE_DEFINE_ENUM( PHY_##a );
PHY_STATE_ENUMS
#undef EM
#undef EMe
#define EM(a) { PHY_##a, #a },
#define EMe(a) { PHY_##a, #a }
> +
> +#if !defined(_TRACE_PHY_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_PHY_H
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(phy_interrupt,
> + TP_PROTO(int irq, struct phy_device *phydev),
> + TP_ARGS(irq, phydev),
> + TP_STRUCT__entry(
> + __field(int, irq)
> + __field(int, addr)
> + __field(int, state)
> + __array(char, ifname, IFNAMSIZ)
> + ),
> + TP_fast_assign(
> + __entry->irq = irq;
> + __entry->addr = phydev->mdio.addr;
> + __entry->state = phydev->state;
> + if (phydev->attached_dev)
> + memcpy(__entry->ifname,
> + netdev_name(phydev->attached_dev),
> + IFNAMSIZ);
> + else
> + memset(__entry->ifname, 0, IFNAMSIZ);
> + ),
> + TP_printk("phy-%d-irq irq=%d ifname=%16s state=%d",
Here you can have "state=%s"
> + __entry->addr,
> + __entry->irq,
> + __entry->ifname,
> + __entry->state
And here you can have:
__print_symbolic(__entry->state,
PHY_STATE_ENUMS )
> + )
> + );
> +
> +TRACE_EVENT(phy_state_change,
> + TP_PROTO(struct phy_device *phydev, enum phy_state old_state),
> + TP_ARGS(phydev, old_state),
> + TP_STRUCT__entry(
> + __field(int, addr)
> + __field(int, state)
> + __field(int, old_state)
> + __array(char, ifname, IFNAMSIZ)
> + ),
> + TP_fast_assign(
> + __entry->addr = phydev->mdio.addr;
> + __entry->state = phydev->state;
> + __entry->old_state = old_state;
> + if (phydev->attached_dev)
> + memcpy(__entry->ifname,
> + netdev_name(phydev->attached_dev),
> + IFNAMSIZ);
> + else
> + memset(__entry->ifname, 0, IFNAMSIZ);
> + ),
> + TP_printk("phy-%d-change ifname=%16s old_state=%d state=%d",
> + __entry->addr,
> + __entry->ifname,
> + __entry->old_state,
> + __entry->state
And do the same above for both old_state and state.
Then instead of just printing numbers and having to remember what state
goes with what number (and god forbid those enums were to change), you
would get actual names of the states, and be easier to read the trace
output.
Note, I didn't test what I wrote, there could be a bug, but it
shouldn't be too far off from what I'm trying to get at.
-- Steve
> + )
> + );
> +
> +#endif /* _TRACE_PHY_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
On Thu, Aug 16, 2018 at 11:34:15AM +0200, Tobias Waldekranz wrote:
> Two tracepoints for now:
>
> * `phy_interrupt` Pretty self-explanatory.
>
> * `phy_state_change` Whenever the PHY's state machine is run, trace
> the old and the new state.
>
> Signed-off-by: Tobias Waldekranz <[email protected]>
> ---
> drivers/net/phy/phy.c | 4 +++
> include/trace/events/phy.h | 68 ++++++++++++++++++++++++++++++++++++++
> 2 files changed, 72 insertions(+)
> create mode 100644 include/trace/events/phy.h
>
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 9aabfa1a455a..8d22926f3962 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -38,6 +38,9 @@
>
> #include <asm/irq.h>
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/phy.h>
> +
> #define PHY_STATE_STR(_state) \
> case PHY_##_state: \
> return __stringify(_state); \
> @@ -1039,6 +1042,7 @@ void phy_state_machine(struct work_struct *work)
> if (err < 0)
> phy_error(phydev);
>
> + trace_phy_state_change(phydev, old_state);
> if (old_state != phydev->state)
> phydev_dbg(phydev, "PHY state change %s -> %s\n",
> phy_state_to_str(old_state),
> diff --git a/include/trace/events/phy.h b/include/trace/events/phy.h
> new file mode 100644
> index 000000000000..7ba6c0dda47e
> --- /dev/null
> +++ b/include/trace/events/phy.h
> @@ -0,0 +1,68 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM phy
> +
> +#if !defined(_TRACE_PHY_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_PHY_H
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(phy_interrupt,
> + TP_PROTO(int irq, struct phy_device *phydev),
> + TP_ARGS(irq, phydev),
> + TP_STRUCT__entry(
> + __field(int, irq)
> + __field(int, addr)
> + __field(int, state)
> + __array(char, ifname, IFNAMSIZ)
> + ),
> + TP_fast_assign(
> + __entry->irq = irq;
> + __entry->addr = phydev->mdio.addr;
> + __entry->state = phydev->state;
> + if (phydev->attached_dev)
> + memcpy(__entry->ifname,
> + netdev_name(phydev->attached_dev),
> + IFNAMSIZ);
> + else
> + memset(__entry->ifname, 0, IFNAMSIZ);
> + ),
> + TP_printk("phy-%d-irq irq=%d ifname=%16s state=%d",
> + __entry->addr,
> + __entry->irq,
> + __entry->ifname,
> + __entry->state
> + )
> + );
> +
> +TRACE_EVENT(phy_state_change,
> + TP_PROTO(struct phy_device *phydev, enum phy_state old_state),
> + TP_ARGS(phydev, old_state),
> + TP_STRUCT__entry(
> + __field(int, addr)
> + __field(int, state)
> + __field(int, old_state)
> + __array(char, ifname, IFNAMSIZ)
> + ),
> + TP_fast_assign(
> + __entry->addr = phydev->mdio.addr;
> + __entry->state = phydev->state;
> + __entry->old_state = old_state;
> + if (phydev->attached_dev)
> + memcpy(__entry->ifname,
> + netdev_name(phydev->attached_dev),
> + IFNAMSIZ);
> + else
> + memset(__entry->ifname, 0, IFNAMSIZ);
> + ),
> + TP_printk("phy-%d-change ifname=%16s old_state=%d state=%d",
> + __entry->addr,
> + __entry->ifname,
> + __entry->old_state,
> + __entry->state
> + )
> + );
> +
> +#endif /* _TRACE_PHY_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> --
> 2.17.1
>
This was sent in error. A v2 will follow. I had not used
get_maintainers.pl before, sorry about the noise!
--
Thanks
- wkz