[PATCH] IRQ: don't suspend nested_thread irqs over system suspend.

[PATCH] IRQ: don't suspend nested_thread irqs over system suspend.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 1741 bytes --]



Nested IRQs can only fire when the parent irq fires.
So when the parent is suspended, there is no need to suspend
the child irq.

Suspending nested irqs can cause a problem is they are suspended or
resumed in the wrong order.
If an interrupt fires while the parent is active but the child is
suspended, then the interrupt will not be acknowledged properly
and so an interrupt storm can result.
This is particularly likely if the parent is resumed before
the child, and the interrupt was raised during suspend.

Ensuring correct ordering would be possible, but it is simpler
to just never suspend nested interrupts.  This patch does that.

This patch allows the IRQF_EARLY_RESUME to be removed from
twl4030_sih_setup().  That flag attempts to fix the same problem
is a very different way, but causes

[   56.095825] WARNING: CPU: 0 PID: 3 at ../kernel/irq/manage.c:661 irq_nested_primary_handler+0x18/0x28()
[   56.095825] Primary handler called for nested irq 348

warnings on resume.

Signed-off-by: NeilBrown <neil@brown.name>

diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
index 3ca532592704..40cbcfb7fc43 100644
--- a/kernel/irq/pm.c
+++ b/kernel/irq/pm.c
@@ -118,6 +118,8 @@ void suspend_device_irqs(void)
 		unsigned long flags;
 		bool sync;
 
+		if (irq_settings_is_nested_thread(desc))
+			continue;
 		raw_spin_lock_irqsave(&desc->lock, flags);
 		sync = suspend_device_irq(desc, irq);
 		raw_spin_unlock_irqrestore(&desc->lock, flags);
@@ -158,6 +160,8 @@ static void resume_irqs(bool want_early)
 
 		if (!is_early && want_early)
 			continue;
+		if (irq_settings_is_nested_thread(desc))
+			continue;
 
 		raw_spin_lock_irqsave(&desc->lock, flags);
 		resume_irq(desc, irq);

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] IRQ: don't suspend nested_thread irqs over system suspend.

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 2049 bytes --]

On Saturday, January 31, 2015 09:25:45 AM NeilBrown wrote:
> 
> Nested IRQs can only fire when the parent irq fires.
> So when the parent is suspended, there is no need to suspend
> the child irq.
> 
> Suspending nested irqs can cause a problem is they are suspended or
> resumed in the wrong order.
> If an interrupt fires while the parent is active but the child is
> suspended, then the interrupt will not be acknowledged properly
> and so an interrupt storm can result.
> This is particularly likely if the parent is resumed before
> the child, and the interrupt was raised during suspend.
> 
> Ensuring correct ordering would be possible, but it is simpler
> to just never suspend nested interrupts.  This patch does that.

Clever. :-)

This is fine by me.  Thomas, what do you think?

> This patch allows the IRQF_EARLY_RESUME to be removed from
> twl4030_sih_setup().  That flag attempts to fix the same problem
> is a very different way, but causes
> 
> [   56.095825] WARNING: CPU: 0 PID: 3 at ../kernel/irq/manage.c:661 irq_nested_primary_handler+0x18/0x28()
> [   56.095825] Primary handler called for nested irq 348
> 
> warnings on resume.
> 
> Signed-off-by: NeilBrown <neil@brown.name>
> 
> diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> index 3ca532592704..40cbcfb7fc43 100644
> --- a/kernel/irq/pm.c
> +++ b/kernel/irq/pm.c
> @@ -118,6 +118,8 @@ void suspend_device_irqs(void)
>  		unsigned long flags;
>  		bool sync;
>  
> +		if (irq_settings_is_nested_thread(desc))
> +			continue;
>  		raw_spin_lock_irqsave(&desc->lock, flags);
>  		sync = suspend_device_irq(desc, irq);
>  		raw_spin_unlock_irqrestore(&desc->lock, flags);
> @@ -158,6 +160,8 @@ static void resume_irqs(bool want_early)
>  
>  		if (!is_early && want_early)
>  			continue;
> +		if (irq_settings_is_nested_thread(desc))
> +			continue;
>  
>  		raw_spin_lock_irqsave(&desc->lock, flags);
>  		resume_irq(desc, irq);

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] IRQ: don't suspend nested_thread irqs over system suspend.

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 2418 bytes --]

On Saturday, January 31, 2015 12:06:37 AM Rafael J. Wysocki wrote:
> On Saturday, January 31, 2015 09:25:45 AM NeilBrown wrote:
> > 
> > Nested IRQs can only fire when the parent irq fires.
> > So when the parent is suspended, there is no need to suspend
> > the child irq.
> > 
> > Suspending nested irqs can cause a problem is they are suspended or
> > resumed in the wrong order.
> > If an interrupt fires while the parent is active but the child is
> > suspended, then the interrupt will not be acknowledged properly
> > and so an interrupt storm can result.
> > This is particularly likely if the parent is resumed before
> > the child, and the interrupt was raised during suspend.
> > 
> > Ensuring correct ordering would be possible, but it is simpler
> > to just never suspend nested interrupts.  This patch does that.
> 
> Clever. :-)
> 
> This is fine by me.  Thomas, what do you think?

It looks like I've overlooked a potential problem, though.

Can a nested interrupt be a wakeup one?  We won't set IRQD_WAKEUP_ARMED for it
then and may not handle wakeup correctly.

> > This patch allows the IRQF_EARLY_RESUME to be removed from
> > twl4030_sih_setup().  That flag attempts to fix the same problem
> > is a very different way, but causes
> > 
> > [   56.095825] WARNING: CPU: 0 PID: 3 at ../kernel/irq/manage.c:661 irq_nested_primary_handler+0x18/0x28()
> > [   56.095825] Primary handler called for nested irq 348
> > 
> > warnings on resume.
> > 
> > Signed-off-by: NeilBrown <neil@brown.name>
> > 
> > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > index 3ca532592704..40cbcfb7fc43 100644
> > --- a/kernel/irq/pm.c
> > +++ b/kernel/irq/pm.c
> > @@ -118,6 +118,8 @@ void suspend_device_irqs(void)
> >  		unsigned long flags;
> >  		bool sync;
> >  
> > +		if (irq_settings_is_nested_thread(desc))
> > +			continue;
> >  		raw_spin_lock_irqsave(&desc->lock, flags);
> >  		sync = suspend_device_irq(desc, irq);
> >  		raw_spin_unlock_irqrestore(&desc->lock, flags);
> > @@ -158,6 +160,8 @@ static void resume_irqs(bool want_early)
> >  
> >  		if (!is_early && want_early)
> >  			continue;
> > +		if (irq_settings_is_nested_thread(desc))
> > +			continue;
> >  
> >  		raw_spin_lock_irqsave(&desc->lock, flags);
> >  		resume_irq(desc, irq);
> 
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

Re: [PATCH] IRQ: don't suspend nested_thread irqs over system suspend.

[NeilBrown]

[-- Attachment #1: Type: text/plain, Size: 3305 bytes --]

On Sat, 31 Jan 2015 00:51:17 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net>
wrote:

> On Saturday, January 31, 2015 12:06:37 AM Rafael J. Wysocki wrote:
> > On Saturday, January 31, 2015 09:25:45 AM NeilBrown wrote:
> > > 
> > > Nested IRQs can only fire when the parent irq fires.
> > > So when the parent is suspended, there is no need to suspend
> > > the child irq.
> > > 
> > > Suspending nested irqs can cause a problem is they are suspended or
> > > resumed in the wrong order.
> > > If an interrupt fires while the parent is active but the child is
> > > suspended, then the interrupt will not be acknowledged properly
> > > and so an interrupt storm can result.
> > > This is particularly likely if the parent is resumed before
> > > the child, and the interrupt was raised during suspend.
> > > 
> > > Ensuring correct ordering would be possible, but it is simpler
> > > to just never suspend nested interrupts.  This patch does that.
> > 
> > Clever. :-)
> > 
> > This is fine by me.  Thomas, what do you think?
> 
> It looks like I've overlooked a potential problem, though.
> 
> Can a nested interrupt be a wakeup one?  We won't set IRQD_WAKEUP_ARMED for it
> then and may not handle wakeup correctly.
> 

I only have a fairly narrow understanding of this stuff, but if you have
nested interrupts, you would surely need the parent to be registered as a
wakeup interrupt, else the device wouldn't wake and the nested interrupt
would be ineffective until something else woke the device.

Very few files mention both '.irq_set_wake' and  'irq_set_nested'.

twl6040-irq.c has code to set irq_wake_enable  on the parent if any nested
irqs have had irq_set_wake calls.
tps6586x.c has something similar, but much simpler.
arizona-irq.c and rc5t583-irq.c do the same as tps6586x.c

So I think that any nested interrupts which might want to be wakeup
interrupts already deal with the issue, and I don't introduce a new problem
here.

Thanks,
NeilBrown

> > > This patch allows the IRQF_EARLY_RESUME to be removed from
> > > twl4030_sih_setup().  That flag attempts to fix the same problem
> > > is a very different way, but causes
> > > 
> > > [   56.095825] WARNING: CPU: 0 PID: 3 at ../kernel/irq/manage.c:661 irq_nested_primary_handler+0x18/0x28()
> > > [   56.095825] Primary handler called for nested irq 348
> > > 
> > > warnings on resume.
> > > 
> > > Signed-off-by: NeilBrown <neil@brown.name>
> > > 
> > > diff --git a/kernel/irq/pm.c b/kernel/irq/pm.c
> > > index 3ca532592704..40cbcfb7fc43 100644
> > > --- a/kernel/irq/pm.c
> > > +++ b/kernel/irq/pm.c
> > > @@ -118,6 +118,8 @@ void suspend_device_irqs(void)
> > >  		unsigned long flags;
> > >  		bool sync;
> > >  
> > > +		if (irq_settings_is_nested_thread(desc))
> > > +			continue;
> > >  		raw_spin_lock_irqsave(&desc->lock, flags);
> > >  		sync = suspend_device_irq(desc, irq);
> > >  		raw_spin_unlock_irqrestore(&desc->lock, flags);
> > > @@ -158,6 +160,8 @@ static void resume_irqs(bool want_early)
> > >  
> > >  		if (!is_early && want_early)
> > >  			continue;
> > > +		if (irq_settings_is_nested_thread(desc))
> > > +			continue;
> > >  
> > >  		raw_spin_lock_irqsave(&desc->lock, flags);
> > >  		resume_irq(desc, irq);
> > 
> > 
> 


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 811 bytes --]

Re: [PATCH] IRQ: don't suspend nested_thread irqs over system suspend.

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 2206 bytes --]

On Saturday, January 31, 2015 02:37:47 PM NeilBrown wrote:
> On Sat, 31 Jan 2015 00:51:17 +0100 "Rafael J. Wysocki" <rjw@rjwysocki.net>
> wrote:
> 
> > On Saturday, January 31, 2015 12:06:37 AM Rafael J. Wysocki wrote:
> > > On Saturday, January 31, 2015 09:25:45 AM NeilBrown wrote:
> > > > 
> > > > Nested IRQs can only fire when the parent irq fires.
> > > > So when the parent is suspended, there is no need to suspend
> > > > the child irq.
> > > > 
> > > > Suspending nested irqs can cause a problem is they are suspended or
> > > > resumed in the wrong order.
> > > > If an interrupt fires while the parent is active but the child is
> > > > suspended, then the interrupt will not be acknowledged properly
> > > > and so an interrupt storm can result.
> > > > This is particularly likely if the parent is resumed before
> > > > the child, and the interrupt was raised during suspend.
> > > > 
> > > > Ensuring correct ordering would be possible, but it is simpler
> > > > to just never suspend nested interrupts.  This patch does that.
> > > 
> > > Clever. :-)
> > > 
> > > This is fine by me.  Thomas, what do you think?
> > 
> > It looks like I've overlooked a potential problem, though.
> > 
> > Can a nested interrupt be a wakeup one?  We won't set IRQD_WAKEUP_ARMED for it
> > then and may not handle wakeup correctly.
> > 
> 
> I only have a fairly narrow understanding of this stuff, but if you have
> nested interrupts, you would surely need the parent to be registered as a
> wakeup interrupt, else the device wouldn't wake and the nested interrupt
> would be ineffective until something else woke the device.
> 
> Very few files mention both '.irq_set_wake' and  'irq_set_nested'.
> 
> twl6040-irq.c has code to set irq_wake_enable  on the parent if any nested
> irqs have had irq_set_wake calls.
> tps6586x.c has something similar, but much simpler.
> arizona-irq.c and rc5t583-irq.c do the same as tps6586x.c
> 
> So I think that any nested interrupts which might want to be wakeup
> interrupts already deal with the issue, and I don't introduce a new problem
> here.

Fair enough.

I wonder if this means that it'll be useful to propagate IRQD_WAKEUP_STATE to
parents, then ...

Rafael

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 836 bytes --]