linux-kernel - Re: [RFC V2 1/2] irq: Add a framework to measure interrupt timings

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:	Thu, 21 Jan 2016 10:25:56 +0100
From:	Daniel Lezcano <daniel.lezcano@...aro.org>
To:	Thomas Gleixner <tglx@...utronix.de>
Cc:	peterz@...radead.org, rafael@...nel.org, linux-pm@...r.kernel.org,
	linux-kernel@...r.kernel.org, nicolas.pitre@...aro.org,
	vincent.guittot@...aro.org
Subject: Re: [RFC V2 1/2] irq: Add a framework to measure interrupt timings

On 01/20/2016 06:55 PM, Thomas Gleixner wrote:
> On Wed, 20 Jan 2016, Daniel Lezcano wrote:
>> +#ifdef CONFIG_IRQ_TIMINGS
>> +/**
>> + * struct irqt_ops - structure to be used by the subsystem to track
>> + *                   irq timings
>> + * @alloc:    called when an irqdesc is allocated
>> + * @free:     called when an irqdesc is free
>> + * @setup:    called when an irq is setup, this is called under lock
>> + * @remove:   called when an irq is removed
>> + * @handler:  called when an interrupt is handled
>> + */
>> +struct irqtimings_ops {
>> +	int (*alloc)(unsigned int);
>> +	void (*free)(unsigned int);
>> +	int (*setup)(unsigned int, struct irqaction *act);
>> +	void (*remove)(unsigned int, void *dev_id);
>> +	irqt_handler_t handler;
>> +};
>> +
>> +/**
>> + * This macro *must* be used by the subsystem interested by the irq
>> + * timing information.
>> + */
>> +#define DECLARE_IRQ_TIMINGS(__ops)				\
>> +	const struct irqtimings_ops *__irqtimings = __ops;
>> +#endif
>
>> @@ -20,6 +20,49 @@ extern bool noirqdebug;
>>
>>   extern struct irqaction chained_action;
>>
>> +#ifdef CONFIG_IRQ_TIMINGS
>> +
>> +extern const struct irqtimings_ops *__irqtimings;
>> +
>> +static inline int alloc_irqtiming(unsigned int irq)
>> +{
>> +	if (__irqtimings->alloc)
>> +		return __irqtimings->alloc(irq);
>
> I really have a hard time to understand that indirection. __irqtimings is
> statically allocated and compiled in. There can be only one user for this in
> the system ever and that user has all callbacks populated.
>
> Why can't you spare all that pointer muck and simply have:
>
> #ifdef CONFIG_IRQ_TIMINGS
> int irqtiming_alloc(usigned int irq);
> ....
> #else
> static int irqtiming_alloc(usigned int irq) { return 0; }
> ...
> #endif
>
> and implement those functions in your idle thingy?

Hi Thomas,

yes sure, I can do something simpler.

Just to be sure, do you suggest to put the function declaration in 
kernel/irq/internal.h and the function definition in 
kernel/sched/idle-sched.c ?

-- 
  <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