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

[Armin Wolf <W_Armin@gmx.de>]

Am 19.05.24 um 20:26 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 which listens to the \\_SB.ADP1
>        node that propagates battery status events
>
>     -  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 (handled through omen_powersource_notify_handler)
>
> 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.

Hi,

the commit description is outdated and the changelog for v2 is missing.

> Signed-off-by: Alexis Belmonte <alexbelm48@gmail.com>
> ---
>   drivers/platform/x86/hp/hp-wmi.c | 137 ++++++++++++++++++++++++++++++-
>   1 file changed, 133 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/hp/hp-wmi.c b/drivers/platform/x86/hp/hp-wmi.c
> index 630519c08617..50286128d08a 100644
> --- a/drivers/platform/x86/hp/hp-wmi.c
> +++ b/drivers/platform/x86/hp/hp-wmi.c
> @@ -38,6 +38,7 @@ MODULE_ALIAS("wmi:5FB7F034-2C63-45E9-BE91-3D44E2C707E4");
>   #define HPWMI_EVENT_GUID "95F24279-4D7B-4334-9387-ACCDC67EF61C"
>   #define HPWMI_BIOS_GUID "5FB7F034-2C63-45E9-BE91-3D44E2C707E4"
>
> +#define HP_OMEN_EC_POWER_STATUS 0x40
>   #define HP_OMEN_EC_THERMAL_PROFILE_FLAGS_OFFSET 0x62
>   #define HP_OMEN_EC_THERMAL_PROFILE_TIMER_OFFSET 0x63
>   #define HP_OMEN_EC_THERMAL_PROFILE_OFFSET 0x95
> @@ -83,6 +84,11 @@ static const char * const victus_thermal_profile_boards[] = {
>   	"8A25"
>   };
>
> +enum hp_wmi_powersource_flags {
> +	HPWMI_POWERSOURCE_AC_OFFLINE	= 0x02,
> +	HPWMI_POWERSOURCE_AC_ONLINE	= 0x0b
> +};
> +
>   enum hp_wmi_radio {
>   	HPWMI_WIFI	= 0x0,
>   	HPWMI_BLUETOOTH	= 0x1,
> @@ -261,8 +267,11 @@ static const struct key_entry hp_wmi_keymap[] = {
>
>   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 +1203,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 +1231,24 @@ 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)
> +{
> +	/*
> +	 * 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_notify_handler.
> +	 */
> +	return active_platform_profile;
> +}
> +
>   static bool has_omen_thermal_profile_ec_timer(void)
>   {
>   	const char *board_name = dmi_get_system_info(DMI_BOARD_NAME);
> @@ -1297,6 +1323,8 @@ static int platform_profile_omen_set(struct platform_profile_handler *pprof,
>   			return err;
>   	}
>
> +	active_platform_profile = profile;
> +
>   	return 0;
>   }
>
> @@ -1381,8 +1409,8 @@ 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,6 +1435,13 @@ static int platform_profile_victus_get(struct platform_profile_handler *pprof,
>   	return 0;
>   }
>
> +static int platform_profile_victus_get(struct platform_profile_handler *pprof,
> +				     enum platform_profile_option *profile)
> +{
> +	/* Same as for platform_profile_omen_get */
> +	return active_platform_profile;
> +}
> +
>   static int platform_profile_victus_set(struct platform_profile_handler *pprof,
>   				     enum platform_profile_option profile)
>   {
> @@ -1430,9 +1465,88 @@ static int platform_profile_victus_set(struct platform_profile_handler *pprof,
>   	if (err < 0)
>   		return err;
>
> +	active_platform_profile = profile;
> +
>   	return 0;
>   }
>
> +static int omen_powersource_notify_handler(struct notifier_block *nb,
> +					   unsigned long value,
> +					   void *data)
> +{
> +	struct acpi_bus_event *event_entry = data;
> +	enum platform_profile_option actual_profile;
> +	u8 state;
> +	int err;
> +
> +	if (strcmp(event_entry->device_class, "ac_adapter") != 0)
> +		return NOTIFY_DONE;
> +
> +	pr_debug("Received power source device event\n");
> +
> +	err = ec_read(HP_OMEN_EC_POWER_STATUS, &state);

Can you confirm that this will work on all HP Omen/Victus models? If not, when maybe
you should instead rely on power_supply_is_system_supplied().

> +	if (err < 0) {
> +		pr_warn("Failed to read power source state (%d)\n", err);
> +		return NOTIFY_BAD;
> +	}
> +
> +	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 (err < 0) {
> +		pr_warn("Failed to read current platform profile (%d)\n", err);
> +		return NOTIFY_BAD;
> +	}
> +
> +	if (!(state & HPWMI_POWERSOURCE_AC_ONLINE) ||
> +	    active_platform_profile == actual_profile)
> +		return NOTIFY_DONE;
> +
> +	err = platform_profile_handler.profile_set(&platform_profile_handler,
> +						   active_platform_profile);
> +	if (err < 0) {
> +		pr_warn("Failed to restore platform profile (%d)\n", err);
> +		return NOTIFY_BAD;
> +	}

I think you need to implement some type of locking here. Otherwise a user can change the selected platform
profile just after active_platform_profile has been read by omen_powersource_notify_handler(), which will
cause the notify handler to overwrite the newly selected profile.

> +
> +	return NOTIFY_OK;
> +}
> +
> +static void omen_register_powersource_notify_handler(void)
> +{
> +	int err;
> +	acpi_status status;
> +
> +	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);
> +
> +	if (err < 0) {
> +		pr_warn("Failed to retrieve active platform profile (%d)\n",
> +			err);
> +		active_platform_profile = PLATFORM_PROFILE_BALANCED;
> +	}
> +
> +	platform_power_source_nb.notifier_call = omen_powersource_notify_handler;
> +	status = register_acpi_notifier(&platform_power_source_nb);
> +
> +	if (ACPI_FAILURE(status))
> +		pr_warn("Failed to install ACPI power source notify handler\n");

Please update the function names.

Thanks,
Armin Wolf

> +}
> +
> +static void hp_wmi_unregister_powersource_notify_handler(void)
> +{
> +	acpi_status status;
> +
> +	status = unregister_acpi_notifier(&platform_power_source_nb);
> +
> +	if (ACPI_FAILURE(status))
> +		pr_err("Failed to remove ACPI power source notify handler\n");
> +}
> +
>   static int thermal_profile_setup(void)
>   {
>   	int err, tp;
> @@ -1534,6 +1648,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);
> +	if (err < 0)
> +		return err;
> +
>   	return 0;
>   }
>
> @@ -1758,6 +1881,9 @@ static int __init hp_wmi_init(void)
>   			goto err_unregister_device;
>   	}
>
> +	if (is_omen_thermal_profile() || is_victus_thermal_profile())
> +		omen_register_powersource_notify_handler();
> +
>   	return 0;
>
>   err_unregister_device:
> @@ -1772,6 +1898,9 @@ module_init(hp_wmi_init);
>
>   static void __exit hp_wmi_exit(void)
>   {
> +	if (is_omen_thermal_profile() || is_victus_thermal_profile())
> +		hp_wmi_unregister_powersource_notify_handler();
> +
>   	if (wmi_has_guid(HPWMI_EVENT_GUID))
>   		hp_wmi_input_destroy();
>