linux-kernel - Re: [PATCH] net: phy: add tracepoints

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] [day] [month] [year] [list]
Message-ID: <20180816084626.19f516c9@gandalf.local.home>
Date:   Thu, 16 Aug 2018 08:46:26 -0400
From:   Steven Rostedt <rostedt@...dmis.org>
To:     Tobias Waldekranz <tobias@...dekranz.com>
Cc:     Tobias Waldekranz <tobias.waldekranz@...termo.se>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Ingo Molnar <mingo@...hat.com>,
        linux-kernel@...r.kernel.org (open list),
        netdev@...r.kernel.org (open list:ETHERNET PHY LIBRARY)
Subject: Re: [PATCH] net: phy: add tracepoints

On Thu, 16 Aug 2018 11:30:53 +0200
Tobias Waldekranz <tobias@...dekranz.com> 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) <rostedt@...dmis.org>

But below I have a possible improvement.

> 
> Signed-off-by: Tobias Waldekranz <tobias@...dekranz.com>
> ---
>  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>