Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 7 Mar 2022 14:13:05 -0500
From:   Steven Rostedt <rostedt@goodmis.org>
To:     Jae Hyun Yoo <quic_jaehyoo@quicinc.com>
Cc:     Wolfram Sang <wsa@kernel.org>, "Ingo Molnar" <mingo@redhat.com>,
        Jamie Iles <quic_jiles@quicinc.com>,
        Graeme Gregory <quic_ggregory@quicinc.com>,
        <linux-kernel@vger.kernel.org>, <linux-i2c@vger.kernel.org>
Subject: Re: [PATCH] i2c: add tracepoints for I2C slave events
Message-ID: <20220307141305.18f0c20b@gandalf.local.home>
In-Reply-To: <20220307182049.3790905-1-quic_jaehyoo@quicinc.com>
References: <20220307182049.3790905-1-quic_jaehyoo@quicinc.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

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>