Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755007AbbLKMUo (ORCPT ); Fri, 11 Dec 2015 07:20:44 -0500 Received: from smtprelay.synopsys.com ([198.182.47.9]:53708 "EHLO smtprelay.synopsys.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754879AbbLKMUm (ORCPT ); Fri, 11 Dec 2015 07:20:42 -0500 Subject: Re: percpu irq APIs and perf To: Marc Zyngier References: <56694C0E.5050707@arm.com> <566AB24C.3040407@arm.com> CC: Daniel Lezcano , Jason Cooper , Peter Zijlstra , "Thomas Gleixner" , arcml , lkml , "maxime.ripard@free-electrons.com" Newsgroups: gmane.linux.kernel,gmane.linux.kernel.arc From: Vineet Gupta Message-ID: <566ABF86.9030308@synopsys.com> Date: Fri, 11 Dec 2015 17:50:22 +0530 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.4.0 MIME-Version: 1.0 In-Reply-To: <566AB24C.3040407@arm.com> Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: 7bit X-Originating-IP: [10.12.197.182] Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3400 Lines: 87 Hi Marc, On Friday 11 December 2015 04:53 PM, Marc Zyngier wrote: > On Fri, 11 Dec 2015 05:26:02 +0000 >> I think we can make percpu irq API a bit easier to use. >> >> (1) First thing which request_percpu_irq() does is check for >> irq_settings_is_per_cpu_devid(). Thus irq_set_percpu_devid() can be built into the >> API itself eliding the need to set it apriori. > > I don't think we can. At least in the case I'm concerned about (GIC's > PPIs), this is a hardware requirement. You cannot turn a global > interrupt into a per-CPU one, nor the other way around. Understood. > We also have > drivers (at least our PMUs) that do test the state of that interrupt > (per-CPU or not) to find out how they should be requested. But they call request_percpu_irq() only after determining that irq is percpu. Otherwise they will call vanilla request_irq() e.g. drivers/perf/arm/arc_pmu.c Which means that request_percpu_irq() can safely assume that caller absolutely wants percpu semantics and hence do equivalent of irq_set_percpu_devid() internally - NO. I'm sure I'm missing something. > I agree that the API is probably not the ideal one, but there is HW > constraints that we cannot just ignore. The API is pretty nice :-) there are these quirks which I want to avoid. My naive'ity in this area of code fails me to see how the hardware constraint is coming into play. >> (2) It seems that disabling autoen by default for percpu irq makes sense as >> evident from drivers/net/ethernet/marvell/mvneta.c where users want to control >> this. However the comment there is misleading >> >> /* Even though the documentation says that request_percpu_irq >> * doesn't enable the interrupts automatically, it actually >> * does so on the local CPU. >> * >> * Make sure it's disabled. >> */ >> >> Either sme core code is clearing NOAUTOEN or calling enable_precpu_irq() making >> request_percpu_irq() enable it. > > If that's the case, this is a bug. Nobody should enable that interrupt > until the driver has chosen to do so. Perhaps Maxim can shed more light as this seems to be his comment. >> IMHO it makes more sense to make autoen explicit in the API. >> Perhaps introduce a API flavour, which takes the autoen as arg. >> It could take flags to make it more extensible / future safe but that will be an >> overkill I think. > > But auto-enabling cannot be done from a single CPU. It can only be done > from the core that is going to be delivered that interrupt. This > requires access to registers that are simply not available to other CPUs. I'm not talking about eliminating enable_percpu_irq() call from all cores and still getting the auto-enable semantics. What I mean is doing the equivalent of irq_set_status_flags(irq, IRQ_NOAUTOEN); from within request_percpu_irq_xxx() based on an additional arg (vs. doing it aprioiri outside). OTOH, thinking a bit more abt this, I think the current semantics of auto-disable w/o any arg is just fine. Most percpu irqs in general purpose drivers would want the auto-disable anyways. Only for core irws such as timer / IPI etc do we want auto-enable. Thx, -Vineet -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/