| Message ID | 20200511135918.8203-1-andrzej.p@collabora.com |
|---|---|
| Headers | show |
| Series | Magic SysRq extensions | expand |
On Mon, May 11, 2020 at 03:59:15PM +0200, Andrzej Pietrasiewicz wrote: > There are existing machines which don't have SysRq key, e.g. chromebooks. > This patch allows configuring which key acts as SysRq. The value is passed > with sysrq's module parameter. > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/tty/sysrq.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c > index 93202fc24308..ebad9799fdc0 100644 > --- a/drivers/tty/sysrq.c > +++ b/drivers/tty/sysrq.c > @@ -604,6 +604,7 @@ EXPORT_SYMBOL(handle_sysrq); > > #ifdef CONFIG_INPUT > static int sysrq_reset_downtime_ms; > +static unsigned short sysrq_key = KEY_SYSRQ; > > /* Simple translation table for the SysRq keys */ > static const unsigned char sysrq_xlate[KEY_CNT] = > @@ -735,10 +736,10 @@ static void sysrq_reinject_alt_sysrq(struct work_struct *work) > > /* Simulate press and release of Alt + SysRq */ > input_inject_event(handle, EV_KEY, alt_code, 1); > - input_inject_event(handle, EV_KEY, KEY_SYSRQ, 1); > + input_inject_event(handle, EV_KEY, sysrq_key, 1); > input_inject_event(handle, EV_SYN, SYN_REPORT, 1); > > - input_inject_event(handle, EV_KEY, KEY_SYSRQ, 0); > + input_inject_event(handle, EV_KEY, sysrq_key, 0); > input_inject_event(handle, EV_KEY, alt_code, 0); > input_inject_event(handle, EV_SYN, SYN_REPORT, 1); > > @@ -770,6 +771,7 @@ static bool sysrq_handle_keypress(struct sysrq_state *sysrq, > } > break; > > +key_sysrq: > case KEY_SYSRQ: > if (value == 1 && sysrq->alt != KEY_RESERVED) { > sysrq->active = true; > @@ -790,11 +792,15 @@ static bool sysrq_handle_keypress(struct sysrq_state *sysrq, > * triggering print screen function. > */ > if (sysrq->active) > - clear_bit(KEY_SYSRQ, sysrq->handle.dev->key); > + clear_bit(sysrq_key, sysrq->handle.dev->key); > > break; > > default: > + /* handle non-default sysrq key */ > + if (code == sysrq_key) > + goto key_sysrq; > + > if (sysrq->active && value && value != 2) { > sysrq->need_reinject = false; > __handle_sysrq(sysrq_xlate[code], true); > @@ -995,6 +1001,8 @@ module_param_array_named(reset_seq, sysrq_reset_seq, sysrq_reset_seq, > > module_param_named(sysrq_downtime_ms, sysrq_reset_downtime_ms, int, 0644); > > +module_param(sysrq_key, ushort, 0644); No documentation of this new module parameter? :(
On Mon, May 11, 2020 at 03:59:17PM +0200, Andrzej Pietrasiewicz wrote: > Some userland might want to implement a policy to signal a configured > process with a configured signal. This patch adds necessary kernel-side > infrastructure and the newly added handler is triggered with > Alt-Shift-SysRq-s. Optionally the userland can also specify the expected > name of parent process of the victim. THat's crazy, what "userspace" wants to do something like this that can't just do it by running a program? Why force the kernel to do it for them? And you don't document any of this :( greg k-h
On Mon, May 11, 2020 at 03:59:18PM +0200, Andrzej Pietrasiewicz wrote: > Some userland might want to execute e.g. 'w' (show blocked tasks), followed > by 's' (sync), followed by 1000 ms delay and then followed by 'c' (crash) > upon a single magic SysRq. Or one might want to execute the famous "Raising > Elephants Is So Utterly Boring" action. This patch adds a configurable > handler, triggered with 'C', for this exact purpose. The user specifies the > composition of the compound action using syntax similar to getopt, where > each letter corresponds to an individual action and a colon followed by a > number corresponds to a delay of that many milliseconds, e.g.: > > ws:1000c > > or > > r:100eis:1000ub Cute, but why? Who needs/wants this type of thing? And again, no documentation :( greg k-h
Hi Andrzej, On Mon, May 11, 2020 at 03:59:13PM +0200, Andrzej Pietrasiewicz wrote: > Nobody in the tree uses linux,sysrq-reset-seq in Device Tree source files. > Remove the corresponding code. Unlike platform data, we do not require or even expect that all DT users be present in mainline, so absence if references to a feature in kernel can not serve as justification for removal. Consider the fact that we support DT-style binding in ACPI (which is BIOS/firmware). That said, I am not against removing this support, but we need to make sure that Android (where this came from) does not use this anymore. Thanks.
Hi Andrzej, On Mon, May 11, 2020 at 03:59:15PM +0200, Andrzej Pietrasiewicz wrote: > There are existing machines which don't have SysRq key, e.g. chromebooks. > This patch allows configuring which key acts as SysRq. The value is passed > with sysrq's module parameter. > > Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> > --- > drivers/tty/sysrq.c | 14 +++++++++++--- > 1 file changed, 11 insertions(+), 3 deletions(-) > > diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c > index 93202fc24308..ebad9799fdc0 100644 > --- a/drivers/tty/sysrq.c > +++ b/drivers/tty/sysrq.c > @@ -604,6 +604,7 @@ EXPORT_SYMBOL(handle_sysrq); > > #ifdef CONFIG_INPUT > static int sysrq_reset_downtime_ms; > +static unsigned short sysrq_key = KEY_SYSRQ; > > /* Simple translation table for the SysRq keys */ > static const unsigned char sysrq_xlate[KEY_CNT] = > @@ -735,10 +736,10 @@ static void sysrq_reinject_alt_sysrq(struct work_struct *work) > > /* Simulate press and release of Alt + SysRq */ > input_inject_event(handle, EV_KEY, alt_code, 1); > - input_inject_event(handle, EV_KEY, KEY_SYSRQ, 1); > + input_inject_event(handle, EV_KEY, sysrq_key, 1); > input_inject_event(handle, EV_SYN, SYN_REPORT, 1); > > - input_inject_event(handle, EV_KEY, KEY_SYSRQ, 0); > + input_inject_event(handle, EV_KEY, sysrq_key, 0); > input_inject_event(handle, EV_KEY, alt_code, 0); > input_inject_event(handle, EV_SYN, SYN_REPORT, 1); Unfortunately this means that if I connect my external keyboard to chromebook SysRq there will stop working, which is not great. If we want to support this we need to figure out how to make this handling per-device. FWIW Chrome OS cheats and simply adds more keys to be handled as SysRq without removing "classic" SysRq. But that hack is obviously not suitable for the mainline. Thanks.
On Mon, May 11, 2020 at 06:21:13PM +0200, Greg Kroah-Hartman wrote: > On Mon, May 11, 2020 at 03:59:18PM +0200, Andrzej Pietrasiewicz wrote: > > Some userland might want to execute e.g. 'w' (show blocked tasks), followed > > by 's' (sync), followed by 1000 ms delay and then followed by 'c' (crash) > > upon a single magic SysRq. Or one might want to execute the famous "Raising > > Elephants Is So Utterly Boring" action. This patch adds a configurable > > handler, triggered with 'C', for this exact purpose. The user specifies the > > composition of the compound action using syntax similar to getopt, where > > each letter corresponds to an individual action and a colon followed by a > > number corresponds to a delay of that many milliseconds, e.g.: > > > > ws:1000c > > > > or > > > > r:100eis:1000ub > > Cute, but why? Who needs/wants this type of thing? On Chrome OS the first time user presses SysRq-X it will try to kill chrome (and that will cause crash to get uploaded if user consented). The 2nd time within 5 seconds the same combo is pressed, it will dump blocked tasks in syslog and try to sync and then panic. On panic the device will reboot, logs will be scraped from pstore, and uploaded for analysis. Thanks.
Hi, W dniu 11.05.2020 o 20:29, Dmitry Torokhov pisze: > On Mon, May 11, 2020 at 06:21:13PM +0200, Greg Kroah-Hartman wrote: >> On Mon, May 11, 2020 at 03:59:18PM +0200, Andrzej Pietrasiewicz wrote: >>> Some userland might want to execute e.g. 'w' (show blocked tasks), followed >>> by 's' (sync), followed by 1000 ms delay and then followed by 'c' (crash) >>> upon a single magic SysRq. Or one might want to execute the famous "Raising >>> Elephants Is So Utterly Boring" action. This patch adds a configurable >>> handler, triggered with 'C', for this exact purpose. The user specifies the >>> composition of the compound action using syntax similar to getopt, where >>> each letter corresponds to an individual action and a colon followed by a >>> number corresponds to a delay of that many milliseconds, e.g.: >>> >>> ws:1000c >>> >>> or >>> >>> r:100eis:1000ub >> >> Cute, but why? Who needs/wants this type of thing? Surely things that can be done in userspace should be done there. So we would envision an input daemon which reacts to a predefined combination of keys. That said, it is not unimaginable to think of userspace being dead enough (e.g. due to memory pressure) to be unable to complete such a compound action. In other words userspace not being able to do it is a good reason for putting the code in the kernel. Dmitry has given a use case where such a compound action is needed. Andrzej > > On Chrome OS the first time user presses SysRq-X it will try to kill > chrome (and that will cause crash to get uploaded if user consented). > The 2nd time within 5 seconds the same combo is pressed, it will dump > blocked tasks in syslog and try to sync and then panic. On panic the > device will reboot, logs will be scraped from pstore, and uploaded for > analysis. > > Thanks. >
Hi Dmitry, W dniu 11.05.2020 o 19:58, Dmitry Torokhov pisze: > Hi Andrzej, > > On Mon, May 11, 2020 at 03:59:13PM +0200, Andrzej Pietrasiewicz wrote: >> Nobody in the tree uses linux,sysrq-reset-seq in Device Tree source files. >> Remove the corresponding code. > > Unlike platform data, we do not require or even expect that all DT users > be present in mainline, so absence if references to a feature in kernel > can not serve as justification for removal. Consider the fact that we > support DT-style binding in ACPI (which is BIOS/firmware). > > That said, I am not against removing this support, but we need to make > sure that Android (where this came from) does not use this anymore. How can I do it? Can I at all? Andrzej
Hi Dmitry, W dniu 11.05.2020 o 20:01, Dmitry Torokhov pisze: > Hi Andrzej, > > On Mon, May 11, 2020 at 03:59:15PM +0200, Andrzej Pietrasiewicz wrote: >> There are existing machines which don't have SysRq key, e.g. chromebooks. >> This patch allows configuring which key acts as SysRq. The value is passed >> with sysrq's module parameter. >> >> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com> >> --- >> drivers/tty/sysrq.c | 14 +++++++++++--- >> 1 file changed, 11 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c >> index 93202fc24308..ebad9799fdc0 100644 >> --- a/drivers/tty/sysrq.c >> +++ b/drivers/tty/sysrq.c >> @@ -604,6 +604,7 @@ EXPORT_SYMBOL(handle_sysrq); >> >> #ifdef CONFIG_INPUT >> static int sysrq_reset_downtime_ms; >> +static unsigned short sysrq_key = KEY_SYSRQ; >> >> /* Simple translation table for the SysRq keys */ >> static const unsigned char sysrq_xlate[KEY_CNT] = >> @@ -735,10 +736,10 @@ static void sysrq_reinject_alt_sysrq(struct work_struct *work) >> >> /* Simulate press and release of Alt + SysRq */ >> input_inject_event(handle, EV_KEY, alt_code, 1); >> - input_inject_event(handle, EV_KEY, KEY_SYSRQ, 1); >> + input_inject_event(handle, EV_KEY, sysrq_key, 1); >> input_inject_event(handle, EV_SYN, SYN_REPORT, 1); >> >> - input_inject_event(handle, EV_KEY, KEY_SYSRQ, 0); >> + input_inject_event(handle, EV_KEY, sysrq_key, 0); >> input_inject_event(handle, EV_KEY, alt_code, 0); >> input_inject_event(handle, EV_SYN, SYN_REPORT, 1); > > Unfortunately this means that if I connect my external keyboard to > chromebook SysRq there will stop working, which is not great. If we want > to support this we need to figure out how to make this handling > per-device. I see your point. So what you envision is SysRq key being configured for each input device separately. I can see these problems: - How to attach such a configuration information to each specific device instance? It is easy if done framework-wise, but gets messy if done per-device. And we are talking per-device rather than per-driver. - If a user has multiple USB keyboards connected (possibly through a cascade of hubs), how would they know which key is valid for which keyboard? Wouldn't it be better if such a piece of configuration were valid for the whole system instead of per-device? Andrzej