| Message ID | 20150131092545.33ed35b8@notabene.brown (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Rafael Wysocki |
| Headers | show |
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);
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); > >
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); > > > > >
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
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);
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>