Re: [PATCHv7] platform/x86: hp-wmi: Fix platform profile option switch bug on Omen and Victus laptops

[Armin Wolf <W_Armin@gmx.de>]

Am 01.07.24 um 17:56 schrieb Armin Wolf:

> Am 01.07.24 um 17:00 schrieb Alexis Belmonte:
>
>> Fix a platform profile option switch/getter bug on some Omen and Victus
>> laptops dismissing userspace choice when selecting performance mode in
>> inadequate conditions (e.g. by being disconnected from the AC power
>> plug)
>> by
>>
>>     -  hooking an ACPI notify handler through the
>>        omen_register_powersource_notifier_handler method that listens
>> to AC
>>        power source changes (plugging in/out the AC power plug)
>>
>>     -  keeping an intermediate active_platform_profile variable that is
>>        set when userspace changes the platform profile setting
>>
>>     -  restoring the selected platform profile kept in
>>        active_platform_profile when AC power is plugged back into the
>>        laptop, unless if the user decided to alter the platform profile
>>        mid-way
>>
>> This ensures that the driver follows the principles defined in the
>> Platform Profile Selection page of the Kernel documentation on those
>> kind
>> of laptops; which is to not "(...) let userspace know about any
>> sub-optimal conditions which are impeding reaching the requested
>> performance level".
>>
>> Since the Omen and Victus laptops share the same embedded controller
>> system, the fix is applicable to both categories of laptops.
>>
>> This patch also provides improvements to how the driver sets/gets the
>> platform profile through the embedded controller, by introducing
>> intermediary functions to leverage code from
>> platform_profile_omen_set and
>> callers.
>>
>> Signed-off-by: Alexis Belmonte <alexbelm48@gmail.com>
>> ---
>> V1 -> V2: - Use register_acpi_notifier and unregister_acpi_notifier
>> instead of
>>              hooking straight through ACPI node \\_SB.ADP1
>> V2 -> V3: - Rely on power_supply_is_system_supplied() instead of an
>> EC-specific
>>              field to determine if the laptop is plugged in
>>            - Refactor omen_powersource_notify_handler to
>> omen_powersource_event
>>            - Refactor omen_powersource_register_notifier_handler to
>>              omen_register_powersource_event_handler
>>            - Use a mutex to protect the active_platform_profile
>> variable from
>>              being altered while the handler is executed
>> V3 -> V4: - Remove the unnecessary enum declaration remains from the
>> initial
>>              implementation
>> V4 -> V5: - Drop unnecessary modifications from the patch
>>            - Call platform_profile_omen_get in
>> platform_profile_victus_get to
>>              avoid code duplication
>>            - Give-up module initialization if we fail to register the
>> ACPI
>>              notifier handler
>>            - Fix code style issues reported by checkpatch.pl --strict
>>            - Add intermediary/helper platform_profile_omen_set_ec and
>>              platform_profile_victus_set_ec functions to leverage
>> code from
>>              platform_profile_omen_set and callers, thus simplifying
>>              omen_powersource_event
>>            - Fix dead-lock when restoring active_platform_profile
>> when the AC
>>              power is plugged back into the laptop
>> V5 -> V6: - Drop unnecessary modifications from the patch
>> V6 -> V7: - Drop EC platform profile readback after set
>>            - Lock the active_platform_profile mutex unconditionally
>>            - Drop the usage of ACPI_FAILURE in favor of a simpler
>> error check
>>              when registering/unregistering the ACPI notifier
>>            - Initialize active_platform_profile in thermal_profile_setup
>> ---
>>   drivers/platform/x86/hp/hp-wmi.c | 211 +++++++++++++++++++++++++++++--
>>   1 file changed, 200 insertions(+), 11 deletions(-)
>>
>> diff --git a/drivers/platform/x86/hp/hp-wmi.c
>> b/drivers/platform/x86/hp/hp-wmi.c
>> index 5fa553023842..6ffc9d7ad8c1 100644
>> --- a/drivers/platform/x86/hp/hp-wmi.c
>> +++ b/drivers/platform/x86/hp/hp-wmi.c
>> @@ -24,6 +24,7 @@
>>   #include <linux/platform_profile.h>
>>   #include <linux/hwmon.h>
>>   #include <linux/acpi.h>
>> +#include <linux/power_supply.h>

I nearly forgot: Please add "depends on POWER_SUPPLY" to the kconfig entry of HP_WMI
in drivers/platform/x86/hp/Kconfig.

Thanks,
Armin Wolf

>>   #include <linux/rfkill.h>
>>   #include <linux/string.h>
>>   #include <linux/dmi.h>
>> @@ -42,6 +43,8 @@
>> MODULE_ALIAS("wmi:5FB7F034-2C63-45E9-BE91-3D44E2C707E4");
>>   #define HP_OMEN_EC_THERMAL_PROFILE_TIMER_OFFSET 0x63
>>   #define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
>>
>> +#define ACPI_AC_CLASS "ac_adapter"
>> +
>>   #define zero_if_sup(tmp) (zero_insize_support?0:sizeof(tmp)) // use
>> when zero insize is required
>>
>>   /* DMI board names of devices that should use the omen specific
>> path for
>> @@ -259,10 +262,18 @@ static const struct key_entry hp_wmi_keymap[] = {
>>       { KE_END, 0 }
>>   };
>>
>> +/*
>> + * Mutex for the active_platform_profile variable,
>> + * see omen_powersource_event.
>> + */
>> +DEFINE_MUTEX(active_platform_profile_lock);
>> +
>>   static struct input_dev *hp_wmi_input_dev;
>>   static struct input_dev *camera_shutter_input_dev;
>>   static struct platform_device *hp_wmi_platform_dev;
>>   static struct platform_profile_handler platform_profile_handler;
>> +static struct notifier_block platform_power_source_nb;
>> +static enum platform_profile_option active_platform_profile;
>>   static bool platform_profile_support;
>>   static bool zero_insize_support;
>>
>> @@ -1194,8 +1205,7 @@ static int __init hp_wmi_rfkill2_setup(struct
>> platform_device *device)
>>       return err;
>>   }
>>
>> -static int platform_profile_omen_get(struct platform_profile_handler
>> *pprof,
>> -                     enum platform_profile_option *profile)
>> +static int platform_profile_omen_get_ec(enum platform_profile_option
>> *profile)
>>   {
>>       int tp;
>>
>> @@ -1223,6 +1233,30 @@ static int platform_profile_omen_get(struct
>> platform_profile_handler *pprof,
>>       return 0;
>>   }
>>
>> +static int platform_profile_omen_get(struct platform_profile_handler
>> *pprof,
>> +                     enum platform_profile_option *profile)
>> +{
>> +    enum platform_profile_option selected_platform_profile;
>> +
>> +    /*
>> +     * We directly return the stored platform profile, as the embedded
>> +     * controller will not accept switching to the performance
>> option when
>> +     * the conditions are not met (e.g. the laptop is not plugged in).
>> +     *
>> +     * If we directly return what the EC reports, the platform
>> profile will
>> +     * immediately "switch back" to normal mode, which is against the
>> +     * expected behaviour from a userspace point of view, as
>> described in
>> +     * the Platform Profile Section page of the kernel documentation.
>> +     *
>> +     * See also omen_powersource_event.
>> +     */
>> +    mutex_lock(&active_platform_profile_lock);
>> +    selected_platform_profile = active_platform_profile;
>> +    mutex_unlock(&active_platform_profile_lock);
>> +
>> +    return selected_platform_profile;
>> +}
>> +
>>   static bool has_omen_thermal_profile_ec_timer(void)
>>   {
>>       const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
>> @@ -1245,8 +1279,7 @@ inline int omen_thermal_profile_ec_timer_set(u8
>> value)
>>       return ec_write(HP_OMEN_EC_THERMAL_PROFILE_TIMER_OFFSET, value);
>>   }
>>
>> -static int platform_profile_omen_set(struct platform_profile_handler
>> *pprof,
>> -                     enum platform_profile_option profile)
>> +static int platform_profile_omen_set_ec(enum platform_profile_option
>> *profile)
>
> Hi,
>
> there is no reason anymore for profile to be a pointer, please fix this.
>
>>   {
>>       int err, tp, tp_version;
>>       enum hp_thermal_profile_omen_flags flags = 0;
>> @@ -1256,7 +1289,7 @@ static int platform_profile_omen_set(struct
>> platform_profile_handler *pprof,
>>       if (tp_version < 0 || tp_version > 1)
>>           return -EOPNOTSUPP;
>>
>> -    switch (profile) {
>> +    switch (*profile) {
>>       case PLATFORM_PROFILE_PERFORMANCE:
>>           if (tp_version == 0)
>>               tp = HP_OMEN_V0_THERMAL_PROFILE_PERFORMANCE;
>> @@ -1288,7 +1321,7 @@ static int platform_profile_omen_set(struct
>> platform_profile_handler *pprof,
>>           if (err < 0)
>>               return err;
>>
>> -        if (profile == PLATFORM_PROFILE_PERFORMANCE)
>> +        if (*profile == PLATFORM_PROFILE_PERFORMANCE)
>>               flags = HP_OMEN_EC_FLAGS_NOTIMER |
>>                   HP_OMEN_EC_FLAGS_TURBO;
>>
>> @@ -1300,6 +1333,25 @@ static int platform_profile_omen_set(struct
>> platform_profile_handler *pprof,
>>       return 0;
>>   }
>>
>> +static int platform_profile_omen_set(struct platform_profile_handler
>> *pprof,
>> +                     enum platform_profile_option profile)
>> +{
>> +    int err;
>> +
>> +    mutex_lock(&active_platform_profile_lock);
>> +
>> +    err = platform_profile_omen_set_ec(&profile);
>> +    if (err < 0) {
>> +        mutex_unlock(&active_platform_profile_lock);
>> +        return err;
>> +    }
>> +
>> +    active_platform_profile = profile;
>> +    mutex_unlock(&active_platform_profile_lock);
>> +
>> +    return 0;
>> +}
>> +
>>   static int thermal_profile_get(void)
>>   {
>>       return hp_wmi_read_int(HPWMI_THERMAL_PROFILE_QUERY);
>> @@ -1381,8 +1433,7 @@ static bool is_victus_thermal_profile(void)
>>                   board_name) >= 0;
>>   }
>>
>> -static int platform_profile_victus_get(struct
>> platform_profile_handler *pprof,
>> -                     enum platform_profile_option *profile)
>> +static int platform_profile_victus_get_ec(enum
>> platform_profile_option *profile)
>>   {
>>       int tp;
>>
>> @@ -1407,12 +1458,18 @@ static int platform_profile_victus_get(struct
>> platform_profile_handler *pprof,
>>       return 0;
>>   }
>>
>> -static int platform_profile_victus_set(struct
>> platform_profile_handler *pprof,
>> -                     enum platform_profile_option profile)
>> +static int platform_profile_victus_get(struct
>> platform_profile_handler *pprof,
>> +                       enum platform_profile_option *profile)
>> +{
>> +    /* Same behaviour as platform_profile_omen_get */
>> +    return platform_profile_omen_get(pprof, profile);
>> +}
>> +
>> +static int platform_profile_victus_set_ec(enum
>> platform_profile_option *profile)
>>   {
>>       int err, tp;
>>
>> -    switch (profile) {
>> +    switch (*profile) {
>
> Same as above.
>
>>       case PLATFORM_PROFILE_PERFORMANCE:
>>           tp = HP_VICTUS_THERMAL_PROFILE_PERFORMANCE;
>>           break;
>> @@ -1433,10 +1490,124 @@ static int
>> platform_profile_victus_set(struct platform_profile_handler *pprof,
>>       return 0;
>>   }
>>
>> +static int platform_profile_victus_set(struct
>> platform_profile_handler *pprof,
>> +                       enum platform_profile_option profile)
>> +{
>> +    int err;
>> +
>> +    mutex_lock(&active_platform_profile_lock);
>> +
>> +    err = platform_profile_victus_set_ec(&profile);
>> +    if (err < 0) {
>> +        mutex_unlock(&active_platform_profile_lock);
>> +        return err;
>> +    }
>> +
>> +    active_platform_profile = profile;
>> +    mutex_unlock(&active_platform_profile_lock);
>> +
>> +    return 0;
>> +}
>> +
>> +static int omen_powersource_event(struct notifier_block *nb,
>> +                  unsigned long value,
>> +                  void *data)
>> +{
>> +    struct acpi_bus_event *event_entry = data;
>> +    enum platform_profile_option actual_profile;
>> +    int err;
>> +
>> +    if (strcmp(event_entry->device_class, ACPI_AC_CLASS) != 0)
>> +        return NOTIFY_DONE;
>> +
>> +    pr_debug("Received power source device event\n");
>> +
>> +    mutex_lock(&active_platform_profile_lock);
>> +
>> +    if (is_omen_thermal_profile()) {
>> +        err = platform_profile_omen_get_ec(&actual_profile);
>> +    } else if (is_victus_thermal_profile()) {
>> +        err = platform_profile_victus_get_ec(&actual_profile);
>> +    }
>
> If neither is_omen_thermal_profile() nor is_victus_thermal_profile()
> is true,
> then err is uninitialized here.
>
> Please return without an error if both conditions are not true.
>
>> +
>> +    if (err < 0) {
>> +        pr_warn("Failed to read current platform profile (%d)\n", err);
>> +
>> +        /*
>> +         * Although we failed to get the current platform profile, we
>> +         * still want the other event consumers to process it.
>> +         */
>> +        return NOTIFY_DONE;
>> +    }
>> +
>> +    /*
>> +     * If we're back on AC and that the user-chosen power profile is
>> +     * different from what the EC reports, we restore the user-chosen
>> +     * one.
>> +     */
>> +    if (power_supply_is_system_supplied() >= 0 ||
>> +        active_platform_profile != actual_profile) {
>> +        mutex_unlock(&active_platform_profile_lock);
>> +
>> +        pr_debug("EC reports same platform profile, no platform
>> profile update required\n");
>> +        return NOTIFY_DONE;
>> +    }
>> +
>> +    if (is_omen_thermal_profile()) {
>> +        err = platform_profile_omen_set_ec(&active_platform_profile);
>> +    } else if (is_victus_thermal_profile()) {
>> +        err = platform_profile_victus_set_ec(&active_platform_profile);
>> +    }
>
> Same problem as above, please set err to zero if both conditions are
> not true.
>
>> +
>> +    if (err < 0) {
>> +        mutex_unlock(&active_platform_profile_lock);
>> +
>> +        pr_warn("Failed to restore platform profile (%d)\n", err);
>> +        return NOTIFY_DONE;
>> +    }
>> +
>> +    mutex_unlock(&active_platform_profile_lock);
>> +
>> +    return NOTIFY_OK;
>> +}
>> +
>> +static int omen_register_powersource_event_handler(void)
>> +{
>> +    int err;
>> +
>> +    if (is_omen_thermal_profile())
>> +        err = platform_profile_omen_get_ec(&active_platform_profile);
>> +    else if (is_victus_thermal_profile())
>> +        err = platform_profile_victus_get_ec(&active_platform_profile);
>
> I will say it again: active_platform_profile has to be initialized
> _before_ thermal_profile_setup()
> calls platform_profile_register().
>
> This is the wrong place to initialize active_platform_profile, its too
> late. Please drop this.
>
>> +
>> +    platform_power_source_nb.notifier_call = omen_powersource_event;
>> +    err = register_acpi_notifier(&platform_power_source_nb);
>> +
>> +    if (err < 0) {
>> +        pr_warn("Failed to install ACPI power source notify
>> handler\n");
>> +        return err;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static void omen_unregister_powersource_event_handler(void)
>> +{
>> +    int err;
>> +
>> +    err = unregister_acpi_notifier(&platform_power_source_nb);
>> +
>> +    if (err < 0)
>> +        pr_err("Failed to remove ACPI power source notify handler\n");
>> +}
>> +
>>   static int thermal_profile_setup(void)
>>   {
>>       int err, tp;
>>
>> +    if (is_omen_thermal_profile() || is_victus_thermal_profile())
>> +        active_platform_profile = PLATFORM_PROFILE_BALANCED;
>
> With "initializing active_platform_profile", i meant to actually read
> the current value from the EC.
>
> I think you can replace the calls of omen_thermal_profile_get() inside
> thermal_profile_setup()
> with platform_profile_omen_get_ec()/platform_profile_victus_get_ec()
> and use them to initialize
> active_platform_profile.
>
>> +
>>       if (is_omen_thermal_profile()) {
>>           tp = omen_thermal_profile_get();
>>           if (tp < 0)
>> @@ -1534,6 +1705,15 @@ static int __init hp_wmi_bios_setup(struct
>> platform_device *device)
>>
>>       thermal_profile_setup();
>>
>> +    /*
>> +     * Query the platform profile once to know which last power profile
>> +     * was set.
>> +     */
>> +    err =
>> platform_profile_handler.profile_get(&platform_profile_handler,
>> +                           &active_platform_profile);
>
> Again, this is too late, the platform profile is already registered.
> Please drop.
>
>> +    if (err < 0)
>> +        return err;
>> +
>>       return 0;
>>   }
>>
>> @@ -1758,6 +1938,12 @@ static int __init hp_wmi_init(void)
>>               goto err_unregister_device;
>>       }
>>
>> +    if (is_omen_thermal_profile() || is_victus_thermal_profile()) {
>> +        err = omen_register_powersource_event_handler();
>> +        if (err)
>> +            goto err_unregister_device;
>> +    }
>> +
>>       return 0;
>>
>>   err_unregister_device:
>> @@ -1772,6 +1958,9 @@ module_init(hp_wmi_init);
>>
>>   static void __exit hp_wmi_exit(void)
>>   {
>> +    if (is_omen_thermal_profile() || is_victus_thermal_profile())
>> +        omen_unregister_powersource_event_handler();
>> +
>>       if (wmi_has_guid(HPWMI_EVENT_GUID))
>>           hp_wmi_input_destroy();
>>
>