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

[Lina Iyer]

On Fri, Feb 02 2018 at 16:32 +0000, Steven Rostedt wrote:
> On Fri,  2 Feb 2018 07:22:00 -0700
> Lina Iyer <ilina@xxxxxxxxxxxxxx> wrote:
>
> Hi Lina,
>
> This looks really good. I have one nit below.
>
>
> > From: Archana Sathyakumar <asathyak@xxxxxxxxxxxxxx>
> >
> > Log key PDC pin configuration in FTRACE.
> >
> > Cc: Steven Rostedt <rostedt@xxxxxxxxxxx>
> > Signed-off-by: Archana Sathyakumar <asathyak@xxxxxxxxxxxxxx>
> > Signed-off-by: Lina Iyer <ilina@xxxxxxxxxxxxxx>
> > ---
> >  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 <linux/platform_device.h>
> >  #include <linux/slab.h>
> >  #include <linux/types.h>
> > +#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(&pdc_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