[PATCH] asus-wmi: Add support for platform_profile

[PATCH] asus-wmi: Add support for platform_profile

[Luke D. Jones]

Add initial support for platform_profile where the support is
based on availability of ASUS_THROTTLE_THERMAL_POLICY.

Signed-off-by: Luke D. Jones <luke@ljones.dev>
---
 drivers/platform/x86/asus-wmi.c | 112 ++++++++++++++++++++++++++++++++
 1 file changed, 112 insertions(+)

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 90a6a0d00deb..62260043c15c 100644
--- a/drivers/platform/x86/asus-wmi.c
+++ b/drivers/platform/x86/asus-wmi.c
@@ -26,6 +26,7 @@
 #include <linux/rfkill.h>
 #include <linux/pci.h>
 #include <linux/pci_hotplug.h>
+#include <linux/platform_profile.h>
 #include <linux/power_supply.h>
 #include <linux/hwmon.h>
 #include <linux/hwmon-sysfs.h>
@@ -219,6 +220,9 @@ struct asus_wmi {
 	bool throttle_thermal_policy_available;
 	u8 throttle_thermal_policy_mode;
 
+	struct platform_profile_handler platform_profile_handler;
+	bool platform_profile_support;
+
 	// The RSOC controls the maximum charging percentage.
 	bool battery_rsoc_available;
 
@@ -2144,6 +2148,106 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
 // Throttle thermal policy: 0 - default, 1 - overboost, 2 - silent
 static DEVICE_ATTR_RW(throttle_thermal_policy);
 
+/* Platform profile ***********************************************************/
+static int platform_profile_get(struct platform_profile_handler *pprof,
+				enum platform_profile_option *profile)
+{
+	struct asus_wmi *asus;
+	int tp;
+
+	asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
+
+	/* All possible toggles like throttle_thermal_policy here */
+	if (asus->throttle_thermal_policy_available) {
+		tp = asus->throttle_thermal_policy_mode;
+	} else {
+		return -1;
+	}
+
+	if (tp < 0)
+		return tp;
+
+	switch (tp) {
+	case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
+		*profile = PLATFORM_PROFILE_BALANCED;
+		break;
+	case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
+		*profile = PLATFORM_PROFILE_PERFORMANCE;
+		break;
+	case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
+		*profile = PLATFORM_PROFILE_QUIET;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
+static int platform_profile_set(struct platform_profile_handler *pprof,
+				enum platform_profile_option profile)
+{
+	struct asus_wmi *asus;
+	int tp;
+
+	asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
+
+	/* All possible toggles like throttle_thermal_policy here */
+	if (!asus->throttle_thermal_policy_available) {
+		return -1;
+	}
+
+	switch (profile) {
+	case PLATFORM_PROFILE_PERFORMANCE:
+		tp = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
+		break;
+	case PLATFORM_PROFILE_BALANCED:
+		tp = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
+		break;
+	case PLATFORM_PROFILE_QUIET:
+		tp = ASUS_THROTTLE_THERMAL_POLICY_SILENT;
+		break;
+	default:
+		return -EOPNOTSUPP;
+	}
+
+	if (asus->throttle_thermal_policy_available) {
+		asus->throttle_thermal_policy_mode = tp;
+		return throttle_thermal_policy_write(asus);
+	}
+	return 0;
+}
+
+static int platform_profile_setup(struct asus_wmi *asus)
+{
+	int err;
+
+	if (asus->throttle_thermal_policy_available) {
+		pr_info("Using throttle_thermal_policy for platform_profile support\n");
+	} else {
+		/*
+		 * Not an error if a component platform_profile relies on is unavailable
+		 * so early return, skipping the setup of platform_profile.
+		*/
+		return 0;
+	}
+	asus->platform_profile_handler.profile_get = platform_profile_get;
+	asus->platform_profile_handler.profile_set = platform_profile_set;
+
+	set_bit(PLATFORM_PROFILE_QUIET, asus->platform_profile_handler.choices);
+	set_bit(PLATFORM_PROFILE_BALANCED,
+		asus->platform_profile_handler.choices);
+	set_bit(PLATFORM_PROFILE_PERFORMANCE,
+		asus->platform_profile_handler.choices);
+
+	err = platform_profile_register(&asus->platform_profile_handler);
+	if (err)
+		return err;
+
+	asus->platform_profile_support = true;
+	return 0;
+}
+
 /* Backlight ******************************************************************/
 
 static int read_backlight_power(struct asus_wmi *asus)
@@ -2904,6 +3008,10 @@ static int asus_wmi_add(struct platform_device *pdev)
 	else
 		throttle_thermal_policy_set_default(asus);
 
+	err = platform_profile_setup(asus);
+	if (err)
+		goto fail_platform_profile_setup;
+
 	err = panel_od_check_present(asus);
 	if (err)
 		goto fail_panel_od;
@@ -2993,6 +3101,7 @@ static int asus_wmi_add(struct platform_device *pdev)
 	asus_wmi_sysfs_exit(asus->platform_device);
 fail_sysfs:
 fail_throttle_thermal_policy:
+fail_platform_profile_setup:
 fail_fan_boost_mode:
 fail_egpu_enable:
 fail_dgpu_disable:
@@ -3017,6 +3126,9 @@ static int asus_wmi_remove(struct platform_device *device)
 	asus_fan_set_auto(asus);
 	asus_wmi_battery_exit(asus);
 
+	if (asus->platform_profile_support)
+		platform_profile_remove();
+
 	kfree(asus);
 	return 0;
 }
-- 
2.31.1

Re: [PATCH] asus-wmi: Add support for platform_profile

[Luke Jones]

I'm unsure of how to update the existing code for fn+f5 (next thermal 
profile) used by laptops like the TUF series that have keyboard over 
i2c. I was thinking of the following:

static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
{
 struct platform_profile_handler *handler = 
&asus->platform_profile_handler; // added
 u8 new_mode = asus->throttle_thermal_policy_mode + 1;

 if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
  new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;

 // asus->throttle_thermal_policy_mode = new_mode;
 // return throttle_thermal_policy_write(asus);
 return handler->profile_set(&asus->platform_profile_handler, 
new_mode); // added
}

* two lines added, two commented

I'm not able to test this though, and there are very few active people 
in my discord with TUF/i2c laptops to ask for testing also.

My concern here is to get the platform_profile correctly updated. 
Simply updating asus->throttle_thermal_policy_mode isn't going to 
achieve what we'll want.

Regards,
Luke.


On Fri, Aug 13 2021 at 14:42:01 +1200, Luke D. Jones <luke@ljones.dev> 
wrote:
> Add initial support for platform_profile where the support is
> based on availability of ASUS_THROTTLE_THERMAL_POLICY.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c | 112 
> ++++++++++++++++++++++++++++++++
>  1 file changed, 112 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c 
> b/drivers/platform/x86/asus-wmi.c
> index 90a6a0d00deb..62260043c15c 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -26,6 +26,7 @@
>  #include <linux/rfkill.h>
>  #include <linux/pci.h>
>  #include <linux/pci_hotplug.h>
> +#include <linux/platform_profile.h>
>  #include <linux/power_supply.h>
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
> @@ -219,6 +220,9 @@ struct asus_wmi {
>  	bool throttle_thermal_policy_available;
>  	u8 throttle_thermal_policy_mode;
> 
> +	struct platform_profile_handler platform_profile_handler;
> +	bool platform_profile_support;
> +
>  	// The RSOC controls the maximum charging percentage.
>  	bool battery_rsoc_available;
> 
> @@ -2144,6 +2148,106 @@ static ssize_t 
> throttle_thermal_policy_store(struct device *dev,
>  // Throttle thermal policy: 0 - default, 1 - overboost, 2 - silent
>  static DEVICE_ATTR_RW(throttle_thermal_policy);
> 
> +/* Platform profile 
> ***********************************************************/
> +static int platform_profile_get(struct platform_profile_handler 
> *pprof,
> +				enum platform_profile_option *profile)
> +{
> +	struct asus_wmi *asus;
> +	int tp;
> +
> +	asus = container_of(pprof, struct asus_wmi, 
> platform_profile_handler);
> +
> +	/* All possible toggles like throttle_thermal_policy here */
> +	if (asus->throttle_thermal_policy_available) {
> +		tp = asus->throttle_thermal_policy_mode;
> +	} else {
> +		return -1;
> +	}
> +
> +	if (tp < 0)
> +		return tp;
> +
> +	switch (tp) {
> +	case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
> +		*profile = PLATFORM_PROFILE_BALANCED;
> +		break;
> +	case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
> +		*profile = PLATFORM_PROFILE_PERFORMANCE;
> +		break;
> +	case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
> +		*profile = PLATFORM_PROFILE_QUIET;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int platform_profile_set(struct platform_profile_handler 
> *pprof,
> +				enum platform_profile_option profile)
> +{
> +	struct asus_wmi *asus;
> +	int tp;
> +
> +	asus = container_of(pprof, struct asus_wmi, 
> platform_profile_handler);
> +
> +	/* All possible toggles like throttle_thermal_policy here */
> +	if (!asus->throttle_thermal_policy_available) {
> +		return -1;
> +	}
> +
> +	switch (profile) {
> +	case PLATFORM_PROFILE_PERFORMANCE:
> +		tp = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
> +		break;
> +	case PLATFORM_PROFILE_BALANCED:
> +		tp = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
> +		break;
> +	case PLATFORM_PROFILE_QUIET:
> +		tp = ASUS_THROTTLE_THERMAL_POLICY_SILENT;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (asus->throttle_thermal_policy_available) {
> +		asus->throttle_thermal_policy_mode = tp;
> +		return throttle_thermal_policy_write(asus);
> +	}
> +	return 0;
> +}
> +
> +static int platform_profile_setup(struct asus_wmi *asus)
> +{
> +	int err;
> +
> +	if (asus->throttle_thermal_policy_available) {
> +		pr_info("Using throttle_thermal_policy for platform_profile 
> support\n");
> +	} else {
> +		/*
> +		 * Not an error if a component platform_profile relies on is 
> unavailable
> +		 * so early return, skipping the setup of platform_profile.
> +		*/
> +		return 0;
> +	}
> +	asus->platform_profile_handler.profile_get = platform_profile_get;
> +	asus->platform_profile_handler.profile_set = platform_profile_set;
> +
> +	set_bit(PLATFORM_PROFILE_QUIET, 
> asus->platform_profile_handler.choices);
> +	set_bit(PLATFORM_PROFILE_BALANCED,
> +		asus->platform_profile_handler.choices);
> +	set_bit(PLATFORM_PROFILE_PERFORMANCE,
> +		asus->platform_profile_handler.choices);
> +
> +	err = platform_profile_register(&asus->platform_profile_handler);
> +	if (err)
> +		return err;
> +
> +	asus->platform_profile_support = true;
> +	return 0;
> +}
> +
>  /* Backlight 
> ******************************************************************/
> 
>  static int read_backlight_power(struct asus_wmi *asus)
> @@ -2904,6 +3008,10 @@ static int asus_wmi_add(struct platform_device 
> *pdev)
>  	else
>  		throttle_thermal_policy_set_default(asus);
> 
> +	err = platform_profile_setup(asus);
> +	if (err)
> +		goto fail_platform_profile_setup;
> +
>  	err = panel_od_check_present(asus);
>  	if (err)
>  		goto fail_panel_od;
> @@ -2993,6 +3101,7 @@ static int asus_wmi_add(struct platform_device 
> *pdev)
>  	asus_wmi_sysfs_exit(asus->platform_device);
>  fail_sysfs:
>  fail_throttle_thermal_policy:
> +fail_platform_profile_setup:
>  fail_fan_boost_mode:
>  fail_egpu_enable:
>  fail_dgpu_disable:
> @@ -3017,6 +3126,9 @@ static int asus_wmi_remove(struct 
> platform_device *device)
>  	asus_fan_set_auto(asus);
>  	asus_wmi_battery_exit(asus);
> 
> +	if (asus->platform_profile_support)
> +		platform_profile_remove();
> +
>  	kfree(asus);
>  	return 0;
>  }
> --
> 2.31.1
> 

Re: [PATCH] asus-wmi: Add support for platform_profile

[Hans de Goede]

Hi,

On 8/13/21 7:27 AM, Luke Jones wrote:
> I'm unsure of how to update the existing code for fn+f5 (next thermal profile) used by laptops like the TUF series that have keyboard over i2c. I was thinking of the following:
> 
> static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
> {
> struct platform_profile_handler *handler = &asus->platform_profile_handler; // added
> u8 new_mode = asus->throttle_thermal_policy_mode + 1;
> 
> if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
>  new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
> 
> // asus->throttle_thermal_policy_mode = new_mode;
> // return throttle_thermal_policy_write(asus);
> return handler->profile_set(&asus->platform_profile_handler, new_mode); // added
> }
> 
> * two lines added, two commented

I was going to say it is best to just send a key-press event to userspace
(we can define a new EV_KEY_.... code for this) and then let userspace
handle things. But I see that this is currently already handled by the kernel,
so that is not really an option.

> I'm not able to test this though, and there are very few active people in my discord with TUF/i2c laptops to ask for testing also.
> 
> My concern here is to get the platform_profile correctly updated. Simply updating asus->throttle_thermal_policy_mode isn't going to achieve what we'll want.

Right, there is no need to go through handler->profile_set() you have defined
profile_set yourself after all and it does not do anything different then the
old code you are replacing here.

The trick is to call platform_profile_notify() after throttle_thermal_policy_write()
this will let userspace, e.g. power-profiles-daemon know that the profile has
been changed and it will re-read it then, resulting in a call to
handler->profile_get()

So the new throttle_thermal_policy_switch_next() would look like this:

static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
{
        u8 new_mode = asus->throttle_thermal_policy_mode + 1;
	int err; // new

        if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
                new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;

        asus->throttle_thermal_policy_mode = new_mode;

        err = throttle_thermal_policy_write(asus); // changed
	if (err == 0)                              // new
		platform_profile_notify();         // new

	return err;                                // new
}

As you can see the only new thing here is the
platform_profile_notify() call on a successful write,
which is such a small change that I'm not overly worried about
not being able to test this.

I hope this helps.

Regards,

Hans


> On Fri, Aug 13 2021 at 14:42:01 +1200, Luke D. Jones <luke@ljones.dev> wrote:
>> Add initial support for platform_profile where the support is
>> based on availability of ASUS_THROTTLE_THERMAL_POLICY.
>>
>> Signed-off-by: Luke D. Jones <luke@ljones.dev>
>> ---
>>  drivers/platform/x86/asus-wmi.c | 112 ++++++++++++++++++++++++++++++++
>>  1 file changed, 112 insertions(+)
>>
>> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
>> index 90a6a0d00deb..62260043c15c 100644
>> --- a/drivers/platform/x86/asus-wmi.c
>> +++ b/drivers/platform/x86/asus-wmi.c
>> @@ -26,6 +26,7 @@
>>  #include <linux/rfkill.h>
>>  #include <linux/pci.h>
>>  #include <linux/pci_hotplug.h>
>> +#include <linux/platform_profile.h>
>>  #include <linux/power_supply.h>
>>  #include <linux/hwmon.h>
>>  #include <linux/hwmon-sysfs.h>
>> @@ -219,6 +220,9 @@ struct asus_wmi {
>>      bool throttle_thermal_policy_available;
>>      u8 throttle_thermal_policy_mode;
>>
>> +    struct platform_profile_handler platform_profile_handler;
>> +    bool platform_profile_support;
>> +
>>      // The RSOC controls the maximum charging percentage.
>>      bool battery_rsoc_available;
>>
>> @@ -2144,6 +2148,106 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
>>  // Throttle thermal policy: 0 - default, 1 - overboost, 2 - silent
>>  static DEVICE_ATTR_RW(throttle_thermal_policy);
>>
>> +/* Platform profile ***********************************************************/
>> +static int platform_profile_get(struct platform_profile_handler *pprof,
>> +                enum platform_profile_option *profile)
>> +{
>> +    struct asus_wmi *asus;
>> +    int tp;
>> +
>> +    asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
>> +
>> +    /* All possible toggles like throttle_thermal_policy here */
>> +    if (asus->throttle_thermal_policy_available) {
>> +        tp = asus->throttle_thermal_policy_mode;
>> +    } else {
>> +        return -1;
>> +    }
>> +
>> +    if (tp < 0)
>> +        return tp;
>> +
>> +    switch (tp) {
>> +    case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
>> +        *profile = PLATFORM_PROFILE_BALANCED;
>> +        break;
>> +    case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
>> +        *profile = PLATFORM_PROFILE_PERFORMANCE;
>> +        break;
>> +    case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
>> +        *profile = PLATFORM_PROFILE_QUIET;
>> +        break;
>> +    default:
>> +        return -EINVAL;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int platform_profile_set(struct platform_profile_handler *pprof,
>> +                enum platform_profile_option profile)
>> +{
>> +    struct asus_wmi *asus;
>> +    int tp;
>> +
>> +    asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
>> +
>> +    /* All possible toggles like throttle_thermal_policy here */
>> +    if (!asus->throttle_thermal_policy_available) {
>> +        return -1;
>> +    }
>> +
>> +    switch (profile) {
>> +    case PLATFORM_PROFILE_PERFORMANCE:
>> +        tp = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
>> +        break;
>> +    case PLATFORM_PROFILE_BALANCED:
>> +        tp = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>> +        break;
>> +    case PLATFORM_PROFILE_QUIET:
>> +        tp = ASUS_THROTTLE_THERMAL_POLICY_SILENT;
>> +        break;
>> +    default:
>> +        return -EOPNOTSUPP;
>> +    }
>> +
>> +    if (asus->throttle_thermal_policy_available) {
>> +        asus->throttle_thermal_policy_mode = tp;
>> +        return throttle_thermal_policy_write(asus);
>> +    }
>> +    return 0;
>> +}
>> +
>> +static int platform_profile_setup(struct asus_wmi *asus)
>> +{
>> +    int err;
>> +
>> +    if (asus->throttle_thermal_policy_available) {
>> +        pr_info("Using throttle_thermal_policy for platform_profile support\n");
>> +    } else {
>> +        /*
>> +         * Not an error if a component platform_profile relies on is unavailable
>> +         * so early return, skipping the setup of platform_profile.
>> +        */
>> +        return 0;
>> +    }
>> +    asus->platform_profile_handler.profile_get = platform_profile_get;
>> +    asus->platform_profile_handler.profile_set = platform_profile_set;
>> +
>> +    set_bit(PLATFORM_PROFILE_QUIET, asus->platform_profile_handler.choices);
>> +    set_bit(PLATFORM_PROFILE_BALANCED,
>> +        asus->platform_profile_handler.choices);
>> +    set_bit(PLATFORM_PROFILE_PERFORMANCE,
>> +        asus->platform_profile_handler.choices);
>> +
>> +    err = platform_profile_register(&asus->platform_profile_handler);
>> +    if (err)
>> +        return err;
>> +
>> +    asus->platform_profile_support = true;
>> +    return 0;
>> +}
>> +
>>  /* Backlight ******************************************************************/
>>
>>  static int read_backlight_power(struct asus_wmi *asus)
>> @@ -2904,6 +3008,10 @@ static int asus_wmi_add(struct platform_device *pdev)
>>      else
>>          throttle_thermal_policy_set_default(asus);
>>
>> +    err = platform_profile_setup(asus);
>> +    if (err)
>> +        goto fail_platform_profile_setup;
>> +
>>      err = panel_od_check_present(asus);
>>      if (err)
>>          goto fail_panel_od;
>> @@ -2993,6 +3101,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>>      asus_wmi_sysfs_exit(asus->platform_device);
>>  fail_sysfs:
>>  fail_throttle_thermal_policy:
>> +fail_platform_profile_setup:
>>  fail_fan_boost_mode:
>>  fail_egpu_enable:
>>  fail_dgpu_disable:
>> @@ -3017,6 +3126,9 @@ static int asus_wmi_remove(struct platform_device *device)
>>      asus_fan_set_auto(asus);
>>      asus_wmi_battery_exit(asus);
>>
>> +    if (asus->platform_profile_support)
>> +        platform_profile_remove();
>> +
>>      kfree(asus);
>>      return 0;
>>  }
>> -- 
>> 2.31.1
>>
> 
> 

Re: [PATCH] asus-wmi: Add support for platform_profile

[Luke Jones]

On Fri, Aug 13 2021 at 09:03:04 +0200, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi,
> 
> On 8/13/21 7:27 AM, Luke Jones wrote:
>>  I'm unsure of how to update the existing code for fn+f5 (next 
>> thermal profile) used by laptops like the TUF series that have 
>> keyboard over i2c. I was thinking of the following:
>> 
>>  static int throttle_thermal_policy_switch_next(struct asus_wmi 
>> *asus)
>>  {
>>  struct platform_profile_handler *handler = 
>> &asus->platform_profile_handler; // added
>>  u8 new_mode = asus->throttle_thermal_policy_mode + 1;
>> 
>>  if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
>>   new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>> 
>>  // asus->throttle_thermal_policy_mode = new_mode;
>>  // return throttle_thermal_policy_write(asus);
>>  return handler->profile_set(&asus->platform_profile_handler, 
>> new_mode); // added
>>  }
>> 
>>  * two lines added, two commented
> 
> I was going to say it is best to just send a key-press event to 
> userspace
> (we can define a new EV_KEY_.... code for this) and then let userspace
> handle things. But I see that this is currently already handled by 
> the kernel,
> so that is not really an option.
> 
>>  I'm not able to test this though, and there are very few active 
>> people in my discord with TUF/i2c laptops to ask for testing also.
>> 
>>  My concern here is to get the platform_profile correctly updated. 
>> Simply updating asus->throttle_thermal_policy_mode isn't going to 
>> achieve what we'll want.
> 
> Right, there is no need to go through handler->profile_set() you have 
> defined
> profile_set yourself after all and it does not do anything different 
> then the
> old code you are replacing here.
> 
> The trick is to call platform_profile_notify() after 
> throttle_thermal_policy_write()
> this will let userspace, e.g. power-profiles-daemon know that the 
> profile has
> been changed and it will re-read it then, resulting in a call to
> handler->profile_get()
> 
> So the new throttle_thermal_policy_switch_next() would look like this:
> 
> static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
> {
>         u8 new_mode = asus->throttle_thermal_policy_mode + 1;
> 	int err; // new
> 
>         if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
>                 new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
> 
>         asus->throttle_thermal_policy_mode = new_mode;
> 
>         err = throttle_thermal_policy_write(asus); // changed
> 	if (err == 0)                              // new
> 		platform_profile_notify();         // new
> 
> 	return err;                                // new
> }
> 
> As you can see the only new thing here is the
> platform_profile_notify() call on a successful write,
> which is such a small change that I'm not overly worried about
> not being able to test this.
> 
> I hope this helps.
> 
> Regards,
> 
> Hans
> 
> 
>>  On Fri, Aug 13 2021 at 14:42:01 +1200, Luke D. Jones 
>> <luke@ljones.dev> wrote:
>>>  Add initial support for platform_profile where the support is
>>>  based on availability of ASUS_THROTTLE_THERMAL_POLICY.
>>> 
>>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>>  ---
>>>   drivers/platform/x86/asus-wmi.c | 112 
>>> ++++++++++++++++++++++++++++++++
>>>   1 file changed, 112 insertions(+)
>>> 
>>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>>> b/drivers/platform/x86/asus-wmi.c
>>>  index 90a6a0d00deb..62260043c15c 100644
>>>  --- a/drivers/platform/x86/asus-wmi.c
>>>  +++ b/drivers/platform/x86/asus-wmi.c
>>>  @@ -26,6 +26,7 @@
>>>   #include <linux/rfkill.h>
>>>   #include <linux/pci.h>
>>>   #include <linux/pci_hotplug.h>
>>>  +#include <linux/platform_profile.h>
>>>   #include <linux/power_supply.h>
>>>   #include <linux/hwmon.h>
>>>   #include <linux/hwmon-sysfs.h>
>>>  @@ -219,6 +220,9 @@ struct asus_wmi {
>>>       bool throttle_thermal_policy_available;
>>>       u8 throttle_thermal_policy_mode;
>>> 
>>>  +    struct platform_profile_handler platform_profile_handler;
>>>  +    bool platform_profile_support;
>>>  +
>>>       // The RSOC controls the maximum charging percentage.
>>>       bool battery_rsoc_available;
>>> 
>>>  @@ -2144,6 +2148,106 @@ static ssize_t 
>>> throttle_thermal_policy_store(struct device *dev,
>>>   // Throttle thermal policy: 0 - default, 1 - overboost, 2 - silent
>>>   static DEVICE_ATTR_RW(throttle_thermal_policy);
>>> 
>>>  +/* Platform profile 
>>> ***********************************************************/
>>>  +static int platform_profile_get(struct platform_profile_handler 
>>> *pprof,
>>>  +                enum platform_profile_option *profile)
>>>  +{
>>>  +    struct asus_wmi *asus;
>>>  +    int tp;
>>>  +
>>>  +    asus = container_of(pprof, struct asus_wmi, 
>>> platform_profile_handler);
>>>  +
>>>  +    /* All possible toggles like throttle_thermal_policy here */
>>>  +    if (asus->throttle_thermal_policy_available) {
>>>  +        tp = asus->throttle_thermal_policy_mode;
>>>  +    } else {
>>>  +        return -1;
>>>  +    }
>>>  +
>>>  +    if (tp < 0)
>>>  +        return tp;
>>>  +
>>>  +    switch (tp) {
>>>  +    case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
>>>  +        *profile = PLATFORM_PROFILE_BALANCED;
>>>  +        break;
>>>  +    case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
>>>  +        *profile = PLATFORM_PROFILE_PERFORMANCE;
>>>  +        break;
>>>  +    case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
>>>  +        *profile = PLATFORM_PROFILE_QUIET;
>>>  +        break;
>>>  +    default:
>>>  +        return -EINVAL;
>>>  +    }
>>>  +
>>>  +    return 0;
>>>  +}
>>>  +
>>>  +static int platform_profile_set(struct platform_profile_handler 
>>> *pprof,
>>>  +                enum platform_profile_option profile)
>>>  +{
>>>  +    struct asus_wmi *asus;
>>>  +    int tp;
>>>  +
>>>  +    asus = container_of(pprof, struct asus_wmi, 
>>> platform_profile_handler);
>>>  +
>>>  +    /* All possible toggles like throttle_thermal_policy here */
>>>  +    if (!asus->throttle_thermal_policy_available) {
>>>  +        return -1;
>>>  +    }
>>>  +
>>>  +    switch (profile) {
>>>  +    case PLATFORM_PROFILE_PERFORMANCE:
>>>  +        tp = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
>>>  +        break;
>>>  +    case PLATFORM_PROFILE_BALANCED:
>>>  +        tp = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>>>  +        break;
>>>  +    case PLATFORM_PROFILE_QUIET:
>>>  +        tp = ASUS_THROTTLE_THERMAL_POLICY_SILENT;
>>>  +        break;
>>>  +    default:
>>>  +        return -EOPNOTSUPP;
>>>  +    }
>>>  +
>>>  +    if (asus->throttle_thermal_policy_available) {
>>>  +        asus->throttle_thermal_policy_mode = tp;
>>>  +        return throttle_thermal_policy_write(asus);
>>>  +    }
>>>  +    return 0;
>>>  +}
>>>  +
>>>  +static int platform_profile_setup(struct asus_wmi *asus)
>>>  +{
>>>  +    int err;
>>>  +
>>>  +    if (asus->throttle_thermal_policy_available) {
>>>  +        pr_info("Using throttle_thermal_policy for 
>>> platform_profile support\n");
>>>  +    } else {
>>>  +        /*
>>>  +         * Not an error if a component platform_profile relies on 
>>> is unavailable
>>>  +         * so early return, skipping the setup of 
>>> platform_profile.
>>>  +        */
>>>  +        return 0;
>>>  +    }
>>>  +    asus->platform_profile_handler.profile_get = 
>>> platform_profile_get;
>>>  +    asus->platform_profile_handler.profile_set = 
>>> platform_profile_set;
>>>  +
>>>  +    set_bit(PLATFORM_PROFILE_QUIET, 
>>> asus->platform_profile_handler.choices);
>>>  +    set_bit(PLATFORM_PROFILE_BALANCED,
>>>  +        asus->platform_profile_handler.choices);
>>>  +    set_bit(PLATFORM_PROFILE_PERFORMANCE,
>>>  +        asus->platform_profile_handler.choices);
>>>  +
>>>  +    err = 
>>> platform_profile_register(&asus->platform_profile_handler);
>>>  +    if (err)
>>>  +        return err;
>>>  +
>>>  +    asus->platform_profile_support = true;
>>>  +    return 0;
>>>  +}
>>>  +
>>>   /* Backlight 
>>> ******************************************************************/
>>> 
>>>   static int read_backlight_power(struct asus_wmi *asus)
>>>  @@ -2904,6 +3008,10 @@ static int asus_wmi_add(struct 
>>> platform_device *pdev)
>>>       else
>>>           throttle_thermal_policy_set_default(asus);
>>> 
>>>  +    err = platform_profile_setup(asus);
>>>  +    if (err)
>>>  +        goto fail_platform_profile_setup;
>>>  +
>>>       err = panel_od_check_present(asus);
>>>       if (err)
>>>           goto fail_panel_od;
>>>  @@ -2993,6 +3101,7 @@ static int asus_wmi_add(struct 
>>> platform_device *pdev)
>>>       asus_wmi_sysfs_exit(asus->platform_device);
>>>   fail_sysfs:
>>>   fail_throttle_thermal_policy:
>>>  +fail_platform_profile_setup:
>>>   fail_fan_boost_mode:
>>>   fail_egpu_enable:
>>>   fail_dgpu_disable:
>>>  @@ -3017,6 +3126,9 @@ static int asus_wmi_remove(struct 
>>> platform_device *device)
>>>       asus_fan_set_auto(asus);
>>>       asus_wmi_battery_exit(asus);
>>> 
>>>  +    if (asus->platform_profile_support)
>>>  +        platform_profile_remove();
>>>  +
>>>       kfree(asus);
>>>       return 0;
>>>   }
>>>  --
>>>  2.31.1
>>> 
>> 
>> 
> 

Hi Hans,

Very helpful, thanks. I'd completely failed to notice 
platform_profile_notify in the platform_profile.h :| I've now put that 
in throttle_thermal_policy_write() just after sysfs_notify().

I will submit new version after completing other feedback.

Many Thanks,
Luke.

Re: [PATCH] asus-wmi: Add support for platform_profile

[Hans de Goede]

Hi,

On 8/13/21 9:13 AM, Luke Jones wrote:
> 
> 
> On Fri, Aug 13 2021 at 09:03:04 +0200, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 8/13/21 7:27 AM, Luke Jones wrote:
>>>  I'm unsure of how to update the existing code for fn+f5 (next thermal profile) used by laptops like the TUF series that have keyboard over i2c. I was thinking of the following:
>>>
>>>  static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
>>>  {
>>>  struct platform_profile_handler *handler = &asus->platform_profile_handler; // added
>>>  u8 new_mode = asus->throttle_thermal_policy_mode + 1;
>>>
>>>  if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
>>>   new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>>>
>>>  // asus->throttle_thermal_policy_mode = new_mode;
>>>  // return throttle_thermal_policy_write(asus);
>>>  return handler->profile_set(&asus->platform_profile_handler, new_mode); // added
>>>  }
>>>
>>>  * two lines added, two commented
>>
>> I was going to say it is best to just send a key-press event to userspace
>> (we can define a new EV_KEY_.... code for this) and then let userspace
>> handle things. But I see that this is currently already handled by the kernel,
>> so that is not really an option.
>>
>>>  I'm not able to test this though, and there are very few active people in my discord with TUF/i2c laptops to ask for testing also.
>>>
>>>  My concern here is to get the platform_profile correctly updated. Simply updating asus->throttle_thermal_policy_mode isn't going to achieve what we'll want.
>>
>> Right, there is no need to go through handler->profile_set() you have defined
>> profile_set yourself after all and it does not do anything different then the
>> old code you are replacing here.
>>
>> The trick is to call platform_profile_notify() after throttle_thermal_policy_write()
>> this will let userspace, e.g. power-profiles-daemon know that the profile has
>> been changed and it will re-read it then, resulting in a call to
>> handler->profile_get()
>>
>> So the new throttle_thermal_policy_switch_next() would look like this:
>>
>> static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
>> {
>>         u8 new_mode = asus->throttle_thermal_policy_mode + 1;
>>     int err; // new
>>
>>         if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
>>                 new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>>
>>         asus->throttle_thermal_policy_mode = new_mode;
>>
>>         err = throttle_thermal_policy_write(asus); // changed
>>     if (err == 0)                              // new
>>         platform_profile_notify();         // new
>>
>>     return err;                                // new
>> }
>>
>> As you can see the only new thing here is the
>> platform_profile_notify() call on a successful write,
>> which is such a small change that I'm not overly worried about
>> not being able to test this.
>>
>> I hope this helps.
>>
>> Regards,
>>
>> Hans

<snip>

> Hi Hans,
> 
> Very helpful, thanks. I'd completely failed to notice platform_profile_notify in the platform_profile.h :| I've now put that in throttle_thermal_policy_write() just after sysfs_notify().

That means that the notify will also happen when the setting is
changed through handler->profile_set() this is not necessarily
a bad thing since there could be multiple user-space
processes accessing the profile and then others will be
notified when one of the processes makes a change.

But ATM the other drivers which use platform_profile_notify()
only do this when the profile is changed from outside of
userspace. Specifically by a hotkey handled directly by the
embedded-controller, this in kernel turbo-key handling is
very similar to that.

So if you add the platform_profile_notify() to 
throttle_thermal_policy_write() then asus-wmi will behave
differently from the other existing implementations.

So I think we need to do a couple of things here:

1. Decided what notify behavior is the correct behavior.
Bastien, since power-profiles-daemon is the main consumer,
what behavior do you want / expect?  If we make the assumption
that there will only be 1 userspace process accessing the
profile settings (e.g. p-p-d) then the current behavior
of e.g. thinkpad_acpi to only do the notify (send POLLPRI)
when the profile is changed by a source outside userspace
seems to make sense. OTOH as I mentioned above if we
assume there might be multiple userspace processes touching
the profile (it could even be an echo from a shell) then
it makes more sense to do the notify on all changes so that
p-p-d's notion of the active profile is always correct.

Thinking more about this always doing the notify seems
like the right thing to do to me.

2. Once we have an answer to 1. we need to documented the
expected behavior in Documentation/ABI/testing/sysfs-platform_profile

3. If we go for doing a notify on any change, then we need
to update the existing drivers to do this.

Regards,

Hans

Re: [PATCH] asus-wmi: Add support for platform_profile

[Luke Jones]

On Fri, Aug 13 2021 at 09:40:25 +0200, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi,
> 
> On 8/13/21 9:13 AM, Luke Jones wrote:
>> 
>> 
>>  On Fri, Aug 13 2021 at 09:03:04 +0200, Hans de Goede 
>> <hdegoede@redhat.com> wrote:
>>>  Hi,
>>> 
>>>  On 8/13/21 7:27 AM, Luke Jones wrote:
>>>>   I'm unsure of how to update the existing code for fn+f5 (next 
>>>> thermal profile) used by laptops like the TUF series that have 
>>>> keyboard over i2c. I was thinking of the following:
>>>> 
>>>>   static int throttle_thermal_policy_switch_next(struct asus_wmi 
>>>> *asus)
>>>>   {
>>>>   struct platform_profile_handler *handler = 
>>>> &asus->platform_profile_handler; // added
>>>>   u8 new_mode = asus->throttle_thermal_policy_mode + 1;
>>>> 
>>>>   if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
>>>>    new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>>>> 
>>>>   // asus->throttle_thermal_policy_mode = new_mode;
>>>>   // return throttle_thermal_policy_write(asus);
>>>>   return handler->profile_set(&asus->platform_profile_handler, 
>>>> new_mode); // added
>>>>   }
>>>> 
>>>>   * two lines added, two commented
>>> 
>>>  I was going to say it is best to just send a key-press event to 
>>> userspace
>>>  (we can define a new EV_KEY_.... code for this) and then let 
>>> userspace
>>>  handle things. But I see that this is currently already handled by 
>>> the kernel,
>>>  so that is not really an option.
>>> 
>>>>   I'm not able to test this though, and there are very few active 
>>>> people in my discord with TUF/i2c laptops to ask for testing also.
>>>> 
>>>>   My concern here is to get the platform_profile correctly 
>>>> updated. Simply updating asus->throttle_thermal_policy_mode isn't 
>>>> going to achieve what we'll want.
>>> 
>>>  Right, there is no need to go through handler->profile_set() you 
>>> have defined
>>>  profile_set yourself after all and it does not do anything 
>>> different then the
>>>  old code you are replacing here.
>>> 
>>>  The trick is to call platform_profile_notify() after 
>>> throttle_thermal_policy_write()
>>>  this will let userspace, e.g. power-profiles-daemon know that the 
>>> profile has
>>>  been changed and it will re-read it then, resulting in a call to
>>>  handler->profile_get()
>>> 
>>>  So the new throttle_thermal_policy_switch_next() would look like 
>>> this:
>>> 
>>>  static int throttle_thermal_policy_switch_next(struct asus_wmi 
>>> *asus)
>>>  {
>>>          u8 new_mode = asus->throttle_thermal_policy_mode + 1;
>>>      int err; // new
>>> 
>>>          if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
>>>                  new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>>> 
>>>          asus->throttle_thermal_policy_mode = new_mode;
>>> 
>>>          err = throttle_thermal_policy_write(asus); // changed
>>>      if (err == 0)                              // new
>>>          platform_profile_notify();         // new
>>> 
>>>      return err;                                // new
>>>  }
>>> 
>>>  As you can see the only new thing here is the
>>>  platform_profile_notify() call on a successful write,
>>>  which is such a small change that I'm not overly worried about
>>>  not being able to test this.
>>> 
>>>  I hope this helps.
>>> 
>>>  Regards,
>>> 
>>>  Hans
> 
> <snip>
> 
>>  Hi Hans,
>> 
>>  Very helpful, thanks. I'd completely failed to notice 
>> platform_profile_notify in the platform_profile.h :| I've now put 
>> that in throttle_thermal_policy_write() just after sysfs_notify().
> 
> That means that the notify will also happen when the setting is
> changed through handler->profile_set() this is not necessarily
> a bad thing since there could be multiple user-space
> processes accessing the profile and then others will be
> notified when one of the processes makes a change.
> 
> But ATM the other drivers which use platform_profile_notify()
> only do this when the profile is changed from outside of
> userspace. Specifically by a hotkey handled directly by the
> embedded-controller, this in kernel turbo-key handling is
> very similar to that.
> 
> So if you add the platform_profile_notify() to
> throttle_thermal_policy_write() then asus-wmi will behave
> differently from the other existing implementations.
> 
> So I think we need to do a couple of things here:
> 
> 1. Decided what notify behavior is the correct behavior.
> Bastien, since power-profiles-daemon is the main consumer,
> what behavior do you want / expect?  If we make the assumption
> that there will only be 1 userspace process accessing the
> profile settings (e.g. p-p-d) then the current behavior
> of e.g. thinkpad_acpi to only do the notify (send POLLPRI)
> when the profile is changed by a source outside userspace
> seems to make sense. OTOH as I mentioned above if we
> assume there might be multiple userspace processes touching
> the profile (it could even be an echo from a shell) then
> it makes more sense to do the notify on all changes so that
> p-p-d's notion of the active profile is always correct.
> 
> Thinking more about this always doing the notify seems
> like the right thing to do to me.
> 
> 2. Once we have an answer to 1. we need to documented the
> expected behavior in Documentation/ABI/testing/sysfs-platform_profile
> 
> 3. If we go for doing a notify on any change, then we need
> to update the existing drivers to do this.
> 
> Regards,
> 
> Hans

My thinking for it was ensuring that a process that wrote to 
/sys/devices/platform/asus-nb-wmi/throttle_thermal_policy would force 
an update across all. I think perhaps I should move the notify to 
throttle_thermal_policy_store() instead which only activates when the 
path is written.

Cheers,
Luke.

Re: [PATCH] asus-wmi: Add support for platform_profile

[Hans de Goede]

Hi,

On 8/13/21 10:27 AM, Luke Jones wrote:
> 
> 
> On Fri, Aug 13 2021 at 09:40:25 +0200, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 8/13/21 9:13 AM, Luke Jones wrote:
>>>
>>>
>>>  On Fri, Aug 13 2021 at 09:03:04 +0200, Hans de Goede <hdegoede@redhat.com> wrote:
>>>>  Hi,
>>>>
>>>>  On 8/13/21 7:27 AM, Luke Jones wrote:
>>>>>   I'm unsure of how to update the existing code for fn+f5 (next thermal profile) used by laptops like the TUF series that have keyboard over i2c. I was thinking of the following:
>>>>>
>>>>>   static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
>>>>>   {
>>>>>   struct platform_profile_handler *handler = &asus->platform_profile_handler; // added
>>>>>   u8 new_mode = asus->throttle_thermal_policy_mode + 1;
>>>>>
>>>>>   if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
>>>>>    new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>>>>>
>>>>>   // asus->throttle_thermal_policy_mode = new_mode;
>>>>>   // return throttle_thermal_policy_write(asus);
>>>>>   return handler->profile_set(&asus->platform_profile_handler, new_mode); // added
>>>>>   }
>>>>>
>>>>>   * two lines added, two commented
>>>>
>>>>  I was going to say it is best to just send a key-press event to userspace
>>>>  (we can define a new EV_KEY_.... code for this) and then let userspace
>>>>  handle things. But I see that this is currently already handled by the kernel,
>>>>  so that is not really an option.
>>>>
>>>>>   I'm not able to test this though, and there are very few active people in my discord with TUF/i2c laptops to ask for testing also.
>>>>>
>>>>>   My concern here is to get the platform_profile correctly updated. Simply updating asus->throttle_thermal_policy_mode isn't going to achieve what we'll want.
>>>>
>>>>  Right, there is no need to go through handler->profile_set() you have defined
>>>>  profile_set yourself after all and it does not do anything different then the
>>>>  old code you are replacing here.
>>>>
>>>>  The trick is to call platform_profile_notify() after throttle_thermal_policy_write()
>>>>  this will let userspace, e.g. power-profiles-daemon know that the profile has
>>>>  been changed and it will re-read it then, resulting in a call to
>>>>  handler->profile_get()
>>>>
>>>>  So the new throttle_thermal_policy_switch_next() would look like this:
>>>>
>>>>  static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
>>>>  {
>>>>          u8 new_mode = asus->throttle_thermal_policy_mode + 1;
>>>>      int err; // new
>>>>
>>>>          if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
>>>>                  new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>>>>
>>>>          asus->throttle_thermal_policy_mode = new_mode;
>>>>
>>>>          err = throttle_thermal_policy_write(asus); // changed
>>>>      if (err == 0)                              // new
>>>>          platform_profile_notify();         // new
>>>>
>>>>      return err;                                // new
>>>>  }
>>>>
>>>>  As you can see the only new thing here is the
>>>>  platform_profile_notify() call on a successful write,
>>>>  which is such a small change that I'm not overly worried about
>>>>  not being able to test this.
>>>>
>>>>  I hope this helps.
>>>>
>>>>  Regards,
>>>>
>>>>  Hans
>>
>> <snip>
>>
>>>  Hi Hans,
>>>
>>>  Very helpful, thanks. I'd completely failed to notice platform_profile_notify in the platform_profile.h :| I've now put that in throttle_thermal_policy_write() just after sysfs_notify().
>>
>> That means that the notify will also happen when the setting is
>> changed through handler->profile_set() this is not necessarily
>> a bad thing since there could be multiple user-space
>> processes accessing the profile and then others will be
>> notified when one of the processes makes a change.
>>
>> But ATM the other drivers which use platform_profile_notify()
>> only do this when the profile is changed from outside of
>> userspace. Specifically by a hotkey handled directly by the
>> embedded-controller, this in kernel turbo-key handling is
>> very similar to that.
>>
>> So if you add the platform_profile_notify() to
>> throttle_thermal_policy_write() then asus-wmi will behave
>> differently from the other existing implementations.
>>
>> So I think we need to do a couple of things here:
>>
>> 1. Decided what notify behavior is the correct behavior.
>> Bastien, since power-profiles-daemon is the main consumer,
>> what behavior do you want / expect?  If we make the assumption
>> that there will only be 1 userspace process accessing the
>> profile settings (e.g. p-p-d) then the current behavior
>> of e.g. thinkpad_acpi to only do the notify (send POLLPRI)
>> when the profile is changed by a source outside userspace
>> seems to make sense. OTOH as I mentioned above if we
>> assume there might be multiple userspace processes touching
>> the profile (it could even be an echo from a shell) then
>> it makes more sense to do the notify on all changes so that
>> p-p-d's notion of the active profile is always correct.
>>
>> Thinking more about this always doing the notify seems
>> like the right thing to do to me.
>>
>> 2. Once we have an answer to 1. we need to documented the
>> expected behavior in Documentation/ABI/testing/sysfs-platform_profile
>>
>> 3. If we go for doing a notify on any change, then we need
>> to update the existing drivers to do this.
>>
>> Regards,
>>
>> Hans
> 
> My thinking for it was ensuring that a process that wrote to /sys/devices/platform/asus-nb-wmi/throttle_thermal_policy would force an update across all. I think perhaps I should move the notify to throttle_thermal_policy_store() instead which only activates when the path is written.

We definitely want the notify when the change is done by the kernel
itself through the hotkey, which would not happen when the
notify call gets moved to store.

The question which I had was if we should also notify on store-s
(writes to the sysfs file from userspace processes). The current
drivers don't do a notify on store, but that seems wrong. Also
see my other somewhat long email.

Thinking more about this I think we should actually do the notify
from the platform_profile_store() function inside
drivers/acpi/platform_profile.c, that way all drivers will
consistently do the notify after a store.

This does mean that the asus-wmi code should only call
platform_profile_notify() in the hotkey path as I suggested
in my original proposal, so that we don't call notify twice
for a single store.

###

So summarizing my current thoughts on this:

1. The asus-wmi code should only call platform_profile_notify()
in the hotkey path, to be consistent with other existing drivers.

2. We should document the POLLPRI stuff in
Documentation/ABI/testing/sysfs-platform_profile
This documentation should say that POLLPRI will be raised on
any changes, independent of those changes coming from a userspace
write; or coming from another source (typically a hotkey
triggered profile change handled either directly by the embedded-controller
or fully handled inside the kernel).

3. We should make platform_profile_store() function inside
drivers/acpi/platform_profile.c call platform_profile_notify()
on a successful store to make all the drivers consistently do
a notify after a store.

And I think it might make sense to fold 2. + 3. into a single
patch. I'll put writing that patch on my to do list.

Regards,

Hans

Re: [PATCH] asus-wmi: Add support for platform_profile

[Hans de Goede]

Hi,

On 8/13/21 9:40 AM, Hans de Goede wrote:
> Hi,
> 
> On 8/13/21 9:13 AM, Luke Jones wrote:
>>
>>
>> On Fri, Aug 13 2021 at 09:03:04 +0200, Hans de Goede <hdegoede@redhat.com> wrote:
>>> Hi,
>>>
>>> On 8/13/21 7:27 AM, Luke Jones wrote:
>>>>  I'm unsure of how to update the existing code for fn+f5 (next thermal profile) used by laptops like the TUF series that have keyboard over i2c. I was thinking of the following:
>>>>
>>>>  static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
>>>>  {
>>>>  struct platform_profile_handler *handler = &asus->platform_profile_handler; // added
>>>>  u8 new_mode = asus->throttle_thermal_policy_mode + 1;
>>>>
>>>>  if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
>>>>   new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>>>>
>>>>  // asus->throttle_thermal_policy_mode = new_mode;
>>>>  // return throttle_thermal_policy_write(asus);
>>>>  return handler->profile_set(&asus->platform_profile_handler, new_mode); // added
>>>>  }
>>>>
>>>>  * two lines added, two commented
>>>
>>> I was going to say it is best to just send a key-press event to userspace
>>> (we can define a new EV_KEY_.... code for this) and then let userspace
>>> handle things. But I see that this is currently already handled by the kernel,
>>> so that is not really an option.
>>>
>>>>  I'm not able to test this though, and there are very few active people in my discord with TUF/i2c laptops to ask for testing also.
>>>>
>>>>  My concern here is to get the platform_profile correctly updated. Simply updating asus->throttle_thermal_policy_mode isn't going to achieve what we'll want.
>>>
>>> Right, there is no need to go through handler->profile_set() you have defined
>>> profile_set yourself after all and it does not do anything different then the
>>> old code you are replacing here.
>>>
>>> The trick is to call platform_profile_notify() after throttle_thermal_policy_write()
>>> this will let userspace, e.g. power-profiles-daemon know that the profile has
>>> been changed and it will re-read it then, resulting in a call to
>>> handler->profile_get()
>>>
>>> So the new throttle_thermal_policy_switch_next() would look like this:
>>>
>>> static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
>>> {
>>>         u8 new_mode = asus->throttle_thermal_policy_mode + 1;
>>>     int err; // new
>>>
>>>         if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
>>>                 new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>>>
>>>         asus->throttle_thermal_policy_mode = new_mode;
>>>
>>>         err = throttle_thermal_policy_write(asus); // changed
>>>     if (err == 0)                              // new
>>>         platform_profile_notify();         // new
>>>
>>>     return err;                                // new
>>> }
>>>
>>> As you can see the only new thing here is the
>>> platform_profile_notify() call on a successful write,
>>> which is such a small change that I'm not overly worried about
>>> not being able to test this.
>>>
>>> I hope this helps.
>>>
>>> Regards,
>>>
>>> Hans
> 
> <snip>
> 
>> Hi Hans,
>>
>> Very helpful, thanks. I'd completely failed to notice platform_profile_notify in the platform_profile.h :| I've now put that in throttle_thermal_policy_write() just after sysfs_notify().
> 
> That means that the notify will also happen when the setting is
> changed through handler->profile_set() this is not necessarily
> a bad thing since there could be multiple user-space
> processes accessing the profile and then others will be
> notified when one of the processes makes a change.
> 
> But ATM the other drivers which use platform_profile_notify()
> only do this when the profile is changed from outside of
> userspace. Specifically by a hotkey handled directly by the
> embedded-controller, this in kernel turbo-key handling is
> very similar to that.
> 
> So if you add the platform_profile_notify() to 
> throttle_thermal_policy_write() then asus-wmi will behave
> differently from the other existing implementations.
> 
> So I think we need to do a couple of things here:
> 
> 1. Decided what notify behavior is the correct behavior.
> Bastien, since power-profiles-daemon is the main consumer,
> what behavior do you want / expect?  If we make the assumption
> that there will only be 1 userspace process accessing the
> profile settings (e.g. p-p-d) then the current behavior
> of e.g. thinkpad_acpi to only do the notify (send POLLPRI)
> when the profile is changed by a source outside userspace
> seems to make sense. OTOH as I mentioned above if we
> assume there might be multiple userspace processes touching
> the profile (it could even be an echo from a shell) then
> it makes more sense to do the notify on all changes so that
> p-p-d's notion of the active profile is always correct.
> 
> Thinking more about this always doing the notify seems
> like the right thing to do to me.

Ok, so I was just thinking that this does not sound right to me,
since I did try echo-ing values to the profile while having the
GNOME control-panel open and I was pretty sure that it did
then actually update. So I just checked again and it does.

The thinkpad_acpi driver does not call platform_profile_notify()
on a write. But it does when receiving an event from the EC
that the profile has changed, which I guess is also fired on
a write from userspace.

But that notify pm an event is only done if the profile
read from the EC on the event is different then the last written
value. So this should not work, yet somehow it does work...

I even added a printk to thinkpad_acpi.c and it is indeed
NOT calling platform_profile_notify() when I echo a new
value to /sys/firmware/acpi/platform_profile.

Ah I just checked the p-p-d code and it is using GFileMonitor
rather then watching for POLLPRI  / G_IO_PRI. I guess that
GFileMonitor is using inotify or some such and that catches
writes by other userspace processes, as well as the POLLPRI
notifies it seems, interesting.

Note that inotify does not really work on sysfs files, since
they are not real files and their contents is generated by the
kernel on the fly when read , so it can change at any time.
But I guess it does catch writes by other processes so it works
in this case.

This does advocate for always doing the notify since normally
userspace processes who want to check for sysfs changes should
do so by doing a (e)poll checking for POLLPRI  / G_IO_PRI and
in that scenario p-p-d would currently not see changes done
through echo-ing a new value to /sys/firmware/acpi/platform_profile.

So long story short, Luke I believe that your decision to call
platform_profile_notify() on every write is correct.

###

This does mean that we still need to do:

2. Once we have an answer to 1. we need to documented the
expected behavior in Documentation/ABI/testing/sysfs-platform_profile

Does anyone feel up to writing a patch for this ?

3. If we go for doing a notify on any change, then we need
to update the existing drivers to do this.

I guess I should add this to my to-do list.

Regards,

Hans

Re: [PATCH] asus-wmi: Add support for platform_profile

[Hans de Goede]

p.s.


<snip>

Note that inotify (and thus GFileMonitor) does not really work on sysfs files.

Interesting email thread about this here:

https://linux-fsdevel.vger.kernel.narkive.com/u0qmXPFK/inotify-sysfs

Re: [PATCH] asus-wmi: Add support for platform_profile

[Luke Jones]

I'll try to follow along here...

On Fri, Aug 13 2021 at 10:44:07 +0200, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi,
> 
> On 8/13/21 9:40 AM, Hans de Goede wrote:
>>  Hi,
>> 
>>  On 8/13/21 9:13 AM, Luke Jones wrote:
>>> 
>>> 
>>>  On Fri, Aug 13 2021 at 09:03:04 +0200, Hans de Goede 
>>> <hdegoede@redhat.com> wrote:
>>>>  Hi,
>>>> 
>>>>  On 8/13/21 7:27 AM, Luke Jones wrote:
>>>>>   I'm unsure of how to update the existing code for fn+f5 (next 
>>>>> thermal profile) used by laptops like the TUF series that have 
>>>>> keyboard over i2c. I was thinking of the following:
>>>>> 
>>>>>   static int throttle_thermal_policy_switch_next(struct asus_wmi 
>>>>> *asus)
>>>>>   {
>>>>>   struct platform_profile_handler *handler = 
>>>>> &asus->platform_profile_handler; // added
>>>>>   u8 new_mode = asus->throttle_thermal_policy_mode + 1;
>>>>> 
>>>>>   if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
>>>>>    new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>>>>> 
>>>>>   // asus->throttle_thermal_policy_mode = new_mode;
>>>>>   // return throttle_thermal_policy_write(asus);
>>>>>   return handler->profile_set(&asus->platform_profile_handler, 
>>>>> new_mode); // added
>>>>>   }
>>>>> 
>>>>>   * two lines added, two commented
>>>> 
>>>>  I was going to say it is best to just send a key-press event to 
>>>> userspace
>>>>  (we can define a new EV_KEY_.... code for this) and then let 
>>>> userspace
>>>>  handle things. But I see that this is currently already handled 
>>>> by the kernel,
>>>>  so that is not really an option.
>>>> 
>>>>>   I'm not able to test this though, and there are very few active 
>>>>> people in my discord with TUF/i2c laptops to ask for testing also.
>>>>> 
>>>>>   My concern here is to get the platform_profile correctly 
>>>>> updated. Simply updating asus->throttle_thermal_policy_mode isn't 
>>>>> going to achieve what we'll want.
>>>> 
>>>>  Right, there is no need to go through handler->profile_set() you 
>>>> have defined
>>>>  profile_set yourself after all and it does not do anything 
>>>> different then the
>>>>  old code you are replacing here.
>>>> 
>>>>  The trick is to call platform_profile_notify() after 
>>>> throttle_thermal_policy_write()
>>>>  this will let userspace, e.g. power-profiles-daemon know that the 
>>>> profile has
>>>>  been changed and it will re-read it then, resulting in a call to
>>>>  handler->profile_get()
>>>> 
>>>>  So the new throttle_thermal_policy_switch_next() would look like 
>>>> this:
>>>> 
>>>>  static int throttle_thermal_policy_switch_next(struct asus_wmi 
>>>> *asus)
>>>>  {
>>>>          u8 new_mode = asus->throttle_thermal_policy_mode + 1;
>>>>      int err; // new
>>>> 
>>>>          if (new_mode > ASUS_THROTTLE_THERMAL_POLICY_SILENT)
>>>>                  new_mode = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>>>> 
>>>>          asus->throttle_thermal_policy_mode = new_mode;
>>>> 
>>>>          err = throttle_thermal_policy_write(asus); // changed
>>>>      if (err == 0)                              // new
>>>>          platform_profile_notify();         // new
>>>> 
>>>>      return err;                                // new
>>>>  }
>>>> 
>>>>  As you can see the only new thing here is the
>>>>  platform_profile_notify() call on a successful write,
>>>>  which is such a small change that I'm not overly worried about
>>>>  not being able to test this.
>>>> 
>>>>  I hope this helps.
>>>> 
>>>>  Regards,
>>>> 
>>>>  Hans
>> 
>>  <snip>
>> 
>>>  Hi Hans,
>>> 
>>>  Very helpful, thanks. I'd completely failed to notice 
>>> platform_profile_notify in the platform_profile.h :| I've now put 
>>> that in throttle_thermal_policy_write() just after sysfs_notify().
>> 
>>  That means that the notify will also happen when the setting is
>>  changed through handler->profile_set() this is not necessarily
>>  a bad thing since there could be multiple user-space
>>  processes accessing the profile and then others will be
>>  notified when one of the processes makes a change.
>> 
>>  But ATM the other drivers which use platform_profile_notify()
>>  only do this when the profile is changed from outside of
>>  userspace. Specifically by a hotkey handled directly by the
>>  embedded-controller, this in kernel turbo-key handling is
>>  very similar to that.
>> 
>>  So if you add the platform_profile_notify() to
>>  throttle_thermal_policy_write() then asus-wmi will behave
>>  differently from the other existing implementations.
>> 
>>  So I think we need to do a couple of things here:
>> 
>>  1. Decided what notify behavior is the correct behavior.
>>  Bastien, since power-profiles-daemon is the main consumer,
>>  what behavior do you want / expect?  If we make the assumption
>>  that there will only be 1 userspace process accessing the
>>  profile settings (e.g. p-p-d) then the current behavior
>>  of e.g. thinkpad_acpi to only do the notify (send POLLPRI)
>>  when the profile is changed by a source outside userspace
>>  seems to make sense. OTOH as I mentioned above if we
>>  assume there might be multiple userspace processes touching
>>  the profile (it could even be an echo from a shell) then
>>  it makes more sense to do the notify on all changes so that
>>  p-p-d's notion of the active profile is always correct.
>> 
>>  Thinking more about this always doing the notify seems
>>  like the right thing to do to me.
> 
> Ok, so I was just thinking that this does not sound right to me,
> since I did try echo-ing values to the profile while having the
> GNOME control-panel open and I was pretty sure that it did
> then actually update. So I just checked again and it does.
> 
> The thinkpad_acpi driver does not call platform_profile_notify()
> on a write. But it does when receiving an event from the EC
> that the profile has changed, which I guess is also fired on
> a write from userspace.
> 
> But that notify pm an event is only done if the profile
> read from the EC on the event is different then the last written
> value. So this should not work, yet somehow it does work...
> 
> I even added a printk to thinkpad_acpi.c and it is indeed
> NOT calling platform_profile_notify() when I echo a new
> value to /sys/firmware/acpi/platform_profile.

Okay I see. Yes I tested this before submission. The issue here for the 
ASUS laptops is that 
/sys/bus/platform/devices/asus-nb-wmi/throttle_thermal_policy is still 
accessible and writeable. If this is written to then the 
platform_profile becomes out of sync with it. So the option here is:

1. notify platform_profile, or
2. remove the sysfs for throttle_thermal_policy

Thinking about it I would prefer option 2 so we do not end up with two 
paths for duplicate functionality. As far as I know asusctl is the only 
(very) widely distributed and used tool for these laptops that uses the 
existing throttle_thermal_policy sysfs path, so it is very easy to sync 
asusctl with the changes made here.

> 
> Ah I just checked the p-p-d code and it is using GFileMonitor
> rather then watching for POLLPRI  / G_IO_PRI. I guess that
> GFileMonitor is using inotify or some such and that catches
> writes by other userspace processes, as well as the POLLPRI
> notifies it seems, interesting.
> 
> Note that inotify does not really work on sysfs files, since
> they are not real files and their contents is generated by the
> kernel on the fly when read , so it can change at any time.
> But I guess it does catch writes by other processes so it works
> in this case.
> 
> This does advocate for always doing the notify since normally
> userspace processes who want to check for sysfs changes should
> do so by doing a (e)poll checking for POLLPRI  / G_IO_PRI and
> in that scenario p-p-d would currently not see changes done
> through echo-ing a new value to /sys/firmware/acpi/platform_profile.
> 
> So long story short, Luke I believe that your decision to call
> platform_profile_notify() on every write is correct.

Just to be super clear:
The notify is on write to 
/sys/bus/platform/devices/asus-nb-wmi/throttle_thermal_policy as 
described above.
Not to /sys/firmware/acpi/platform_profile

Cheers,
Luke.

> 
> ###
> 
> This does mean that we still need to do:
> 
> 2. Once we have an answer to 1. we need to documented the
> expected behavior in Documentation/ABI/testing/sysfs-platform_profile
> 
> Does anyone feel up to writing a patch for this ?
> 
> 3. If we go for doing a notify on any change, then we need
> to update the existing drivers to do this.
> 
> I guess I should add this to my to-do list.
> 
> Regards,
> 
> Hans
> 

Re: [PATCH] asus-wmi: Add support for platform_profile

[Hans de Goede]

Hi,

On 8/13/21 11:42 AM, Luke Jones wrote:
> I'll try to follow along here...
> 
> On Fri, Aug 13 2021 at 10:44:07 +0200, Hans de Goede <hdegoede@redhat.com> wrote:

<snip>

>>>  That means that the notify will also happen when the setting is
>>>  changed through handler->profile_set() this is not necessarily
>>>  a bad thing since there could be multiple user-space
>>>  processes accessing the profile and then others will be
>>>  notified when one of the processes makes a change.
>>>
>>>  But ATM the other drivers which use platform_profile_notify()
>>>  only do this when the profile is changed from outside of
>>>  userspace. Specifically by a hotkey handled directly by the
>>>  embedded-controller, this in kernel turbo-key handling is
>>>  very similar to that.
>>>
>>>  So if you add the platform_profile_notify() to
>>>  throttle_thermal_policy_write() then asus-wmi will behave
>>>  differently from the other existing implementations.
>>>
>>>  So I think we need to do a couple of things here:
>>>
>>>  1. Decided what notify behavior is the correct behavior.
>>>  Bastien, since power-profiles-daemon is the main consumer,
>>>  what behavior do you want / expect?  If we make the assumption
>>>  that there will only be 1 userspace process accessing the
>>>  profile settings (e.g. p-p-d) then the current behavior
>>>  of e.g. thinkpad_acpi to only do the notify (send POLLPRI)
>>>  when the profile is changed by a source outside userspace
>>>  seems to make sense. OTOH as I mentioned above if we
>>>  assume there might be multiple userspace processes touching
>>>  the profile (it could even be an echo from a shell) then
>>>  it makes more sense to do the notify on all changes so that
>>>  p-p-d's notion of the active profile is always correct.
>>>
>>>  Thinking more about this always doing the notify seems
>>>  like the right thing to do to me.
>>
>> Ok, so I was just thinking that this does not sound right to me,
>> since I did try echo-ing values to the profile while having the
>> GNOME control-panel open and I was pretty sure that it did
>> then actually update. So I just checked again and it does.
>>
>> The thinkpad_acpi driver does not call platform_profile_notify()
>> on a write. But it does when receiving an event from the EC
>> that the profile has changed, which I guess is also fired on
>> a write from userspace.
>>
>> But that notify pm an event is only done if the profile
>> read from the EC on the event is different then the last written
>> value. So this should not work, yet somehow it does work...
>>
>> I even added a printk to thinkpad_acpi.c and it is indeed
>> NOT calling platform_profile_notify() when I echo a new
>> value to /sys/firmware/acpi/platform_profile.
> 
> Okay I see. Yes I tested this before submission. The issue here for the ASUS laptops is that /sys/bus/platform/devices/asus-nb-wmi/throttle_thermal_policy is still accessible and writeable. If this is written to then the platform_profile becomes out of sync with it. So the option here is:
> 
> 1. notify platform_profile, or
> 2. remove the sysfs for throttle_thermal_policy
> 
> Thinking about it I would prefer option 2 so we do not end up with two paths for duplicate functionality. As far as I know asusctl is the only (very) widely distributed and used tool for these laptops that uses the existing throttle_thermal_policy sysfs path, so it is very easy to sync asusctl with the changes made here.

2. is something which we may do in a year or so, we have a strict
no breaking userspace policy in the kernel; so people should be
able to drop in a new kernel without updating asusctl and things
should keep working. Which means that we are stuck with the
/sys/bus/platform/devices/asus-nb-wmi/throttle_thermal_policy
file for at least a year.

So we need to 1, call platform_profily_notify on writes
to /sys/bus/platform/devices/asus-nb-wmi/throttle_thermal_policy
and on changes because of the hotkey, while not calling it
from the profile_set callback.

>> Ah I just checked the p-p-d code and it is using GFileMonitor
>> rather then watching for POLLPRI  / G_IO_PRI. I guess that
>> GFileMonitor is using inotify or some such and that catches
>> writes by other userspace processes, as well as the POLLPRI
>> notifies it seems, interesting.
>>
>> Note that inotify does not really work on sysfs files, since
>> they are not real files and their contents is generated by the
>> kernel on the fly when read , so it can change at any time.
>> But I guess it does catch writes by other processes so it works
>> in this case.
>>
>> This does advocate for always doing the notify since normally
>> userspace processes who want to check for sysfs changes should
>> do so by doing a (e)poll checking for POLLPRI  / G_IO_PRI and
>> in that scenario p-p-d would currently not see changes done
>> through echo-ing a new value to /sys/firmware/acpi/platform_profile.
>>
>> So long story short, Luke I believe that your decision to call
>> platform_profile_notify() on every write is correct.
> 
> Just to be super clear:
> The notify is on write to /sys/bus/platform/devices/asus-nb-wmi/throttle_thermal_policy as described above.
> Not to /sys/firmware/acpi/platform_profile

Ah I see, yes I agree platform_profile_notify() should be called
from the store handler for /sys/bus/platform/devices/asus-nb-wmi/throttle_thermal_policy.

Basically it should be called for _all_ changes not done through
the profile_set callback.

Regards,

Hans

Re: [PATCH] asus-wmi: Add support for platform_profile

[Hans de Goede]

Hi Luke,

Thank you for this patch, it is great to see more and more laptop
models getting support for the standard platform_profile userspace API.

On 8/13/21 4:42 AM, Luke D. Jones wrote:
> Add initial support for platform_profile where the support is
> based on availability of ASUS_THROTTLE_THERMAL_POLICY.
> 
> Signed-off-by: Luke D. Jones <luke@ljones.dev>
> ---
>  drivers/platform/x86/asus-wmi.c | 112 ++++++++++++++++++++++++++++++++
>  1 file changed, 112 insertions(+)
> 
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 90a6a0d00deb..62260043c15c 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -26,6 +26,7 @@
>  #include <linux/rfkill.h>
>  #include <linux/pci.h>
>  #include <linux/pci_hotplug.h>
> +#include <linux/platform_profile.h>
>  #include <linux/power_supply.h>
>  #include <linux/hwmon.h>
>  #include <linux/hwmon-sysfs.h>
> @@ -219,6 +220,9 @@ struct asus_wmi {
>  	bool throttle_thermal_policy_available;
>  	u8 throttle_thermal_policy_mode;
>  
> +	struct platform_profile_handler platform_profile_handler;
> +	bool platform_profile_support;
> +
>  	// The RSOC controls the maximum charging percentage.
>  	bool battery_rsoc_available;
>  
> @@ -2144,6 +2148,106 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
>  // Throttle thermal policy: 0 - default, 1 - overboost, 2 - silent
>  static DEVICE_ATTR_RW(throttle_thermal_policy);
>  
> +/* Platform profile ***********************************************************/
> +static int platform_profile_get(struct platform_profile_handler *pprof,
> +				enum platform_profile_option *profile)
> +{
> +	struct asus_wmi *asus;
> +	int tp;
> +
> +	asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
> +
> +	/* All possible toggles like throttle_thermal_policy here */
> +	if (asus->throttle_thermal_policy_available) {
> +		tp = asus->throttle_thermal_policy_mode;
> +	} else {
> +		return -1;
> +	}
> +
> +	if (tp < 0)
> +		return tp;
> +
> +	switch (tp) {
> +	case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
> +		*profile = PLATFORM_PROFILE_BALANCED;
> +		break;
> +	case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
> +		*profile = PLATFORM_PROFILE_PERFORMANCE;
> +		break;
> +	case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
> +		*profile = PLATFORM_PROFILE_QUIET;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
> +static int platform_profile_set(struct platform_profile_handler *pprof,
> +				enum platform_profile_option profile)
> +{
> +	struct asus_wmi *asus;
> +	int tp;
> +
> +	asus = container_of(pprof, struct asus_wmi, platform_profile_handler);
> +
> +	/* All possible toggles like throttle_thermal_policy here */
> +	if (!asus->throttle_thermal_policy_available) {
> +		return -1;
> +	}
> +
> +	switch (profile) {
> +	case PLATFORM_PROFILE_PERFORMANCE:
> +		tp = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
> +		break;
> +	case PLATFORM_PROFILE_BALANCED:
> +		tp = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
> +		break;
> +	case PLATFORM_PROFILE_QUIET:
> +		tp = ASUS_THROTTLE_THERMAL_POLICY_SILENT;
> +		break;
> +	default:
> +		return -EOPNOTSUPP;
> +	}
> +
> +	if (asus->throttle_thermal_policy_available) {

You already check for asus->throttle_thermal_policy_available a couple
of lines higher in this function and you return when it is not
available ...

I guess this was intended to also have a second if () for
fan-boost mode ?

Since you are not adding support for fan-boost mode at this time,
it would be best (IMHO) to drop this second if for now and
just take this path unconditionally.


> +		asus->throttle_thermal_policy_mode = tp;
> +		return throttle_thermal_policy_write(asus);
> +	}
> +	return 0;
> +}
> +
> +static int platform_profile_setup(struct asus_wmi *asus)
> +{
> +	int err;
> +
> +	if (asus->throttle_thermal_policy_available) {
> +		pr_info("Using throttle_thermal_policy for platform_profile support\n");
> +	} else {
> +		/*
> +		 * Not an error if a component platform_profile relies on is unavailable
> +		 * so early return, skipping the setup of platform_profile.
> +		*/
> +		return 0;
> +	}
> +	asus->platform_profile_handler.profile_get = platform_profile_get;
> +	asus->platform_profile_handler.profile_set = platform_profile_set;
> +
> +	set_bit(PLATFORM_PROFILE_QUIET, asus->platform_profile_handler.choices);
> +	set_bit(PLATFORM_PROFILE_BALANCED,
> +		asus->platform_profile_handler.choices);
> +	set_bit(PLATFORM_PROFILE_PERFORMANCE,
> +		asus->platform_profile_handler.choices);
> +
> +	err = platform_profile_register(&asus->platform_profile_handler);
> +	if (err)
> +		return err;
> +
> +	asus->platform_profile_support = true;
> +	return 0;
> +}
> +
>  /* Backlight ******************************************************************/
>  
>  static int read_backlight_power(struct asus_wmi *asus)
> @@ -2904,6 +3008,10 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	else
>  		throttle_thermal_policy_set_default(asus);
>  
> +	err = platform_profile_setup(asus);
> +	if (err)
> +		goto fail_platform_profile_setup;
> +
>  	err = panel_od_check_present(asus);
>  	if (err)
>  		goto fail_panel_od;
> @@ -2993,6 +3101,7 @@ static int asus_wmi_add(struct platform_device *pdev)
>  	asus_wmi_sysfs_exit(asus->platform_device);
>  fail_sysfs:
>  fail_throttle_thermal_policy:
> +fail_platform_profile_setup:
>  fail_fan_boost_mode:

I think you need to add this:

	if (asus->platform_profile_support)
		platform_profile_remove();

here so that if later fail-exit paths are taken the platform_profile
support gets unregistered again.

>  fail_egpu_enable:
>  fail_dgpu_disable:
> @@ -3017,6 +3126,9 @@ static int asus_wmi_remove(struct platform_device *device)
>  	asus_fan_set_auto(asus);
>  	asus_wmi_battery_exit(asus);
>  
> +	if (asus->platform_profile_support)
> +		platform_profile_remove();
> +
>  	kfree(asus);
>  	return 0;
>  }
> 

Regards,

Hans

Re: [PATCH] asus-wmi: Add support for platform_profile

[Luke Jones]

On Fri, Aug 13 2021 at 08:47:56 +0200, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi Luke,
> 
> Thank you for this patch, it is great to see more and more laptop
> models getting support for the standard platform_profile userspace 
> API.
> 
> On 8/13/21 4:42 AM, Luke D. Jones wrote:
>>  Add initial support for platform_profile where the support is
>>  based on availability of ASUS_THROTTLE_THERMAL_POLICY.
>> 
>>  Signed-off-by: Luke D. Jones <luke@ljones.dev>
>>  ---
>>   drivers/platform/x86/asus-wmi.c | 112 
>> ++++++++++++++++++++++++++++++++
>>   1 file changed, 112 insertions(+)
>> 
>>  diff --git a/drivers/platform/x86/asus-wmi.c 
>> b/drivers/platform/x86/asus-wmi.c
>>  index 90a6a0d00deb..62260043c15c 100644
>>  --- a/drivers/platform/x86/asus-wmi.c
>>  +++ b/drivers/platform/x86/asus-wmi.c
>>  @@ -26,6 +26,7 @@
>>   #include <linux/rfkill.h>
>>   #include <linux/pci.h>
>>   #include <linux/pci_hotplug.h>
>>  +#include <linux/platform_profile.h>
>>   #include <linux/power_supply.h>
>>   #include <linux/hwmon.h>
>>   #include <linux/hwmon-sysfs.h>
>>  @@ -219,6 +220,9 @@ struct asus_wmi {
>>   	bool throttle_thermal_policy_available;
>>   	u8 throttle_thermal_policy_mode;
>> 
>>  +	struct platform_profile_handler platform_profile_handler;
>>  +	bool platform_profile_support;
>>  +
>>   	// The RSOC controls the maximum charging percentage.
>>   	bool battery_rsoc_available;
>> 
>>  @@ -2144,6 +2148,106 @@ static ssize_t 
>> throttle_thermal_policy_store(struct device *dev,
>>   // Throttle thermal policy: 0 - default, 1 - overboost, 2 - silent
>>   static DEVICE_ATTR_RW(throttle_thermal_policy);
>> 
>>  +/* Platform profile 
>> ***********************************************************/
>>  +static int platform_profile_get(struct platform_profile_handler 
>> *pprof,
>>  +				enum platform_profile_option *profile)
>>  +{
>>  +	struct asus_wmi *asus;
>>  +	int tp;
>>  +
>>  +	asus = container_of(pprof, struct asus_wmi, 
>> platform_profile_handler);
>>  +
>>  +	/* All possible toggles like throttle_thermal_policy here */
>>  +	if (asus->throttle_thermal_policy_available) {
>>  +		tp = asus->throttle_thermal_policy_mode;
>>  +	} else {
>>  +		return -1;
>>  +	}
>>  +
>>  +	if (tp < 0)
>>  +		return tp;
>>  +
>>  +	switch (tp) {
>>  +	case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
>>  +		*profile = PLATFORM_PROFILE_BALANCED;
>>  +		break;
>>  +	case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
>>  +		*profile = PLATFORM_PROFILE_PERFORMANCE;
>>  +		break;
>>  +	case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
>>  +		*profile = PLATFORM_PROFILE_QUIET;
>>  +		break;
>>  +	default:
>>  +		return -EINVAL;
>>  +	}
>>  +
>>  +	return 0;
>>  +}
>>  +
>>  +static int platform_profile_set(struct platform_profile_handler 
>> *pprof,
>>  +				enum platform_profile_option profile)
>>  +{
>>  +	struct asus_wmi *asus;
>>  +	int tp;
>>  +
>>  +	asus = container_of(pprof, struct asus_wmi, 
>> platform_profile_handler);
>>  +
>>  +	/* All possible toggles like throttle_thermal_policy here */
>>  +	if (!asus->throttle_thermal_policy_available) {
>>  +		return -1;
>>  +	}
>>  +
>>  +	switch (profile) {
>>  +	case PLATFORM_PROFILE_PERFORMANCE:
>>  +		tp = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
>>  +		break;
>>  +	case PLATFORM_PROFILE_BALANCED:
>>  +		tp = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>>  +		break;
>>  +	case PLATFORM_PROFILE_QUIET:
>>  +		tp = ASUS_THROTTLE_THERMAL_POLICY_SILENT;
>>  +		break;
>>  +	default:
>>  +		return -EOPNOTSUPP;
>>  +	}
>>  +
>>  +	if (asus->throttle_thermal_policy_available) {
> 
> You already check for asus->throttle_thermal_policy_available a couple
> of lines higher in this function and you return when it is not
> available ...
> 
> I guess this was intended to also have a second if () for
> fan-boost mode ?

Yes, sorry I missed that. I'd initially looked at adding fan-boost also 
but as it is *fans only* I didn't see the value in adding it. I've also 
not seen any use of this in the wild bar one laptop which was likely to 
be retired many months ago.

> 
> Since you are not adding support for fan-boost mode at this time,
> it would be best (IMHO) to drop this second if for now and
> just take this path unconditionally.
> 

Adjusted now.

> 
>>  +		asus->throttle_thermal_policy_mode = tp;
>>  +		return throttle_thermal_policy_write(asus);
>>  +	}
>>  +	return 0;
>>  +}
>>  +
>>  +static int platform_profile_setup(struct asus_wmi *asus)
>>  +{
>>  +	int err;
>>  +
>>  +	if (asus->throttle_thermal_policy_available) {
>>  +		pr_info("Using throttle_thermal_policy for platform_profile 
>> support\n");
>>  +	} else {
>>  +		/*
>>  +		 * Not an error if a component platform_profile relies on is 
>> unavailable
>>  +		 * so early return, skipping the setup of platform_profile.
>>  +		*/
>>  +		return 0;
>>  +	}
>>  +	asus->platform_profile_handler.profile_get = platform_profile_get;
>>  +	asus->platform_profile_handler.profile_set = platform_profile_set;
>>  +
>>  +	set_bit(PLATFORM_PROFILE_QUIET, 
>> asus->platform_profile_handler.choices);
>>  +	set_bit(PLATFORM_PROFILE_BALANCED,
>>  +		asus->platform_profile_handler.choices);
>>  +	set_bit(PLATFORM_PROFILE_PERFORMANCE,
>>  +		asus->platform_profile_handler.choices);
>>  +
>>  +	err = platform_profile_register(&asus->platform_profile_handler);
>>  +	if (err)
>>  +		return err;
>>  +
>>  +	asus->platform_profile_support = true;
>>  +	return 0;
>>  +}
>>  +
>>   /* Backlight 
>> ******************************************************************/
>> 
>>   static int read_backlight_power(struct asus_wmi *asus)
>>  @@ -2904,6 +3008,10 @@ static int asus_wmi_add(struct 
>> platform_device *pdev)
>>   	else
>>   		throttle_thermal_policy_set_default(asus);
>> 
>>  +	err = platform_profile_setup(asus);
>>  +	if (err)
>>  +		goto fail_platform_profile_setup;
>>  +
>>   	err = panel_od_check_present(asus);
>>   	if (err)
>>   		goto fail_panel_od;
>>  @@ -2993,6 +3101,7 @@ static int asus_wmi_add(struct 
>> platform_device *pdev)
>>   	asus_wmi_sysfs_exit(asus->platform_device);
>>   fail_sysfs:
>>   fail_throttle_thermal_policy:
>>  +fail_platform_profile_setup:
>>   fail_fan_boost_mode:
> 
> I think you need to add this:
> 
> 	if (asus->platform_profile_support)
> 		platform_profile_remove();
> 
> here so that if later fail-exit paths are taken the platform_profile
> support gets unregistered again.

Good catch, thanks! :)

Next version will come after some testing.

> 
>>   fail_egpu_enable:
>>   fail_dgpu_disable:
>>  @@ -3017,6 +3126,9 @@ static int asus_wmi_remove(struct 
>> platform_device *device)
>>   	asus_fan_set_auto(asus);
>>   	asus_wmi_battery_exit(asus);
>> 
>>  +	if (asus->platform_profile_support)
>>  +		platform_profile_remove();
>>  +
>>   	kfree(asus);
>>   	return 0;
>>   }
>> 
> 
> Regards,
> 
> Hans
>