On Wed, Sep 11, 2013 at 10:16:56AM -0600, Stephen Warren wrote:
On 09/09/2013 02:27 PM, Mark Brown wrote:
Drivers usually work around this by disabling their interrupt with disable_irq() between suspend() and suspend_late(), then doing the same thing again between resume_noirq() and resume() covering both the windows where this can happen. This preserves the ability to
However, I'm not sure it's possible for genirq to handle this transparently in all situations.
When would the IRQ be re-enabled again?
See above - during the period between the device being suspended and interrupts being globally disabled, and then during the period between interrupts being reenabled and the device resumed.
If it the IRQ is enabled immediately before the device's resume() is called, then IRQs can still fire while the device is suspended. That would solve the issue at hand here; that the I2C device used for register IO wasn't resumed when the IRQ fired, but could presumably still be problematic for the device itself, in general if not in this case. Perhaps drivers just need to support this?
Yes, that's a problem if it's an issue in the driver/device itself - in that case it seems reasonable for the device to be responsible for quiescing itself (as they mostly do already).
If the IRQ is enabled immediately after the device's resume() is called, then a device wouldn't be able to receive IRQs during the execution of resume(), which I assume would completely break some drivers?
I think you're right there.
This gets more complicated with shared IRQ lines, since it wouldn't be possible to enable the IRQ immediately before/after *a* device's resume() was called, but rather, this could only happen immediately before/after the resume() for *all* devices using that IRQ were called.
Yeah. Fortunately I've only ever run into this with threaded interrupts so they can't be shared in the first place (which to be fair tends to come along with the buses that cause issues here).
It seems like the only way a driver can really manage IRQ handling in the general case is to mask interrupts at the source, like this patch does, rather than mask them at the interrupt controller, where the IRQ may already be shared.
The problem is that this doesn't actually fix the problem in some quite common cases, you can still trigger it so long as the device is still supposed to act as a wake source (and hence won't actually be suspended) - masking the interrupts at the source would actively break those systems since they do want to be woken by the device. To make matters worse if the device is the wake source then the interrupt will be asserted during resume making it more likely that the resume side of the issue will be triggered.
I guess that means that for wakeup interrupts, the driver does indeed need to disable the interrupt at the source between resume_noirq() and resume(), although I have no idea how that could possibly work, if the register IO is over an I2C device that itself uses its own interrupts...
The disable needs to be done with disable_irq() rather than by interacting with the device, just stop the interrupts being delivered until the device is capable of handling them rather than stopping the hardware asserting them. Fortunately since genirq doesn't allow threaded interrupts to be shared I2C devices won't normally have to worry about how rude this is for shared interrupts and generally the buses affected by this will be threaded.