RE: [PATCH] powerpc/mpic: supply a .disable callback
-Original Message- From: Wood Scott-B07421 Sent: Tuesday, January 07, 2014 2:39 PM To: Wang Dongsheng-B40534 Cc: b...@kernel.crashing.org; linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH] powerpc/mpic: supply a .disable callback On Tue, 2014-01-07 at 13:38 +0800, Dongsheng Wang wrote: From: Wang Dongsheng dongsheng.w...@freescale.com Why did you change the author field? :(. I forgot that, I added this code, not use git. Sorry about that, I'll change back after our discussion -Dongsheng -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
RE: [PATCH] powerpc/mpic: supply a .disable callback
-Original Message- From: Benjamin Herrenschmidt [mailto:b...@kernel.crashing.org] Sent: Tuesday, January 07, 2014 1:50 PM To: Wang Dongsheng-B40534 Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH] powerpc/mpic: supply a .disable callback On Tue, 2014-01-07 at 13:38 +0800, Dongsheng Wang wrote: From: Wang Dongsheng dongsheng.w...@freescale.com Currently MPIC provides .mask, but not .disable. This means that effectively disable_irq() soft-disables the interrupt, and you get a .mask call if an interrupt actually occurs. I'm not sure if this was intended as a performance benefit (it seems common to omit .disable on powerpc interrupt controllers, but nowhere else), but it interacts badly with threaded/workqueue interrupts (including KVM reflection). In such cases, where the real interrupt handler does a disable_irq_nosync(), schedules defered handling, and returns, we get two interrupts for every real interrupt. The second interrupt does nothing but see that IRQ_DISABLED is set, and decide that it would be a good idea to actually call .mask. We probably don't want to do that for edge, only level interrupts. Sorry Ben, I am not understand your comments. This issue is the kernel api irq_disable() only use chip-irq_disable(), but mpic not have this interface so we don't real disable the interrupt. -Dongsheng Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/mpic: supply a .disable callback
On Tue, 2014-01-07 at 04:18 -0600, Wang Dongsheng-B40534 wrote: -Original Message- From: Benjamin Herrenschmidt [mailto:b...@kernel.crashing.org] Sent: Tuesday, January 07, 2014 1:50 PM To: Wang Dongsheng-B40534 Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH] powerpc/mpic: supply a .disable callback On Tue, 2014-01-07 at 13:38 +0800, Dongsheng Wang wrote: From: Wang Dongsheng dongsheng.w...@freescale.com Currently MPIC provides .mask, but not .disable. This means that effectively disable_irq() soft-disables the interrupt, and you get a .mask call if an interrupt actually occurs. I'm not sure if this was intended as a performance benefit (it seems common to omit .disable on powerpc interrupt controllers, but nowhere else), but it interacts badly with threaded/workqueue interrupts (including KVM reflection). In such cases, where the real interrupt handler does a disable_irq_nosync(), schedules defered handling, and returns, we get two interrupts for every real interrupt. The second interrupt does nothing but see that IRQ_DISABLED is set, and decide that it would be a good idea to actually call .mask. We probably don't want to do that for edge, only level interrupts. Sorry Ben, I am not understand your comments. This issue is the kernel api irq_disable() only use chip-irq_disable(), but mpic not have this interface so we don't real disable the interrupt. I think he means that the two interrupts for every real interrupt effect will only happen with level triggered interrupts, and he'd like to keep the potential performance benefit of lazy disabling for edge interrupts. To implement this for ordinary edge interrupts (not IPI, timer, etc) we'd need to add a new .irq_disable() function that checks whether it's level/edge and only calls .irq_mask() if level -- or, introduce a separate struct irq_chip for edge versus level. -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/mpic: supply a .disable callback
On Tue, 2014-01-07 at 10:18 +, dongsheng.w...@freescale.com wrote: -Original Message- From: Benjamin Herrenschmidt [mailto:b...@kernel.crashing.org] Sent: Tuesday, January 07, 2014 1:50 PM To: Wang Dongsheng-B40534 Cc: Wood Scott-B07421; linuxppc-dev@lists.ozlabs.org Subject: Re: [PATCH] powerpc/mpic: supply a .disable callback On Tue, 2014-01-07 at 13:38 +0800, Dongsheng Wang wrote: From: Wang Dongsheng dongsheng.w...@freescale.com Currently MPIC provides .mask, but not .disable. This means that effectively disable_irq() soft-disables the interrupt, and you get a .mask call if an interrupt actually occurs. I'm not sure if this was intended as a performance benefit (it seems common to omit .disable on powerpc interrupt controllers, but nowhere else), but it interacts badly with threaded/workqueue interrupts (including KVM reflection). In such cases, where the real interrupt handler does a disable_irq_nosync(), schedules defered handling, and returns, we get two interrupts for every real interrupt. The second interrupt does nothing but see that IRQ_DISABLED is set, and decide that it would be a good idea to actually call .mask. We probably don't want to do that for edge, only level interrupts. Sorry Ben, I am not understand your comments. This issue is the kernel api irq_disable() only use chip-irq_disable(), but mpic not have this interface so we don't real disable the interrupt. Yes, because we want to keep the existing behaviour of lazy disable for edge interrupts. It's faster. Cheers, Ben. -Dongsheng Cheers, Ben. ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
[PATCH] powerpc/mpic: supply a .disable callback
From: Wang Dongsheng dongsheng.w...@freescale.com
Currently MPIC provides .mask, but not .disable. This means that
effectively disable_irq() soft-disables the interrupt, and you get
a .mask call if an interrupt actually occurs.
I'm not sure if this was intended as a performance benefit (it seems common
to omit .disable on powerpc interrupt controllers, but nowhere else), but it
interacts badly with threaded/workqueue interrupts (including KVM
reflection). In such cases, where the real interrupt handler does a
disable_irq_nosync(), schedules defered handling, and returns, we get two
interrupts for every real interrupt. The second interrupt does nothing
but see that IRQ_DISABLED is set, and decide that it would be a good
idea to actually call .mask.
Signed-off-by: Scott Wood scottw...@freescale.com
Signed-off-by: Wang Dongsheng dongsheng.w...@freescale.com
diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index 0e166ed..dd7564b 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c
@@ -975,6 +975,7 @@ void mpic_set_destination(unsigned int virq, unsigned int
cpuid)
}
static struct irq_chip mpic_irq_chip = {
+ .irq_disable= mpic_mask_irq,
.irq_mask = mpic_mask_irq,
.irq_unmask = mpic_unmask_irq,
.irq_eoi= mpic_end_irq,
@@ -984,6 +985,7 @@ static struct irq_chip mpic_irq_chip = {
#ifdef CONFIG_SMP
static struct irq_chip mpic_ipi_chip = {
+ .irq_disable= mpic_mask_ipi,
.irq_mask = mpic_mask_ipi,
.irq_unmask = mpic_unmask_ipi,
.irq_eoi= mpic_end_ipi,
@@ -991,6 +993,7 @@ static struct irq_chip mpic_ipi_chip = {
#endif /* CONFIG_SMP */
static struct irq_chip mpic_tm_chip = {
+ .irq_disable= mpic_mask_tm,
.irq_mask = mpic_mask_tm,
.irq_unmask = mpic_unmask_tm,
.irq_eoi= mpic_end_irq,
@@ -1001,6 +1004,7 @@ static struct irq_chip mpic_tm_chip = {
static struct irq_chip mpic_irq_ht_chip = {
.irq_startup= mpic_startup_ht_irq,
.irq_shutdown = mpic_shutdown_ht_irq,
+ .irq_disable= mpic_mask_irq,
.irq_mask = mpic_mask_irq,
.irq_unmask = mpic_unmask_ht_irq,
.irq_eoi= mpic_end_ht_irq,
--
1.8.5
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/mpic: supply a .disable callback
On Tue, 2014-01-07 at 13:38 +0800, Dongsheng Wang wrote:
From: Wang Dongsheng dongsheng.w...@freescale.com
Currently MPIC provides .mask, but not .disable. This means that
effectively disable_irq() soft-disables the interrupt, and you get
a .mask call if an interrupt actually occurs.
I'm not sure if this was intended as a performance benefit (it seems common
to omit .disable on powerpc interrupt controllers, but nowhere else), but it
interacts badly with threaded/workqueue interrupts (including KVM
reflection). In such cases, where the real interrupt handler does a
disable_irq_nosync(), schedules defered handling, and returns, we get two
interrupts for every real interrupt. The second interrupt does nothing
but see that IRQ_DISABLED is set, and decide that it would be a good
idea to actually call .mask.
We probably don't want to do that for edge, only level interrupts.
Cheers,
Ben.
Signed-off-by: Scott Wood scottw...@freescale.com
Signed-off-by: Wang Dongsheng dongsheng.w...@freescale.com
diff --git a/arch/powerpc/sysdev/mpic.c b/arch/powerpc/sysdev/mpic.c
index 0e166ed..dd7564b 100644
--- a/arch/powerpc/sysdev/mpic.c
+++ b/arch/powerpc/sysdev/mpic.c
@@ -975,6 +975,7 @@ void mpic_set_destination(unsigned int virq, unsigned int
cpuid)
}
static struct irq_chip mpic_irq_chip = {
+ .irq_disable= mpic_mask_irq,
.irq_mask = mpic_mask_irq,
.irq_unmask = mpic_unmask_irq,
.irq_eoi= mpic_end_irq,
@@ -984,6 +985,7 @@ static struct irq_chip mpic_irq_chip = {
#ifdef CONFIG_SMP
static struct irq_chip mpic_ipi_chip = {
+ .irq_disable= mpic_mask_ipi,
.irq_mask = mpic_mask_ipi,
.irq_unmask = mpic_unmask_ipi,
.irq_eoi= mpic_end_ipi,
@@ -991,6 +993,7 @@ static struct irq_chip mpic_ipi_chip = {
#endif /* CONFIG_SMP */
static struct irq_chip mpic_tm_chip = {
+ .irq_disable= mpic_mask_tm,
.irq_mask = mpic_mask_tm,
.irq_unmask = mpic_unmask_tm,
.irq_eoi= mpic_end_irq,
@@ -1001,6 +1004,7 @@ static struct irq_chip mpic_tm_chip = {
static struct irq_chip mpic_irq_ht_chip = {
.irq_startup= mpic_startup_ht_irq,
.irq_shutdown = mpic_shutdown_ht_irq,
+ .irq_disable= mpic_mask_irq,
.irq_mask = mpic_mask_irq,
.irq_unmask = mpic_unmask_ht_irq,
.irq_eoi= mpic_end_ht_irq,
___
Linuxppc-dev mailing list
Linuxppc-dev@lists.ozlabs.org
https://lists.ozlabs.org/listinfo/linuxppc-dev
Re: [PATCH] powerpc/mpic: supply a .disable callback
On Tue, 2014-01-07 at 13:38 +0800, Dongsheng Wang wrote: From: Wang Dongsheng dongsheng.w...@freescale.com Why did you change the author field? -Scott ___ Linuxppc-dev mailing list Linuxppc-dev@lists.ozlabs.org https://lists.ozlabs.org/listinfo/linuxppc-dev
