[patch net-next v2 1/2] devlink: add hardware messages tracing facility

[patch net-next v2 1/2] devlink: add hardware messages tracing facility

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Define a tracepoint and allow user to trace messages going to and from
hardware associated with devlink instance.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
---
v1->v2:
- Use EXPORT_TRACEPOINT_SYMBOL_GPL instead of a wrapper function
  as suggested by David Ahern and Steven Rostedt
---
 include/trace/events/devlink.h | 68 ++++++++++++++++++++++++++++++++++++++++++
 net/core/devlink.c             |  4 +++
 2 files changed, 72 insertions(+)
 create mode 100644 include/trace/events/devlink.h

diff --git a/include/trace/events/devlink.h b/include/trace/events/devlink.h
new file mode 100644
index 0000000..333c32a
--- /dev/null
+++ b/include/trace/events/devlink.h
@@ -0,0 +1,68 @@
+#if IS_ENABLED(CONFIG_NET_DEVLINK)
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM devlink
+
+#if !defined(_TRACE_DEVLINK_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_DEVLINK_H
+
+#include <linux/device.h>
+#include <net/devlink.h>
+#include <linux/tracepoint.h>
+
+/*
+ * Tracepoint for devlink hardware message:
+ */
+TRACE_EVENT(devlink_hwmsg,
+	TP_PROTO(const struct devlink *devlink, bool incoming,
+		 unsigned long type, const u8 *buf, size_t len),
+
+	TP_ARGS(devlink, incoming, type, buf, len),
+
+	TP_STRUCT__entry(
+		__string(bus_name, devlink->dev->bus->name)
+		__string(dev_name, dev_name(devlink->dev))
+		__string(owner_name, devlink->dev->driver->owner->name)
+		__field(bool, incoming)
+		__field(unsigned long, type)
+		__dynamic_array(u8, buf, len)
+		__field(size_t, len)
+	),
+
+	TP_fast_assign(
+		__assign_str(bus_name, devlink->dev->bus->name);
+		__assign_str(dev_name, dev_name(devlink->dev));
+		__assign_str(owner_name, devlink->dev->driver->owner->name);
+		__entry->incoming = incoming;
+		__entry->type = type;
+		memcpy(__get_dynamic_array(buf), buf, len);
+		__entry->len = len;
+	),
+
+	TP_printk("bus_name=%s dev_name=%s owner_name=%s incoming=%d type=%lu buf=0x[%*phD] len=%lu",
+		  __get_str(bus_name), __get_str(dev_name),
+		  __get_str(owner_name), __entry->incoming, __entry->type,
+		  (int) __entry->len, __get_dynamic_array(buf), __entry->len)
+);
+
+#endif /* _TRACE_DEVLINK_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
+
+#else /* CONFIG_NET_DEVLINK */
+
+#if !defined(_TRACE_DEVLINK_H)
+#define _TRACE_DEVLINK_H
+
+#include <net/devlink.h>
+
+static inline void trace_devlink_hwmsg(const struct devlink *devlink,
+				       bool incoming, unsigned long type,
+				       const u8 *buf, size_t len)
+{
+}
+
+#endif /* _TRACE_DEVLINK_H */
+
+#endif
diff --git a/net/core/devlink.c b/net/core/devlink.c
index b2e592a..1b50630 100644
--- a/net/core/devlink.c
+++ b/net/core/devlink.c
@@ -26,6 +26,10 @@
 #include <net/net_namespace.h>
 #include <net/sock.h>
 #include <net/devlink.h>
+#define CREATE_TRACE_POINTS
+#include <trace/events/devlink.h>
+
+EXPORT_TRACEPOINT_SYMBOL_GPL(devlink_hwmsg);
 
 static LIST_HEAD(devlink_list);
 
-- 
2.5.5

[patch net-next v2 2/2] mlxsw: core: Trace EMAD messages

[Jiri Pirko]

From: Jiri Pirko <jiri@mellanox.com>

Trace EMAD messages going down to HW and up from HW. Devlink needs to be
registered before EMAD init so the trace function can be called
with valid devlink handle.

Signed-off-by: Jiri Pirko <jiri@mellanox.com>
v1->v2:
- Use trace_devlink_hwmsg directly
---
 drivers/net/ethernet/mellanox/mlxsw/core.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/net/ethernet/mellanox/mlxsw/core.c b/drivers/net/ethernet/mellanox/mlxsw/core.c
index 01ae548..480a3ba 100644
--- a/drivers/net/ethernet/mellanox/mlxsw/core.c
+++ b/drivers/net/ethernet/mellanox/mlxsw/core.c
@@ -58,6 +58,7 @@
 #include <linux/workqueue.h>
 #include <asm/byteorder.h>
 #include <net/devlink.h>
+#include <trace/events/devlink.h>
 
 #include "core.h"
 #include "item.h"
@@ -447,6 +448,10 @@ static int mlxsw_emad_transmit(struct mlxsw_core *mlxsw_core,
 	if (!skb)
 		return -ENOMEM;
 
+	trace_devlink_hwmsg(priv_to_devlink(mlxsw_core), false, 0,
+			    skb->data + mlxsw_core->driver->txhdr_len,
+			    skb->len - mlxsw_core->driver->txhdr_len);
+
 	atomic_set(&trans->active, 1);
 	err = mlxsw_core_skb_transmit(mlxsw_core, skb, &trans->tx_info);
 	if (err) {
@@ -529,6 +534,9 @@ static void mlxsw_emad_rx_listener_func(struct sk_buff *skb, u8 local_port,
 	struct mlxsw_core *mlxsw_core = priv;
 	struct mlxsw_reg_trans *trans;
 
+	trace_devlink_hwmsg(priv_to_devlink(mlxsw_core), true, 0,
+			    skb->data, skb->len);
+
 	if (!mlxsw_emad_is_resp(skb))
 		goto free_skb;
 
@@ -1110,14 +1118,14 @@ int mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
 	if (err)
 		goto err_emad_init;
 
-	err = mlxsw_hwmon_init(mlxsw_core, mlxsw_bus_info, &mlxsw_core->hwmon);
-	if (err)
-		goto err_hwmon_init;
-
 	err = devlink_register(devlink, mlxsw_bus_info->dev);
 	if (err)
 		goto err_devlink_register;
 
+	err = mlxsw_hwmon_init(mlxsw_core, mlxsw_bus_info, &mlxsw_core->hwmon);
+	if (err)
+		goto err_hwmon_init;
+
 	err = mlxsw_driver->init(mlxsw_core, mlxsw_bus_info);
 	if (err)
 		goto err_driver_init;
@@ -1131,9 +1139,9 @@ int mlxsw_core_bus_device_register(const struct mlxsw_bus_info *mlxsw_bus_info,
 err_debugfs_init:
 	mlxsw_core->driver->fini(mlxsw_core);
 err_driver_init:
+err_hwmon_init:
 	devlink_unregister(devlink);
 err_devlink_register:
-err_hwmon_init:
 	mlxsw_emad_fini(mlxsw_core);
 err_emad_init:
 	mlxsw_bus->fini(bus_priv);
-- 
2.5.5

Re: [patch net-next v2 2/2] mlxsw: core: Trace EMAD messages

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Tue, 12 Jul 2016 18:05:04 +0200

> From: Jiri Pirko <jiri@mellanox.com>
> 
> Trace EMAD messages going down to HW and up from HW. Devlink needs to be
> registered before EMAD init so the trace function can be called
> with valid devlink handle.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> v1->v2:
> - Use trace_devlink_hwmsg directly

Applied.

Re: [patch net-next v2 1/2] devlink: add hardware messages tracing facility

[Steven Rostedt]

On Tue, 12 Jul 2016 18:05:03 +0200
Jiri Pirko <jiri@resnulli.us> wrote:

> From: Jiri Pirko <jiri@mellanox.com>
> 
> Define a tracepoint and allow user to trace messages going to and from
> hardware associated with devlink instance.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> ---
> v1->v2:
> - Use EXPORT_TRACEPOINT_SYMBOL_GPL instead of a wrapper function
>   as suggested by David Ahern and Steven Rostedt

FYI, you can use the Suggested-by: tag too ;-)

> ---
>  include/trace/events/devlink.h | 68 ++++++++++++++++++++++++++++++++++++++++++
>  net/core/devlink.c             |  4 +++
>  2 files changed, 72 insertions(+)
>  create mode 100644 include/trace/events/devlink.h
> 
> diff --git a/include/trace/events/devlink.h b/include/trace/events/devlink.h
> new file mode 100644
> index 0000000..333c32a
> --- /dev/null
> +++ b/include/trace/events/devlink.h
> @@ -0,0 +1,68 @@
> +#if IS_ENABLED(CONFIG_NET_DEVLINK)
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM devlink
> +
> +#if !defined(_TRACE_DEVLINK_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_DEVLINK_H
> +
> +#include <linux/device.h>
> +#include <net/devlink.h>
> +#include <linux/tracepoint.h>
> +
> +/*
> + * Tracepoint for devlink hardware message:
> + */
> +TRACE_EVENT(devlink_hwmsg,
> +	TP_PROTO(const struct devlink *devlink, bool incoming,
> +		 unsigned long type, const u8 *buf, size_t len),
> +
> +	TP_ARGS(devlink, incoming, type, buf, len),
> +
> +	TP_STRUCT__entry(
> +		__string(bus_name, devlink->dev->bus->name)
> +		__string(dev_name, dev_name(devlink->dev))
> +		__string(owner_name, devlink->dev->driver->owner->name)
> +		__field(bool, incoming)
> +		__field(unsigned long, type)
> +		__dynamic_array(u8, buf, len)
> +		__field(size_t, len)
> +	),
> +
> +	TP_fast_assign(
> +		__assign_str(bus_name, devlink->dev->bus->name);
> +		__assign_str(dev_name, dev_name(devlink->dev));
> +		__assign_str(owner_name, devlink->dev->driver->owner->name);
> +		__entry->incoming = incoming;
> +		__entry->type = type;
> +		memcpy(__get_dynamic_array(buf), buf, len);
> +		__entry->len = len;
> +	),
> +
> +	TP_printk("bus_name=%s dev_name=%s owner_name=%s incoming=%d type=%lu buf=0x[%*phD] len=%lu",
> +		  __get_str(bus_name), __get_str(dev_name),
> +		  __get_str(owner_name), __entry->incoming, __entry->type,
> +		  (int) __entry->len, __get_dynamic_array(buf), __entry->len)
> +);
> +
> +#endif /* _TRACE_DEVLINK_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> +
> +#else /* CONFIG_NET_DEVLINK */
> +
> +#if !defined(_TRACE_DEVLINK_H)
> +#define _TRACE_DEVLINK_H

Ah, I guess you do need header protection here.

OK, looks good to me.

Acked-by: Steven Rostedt <rostedt@goodmis.org>

-- Steve

> +
> +#include <net/devlink.h>
> +
> +static inline void trace_devlink_hwmsg(const struct devlink *devlink,
> +				       bool incoming, unsigned long type,
> +				       const u8 *buf, size_t len)
> +{
> +}
> +
> +#endif /* _TRACE_DEVLINK_H */
> +
> +#endif
> diff --git a/net/core/devlink.c b/net/core/devlink.c
> index b2e592a..1b50630 100644
> --- a/net/core/devlink.c
> +++ b/net/core/devlink.c
> @@ -26,6 +26,10 @@
>  #include <net/net_namespace.h>
>  #include <net/sock.h>
>  #include <net/devlink.h>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/devlink.h>
> +
> +EXPORT_TRACEPOINT_SYMBOL_GPL(devlink_hwmsg);
>  
>  static LIST_HEAD(devlink_list);
>  

Re: [patch net-next v2 1/2] devlink: add hardware messages tracing facility

[Jiri Pirko]

Tue, Jul 12, 2016 at 06:38:26PM CEST, rostedt@goodmis.org wrote:
>On Tue, 12 Jul 2016 18:05:03 +0200
>Jiri Pirko <jiri@resnulli.us> wrote:
>
>> From: Jiri Pirko <jiri@mellanox.com>
>> 
>> Define a tracepoint and allow user to trace messages going to and from
>> hardware associated with devlink instance.
>> 
>> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
>> ---
>> v1->v2:
>> - Use EXPORT_TRACEPOINT_SYMBOL_GPL instead of a wrapper function
>>   as suggested by David Ahern and Steven Rostedt
>
>FYI, you can use the Suggested-by: tag too ;-)

Thought that the purpose of that tag is if someone suggested the whole
patch existence.

<snip>

>> +/* This part must be outside protection */
>> +#include <trace/define_trace.h>
>> +
>> +#else /* CONFIG_NET_DEVLINK */
>> +
>> +#if !defined(_TRACE_DEVLINK_H)
>> +#define _TRACE_DEVLINK_H
>
>Ah, I guess you do need header protection here.

Yes, you do.

>
>OK, looks good to me.
>
>Acked-by: Steven Rostedt <rostedt@goodmis.org>

Thanks Steven!

Re: [patch net-next v2 1/2] devlink: add hardware messages tracing facility

[Steven Rostedt]

On Tue, 12 Jul 2016 19:10:46 +0200
Jiri Pirko <jiri@resnulli.us> wrote:

> Tue, Jul 12, 2016 at 06:38:26PM CEST, rostedt@goodmis.org wrote:
> >On Tue, 12 Jul 2016 18:05:03 +0200
> >Jiri Pirko <jiri@resnulli.us> wrote:
> >  
> >> From: Jiri Pirko <jiri@mellanox.com>
> >> 
> >> Define a tracepoint and allow user to trace messages going to and from
> >> hardware associated with devlink instance.
> >> 
> >> Signed-off-by: Jiri Pirko <jiri@mellanox.com>
> >> ---
> >> v1->v2:
> >> - Use EXPORT_TRACEPOINT_SYMBOL_GPL instead of a wrapper function
> >>   as suggested by David Ahern and Steven Rostedt  
> >
> >FYI, you can use the Suggested-by: tag too ;-)  
> 
> Thought that the purpose of that tag is if someone suggested the whole
> patch existence.

Yeah, it has different meanings. You could always be creative and add: 

Parts-suggested-by: ....

;-)

-- Steve

Re: [patch net-next v2 1/2] devlink: add hardware messages tracing facility

[David Miller]

From: Jiri Pirko <jiri@resnulli.us>
Date: Tue, 12 Jul 2016 18:05:03 +0200

> From: Jiri Pirko <jiri@mellanox.com>
> 
> Define a tracepoint and allow user to trace messages going to and from
> hardware associated with devlink instance.
> 
> Signed-off-by: Jiri Pirko <jiri@mellanox.com>

Applied.