Re: [PATCH v3 1/2] Input: Add a Kconfig to emulate KEY_SCREENLOCK with META + L

[Mario Limonciello <superm1@kernel.org>]

On 4/17/25 05:48, Hans de Goede wrote:
> Hi Mario,
> 
> On 17-Apr-25 3:37 AM, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> In the PC industry KEY_SCREENLOCK isn't used as frequently as it used
>> to be. Modern versions of Windows [1], GNOME and KDE support "META" + "L"
>> to lock the screen. Modern hardware [2] also sends this sequence of
>> events for keys with a silkscreen for screen lock.
>>
>> Introduced a new Kconfig option that will change KEY_SCREENLOCK when
>> emitted by driver to META + L.
>>
>> Link: https://support.microsoft.com/en-us/windows/keyboard-shortcuts-in-windows-dcc61a57-8ff0-cffe-9796-cb9706c75eec [1]
>> Link: https://www.logitech.com/en-us/shop/p/k860-split-ergonomic.920-009166 [2]
>> Suggested-by: Armin Wolf <W_Armin@gmx.de>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>> v3:
>>   * Emulation in the input core instead
>>
>>   drivers/input/Kconfig |  8 ++++++++
>>   drivers/input/input.c | 19 +++++++++++++++++++
>>   2 files changed, 27 insertions(+)
>>
>> diff --git a/drivers/input/Kconfig b/drivers/input/Kconfig
>> index 88ecdf5218ee9..ffb4163fe315f 100644
>> --- a/drivers/input/Kconfig
>> +++ b/drivers/input/Kconfig
>> @@ -174,6 +174,14 @@ config INPUT_APMPOWER
>>   	  To compile this driver as a module, choose M here: the
>>   	  module will be called apm-power.
>>   
>> +config INPUT_SCREENLOCK_EMULATION
>> +	bool "Pass KEY_SCREENLOCK as META + L"
>> +	help
>> +	  Say Y here if you want KEY_SCREENLOCK to be passed to userspace as
>> +	  META + L.
>> +
>> +	  If unsure, say Y.
>> +
>>   comment "Input Device Drivers"
>>   
>>   source "drivers/input/keyboard/Kconfig"
>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>> index dfeace85c4710..08a857cea0c5d 100644
>> --- a/drivers/input/input.c
>> +++ b/drivers/input/input.c
>> @@ -370,6 +370,13 @@ void input_handle_event(struct input_dev *dev,
>>   	}
>>   }
>>   
>> +static void handle_screenlock_as_meta_l(struct input_dev *dev, unsigned int type,
>> +					int value)
>> +{
>> +	input_handle_event(dev, type, KEY_LEFTMETA, value);
>> +	input_handle_event(dev, type, KEY_L, value);
>> +}
>> +
>>   /**
>>    * input_event() - report new input event
>>    * @dev: device that generated the event
>> @@ -392,6 +399,12 @@ void input_event(struct input_dev *dev,
>>   {
>>   	if (is_event_supported(type, dev->evbit, EV_MAX)) {
>>   		guard(spinlock_irqsave)(&dev->event_lock);
>> +#ifdef CONFIG_INPUT_SCREENLOCK_EMULATION
>> +		if (code == KEY_SCREENLOCK) {
>> +			handle_screenlock_as_meta_l(dev, type, value);
>> +			return;
>> +		}
>> +#endif
>>   		input_handle_event(dev, type, code, value);
>>   	}
>>   }
>> @@ -2134,6 +2147,12 @@ void input_set_capability(struct input_dev *dev, unsigned int type, unsigned int
>>   
>>   	switch (type) {
>>   	case EV_KEY:
>> +#ifdef CONFIG_INPUT_SCREENLOCK_EMULATION
>> +		if (code == KEY_SCREENLOCK) {
>> +			__set_bit(KEY_L, dev->keybit);
>> +			__set_bit(KEY_LEFTMETA, dev->keybit);
>> +		}
> 
> Shouldn't there be a "break;" inside the if?
> 
> Since KEY_SCREENLOCK events will never get reported when
> CONFIG_INPUT_SCREENLOCK_EMULATION is set, I think it would
> be netter to not set it in dev->keybit ?
> 
> Regards,

This is something I wasn't sure about.

I was worried there might be other parts of the kernel that check for a 
capability of a specific keycode and conditionalize behavior.

If that isn't a concern yes I could add a break into this block in the 
next version.

> 
> Hans
> 
> 
> 
> 
>> +#endif
>>   		__set_bit(code, dev->keybit);
>>   		break;
>>   
>