[PATCH v2] drivers/tty: Folding Android's keyreset driver in sysRQ

[PATCH v2] drivers/tty: Folding Android's keyreset driver in sysRQ

[mathieu.poirier]

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

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.  A platform device that specify a
reset key-combo should be added to the board file to trigger the
feature.

This functionality comes from the keyreset driver submitted by
Arve Hjønnevåg in the Android kernel.

Cc: arve@android.com
Cc: kernel-team@android.com
Cc: dmitry.torokhov@gmail.com
Cc: john.stultz@linaro.org
Signed-off-by: Mathieu Poirier <mathieu.poirier@linaro.org>
---
 drivers/tty/sysrq.c   |  161 +++++++++++++++++++++++++++++++++++++++++++++++++
 include/linux/sysrq.h |    8 +++
 2 files changed, 169 insertions(+), 0 deletions(-)

diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
index 05728894..f210853 100644
--- a/drivers/tty/sysrq.c
+++ b/drivers/tty/sysrq.c
@@ -41,6 +41,9 @@
 #include <linux/slab.h>
 #include <linux/input.h>
 #include <linux/uaccess.h>
+#include <linux/platform_device.h>
+#include <linux/syscalls.h>
+#include <linux/atomic.h>
 
 #include <asm/ptrace.h>
 #include <asm/irq_regs.h>
@@ -49,6 +52,11 @@
 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 +578,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 +620,93 @@ static void sysrq_reinject_alt_sysrq(struct work_struct *work)
 	}
 }
 
+
+static int sysrq_probe(struct platform_device *pdev)
+{
+	struct keyreset_platform_data *pdata = pdev->dev.platform_data;
+
+	/*
+	 * No sequence of keys to trigger on,
+	 * assuming default sysRQ behavior.
+	 */
+	if (pdata) {
+		atomic_set(&restart_requested, 0);
+		sysrq_handler.private = pdata;
+	} else
+		sysrq_handler.private = NULL;
+
+	/* FETCH DT INFO HERE */
+
+	return 0;
+
+}
+
+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;
+
+	/* Is the code is of interest to us */
+	if (!test_bit(code, state->keybit))
+		return processed;
+
+	/* No need to take care of key up events */
+	if (!test_bit(code, state->key) == !value)
+		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);
+		}
+	}
+
+	/* 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)
 {
@@ -669,6 +773,11 @@ static bool sysrq_filter(struct input_handle *handle,
 			if (sysrq->active && value && value != 2) {
 				sysrq->need_reinject = false;
 				__handle_sysrq(sysrq_xlate[code], true);
+			} else if (sysrq->keyreset_data) {
+				if (do_keyreset_event(sysrq, code, value)) {
+					suppress = sysrq->active;
+					goto end;
+				}
 			}
 			break;
 		}
@@ -704,9 +813,44 @@ static bool sysrq_filter(struct input_handle *handle,
 		break;
 	}
 
+	/*
+	 * suppress == true - suppress passing to other subsystems.
+	 * suppress == false - passing to other subsystems.
+	 */
+end:
 	return suppress;
 }
 
+static void parse_platform_data(struct sysrq_state *sysrq)
+{
+	int key, *keyp;
+	struct keyreset_platform_data *pdata = sysrq_handler.private;
+
+
+	keyp = pdata->keys_down;
+	while ((key = *keyp++)) {
+		if (key >= KEY_MAX)
+			continue;
+		sysrq->key_down_target++;
+		__set_bit(key, sysrq->keybit);
+	}
+
+	if (pdata->keys_up) {
+		keyp = pdata->keys_up;
+		while ((key = *keyp++)) {
+			if (key >= KEY_MAX)
+				continue;
+			__set_bit(key, sysrq->keybit);
+			__set_bit(key, sysrq->upbit);
+		}
+	}
+
+	if (pdata->reset_fn)
+		sysrq->reset_fn = pdata->reset_fn;
+
+	sysrq->keyreset_data = 1;
+}
+
 static int sysrq_connect(struct input_handler *handler,
 			 struct input_dev *dev,
 			 const struct input_device_id *id)
@@ -718,6 +862,14 @@ static int sysrq_connect(struct input_handler *handler,
 	if (!sysrq)
 		return -ENOMEM;
 
+	/*
+	 * input_register_handle() calls sysrq_probe(), who
+	 * in turn will put the keyreset information in
+	 * sysrq_handler's private field.
+	 */
+	if (handler->private)
+		parse_platform_data(sysrq);
+
 	INIT_WORK(&sysrq->reinject_work, sysrq_reinject_alt_sysrq);
 
 	sysrq->handle.dev = dev;
@@ -780,12 +932,21 @@ static struct input_handler sysrq_handler = {
 	.id_table	= sysrq_ids,
 };
 
+struct platform_driver sysrq_driver = {
+	.driver.name = SYSRQ_KRESET_NAME,
+	.probe = sysrq_probe,
+};
+
 static bool sysrq_handler_registered;
 
 static inline void sysrq_register_handler(void)
 {
 	int error;
 
+	error = platform_driver_register(&sysrq_driver);
+	if (error)
+		pr_err("Failed to sysrq_keyreset driver, error %d", error);
+
 	error = input_register_handler(&sysrq_handler);
 	if (error)
 		pr_err("Failed to register input handler, error %d", error);
diff --git a/include/linux/sysrq.h b/include/linux/sysrq.h
index 7faf933..d470ae5 100644
--- a/include/linux/sysrq.h
+++ b/include/linux/sysrq.h
@@ -17,6 +17,8 @@
 #include <linux/errno.h>
 #include <linux/types.h>
 
+#define SYSRQ_KRESET_NAME	"keyreset"
+
 /* Enable/disable SYSRQ support by default (0==no, 1==yes). */
 #define SYSRQ_DEFAULT_ENABLE	1
 
@@ -38,6 +40,12 @@ struct sysrq_key_op {
 	int enable_mask;
 };
 
+struct keyreset_platform_data {
+	int (*reset_fn)(void);
+	int *keys_up;
+	int keys_down[]; /* 0 terminated */
+};
+
 #ifdef CONFIG_MAGIC_SYSRQ
 
 /* Generic SysRq interface -- you may call it from any device driver, supplying
-- 
1.7.5.4

Re: [PATCH v2] drivers/tty: Folding Android's keyreset driver in sysRQ

[Dmitry Torokhov]

Hi Matthieu,

On Thu, Aug 30, 2012 at 04:30:54PM -0600, mathieu.poirier@linaro.org wrote:
> From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
> 
> 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.  A platform device that specify a
> reset key-combo should be added to the board file to trigger the
> feature.

Why do we need to involve a platform device and not use, for example, a module
parameter, that could be set up from userspace?

Also, why do we need reset_fn() and not simply invoke SysRq-B handler
that should call ctrl_alt_del() for us?

Thanks.

-- 
Dmitry

Re: [PATCH v2] drivers/tty: Folding Android's keyreset driver in sysRQ

[Mathieu Poirier]

On 12-08-30 05:01 PM, Dmitry Torokhov wrote:
> Hi Matthieu,
> 
> On Thu, Aug 30, 2012 at 04:30:54PM -0600, mathieu.poirier@linaro.org wrote:
>> From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
>>
>> 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.  A platform device that specify a
>> reset key-combo should be added to the board file to trigger the
>> feature.
>
> Why do we need to involve a platform device and not use, for example, a module
> parameter, that could be set up from userspace?

The platform device comes from the original design and was included to
minimise the amount of changes in code that make use of the current
keyreset driver.

I am definitely willing to explore the possibility of adding module
parameter to complement the platform data but again, to avoid impacting
board code I'm in favour of keeping the platform data/device - get back
to me if you disagree.

Thinking back on this it may be better to call 'platform_driver_probe'
rather than 'platform_driver_register'.  That way one wouldn't have to
instantiate a platform_device.

> 
> Also, why do we need reset_fn() and not simply invoke SysRq-B handler
> that should call ctrl_alt_del() for us?

The reset_fn() gives an implementer the chance of calling some custom
function before the reset sequence is started and in my opinion should
stay.  On the flip side I'm not sure that I understand what you mean by
"SysRq-B handler" - are you talking about the "sysrq_reboot_op" ?
Please expand on that ?

Thanks for the review and comments,
Mathieu.

> 
> Thanks.
> 

Re: [PATCH v2] drivers/tty: Folding Android's keyreset driver in sysRQ

[Alan Cox]

> > Why do we need to involve a platform device and not use, for example, a module
> > parameter, that could be set up from userspace?
> 
> The platform device comes from the original design and was included to
> minimise the amount of changes in code that make use of the current
> keyreset driver.

The platform device is IMHO the right answer. In this class of devices
the stuff is compiled in, the userspace is Android, there are no modules
and there is no reason for it to be configurable.

> I am definitely willing to explore the possibility of adding module
> parameter to complement the platform data but again, to avoid impacting
> board code I'm in favour of keeping the platform data/device - get back
> to me if you disagree.
> 
> Thinking back on this it may be better to call 'platform_driver_probe'
> rather than 'platform_driver_register'.  That way one wouldn't have to
> instantiate a platform_device.
> 
> > 
> > Also, why do we need reset_fn() and not simply invoke SysRq-B handler
> > that should call ctrl_alt_del() for us?
> 
> The reset_fn() gives an implementer the chance of calling some custom
> function before the reset sequence is started and in my opinion should

So why wouldn't that already be using the reset notifiers ?

Alan

Re: [PATCH v2] drivers/tty: Folding Android's keyreset driver in sysRQ

[Dmitry Torokhov]

On Fri, Aug 31, 2012 at 11:02:27PM +0100, Alan Cox wrote:
> > > Why do we need to involve a platform device and not use, for example, a module
> > > parameter, that could be set up from userspace?
> > 
> > The platform device comes from the original design and was included to
> > minimise the amount of changes in code that make use of the current
> > keyreset driver.
> 
> The platform device is IMHO the right answer. In this class of devices
> the stuff is compiled in, the userspace is Android, there are no modules
> and there is no reason for it to be configurable.

It does not matter if it is built in or not, /sys/module/XXX/parameters
is still there, and while havig it in kernel is "easy" you could as
easily stuff needed data into a sysfs attribute during booting.

And we should be able to get this from DT even without the platform
device (this was the next step, wasn't it?).

Thanks.

-- 
Dmitry

Re: [PATCH v2] drivers/tty: Folding Android's keyreset driver in sysRQ

[Mathieu Poirier]

On 12-08-31 04:02 PM, Alan Cox wrote:
>>> Why do we need to involve a platform device and not use, for example, a module
>>> parameter, that could be set up from userspace?
>>
>> The platform device comes from the original design and was included to
>> minimise the amount of changes in code that make use of the current
>> keyreset driver.
> 
> The platform device is IMHO the right answer. In this class of devices
> the stuff is compiled in, the userspace is Android, there are no modules
> and there is no reason for it to be configurable.
> 
>> I am definitely willing to explore the possibility of adding module
>> parameter to complement the platform data but again, to avoid impacting
>> board code I'm in favour of keeping the platform data/device - get back
>> to me if you disagree.
>>
>> Thinking back on this it may be better to call 'platform_driver_probe'
>> rather than 'platform_driver_register'.  That way one wouldn't have to
>> instantiate a platform_device.
>>
>>>
>>> Also, why do we need reset_fn() and not simply invoke SysRq-B handler
>>> that should call ctrl_alt_del() for us?
>>
>> The reset_fn() gives an implementer the chance of calling some custom
>> function before the reset sequence is started and in my opinion should

I did not express myself clearly - with reset_fn() a system can do
whatever it wants when a specific series of keys is pressed.

Granted that the next steps are most likely converging toward rebooting
the system - but it may not be right away and depending on the
circumstances a reboot could be avoided altogether.

> 
> So why wouldn't that already be using the reset notifiers ?

I am not familiar with the "reset notifiers" that have been referred to
but a little bit of research indicate that a registering subsystem gets
notified when the event of interest (in this case a reboot) happens.

I understand your proposition here but aren't we loosing flexibility in
what we can achieve when the event has been triggered ?

What do you think of adding a keyreset event that would be fired (and
caught by a registering subsystem) instead of calling reset_fn() ?

Thanks,
Mathieu.

> 
> Alan
> 

Re: [PATCH v2] drivers/tty: Folding Android's keyreset driver in sysRQ

[Mathieu Poirier]

On 12-08-31 04:41 PM, Dmitry Torokhov wrote:
> On Fri, Aug 31, 2012 at 11:02:27PM +0100, Alan Cox wrote:
>>>> Why do we need to involve a platform device and not use, for example, a module
>>>> parameter, that could be set up from userspace?
>>>
>>> The platform device comes from the original design and was included to
>>> minimise the amount of changes in code that make use of the current
>>> keyreset driver.
>>
>> The platform device is IMHO the right answer. In this class of devices
>> the stuff is compiled in, the userspace is Android, there are no modules
>> and there is no reason for it to be configurable.
> 
> It does not matter if it is built in or not, /sys/module/XXX/parameters
> is still there, and while havig it in kernel is "easy" you could as
> easily stuff needed data into a sysfs attribute during booting.
> 
> And we should be able to get this from DT even without the platform
> device (this was the next step, wasn't it?).

Correct - my hope was to get the main functionality accepted before
adding DT support.  Do you think the lack of DT support is a blocker for
acceptance ?  Please confirm.

Mathieu.

> 
> Thanks.
> 

Re: [PATCH v2] drivers/tty: Folding Android's keyreset driver in sysRQ

[Dmitry Torokhov]

On Fri, Aug 31, 2012 at 04:57:04PM -0600, Mathieu Poirier wrote:
> On 12-08-31 04:41 PM, Dmitry Torokhov wrote:
> > On Fri, Aug 31, 2012 at 11:02:27PM +0100, Alan Cox wrote:
> >>>> Why do we need to involve a platform device and not use, for example, a module
> >>>> parameter, that could be set up from userspace?
> >>>
> >>> The platform device comes from the original design and was included to
> >>> minimise the amount of changes in code that make use of the current
> >>> keyreset driver.
> >>
> >> The platform device is IMHO the right answer. In this class of devices
> >> the stuff is compiled in, the userspace is Android, there are no modules
> >> and there is no reason for it to be configurable.
> > 
> > It does not matter if it is built in or not, /sys/module/XXX/parameters
> > is still there, and while havig it in kernel is "easy" you could as
> > easily stuff needed data into a sysfs attribute during booting.
> > 
> > And we should be able to get this from DT even without the platform
> > device (this was the next step, wasn't it?).
> 
> Correct - my hope was to get the main functionality accepted before
> adding DT support.  Do you think the lack of DT support is a blocker for
> acceptance ?  Please confirm.
> 

No, lack of DT is not a blocker, but I am unconvinced that we need
platform device.

Thanks,

-- 
Dmitry

Re: [PATCH v2] drivers/tty: Folding Android's keyreset driver in sysRQ

[Colin Cross]

On Fri, Aug 31, 2012 at 2:52 PM, Mathieu Poirier
<mathieu.poirier@linaro.org> wrote:
> On 12-08-30 05:01 PM, Dmitry Torokhov wrote:
>> Hi Matthieu,
>>
>> On Thu, Aug 30, 2012 at 04:30:54PM -0600, mathieu.poirier@linaro.org wrote:
>>> From: "Mathieu J. Poirier" <mathieu.poirier@linaro.org>
>>>
>>> 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.  A platform device that specify a
>>> reset key-combo should be added to the board file to trigger the
>>> feature.
>>
>> Why do we need to involve a platform device and not use, for example, a module
>> parameter, that could be set up from userspace?
>
> The platform device comes from the original design and was included to
> minimise the amount of changes in code that make use of the current
> keyreset driver.
>
> I am definitely willing to explore the possibility of adding module
> parameter to complement the platform data but again, to avoid impacting
> board code I'm in favour of keeping the platform data/device - get back
> to me if you disagree.

I wouldn't worry too much about compatibility with existing board
files, they are unlikely to move to a new driver anyways, and if they
do, changing the way they are configured is not a big deal.  That
said, I generally don't like module parameters because it ends up
cluttering the kernel command line with configuration for every
device.  Setting them through sysfs is too late, if a device with a
non-removable battery hangs during boot you would like to be able to
reset it with this driver.

Re: [PATCH v2] drivers/tty: Folding Android's keyreset driver in sysRQ

[Mathieu Poirier]

On 12-08-31 05:22 PM, Dmitry Torokhov wrote:
> On Fri, Aug 31, 2012 at 04:57:04PM -0600, Mathieu Poirier wrote:
>> On 12-08-31 04:41 PM, Dmitry Torokhov wrote:
>>> On Fri, Aug 31, 2012 at 11:02:27PM +0100, Alan Cox wrote:
>>>>>> Why do we need to involve a platform device and not use, for example, a module
>>>>>> parameter, that could be set up from userspace?
>>>>>
>>>>> The platform device comes from the original design and was included to
>>>>> minimise the amount of changes in code that make use of the current
>>>>> keyreset driver.
>>>>
>>>> The platform device is IMHO the right answer. In this class of devices
>>>> the stuff is compiled in, the userspace is Android, there are no modules
>>>> and there is no reason for it to be configurable.
>>>
>>> It does not matter if it is built in or not, /sys/module/XXX/parameters
>>> is still there, and while havig it in kernel is "easy" you could as
>>> easily stuff needed data into a sysfs attribute during booting.
>>>
>>> And we should be able to get this from DT even without the platform
>>> device (this was the next step, wasn't it?).
>>
>> Correct - my hope was to get the main functionality accepted before
>> adding DT support.  Do you think the lack of DT support is a blocker for
>> acceptance ?  Please confirm.
>>
> 
> No, lack of DT is not a blocker, but I am unconvinced that we need
> platform device.
> 
> Thanks,
> 

A platform device is really easy to spin-off in a board file and once it
is there you don't have to worry about other loose ends to tie in before
the solution is functional.

I don't mind supplementing the current proposition with a module
parameter interface to get the "key_down" and "key_up" sequences.

Which brings us to the "reset_fn()" function - in my opinion it offers
significant advantages and should be kept in the design.  What I'm not
so clear about is on the implementation.  Should it be kept as part of a
platform data or be implemented as a notifier as suggested by Alan.  I
am looking for guidance here and suggestions are encouraged.

Regards,
Mathieu.

Re: [PATCH v2] drivers/tty: Folding Android's keyreset driver in sysRQ

[Alan Cox]

> which brings us to the "reset_fn()" function - in my opinion it offers
> significant advantages and should be kept in the design.  What I'm not
> so clear about is on the implementation.  Should it be kept as part of a
> platform data or be implemented as a notifier as suggested by Alan.  I
> am looking for guidance here and suggestions are encouraged.

Not sure reset_fn is the best name but it'll do for me. As for things like
notifiers, thats extra complexity. *When* someone needs it then they can
add it. We don't need to add extra complexity now.