| Message ID | 20110802154916.624619D401C@zog.reactivated.net (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
Hi Daniel, On Tue, Aug 02, 2011 at 04:49:15PM +0100, Daniel Drake wrote: > The OLPC XO laptop is able to use the PS/2 controller as a wakeup source. > When used as a wakeup source, the key press/mouse motion must not be lost. > > Add infrastructure to allow controllers to be marked as wakeup-capable, > and avoid doing power control/reset on the i8042/serio/input devices when > running in this mode. For systems where this functionality is available, > you are expected to enable wakeups on the i8042 device, the serio > devices, and the relevant input devices, to ensure that the hardware is > left powered and untouched throughout the suspend/resume. > > Signed-off-by: Daniel Drake <dsd@laptop.org> > --- > drivers/input/input.c | 6 +++- > drivers/input/keyboard/atkbd.c | 4 ++- > drivers/input/mouse/hgpk.c | 2 + > drivers/input/mouse/psmouse-base.c | 4 ++- > drivers/input/serio/i8042-io.h | 4 ++ > drivers/input/serio/i8042-ip22io.h | 4 ++ > drivers/input/serio/i8042-jazzio.h | 4 ++ > drivers/input/serio/i8042-ppcio.h | 4 ++ > drivers/input/serio/i8042-snirm.h | 4 ++ > drivers/input/serio/i8042-sparcio.h | 4 ++ > drivers/input/serio/i8042-x86ia64io.h | 4 ++ > drivers/input/serio/i8042.c | 62 +++++++++++++++++++++++++++++--- > drivers/input/serio/serio.c | 29 +++++++++++++-- > 13 files changed, 122 insertions(+), 13 deletions(-) > > On original submission, Dmitry was worried about this functionality not > working at all on other platforms. I agree, it will only work where the > hardware has been specifically designed with this consideration. v2 of > the patch therefore removes the module param option, meaning that it > will only be activated on platforms that explicitly enable it at the > code level. > > v2 also performs a more extensive job. We avoid resetting the device > at the input_device level during suspend/resume. We also disable i8042 > interrupts when going into suspend, to avoid races handling interrupts > in the wrong order during resume. > > v3 includes a cleanup suggested by Rafael, and explains the corner case > that we have to handle in the input layer. > > v4 changes direction a little: each layer now just looks at the wakeup > capability of its own device, avoiding some of the earlier tree > traversal. Userspace must now enable wakeups on 5 devices: > i8042 > i8042/serio0 > i8042/serio0/input* > i8042/serio1 > i8042/serio1/input* > This matches the behaviour of USB devices, where both the device and the > parent controller must have wakeups enabled for wakeup functionality > to work. Suggested by Rafael J. Wysocki. 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... > > 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? > 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. > return 0; > } > diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c > index 19cfc0c..4bb81c2 100644 > --- a/drivers/input/keyboard/atkbd.c > +++ b/drivers/input/keyboard/atkbd.c > @@ -1027,6 +1027,7 @@ static void atkbd_set_keycode_table(struct atkbd *atkbd) > static void atkbd_set_device_attrs(struct atkbd *atkbd) > { > struct input_dev *input_dev = atkbd->dev; > + struct device *parent = &atkbd->ps2dev.serio->dev; > int i; > > if (atkbd->extra) > @@ -1047,7 +1048,8 @@ static void atkbd_set_device_attrs(struct atkbd *atkbd) > input_dev->id.product = atkbd->translated ? 1 : atkbd->set; > input_dev->id.version = atkbd->id; > input_dev->event = atkbd_event; > - input_dev->dev.parent = &atkbd->ps2dev.serio->dev; > + input_dev->dev.parent = parent; > + device_set_wakeup_capable(&input_dev->dev, device_can_wakeup(parent)); > > input_set_drvdata(input_dev, atkbd); > > diff --git a/drivers/input/mouse/hgpk.c b/drivers/input/mouse/hgpk.c > index 95577c1..d5dd990 100644 > --- a/drivers/input/mouse/hgpk.c > +++ b/drivers/input/mouse/hgpk.c > @@ -548,6 +548,8 @@ static void hgpk_setup_input_device(struct input_dev *input, > input->phys = old_input->phys; > input->id = old_input->id; > input->dev.parent = old_input->dev.parent; > + device_set_wakeup_capable(&input->dev, > + device_can_wakeup(&old_input->dev)); > } > > memset(input->evbit, 0, sizeof(input->evbit)); > diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c > index 3f74bae..de8ecd5 100644 > --- a/drivers/input/mouse/psmouse-base.c > +++ b/drivers/input/mouse/psmouse-base.c > @@ -1232,8 +1232,10 @@ static int psmouse_switch_protocol(struct psmouse *psmouse, > { > const struct psmouse_protocol *selected_proto; > struct input_dev *input_dev = psmouse->dev; > + struct device *parent = &psmouse->ps2dev.serio->dev; > > - input_dev->dev.parent = &psmouse->ps2dev.serio->dev; > + input_dev->dev.parent = parent; > + device_set_wakeup_capable(&input_dev->dev, device_can_wakeup(parent)); > > input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REL); > input_dev->keybit[BIT_WORD(BTN_MOUSE)] = > diff --git a/drivers/input/serio/i8042-io.h b/drivers/input/serio/i8042-io.h > index 5d48bb6..296633c 100644 > --- a/drivers/input/serio/i8042-io.h > +++ b/drivers/input/serio/i8042-io.h > @@ -92,4 +92,8 @@ static inline void i8042_platform_exit(void) > #endif > } > > +static inline void i8042_platform_suspend(struct device *dev, bool may_wakeup) > +{ > +} This is ugly but I guess it's the best option without switching to i8042_ops or something. > + > #endif /* _I8042_IO_H */ > diff --git a/drivers/input/serio/i8042-ip22io.h b/drivers/input/serio/i8042-ip22io.h > index ee1ad27..c5b76a4 100644 > --- a/drivers/input/serio/i8042-ip22io.h > +++ b/drivers/input/serio/i8042-ip22io.h > @@ -73,4 +73,8 @@ static inline void i8042_platform_exit(void) > #endif > } > > +static inline void i8042_platform_suspend(struct device *dev, bool may_wakeup) > +{ > +} > + > #endif /* _I8042_IP22_H */ > diff --git a/drivers/input/serio/i8042-jazzio.h b/drivers/input/serio/i8042-jazzio.h > index 13fd710..a11913a 100644 > --- a/drivers/input/serio/i8042-jazzio.h > +++ b/drivers/input/serio/i8042-jazzio.h > @@ -66,4 +66,8 @@ static inline void i8042_platform_exit(void) > #endif > } > > +static inline void i8042_platform_suspend(struct device *dev, bool may_wakeup) > +{ > +} > + > #endif /* _I8042_JAZZ_H */ > diff --git a/drivers/input/serio/i8042-ppcio.h b/drivers/input/serio/i8042-ppcio.h > index f708c75..c9f4292 100644 > --- a/drivers/input/serio/i8042-ppcio.h > +++ b/drivers/input/serio/i8042-ppcio.h > @@ -52,6 +52,10 @@ static inline void i8042_platform_exit(void) > { > } > > +static inline void i8042_platform_suspend(struct device *dev, bool may_wakeup) > +{ > +} > + > #else > > #include "i8042-io.h" > diff --git a/drivers/input/serio/i8042-snirm.h b/drivers/input/serio/i8042-snirm.h > index 409a934..96d034f 100644 > --- a/drivers/input/serio/i8042-snirm.h > +++ b/drivers/input/serio/i8042-snirm.h > @@ -72,4 +72,8 @@ static inline void i8042_platform_exit(void) > > } > > +static inline void i8042_platform_suspend(struct device *dev, bool may_wakeup) > +{ > +} > + > #endif /* _I8042_SNIRM_H */ > diff --git a/drivers/input/serio/i8042-sparcio.h b/drivers/input/serio/i8042-sparcio.h > index 395a9af..e5381d3 100644 > --- a/drivers/input/serio/i8042-sparcio.h > +++ b/drivers/input/serio/i8042-sparcio.h > @@ -154,4 +154,8 @@ static inline void i8042_platform_exit(void) > } > #endif /* !CONFIG_PCI */ > > +static inline void i8042_platform_suspend(struct device *dev, bool may_wakeup) > +{ > +} > + > #endif /* _I8042_SPARCIO_H */ > diff --git a/drivers/input/serio/i8042-x86ia64io.h b/drivers/input/serio/i8042-x86ia64io.h > index bb9f5d3..76b2e58 100644 > --- a/drivers/input/serio/i8042-x86ia64io.h > +++ b/drivers/input/serio/i8042-x86ia64io.h > @@ -875,6 +875,10 @@ static inline int i8042_pnp_init(void) { return 0; } > static inline void i8042_pnp_exit(void) { } > #endif > > +static inline void i8042_platform_suspend(struct device *dev, bool may_wakeup) > +{ > +} > + > static int __init i8042_platform_init(void) > { > int retval; > diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c > index d37a48e..d275130 100644 > --- a/drivers/input/serio/i8042.c > +++ b/drivers/input/serio/i8042.c > @@ -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? > + > error = i8042_controller_check(); > if (error) > return error; > @@ -1129,6 +1137,7 @@ static int i8042_controller_resume(bool force_reset) > if (i8042_ports[I8042_KBD_PORT_NO].serio) > i8042_enable_kbd_port(); > > +soft: > i8042_interrupt(0, NULL); > > return 0; > @@ -1146,14 +1155,48 @@ static int i8042_pm_reset(struct device *dev) > return 0; > } > > +static int i8042_pm_suspend(struct device *dev) > +{ > + i8042_platform_suspend(dev, device_may_wakeup(dev)); > + > + /* > + * If device is selected as a wakeup source, don't powerdown or reset > + * during suspend. Just disable IRQs to ensure race-free resume. > + */ > + if (device_may_wakeup(dev)) { > + if (i8042_kbd_irq_registered) > + disable_irq(I8042_KBD_IRQ); > + if (i8042_aux_irq_registered) > + disable_irq(I8042_AUX_IRQ); > + return 0; > + } > + > + return i8042_pm_reset(dev); > +} > + > 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. > - return i8042_controller_resume(true); > + if (device_may_wakeup(dev)) { > + if (i8042_kbd_irq_registered) > + enable_irq(I8042_KBD_IRQ); > + if (i8042_aux_irq_registered) > + enable_irq(I8042_AUX_IRQ); > + } > + > + return r; > } > > static int i8042_pm_thaw(struct device *dev) > @@ -1165,11 +1208,11 @@ static int i8042_pm_thaw(struct device *dev) > > static int i8042_pm_restore(struct device *dev) > { > - return i8042_controller_resume(false); > + return i8042_controller_resume(false, false); > } > > static const struct dev_pm_ops i8042_pm_ops = { > - .suspend = i8042_pm_reset, > + .suspend = i8042_pm_suspend, > .resume = i8042_pm_resume, > .thaw = i8042_pm_thaw, > .poweroff = i8042_pm_reset, > @@ -1192,6 +1235,7 @@ static int __init i8042_create_kbd_port(void) > { > struct serio *serio; > struct i8042_port *port = &i8042_ports[I8042_KBD_PORT_NO]; > + struct device *parent = &i8042_platform_device->dev; > > serio = kzalloc(sizeof(struct serio), GFP_KERNEL); > if (!serio) > @@ -1203,7 +1247,8 @@ static int __init i8042_create_kbd_port(void) > serio->stop = i8042_stop; > serio->close = i8042_port_close; > serio->port_data = port; > - serio->dev.parent = &i8042_platform_device->dev; > + serio->dev.parent = parent; > + device_set_wakeup_capable(&serio->dev, device_can_wakeup(parent)); > strlcpy(serio->name, "i8042 KBD port", sizeof(serio->name)); > strlcpy(serio->phys, I8042_KBD_PHYS_DESC, sizeof(serio->phys)); > > @@ -1218,6 +1263,7 @@ static int __init i8042_create_aux_port(int idx) > struct serio *serio; > int port_no = idx < 0 ? I8042_AUX_PORT_NO : I8042_MUX_PORT_NO + idx; > struct i8042_port *port = &i8042_ports[port_no]; > + struct device *parent = &i8042_platform_device->dev; > > serio = kzalloc(sizeof(struct serio), GFP_KERNEL); > if (!serio) > @@ -1228,7 +1274,8 @@ static int __init i8042_create_aux_port(int idx) > serio->start = i8042_start; > serio->stop = i8042_stop; > serio->port_data = port; > - serio->dev.parent = &i8042_platform_device->dev; > + serio->dev.parent = parent; > + device_set_wakeup_capable(&serio->dev, device_can_wakeup(parent)); > if (idx < 0) { > strlcpy(serio->name, "i8042 AUX port", sizeof(serio->name)); > strlcpy(serio->phys, I8042_AUX_PHYS_DESC, sizeof(serio->phys)); > @@ -1403,6 +1450,9 @@ static int __init i8042_probe(struct platform_device *dev) > i8042_dritek_enable(); > #endif > > + if (i8042_enable_wakeup) > + device_set_wakeup_capable(&dev->dev, true); > + device_set_wakeup_capable(&dev->dev, i8042_enable_wakeup); ? > if (!i8042_noaux) { > error = i8042_setup_aux(); > if (error && error != -ENODEV && error != -EBUSY) > diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c > index ba70058..9e2fdb4 100644 > --- a/drivers/input/serio/serio.c > +++ b/drivers/input/serio/serio.c > @@ -931,7 +931,7 @@ static int serio_uevent(struct device *dev, struct kobj_uevent_env *env) > #endif /* CONFIG_HOTPLUG */ > > #ifdef CONFIG_PM > -static int serio_suspend(struct device *dev) > +static int serio_poweroff(struct device *dev) > { > struct serio *serio = to_serio_port(dev); > > @@ -940,7 +940,16 @@ static int serio_suspend(struct device *dev) > return 0; > } > > -static int serio_resume(struct device *dev) > +static int serio_suspend(struct device *dev) > +{ > + /* If configured as a wakeup source, don't power off. */ > + if (device_may_wakeup(dev)) > + return 0; > + > + return serio_poweroff(dev); > +} > + > +static int serio_restore(struct device *dev) > { > struct serio *serio = to_serio_port(dev); > > @@ -953,11 +962,23 @@ static int serio_resume(struct device *dev) > return 0; > } > > +static int serio_resume(struct device *dev) > +{ > + /* > + * If configured as a wakeup source, we didn't power off during > + * suspend, and hence have nothing to do. > + */ > + if (device_may_wakeup(dev)) > + return 0; > + > + return serio_restore(dev); > +} > + > static const struct dev_pm_ops serio_pm_ops = { > .suspend = serio_suspend, > .resume = serio_resume, > - .poweroff = serio_suspend, > - .restore = serio_resume, > + .poweroff = serio_poweroff, > + .restore = serio_restore, > }; > #endif /* CONFIG_PM */ > > -- > 1.7.6 >
On Wed, Aug 3, 2011 at 7:59 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> 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
On 08/03/2011 02:59 PM, Dmitry Torokhov wrote: > Hi Daniel, > > On Tue, Aug 02, 2011 at 04:49:15PM +0100, Daniel Drake wrote: >> The OLPC XO laptop is able to use the PS/2 controller as a wakeup source. >> When used as a wakeup source, the key press/mouse motion must not be lost. >> >> Add infrastructure to allow controllers to be marked as wakeup-capable, >> and avoid doing power control/reset on the i8042/serio/input devices when >> running in this mode. For systems where this functionality is available, >> you are expected to enable wakeups on the i8042 device, the serio >> devices, and the relevant input devices, to ensure that the hardware is >> left powered and untouched throughout the suspend/resume. >> >> Signed-off-by: Daniel Drake<dsd@laptop.org> >> --- >> drivers/input/input.c | 6 +++- >> drivers/input/keyboard/atkbd.c | 4 ++- >> drivers/input/mouse/hgpk.c | 2 + >> drivers/input/mouse/psmouse-base.c | 4 ++- >> drivers/input/serio/i8042-io.h | 4 ++ >> drivers/input/serio/i8042-ip22io.h | 4 ++ >> drivers/input/serio/i8042-jazzio.h | 4 ++ >> drivers/input/serio/i8042-ppcio.h | 4 ++ >> drivers/input/serio/i8042-snirm.h | 4 ++ >> drivers/input/serio/i8042-sparcio.h | 4 ++ >> drivers/input/serio/i8042-x86ia64io.h | 4 ++ >> drivers/input/serio/i8042.c | 62 +++++++++++++++++++++++++++++--- >> drivers/input/serio/serio.c | 29 +++++++++++++-- >> 13 files changed, 122 insertions(+), 13 deletions(-) >> >> serio->port_data = port; >> - serio->dev.parent =&i8042_platform_device->dev; >> + serio->dev.parent = parent; >> + device_set_wakeup_capable(&serio->dev, device_can_wakeup(parent)); >> if (idx< 0) { >> strlcpy(serio->name, "i8042 AUX port", sizeof(serio->name)); >> strlcpy(serio->phys, I8042_AUX_PHYS_DESC, sizeof(serio->phys)); >> @@ -1403,6 +1450,9 @@ static int __init i8042_probe(struct platform_device *dev) >> i8042_dritek_enable(); >> #endif >> >> + if (i8042_enable_wakeup) >> + device_set_wakeup_capable(&dev->dev, true); >> + > > device_set_wakeup_capable(&dev->dev, i8042_enable_wakeup); ? > Hi Daniel: May be you missed this comment by Dmitry? Thanks Best Regards Wanlong Gao
On Wed, Aug 03, 2011 at 09:04:56AM +0100, Daniel Drake wrote: > On Wed, Aug 3, 2011 at 7:59 AM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> 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? I think we should leave input core out of it completely, see below... > > >> > >> 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. No, I do not believe we can do what you are doing in input core. You are changing behavior for one platform and one (or 2 drivers) that will not be matched (at least I don't think so) by anything else out there. What will happen if you plug a random USB keyboard into OLPC box? Will it keep leds powered? > > >> 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. Just to confirm, we 3 events will be delivered in this case: 1 - old key release 2 - wakeup key press 3 - wakeup key release ? I also wonder what will happen with non-PS2 devices... Instead of wiring it all through input core could we contain this in atkbd and hgpk by registering pm_notifiers and ignoring certain requests from input/serio cores during system state transition on OLPC only?
On Wed, Aug 3, 2011 at 7:43 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > No, I do not believe we can do what you are doing in input core. You are > changing behavior for one platform and one (or 2 drivers) that will not > be matched (at least I don't think so) by anything else out there. What > will happen if you plug a random USB keyboard into OLPC box? Will it keep > leds powered? In all other cases, behaviour is unchanged. The input device is not marked as wakeup capable, therefore the user cannot mark it as wakeup-enabled, therefore this code does not execute. So a USB keyboard on an OLPC laptop would behave exactly as it does before this patch, as would devices on other systems. The only way this functionality gets enabled is at the code level (as it does in patch 2/2), and then once the user has requested wakeups on all devices in the chain. >> > 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. > > Just to confirm, we 3 events will be delivered in this case: Sorry, I misunderstood the question and answered "what happens if the system is sleeping and you press and quickly release a key before the system is fully running" - in which case the system wakes up and delivers both events. For the question you asked, regarding a key being pressed as the system goes into suspend, the system wakes up from suspend immediately. I just tested this by holding the 'i' key down while going into suspend. While the key is being held down, the system receives repeated interrupts with data 0x17 ('i' key pressed). The system suspends, but then immediately wakes up, and continues the flood of 0x17 interrupts. When I release the key a few seconds later, a 0x97 ('i' released) interrupt arrives. I guess the more accurate way to describe the wakeup condition is that it wakes up on any i8042 interrupt. > I also wonder what will happen with non-PS2 devices... Again - other devices are not affected. > Instead of wiring it all through input core could we contain this in > atkbd and hgpk by registering pm_notifiers and ignoring certain requests > from input/serio cores during system state transition on OLPC only? I'll look into this, thanks for the suggestion. Daniel
On Wed, Aug 03, 2011 at 08:24:46PM +0100, Daniel Drake wrote: > On Wed, Aug 3, 2011 at 7:43 PM, Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > No, I do not believe we can do what you are doing in input core. You are > > changing behavior for one platform and one (or 2 drivers) that will not > > be matched (at least I don't think so) by anything else out there. What > > will happen if you plug a random USB keyboard into OLPC box? Will it keep > > leds powered? > > In all other cases, behaviour is unchanged. The input device is not > marked as wakeup capable, therefore the user cannot mark it as > wakeup-enabled, therefore this code does not execute. > > So a USB keyboard on an OLPC laptop would behave exactly as it does > before this patch, as would devices on other systems. > > The only way this functionality gets enabled is at the code level (as > it does in patch 2/2), and then once the user has requested wakeups on > all devices in the chain. I believe we can and do mark devices such as USB as wakeup capable on other arches, you do not have control here. That is why I am uneasy with doing this in input core. > > >> > 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. > > > > Just to confirm, we 3 events will be delivered in this case: > > Sorry, I misunderstood the question and answered "what happens if the > system is sleeping and you press and quickly release a key before the > system is fully running" - in which case the system wakes up and > delivers both events. > > For the question you asked, regarding a key being pressed as the > system goes into suspend, the system wakes up from suspend > immediately. > I just tested this by holding the 'i' key down while going into suspend. > While the key is being held down, the system receives repeated > interrupts with data 0x17 ('i' key pressed). The system suspends, but > then immediately wakes up, and continues the flood of 0x17 interrupts. > When I release the key a few seconds later, a 0x97 ('i' released) > interrupt arrives. Is there any keys that are not autorepeating. For example regular (non-OLPC) laptops usually do not repeat suspend and other special keys. In fact, they quite often forget to send release events for them ;) > > I guess the more accurate way to describe the wakeup condition is that > it wakes up on any i8042 interrupt. > > > I also wonder what will happen with non-PS2 devices... > > Again - other devices are not affected. > > > Instead of wiring it all through input core could we contain this in > > atkbd and hgpk by registering pm_notifiers and ignoring certain requests > > from input/serio cores during system state transition on OLPC only? > > I'll look into this, thanks for the suggestion. Much appreciated, sorry for being a pain.
On Wed, Aug 3, 2011 at 8:38 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > I believe we can and do mark devices such as USB as wakeup capable on > other arches, you do not have control here. That is why I am uneasy with > doing this in input core. That's possible, but do note that the latest iteration of the patch only looks at the wakeup capability of the struct input_device. It is unlikely that another part of the kernel marks it as wakeup-capable without taking these considerations into account. It is more likely that, in the USB case, the wakeup field of the struct usb_device is marked wakeup capable. With Alan's ACPI case earlier in the discussion, this was true: we found that only the ACPI device gets marked as wakeup-capable/wakeup-enabled, and not the input device or even the i8042/serio devices, even though it is implementing (some form of) keyboard wakeup. Anyway, I'll look at implementing it according to how you suggested, which wouldn't involve such wide-reaching changes. > Is there any keys that are not autorepeating. For example regular > (non-OLPC) laptops usually do not repeat suspend and other special keys. > In fact, they quite often forget to send release events for them ;) I just tested, and all of our keys autorepeat in that fashion. Even the odd keys like "rotate" and "change language". I guess that makes us irregular ;) Daniel
Hi Dmitry, On Wed, Aug 3, 2011 at 7:43 PM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > Instead of wiring it all through input core could we contain this in > atkbd and hgpk by registering pm_notifiers and ignoring certain requests > from input/serio cores during system state transition on OLPC only? I've started looking at this, but my first problem is that when the input layer asks atkbd to turn off LEDs, atkbd schedules a workqueue item to undertake this work. However, as there is no synchronization, this work only seems to get executed during or after system resume. This makes it hard to catch with such a scheme. Any ideas? Daniel
Hi Dmitry, On Fri, Aug 5, 2011 at 4:24 PM, Daniel Drake <dsd@laptop.org> wrote: > I've started looking at this, but my first problem is that when the > input layer asks atkbd to turn off LEDs, atkbd schedules a workqueue > item to undertake this work. However, as there is no synchronization, > this work only seems to get executed during or after system resume. > This makes it hard to catch with such a scheme. Any ideas? I've looked closer, and am keeping my eyes open for other options too. During suspend of the input layer, input_dev_suspend() calls input_dev_toggle(dev, false) which turns off LEDs and sounds. On the XO laptops, we don't have LEDs, nor sounds, and we don't have num lock, scroll lock or caps lock keys. So the suspend code does nothing - there are no lights to turn off. So far so good. Moving to the resume case: input_dev_resume() calls input_reset_device(). This calls input_dev_toggle(dev, true), which asks atkbd to turn the non-existent keyboard LEDs off. I guess this is harmless and is unlikely to be the cause of the problems which led us to need to disable this code in light of the following: After calling input_dev_toggle(), input_dev_resume() does this: /* * Keys that have been pressed at suspend time are unlikely * to be still pressed when we resume. */ spin_lock_irq(&dev->event_lock); input_dev_release_keys(dev); spin_unlock_irq(&dev->event_lock); We definitely don't want this to happen in our case, where we have maintained full control and power of the device during suspend/resume. This particular important code snippet would not even have been disabled by your suggestion of blocking data transfer at the i8042-level. So, I think we need a new approach at solving this, and I think it has to be one that interacts at the input_dev layer. Thoughts? Thanks, Daniel
Hi Daniel, On Thu, Aug 11, 2011 at 07:02:17PM +0100, Daniel Drake wrote: > Hi Dmitry, > > On Fri, Aug 5, 2011 at 4:24 PM, Daniel Drake <dsd@laptop.org> wrote: > > I've started looking at this, but my first problem is that when the > > input layer asks atkbd to turn off LEDs, atkbd schedules a workqueue > > item to undertake this work. However, as there is no synchronization, > > this work only seems to get executed during or after system resume. > > This makes it hard to catch with such a scheme. Any ideas? > > I've looked closer, and am keeping my eyes open for other options too. > > During suspend of the input layer, input_dev_suspend() calls > input_dev_toggle(dev, false) which turns off LEDs and sounds. > > On the XO laptops, we don't have LEDs, nor sounds, and we don't have > num lock, scroll lock or caps lock keys. So the suspend code does > nothing - there are no lights to turn off. So far so good. > > Moving to the resume case: input_dev_resume() calls > input_reset_device(). This calls input_dev_toggle(dev, true), which > asks atkbd to turn the non-existent keyboard LEDs off. I guess this is > harmless and is unlikely to be the cause of the problems which led us > to need to disable this code in light of the following: > > After calling input_dev_toggle(), input_dev_resume() does this: > > /* > * Keys that have been pressed at suspend time are unlikely > * to be still pressed when we resume. > */ > spin_lock_irq(&dev->event_lock); > input_dev_release_keys(dev); > spin_unlock_irq(&dev->event_lock); > > We definitely don't want this to happen in our case, where we have > maintained full control and power of the device during suspend/resume. > > This particular important code snippet would not even have been > disabled by your suggestion of blocking data transfer at the > i8042-level. > > So, I think we need a new approach at solving this, and I think it has > to be one that interacts at the input_dev layer. OK, so we have the following scenario: - we do not need to worry about SND and LED on OLPC; - we do not need to worry about releasing the key that was pressed when we went to sleep because they system would not go to sleep while there is a key pressed. So the code in input_dev_resume() won't actually release any keys unless a key was held pressed and we forced system to sleep somehow. - we need to make sure we do not loose key press (and following events) when we are waking up. Can atkbd driver stash away serio byte data (when not in command mode) and replay it when pm notifier signals us that resume process is done? Thanks.
Hi, Sorry for the delay in replying to this - I took sime time off. On Wed, Aug 17, 2011 at 8:03 AM, Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > OK, so we have the following scenario: > > - we do not need to worry about SND and LED on OLPC; > > - we do not need to worry about releasing the key that was pressed > when we went to sleep because they system would not go to sleep while > there is a key pressed. So the code in input_dev_resume() won't > actually release any keys unless a key was held pressed and we forced > system to sleep somehow. > > - we need to make sure we do not loose key press (and following events) > when we are waking up. Yes, that seems accurate, with one more addition: - we need to make sure we do not loose mouse button press or mouse movement (and following events) when we are waking up. > Can atkbd driver stash away serio byte data (when not in command mode) > and replay it when pm notifier signals us that resume process is done? I'm having trouble understanding this suggestion or relating it to the problems in question and the previous patches posted. Are you suggesting stashing of data that is sent from the host to the keyboard, or from the keyboard to the host? How does this solve our issues? What about the mouse? How is it going to avoid the input-level issues? Thanks, Daniel
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; + 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); return 0; } diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c index 19cfc0c..4bb81c2 100644 --- a/drivers/input/keyboard/atkbd.c +++ b/drivers/input/keyboard/atkbd.c @@ -1027,6 +1027,7 @@ static void atkbd_set_keycode_table(struct atkbd *atkbd) static void atkbd_set_device_attrs(struct atkbd *atkbd) { struct input_dev *input_dev = atkbd->dev; + struct device *parent = &atkbd->ps2dev.serio->dev; int i; if (atkbd->extra) @@ -1047,7 +1048,8 @@ static void atkbd_set_device_attrs(struct atkbd *atkbd) input_dev->id.product = atkbd->translated ? 1 : atkbd->set; input_dev->id.version = atkbd->id; input_dev->event = atkbd_event; - input_dev->dev.parent = &atkbd->ps2dev.serio->dev; + input_dev->dev.parent = parent; + device_set_wakeup_capable(&input_dev->dev, device_can_wakeup(parent)); input_set_drvdata(input_dev, atkbd); diff --git a/drivers/input/mouse/hgpk.c b/drivers/input/mouse/hgpk.c index 95577c1..d5dd990 100644 --- a/drivers/input/mouse/hgpk.c +++ b/drivers/input/mouse/hgpk.c @@ -548,6 +548,8 @@ static void hgpk_setup_input_device(struct input_dev *input, input->phys = old_input->phys; input->id = old_input->id; input->dev.parent = old_input->dev.parent; + device_set_wakeup_capable(&input->dev, + device_can_wakeup(&old_input->dev)); } memset(input->evbit, 0, sizeof(input->evbit)); diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c index 3f74bae..de8ecd5 100644 --- a/drivers/input/mouse/psmouse-base.c +++ b/drivers/input/mouse/psmouse-base.c @@ -1232,8 +1232,10 @@ static int psmouse_switch_protocol(struct psmouse *psmouse, { const struct psmouse_protocol *selected_proto; struct input_dev *input_dev = psmouse->dev; + struct device *parent = &psmouse->ps2dev.serio->dev; - input_dev->dev.parent = &psmouse->ps2dev.serio->dev; + input_dev->dev.parent = parent; + device_set_wakeup_capable(&input_dev->dev, device_can_wakeup(parent)); input_dev->evbit[0] = BIT_MASK(EV_KEY) | BIT_MASK(EV_REL); input_dev->keybit[BIT_WORD(BTN_MOUSE)] = diff --git a/drivers/input/serio/i8042-io.h b/drivers/input/serio/i8042-io.h index 5d48bb6..296633c 100644 --- a/drivers/input/serio/i8042-io.h +++ b/drivers/input/serio/i8042-io.h @@ -92,4 +92,8 @@ static inline void i8042_platform_exit(void) #endif } +static inline void i8042_platform_suspend(struct device *dev, bool may_wakeup) +{ +} + #endif /* _I8042_IO_H */ diff --git a/drivers/input/serio/i8042-ip22io.h b/drivers/input/serio/i8042-ip22io.h index ee1ad27..c5b76a4 100644 --- a/drivers/input/serio/i8042-ip22io.h +++ b/drivers/input/serio/i8042-ip22io.h @@ -73,4 +73,8 @@ static inline void i8042_platform_exit(void) #endif } +static inline void i8042_platform_suspend(struct device *dev, bool may_wakeup) +{ +} + #endif /* _I8042_IP22_H */ diff --git a/drivers/input/serio/i8042-jazzio.h b/drivers/input/serio/i8042-jazzio.h index 13fd710..a11913a 100644 --- a/drivers/input/serio/i8042-jazzio.h +++ b/drivers/input/serio/i8042-jazzio.h @@ -66,4 +66,8 @@ static inline void i8042_platform_exit(void) #endif } +static inline void i8042_platform_suspend(struct device *dev, bool may_wakeup) +{ +} + #endif /* _I8042_JAZZ_H */ diff --git a/drivers/input/serio/i8042-ppcio.h b/drivers/input/serio/i8042-ppcio.h index f708c75..c9f4292 100644 --- a/drivers/input/serio/i8042-ppcio.h +++ b/drivers/input/serio/i8042-ppcio.h @@ -52,6 +52,10 @@ static inline void i8042_platform_exit(void) { } +static inline void i8042_platform_suspend(struct device *dev, bool may_wakeup) +{ +} + #else #include "i8042-io.h" diff --git a/drivers/input/serio/i8042-snirm.h b/drivers/input/serio/i8042-snirm.h index 409a934..96d034f 100644 --- a/drivers/input/serio/i8042-snirm.h +++ b/drivers/input/serio/i8042-snirm.h @@ -72,4 +72,8 @@ static inline void i8042_platform_exit(void) } +static inline void i8042_platform_suspend(struct device *dev, bool may_wakeup) +{ +} + #endif /* _I8042_SNIRM_H */ diff --git a/drivers/input/serio/i8042-sparcio.h b/drivers/input/serio/i8042-sparcio.h index 395a9af..e5381d3 100644 --- a/drivers/input/serio/i8042-sparcio.h +++ b/drivers/input/serio/i8042-sparcio.h @@ -154,4 +154,8 @@ static inline void i8042_platform_exit(void) } #endif /* !CONFIG_PCI */ +static inline void i8042_platform_suspend(struct device *dev, bool may_wakeup) +{ +} + #endif /* _I8042_SPARCIO_H */ diff --git a/drivers/input/serio/i8042-x86ia64io.h b/drivers/input/serio/i8042-x86ia64io.h index bb9f5d3..76b2e58 100644 --- a/drivers/input/serio/i8042-x86ia64io.h +++ b/drivers/input/serio/i8042-x86ia64io.h @@ -875,6 +875,10 @@ static inline int i8042_pnp_init(void) { return 0; } static inline void i8042_pnp_exit(void) { } #endif +static inline void i8042_platform_suspend(struct device *dev, bool may_wakeup) +{ +} + static int __init i8042_platform_init(void) { int retval; diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c index d37a48e..d275130 100644 --- a/drivers/input/serio/i8042.c +++ b/drivers/input/serio/i8042.c @@ -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; + error = i8042_controller_check(); if (error) return error; @@ -1129,6 +1137,7 @@ static int i8042_controller_resume(bool force_reset) if (i8042_ports[I8042_KBD_PORT_NO].serio) i8042_enable_kbd_port(); +soft: i8042_interrupt(0, NULL); return 0; @@ -1146,14 +1155,48 @@ static int i8042_pm_reset(struct device *dev) return 0; } +static int i8042_pm_suspend(struct device *dev) +{ + i8042_platform_suspend(dev, device_may_wakeup(dev)); + + /* + * If device is selected as a wakeup source, don't powerdown or reset + * during suspend. Just disable IRQs to ensure race-free resume. + */ + if (device_may_wakeup(dev)) { + if (i8042_kbd_irq_registered) + disable_irq(I8042_KBD_IRQ); + if (i8042_aux_irq_registered) + disable_irq(I8042_AUX_IRQ); + return 0; + } + + return i8042_pm_reset(dev); +} + 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. */ - return i8042_controller_resume(true); + if (device_may_wakeup(dev)) { + if (i8042_kbd_irq_registered) + enable_irq(I8042_KBD_IRQ); + if (i8042_aux_irq_registered) + enable_irq(I8042_AUX_IRQ); + } + + return r; } static int i8042_pm_thaw(struct device *dev) @@ -1165,11 +1208,11 @@ static int i8042_pm_thaw(struct device *dev) static int i8042_pm_restore(struct device *dev) { - return i8042_controller_resume(false); + return i8042_controller_resume(false, false); } static const struct dev_pm_ops i8042_pm_ops = { - .suspend = i8042_pm_reset, + .suspend = i8042_pm_suspend, .resume = i8042_pm_resume, .thaw = i8042_pm_thaw, .poweroff = i8042_pm_reset, @@ -1192,6 +1235,7 @@ static int __init i8042_create_kbd_port(void) { struct serio *serio; struct i8042_port *port = &i8042_ports[I8042_KBD_PORT_NO]; + struct device *parent = &i8042_platform_device->dev; serio = kzalloc(sizeof(struct serio), GFP_KERNEL); if (!serio) @@ -1203,7 +1247,8 @@ static int __init i8042_create_kbd_port(void) serio->stop = i8042_stop; serio->close = i8042_port_close; serio->port_data = port; - serio->dev.parent = &i8042_platform_device->dev; + serio->dev.parent = parent; + device_set_wakeup_capable(&serio->dev, device_can_wakeup(parent)); strlcpy(serio->name, "i8042 KBD port", sizeof(serio->name)); strlcpy(serio->phys, I8042_KBD_PHYS_DESC, sizeof(serio->phys)); @@ -1218,6 +1263,7 @@ static int __init i8042_create_aux_port(int idx) struct serio *serio; int port_no = idx < 0 ? I8042_AUX_PORT_NO : I8042_MUX_PORT_NO + idx; struct i8042_port *port = &i8042_ports[port_no]; + struct device *parent = &i8042_platform_device->dev; serio = kzalloc(sizeof(struct serio), GFP_KERNEL); if (!serio) @@ -1228,7 +1274,8 @@ static int __init i8042_create_aux_port(int idx) serio->start = i8042_start; serio->stop = i8042_stop; serio->port_data = port; - serio->dev.parent = &i8042_platform_device->dev; + serio->dev.parent = parent; + device_set_wakeup_capable(&serio->dev, device_can_wakeup(parent)); if (idx < 0) { strlcpy(serio->name, "i8042 AUX port", sizeof(serio->name)); strlcpy(serio->phys, I8042_AUX_PHYS_DESC, sizeof(serio->phys)); @@ -1403,6 +1450,9 @@ static int __init i8042_probe(struct platform_device *dev) i8042_dritek_enable(); #endif + if (i8042_enable_wakeup) + device_set_wakeup_capable(&dev->dev, true); + if (!i8042_noaux) { error = i8042_setup_aux(); if (error && error != -ENODEV && error != -EBUSY) diff --git a/drivers/input/serio/serio.c b/drivers/input/serio/serio.c index ba70058..9e2fdb4 100644 --- a/drivers/input/serio/serio.c +++ b/drivers/input/serio/serio.c @@ -931,7 +931,7 @@ static int serio_uevent(struct device *dev, struct kobj_uevent_env *env) #endif /* CONFIG_HOTPLUG */ #ifdef CONFIG_PM -static int serio_suspend(struct device *dev) +static int serio_poweroff(struct device *dev) { struct serio *serio = to_serio_port(dev); @@ -940,7 +940,16 @@ static int serio_suspend(struct device *dev) return 0; } -static int serio_resume(struct device *dev) +static int serio_suspend(struct device *dev) +{ + /* If configured as a wakeup source, don't power off. */ + if (device_may_wakeup(dev)) + return 0; + + return serio_poweroff(dev); +} + +static int serio_restore(struct device *dev) { struct serio *serio = to_serio_port(dev); @@ -953,11 +962,23 @@ static int serio_resume(struct device *dev) return 0; } +static int serio_resume(struct device *dev) +{ + /* + * If configured as a wakeup source, we didn't power off during + * suspend, and hence have nothing to do. + */ + if (device_may_wakeup(dev)) + return 0; + + return serio_restore(dev); +} + static const struct dev_pm_ops serio_pm_ops = { .suspend = serio_suspend, .resume = serio_resume, - .poweroff = serio_suspend, - .restore = serio_resume, + .poweroff = serio_poweroff, + .restore = serio_restore, }; #endif /* CONFIG_PM */
The OLPC XO laptop is able to use the PS/2 controller as a wakeup source. When used as a wakeup source, the key press/mouse motion must not be lost. Add infrastructure to allow controllers to be marked as wakeup-capable, and avoid doing power control/reset on the i8042/serio/input devices when running in this mode. For systems where this functionality is available, you are expected to enable wakeups on the i8042 device, the serio devices, and the relevant input devices, to ensure that the hardware is left powered and untouched throughout the suspend/resume. Signed-off-by: Daniel Drake <dsd@laptop.org> --- drivers/input/input.c | 6 +++- drivers/input/keyboard/atkbd.c | 4 ++- drivers/input/mouse/hgpk.c | 2 + drivers/input/mouse/psmouse-base.c | 4 ++- drivers/input/serio/i8042-io.h | 4 ++ drivers/input/serio/i8042-ip22io.h | 4 ++ drivers/input/serio/i8042-jazzio.h | 4 ++ drivers/input/serio/i8042-ppcio.h | 4 ++ drivers/input/serio/i8042-snirm.h | 4 ++ drivers/input/serio/i8042-sparcio.h | 4 ++ drivers/input/serio/i8042-x86ia64io.h | 4 ++ drivers/input/serio/i8042.c | 62 +++++++++++++++++++++++++++++--- drivers/input/serio/serio.c | 29 +++++++++++++-- 13 files changed, 122 insertions(+), 13 deletions(-) On original submission, Dmitry was worried about this functionality not working at all on other platforms. I agree, it will only work where the hardware has been specifically designed with this consideration. v2 of the patch therefore removes the module param option, meaning that it will only be activated on platforms that explicitly enable it at the code level. v2 also performs a more extensive job. We avoid resetting the device at the input_device level during suspend/resume. We also disable i8042 interrupts when going into suspend, to avoid races handling interrupts in the wrong order during resume. v3 includes a cleanup suggested by Rafael, and explains the corner case that we have to handle in the input layer. v4 changes direction a little: each layer now just looks at the wakeup capability of its own device, avoiding some of the earlier tree traversal. Userspace must now enable wakeups on 5 devices: i8042 i8042/serio0 i8042/serio0/input* i8042/serio1 i8042/serio1/input* This matches the behaviour of USB devices, where both the device and the parent controller must have wakeups enabled for wakeup functionality to work. Suggested by Rafael J. Wysocki.