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

[Armin Wolf <W_Armin@xxxxxx>]

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@xxxxxxxxx>
> > ---
> > 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();