On Tue, Jan 23 2018 at 18:13 +0000, Steven Rostedt wrote:
On Tue, 23 Jan 2018 10:56:55 -0700
Lina Iyer <[email protected]> wrote:

From: Archana Sathyakumar <[email protected]>

Hi Lina and Archana,


Log key PDC pin configuration in FTRACE.

Cc: Steven Rostedt <[email protected]>
Signed-off-by: Archana Sathyakumar <[email protected]>
Signed-off-by: Lina Iyer <[email protected]>
---
 drivers/irqchip/qcom-pdc.c |  6 ++++++
 include/trace/events/pdc.h | 50 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 56 insertions(+)
 create mode 100644 include/trace/events/pdc.h

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index 57f1348bd81c..9b626e9f3a29 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -26,6 +26,8 @@
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/types.h>
+#define CREATE_TRACE_POINTS
+#include "trace/events/pdc.h"

 #include "qcom-pdc.h"

@@ -78,6 +80,7 @@ static inline void pdc_enable_intr(struct irq_data *d, bool 
on)
                        break;
                udelay(5);
        } while (1);
+       trace_irq_pin_config("enable", (u32)pin_out, (u32)d->hwirq, 0, on);

I'm concerned with that string you are passing in.

        spin_unlock_irqrestore(&pdc_lock, flags);
        WARN_ON(r_enable != enable);
 }
@@ -161,6 +164,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, 
unsigned int type)
                udelay(5);
        } while (1);

+       trace_irq_pin_config("type_config", (u32)pin_out, (u32)d->hwirq,

This one too.

+                               pdc_type, 0);
+
        /*
         * If type is edge triggered, forward that as Rising edge as PDC
         * takes care of converting falling edge to rising edge signal
diff --git a/include/trace/events/pdc.h b/include/trace/events/pdc.h
new file mode 100644
index 000000000000..bec54c414880
--- /dev/null
+++ b/include/trace/events/pdc.h
@@ -0,0 +1,50 @@
+/* Copyright (c) 2017-2018, The Linux Foundation. All rights reserved.
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 and
+ * only version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful,
+ * but WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+ * GNU General Public License for more details.
+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM pdc
+
+#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_PDC_H_
+
+#include <linux/tracepoint.h>
+
+TRACE_EVENT(irq_pin_config,
+
+       TP_PROTO(char *func, u32 pin, u32 hwirq, u32 type, u32 enable),
+
+       TP_ARGS(func, pin, hwirq, type, enable),
+
+       TP_STRUCT__entry(
+               __field(char *, func)
+               __field(u32, pin)
+               __field(u32, hwirq)
+               __field(u32, type)
+               __field(u32, enable)
+       ),
+
+       TP_fast_assign(
+               __entry->pin = pin;
+               __entry->func = func;
+               __entry->hwirq = hwirq;
+               __entry->type = type;
+               __entry->enable = enable;
+       ),
+
+       TP_printk("%s hwirq:%u pin:%u type:%u enable:%u",

%s is dereferencing a pointer, which works fine when reading the ASCII
translated trace files (/sys/kernel/debug/tracing/trace and friends).
But it breaks when we use trace-cmd or perf. Because they record the
binary data and they do not have access to memory inside the kernel.

What I would recommend is in pdc.h have:

#define PDC_ENTRY               1
#define PDC_TYPE_CONFIG         2

Then have the tracepoints do instead of:

        trace_irq_pin_config("enable", ...);
        trace_irq_pin_config("type_config", ...);

Have them do:

        trace_irq_pin_config(PDC_ENTRY, ...);
        trace_irq_pin_config(PDC_TYPE_CONFIG, ...);

And then have:

        TP_PROTO(u32 func, ...)

        TP_STRUCT__entry(
                __field(u32, func)
                [...]

        TP_fast_assign(
                __entry->func = func;
                [...]

        TP_printk("%s hwirq...",
                print_symbolic(__entry->func,
                                { PDC_ENTRY, "entry" },
                                { PDC_TYPE_CONFIG, "type_config" }),
                [...]

And you will get the same effect, with trace-cmd and perf still working
as expected.

Thanks Steve. I will take care of it in the next spin.

-- Lina

-- Steve


+               __entry->func, __entry->pin, __entry->hwirq, __entry->type,
+               __entry->enable)
+);
+
+#endif
+#define TRACE_INCLUDE_FILE pdc
+#include <trace/define_trace.h>

Reply via email to