lkml.org 
[lkml]   [2016]   [Jan]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [RFC PATCH 1/2] irq: Add a framework to measure interrupt timings
Date


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

\
 
 \ /
  Last update: 2016-01-12 13:21    [W:0.481 / U:0.108 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site