| 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
| ||
|
Message-ID: <c60ce488-44b0-89cf-4dd5-5f178b004254@quicinc.com>
Date: Mon, 7 Mar 2022 11:29:30 -0800
From: Jae Hyun Yoo <quic_jaehyoo@...cinc.com>
To: Steven Rostedt <rostedt@...dmis.org>
CC: Wolfram Sang <wsa@...nel.org>, Ingo Molnar <mingo@...hat.com>,
Jamie Iles <quic_jiles@...cinc.com>,
Graeme Gregory <quic_ggregory@...cinc.com>,
<linux-kernel@...r.kernel.org>, <linux-i2c@...r.kernel.org>
Subject: Re: [PATCH] i2c: add tracepoints for I2C slave events
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@...cinc.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@...cinc.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>
>
Powered by blists - more mailing lists