Re: [RFC PATCH 1/2] irq: Add a framework to measure interrupt timings

[Daniel Lezcano]

Hi Thomas,

thanks for taking some time to review the patches.

On 01/08/2016 04:31 PM, Thomas Gleixner wrote:
> On Wed, 6 Jan 2016, Daniel Lezcano wrote:
> > +#ifdef CONFIG_IRQ_TIMINGS
> > +/**
> > + * timing handler to be called when an interrupt happens
> > + */
> > +typedef void (*irqt_handler_t)(unsigned int, ktime_t, void *, void *);
> > +
> > +/**
> > + * struct irqtimings - per interrupt irq timings descriptor
> > + * @handler:   interrupt handler timings function
> > + * @data:      pointer to the private data to be passed to the handler
> > + * @timestamp: latest interruption occurence
>
> There is no timestamp member.
>
> > + */
> > +struct irqtimings {
> > +	irqt_handler_t	  handler;
> > +	void              *data;
>
> What's that data thingy for. The proposed user does not use it at all and I
> have no idea why any user would want it. All it does is provide another level
> of indirection in the hotpath.

Yes, I agree. I added this private_data field for future use in case it 
would be needed but it does not make sense now.

> > +} ____cacheline_internodealigned_in_smp;
>
> > +/**
> > + * struct irqt_ops - structure to be used by the subsystem to call the
> > + * register and unregister ops when an irq is setup or freed.
> > + * @setup: registering callback
> > + * @free: unregistering callback
> > + *
> > + * The callbacks assumes the lock is held on the irq desc
>
> Crap. It's called outside of the locked region and the proposed user locks the
> descriptor itself, but that's a different story.
>
> > +static inline void free_irq_timings(unsigned int irq, void *dev_id)
> > +{
> > +	;
>
> What's the purpose of this semicolon?

Bah, old habit. I will remove it.

> > +#ifdef CONFIG_IRQ_TIMINGS
> > +void handle_irqt_event(struct irqtimings *irqt, struct irqaction *action)
>
> static ?
>
> > +{
> > +	if (irqt)
>
> This want's to use a static key.

Ok, I will look at that. I already used static keys to disable a portion 
of code from sysfs but never this way.

> > +		irqt->handler(action->irq, ktime_get(),
> > +			      action->dev_id, irqt->data);
> > +}
> > +#else
> > +#define handle_irqt_event(a, b)
>
> static inline stub if at all.

ok.

> > +#ifdef CONFIG_IRQ_TIMINGS
> > +/*
> > + * Global variable, only used by accessor functions, currently only
> > + * one user is allowed ...
>
> That variable is static not global. And what the heck means:
>
> >                          ... and it is up to the caller to make sure to
> > + * setup the irq timings which are already setup.
>
> -ENOPARSE.

hmm , yes ... it is not clear :)

I should have say:

"... and it is up to the caller to register the irq timing callback for 
the interrupts which are already setup."

> > + */
> > +static struct irqtimings_ops *irqtimings_ops;
> > +
> > +/**
> > + * register_irq_timings - register the ops when an irq is setup or freed
> > + *
> > + * @ops: the register/unregister ops to be called when at setup or
> > + * free time
> > + *
> > + * Returns -EBUSY if the slot is already in use, zero on success.
> > + */
> > +int register_irq_timings(struct irqtimings_ops *ops)
> > +{
> > +	if (irqtimings_ops)
> > +		return -EBUSY;
> > +
> > +	irqtimings_ops = ops;
> > +
> > +	return 0;
> > +}
> > +
> > +/**
> > + * setup_irq_timings - call the timing register callback
> > + *
> > + * @desc: an irq desc structure
>
> The argument list tells a different story.
>
> > + *
> > + * Returns -EINVAL in case of error, zero on success.
> > + */
> > +int setup_irq_timings(unsigned int irq, struct irqaction *act)
>
> static is not in your book, right? These functions are only used in this file,
> so no point for having them global visible and the stubs should be local as
> well.

Ok.

> > +{
> > +	if (irqtimings_ops && irqtimings_ops->setup)
> > +		return irqtimings_ops->setup(irq, act);
> > +	return 0;
> > +}
>
> ...
>
> > @@ -1408,6 +1469,8 @@ static struct irqaction *__free_irq(unsigned int irq, void *dev_id)
> >
> >   	unregister_handler_proc(irq, action);
> >
> > +	free_irq_timings(irq, dev_id);
>
> This needs to go to the point where the interrupt is already synchronized and
> the action about to be destroyed.

Ok, noted.

Thanks !

  -- Daniel


--
 <http://www.linaro.org/> Linaro.org â Open source software for ARM SoCs

Follow Linaro:  <http://www.facebook.com/pages/Linaro> Facebook |
<http://twitter.com/#!/linaroorg> Twitter |
<http://www.linaro.org/linaro-blog/> Blog