[PATCH v2] sysrq: supplementing reset sequence with timeout functionality

[PATCH v2] sysrq: supplementing reset sequence with timeout functionality

[mathieu.poirier]

From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>

Some devices have too few buttons, which it makes it hard to have
a reset combo that won't trigger automatically.  As such a
timeout functionality that requires the combination to be held for
a given amount of time before triggering is introduced.

If a key combo is recognized and held for a 'timeout' amount of time,
the system triggers a reset.  If the timeout value is omitted the
driver simply ignores the functionality.

Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
Changes for v2:
   - Replaced delayed_work with timer.
   - Made timer on per-device basis.
   - Removed unecessary parameter check macro.

 drivers/tty/sysrq.c |   40 +++++++++++++++++++++++++++++-----------
 1 file changed, 29 insertions(+), 11 deletions(-)

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 7953dfa..8e62062 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -42,6 +42,7 @@
 #include <linux/input.h>
 #include <linux/uaccess.h>
 #include <linux/moduleparam.h>
+#include <linux/jiffies.h>
 
 #include <asm/ptrace.h>
 #include <asm/irq_regs.h>
@@ -50,6 +51,9 @@
 static int __read_mostly sysrq_enabled = SYSRQ_DEFAULT_ENABLE;
 static bool __read_mostly sysrq_always_enabled;
 
+unsigned short platform_sysrq_reset_seq[] __weak = { KEY_RESERVED };
+int sysrq_downtime_ms __weak;
+
 static bool sysrq_on(void)
 {
 	return sysrq_enabled || sysrq_always_enabled;
@@ -584,6 +588,7 @@ struct sysrq_state {
 	int reset_seq_len;
 	int reset_seq_cnt;
 	int reset_seq_version;
+	struct timer_list keyreset_timeout;
 };
 
 #define SYSRQ_KEY_RESET_MAX	20 /* Should be plenty */
@@ -617,7 +622,14 @@ static void sysrq_parse_reset_sequence(struct sysrq_state *state)
 	state->reset_seq_version = sysrq_reset_seq_version;
 }
 
-static bool sysrq_detect_reset_sequence(struct sysrq_state *state,
+static void sysrq_handle_reset_request(struct sysrq_state *state)
+{
+	unsigned long expires = jiffies + msecs_to_jiffies(sysrq_downtime_ms);
+	state->keyreset_timeout.expires = expires;
+	add_timer(&state->keyreset_timeout);
+}
+
+static void sysrq_detect_reset_sequence(struct sysrq_state *state,
 					unsigned int code, int value)
 {
 	if (!test_bit(code, state->reset_keybit)) {
@@ -629,17 +641,22 @@ static bool sysrq_detect_reset_sequence(struct sysrq_state *state,
 			state->reset_canceled = true;
 	} else if (value == 0) {
 		/* key release */
-		if (--state->reset_seq_cnt == 0)
+		if (--state->reset_seq_cnt == 0) {
 			state->reset_canceled = false;
+			del_timer(&state->keyreset_timeout);
+		}
 	} else if (value == 1) {
 		/* key press, not autorepeat */
 		if (++state->reset_seq_cnt == state->reset_seq_len &&
-		    !state->reset_canceled) {
-			return true;
+						!state->reset_canceled) {
+			sysrq_handle_reset_request(state);
 		}
 	}
+}
 
-	return false;
+static void sysrq_handle_reset_timeout(unsigned long data)
+{
+	__handle_sysrq(sysrq_xlate[KEY_B], false);
 }
 
 static void sysrq_reinject_alt_sysrq(struct work_struct *work)
@@ -746,10 +763,8 @@ static bool sysrq_handle_keypress(struct sysrq_state *sysrq,
 		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);
-		}
+		/* Check for reset sequence */
+		sysrq_detect_reset_sequence(sysrq, code, value);
 
 	} else if (value == 0 && test_and_clear_bit(code, sysrq->key_down)) {
 		/*
@@ -810,6 +825,8 @@ static int sysrq_connect(struct input_handler *handler,
 	sysrq->handle.handler = handler;
 	sysrq->handle.name = "sysrq";
 	sysrq->handle.private = sysrq;
+	init_timer(&sysrq->keyreset_timeout);
+	sysrq->keyreset_timeout.function = sysrq_handle_reset_timeout;
 
 	error = input_register_handle(&sysrq->handle);
 	if (error) {
@@ -839,6 +856,7 @@ static void sysrq_disconnect(struct input_handle *handle)
 
 	input_close_device(handle);
 	cancel_work_sync(&sysrq->reinject_work);
+	del_timer(&sysrq->keyreset_timeout);
 	input_unregister_handle(handle);
 	kfree(sysrq);
 }
@@ -868,8 +886,6 @@ static struct input_handler sysrq_handler = {
 
 static bool sysrq_handler_registered;
 
-unsigned short platform_sysrq_reset_seq[] __weak = { KEY_RESERVED };
-
 static inline void sysrq_register_handler(void)
 {
 	unsigned short key;
@@ -929,6 +945,8 @@ static struct kernel_param_ops param_ops_sysrq_reset_seq = {
 module_param_array_named(reset_seq, sysrq_reset_seq, sysrq_reset_seq,
 			 &sysrq_reset_seq_len, 0644);
 
+module_param(sysrq_downtime_ms, int, 0644);
+
 #else
 
 static inline void sysrq_register_handler(void)
-- 
1.7.9.5

Re: [PATCH v2] sysrq: supplementing reset sequence with timeout functionality

[Arve Hjønnevåg]

On Sat, Mar 16, 2013 at 10:14 AM,  <mathieu.poirier@linaro.org> wrote:
> From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
>
> Some devices have too few buttons, which it makes it hard to have
> a reset combo that won't trigger automatically.  As such a
> timeout functionality that requires the combination to be held for
> a given amount of time before triggering is introduced.
>
> If a key combo is recognized and held for a 'timeout' amount of time,
> the system triggers a reset.

The code seems to only require one of the keys in the combo to be held
for the full 'timeout' amount of time:

...
>                 /* key release */
> -               if (--state->reset_seq_cnt == 0)
> +               if (--state->reset_seq_cnt == 0) {
>                         state->reset_canceled = false;
> +                       del_timer(&state->keyreset_timeout);
> +               }

-- 
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] sysrq: supplementing reset sequence with timeout functionality

[Mathieu Poirier]

On 13-03-18 06:44 PM, Arve Hjønnevåg wrote:
> On Sat, Mar 16, 2013 at 10:14 AM,  <mathieu.poirier@linaro.org> wrote:
>> From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
>>
>> Some devices have too few buttons, which it makes it hard to have
>> a reset combo that won't trigger automatically.  As such a
>> timeout functionality that requires the combination to be held for
>> a given amount of time before triggering is introduced.
>>
>> If a key combo is recognized and held for a 'timeout' amount of time,
>> the system triggers a reset.
> 
> The code seems to only require one of the keys in the combo to be held
> for the full 'timeout' amount of time:
> 
> ...
>>                 /* key release */
>> -               if (--state->reset_seq_cnt == 0)
>> +               if (--state->reset_seq_cnt == 0) {
>>                         state->reset_canceled = false;
>> +                       del_timer(&state->keyreset_timeout);
>> +               }
> 

I think the case you are referring to is when keys are released - please
correct me if I'm wrong.

You are correct, after the timer has been initialised (meaning that
_all_ the keys have been pressed) only a single key need to be held down
to carry out the reset order.

The current keyreset driver works exactly the same way - I just tried it
on my board.

Get back to me if you want to see this corrected.

Thanks,
Mathieu.
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] sysrq: supplementing reset sequence with timeout functionality

[Arve Hjønnevåg]

On Wed, Mar 20, 2013 at 8:11 AM, Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
> On 13-03-18 06:44 PM, Arve Hjønnevåg wrote:
>> On Sat, Mar 16, 2013 at 10:14 AM,  <mathieu.poirier@linaro.org> wrote:
>>> From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
>>>
>>> Some devices have too few buttons, which it makes it hard to have
>>> a reset combo that won't trigger automatically.  As such a
>>> timeout functionality that requires the combination to be held for
>>> a given amount of time before triggering is introduced.
>>>
>>> If a key combo is recognized and held for a 'timeout' amount of time,
>>> the system triggers a reset.
>>
>> The code seems to only require one of the keys in the combo to be held
>> for the full 'timeout' amount of time:
>>
>> ...
>>>                 /* key release */
>>> -               if (--state->reset_seq_cnt == 0)
>>> +               if (--state->reset_seq_cnt == 0) {
>>>                         state->reset_canceled = false;
>>> +                       del_timer(&state->keyreset_timeout);
>>> +               }
>>
>
> I think the case you are referring to is when keys are released - please
> correct me if I'm wrong.
>
> You are correct, after the timer has been initialised (meaning that
> _all_ the keys have been pressed) only a single key need to be held down
> to carry out the reset order.
>
> The current keyreset driver works exactly the same way - I just tried it
> on my board.
>
> Get back to me if you want to see this corrected.
>

Since this feature so far has only been used with a single key, fixing
the code is not a high priority. However, I don't think the current
description matches the implementation you have, so one of them should
change.

-- 
Arve Hjønnevåg
--
To unsubscribe from this list: send the line "unsubscribe linux-input" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html