Re: [PATCH v3 04/18] irqchip: add nps Internal and external irqchips
On Tuesday 01 December 2015 06:59 PM, Marc Zyngier wrote:
>> +static int nps400_irq_map(struct irq_domain *d, unsigned int irq,
>> > +irq_hw_number_t hw)
>> > +{
>> > + switch (irq) {
>> > + case TIMER0_IRQ:
>> > +#if defined(CONFIG_SMP)
>> > + case IPI_IRQ:
>> > +#endif
>> > + irq_set_chip_and_handler(irq, &nps400_irq_chip_percpu,
>> > + handle_percpu_irq);
>> > + break;
>> > + default:
>> > + irq_set_chip_and_handler(irq, &nps400_irq_chip_fasteoi,
>> > + handle_fasteoi_irq);
>> > + break;
>> > + }
> No. This is just wrong. Either you get per interrupt information from
> the device tree to configure the interrupt the right way, or you have
> different interrupt controllers for each device.
>
> But using the Linux irq number is always wrong. You should only consider
> the hwirq.
The source is this incorrectness is ARC core intc code which also does the same
thing and we get away with it because of the legacy domain usage.
I'll fix that up.
Thx,
-Vineet
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH 0/3] ARC perf interrpt fixes
Hi Thomas, Marc, Peter Fixed a bunch of perf interrupt issues in ARC port. Apologies for extraneous CC. I think I'm doing the right thing here, and things start to work after these patches, but still running by domain experts to keep them in loop :-) Thx, -Vineet Vineet Gupta (3): ARCv2: intc: Fix random perf irq disabling in SMP setup ARC: intc: No need to clear IRQ_NOAUTOEN ARCv2: perf: Ensure perf intr gets enabled on all cores arch/arc/kernel/intc-arcv2.c | 15 +-- arch/arc/kernel/irq.c| 13 - arch/arc/kernel/perf_event.c | 32 +--- 3 files changed, 30 insertions(+), 30 deletions(-) -- 1.9.1 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH 1/3] ARCv2: intc: Fix random perf irq disabling in SMP setup
As part of fixing another perf issue, observed that after a perf run,
the interrupt got disabled on one/more cores.
| [ARCLinux]# cat /proc/interrupts | grep perf
| 20:0 0 0 0 ARCv2 core Intc 20 ARC perf counters
|
| [ARCLinux]# perf record -c 2 /sbin/hackbench
| Running with 10*40 (== 400) tasks.
|
| [ARCLinux]# cat /proc/interrupts | grep perf
| 20:0522 851916 ARCv2 core Intc 20 ARC perf counters
|
| [ARCLinux]# perf record -c 2 /sbin/hackbench
| Running with 10*40 (== 400) tasks.
|
| [ARCLinux]# cat /proc/interrupts | grep perf
| 20:0522 8 104368 ARCv2 core Intc 20 ARC perf counters
Turns out that despite requesting perf irq as percpu, the flow handler
registered was not handle_percpu_irq()
Given that on ARCv2 cores, IRQs < 24 are always private to cpu, we
register the right handler at the very onset.
Cc: Marc Zyngier
Cc: Thomas Gleixner
Cc: Peter Zijlstra
Cc: Alexey Brodkin
Cc: sta...@vger.kernel.org #4.2+
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Vineet Gupta
---
arch/arc/kernel/intc-arcv2.c | 15 +--
1 file changed, 13 insertions(+), 2 deletions(-)
diff --git a/arch/arc/kernel/intc-arcv2.c b/arch/arc/kernel/intc-arcv2.c
index 26c156827479..0394f9f61b46 100644
--- a/arch/arc/kernel/intc-arcv2.c
+++ b/arch/arc/kernel/intc-arcv2.c
@@ -106,10 +106,21 @@ static struct irq_chip arcv2_irq_chip = {
static int arcv2_irq_map(struct irq_domain *d, unsigned int irq,
irq_hw_number_t hw)
{
- if (irq == TIMER0_IRQ || irq == IPI_IRQ)
+ /*
+* core intc IRQs [16, 23]:
+* Statically assigned always private-per-core (Timers, WDT, IPI, PCT)
+*/
+ if (hw < 24) {
+ /*
+* A subsequent request_percpu_irq() fails if percpu_devid is
+* not set. That in turns sets NOAUTOEN, meaning each core needs
+* to call enable_percpu_irq()
+*/
+ irq_set_percpu_devid(irq);
irq_set_chip_and_handler(irq, &arcv2_irq_chip,
handle_percpu_irq);
- else
+ } else {
irq_set_chip_and_handler(irq, &arcv2_irq_chip,
handle_level_irq);
+ }
return 0;
}
--
1.9.1
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH 2/3] ARC: intc: No need to clear IRQ_NOAUTOEN
arc_request_percpu_irq() is called by all cores to request/enable percpu
irq. It has some "prep" calls needed by genirq:
- setup percpu devid
- disable IRQ_NOAUTOEN
However given that enable_percpu_irq() is called enayways, latter can be
avoided.
We are now left with irq_set_percpu_devid() quirk and that too for
ARCompact builds only, since previous patch updated ARCv2 intc to do this
in the "right" place, i.e. irq map function.
By next release, this will ultimately be fixed for ARCompact as well.
Cc: Marc Zyngier
Cc: Thomas Gleixner
Cc: Alexey Brodkin
Cc: linux-snps-arc@lists.infradead.org
Cc: linux-ker...@vger.kernel.org
Signed-off-by: Vineet Gupta
---
arch/arc/kernel/irq.c | 13 -
1 file changed, 8 insertions(+), 5 deletions(-)
diff --git a/arch/arc/kernel/irq.c b/arch/arc/kernel/irq.c
index 2ee226546c6a..d736678d2724 100644
--- a/arch/arc/kernel/irq.c
+++ b/arch/arc/kernel/irq.c
@@ -60,14 +60,17 @@ void arc_request_percpu_irq(int irq, int cpu,
if (!cpu) {
int rc;
+#ifdef CONFIG_ISA_ARCOMPACT
/*
-* These 2 calls are essential to making percpu IRQ APIs work
-* Ideally these details could be hidden in irq chip map
function
-* but the issue is IPIs IRQs being static (non-DT) and platform
-* specific, so we can't identify them there.
+* A subsequent request_percpu_irq() fails if percpu_devid is
+* not set. That in turns sets NOAUTOEN, meaning each core needs
+* to call enable_percpu_irq()
+*
+* For ARCv2, this is done in irq map function since we know
+* which irqs are strictly per cpu
*/
irq_set_percpu_devid(irq);
- irq_modify_status(irq, IRQ_NOAUTOEN, 0); /* @irq, @clr, @set */
+#endif
rc = request_percpu_irq(irq, isr, irq_nm, percpu_dev);
if (rc)
--
1.9.1
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
[PATCH 3/3] ARCv2: perf: Ensure perf intr gets enabled on all cores
This was the second perf intr issue
perf sampling on multicore requires intr to be enabled on all cores.
ARC perf probe code used helper arc_request_percpu_irq() which calls
- request_percpu_irq() on core0
- enable_percpu_irq() on all all cores (including core0)
genirq requires that request be made ahead of enable call.
However if perf probe happened on non core0 (observed on a 3.18 kernel),
enable would get called ahead of request, failing obviously and
rendering perf intr disabled on all such cores
[ 11.12] 1 ARC perf : 8 counters (48 bits), 113 conditions,
[overflow IRQ support]
[ 11.13] 1 -> enable_percpu_irq() IRQ 20 failed
[ 11.14] 3 -> enable_percpu_irq() IRQ 20 failed
[ 11.14] 2 -> enable_percpu_irq() IRQ 20 failed
[ 11.14] 0 => request_percpu_irq() IRQ 20
[ 11.14] 0 -> enable_percpu_irq() IRQ 20
Fix this fragility, by calling request_percpu_irq() on whatever core
calls probe (there is no requirement on which core calls this anyways)
and then calling enable on each cores.
Interestingly this started as invesigation of STAR 9000838902:
"sporadically IRQs enabled on perf prob"
which was about occassional boot spew as request_percpu_irq got called
non-locally (from an IPI), and re-enabled interrupts in following path
proc_mkdir -> spin_unlock_irq()
which the irq work code didn't like.
| ARC perf : 8 counters (48 bits), 113 conditions, [overflow IRQ support]
|
| BUG: failure at ../kernel/irq_work.c:135/irq_work_run_list()!
| CPU: 0 PID: 0 Comm: swapper/0 Not tainted 3.18.10-01127-g285efb8e66d1 #2
|
| Stack Trace:
| arc_unwind_core.constprop.1+0x94/0x104
| dump_stack+0x62/0x98
| irq_work_run_list+0xb0/0xb4
| irq_work_run+0x22/0x3c
| do_IPI+0x74/0x9c
| handle_irq_event_percpu+0x34/0x164
| handle_percpu_irq+0x58/0x78
| generic_handle_irq+0x1e/0x2c
| arch_do_IRQ+0x3c/0x60
| ret_from_exception+0x0/0x8
Cc: Marc Zyngier
Cc: Peter Zijlstra
Cc: Thomas Gleixner
Cc: linux-snps-arc@lists.infradead.org
Cc: linux-ker...@vger.kernel.org
Cc: Alexey Brodkin
Signed-off-by: Vineet Gupta
---
arch/arc/kernel/perf_event.c | 32 +---
1 file changed, 9 insertions(+), 23 deletions(-)
diff --git a/arch/arc/kernel/perf_event.c b/arch/arc/kernel/perf_event.c
index 0c08bb1ce15a..8b134cfe5e1f 100644
--- a/arch/arc/kernel/perf_event.c
+++ b/arch/arc/kernel/perf_event.c
@@ -428,12 +428,11 @@ static irqreturn_t arc_pmu_intr(int irq, void *dev)
#endif /* CONFIG_ISA_ARCV2 */
-void arc_cpu_pmu_irq_init(void)
+static void arc_cpu_pmu_irq_init(void *data)
{
- struct arc_pmu_cpu *pmu_cpu = this_cpu_ptr(&arc_pmu_cpu);
+ int irq = *(int *)data;
- arc_request_percpu_irq(arc_pmu->irq, smp_processor_id(), arc_pmu_intr,
- "ARC perf counters", pmu_cpu);
+ enable_percpu_irq(irq, IRQ_TYPE_NONE);
/* Clear all pending interrupt flags */
write_aux_reg(ARC_REG_PCT_INT_ACT, 0x);
@@ -515,7 +514,6 @@ static int arc_pmu_device_probe(struct platform_device
*pdev)
if (has_interrupts) {
int irq = platform_get_irq(pdev, 0);
- unsigned long flags;
if (irq < 0) {
pr_err("Cannot get IRQ number for the platform\n");
@@ -524,24 +522,12 @@ static int arc_pmu_device_probe(struct platform_device
*pdev)
arc_pmu->irq = irq;
- /*
-* arc_cpu_pmu_irq_init() needs to be called on all cores for
-* their respective local PMU.
-* However we use opencoded on_each_cpu() to ensure it is called
-* on core0 first, so that arc_request_percpu_irq() sets up
-* AUTOEN etc. Otherwise enable_percpu_irq() fails to enable
-* perf IRQ on non master cores.
-* see arc_request_percpu_irq()
-*/
- preempt_disable();
- local_irq_save(flags);
- arc_cpu_pmu_irq_init();
- local_irq_restore(flags);
- smp_call_function((smp_call_func_t)arc_cpu_pmu_irq_init, 0, 1);
- preempt_enable();
-
- /* Clean all pending interrupt flags */
- write_aux_reg(ARC_REG_PCT_INT_ACT, 0x);
+ /* intc map function ensures irq_set_percpu_devid() called */
+ request_percpu_irq(irq, arc_pmu_intr, "ARC perf counters",
+ this_cpu_ptr(&arc_pmu_cpu));
+
+ on_each_cpu(arc_cpu_pmu_irq_init, &irq, 1);
+
} else
arc_pmu->pmu.capabilities |= PERF_PMU_CAP_NO_INTERRUPT;
--
1.9.1
___
linux-snps-arc mailing list
linux-snps-arc@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: percpu irq APIs and perf
On Fri, 11 Dec 2015 05:26:02 + Vineet Gupta wrote: Hi Vineet, > Hi Marc, > > On Thursday 10 December 2015 03:26 PM, Marc Zyngier wrote: > > Hi Vinnet, > > > > On 10/12/15 09:25, Vineet Gupta wrote: > >> Hi Marc / Daniel / Jason, > >> > >> I had a couple of questions about percpu irq API, hopefully you can help > >> answer. > >> > >> On ARM, how do u handle requesting per cpu IRQs - specifically usage > >> of request_percpu_irq() / enable_percpu_irq() API. > >> It seems, for using them, we obviously need to explicitly set irq as > >> percpu and as a consequence explicitly enable autoen (since former > >> disables that). See arch/arc/kernel/irq.c: arc_request_percpu_irq() > >> called by ARC per cpu timer setup. > > Indeed. The interrupt controller code flags these interrupts as being > > per-cpu, and we do rely on each CPU performing an enable_percpu_irq(). > > > > So the way the whole thing flows is as such: > > - Interrupt controller (GIC) flags the PPIs (Private Peripheral > > Interrupt) as per-CPU (hwirq 16 to 31 are replicated per CPU) very early > > in the boot process > > Thx for your reply and the pointers > > irq-gic.c seems to be doing > >irq_set_status_flags(irq, IRQ_NOAUTOEN); > > So it is setting NOAUTOEN, which is already the case per side effect of > irq_set_percpu_devid(). No harm in making it explicit. Indeed, this looks completely superfluous. I'll fix that. > So this will make __setup_irq() skip irq_startup() and instead rely on > enable_precpu_irq() to be called even for the local core. > > 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. 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. I agree that the API is probably not the ideal one, but there is HW constraints that we cannot just ignore. > (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. > 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. > Do let me know what you think and I can send RFC patches to same effect. If you can find an elegant way to do this and keep the existing semantics, I'm all ear! Thanks, M. -- Jazz is not dead. It just smells funny. ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: percpu irq APIs and perf
Hi Marc, On Friday 11 December 2015 04:53 PM, Marc Zyngier wrote: > On Fri, 11 Dec 2015 05:26:02 + >> 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 ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
Re: percpu irq APIs and perf
Vineet, On 11/12/15 12:20, Vineet Gupta wrote: > Hi Marc, > > On Friday 11 December 2015 04:53 PM, Marc Zyngier wrote: >> On Fri, 11 Dec 2015 05:26:02 + >>> 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. It actually works the other way around. The caller cannot find out about the per-cpu property of the interrupt just by looking at the virtual IRQ number. It needs to ask the core code about it, and that's why the GIC tags these interrupts as per-cpu. >> 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. So assuming we can do this (forgetting about any form of HW limitation): CPU0 request the per-CPU IRQ with an AUTOEN flag. What happens on CPU1? Are you expecting it to immediately be able to take interrupts? What handler data gets passed to it? Thanks, M. -- Jazz is not dead. It just smells funny... ___ linux-snps-arc mailing list linux-snps-arc@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-snps-arc
