On Wed, Aug 3, 2011 at 7:59 AM, Dmitry Torokhov
<dmitry.torokhov@xxxxxxxxx> wrote:
> I am not sure that we should be marking input devices themselves as
> wakeup capable - they are in no way physical devices. I'd stop at serio
> level...
We need a way to make sure the device state is untouched during
suspend/resume. Would you be happier if an input device looked at the
device_may_wakeup() of its parent?
>>
>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>> index da38d97..639aba7 100644
>> --- a/drivers/input/input.c
>> +++ b/drivers/input/input.c
>> @@ -1588,6 +1588,9 @@ static int input_dev_suspend(struct device *dev)
>> {
>> struct input_dev *input_dev = to_input_dev(dev);
>>
>> + if (device_may_wakeup(dev))
>> + return 0;
>> +
>
> On suspend should we not try to shut off leds and sound?
Thats right. The setup is that the system appears to be running as
normal, our display also remains on during suspend (and wifi, and so
on). Bar the laptop's power LED which goes from always-on to flashing,
the user is unaware that the laptop has suspended.
>> mutex_lock(&input_dev->mutex);
>>
>> if (input_dev->users)
>> @@ -1602,7 +1605,8 @@ static int input_dev_resume(struct device *dev)
>> {
>> struct input_dev *input_dev = to_input_dev(dev);
>>
>> - input_reset_device(input_dev);
>> + if (!device_may_wakeup(dev))
>> + input_reset_device(input_dev);
>>
>
> Does the controller wakes up the system on key release or only press? My
> concern is with cases when we suspend with a key pressed and wake up
> with it already released.
It wakes up on key press, but our EC buffers communication, so both
the key press and key release event would be delivered in the above
scenario.
>> @@ -87,6 +87,7 @@ MODULE_PARM_DESC(debug, "Turn i8042 debugging mode on and off");
>> #endif
>>
>> static bool i8042_bypass_aux_irq_test;
>> +static bool i8042_enable_wakeup;
>>
>> #include "i8042.h"
>>
>> @@ -1082,10 +1083,17 @@ static void i8042_dritek_enable(void)
>> * before suspending.
>> */
>>
>> -static int i8042_controller_resume(bool force_reset)
>> +static int i8042_controller_resume(bool force_reset, bool soft_resume)
>> {
>> int error;
>>
>> + /*
>> + * If device is selected as a wakeup source, it was not powered down
>> + * or reset during suspend, so we have very little to do.
>> + */
>> + if (soft_resume)
>> + goto soft;
>
> Maybe instead of adding a parameter simply not call the function and
> move i8042_interrupt as well?
OK
>> static int i8042_pm_resume(struct device *dev)
>> {
>> /*
>> * On resume from S2R we always try to reset the controller
>> * to bring it in a sane state. (In case of S2D we expect
>> * BIOS to reset the controller for us.)
>> + * This function call will also handle any pending keypress event
>> + * (perhaps the system wakeup reason)
>> + */
>> + int r = i8042_controller_resume(true, device_may_wakeup(dev));
>> +
>> + /* If the device was left running during suspend, enable IRQs again
>> + * now. Must be done last to avoid races with interrupt processing
>> + * inside i8042_controller_resume.
>> */
>
> Hmm, we have proper locking so I am not sure how true this statement is.
What happens is that the event used to wake up the system is not
delivered as an interrupt (consistent with the fact that the resume
handler already calls i8042_interrupt() manually). However, there is a
race window. If the system was woken up with a key press, and then the
key was released during early resume, the key press event will not be
presented as an interrupt, but the key release event will. That
interrupt can arrive before i8042_controller_resume() reaches
i8042_interrupt(), which would result in those 2 events being
processed in the wrong order. I'll make this clearer in the comment.
Thanks for the fast review comments!
Daniel
_______________________________________________
linux-pm mailing list
linux-pm@xxxxxxxxxxxxxxxxxxxxxxxxxx
https://lists.linux-foundation.org/mailman/listinfo/linux-pm
