Re: [PATCH] PM: suspend_device_irqs(): don't disable wakeup IRQs

[Kim Kyuwon <chammoru@gmail.com>]

On Sat, May 23, 2009 at 8:47 AM, Rafael J. Wysocki <rjw@sisk.pl> wrote:
>> >> >>> > What do you do if the interrupt triggers right after your driver has
>> >> >>> > returned from its late suspend hook?
>> >> >>>
>> >> >>> If it's a wakeup IRQ, I assume you want it to prevent suspend.
>> >> >>>
>> >> >>> But I don't see how that can happen in the current code. IIUC, by the
>> >> >>> time your late suspend hook is run, your device IRQ is already
>> >> >>> disabled, so it won't trigger an interrupt that will be caught by
>> >> >>> check_wakeup_irqs() anyways.
>> >> >>
>> >> >> My understanding of __disable_irq() was that it didn't actually disable the
>> >> >> IRQ at the hardware level, allowing the CPU to actually receive the interrupt
>> >> >> and acknowledge it, but preventing the device driver for receiving it.  Does
>> >> >> it work differently on the affected systems?
>> >> >
>> >> > Hi, Rafael.
>> >> > Sorry for bring the old issue but please let me ask you about
>> >> > suspend_device_irqs() function.
>> >> >
>> >> > __disable_irq() disables the IRQ at the hardware level in the
>> >> > following irq_chips
>> >> >
>> >> > i8259A_chip
>> >> > i8259_pic
>> >> > i8259A_chip
>> >> > bfin_internal_irqchip
>> >> > crisv10_irq_type
>> >> > crisv32_irq_type
>> >> > h8300irq_chip
>> >> > m_irq_chip
>> >> > mn10300_cpu_pic_level
>> >> > xtensa_irq_chip
>> >> > iop13xx_msi_chip
>> >> > msi_irq
>> >> >
>> >> > Because these irq_chips mask interrupts in 'disable' hook.
>> >> >
>> >> > Thus, your suspend_device_irqs() function disables all IRQs at the
>> >> > hardware level on all architectures which use irq_chips listed above
>> >> > in suspend state.
>> >> > Is this really what you wanted?
>> >> >
>> >> > If interrupt can wake up the system from suspend in some architectures
>> >> > and if disable_irq_wake is not supported in these architectures, I
>> >> > wonder if suspend_device_irqs() don't allow waking up by interrupt.
>> >> >
>> >> > Regards,
>> >> > Kyuwon
>> >> >
>> >>
>> >> I saw resume_device_irqs() is invoked after arch_suspend_enable_irqs()
>> >> in your resume code.
>> >> So in this gap between resume_device_irqs() and
>> >> arch_suspend_enable_irqs(), a few interrupts would be discarded.
>> >> i.e, a few data would be lost.
>> >>
>> >> If keypad wake up the system, first key pressed information would be lost.
>> >> If I2C, USB, SPI, UART wake up the system, first a few data would be lost.
>> >>
>> >> Did you also consider this issue?
>> >
>> > I think it would happen anyway with the old code, wouldn't it?
>>
>> That's not quite right.
>>
>> For example, let's assume a keypad device is alive in suspend/resume
>> state to wake up the system. Before arch_suspend_enable_irqs(), none
>> of keypad irqs is dropped. It is just pending.
>>
>> But in your code, a few irqs are discarded due to your resume_device_irqs().
>
> You can't say for sure they are discarded, but there's a small window in which
> they can be discarded.  The question is whether it does cause problems in
> practice.

I tested it on my S3C6410(NCP) Board with MAX7359 keypad which is in
the Dmitry's Input repo.

> Anyway, IMO the device that caused a wake-up event should be deactivated before
> arch_suspend_enable_irqs() and remain inactive until its driver is actually
> ready to handle interrupts generated by it.

Some devices such as Bluetooth chip, keypad(including gpio-key),
Modem(Telephony, Wlan) chip and etc, should be activated while the
system suspend/resume state. Because these devices may communicate
other systems and should receive external wake-up events.

Thanks
Kyuwon