From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S933383AbcALLmV (ORCPT ); Tue, 12 Jan 2016 06:42:21 -0500 Received: from mail-wm0-f51.google.com ([74.125.82.51]:34888 "EHLO mail-wm0-f51.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758653AbcALLmS (ORCPT ); Tue, 12 Jan 2016 06:42:18 -0500 From: Daniel Lezcano Subject: Re: [RFC PATCH 1/2] irq: Add a framework to measure interrupt timings To: Thomas Gleixner References: <1452093774-17831-1-git-send-email-daniel.lezcano@linaro.org> <1452093774-17831-2-git-send-email-daniel.lezcano@linaro.org> Cc: peterz@infradead.org, rafael@kernel.org, linux-pm@vger.kernel.org, linux-kernel@vger.kernel.org, nicolas.pitre@linaro.org, vincent.guittot@linaro.org Message-ID: <5694E697.6020306@linaro.org> Date: Tue, 12 Jan 2016 12:42:15 +0100 User-Agent: Mozilla/5.0 (X11; Linux i686; rv:38.0) Gecko/20100101 Thunderbird/38.3.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.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 -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog