Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE

[Steven Rostedt]

On Mon, 5 Feb 2018 15:50:48 +
Lina Iyer  wrote:


> >diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> >index a392380eada6..7f177ad88713 100644
> >--- a/drivers/irqchip/qcom-pdc.c
> >+++ b/drivers/irqchip/qcom-pdc.c
> >@@ -26,6 +26,8 @@
> > #include 
> > #include 
> > #include 
> >+#define CREATE_TRACE_POINTS
> >+#include "trace/events/pdc.h"
> >  
> In addition to this PDC patch, there are a few drivers with FTRACE
> support coming for up QCOM SoCs. Would it make sense to open up a new
> sub-folder for SoC specific FTRACE like say,
> trace/events/soc/
> trace/events/soc/qcom/
> 
> What would be your recommendation?
> 
> At the very least, I am thinking of renaming this file to
> trace/events/qcom-pdc.h.

I'm thinking it makes more sense to create the trace.h file inside the
drivers/irqchip/ directory. xfs does this, as well as other locations.

The include/trace/events/ was more of a generic method, but when events
start getting more specific, they should be in their own directories.

See samples/trace_events/* on how to have the file locally (or look at
xfs, search for trace in fs/xfs/Makefile).

-- Steve

Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE

[Steven Rostedt]

On Mon, 5 Feb 2018 15:50:48 +
Lina Iyer  wrote:


> >diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> >index a392380eada6..7f177ad88713 100644
> >--- a/drivers/irqchip/qcom-pdc.c
> >+++ b/drivers/irqchip/qcom-pdc.c
> >@@ -26,6 +26,8 @@
> > #include 
> > #include 
> > #include 
> >+#define CREATE_TRACE_POINTS
> >+#include "trace/events/pdc.h"
> >  
> In addition to this PDC patch, there are a few drivers with FTRACE
> support coming for up QCOM SoCs. Would it make sense to open up a new
> sub-folder for SoC specific FTRACE like say,
> trace/events/soc/
> trace/events/soc/qcom/
> 
> What would be your recommendation?
> 
> At the very least, I am thinking of renaming this file to
> trace/events/qcom-pdc.h.

I'm thinking it makes more sense to create the trace.h file inside the
drivers/irqchip/ directory. xfs does this, as well as other locations.

The include/trace/events/ was more of a generic method, but when events
start getting more specific, they should be in their own directories.

See samples/trace_events/* on how to have the file locally (or look at
xfs, search for trace in fs/xfs/Makefile).

-- Steve

Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE

[Steven Rostedt]

On Mon, 5 Feb 2018 15:18:46 +
Lina Iyer  wrote:

> >Some product configurations disable debugfs. I will ask around if this
> >can be dropped.
> >  
> Memory dumps after a crash have support for FTRACE and it helps greatly
> on production issues. Hence the preference for FTRACE support.

Note, ftrace no longer depends on debugfs. It has its own tracefs file
system:

 # mount -t tracefs nodev /sys/kernel/tracing

-- Steve

Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE

[Steven Rostedt]

On Mon, 5 Feb 2018 15:18:46 +
Lina Iyer  wrote:

> >Some product configurations disable debugfs. I will ask around if this
> >can be dropped.
> >  
> Memory dumps after a crash have support for FTRACE and it helps greatly
> on production issues. Hence the preference for FTRACE support.

Note, ftrace no longer depends on debugfs. It has its own tracefs file
system:

 # mount -t tracefs nodev /sys/kernel/tracing

-- Steve

Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE

[Lina Iyer]

Hi Steve,

On Fri, Feb 02 2018 at 14:22 +, Lina Iyer wrote:

From: Archana Sathyakumar 

Log key PDC pin configuration in FTRACE.

Cc: Steven Rostedt 
Signed-off-by: Archana Sathyakumar 
Signed-off-by: Lina Iyer 
---
drivers/irqchip/qcom-pdc.c |  7 ++
include/trace/events/pdc.h | 55 ++
2 files changed, 62 insertions(+)
create mode 100644 include/trace/events/pdc.h

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index a392380eada6..7f177ad88713 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -26,6 +26,8 @@
#include 
#include 
#include 
+#define CREATE_TRACE_POINTS
+#include "trace/events/pdc.h"


In addition to this PDC patch, there are a few drivers with FTRACE
support coming for up QCOM SoCs. Would it make sense to open up a new
sub-folder for SoC specific FTRACE like say,
trace/events/soc/
trace/events/soc/qcom/

What would be your recommendation?

At the very least, I am thinking of renaming this file to
trace/events/qcom-pdc.h.

Thanks,
Lina

Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE

[Lina Iyer]

Hi Steve,

On Fri, Feb 02 2018 at 14:22 +, Lina Iyer wrote:

From: Archana Sathyakumar 

Log key PDC pin configuration in FTRACE.

Cc: Steven Rostedt 
Signed-off-by: Archana Sathyakumar 
Signed-off-by: Lina Iyer 
---
drivers/irqchip/qcom-pdc.c |  7 ++
include/trace/events/pdc.h | 55 ++
2 files changed, 62 insertions(+)
create mode 100644 include/trace/events/pdc.h

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index a392380eada6..7f177ad88713 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -26,6 +26,8 @@
#include 
#include 
#include 
+#define CREATE_TRACE_POINTS
+#include "trace/events/pdc.h"


In addition to this PDC patch, there are a few drivers with FTRACE
support coming for up QCOM SoCs. Would it make sense to open up a new
sub-folder for SoC specific FTRACE like say,
trace/events/soc/
trace/events/soc/qcom/

What would be your recommendation?

At the very least, I am thinking of renaming this file to
trace/events/qcom-pdc.h.

Thanks,
Lina

Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE

[Lina Iyer]

On Fri, Feb 02 2018 at 23:02 +, Lina Iyer wrote:

On Fri, Feb 02 2018 at 15:57 +, Thomas Gleixner wrote:

On Fri, 2 Feb 2018, Lina Iyer wrote:

+++ b/include/trace/events/pdc.h
@@ -0,0 +1,55 @@
+/* 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.


Can you please use the proper SPDX identifiers instead of the boiler plate?
Same for the driver source file.


Sure.


+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM pdc
+
+#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_PDC_H_
+
+#include 
+
+#define PDC_ENTRY  1
+#define PDC_TYPE_CONFIG2
+
+TRACE_EVENT(irq_pin_config,


This is really a too generic name for a PDC specific breakpoint.


Hmm.. right.


Aside of that the question is whether this really justifies a trace
point. Wouldn't it be sufficient to use the GENERIC_IRQ_DEBUGFS
infrastructure to make this accessible via debugfs?



Some product configurations disable debugfs. I will ask around if this
can be dropped.


Memory dumps after a crash have support for FTRACE and it helps greatly
on production issues. Hence the preference for FTRACE support.

-- Lina

Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE

[Lina Iyer]

On Fri, Feb 02 2018 at 23:02 +, Lina Iyer wrote:

On Fri, Feb 02 2018 at 15:57 +, Thomas Gleixner wrote:

On Fri, 2 Feb 2018, Lina Iyer wrote:

+++ b/include/trace/events/pdc.h
@@ -0,0 +1,55 @@
+/* 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.


Can you please use the proper SPDX identifiers instead of the boiler plate?
Same for the driver source file.


Sure.


+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM pdc
+
+#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_PDC_H_
+
+#include 
+
+#define PDC_ENTRY  1
+#define PDC_TYPE_CONFIG2
+
+TRACE_EVENT(irq_pin_config,


This is really a too generic name for a PDC specific breakpoint.


Hmm.. right.


Aside of that the question is whether this really justifies a trace
point. Wouldn't it be sufficient to use the GENERIC_IRQ_DEBUGFS
infrastructure to make this accessible via debugfs?



Some product configurations disable debugfs. I will ask around if this
can be dropped.


Memory dumps after a crash have support for FTRACE and it helps greatly
on production issues. Hence the preference for FTRACE support.

-- Lina

Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE

[Lina Iyer]

On Fri, Feb 02 2018 at 15:57 +, Thomas Gleixner wrote:

On Fri, 2 Feb 2018, Lina Iyer wrote:

+++ b/include/trace/events/pdc.h
@@ -0,0 +1,55 @@
+/* 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.


Can you please use the proper SPDX identifiers instead of the boiler plate?
Same for the driver source file.


Sure.


+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM pdc
+
+#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_PDC_H_
+
+#include 
+
+#define PDC_ENTRY  1
+#define PDC_TYPE_CONFIG2
+
+TRACE_EVENT(irq_pin_config,


This is really a too generic name for a PDC specific breakpoint.


Hmm.. right.


Aside of that the question is whether this really justifies a trace
point. Wouldn't it be sufficient to use the GENERIC_IRQ_DEBUGFS
infrastructure to make this accessible via debugfs?



Some product configurations disable debugfs. I will ask around if this
can be dropped.

-- Lina

Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE

[Lina Iyer]

On Fri, Feb 02 2018 at 15:57 +, Thomas Gleixner wrote:

On Fri, 2 Feb 2018, Lina Iyer wrote:

+++ b/include/trace/events/pdc.h
@@ -0,0 +1,55 @@
+/* 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.


Can you please use the proper SPDX identifiers instead of the boiler plate?
Same for the driver source file.


Sure.


+ */
+
+#undef TRACE_SYSTEM
+#define TRACE_SYSTEM pdc
+
+#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ)
+#define _TRACE_PDC_H_
+
+#include 
+
+#define PDC_ENTRY  1
+#define PDC_TYPE_CONFIG2
+
+TRACE_EVENT(irq_pin_config,


This is really a too generic name for a PDC specific breakpoint.


Hmm.. right.


Aside of that the question is whether this really justifies a trace
point. Wouldn't it be sufficient to use the GENERIC_IRQ_DEBUGFS
infrastructure to make this accessible via debugfs?



Some product configurations disable debugfs. I will ask around if this
can be dropped.

-- Lina

Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE

[Lina Iyer]

On Fri, Feb 02 2018 at 16:32 +, Steven Rostedt wrote:

On Fri,  2 Feb 2018 07:22:00 -0700
Lina Iyer  wrote:

Hi Lina,

This looks really good. I have one nit below.



From: Archana Sathyakumar 

Log key PDC pin configuration in FTRACE.

Cc: Steven Rostedt 
Signed-off-by: Archana Sathyakumar 
Signed-off-by: Lina Iyer 
---
 drivers/irqchip/qcom-pdc.c |  7 ++
 include/trace/events/pdc.h | 55 ++
 2 files changed, 62 insertions(+)
 create mode 100644 include/trace/events/pdc.h

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index a392380eada6..7f177ad88713 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -26,6 +26,8 @@
 #include 
 #include 
 #include 
+#define CREATE_TRACE_POINTS
+#include "trace/events/pdc.h"

 #define PDC_MAX_IRQS   126

@@ -68,6 +70,8 @@ static inline void pdc_enable_intr(struct irq_data *d, bool 
on)
enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask);
pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
spin_unlock_irqrestore(_lock, flags);
+
+   trace_irq_pin_config(PDC_ENTRY, pin_out, (u64)d->chip_data, 0, on);
 }

 static void qcom_pdc_gic_mask(struct irq_data *d)
@@ -149,6 +153,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, 
unsigned int type)

pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);

+   trace_irq_pin_config(PDC_TYPE_CONFIG, pin_out, (u64)d->chip_data,
+   pdc_type, 0);


I wonder if it makes more sense to just pass 'd' into the trace events,
and then do the dereference there. The reason is to try to get as much
code out of the calling path as possible. Even though trace events use
jump labels and have no conditional branches, the code to call the
function is still within the code using the trace events. By passing in
'd' and doing the redirect in the trace event code, we remove the
setting up of the redirect from the caller, and save some cache lines
in the process.

Makes sense. Will fix it.

Thanks Steve.

-- Lina

Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE

[Lina Iyer]

On Fri, Feb 02 2018 at 16:32 +, Steven Rostedt wrote:

On Fri,  2 Feb 2018 07:22:00 -0700
Lina Iyer  wrote:

Hi Lina,

This looks really good. I have one nit below.



From: Archana Sathyakumar 

Log key PDC pin configuration in FTRACE.

Cc: Steven Rostedt 
Signed-off-by: Archana Sathyakumar 
Signed-off-by: Lina Iyer 
---
 drivers/irqchip/qcom-pdc.c |  7 ++
 include/trace/events/pdc.h | 55 ++
 2 files changed, 62 insertions(+)
 create mode 100644 include/trace/events/pdc.h

diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
index a392380eada6..7f177ad88713 100644
--- a/drivers/irqchip/qcom-pdc.c
+++ b/drivers/irqchip/qcom-pdc.c
@@ -26,6 +26,8 @@
 #include 
 #include 
 #include 
+#define CREATE_TRACE_POINTS
+#include "trace/events/pdc.h"

 #define PDC_MAX_IRQS   126

@@ -68,6 +70,8 @@ static inline void pdc_enable_intr(struct irq_data *d, bool 
on)
enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask);
pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
spin_unlock_irqrestore(_lock, flags);
+
+   trace_irq_pin_config(PDC_ENTRY, pin_out, (u64)d->chip_data, 0, on);
 }

 static void qcom_pdc_gic_mask(struct irq_data *d)
@@ -149,6 +153,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, 
unsigned int type)

pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);

+   trace_irq_pin_config(PDC_TYPE_CONFIG, pin_out, (u64)d->chip_data,
+   pdc_type, 0);


I wonder if it makes more sense to just pass 'd' into the trace events,
and then do the dereference there. The reason is to try to get as much
code out of the calling path as possible. Even though trace events use
jump labels and have no conditional branches, the code to call the
function is still within the code using the trace events. By passing in
'd' and doing the redirect in the trace event code, we remove the
setting up of the redirect from the caller, and save some cache lines
in the process.

Makes sense. Will fix it.

Thanks Steve.

-- Lina

Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE

[Steven Rostedt]

On Fri,  2 Feb 2018 07:22:00 -0700
Lina Iyer  wrote:

Hi Lina,

This looks really good. I have one nit below.


> From: Archana Sathyakumar 
> 
> Log key PDC pin configuration in FTRACE.
> 
> Cc: Steven Rostedt 
> Signed-off-by: Archana Sathyakumar 
> Signed-off-by: Lina Iyer 
> ---
>  drivers/irqchip/qcom-pdc.c |  7 ++
>  include/trace/events/pdc.h | 55 
> ++
>  2 files changed, 62 insertions(+)
>  create mode 100644 include/trace/events/pdc.h
> 
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index a392380eada6..7f177ad88713 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -26,6 +26,8 @@
>  #include 
>  #include 
>  #include 
> +#define CREATE_TRACE_POINTS
> +#include "trace/events/pdc.h"
>  
>  #define PDC_MAX_IRQS 126
>  
> @@ -68,6 +70,8 @@ static inline void pdc_enable_intr(struct irq_data *d, bool 
> on)
>   enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask);
>   pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
>   spin_unlock_irqrestore(_lock, flags);
> +
> + trace_irq_pin_config(PDC_ENTRY, pin_out, (u64)d->chip_data, 0, on);
>  }
>  
>  static void qcom_pdc_gic_mask(struct irq_data *d)
> @@ -149,6 +153,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, 
> unsigned int type)
>  
>   pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
>  
> + trace_irq_pin_config(PDC_TYPE_CONFIG, pin_out, (u64)d->chip_data,
> + pdc_type, 0);

I wonder if it makes more sense to just pass 'd' into the trace events,
and then do the dereference there. The reason is to try to get as much
code out of the calling path as possible. Even though trace events use
jump labels and have no conditional branches, the code to call the
function is still within the code using the trace events. By passing in
'd' and doing the redirect in the trace event code, we remove the
setting up of the redirect from the caller, and save some cache lines
in the process.

-- Steve



> +
>   return irq_chip_set_type_parent(d, type);
>  }
>  
> diff --git a/include/trace/events/pdc.h b/include/trace/events/pdc.h
> new file mode 100644
> index ..0e894bbc9e85
> --- /dev/null
> +++ b/include/trace/events/pdc.h
> @@ -0,0 +1,55 @@
> +/* 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 
> +
> +#define PDC_ENTRY1
> +#define PDC_TYPE_CONFIG  2
> +
> +TRACE_EVENT(irq_pin_config,
> +
> + TP_PROTO(u32 func, int pin, u64 hwirq, u32 type, u32 enable),
> +
> + TP_ARGS(func, pin, hwirq, type, enable),
> +
> + TP_STRUCT__entry(
> + __field(u32, func)
> + __field(int, pin)
> + __field(u64, hwirq)
> + __field(u32, type)
> + __field(u32, enable)
> + ),
> +
> + TP_fast_assign(
> + __entry->func = func;
> + __entry->pin = pin;
> + __entry->hwirq = hwirq;
> + __entry->type = type;
> + __entry->enable = enable;
> + ),
> +
> + TP_printk("%s hwirq:%lu pin:%d type:%u enable:%u",
> + print_symbolic(__entry->func,
> + { PDC_ENTRY, "pdc_entry" },
> + { PDC_TYPE_CONFIG, "pdc_type_config" }),
> + __entry->pin, __entry->hwirq, __entry->type, __entry->enable)
> +);
> +
> +#endif
> +#define TRACE_INCLUDE_FILE pdc
> +#include 

Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE

[Steven Rostedt]

On Fri,  2 Feb 2018 07:22:00 -0700
Lina Iyer  wrote:

Hi Lina,

This looks really good. I have one nit below.


> From: Archana Sathyakumar 
> 
> Log key PDC pin configuration in FTRACE.
> 
> Cc: Steven Rostedt 
> Signed-off-by: Archana Sathyakumar 
> Signed-off-by: Lina Iyer 
> ---
>  drivers/irqchip/qcom-pdc.c |  7 ++
>  include/trace/events/pdc.h | 55 
> ++
>  2 files changed, 62 insertions(+)
>  create mode 100644 include/trace/events/pdc.h
> 
> diff --git a/drivers/irqchip/qcom-pdc.c b/drivers/irqchip/qcom-pdc.c
> index a392380eada6..7f177ad88713 100644
> --- a/drivers/irqchip/qcom-pdc.c
> +++ b/drivers/irqchip/qcom-pdc.c
> @@ -26,6 +26,8 @@
>  #include 
>  #include 
>  #include 
> +#define CREATE_TRACE_POINTS
> +#include "trace/events/pdc.h"
>  
>  #define PDC_MAX_IRQS 126
>  
> @@ -68,6 +70,8 @@ static inline void pdc_enable_intr(struct irq_data *d, bool 
> on)
>   enable = on ? ENABLE_INTR(enable, mask) : CLEAR_INTR(enable, mask);
>   pdc_reg_write(IRQ_ENABLE_BANK, index, enable);
>   spin_unlock_irqrestore(_lock, flags);
> +
> + trace_irq_pin_config(PDC_ENTRY, pin_out, (u64)d->chip_data, 0, on);
>  }
>  
>  static void qcom_pdc_gic_mask(struct irq_data *d)
> @@ -149,6 +153,9 @@ static int qcom_pdc_gic_set_type(struct irq_data *d, 
> unsigned int type)
>  
>   pdc_reg_write(IRQ_i_CFG, pin_out, pdc_type);
>  
> + trace_irq_pin_config(PDC_TYPE_CONFIG, pin_out, (u64)d->chip_data,
> + pdc_type, 0);

I wonder if it makes more sense to just pass 'd' into the trace events,
and then do the dereference there. The reason is to try to get as much
code out of the calling path as possible. Even though trace events use
jump labels and have no conditional branches, the code to call the
function is still within the code using the trace events. By passing in
'd' and doing the redirect in the trace event code, we remove the
setting up of the redirect from the caller, and save some cache lines
in the process.

-- Steve



> +
>   return irq_chip_set_type_parent(d, type);
>  }
>  
> diff --git a/include/trace/events/pdc.h b/include/trace/events/pdc.h
> new file mode 100644
> index ..0e894bbc9e85
> --- /dev/null
> +++ b/include/trace/events/pdc.h
> @@ -0,0 +1,55 @@
> +/* 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 
> +
> +#define PDC_ENTRY1
> +#define PDC_TYPE_CONFIG  2
> +
> +TRACE_EVENT(irq_pin_config,
> +
> + TP_PROTO(u32 func, int pin, u64 hwirq, u32 type, u32 enable),
> +
> + TP_ARGS(func, pin, hwirq, type, enable),
> +
> + TP_STRUCT__entry(
> + __field(u32, func)
> + __field(int, pin)
> + __field(u64, hwirq)
> + __field(u32, type)
> + __field(u32, enable)
> + ),
> +
> + TP_fast_assign(
> + __entry->func = func;
> + __entry->pin = pin;
> + __entry->hwirq = hwirq;
> + __entry->type = type;
> + __entry->enable = enable;
> + ),
> +
> + TP_printk("%s hwirq:%lu pin:%d type:%u enable:%u",
> + print_symbolic(__entry->func,
> + { PDC_ENTRY, "pdc_entry" },
> + { PDC_TYPE_CONFIG, "pdc_type_config" }),
> + __entry->pin, __entry->hwirq, __entry->type, __entry->enable)
> +);
> +
> +#endif
> +#define TRACE_INCLUDE_FILE pdc
> +#include 

Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE

[Thomas Gleixner]

On Fri, 2 Feb 2018, Lina Iyer wrote:
> +++ b/include/trace/events/pdc.h
> @@ -0,0 +1,55 @@
> +/* 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.

Can you please use the proper SPDX identifiers instead of the boiler plate?
Same for the driver source file.

> + */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM pdc
> +
> +#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_PDC_H_
> +
> +#include 
> +
> +#define PDC_ENTRY1
> +#define PDC_TYPE_CONFIG  2
> +
> +TRACE_EVENT(irq_pin_config,

This is really a too generic name for a PDC specific breakpoint.

Aside of that the question is whether this really justifies a trace
point. Wouldn't it be sufficient to use the GENERIC_IRQ_DEBUGFS
infrastructure to make this accessible via debugfs?

Thanks,

tglx

Re: [PATCH RFC v2 3/3] drivers: irqchip: pdc: log PDC info in FTRACE

[Thomas Gleixner]

On Fri, 2 Feb 2018, Lina Iyer wrote:
> +++ b/include/trace/events/pdc.h
> @@ -0,0 +1,55 @@
> +/* 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.

Can you please use the proper SPDX identifiers instead of the boiler plate?
Same for the driver source file.

> + */
> +
> +#undef TRACE_SYSTEM
> +#define TRACE_SYSTEM pdc
> +
> +#if !defined(_TRACE_PDC_) || defined(TRACE_HEADER_MULTI_READ)
> +#define _TRACE_PDC_H_
> +
> +#include 
> +
> +#define PDC_ENTRY1
> +#define PDC_TYPE_CONFIG  2
> +
> +TRACE_EVENT(irq_pin_config,

This is really a too generic name for a PDC specific breakpoint.

Aside of that the question is whether this really justifies a trace
point. Wouldn't it be sufficient to use the GENERIC_IRQ_DEBUGFS
infrastructure to make this accessible via debugfs?

Thanks,

tglx