Re: [PATCH v4] drivers/tty: Folding Android's keyreset driver in sysRQ
On Wed, Feb 27, 2013 at 10:58:44AM -0700, Mathieu Poirier wrote: > On 13-02-27 09:57 AM, Linus Torvalds wrote: > > On Tue, Feb 26, 2013 at 11:33 PM, Dave Airlie wrote: > >> > >> It looks to me like the weak bit isn't working so well > >> > >> if (platform_sysrq_reset_seq) { > >> for (i = 0; i < ARRAY_SIZE(sysrq_reset_seq); i++) { > >> key = platform_sysrq_reset_seq[i]; > >> 6d: 66 8b 8c 00 00 00 00mov0x0(%eax,%eax,1),%cx > >> 74: 00 > >> > >> is around where it craps out. > >> gcc version 4.7.2 20121109 (Red Hat 4.7.2-8) (GCC) > >> Fedora 18 machine. > > > > Hmm. I would love to blame gcc, but no, I think the code is crap. > > > > The whole 'platform_sysrq_reset_seq[]' thing is broken in current git, > > and it apparently only happens to work by mistake for most of us. > > > > Doing a "grep" for it shows all three uses: > > > >git grep platform_sysrq_reset_seq > > > > extern unsigned short platform_sysrq_reset_seq[] __weak; > > if (platform_sysrq_reset_seq) { > > key = platform_sysrq_reset_seq[i]; > > > > and the thing is, if it is declared as an array (not a pointer), then > > I think it is perfectly understandable that when then testing the > > *address* of that array, gcc just says "you're stupid, you're testing > > something that cannot possibly be NULL, so I'll throw your idiotic > > test away". > > > > And gcc would be completely correct. That test is moronic. You just > > said that platform_sysrq_reset_seq[] was an external array, there is > > no way in hell that is NULL. > > > > Now, if it was a _pointer_, that would be a different thing entirely. > > A pointer can have a NULL value. A named array, not so much. > > > > So I *think* the fix might be something like the attached. Totally > > untested. It may compile, or it may not. > > > > Linus > > > > Your fix is compiling, running and yielding the correct results - > apologies about that. > Actually defining platform_sysrq_reset_seq as __weak array was my doing so the fault is actually mine. Mathieu had a function there originally. Thanks. -- Dmitry -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] drivers/tty: Folding Android's keyreset driver in sysRQ
Linus Torvalds wrote: > > Your fix is compiling, running and yielding the correct results - > > apologies about that. > > > > Acked-by: Mathieu Poirier > > Ok, good. Committed and pushed out. Adding rmk and dhowells to the cc > just to let them know, since Dave said they were debugging this on > irc. Seems reasonable to me. After dissecting stuff this morning, we noted that the code dump in the oops shows that the MOV insn that gets the value to test does not load 0. The first five bytes of: ba 00 00 40 ff | 31 c0 | 8b 1d 0c 0e a0 correspond to this: mov $0xff40,%edx when the linker should've made it this: mov $0x,%edx I suspect that the number may be some metadata reference that didn't get substituted. Dave Airlie tried disassembling his vmlinux with gdb - and that apparently did show a load of 0, but the EIP in his oops doesn't match the addresses from gdb's disassembly - so I'm not sure what's happening there. In the oops, the %cr2 value was 0xff40, but the offending instruction bytes of the actual faulting instruction in the code dump were cropped by his photo of the screen. David -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] drivers/tty: Folding Android's keyreset driver in sysRQ
On Wed, Feb 27, 2013 at 9:58 AM, Mathieu Poirier wrote: > > Your fix is compiling, running and yielding the correct results - > apologies about that. > > Acked-by: Mathieu Poirier Ok, good. Committed and pushed out. Adding rmk and dhowells to the cc just to let them know, since Dave said they were debugging this on irc. Linus -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] drivers/tty: Folding Android's keyreset driver in sysRQ
On 13-02-27 09:57 AM, Linus Torvalds wrote: > On Tue, Feb 26, 2013 at 11:33 PM, Dave Airlie wrote: >> >> It looks to me like the weak bit isn't working so well >> >> if (platform_sysrq_reset_seq) { >> for (i = 0; i < ARRAY_SIZE(sysrq_reset_seq); i++) { >> key = platform_sysrq_reset_seq[i]; >> 6d: 66 8b 8c 00 00 00 00mov0x0(%eax,%eax,1),%cx >> 74: 00 >> >> is around where it craps out. >> gcc version 4.7.2 20121109 (Red Hat 4.7.2-8) (GCC) >> Fedora 18 machine. > > Hmm. I would love to blame gcc, but no, I think the code is crap. > > The whole 'platform_sysrq_reset_seq[]' thing is broken in current git, > and it apparently only happens to work by mistake for most of us. > > Doing a "grep" for it shows all three uses: > >git grep platform_sysrq_reset_seq > > extern unsigned short platform_sysrq_reset_seq[] __weak; > if (platform_sysrq_reset_seq) { > key = platform_sysrq_reset_seq[i]; > > and the thing is, if it is declared as an array (not a pointer), then > I think it is perfectly understandable that when then testing the > *address* of that array, gcc just says "you're stupid, you're testing > something that cannot possibly be NULL, so I'll throw your idiotic > test away". > > And gcc would be completely correct. That test is moronic. You just > said that platform_sysrq_reset_seq[] was an external array, there is > no way in hell that is NULL. > > Now, if it was a _pointer_, that would be a different thing entirely. > A pointer can have a NULL value. A named array, not so much. > > So I *think* the fix might be something like the attached. Totally > untested. It may compile, or it may not. > > Linus > Your fix is compiling, running and yielding the correct results - apologies about that. Acked-by: Mathieu Poirier -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] drivers/tty: Folding Android's keyreset driver in sysRQ
On Tue, Feb 26, 2013 at 11:33 PM, Dave Airlie wrote: > > It looks to me like the weak bit isn't working so well > > if (platform_sysrq_reset_seq) { > for (i = 0; i < ARRAY_SIZE(sysrq_reset_seq); i++) { > key = platform_sysrq_reset_seq[i]; > 6d: 66 8b 8c 00 00 00 00mov0x0(%eax,%eax,1),%cx > 74: 00 > > is around where it craps out. > gcc version 4.7.2 20121109 (Red Hat 4.7.2-8) (GCC) > Fedora 18 machine. Hmm. I would love to blame gcc, but no, I think the code is crap. The whole 'platform_sysrq_reset_seq[]' thing is broken in current git, and it apparently only happens to work by mistake for most of us. Doing a "grep" for it shows all three uses: git grep platform_sysrq_reset_seq extern unsigned short platform_sysrq_reset_seq[] __weak; if (platform_sysrq_reset_seq) { key = platform_sysrq_reset_seq[i]; and the thing is, if it is declared as an array (not a pointer), then I think it is perfectly understandable that when then testing the *address* of that array, gcc just says "you're stupid, you're testing something that cannot possibly be NULL, so I'll throw your idiotic test away". And gcc would be completely correct. That test is moronic. You just said that platform_sysrq_reset_seq[] was an external array, there is no way in hell that is NULL. Now, if it was a _pointer_, that would be a different thing entirely. A pointer can have a NULL value. A named array, not so much. So I *think* the fix might be something like the attached. Totally untested. It may compile, or it may not. Linus patch.diff Description: Binary data
Re: [PATCH v4] drivers/tty: Folding Android's keyreset driver in sysRQ
It looks to me like the weak bit isn't working so well if (platform_sysrq_reset_seq) { for (i = 0; i < ARRAY_SIZE(sysrq_reset_seq); i++) { key = platform_sysrq_reset_seq[i]; 6d: 66 8b 8c 00 00 00 00mov0x0(%eax,%eax,1),%cx 74: 00 is around where it craps out. gcc version 4.7.2 20121109 (Red Hat 4.7.2-8) (GCC) Fedora 18 machine. >>> >>> And just to confirm reverting >>> 154b7a489a5b1d808323b933b04864958c2f1056.in Linus' tree allows boot to >>> proceed. >> >> >> Looks like my first picture bounced, >> >> here's a link >> http://www.skynet.ie/~airlied/sysrq_oops.jpg > > Just some more disasm: > >if (platform_sysrq_reset_seq) { > 4c: ba 00 00 00 00 mov$0x0,%edx > 51: 31 c0 xor%eax,%eax > 53: 8b 1d 04 00 00 00 mov0x4,%ebx > 59: 85 d2 test %edx,%edx > 5b: 75 2d jne8a > > sysrq_reset_seq[sysrq_reset_seq_len++] = key; > } > } > > error = input_register_handler(&sysrq_handler); > 5d: b8 b4 00 00 00 mov$0xb4,%eax > 62: e8 fc ff ff ff call 63 > if (error) > 67: 85 c0 test %eax,%eax > 69: 74 3e je a9 > 6b: eb 2d jmp9a > int error; > int i; > 6d: 66 8b 8c 00 00 00 00mov0x0(%eax,%eax,1),%cx > 74: 00 > if (key == KEY_RESERVED || key > KEY_MAX) > 75: 8d 71 fflea-0x1(%ecx),%esi > 78: 66 81 fe fe 02 cmp$0x2fe,%si > 7d: 76 02 jbe81 > 7f: eb 11 jmp92 > break; > > sysrq_reset_seq[sysrq_reset_seq_len++] = key; > 81: 66 89 8c 12 08 00 00mov%cx,0x8(%edx,%edx,1) > 88: 00 > > Dave. It really does look like gcc is screwing up here, after a debug session on irc with dhowells and rmk. I'm going to sleep, its too much for my brain at the moment. Dave. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] drivers/tty: Folding Android's keyreset driver in sysRQ
On Wed, Feb 27, 2013 at 9:06 PM, Dave Airlie wrote: > On Wed, Feb 27, 2013 at 6:26 PM, Dave Airlie wrote: >>> >>> this patch seems to be oopsing on my x86 32-bit machine on bootup, pic >>> attached. >>> >>> .config attached. >>> >>> It looks to me like the weak bit isn't working so well >>> >>> if (platform_sysrq_reset_seq) { >>> for (i = 0; i < ARRAY_SIZE(sysrq_reset_seq); i++) { >>> key = platform_sysrq_reset_seq[i]; >>> 6d: 66 8b 8c 00 00 00 00mov0x0(%eax,%eax,1),%cx >>> 74: 00 >>> >>> is around where it craps out. >>> gcc version 4.7.2 20121109 (Red Hat 4.7.2-8) (GCC) >>> Fedora 18 machine. >> >> And just to confirm reverting >> 154b7a489a5b1d808323b933b04864958c2f1056.in Linus' tree allows boot to >> proceed. > > > Looks like my first picture bounced, > > here's a link > http://www.skynet.ie/~airlied/sysrq_oops.jpg Just some more disasm: if (platform_sysrq_reset_seq) { 4c: ba 00 00 00 00 mov$0x0,%edx 51: 31 c0 xor%eax,%eax 53: 8b 1d 04 00 00 00 mov0x4,%ebx 59: 85 d2 test %edx,%edx 5b: 75 2d jne8a sysrq_reset_seq[sysrq_reset_seq_len++] = key; } } error = input_register_handler(&sysrq_handler); 5d: b8 b4 00 00 00 mov$0xb4,%eax 62: e8 fc ff ff ff call 63 if (error) 67: 85 c0 test %eax,%eax 69: 74 3e je a9 6b: eb 2d jmp9a int error; int i; 6d: 66 8b 8c 00 00 00 00mov0x0(%eax,%eax,1),%cx 74: 00 if (key == KEY_RESERVED || key > KEY_MAX) 75: 8d 71 fflea-0x1(%ecx),%esi 78: 66 81 fe fe 02 cmp$0x2fe,%si 7d: 76 02 jbe81 7f: eb 11 jmp92 break; sysrq_reset_seq[sysrq_reset_seq_len++] = key; 81: 66 89 8c 12 08 00 00mov%cx,0x8(%edx,%edx,1) 88: 00 Dave. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] drivers/tty: Folding Android's keyreset driver in sysRQ
On Wed, Feb 27, 2013 at 6:26 PM, Dave Airlie wrote: >> >> this patch seems to be oopsing on my x86 32-bit machine on bootup, pic >> attached. >> >> .config attached. >> >> It looks to me like the weak bit isn't working so well >> >> if (platform_sysrq_reset_seq) { >> for (i = 0; i < ARRAY_SIZE(sysrq_reset_seq); i++) { >> key = platform_sysrq_reset_seq[i]; >> 6d: 66 8b 8c 00 00 00 00mov0x0(%eax,%eax,1),%cx >> 74: 00 >> >> is around where it craps out. >> gcc version 4.7.2 20121109 (Red Hat 4.7.2-8) (GCC) >> Fedora 18 machine. > > And just to confirm reverting > 154b7a489a5b1d808323b933b04864958c2f1056.in Linus' tree allows boot to > proceed. Looks like my first picture bounced, here's a link http://www.skynet.ie/~airlied/sysrq_oops.jpg Dave. (this time with reply-all). -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] drivers/tty: Folding Android's keyreset driver in sysRQ
> > this patch seems to be oopsing on my x86 32-bit machine on bootup, pic > attached. > > .config attached. > > It looks to me like the weak bit isn't working so well > > if (platform_sysrq_reset_seq) { > for (i = 0; i < ARRAY_SIZE(sysrq_reset_seq); i++) { > key = platform_sysrq_reset_seq[i]; > 6d: 66 8b 8c 00 00 00 00mov0x0(%eax,%eax,1),%cx > 74: 00 > > is around where it craps out. > gcc version 4.7.2 20121109 (Red Hat 4.7.2-8) (GCC) > Fedora 18 machine. And just to confirm reverting 154b7a489a5b1d808323b933b04864958c2f1056.in Linus' tree allows boot to proceed. Dave. -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/
Re: [PATCH v4] drivers/tty: Folding Android's keyreset driver in sysRQ
Hi Mathieu, On Sun, Nov 11, 2012 at 01:24:48PM -0700, mathieu.poir...@linaro.org wrote: > From: "Mathieu J. Poirier" > > This patch adds keyreset functionality to the sysrq driver. It > allows certain button/key combinations to be used in order to > trigger device resets. > > The first time the key-combo is detected a work function that syncs > the filesystems is scheduled and the kernel rebooted. If all the keys > are released and then pressed again, it calls panic. Reboot on panic > should be set for this to work. > > Redefining the '__weak sysrq_keyreset_get_params' function is required > to trigger the feature. Alternatively keys can be passed to the > driver via the "/sys/module/sysrq" interface. > > This functionality comes from the keyreset driver submitted by > Arve Hjønnevåg in the Android kernel. Thank you for making the changes. This still looks pretty complicated, how about if we trim it a bit, like in the patch below. Thanks. -- Dmitry Input: sysrq - allow specifying alternate reset sequence From: Mathieu J. Poirier This patch adds keyreset functionality to the sysrq driver. It allows certain button/key combinations to be used in order to trigger emergency reboots. Redefining the '__weak platform_sysrq_reset_seq' variable is required to trigger the feature. Alternatively keys can be passed to the driver via a module parameter. This functionality comes from the keyreset driver submitted by Arve Hjønnevåg in the Android kernel. Signed-off-by: Mathieu Poirier Signed-off-by: Dmitry Torokhov --- drivers/tty/sysrq.c | 114 +++ 1 file changed, 114 insertions(+) diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c index 16ee6ce..9dddaf7 100644 --- a/drivers/tty/sysrq.c +++ b/drivers/tty/sysrq.c @@ -41,6 +41,7 @@ #include #include #include +#include #include #include @@ -576,8 +577,73 @@ struct sysrq_state { bool active; bool need_reinject; bool reinjecting; + + /* reset sequence handling */ + bool reset_canceled; + unsigned long reset_keybit[BITS_TO_LONGS(KEY_CNT)]; + int reset_seq_len; + int reset_seq_cnt; + int reset_seq_version; }; +#define SYSRQ_KEY_RESET_MAX20 /* Should be plenty */ +static unsigned short sysrq_reset_seq[SYSRQ_KEY_RESET_MAX]; +static unsigned int sysrq_reset_seq_len; +static unsigned int sysrq_reset_seq_version = 1; + +static void sysrq_parse_reset_sequence(struct sysrq_state *state) +{ + int i; + unsigned short key; + + state->reset_seq_cnt = 0; + + for (i = 0; i < sysrq_reset_seq_len; i++) { + key = sysrq_reset_seq[i]; + + if (key == KEY_RESERVED || key > KEY_MAX) + break; + + __set_bit(key, state->reset_keybit); + state->reset_seq_len++; + + if (test_bit(key, state->key_down)) + state->reset_seq_cnt++; + } +} + +static bool sysrq_detect_reset_sequence(struct sysrq_state *state, + unsigned int code, int value) +{ + if (state->reset_seq_version != sysrq_reset_seq_version) { + sysrq_parse_reset_sequence(state); + /* Disable reset until old keys are not released */ + state->reset_canceled = state->reset_seq_cnt != 0; + state->reset_seq_version = sysrq_reset_seq_version; + } + + if (!test_bit(code, state->reset_keybit)) { + /* +* Pressing any key _not_ in reset sequence cancels +* the reset sequence. +*/ + if (value && state->reset_seq_cnt) + state->reset_canceled = true; + } else if (value == 0) { + /* key release */ + if (--state->reset_seq_cnt == 0) + state->reset_canceled = false; + } else if (value == 1) { + /* key press, not autorepeat */ + if (++state->reset_seq_cnt == state->reset_seq_len && + !state->reset_canceled) { + return true; + } + } + + return false; +} + static void sysrq_reinject_alt_sysrq(struct work_struct *work) { struct sysrq_state *sysrq = @@ -690,6 +756,11 @@ static bool sysrq_filter(struct input_handle *handle, if (was_active) schedule_work(&sysrq->reinject_work); + if (sysrq_detect_reset_sequence(sysrq, code, value)) { + /* Force emergency reboot */ + __handle_sysrq(sysrq_xlate[KEY_B], false); + } + } else if (value == 0 && test_and_clear_bit(code, sysrq->key_down)) { /* @@ -785,7 +856,20 @@ static bool sysrq_handler_registered; static inline void sysrq_register_hand
[PATCH v4] drivers/tty: Folding Android's keyreset driver in sysRQ
From: "Mathieu J. Poirier" This patch adds keyreset functionality to the sysrq driver. It allows certain button/key combinations to be used in order to trigger device resets. The first time the key-combo is detected a work function that syncs the filesystems is scheduled and the kernel rebooted. If all the keys are released and then pressed again, it calls panic. Reboot on panic should be set for this to work. Redefining the '__weak sysrq_keyreset_get_params' function is required to trigger the feature. Alternatively keys can be passed to the driver via the "/sys/module/sysrq" interface. This functionality comes from the keyreset driver submitted by Arve Hjønnevåg in the Android kernel. Cc: a...@android.com Cc: kernel-t...@android.com Cc: dmitry.torok...@gmail.com Cc: john.stu...@linaro.org Cc: a...@lxorguk.ukuu.org.uk Signed-off-by: Mathieu Poirier --- drivers/tty/sysrq.c | 303 + include/linux/sysrq.h |2 + 2 files changed, 305 insertions(+), 0 deletions(-) diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c index 05728894..da4f538 100644 --- a/drivers/tty/sysrq.c +++ b/drivers/tty/sysrq.c @@ -41,14 +41,29 @@ #include #include #include +#include +#include +#include +#include #include #include +#define KEY_RESET_MAX 20 /* how many is enough ? */ +int keyreset_param[KEY_RESET_MAX]; +struct mutex sysrq_mutex; +static struct sysrq_state *sysrq_handle; + /* Whether we react on sysrq keys or just ignore them */ static int __read_mostly sysrq_enabled = SYSRQ_DEFAULT_ENABLE; static bool __read_mostly sysrq_always_enabled; +static struct input_handler sysrq_handler; + +/* Keep track of what has been called */ +static atomic_t restart_requested; + + static bool sysrq_on(void) { return sysrq_enabled || sysrq_always_enabled; @@ -570,6 +585,15 @@ struct sysrq_state { struct input_handle handle; struct work_struct reinject_work; unsigned long key_down[BITS_TO_LONGS(KEY_CNT)]; + unsigned long keybit[BITS_TO_LONGS(KEY_CNT)]; + unsigned long upbit[BITS_TO_LONGS(KEY_CNT)]; + unsigned long key[BITS_TO_LONGS(KEY_CNT)]; + int (*reset_fn)(void); + int key_down_target; + int key_down_ctn; + int key_up_ctn; + int keyreset_data; + int restart_disabled; unsigned int alt; unsigned int alt_use; bool active; @@ -603,6 +627,80 @@ static void sysrq_reinject_alt_sysrq(struct work_struct *work) } } +static void deferred_restart(struct work_struct *dummy) +{ + atomic_inc(&restart_requested); + sys_sync(); + atomic_inc(&restart_requested); + kernel_restart(NULL); +} +static DECLARE_WORK(restart_work, deferred_restart); + +static int do_keyreset_event(struct sysrq_state *state, +unsigned int code, int value) +{ + int ret; + int processed = 0; + + mutex_lock(&sysrq_mutex); + + /* Is the code of interest to us */ + if (!test_bit(code, state->keybit)) { + mutex_unlock(&sysrq_mutex); + return processed; + } + + /* No need to take care of key up events */ + if (!test_bit(code, state->key) == !value) { + mutex_unlock(&sysrq_mutex); + return processed; + } + + /* Record new entry */ + __change_bit(code, state->key); + + processed = 1; + + if (test_bit(code, state->upbit)) { + if (value) { + state->restart_disabled = 1; + state->key_up_ctn++; + } else + state->key_up_ctn--; + } else { + if (value) + state->key_down_ctn++; + else + state->key_down_ctn--; + } + + if (state->key_down_ctn == 0 && state->key_up_ctn == 0) + state->restart_disabled = 0; + + if (value && !state->restart_disabled && + state->key_down_ctn == state->key_down_target) { + state->restart_disabled = 1; + if (atomic_read(&restart_requested)) + panic("keyboard reset failed, %d - panic\n", +atomic_read(&restart_requested)); + if (state->reset_fn) { + ret = state->reset_fn(); + atomic_set(&restart_requested, ret); + } else { + pr_info("keyboard reset\n"); + schedule_work(&restart_work); + atomic_inc(&restart_requested); + } + } + + mutex_unlock(&sysrq_mutex); + + /* no need to suppress keyreset characters */ + state->active = false; + + return processed; +} + static bool sysrq_filter(struct input_handle *handle, unsigned int type, unsigned int code, int value)