Re: [PATCH] tty/sysrq: Add alternative SysRq key

[Andrzej Pietrasiewicz <andrzej.p@collabora.com>]

Hi Dmitry,

W dniu 09.07.2020 o 07:05, Dmitry Torokhov pisze:
> Hi Andrzej,
> 
> On Fri, Jun 19, 2020 at 06:28:19PM +0200, Andrzej Pietrasiewicz wrote:
>> There exist machines which don't have SysRq key at all, e.g. chromebooks.
>>
>> This patch allows configuring an alternative key to act as SysRq. Devices
>> which declare KEY_SYSRQ in their 'keybit' bitmap continue using KEY_SYSRQ,
>> but other devices use the alternative SysRq key instead, by default F10.
>> Which key is actually used can be modified with sysrq's module parameter.
> 
> I guess you will be removing KEY_SYSRQ form all Chrome OS internal AT
> keyboards and external USB keyboard with Chrome OS layouts as well? Via
> udev keymap? I suppose this could work... Or have a per device setting
> as we allocate a separate handle for each input device attached to the
> SysRq handler.
> 

To me it makes most sense to have the decision taken per each input
device - if it is capable of providing KEY_SYSRQ, then it is used,
otherwise the alternative is taken.

The question is how to provide this information at ->connect() time.

Ideally chromebook's keyboard should be modelled in such a way that
it reflects reality. And the reality is that chromebooks probably
declare they use full AT PS/2 keyboard even though they have less keys.

It is unclear to me whether it makes sense to struggle to better
reflect actual keys repertoire at the kernel level. If udev's keymap
can be used, that should do. Now, are we able to guarantee that the
modification of the keyboard layout happens before the sysrq handler
is matched against the keyboard?

Andrzej

>>
>> Signed-off-by: Andrzej Pietrasiewicz <andrzej.p@collabora.com>
>> ---
>>   drivers/tty/sysrq.c | 28 +++++++++++++++++++++++++---
>>   1 file changed, 25 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/tty/sysrq.c b/drivers/tty/sysrq.c
>> index 0dc3878794fd..e1d271c84746 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 alternative_sysrq_key = KEY_F10;
>>   
>>   /* Simple translation table for the SysRq keys */
>>   static const unsigned char sysrq_xlate[KEY_CNT] =
>> @@ -621,6 +622,7 @@ struct sysrq_state {
>>   	unsigned long key_down[BITS_TO_LONGS(KEY_CNT)];
>>   	unsigned int alt;
>>   	unsigned int alt_use;
>> +	unsigned short sysrq_key;
>>   	bool active;
>>   	bool need_reinject;
>>   	bool reinjecting;
>> @@ -770,10 +772,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->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->sysrq_key, 0);
>>   		input_inject_event(handle, EV_KEY, alt_code, 0);
>>   		input_inject_event(handle, EV_SYN, SYN_REPORT, 1);
>>   
>> @@ -805,6 +807,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;
>> @@ -825,11 +828,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->sysrq_key, sysrq->handle.dev->key);
>>   
>>   		break;
>>   
>>   	default:
>> +		/* handle non-default sysrq key */
>> +		if (code == sysrq->sysrq_key)
>> +			goto key_sysrq;
>> +
>>   		if (sysrq->active && value && value != 2) {
>>   			sysrq->need_reinject = false;
>>   			__handle_sysrq(sysrq_xlate[code], true);
>> @@ -924,6 +931,14 @@ static int sysrq_connect(struct input_handler *handler,
>>   	sysrq->handle.private = sysrq;
>>   	timer_setup(&sysrq->keyreset_timer, sysrq_do_reset, 0);
>>   
>> +	if (test_bit(KEY_SYSRQ, dev->keybit)) {
>> +		sysrq->sysrq_key = KEY_SYSRQ;
>> +		pr_info("%s: using default sysrq key [%x]\n", dev->name, KEY_SYSRQ);
>> +	} else {
>> +		sysrq->sysrq_key = alternative_sysrq_key;
>> +		pr_info("%s: Using alternative sysrq key: [%x]\n", dev->name, sysrq->sysrq_key);
>> +	}
> 
> This is way too noisy IMO.
> 
>> +
>>   	error = input_register_handle(&sysrq->handle);
>>   	if (error) {
>>   		pr_err("Failed to register input sysrq handler, error %d\n",
>> @@ -1032,6 +1047,13 @@ 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(alternative_sysrq_key, ushort, 0644);
>> +MODULE_PARM_DESC(alternative_sysrq_key,
>> +	"Alternative SysRq key for input devices that don't have SysRq key. F10 by default.\n"
>> +	"Example\n"
>> +	"Using F9 as SysRq:\n"
>> +	"sysrq.alternative_sysrq_key=0x43\n");
>> +
>>   #else
>>   
>>   static inline void sysrq_register_handler(void)
>>
>> base-commit: 3d77e6a8804abcc0504c904bd6e5cdf3a5cf8162
>> -- 
>> 2.17.1
>>
>