[PATCH] powerpc/mpic: supply a .disable callback

[PATCH] powerpc/mpic: supply a .disable callback

[Dongsheng Wang]

From: Wang Dongsheng <dongsheng.wang@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 <scottwood@freescale.com>
Signed-off-by: Wang Dongsheng <dongsheng.wang@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

Re: [PATCH] powerpc/mpic: supply a .disable callback

[Benjamin Herrenschmidt]

On Tue, 2014-01-07 at 13:38 +0800, Dongsheng Wang wrote:
> From: Wang Dongsheng <dongsheng.wang@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 <scottwood@freescale.com>
> Signed-off-by: Wang Dongsheng <dongsheng.wang@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,

RE: [PATCH] powerpc/mpic: supply a .disable callback

[Dongsheng.Wang]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogQmVuamFtaW4gSGVycmVu
c2NobWlkdCBbbWFpbHRvOmJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZ10NCj4gU2VudDogVHVlc2Rh
eSwgSmFudWFyeSAwNywgMjAxNCAxOjUwIFBNDQo+IFRvOiBXYW5nIERvbmdzaGVuZy1CNDA1MzQN
Cj4gQ2M6IFdvb2QgU2NvdHQtQjA3NDIxOyBsaW51eHBwYy1kZXZAbGlzdHMub3psYWJzLm9yZw0K
PiBTdWJqZWN0OiBSZTogW1BBVENIXSBwb3dlcnBjL21waWM6IHN1cHBseSBhIC5kaXNhYmxlIGNh
bGxiYWNrDQo+IA0KPiBPbiBUdWUsIDIwMTQtMDEtMDcgYXQgMTM6MzggKzA4MDAsIERvbmdzaGVu
ZyBXYW5nIHdyb3RlOg0KPiA+IEZyb206IFdhbmcgRG9uZ3NoZW5nIDxkb25nc2hlbmcud2FuZ0Bm
cmVlc2NhbGUuY29tPg0KPiA+DQo+ID4gQ3VycmVudGx5IE1QSUMgcHJvdmlkZXMgLm1hc2ssIGJ1
dCBub3QgLmRpc2FibGUuICBUaGlzIG1lYW5zIHRoYXQNCj4gPiBlZmZlY3RpdmVseSBkaXNhYmxl
X2lycSgpIHNvZnQtZGlzYWJsZXMgdGhlIGludGVycnVwdCwgYW5kIHlvdSBnZXQNCj4gPiBhIC5t
YXNrIGNhbGwgaWYgYW4gaW50ZXJydXB0IGFjdHVhbGx5IG9jY3Vycy4NCj4gPg0KPiA+IEknbSBu
b3Qgc3VyZSBpZiB0aGlzIHdhcyBpbnRlbmRlZCBhcyBhIHBlcmZvcm1hbmNlIGJlbmVmaXQgKGl0
IHNlZW1zIGNvbW1vbg0KPiA+IHRvIG9taXQgLmRpc2FibGUgb24gcG93ZXJwYyBpbnRlcnJ1cHQg
Y29udHJvbGxlcnMsIGJ1dCBub3doZXJlIGVsc2UpLCBidXQgaXQNCj4gPiBpbnRlcmFjdHMgYmFk
bHkgd2l0aCB0aHJlYWRlZC93b3JrcXVldWUgaW50ZXJydXB0cyAoaW5jbHVkaW5nIEtWTQ0KPiA+
IHJlZmxlY3Rpb24pLiAgSW4gc3VjaCBjYXNlcywgd2hlcmUgdGhlIHJlYWwgaW50ZXJydXB0IGhh
bmRsZXIgZG9lcyBhDQo+ID4gZGlzYWJsZV9pcnFfbm9zeW5jKCksIHNjaGVkdWxlcyBkZWZlcmVk
IGhhbmRsaW5nLCBhbmQgcmV0dXJucywgd2UgZ2V0IHR3bw0KPiA+IGludGVycnVwdHMgZm9yIGV2
ZXJ5IHJlYWwgaW50ZXJydXB0LiAgVGhlIHNlY29uZCBpbnRlcnJ1cHQgZG9lcyBub3RoaW5nDQo+
ID4gYnV0IHNlZSB0aGF0IElSUV9ESVNBQkxFRCBpcyBzZXQsIGFuZCBkZWNpZGUgdGhhdCBpdCB3
b3VsZCBiZSBhIGdvb2QNCj4gPiBpZGVhIHRvIGFjdHVhbGx5IGNhbGwgLm1hc2suDQo+IA0KPiBX
ZSBwcm9iYWJseSBkb24ndCB3YW50IHRvIGRvIHRoYXQgZm9yIGVkZ2UsIG9ubHkgbGV2ZWwgaW50
ZXJydXB0cy4NCj4gDQpTb3JyeSBCZW4sIEkgYW0gbm90IHVuZGVyc3RhbmQgeW91ciBjb21tZW50
cy4NCg0KVGhpcyBpc3N1ZSBpcyB0aGUga2VybmVsIGFwaSBpcnFfZGlzYWJsZSgpIG9ubHkgdXNl
IGNoaXAtPmlycV9kaXNhYmxlKCksIGJ1dCBtcGljDQpub3QgaGF2ZSB0aGlzIGludGVyZmFjZSBz
byB3ZSBkb24ndCByZWFsIGRpc2FibGUgdGhlIGludGVycnVwdC4NCg0KLURvbmdzaGVuZw0KDQo+
IENoZWVycywNCj4gQmVuLg0KPiANCg0K

Re: [PATCH] powerpc/mpic: supply a .disable callback

[Scott Wood]

On Tue, 2014-01-07 at 04:18 -0600, Wang Dongsheng-B40534 wrote:
> 
> > -----Original Message-----
> > From: Benjamin Herrenschmidt [mailto:benh@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.wang@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

Re: [PATCH] powerpc/mpic: supply a .disable callback

[Benjamin Herrenschmidt]

On Tue, 2014-01-07 at 10:18 +0000, Dongsheng.Wang@freescale.com wrote:
> 
> > -----Original Message-----
> > From: Benjamin Herrenschmidt [mailto:benh@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.wang@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.
> > 
> 

Re: [PATCH] powerpc/mpic: supply a .disable callback

[Scott Wood]

On Tue, 2014-01-07 at 13:38 +0800, Dongsheng Wang wrote:
> From: Wang Dongsheng <dongsheng.wang@freescale.com>

Why did you change the author field?

-Scott

RE: [PATCH] powerpc/mpic: supply a .disable callback

[Dongsheng.Wang]

DQoNCj4gLS0tLS1PcmlnaW5hbCBNZXNzYWdlLS0tLS0NCj4gRnJvbTogV29vZCBTY290dC1CMDc0
MjENCj4gU2VudDogVHVlc2RheSwgSmFudWFyeSAwNywgMjAxNCAyOjM5IFBNDQo+IFRvOiBXYW5n
IERvbmdzaGVuZy1CNDA1MzQNCj4gQ2M6IGJlbmhAa2VybmVsLmNyYXNoaW5nLm9yZzsgbGludXhw
cGMtZGV2QGxpc3RzLm96bGFicy5vcmcNCj4gU3ViamVjdDogUmU6IFtQQVRDSF0gcG93ZXJwYy9t
cGljOiBzdXBwbHkgYSAuZGlzYWJsZSBjYWxsYmFjaw0KPiANCj4gT24gVHVlLCAyMDE0LTAxLTA3
IGF0IDEzOjM4ICswODAwLCBEb25nc2hlbmcgV2FuZyB3cm90ZToNCj4gPiBGcm9tOiBXYW5nIERv
bmdzaGVuZyA8ZG9uZ3NoZW5nLndhbmdAZnJlZXNjYWxlLmNvbT4NCj4gDQo+IFdoeSBkaWQgeW91
IGNoYW5nZSB0aGUgYXV0aG9yIGZpZWxkPw0KPiANCjooLiBJIGZvcmdvdCB0aGF0LCBJIGFkZGVk
IHRoaXMgY29kZSwgbm90IHVzZSBnaXQuDQpTb3JyeSBhYm91dCB0aGF0LCBJJ2xsIGNoYW5nZSBi
YWNrIGFmdGVyIG91ciBkaXNjdXNzaW9uDQoNCi1Eb25nc2hlbmcNCj4gLVNjb3R0DQo+IA0KDQo=