[PATCH] gpio: add trace events for setting direction and value

[PATCH] gpio: add trace events for setting direction and value

[Uwe Kleine-König]

This patch allows to trace gpio operations using ftrace

Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
---
Hello,

two things that could be optimised later are:

 - combine gpio_direction_output into a single event?
 - record _cansleep?

Best regards
Uwe

 drivers/gpio/gpiolib.c      |   18 ++++++++++++-
 include/trace/events/gpio.h |   56 +++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 72 insertions(+), 2 deletions(-)
 create mode 100644 include/trace/events/gpio.h

diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
index 649550e..5fc5e2d 100644
--- a/drivers/gpio/gpiolib.c
+++ b/drivers/gpio/gpiolib.c
@@ -12,6 +12,8 @@
 #include <linux/idr.h>
 #include <linux/slab.h>
 
+#define CREATE_TRACE_POINTS
+#include <trace/events/gpio.h>
 
 /* Optional implementation infrastructure for GPIO interfaces.
  *
@@ -1404,6 +1406,8 @@ int gpio_direction_input(unsigned gpio)
 	status = chip->direction_input(chip, gpio);
 	if (status == 0)
 		clear_bit(FLAG_IS_OUT, &desc->flags);
+
+	trace_gpio_direction(chip->base + gpio, 1, status);
 lose:
 	return status;
 fail:
@@ -1457,6 +1461,8 @@ int gpio_direction_output(unsigned gpio, int value)
 	status = chip->direction_output(chip, gpio, value);
 	if (status == 0)
 		set_bit(FLAG_IS_OUT, &desc->flags);
+	trace_gpio_value(chip->base + gpio, 0, value);
+	trace_gpio_direction(chip->base + gpio, 0, status);
 lose:
 	return status;
 fail:
@@ -1546,10 +1552,13 @@ EXPORT_SYMBOL_GPL(gpio_set_debounce);
 int __gpio_get_value(unsigned gpio)
 {
 	struct gpio_chip	*chip;
+	int value;
 
 	chip = gpio_to_chip(gpio);
 	WARN_ON(chip->can_sleep);
-	return chip->get ? chip->get(chip, gpio - chip->base) : 0;
+	value = chip->get ? chip->get(chip, gpio - chip->base) : 0;
+	trace_gpio_value(gpio, 1, value);
+	return value;
 }
 EXPORT_SYMBOL_GPL(__gpio_get_value);
 
@@ -1568,6 +1577,7 @@ void __gpio_set_value(unsigned gpio, int value)
 
 	chip = gpio_to_chip(gpio);
 	WARN_ON(chip->can_sleep);
+	trace_gpio_value(gpio, 0, value);
 	chip->set(chip, gpio - chip->base, value);
 }
 EXPORT_SYMBOL_GPL(__gpio_set_value);
@@ -1618,10 +1628,13 @@ EXPORT_SYMBOL_GPL(__gpio_to_irq);
 int gpio_get_value_cansleep(unsigned gpio)
 {
 	struct gpio_chip	*chip;
+	int value;
 
 	might_sleep_if(extra_checks);
 	chip = gpio_to_chip(gpio);
-	return chip->get ? chip->get(chip, gpio - chip->base) : 0;
+	value = chip->get ? chip->get(chip, gpio - chip->base) : 0;
+	trace_gpio_value(gpio, 1, value);
+	return value;
 }
 EXPORT_SYMBOL_GPL(gpio_get_value_cansleep);
 
@@ -1631,6 +1644,7 @@ void gpio_set_value_cansleep(unsigned gpio, int value)
 
 	might_sleep_if(extra_checks);
 	chip = gpio_to_chip(gpio);
+	trace_gpio_value(gpio, 0, value);
 	chip->set(chip, gpio - chip->base, value);
 }
 EXPORT_SYMBOL_GPL(gpio_set_value_cansleep);
diff --git a/include/trace/events/gpio.h b/include/trace/events/gpio.h
new file mode 100644
index 0000000..927a8ad
--- /dev/null
+++ b/include/trace/events/gpio.h
@@ -0,0 +1,56 @@
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM gpio
+
+#if !defined(_TRACE_GPIO_H) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_GPIO_H
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(gpio_direction,
+
+	TP_PROTO(unsigned gpio, int in, int err),
+
+	TP_ARGS(gpio, in, err),
+
+	TP_STRUCT__entry(
+		__field(unsigned, gpio)
+		__field(int, in)
+		__field(int, err)
+	),
+
+	TP_fast_assign(
+		__entry->gpio = gpio;
+		__entry->in = in;
+		__entry->err = err;
+	),
+
+	TP_printk("%u %3s (%d)", __entry->gpio,
+		__entry->in ? "in" : "out", __entry->err)
+);
+
+TRACE_EVENT(gpio_value,
+
+	TP_PROTO(unsigned gpio, int get, int value),
+
+	TP_ARGS(gpio, get, value),
+
+	TP_STRUCT__entry(
+		__field(unsigned, gpio)
+		__field(int, get)
+		__field(int, value)
+	),
+
+	TP_fast_assign(
+		__entry->gpio = gpio;
+		__entry->get = get;
+		__entry->value = value;
+	),
+
+	TP_printk("%u %3s %d", __entry->gpio,
+		__entry->get ? "get" : "set", __entry->value)
+);
+
+#endif /* if !defined(_TRACE_GPIO_H) || defined(TRACE_HEADER_MULTI_READ) */
+
+/* This part must be outside protection */
+#include <trace/define_trace.h>
-- 
1.7.2.3

Re: [PATCH] gpio: add trace events for setting direction and value

[Steven Rostedt]

On Wed, 2011-02-16 at 18:03 +0100, Uwe Kleine-König wrote:

> --- /dev/null
> +++ b/include/trace/events/gpio.h
> @@ -0,0 +1,56 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM gpio
> +
> +#if !defined(_TRACE_GPIO_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_GPIO_H
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(gpio_direction,
> +
> +	TP_PROTO(unsigned gpio, int in, int err),
> +
> +	TP_ARGS(gpio, in, err),
> +
> +	TP_STRUCT__entry(
> +		__field(unsigned, gpio)
> +		__field(int, in)
> +		__field(int, err)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->gpio = gpio;
> +		__entry->in = in;
> +		__entry->err = err;
> +	),
> +
> +	TP_printk("%u %3s (%d)", __entry->gpio,
> +		__entry->in ? "in" : "out", __entry->err)
> +);
> +
> +TRACE_EVENT(gpio_value,
> +
> +	TP_PROTO(unsigned gpio, int get, int value),
> +
> +	TP_ARGS(gpio, get, value),
> +
> +	TP_STRUCT__entry(
> +		__field(unsigned, gpio)
> +		__field(int, get)
> +		__field(int, value)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->gpio = gpio;
> +		__entry->get = get;
> +		__entry->value = value;
> +	),
> +
> +	TP_printk("%u %3s %d", __entry->gpio,
> +		__entry->get ? "get" : "set", __entry->value)
> +);
> +

Note: to save the memory footprint of these tracepoints, you can use
DEFINE_EVENT_PRINT(). You can see the usage for this in the
include/trace/events/kmem.h.

But to do this, you will need to have a single TP_STRUCT__entry() for
both. Not sure if this is what you want.

	TP_STRUCT__entry(
		__field(unsigned, gpiq)
		__field(int, get_in)
		__field(int, value_err)

??

Just a suggestion, but may not be worth it.

-- Steve

Re: [PATCH] gpio: add trace events for setting direction and value

[Uwe Kleine-König]

Hi Steven, hi Grant,

On Wed, Feb 16, 2011 at 12:12:51PM -0500, Steven Rostedt wrote:
> Note: to save the memory footprint of these tracepoints, you can use
> DEFINE_EVENT_PRINT(). You can see the usage for this in the
> include/trace/events/kmem.h.
> But to do this, you will need to have a single TP_STRUCT__entry() for
> both. Not sure if this is what you want.
> 
> 	TP_STRUCT__entry(
> 		__field(unsigned, gpiq)
> 		__field(int, get_in)
> 		__field(int, value_err)
> 
> ??
> 
> Just a suggestion, but may not be worth it.
Yeah, I saw that, still I think it's sane to keep them seperated.
Or how much would we save?  Can you estimate that?

@Grant: Steven told me this should go via your tree, so if you are OK
with the change, feel free to take it.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] gpio: add trace events for setting direction and value

[Steven Rostedt]

On Fri, 2011-02-18 at 10:58 +0100, Uwe Kleine-König wrote:
> Hi Steven, hi Grant,
> 
> On Wed, Feb 16, 2011 at 12:12:51PM -0500, Steven Rostedt wrote:
> > Note: to save the memory footprint of these tracepoints, you can use
> > DEFINE_EVENT_PRINT(). You can see the usage for this in the
> > include/trace/events/kmem.h.
> > But to do this, you will need to have a single TP_STRUCT__entry() for
> > both. Not sure if this is what you want.
> > 
> > 	TP_STRUCT__entry(
> > 		__field(unsigned, gpiq)
> > 		__field(int, get_in)
> > 		__field(int, value_err)
> > 
> > ??
> > 
> > Just a suggestion, but may not be worth it.
> Yeah, I saw that, still I think it's sane to keep them seperated.
> Or how much would we save?  Can you estimate that?

You can do it :)   Especially since it can vary by archs.

Just compile the kernel once this way, and then try it with
DEFINE_EVENT_PRINT(), compile the kernel and run size on the two.

Then you can see the difference it makes. It may end up not being worth
the difference. But as embedded uses gpio the most, I'll leave that up
to you.

-- Steve

Re: [PATCH] gpio: add trace events for setting direction and value

[Grant Likely]

On Fri, Feb 18, 2011 at 10:48:06AM -0500, Steven Rostedt wrote:
> On Fri, 2011-02-18 at 10:58 +0100, Uwe Kleine-König wrote:
> > Hi Steven, hi Grant,
> > 
> > On Wed, Feb 16, 2011 at 12:12:51PM -0500, Steven Rostedt wrote:
> > > Note: to save the memory footprint of these tracepoints, you can use
> > > DEFINE_EVENT_PRINT(). You can see the usage for this in the
> > > include/trace/events/kmem.h.
> > > But to do this, you will need to have a single TP_STRUCT__entry() for
> > > both. Not sure if this is what you want.
> > > 
> > > 	TP_STRUCT__entry(
> > > 		__field(unsigned, gpiq)
> > > 		__field(int, get_in)
> > > 		__field(int, value_err)
> > > 
> > > ??
> > > 
> > > Just a suggestion, but may not be worth it.
> > Yeah, I saw that, still I think it's sane to keep them seperated.
> > Or how much would we save?  Can you estimate that?
> 
> You can do it :)   Especially since it can vary by archs.
> 
> Just compile the kernel once this way, and then try it with
> DEFINE_EVENT_PRINT(), compile the kernel and run size on the two.
> 
> Then you can see the difference it makes. It may end up not being worth
> the difference. But as embedded uses gpio the most, I'll leave that up
> to you.

Uwe, any update on this?  Are you going to spin a new patch, or should
I take this one?

g.

Re: [PATCH] gpio: add trace events for setting direction and value

[Uwe Kleine-König]

Hi Grant,

On Wed, Mar 02, 2011 at 03:03:05PM -0700, Grant Likely wrote:
> On Fri, Feb 18, 2011 at 10:48:06AM -0500, Steven Rostedt wrote:
> > On Fri, 2011-02-18 at 10:58 +0100, Uwe Kleine-König wrote:
> > > Hi Steven, hi Grant,
> > > 
> > > On Wed, Feb 16, 2011 at 12:12:51PM -0500, Steven Rostedt wrote:
> > > > Note: to save the memory footprint of these tracepoints, you can use
> > > > DEFINE_EVENT_PRINT(). You can see the usage for this in the
> > > > include/trace/events/kmem.h.
> > > > But to do this, you will need to have a single TP_STRUCT__entry() for
> > > > both. Not sure if this is what you want.
> > > > 
> > > > 	TP_STRUCT__entry(
> > > > 		__field(unsigned, gpiq)
> > > > 		__field(int, get_in)
> > > > 		__field(int, value_err)
> > > > 
> > > > ??
> > > > 
> > > > Just a suggestion, but may not be worth it.
> > > Yeah, I saw that, still I think it's sane to keep them seperated.
> > > Or how much would we save?  Can you estimate that?
> > 
> > You can do it :)   Especially since it can vary by archs.
> > 
> > Just compile the kernel once this way, and then try it with
> > DEFINE_EVENT_PRINT(), compile the kernel and run size on the two.
> > 
> > Then you can see the difference it makes. It may end up not being worth
> > the difference. But as embedded uses gpio the most, I'll leave that up
> > to you.
> 
> Uwe, any update on this?  Are you going to spin a new patch, or should
> I take this one?
I think it's OK to take as is.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

Re: [PATCH] gpio: add trace events for setting direction and value

[Grant Likely]

On Wed, Feb 16, 2011 at 06:03:04PM +0100, Uwe Kleine-König wrote:
> This patch allows to trace gpio operations using ftrace
> 
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Applied, thanks.

g.

> ---
> Hello,
> 
> two things that could be optimised later are:
> 
>  - combine gpio_direction_output into a single event?
>  - record _cansleep?
> 
> Best regards
> Uwe
> 
>  drivers/gpio/gpiolib.c      |   18 ++++++++++++-
>  include/trace/events/gpio.h |   56 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 72 insertions(+), 2 deletions(-)
>  create mode 100644 include/trace/events/gpio.h
> 
> diff --git a/drivers/gpio/gpiolib.c b/drivers/gpio/gpiolib.c
> index 649550e..5fc5e2d 100644
> --- a/drivers/gpio/gpiolib.c
> +++ b/drivers/gpio/gpiolib.c
> @@ -12,6 +12,8 @@
>  #include <linux/idr.h>
>  #include <linux/slab.h>
>  
> +#define CREATE_TRACE_POINTS
> +#include <trace/events/gpio.h>
>  
>  /* Optional implementation infrastructure for GPIO interfaces.
>   *
> @@ -1404,6 +1406,8 @@ int gpio_direction_input(unsigned gpio)
>  	status = chip->direction_input(chip, gpio);
>  	if (status == 0)
>  		clear_bit(FLAG_IS_OUT, &desc->flags);
> +
> +	trace_gpio_direction(chip->base + gpio, 1, status);
>  lose:
>  	return status;
>  fail:
> @@ -1457,6 +1461,8 @@ int gpio_direction_output(unsigned gpio, int value)
>  	status = chip->direction_output(chip, gpio, value);
>  	if (status == 0)
>  		set_bit(FLAG_IS_OUT, &desc->flags);
> +	trace_gpio_value(chip->base + gpio, 0, value);
> +	trace_gpio_direction(chip->base + gpio, 0, status);
>  lose:
>  	return status;
>  fail:
> @@ -1546,10 +1552,13 @@ EXPORT_SYMBOL_GPL(gpio_set_debounce);
>  int __gpio_get_value(unsigned gpio)
>  {
>  	struct gpio_chip	*chip;
> +	int value;
>  
>  	chip = gpio_to_chip(gpio);
>  	WARN_ON(chip->can_sleep);
> -	return chip->get ? chip->get(chip, gpio - chip->base) : 0;
> +	value = chip->get ? chip->get(chip, gpio - chip->base) : 0;
> +	trace_gpio_value(gpio, 1, value);
> +	return value;
>  }
>  EXPORT_SYMBOL_GPL(__gpio_get_value);
>  
> @@ -1568,6 +1577,7 @@ void __gpio_set_value(unsigned gpio, int value)
>  
>  	chip = gpio_to_chip(gpio);
>  	WARN_ON(chip->can_sleep);
> +	trace_gpio_value(gpio, 0, value);
>  	chip->set(chip, gpio - chip->base, value);
>  }
>  EXPORT_SYMBOL_GPL(__gpio_set_value);
> @@ -1618,10 +1628,13 @@ EXPORT_SYMBOL_GPL(__gpio_to_irq);
>  int gpio_get_value_cansleep(unsigned gpio)
>  {
>  	struct gpio_chip	*chip;
> +	int value;
>  
>  	might_sleep_if(extra_checks);
>  	chip = gpio_to_chip(gpio);
> -	return chip->get ? chip->get(chip, gpio - chip->base) : 0;
> +	value = chip->get ? chip->get(chip, gpio - chip->base) : 0;
> +	trace_gpio_value(gpio, 1, value);
> +	return value;
>  }
>  EXPORT_SYMBOL_GPL(gpio_get_value_cansleep);
>  
> @@ -1631,6 +1644,7 @@ void gpio_set_value_cansleep(unsigned gpio, int value)
>  
>  	might_sleep_if(extra_checks);
>  	chip = gpio_to_chip(gpio);
> +	trace_gpio_value(gpio, 0, value);
>  	chip->set(chip, gpio - chip->base, value);
>  }
>  EXPORT_SYMBOL_GPL(gpio_set_value_cansleep);
> diff --git a/include/trace/events/gpio.h b/include/trace/events/gpio.h
> new file mode 100644
> index 0000000..927a8ad
> --- /dev/null
> +++ b/include/trace/events/gpio.h
> @@ -0,0 +1,56 @@
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM gpio
> +
> +#if !defined(_TRACE_GPIO_H) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_GPIO_H
> +
> +#include <linux/tracepoint.h>
> +
> +TRACE_EVENT(gpio_direction,
> +
> +	TP_PROTO(unsigned gpio, int in, int err),
> +
> +	TP_ARGS(gpio, in, err),
> +
> +	TP_STRUCT__entry(
> +		__field(unsigned, gpio)
> +		__field(int, in)
> +		__field(int, err)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->gpio = gpio;
> +		__entry->in = in;
> +		__entry->err = err;
> +	),
> +
> +	TP_printk("%u %3s (%d)", __entry->gpio,
> +		__entry->in ? "in" : "out", __entry->err)
> +);
> +
> +TRACE_EVENT(gpio_value,
> +
> +	TP_PROTO(unsigned gpio, int get, int value),
> +
> +	TP_ARGS(gpio, get, value),
> +
> +	TP_STRUCT__entry(
> +		__field(unsigned, gpio)
> +		__field(int, get)
> +		__field(int, value)
> +	),
> +
> +	TP_fast_assign(
> +		__entry->gpio = gpio;
> +		__entry->get = get;
> +		__entry->value = value;
> +	),
> +
> +	TP_printk("%u %3s %d", __entry->gpio,
> +		__entry->get ? "get" : "set", __entry->value)
> +);
> +
> +#endif /* if !defined(_TRACE_GPIO_H) || defined(TRACE_HEADER_MULTI_READ) */
> +
> +/* This part must be outside protection */
> +#include <trace/define_trace.h>
> -- 
> 1.7.2.3
>