Re: Dell Vostro V131 hotkeys revisited

[Hans de Goede <hdegoede@redhat.com>]

Hi,

On 17-12-15 19:47, Pali Rohár wrote:
> Hi Hans! See my comments below about your patches.
>
>>  From 9355058f5c1d7c815a293e0c0731d85f0a59b4a1 Mon Sep 17 00:00:00 2001
>> From: Hans de Goede <hdegoede@redhat.com>
>> Date: Thu, 17 Dec 2015 09:59:01 +0100
>> Subject: [PATCH 2/4] dell-wmi: Use acpi_video_handles_brightness_key_presses()
>>
>> Use the new acpi_video_handles_brightness_key_presses function to check
>> if we should report brightness key-presses.
>>
>> This makes the code both easier to read and makes it properly report
>> key-presses when acpi-video is not reporting them for reasons other
>> then the backlight type being vendor.
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/platform/x86/dell-wmi.c | 6 ++----
>>   1 file changed, 2 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/platform/x86/dell-wmi.c b/drivers/platform/x86/dell-wmi.c
>> index f2d77fe..cb8a9c2 100644
>> --- a/drivers/platform/x86/dell-wmi.c
>> +++ b/drivers/platform/x86/dell-wmi.c
>> @@ -43,8 +43,6 @@ MODULE_LICENSE("GPL");
>>
>>   #define DELL_EVENT_GUID "9DBB5994-A997-11DA-B012-B622A1EF5492"
>>
>> -static int acpi_video;
>> -
>>   MODULE_ALIAS("wmi:"DELL_EVENT_GUID);
>>
>>   /*
>> @@ -159,7 +157,8 @@ static void dell_wmi_process_key(int reported_key)
>>
>>   	/* Don't report brightness notifications that will also come via ACPI */
>>   	if ((key->keycode == KEY_BRIGHTNESSUP ||
>> -	     key->keycode == KEY_BRIGHTNESSDOWN) && acpi_video)
>> +	     key->keycode == KEY_BRIGHTNESSDOWN) &&
>> +	    acpi_video_handles_brightness_key_presses())
>
> I do not like this, because that function uses mutex and is called every
> time when brightness key event is received by wmi notify handler.

Right and this is going to happen 1000-s of times a second so is
really going to be a performance problem (not).

We cannot cache the return value as was being done before because it
can change during startup depending in module loading order (the old
code actually got this somewhat wrong), and taking a mutex in a code-path
which gets executed only once a second tops is really a non issue.

>
>>   		return;
>>
>>   	sparse_keymap_report_entry(dell_wmi_input_dev, key, 1, true);
>> @@ -398,7 +397,6 @@ static int __init dell_wmi_init(void)
>>   	}
>>
>>   	dmi_walk(find_hk_type, NULL);
>> -	acpi_video = acpi_video_get_backlight_type() != acpi_backlight_vendor;
>>
>>   	err = dell_wmi_input_setup();
>>   	if (err)
>> --
>> 2.5.0
>>
>
>>  From 5e7ff99407aeb60c2f1516cdd80d7859646497dd Mon Sep 17 00:00:00 2001
>> From: Hans de Goede <hdegoede@redhat.com>
>> Date: Wed, 16 Dec 2015 11:19:00 +0100
>> Subject: [PATCH 4/4] acpi-video: Add a module option to disable the reporting
>>   of keypresses
>>
>> Add a module option to disable the reporting of keypresses, in some buggy
>> firmware implementatinon, the reported events are wrong. E.g. they lag
>> reality by one event in the case triggering the writing of this patch.
>>
>> In this case it is better to not forward these wrong events to userspace
>> (esp.) when there is another source of the same events which is not buggy.
>>
>> Note this is only intended to work around implementations which send
>> events which are plain wrong. In some cases we get double events, e.g.
>> from both acpi-video and the atkbd driver, in this case acpi-video is
>> considered the canonical source, and the events from the other source
>> should be filtered (using e.g. /lib/udev/hwdb.d/60-keyboard.hwdb).
>>
>> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
>> ---
>>   drivers/acpi/acpi_video.c | 14 +++++++++++---
>>   1 file changed, 11 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/acpi/acpi_video.c b/drivers/acpi/acpi_video.c
>> index 2a649f3e..2971154 100644
>> --- a/drivers/acpi/acpi_video.c
>> +++ b/drivers/acpi/acpi_video.c
>> @@ -77,6 +77,13 @@ module_param(allow_duplicates, bool, 0644);
>>   static int disable_backlight_sysfs_if = -1;
>>   module_param(disable_backlight_sysfs_if, int, 0444);
>>
>> +#define REPORT_OUTPUT_KEY_EVENTS		0x01
>> +#define REPORT_BRIGHTNESS_KEY_EVENTS		0x02
>> +static int report_key_events = -1;
>> +module_param(report_key_events, int, 0644);
>> +MODULE_PARM_DESC(report_key_events,
>> +	"0: none, 1: output changes, 2: brightness changes, 3: all");
>> +
>
> -1 is default value? Should not be it 3? Or -1 as some default which use
> quirks?

Right, the -1 is to later detect if the value was overriden from the kernel
cmdline so that we do not apply dmi quirks when the value comes from the
kernel cmdline.


>
>>   static bool device_id_scheme = false;
>>   module_param(device_id_scheme, bool, 0444);
>>
>> @@ -1480,7 +1487,7 @@ static void acpi_video_bus_notify(struct acpi_device *device, u32 event)
>>   		/* Something vetoed the keypress. */
>>   		keycode = 0;
>>
>> -	if (keycode) {
>> +	if (keycode && (report_key_events & REPORT_OUTPUT_KEY_EVENTS)) {
>>   		input_report_key(input, keycode, 1);
>>   		input_sync(input);
>>   		input_report_key(input, keycode, 0);
>> @@ -1544,7 +1551,7 @@ static void acpi_video_device_notify(acpi_handle handle, u32 event, void *data)
>>
>>   	acpi_notifier_call_chain(device, event, 0);
>>
>> -	if (keycode) {
>> +	if (keycode && (report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS)) {
>>   		input_report_key(input, keycode, 1);
>>   		input_sync(input);
>>   		input_report_key(input, keycode, 0);
>> @@ -2080,7 +2087,8 @@ bool acpi_video_handles_brightness_key_presses(void)
>>   	have_video_busses = !list_empty(&video_bus_head);
>>   	mutex_unlock(&video_list_lock);
>>
>> -	return have_video_busses;
>> +	return have_video_busses &&
>> +	       (report_key_events & REPORT_BRIGHTNESS_KEY_EVENTS);
>>   }
>>   EXPORT_SYMBOL(acpi_video_handles_brightness_key_presses);
>>
>> --
>> 2.5.0
>>
>

Regards,

Hans