| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <G7V6HR.3WZF23WP4MCJ3@ljones.dev>
Date: Fri, 26 Aug 2022 08:50:04 +1200
From: Luke Jones <luke@...nes.dev>
To: Hans de Goede <hdegoede@...hat.com>
Cc: markgross@...nel.org, platform-driver-x86@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] asus-wmi: Adjust tablet/lidflip handling to use enum
On Wed, Aug 24 2022 at 16:12:42 +0200, Hans de Goede
<hdegoede@...hat.com> wrote:
> Hi,
>
> On 8/13/22 11:27, Luke D. Jones wrote:
>> Due to multiple types of tablet/lidflip, the existing code for
>> handling these events is refactored to use an enum for each type.
>>
>> Signed-off-by: Luke D. Jones <luke@...nes.dev>
>
> Thank you for the patch, I've merged this but with one change:
>
>> ---
>> drivers/platform/x86/asus-nb-wmi.c | 13 +++---
>> drivers/platform/x86/asus-wmi.c | 67
>> ++++++++++++++++++++----------
>> drivers/platform/x86/asus-wmi.h | 9 +++-
>> 3 files changed, 58 insertions(+), 31 deletions(-)
>>
>> diff --git a/drivers/platform/x86/asus-nb-wmi.c
>> b/drivers/platform/x86/asus-nb-wmi.c
>> index a81dc4b191b7..3a93e056c4e1 100644
>> --- a/drivers/platform/x86/asus-nb-wmi.c
>> +++ b/drivers/platform/x86/asus-nb-wmi.c
>> @@ -115,12 +115,12 @@ static struct quirk_entry quirk_asus_forceals
>> = {
>> };
>>
>> static struct quirk_entry quirk_asus_use_kbd_dock_devid = {
>> - .use_kbd_dock_devid = true,
>> + .tablet_switch_mode = asus_wmi_kbd_dock_devid,
>> };
>>
>> static struct quirk_entry quirk_asus_use_lid_flip_devid = {
>> .wmi_backlight_set_devstate = true,
>> - .use_lid_flip_devid = true,
>> + .tablet_switch_mode = asus_wmi_lid_flip_devid,
>> };
>>
>> static int dmi_matched(const struct dmi_system_id *dmi)
>> @@ -492,16 +492,13 @@ static void asus_nb_wmi_quirks(struct
>> asus_wmi_driver *driver)
>>
>> switch (tablet_mode_sw) {
>> case 0:
>> - quirks->use_kbd_dock_devid = false;
>> - quirks->use_lid_flip_devid = false;
>> + quirks->tablet_switch_mode = asus_wmi_no_tablet_switch;
>> break;
>> case 1:
>> - quirks->use_kbd_dock_devid = true;
>> - quirks->use_lid_flip_devid = false;
>> + quirks->tablet_switch_mode = asus_wmi_kbd_dock_devid;
>> break;
>> case 2:
>> - quirks->use_kbd_dock_devid = false;
>> - quirks->use_lid_flip_devid = true;
>> + quirks->tablet_switch_mode = asus_wmi_lid_flip_devid;
>> break;
>> }
>>
>> diff --git a/drivers/platform/x86/asus-wmi.c
>> b/drivers/platform/x86/asus-wmi.c
>> index 0421ffb81927..b4690ac39147 100644
>> --- a/drivers/platform/x86/asus-wmi.c
>> +++ b/drivers/platform/x86/asus-wmi.c
>> @@ -485,8 +485,11 @@ static bool asus_wmi_dev_is_present(struct
>> asus_wmi *asus, u32 dev_id)
>>
>> static int asus_wmi_input_init(struct asus_wmi *asus)
>> {
>> + struct device *dev;
>> int err, result;
>>
>> + dev = &asus->platform_device->dev;
>> +
>> asus->inputdev = input_allocate_device();
>> if (!asus->inputdev)
>> return -ENOMEM;
>> @@ -494,35 +497,38 @@ static int asus_wmi_input_init(struct
>> asus_wmi *asus)
>> asus->inputdev->name = asus->driver->input_name;
>> asus->inputdev->phys = asus->driver->input_phys;
>> asus->inputdev->id.bustype = BUS_HOST;
>> - asus->inputdev->dev.parent = &asus->platform_device->dev;
>> + asus->inputdev->dev.parent = dev;
>> set_bit(EV_REP, asus->inputdev->evbit);
>>
>> err = sparse_keymap_setup(asus->inputdev, asus->driver->keymap,
>> NULL);
>> if (err)
>> goto err_free_dev;
>>
>> - if (asus->driver->quirks->use_kbd_dock_devid) {
>> + switch (asus->driver->quirks->tablet_switch_mode) {
>> + case asus_wmi_no_tablet_switch:
>> + break;
>> + case asus_wmi_kbd_dock_devid:
>> result = asus_wmi_get_devstate_simple(asus,
>> ASUS_WMI_DEVID_KBD_DOCK);
>> if (result >= 0) {
>> input_set_capability(asus->inputdev, EV_SW, SW_TABLET_MODE);
>> input_report_switch(asus->inputdev, SW_TABLET_MODE, !result);
>> } else if (result != -ENODEV) {
>> - pr_err("Error checking for keyboard-dock: %d\n", result);
>> + dev_err(dev, "Error checking for keyboard-dock: %d\n", result);
>> }
>> - }
>> -
>> - if (asus->driver->quirks->use_lid_flip_devid) {
>> + break;
>> + case asus_wmi_lid_flip_devid:
>> result = asus_wmi_get_devstate_simple(asus,
>> ASUS_WMI_DEVID_LID_FLIP);
>> if (result < 0)
>> - asus->driver->quirks->use_lid_flip_devid = 0;
>> + asus->driver->quirks->tablet_switch_mode =
>> asus_wmi_no_tablet_switch;
>> if (result >= 0) {
>> input_set_capability(asus->inputdev, EV_SW, SW_TABLET_MODE);
>> input_report_switch(asus->inputdev, SW_TABLET_MODE, result);
>> } else if (result == -ENODEV) {
>> - pr_err("This device has lid_flip quirk but got ENODEV checking
>> it. This is a bug.");
>> + dev_err(dev, "This device has lid_flip quirk but got ENODEV
>> checking it. This is a bug.");
>> } else {
>> - pr_err("Error checking for lid-flip: %d\n", result);
>> + dev_err(dev, "Error checking for lid-flip: %d\n", result);
>> }
>> + break;
>> }
>>
>> err = input_register_device(asus->inputdev);
>> @@ -548,8 +554,9 @@ static void asus_wmi_input_exit(struct asus_wmi
>> *asus)
>>
>> static void lid_flip_tablet_mode_get_state(struct asus_wmi *asus)
>> {
>> - int result = asus_wmi_get_devstate_simple(asus,
>> ASUS_WMI_DEVID_LID_FLIP);
>> + int result;
>>
>> + result = asus_wmi_get_devstate_simple(asus,
>> ASUS_WMI_DEVID_LID_FLIP);
>> if (result >= 0) {
>> input_report_switch(asus->inputdev, SW_TABLET_MODE, result);
>> input_sync(asus->inputdev);
>> @@ -3044,20 +3051,26 @@ static void asus_wmi_handle_event_code(int
>> code, struct asus_wmi *asus)
>> return;
>> }
>>
>> - if (asus->driver->quirks->use_kbd_dock_devid && code ==
>> NOTIFY_KBD_DOCK_CHANGE) {
>
> Given that we need to check for both the tablet_switch_mode as well as
> different event codes, I believe that keep using something like:
>
> if (asus->driver->quirks->tablet_switch_mode ==
> asus_wmi_kbd_dock_devid &&
> code == NOTIFY_KBD_DOCK_CHANGE) {
>
> here is better then doing a switch-case with the code checkes nested
> under
I think I recall that Andy requested use of the switch/case but I might
be wrong.
Thank you for your help and clean up of this. The patch is making quite
a few people happy.
>
>> - result = asus_wmi_get_devstate_simple(asus,
>> - ASUS_WMI_DEVID_KBD_DOCK);
>> + switch (asus->driver->quirks->tablet_switch_mode) {
>> + case asus_wmi_no_tablet_switch:
>> + break;
>> + case asus_wmi_kbd_dock_devid:
>> + if (code == NOTIFY_KBD_DOCK_CHANGE) {
>> + result = asus_wmi_get_devstate_simple(asus,
>> ASUS_WMI_DEVID_KBD_DOCK);
>> if (result >= 0) {
>> input_report_switch(asus->inputdev, SW_TABLET_MODE,
>
> Note the indent of these 2 lines is wrong now, it should be indentend
> one
> extra step because it is inside the if (code ==
> NOTIFY_KBD_DOCK_CHANGE) {
> block now, sticking with a:
>
> if (asus->driver->quirks->tablet_switch_mode ==
> asus_wmi_kbd_dock_devid &&
> code == NOTIFY_KBD_DOCK_CHANGE) {
>
> is style instead of nesting avoids the need for this extra
> indentation.
>
> I have fixed this up while merging the patches.
>
> Regards,
>
> Hans
>
>
>
>
>> - !result);
>> + !result);
>> input_sync(asus->inputdev);
>> }
>> return;
>> - }
>> -
>> - if (asus->driver->quirks->use_lid_flip_devid && code ==
>> NOTIFY_LID_FLIP) {
>> - lid_flip_tablet_mode_get_state(asus);
>> - return;
>> + }
>> + break;
>> + case asus_wmi_lid_flip_devid:
>> + if (code == NOTIFY_LID_FLIP) {
>> + lid_flip_tablet_mode_get_state(asus);
>> + return;
>> + }
>> + break;
>> }
>>
>> if (asus->fan_boost_mode_available && code == NOTIFY_KBD_FBM) {
>> @@ -3700,8 +3713,14 @@ static int asus_hotk_resume(struct device
>> *device)
>> if (asus_wmi_has_fnlock_key(asus))
>> asus_wmi_fnlock_update(asus);
>>
>> - if (asus->driver->quirks->use_lid_flip_devid)
>> + switch (asus->driver->quirks->tablet_switch_mode) {
>> + case asus_wmi_no_tablet_switch:
>> + case asus_wmi_kbd_dock_devid:
>> + break;
>> + case asus_wmi_lid_flip_devid:
>> lid_flip_tablet_mode_get_state(asus);
>> + break;
>> + }
>>
>> return 0;
>> }
>> @@ -3742,8 +3761,14 @@ static int asus_hotk_restore(struct device
>> *device)
>> if (asus_wmi_has_fnlock_key(asus))
>> asus_wmi_fnlock_update(asus);
>>
>> - if (asus->driver->quirks->use_lid_flip_devid)
>> + switch (asus->driver->quirks->tablet_switch_mode) {
>> + case asus_wmi_no_tablet_switch:
>> + case asus_wmi_kbd_dock_devid:
>> + break;
>> + case asus_wmi_lid_flip_devid:
>> lid_flip_tablet_mode_get_state(asus);
>> + break;
>> + }
>>
>> return 0;
>> }
>> diff --git a/drivers/platform/x86/asus-wmi.h
>> b/drivers/platform/x86/asus-wmi.h
>> index b302415bf1d9..413920bad0c6 100644
>> --- a/drivers/platform/x86/asus-wmi.h
>> +++ b/drivers/platform/x86/asus-wmi.h
>> @@ -25,6 +25,12 @@ struct module;
>> struct key_entry;
>> struct asus_wmi;
>>
>> +enum asus_wmi_tablet_switch_mode {
>> + asus_wmi_no_tablet_switch,
>> + asus_wmi_kbd_dock_devid,
>> + asus_wmi_lid_flip_devid,
>> +};
>> +
>> struct quirk_entry {
>> bool hotplug_wireless;
>> bool scalar_panel_brightness;
>> @@ -33,8 +39,7 @@ struct quirk_entry {
>> bool wmi_backlight_native;
>> bool wmi_backlight_set_devstate;
>> bool wmi_force_als_set;
>> - bool use_kbd_dock_devid;
>> - bool use_lid_flip_devid;
>> + enum asus_wmi_tablet_switch_mode tablet_switch_mode;
>> int wapf;
>> /*
>> * For machines with AMD graphic chips, it will send out WMI event
>
Powered by blists - more mailing lists