| Message ID | 20090307203946.GA9942@srcf.ucam.org (mailing list archive) |
|---|---|
| State | Not Applicable, archived |
| Headers | show |
On Sat, Mar 7, 2009 at 8:39 PM, Matthew Garrett <mjg@redhat.com> wrote: > We don't have to at all - as far as I've been able to tell, the kernel > is utterly consistent in its current usage. The only drivers that emit > KEY_SLEEP are either embedded-specific (where it's clearly suspend to > RAM and not hibernate), the ACPI driver (where usage in other operating > systems is consistent with it being suspent to RAM) and the panasonic > and thinkpad drivers which use it consistently. If there's any > confusion, it's over the fact that KEY_SUSPEND is is used for suspend to > RAM in a (smaller) number of places. The fact that we're mapping x->y and y->x is the reason people keep getting it wrong. > I'd suggest reverting these current patches and doing something like the > following and then changing any drivers where it's worth clarifying > things. This has exactly the same effect with the advantage that no > userspace needs to be changed. So how do we specify a sleep button where policy is to be decided by userspace? For instance, the sleep button on a remote control? Do we hardcode policy in the kernel? > -#define KEY_SLEEP Â Â Â Â Â Â Â 142 Â Â /* SC System Sleep */ > +#define KEY_SUSPEND_TO_RAM Â Â 142 Â Â /* SC System Sleep */ I deliberately avoided using the terms DISK and RAM in the key defines used in my patch. > +/* Deprecated - use KEY_SUSPEND_TO_RAM and KEY_HIBERNATE instead */ > +#define KEY_SLEEP Â Â Â Â Â Â Â KEY_SUSPEND_TO_RAM > +#define KEY_SUSPEND Â Â Â Â Â Â KEY_HIBERNATE Yet more confusion. Can't we sort this mess out once and for all? If you're that bothered about userspace using the bodged mappings, I would suggest changing my patch so that KEY_SLEEP is 247, KEY_HIBERNATE is 205 and KEY_SUSPEND is 142. Richard. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Mar 08, 2009 at 08:45:59AM +0000, Richard Hughes wrote: > On Sat, Mar 7, 2009 at 8:39 PM, Matthew Garrett <mjg@redhat.com> wrote: > > We don't have to at all - as far as I've been able to tell, the kernel > > is utterly consistent in its current usage. The only drivers that emit > > KEY_SLEEP are either embedded-specific (where it's clearly suspend to > > RAM and not hibernate), the ACPI driver (where usage in other operating > > systems is consistent with it being suspent to RAM) and the panasonic > > and thinkpad drivers which use it consistently. If there's any > > confusion, it's over the fact that KEY_SUSPEND is is used for suspend to > > RAM in a (smaller) number of places. > > The fact that we're mapping x->y and y->x is the reason people keep > getting it wrong. Sure, doing things differently would have made sense several years ago when nobody was relying on this behaviour. We don't have that option now - making this change will break things, and we've got no idea how much it'll break. > > I'd suggest reverting these current patches and doing something like the > > following and then changing any drivers where it's worth clarifying > > things. This has exactly the same effect with the advantage that no > > userspace needs to be changed. > > So how do we specify a sleep button where policy is to be decided by > userspace? For instance, the sleep button on a remote control? Do we > hardcode policy in the kernel? Just carry on using KEY_SLEEP for that. We've already got the UI for it. > > -#define KEY_SLEEP Â Â Â Â Â Â Â 142 Â Â /* SC System Sleep */ > > +#define KEY_SUSPEND_TO_RAM Â Â 142 Â Â /* SC System Sleep */ > > I deliberately avoided using the terms DISK and RAM in the key defines > used in my patch. Why? This isn't user-visible. The aim is to reduce confusion amongst driver authors, not be consistent with userspace terminology. > > +/* Deprecated - use KEY_SUSPEND_TO_RAM and KEY_HIBERNATE instead */ > > +#define KEY_SLEEP Â Â Â Â Â Â Â KEY_SUSPEND_TO_RAM > > +#define KEY_SUSPEND Â Â Â Â Â Â KEY_HIBERNATE > > Yet more confusion. Can't we sort this mess out once and for all? If > you're that bothered about userspace using the bodged mappings, I > would suggest changing my patch so that KEY_SLEEP is 247, > KEY_HIBERNATE is 205 and KEY_SUSPEND is 142. No, then the behaviour of userspace would depend on whether it had been recompiled or not.
On Sunday 08 March 2009, Matthew Garrett wrote: > On Sun, Mar 08, 2009 at 08:45:59AM +0000, Richard Hughes wrote: > > On Sat, Mar 7, 2009 at 8:39 PM, Matthew Garrett <mjg@redhat.com> wrote: > > > We don't have to at all - as far as I've been able to tell, the kernel > > > is utterly consistent in its current usage. The only drivers that emit > > > KEY_SLEEP are either embedded-specific (where it's clearly suspend to > > > RAM and not hibernate), the ACPI driver (where usage in other operating > > > systems is consistent with it being suspent to RAM) and the panasonic > > > and thinkpad drivers which use it consistently. If there's any > > > confusion, it's over the fact that KEY_SUSPEND is is used for suspend to > > > RAM in a (smaller) number of places. > > > > The fact that we're mapping x->y and y->x is the reason people keep > > getting it wrong. > > Sure, doing things differently would have made sense several years ago > when nobody was relying on this behaviour. We don't have that option now > - making this change will break things, and we've got no idea how much > it'll break. Which is a good enough reason to avoid it. Alternatively, we can add completely new definitions _along_ _with_ the old ones, mark the old ones as obsolete (after some time) and try to make the user space start using the new ones only (that may be difficult, though). I said I liked the names, but I didn't realize that changing them would break things. Thanks, Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sun, Mar 08, 2009 at 09:56:45PM +0100, Rafael J. Wysocki wrote: > On Sunday 08 March 2009, Matthew Garrett wrote: > > On Sun, Mar 08, 2009 at 08:45:59AM +0000, Richard Hughes wrote: > > > On Sat, Mar 7, 2009 at 8:39 PM, Matthew Garrett <mjg@redhat.com> wrote: > > > > We don't have to at all - as far as I've been able to tell, the kernel > > > > is utterly consistent in its current usage. The only drivers that emit > > > > KEY_SLEEP are either embedded-specific (where it's clearly suspend to > > > > RAM and not hibernate), the ACPI driver (where usage in other operating > > > > systems is consistent with it being suspent to RAM) and the panasonic > > > > and thinkpad drivers which use it consistently. If there's any > > > > confusion, it's over the fact that KEY_SUSPEND is is used for suspend to > > > > RAM in a (smaller) number of places. > > > > > > The fact that we're mapping x->y and y->x is the reason people keep > > > getting it wrong. > > > > Sure, doing things differently would have made sense several years ago > > when nobody was relying on this behaviour. We don't have that option now > > - making this change will break things, and we've got no idea how much > > it'll break. > > Which is a good enough reason to avoid it. > > Alternatively, we can add completely new definitions _along_ _with_ the old > ones, mark the old ones as obsolete (after some time) and try to make the > user space start using the new ones only (that may be difficult, though). > > I said I liked the names, but I didn't realize that changing them would break > things. > I don't think we want to break anything if we can help it. The problem with Richard's patch is that it changes meaning of KEY_SUSPEND from STD to STR. I would prefer if we could do the following: - KEY_SLEEP - leave the keycode, the action should be the default system state defined by either platform or user. I expect that the vast majority of system have default state similar to S3 so there should not be anysurprises. - KEY_SUSPEND - provide better comment for its intended usage and maybe add KEY_HIBERNATE alias. - KEY_SUSPEND2RAM - add a new definition. Do you think this would this work? I intend to back out the patch in question for the time being. Thanks.
On Sun, Mar 08, 2009 at 04:07:50PM -0700, Dmitry Torokhov wrote: > - KEY_SLEEP - leave the keycode, the action should be the default > system state defined by either platform or user. I expect that the vast > majority of system have default state similar to S3 so there should not > be anysurprises. > > - KEY_SUSPEND - provide better comment for its intended usage and maybe > add KEY_HIBERNATE alias. > > - KEY_SUSPEND2RAM - add a new definition. > > Do you think this would this work? I think that would be fine, though I'm not entirely convinced that we need KEY_SUSPEND2RAM as a separate keycode.
Dmitry Torokhov wrote: > On Sun, Mar 08, 2009 at 09:56:45PM +0100, Rafael J. Wysocki wrote: >> On Sunday 08 March 2009, Matthew Garrett wrote: >> > On Sun, Mar 08, 2009 at 08:45:59AM +0000, Richard Hughes wrote: >> > > On Sat, Mar 7, 2009 at 8:39 PM, Matthew Garrett <mjg@redhat.com> >> > > wrote: >> > > > We don't have to at all - as far as I've been able to tell, the >> > > > kernel is utterly consistent in its current usage. The only drivers >> > > > that emit KEY_SLEEP are either embedded-specific (where it's >> > > > clearly suspend to RAM and not hibernate), the ACPI driver (where >> > > > usage in other operating systems is consistent with it being >> > > > suspent to RAM) and the panasonic and thinkpad drivers which use it >> > > > consistently. If there's any confusion, it's over the fact that >> > > > KEY_SUSPEND is is used for suspend to RAM in a (smaller) number of >> > > > places. >> > > >> > > The fact that we're mapping x->y and y->x is the reason people keep >> > > getting it wrong. >> > >> > Sure, doing things differently would have made sense several years ago >> > when nobody was relying on this behaviour. We don't have that option >> > now - making this change will break things, and we've got no idea how >> > much it'll break. >> >> Which is a good enough reason to avoid it. >> >> Alternatively, we can add completely new definitions _along_ _with_ the >> old ones, mark the old ones as obsolete (after some time) and try to make >> the user space start using the new ones only (that may be difficult, >> though). >> >> I said I liked the names, but I didn't realize that changing them would >> break things. >> > > I don't think we want to break anything if we can help it. The problem > with Richard's patch is that it changes meaning of KEY_SUSPEND from STD > to STR. I would prefer if we could do the following: > > - KEY_SLEEP - leave the keycode, the action should be the default > system state defined by either platform or user. I expect that the vast > majority of system have default state similar to S3 so there should not > be anysurprises. > > - KEY_SUSPEND - provide better comment for its intended usage and maybe > add KEY_HIBERNATE alias. > > - KEY_SUSPEND2RAM - add a new definition. > > Do you think this would this work? > > I intend to back out the patch in question for the time being. > What about systems that have clear indication of STR and STD on respective keys? Should they continue to return KEY_SLEEP for STR? I think this conflicts with definition above. So in the long run those should be changed to return KEY_SUSPEND2RAM it seems. -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Mon, Mar 09, 2009 at 11:31:36AM +0300, Andrey Borzenkov wrote: > What about systems that have clear indication of STR and STD on respective > keys? Should they continue to return KEY_SLEEP for STR? I think this > conflicts with definition above. So in the long run those should be changed to > return KEY_SUSPEND2RAM it seems. Perhaps, but at present they KEY_SLEEP is probably adequate. KEY_SLEEP is always going to default to suspend to RAM, so really the only distinction between it and a dedicated suspend to RAM key is whether a UI element affects its behaviour or not.
On Sun, Mar 08, 2009 at 11:19:35PM +0000, Matthew Garrett wrote: > On Sun, Mar 08, 2009 at 04:07:50PM -0700, Dmitry Torokhov wrote: > > > - KEY_SLEEP - leave the keycode, the action should be the default > > system state defined by either platform or user. I expect that the vast > > majority of system have default state similar to S3 so there should not > > be anysurprises. > > > > - KEY_SUSPEND - provide better comment for its intended usage and maybe > > add KEY_HIBERNATE alias. > > > > - KEY_SUSPEND2RAM - add a new definition. > > > > Do you think this would this work? > > I think that would be fine, though I'm not entirely convinced that we > need KEY_SUSPEND2RAM as a separate keycode. If you think this using common sense I think the following would be the most obvious mapping: sleep = STR, hibernate = STD, suspend = preferred suspend mode Both sleep and hibernate are biological processes and the difference between them should be obvious for everyone. Suspend on that other hand just says that your work is suspended. I don't know if common sense has any room in these discussions though. I would at least avoid mixing the suspend2ram and hibernate definitions. The first one comes from the technical aspect and the second comes from the biological aspect so mixing them IMO just creates more confusion. Of course the API is a technical thing anyway so perhaps just go for something explicit like suspend2ram, suspend2disk and suspend2preferred. The old definitions would of course need to be mapped to the new definitions somehow.
On Mon, Mar 09, 2009 at 03:52:42PM +0200, Ville Syrjälä wrote: > If you think this using common sense I think the following would be the > most obvious mapping: > sleep = STR, hibernate = STD, suspend = preferred suspend mode I agree, but this isn't a discussion about user-visible nomenclature - it's a discussion about using the keycodes we have in the kernel. The aim is to ensure that everyone in-kernel uses the correct codes, and if we can do that without breaking existing userspace (even if it means the nomenclature differs) then that's preferable.
On Mon, Mar 09, 2009 at 02:00:09PM +0000, Matthew Garrett wrote: > On Mon, Mar 09, 2009 at 03:52:42PM +0200, Ville Syrjälä wrote: > > > If you think this using common sense I think the following would be the > > most obvious mapping: > > sleep = STR, hibernate = STD, suspend = preferred suspend mode > > I agree, but this isn't a discussion about user-visible nomenclature - > it's a discussion about using the keycodes we have in the kernel. The > aim is to ensure that everyone in-kernel uses the correct codes, and if > we can do that without breaking existing userspace (even if it means the > nomenclature differs) then that's preferable. I don't think that invalidates my point. There's always the possibility of re-creating the same mess in the future if the definitions aren't crystal clear. The less room there is for personal interpretation of the definitions the better. I understand that backwards compatibility is critical but perhaps that is reason enough to create a completely a new set of definitions and just map the old definitions in the best way possible. Something like this? KEY_SUSPEND_TO_RAM KEY_SUSPEND_TO_DISK KEY_SUSPEND_TO_PREFERRED
diff --git a/include/linux/input.h b/include/linux/input.h index 1249a0c..90abe27 100644 --- a/include/linux/input.h +++ b/include/linux/input.h @@ -263,7 +263,7 @@ struct input_absinfo { #define KEY_MENU 139 /* Menu (show menu) */ #define KEY_CALC 140 /* AL Calculator */ #define KEY_SETUP 141 -#define KEY_SLEEP 142 /* SC System Sleep */ +#define KEY_SUSPEND_TO_RAM 142 /* SC System Sleep */ #define KEY_WAKEUP 143 /* System Wake Up */ #define KEY_FILE 144 /* AL Local Machine Browser */ #define KEY_SENDFILE 145 @@ -324,7 +324,7 @@ struct input_absinfo { #define KEY_PROG3 202 #define KEY_PROG4 203 #define KEY_DASHBOARD 204 /* AL Dashboard */ -#define KEY_SUSPEND 205 +#define KEY_HIBERNATE 205 #define KEY_CLOSE 206 /* AC Close */ #define KEY_PLAY 207 #define KEY_FASTFORWARD 208 @@ -377,6 +377,10 @@ struct input_absinfo { /* Range 248 - 255 is reserved for special needs of AT keyboard driver */ +/* Deprecated - use KEY_SUSPEND_TO_RAM and KEY_HIBERNATE instead */ +#define KEY_SLEEP KEY_SUSPEND_TO_RAM +#define KEY_SUSPEND KEY_HIBERNATE + #define BTN_MISC 0x100 #define BTN_0 0x100 #define BTN_1 0x101