Re: Reworking suspend-resume sequence (was: Re: PCI PM: Restore standard config registers of all devices early)

["Rafael J. Wysocki" <rjw@sisk.pl>]

On Tuesday 03 February 2009, Benjamin Herrenschmidt wrote:
> On Tue, 2009-02-03 at 11:19 -0800, Linus Torvalds wrote:
> > 
> > On Tue, 3 Feb 2009, Linus Torvalds wrote:
> > > 
> > > Hmm. The _normal_ simple irq handler does this the way I described, but 
> > > for some reason the "handle_edge_irq()" does not. And the reason is 
> > > actually a buglet: it needs to mask things for the "recursive interrupt" 
> > > case.
> > 
> > Btw, just to clarify: none of this happens at the actual "irq_disable()" 
> > time: it only happens if you get an interrupt _while_ it's disabled. Which 
> > obviously shouldn't happen in the shutdown/wakeup path anyway for MSI, 
> > since the interrupts aren't shared, but it would be good to just be extra 
> > safe.
> > 
> > I do suspect we could/should just get rid of the msi masking entirely, but 
> > that may be too scary a step.
> 
> I agree :-) It's been a source of problem anyway, I remember hearing
> some device reacting strangely (aka, losing MSIs) when masked, and it's
> not even implemented by all devices (somebody had the "smart" idea of
> making an optional feature). So it ends up being a lot of not very
> useful code...
> 
> > For the current suspend/resume situation, maybe it's enough to know that 
> > it shouldn't be happening anyway, and even if it _does_ happen on a device 
> > that has been shut down, it's just not going to do anything. Sure, it's 
> > doing that "writel/readl", but if it gets lost, who really cares? Nobody.
> 
> Note that I still believe that life would be simpler to just keep the
> current local_irq_save() and just tweak might_sleep() etc... so that
> ACPI is safe to call. At least it can be done orthogonally to this
> interrupt change.

That would change the ordering of ACPI method calls, which also is important
and prone to breaking, as I wrote in the original message.

> IE. As I said earlier, that interrupt masking -will- change the exposed
> semantics of suspend_late() to drivers. In fact, iirc, it's you who
> advertised initially suspend_late() as being the 'easy' way to write a
> PCI driver suspend routine specifically because you don't have to bother
> about being re-entered from anywhere for things like request processing
> etc... With the change, the kernel is essentially still operating,
> timers are ticking, we may even be scheduling, so this important
> assumption is gone. That means a lot more chances for driver to screw
> up.

Whatever we do, it will have drawbacks and the approach with moving
local_irq_disable() seems to be more straightforward to me.

Thanks,
Rafael