* [PATCH] i2c: add tracepoints for I2C slave events
@ 2022-03-07 18:20 Jae Hyun Yoo
2022-03-07 19:13 ` Steven Rostedt
0 siblings, 1 reply; 3+ messages in thread
From: Jae Hyun Yoo @ 2022-03-07 18:20 UTC (permalink / raw)
To: Wolfram Sang, Steven Rostedt, Ingo Molnar
Cc: Jamie Iles, Graeme Gregory, linux-kernel, linux-i2c, Jae Hyun Yoo
I2C slave events tracepoints can be enabled by:
echo 1 > /sys/kernel/tracing/events/i2c_slave/enable
and logs in /sys/kernel/tracing/trace will look like:
... i2c_slave: i2c-0 a=010 WR_REQ []
... i2c_slave: i2c-0 a=010 WR_RCV [02]
... i2c_slave: i2c-0 a=010 WR_RCV [0c]
... i2c_slave: i2c-0 a=010 STOP []
... i2c_slave: i2c-0 a=010 RD_REQ [04]
... i2c_slave: i2c-0 a=010 RD_PRO [b4]
... i2c_slave: i2c-0 a=010 STOP []
formatted as:
i2c-<adapter-nr>
a=<addr>
<event>
[<data>]
trace printings can be selected by adding a filter like:
echo adapter_nr==1 >/sys/kernel/tracing/events/i2c_slave/filter
Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
---
drivers/i2c/i2c-core-slave.c | 15 +++++++++
include/linux/i2c.h | 8 ++---
include/trace/events/i2c_slave.h | 57 ++++++++++++++++++++++++++++++++
3 files changed, 74 insertions(+), 6 deletions(-)
create mode 100644 include/trace/events/i2c_slave.h
diff --git a/drivers/i2c/i2c-core-slave.c b/drivers/i2c/i2c-core-slave.c
index 1589179d5eb9..4968a17328b3 100644
--- a/drivers/i2c/i2c-core-slave.c
+++ b/drivers/i2c/i2c-core-slave.c
@@ -14,6 +14,9 @@
#include "i2c-core.h"
+#define CREATE_TRACE_POINTS
+#include <trace/events/i2c_slave.h>
+
int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb)
{
int ret;
@@ -79,6 +82,18 @@ int i2c_slave_unregister(struct i2c_client *client)
}
EXPORT_SYMBOL_GPL(i2c_slave_unregister);
+int i2c_slave_event(struct i2c_client *client,
+ enum i2c_slave_event event, u8 *val)
+{
+ int ret = client->slave_cb(client, event, val);
+
+ if (!ret)
+ trace_i2c_slave(client, event, val);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(i2c_slave_event);
+
/**
* i2c_detect_slave_mode - detect operation mode
* @dev: The device owning the bus
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index 7d4f52ceb7b5..fbda5ada2afc 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -392,12 +392,8 @@ enum i2c_slave_event {
int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
int i2c_slave_unregister(struct i2c_client *client);
bool i2c_detect_slave_mode(struct device *dev);
-
-static inline int i2c_slave_event(struct i2c_client *client,
- enum i2c_slave_event event, u8 *val)
-{
- return client->slave_cb(client, event, val);
-}
+int i2c_slave_event(struct i2c_client *client,
+ enum i2c_slave_event event, u8 *val);
#else
static inline bool i2c_detect_slave_mode(struct device *dev) { return false; }
#endif
diff --git a/include/trace/events/i2c_slave.h b/include/trace/events/i2c_slave.h
new file mode 100644
index 000000000000..1f0c1cfbf2ef
--- /dev/null
+++ b/include/trace/events/i2c_slave.h
@@ -0,0 +1,57 @@
+/* SPDX-License-Identifier: GPL-2.0-or-later */
+/*
+ * I2C slave tracepoints
+ *
+ * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
+ */
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM i2c_slave
+
+#if !defined(_TRACE_I2C_SLAVE_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_I2C_SLAVE_H
+
+#include <linux/i2c.h>
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(i2c_slave,
+ TP_PROTO(const struct i2c_client *client, enum i2c_slave_event event,
+ __u8 *val),
+ TP_ARGS(client, event, val),
+ TP_STRUCT__entry(
+ __field(int, adapter_nr )
+ __field(__u16, addr )
+ __field(enum i2c_slave_event, event )
+ __field(__u16, len )
+ __dynamic_array(__u8, buf, 1) ),
+
+ TP_fast_assign(
+ __entry->adapter_nr = client->adapter->nr;
+ __entry->addr = client->addr;
+ __entry->event = event;
+ switch (event) {
+ case I2C_SLAVE_READ_REQUESTED:
+ case I2C_SLAVE_READ_PROCESSED:
+ case I2C_SLAVE_WRITE_RECEIVED:
+ __entry->len = 1;
+ memcpy(__get_dynamic_array(buf), val, __entry->len);
+ break;
+ default:
+ __entry->len = 0;
+ break;
+ }
+ ),
+ TP_printk("i2c-%d a=%03x %s [%*phD]",
+ __entry->adapter_nr, __entry->addr,
+ __print_symbolic(__entry->event,
+ { I2C_SLAVE_READ_REQUESTED, "RD_REQ" },
+ { I2C_SLAVE_WRITE_REQUESTED, "WR_REQ" },
+ { I2C_SLAVE_READ_PROCESSED, "RD_PRO" },
+ { I2C_SLAVE_WRITE_RECEIVED, "WR_RCV" },
+ { I2C_SLAVE_STOP, " STOP" }),
+ __entry->len, __get_dynamic_array(buf)
+ ));
+
+#endif /* _TRACE_I2C_SLAVE_H */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
--
2.25.1
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] i2c: add tracepoints for I2C slave events
2022-03-07 18:20 [PATCH] i2c: add tracepoints for I2C slave events Jae Hyun Yoo
@ 2022-03-07 19:13 ` Steven Rostedt
2022-03-07 19:29 ` Jae Hyun Yoo
0 siblings, 1 reply; 3+ messages in thread
From: Steven Rostedt @ 2022-03-07 19:13 UTC (permalink / raw)
To: Jae Hyun Yoo
Cc: Wolfram Sang, Ingo Molnar, Jamie Iles, Graeme Gregory,
linux-kernel, linux-i2c
On Mon, 7 Mar 2022 10:20:49 -0800
Jae Hyun Yoo <quic_jaehyoo@quicinc.com> wrote:
> I2C slave events tracepoints can be enabled by:
>
> echo 1 > /sys/kernel/tracing/events/i2c_slave/enable
>
> and logs in /sys/kernel/tracing/trace will look like:
>
> ... i2c_slave: i2c-0 a=010 WR_REQ []
> ... i2c_slave: i2c-0 a=010 WR_RCV [02]
> ... i2c_slave: i2c-0 a=010 WR_RCV [0c]
> ... i2c_slave: i2c-0 a=010 STOP []
> ... i2c_slave: i2c-0 a=010 RD_REQ [04]
> ... i2c_slave: i2c-0 a=010 RD_PRO [b4]
> ... i2c_slave: i2c-0 a=010 STOP []
>
> formatted as:
>
> i2c-<adapter-nr>
> a=<addr>
> <event>
> [<data>]
>
> trace printings can be selected by adding a filter like:
>
> echo adapter_nr==1 >/sys/kernel/tracing/events/i2c_slave/filter
>
> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
> ---
> drivers/i2c/i2c-core-slave.c | 15 +++++++++
> include/linux/i2c.h | 8 ++---
> include/trace/events/i2c_slave.h | 57 ++++++++++++++++++++++++++++++++
> 3 files changed, 74 insertions(+), 6 deletions(-)
> create mode 100644 include/trace/events/i2c_slave.h
>
> diff --git a/drivers/i2c/i2c-core-slave.c b/drivers/i2c/i2c-core-slave.c
> index 1589179d5eb9..4968a17328b3 100644
> --- a/drivers/i2c/i2c-core-slave.c
> +++ b/drivers/i2c/i2c-core-slave.c
> @@ -14,6 +14,9 @@
>
> #include "i2c-core.h"
>
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/i2c_slave.h>
> +
> int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb)
> {
> int ret;
> @@ -79,6 +82,18 @@ int i2c_slave_unregister(struct i2c_client *client)
> }
> EXPORT_SYMBOL_GPL(i2c_slave_unregister);
>
> +int i2c_slave_event(struct i2c_client *client,
> + enum i2c_slave_event event, u8 *val)
> +{
> + int ret = client->slave_cb(client, event, val);
> +
> + if (!ret)
You can make the above into:
if (trace_i2c_slave_enabled() && !ret)
to make this conditional compare only happen if the tracepoint is enabled.
As the trace_i2c_slave_enabled() is a static branch (non-conditional jump).
> + trace_i2c_slave(client, event, val);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(i2c_slave_event);
> +
> /**
> * i2c_detect_slave_mode - detect operation mode
> * @dev: The device owning the bus
> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
> index 7d4f52ceb7b5..fbda5ada2afc 100644
> --- a/include/linux/i2c.h
> +++ b/include/linux/i2c.h
> @@ -392,12 +392,8 @@ enum i2c_slave_event {
> int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
> int i2c_slave_unregister(struct i2c_client *client);
> bool i2c_detect_slave_mode(struct device *dev);
> -
> -static inline int i2c_slave_event(struct i2c_client *client,
> - enum i2c_slave_event event, u8 *val)
> -{
> - return client->slave_cb(client, event, val);
> -}
> +int i2c_slave_event(struct i2c_client *client,
> + enum i2c_slave_event event, u8 *val);
> #else
> static inline bool i2c_detect_slave_mode(struct device *dev) { return false; }
> #endif
> diff --git a/include/trace/events/i2c_slave.h b/include/trace/events/i2c_slave.h
> new file mode 100644
> index 000000000000..1f0c1cfbf2ef
> --- /dev/null
> +++ b/include/trace/events/i2c_slave.h
> @@ -0,0 +1,57 @@
> +/* SPDX-License-Identifier: GPL-2.0-or-later */
> +/*
> + * I2C slave tracepoints
> + *
> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
> + */
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM i2c_slave
> +
> +#if !defined(_TRACE_I2C_SLAVE_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_I2C_SLAVE_H
> +
> +#include <linux/i2c.h>
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(i2c_slave,
> + TP_PROTO(const struct i2c_client *client, enum i2c_slave_event event,
> + __u8 *val),
> + TP_ARGS(client, event, val),
> + TP_STRUCT__entry(
> + __field(int, adapter_nr )
> + __field(__u16, addr )
> + __field(enum i2c_slave_event, event )
> + __field(__u16, len )
I would keep the u16 together:
__field(int, adapter_nr )
__field(__u16, addr )
__field(__u16, len )
__field(enum i2c_slave_event, event )
Otherwise you will likely have a hole in the event, which wastes space on
the ring buffer.
> + __dynamic_array(__u8, buf, 1) ),
> +
> + TP_fast_assign(
> + __entry->adapter_nr = client->adapter->nr;
> + __entry->addr = client->addr;
> + __entry->event = event;
> + switch (event) {
> + case I2C_SLAVE_READ_REQUESTED:
> + case I2C_SLAVE_READ_PROCESSED:
> + case I2C_SLAVE_WRITE_RECEIVED:
> + __entry->len = 1;
> + memcpy(__get_dynamic_array(buf), val, __entry->len);
Why the dynamic event, if it is always the size of 1? Why not make it an
array. It will save space, as the dynamic meta data has to live on the
event which is 4 bytes big. Just make it:
__array(__u8, buf, 1);
It's faster and saves space.
-- Steve
> + break;
> + default:
> + __entry->len = 0;
> + break;
> + }
> + ),
> + TP_printk("i2c-%d a=%03x %s [%*phD]",
> + __entry->adapter_nr, __entry->addr,
> + __print_symbolic(__entry->event,
> + { I2C_SLAVE_READ_REQUESTED, "RD_REQ" },
> + { I2C_SLAVE_WRITE_REQUESTED, "WR_REQ" },
> + { I2C_SLAVE_READ_PROCESSED, "RD_PRO" },
> + { I2C_SLAVE_WRITE_RECEIVED, "WR_RCV" },
> + { I2C_SLAVE_STOP, " STOP" }),
> + __entry->len, __get_dynamic_array(buf)
> + ));
> +
> +#endif /* _TRACE_I2C_SLAVE_H */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] i2c: add tracepoints for I2C slave events
2022-03-07 19:13 ` Steven Rostedt
@ 2022-03-07 19:29 ` Jae Hyun Yoo
0 siblings, 0 replies; 3+ messages in thread
From: Jae Hyun Yoo @ 2022-03-07 19:29 UTC (permalink / raw)
To: Steven Rostedt
Cc: Wolfram Sang, Ingo Molnar, Jamie Iles, Graeme Gregory,
linux-kernel, linux-i2c
Hi Steven,
On 3/7/2022 11:13 AM, Steven Rostedt wrote:
> On Mon, 7 Mar 2022 10:20:49 -0800
> Jae Hyun Yoo <quic_jaehyoo@quicinc.com> wrote:
>
>> I2C slave events tracepoints can be enabled by:
>>
>> echo 1 > /sys/kernel/tracing/events/i2c_slave/enable
>>
>> and logs in /sys/kernel/tracing/trace will look like:
>>
>> ... i2c_slave: i2c-0 a=010 WR_REQ []
>> ... i2c_slave: i2c-0 a=010 WR_RCV [02]
>> ... i2c_slave: i2c-0 a=010 WR_RCV [0c]
>> ... i2c_slave: i2c-0 a=010 STOP []
>> ... i2c_slave: i2c-0 a=010 RD_REQ [04]
>> ... i2c_slave: i2c-0 a=010 RD_PRO [b4]
>> ... i2c_slave: i2c-0 a=010 STOP []
>>
>> formatted as:
>>
>> i2c-<adapter-nr>
>> a=<addr>
>> <event>
>> [<data>]
>>
>> trace printings can be selected by adding a filter like:
>>
>> echo adapter_nr==1 >/sys/kernel/tracing/events/i2c_slave/filter
>>
>> Signed-off-by: Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
>> ---
>> drivers/i2c/i2c-core-slave.c | 15 +++++++++
>> include/linux/i2c.h | 8 ++---
>> include/trace/events/i2c_slave.h | 57 ++++++++++++++++++++++++++++++++
>> 3 files changed, 74 insertions(+), 6 deletions(-)
>> create mode 100644 include/trace/events/i2c_slave.h
>>
>> diff --git a/drivers/i2c/i2c-core-slave.c b/drivers/i2c/i2c-core-slave.c
>> index 1589179d5eb9..4968a17328b3 100644
>> --- a/drivers/i2c/i2c-core-slave.c
>> +++ b/drivers/i2c/i2c-core-slave.c
>> @@ -14,6 +14,9 @@
>>
>> #include "i2c-core.h"
>>
>> +#define CREATE_TRACE_POINTS
>> +#include <trace/events/i2c_slave.h>
>> +
>> int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb)
>> {
>> int ret;
>> @@ -79,6 +82,18 @@ int i2c_slave_unregister(struct i2c_client *client)
>> }
>> EXPORT_SYMBOL_GPL(i2c_slave_unregister);
>>
>> +int i2c_slave_event(struct i2c_client *client,
>> + enum i2c_slave_event event, u8 *val)
>> +{
>> + int ret = client->slave_cb(client, event, val);
>> +
>> + if (!ret)
>
> You can make the above into:
>
> if (trace_i2c_slave_enabled() && !ret)
>
> to make this conditional compare only happen if the tracepoint is enabled.
> As the trace_i2c_slave_enabled() is a static branch (non-conditional jump).
Right, that's better. I'll fix it in the next spin.
>> + trace_i2c_slave(client, event, val);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(i2c_slave_event);
>> +
>> /**
>> * i2c_detect_slave_mode - detect operation mode
>> * @dev: The device owning the bus
>> diff --git a/include/linux/i2c.h b/include/linux/i2c.h
>> index 7d4f52ceb7b5..fbda5ada2afc 100644
>> --- a/include/linux/i2c.h
>> +++ b/include/linux/i2c.h
>> @@ -392,12 +392,8 @@ enum i2c_slave_event {
>> int i2c_slave_register(struct i2c_client *client, i2c_slave_cb_t slave_cb);
>> int i2c_slave_unregister(struct i2c_client *client);
>> bool i2c_detect_slave_mode(struct device *dev);
>> -
>> -static inline int i2c_slave_event(struct i2c_client *client,
>> - enum i2c_slave_event event, u8 *val)
>> -{
>> - return client->slave_cb(client, event, val);
>> -}
>> +int i2c_slave_event(struct i2c_client *client,
>> + enum i2c_slave_event event, u8 *val);
>> #else
>> static inline bool i2c_detect_slave_mode(struct device *dev) { return false; }
>> #endif
>> diff --git a/include/trace/events/i2c_slave.h b/include/trace/events/i2c_slave.h
>> new file mode 100644
>> index 000000000000..1f0c1cfbf2ef
>> --- /dev/null
>> +++ b/include/trace/events/i2c_slave.h
>> @@ -0,0 +1,57 @@
>> +/* SPDX-License-Identifier: GPL-2.0-or-later */
>> +/*
>> + * I2C slave tracepoints
>> + *
>> + * Copyright (c) 2022 Qualcomm Innovation Center, Inc. All rights reserved.
>> + */
>> +#undef TRACE_SYSTEM
>> +#define TRACE_SYSTEM i2c_slave
>> +
>> +#if !defined(_TRACE_I2C_SLAVE_H) || defined(TRACE_HEADER_MULTI_READ)
>> +#define _TRACE_I2C_SLAVE_H
>> +
>> +#include <linux/i2c.h>
>> +#include <linux/tracepoint.h>
>> +
>> +TRACE_EVENT(i2c_slave,
>> + TP_PROTO(const struct i2c_client *client, enum i2c_slave_event event,
>> + __u8 *val),
>> + TP_ARGS(client, event, val),
>> + TP_STRUCT__entry(
>> + __field(int, adapter_nr )
>> + __field(__u16, addr )
>> + __field(enum i2c_slave_event, event )
>> + __field(__u16, len )
>
> I would keep the u16 together:
>
> __field(int, adapter_nr )
> __field(__u16, addr )
> __field(__u16, len )
> __field(enum i2c_slave_event, event )
>
> Otherwise you will likely have a hole in the event, which wastes space on
> the ring buffer.
Thanks for your pointing it out. I'll fix it too in v2.
>> + __dynamic_array(__u8, buf, 1) ),
>> +
>> + TP_fast_assign(
>> + __entry->adapter_nr = client->adapter->nr;
>> + __entry->addr = client->addr;
>> + __entry->event = event;
>> + switch (event) {
>> + case I2C_SLAVE_READ_REQUESTED:
>> + case I2C_SLAVE_READ_PROCESSED:
>> + case I2C_SLAVE_WRITE_RECEIVED:
>> + __entry->len = 1;
>> + memcpy(__get_dynamic_array(buf), val, __entry->len);
>
> Why the dynamic event, if it is always the size of 1? Why not make it an
> array. It will save space, as the dynamic meta data has to live on the
> event which is 4 bytes big. Just make it:
>
> __array(__u8, buf, 1);
>
> It's faster and saves space.
Yes, the data length is always 1. I'll fix it too.
Thanks a lot for your review and suggestions! I'll address all your
comments in the next spin.
-Jae
> -- Steve
>
>> + break;
>> + default:
>> + __entry->len = 0;
>> + break;
>> + }
>> + ),
>> + TP_printk("i2c-%d a=%03x %s [%*phD]",
>> + __entry->adapter_nr, __entry->addr,
>> + __print_symbolic(__entry->event,
>> + { I2C_SLAVE_READ_REQUESTED, "RD_REQ" },
>> + { I2C_SLAVE_WRITE_REQUESTED, "WR_REQ" },
>> + { I2C_SLAVE_READ_PROCESSED, "RD_PRO" },
>> + { I2C_SLAVE_WRITE_RECEIVED, "WR_RCV" },
>> + { I2C_SLAVE_STOP, " STOP" }),
>> + __entry->len, __get_dynamic_array(buf)
>> + ));
>> +
>> +#endif /* _TRACE_I2C_SLAVE_H */
>> +
>> +/* This part must be outside protection */
>> +#include <trace/define_trace.h>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2022-03-07 19:29 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-03-07 18:20 [PATCH] i2c: add tracepoints for I2C slave events Jae Hyun Yoo
2022-03-07 19:13 ` Steven Rostedt
2022-03-07 19:29 ` Jae Hyun Yoo
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.