Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756403AbdCWTVK (ORCPT ); Thu, 23 Mar 2017 15:21:10 -0400 Received: from mail-wm0-f46.google.com ([74.125.82.46]:35000 "EHLO mail-wm0-f46.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753764AbdCWTVF (ORCPT ); Thu, 23 Mar 2017 15:21:05 -0400 Date: Thu, 23 Mar 2017 20:19:53 +0100 From: Daniel Lezcano To: Mark Rutland Cc: tglx@linutronix.de, nicolas.pitre@linaro.org, linux-samsung-soc@vger.kernel.org, vincent.guittot@linaro.org, kernel@stlinux.com, kvm@vger.kernel.org, rafael@kernel.org, peterz@infradead.org, netdev@vger.kernel.org, linux-kernel@vger.kernel.org, xen-devel@lists.xenproject.org, linux-snps-arc@lists.infradead.org, kvmarm@lists.cs.columbia.edu, linux-arm-kernel@lists.infradead.org, will.deacon@arm.com, marc.zyngier@arm.com Subject: Re: [PATCH V8 1/3] irq: Add flags to request_percpu_irq function Message-ID: <20170323191953.GC24630@mai> References: <1490290924-12958-1-git-send-email-daniel.lezcano@linaro.org> <20170323185449.GA21359@leverpostej> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170323185449.GA21359@leverpostej> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3256 Lines: 99 Hi Mark, On Thu, Mar 23, 2017 at 06:54:52PM +0000, Mark Rutland wrote: > Hi Daniel, > > On Thu, Mar 23, 2017 at 06:42:01PM +0100, Daniel Lezcano wrote: > > In the next changes, we track the interrupts but we discard the timers as > > that does not make sense. The next interrupt on a timer is predictable. > > Sorry, but I could not parse this. I meant we are measuring when are happening the interrupts by getting the local clock in the interrupt handler. But if the interrupts are coming from a timer, it is not necessary to do that because we already know when they will occur. So, in order to sort out which interrupt we measure, we use the IRQF_TIMER flag. Unfortunately, this flag is missing when we do a request_percpu_irq. The purpose of this patch is to fix that. > > diff --git a/drivers/perf/arm_pmu.c b/drivers/perf/arm_pmu.c > > index 9612b84..0f5ab4a 100644 > > --- a/drivers/perf/arm_pmu.c > > +++ b/drivers/perf/arm_pmu.c > > @@ -661,7 +661,7 @@ static int cpu_pmu_request_irq(struct arm_pmu *cpu_pmu, irq_handler_t handler) > > > > irq = platform_get_irq(pmu_device, 0); > > if (irq > 0 && irq_is_percpu(irq)) { > > - err = request_percpu_irq(irq, handler, "arm-pmu", > > + err = request_percpu_irq(irq, 0, handler, "arm-pmu", > > &hw_events->percpu_pmu); > > if (err) { > > pr_err("unable to request IRQ%d for ARM PMU counters\n", > > Please Cc myself and Will Deacon when modifying the arm_pmu driver, as > per MAINTAINERS. I only spotted this patch by chance. Ah, ok, sorry for that. Thanks for spotting this, you should have been Cc'ed by my cccmd script. I will check that. > This conflicts with arm_pmu changes I have queued for v4.12 [1]. > > So, can we leave the prototype of request_percpu_irq() as-is? > > Why not add a new request_percpu_irq_flags() function, and leave > request_percpu_irq() as a wrapper for that? e.g. [ ... ] > static inline int > request_percpu_irq(unsigned int irq, irq_handler_t handler, > const char *devname, void __percpu *percpu_dev_id) > { > return request_percpu_irq_flags(irq, handler, devname, > percpu_dev_id, 0); > } > > ... that would avoid having to touch any non-timer driver for now. Mmh, yes. That's a good suggestion. > [...] > > > -request_percpu_irq(unsigned int irq, irq_handler_t handler, > > - const char *devname, void __percpu *percpu_dev_id); > > +request_percpu_irq(unsigned int irq, unsigned long flags, > > + irq_handler_t handler, const char *devname, > > + void __percpu *percpu_dev_id); > > > > Looking at request_irq, the prototype is: > > int __must_check > request_irq(unsigned int irq, irq_handler_t handler, > unsigned long flags, const char *name, > void *dev); > > ... surely it would be better to share the same argument order? i.e. > > int __must_check > request_percpu_irq(unsigned int irq, irq_handler_t handler, > unsigned long flags, const char *devname, > void __percpu *percpu_dev_id); > Agree. Thanks for the review. -- Daniel -- Linaro.org │ Open source software for ARM SoCs Follow Linaro: Facebook | Twitter | Blog