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

[Daniel Lezcano <daniel.lezcano@linaro.org>]

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