[PATCH v3 0/1] asus-wmi: add platform_profile support

[PATCH v3 0/1] asus-wmi: add platform_profile support

[Luke D. Jones]

Changelog:
- V2
  + Correctly unregister from platform_profile if
    throttle_thermal_policy fails
  + Do platform_profile_notify() in both throttle_thermal_policy_store()
    and in throttle_thermal_policy_switch_next()
  + Remove unnecessary prep for possible fan-boost modes as this
    doesn't match expected platform_profile behaviour
- V3
  + Add missing declaration for err in
    throttle_thermal_policy_switch_next

Luke D. Jones (1):
  asus-wmi: Add support for platform_profile

 drivers/platform/x86/asus-wmi.c | 139 +++++++++++++++++++++++++++++++-
 1 file changed, 135 insertions(+), 4 deletions(-)

-- 
2.31.1

[PATCH v3 1/1] 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.

Because throttle_thermal_policy is used by platform_profile and is
writeable separately to platform_profile any userspace changes to
throttle_thermal_policy need to notify platform_profile.

In future throttle_thermal_policy sysfs should be removed so that
only one method controls the laptop power profile.

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

diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
index 90a6a0d00deb..909fc2663008 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;
 
@@ -2103,12 +2107,23 @@ static int throttle_thermal_policy_set_default(struct asus_wmi *asus)
 static int throttle_thermal_policy_switch_next(struct asus_wmi *asus)
 {
 	u8 new_mode = asus->throttle_thermal_policy_mode + 1;
+	int err;
 
 	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);
+	err = throttle_thermal_policy_write(asus);
+	if (err)
+		return err;
+
+	/*
+	 * Ensure that platform_profile updates userspace with the change to ensure
+	 * that platform_profile and throttle_thermal_policy_mode are in sync
+	 */
+	platform_profile_notify();
+
+	return 0;
 }
 
 static ssize_t throttle_thermal_policy_show(struct device *dev,
@@ -2124,9 +2139,10 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
 				    struct device_attribute *attr,
 				    const char *buf, size_t count)
 {
-	int result;
-	u8 new_mode;
 	struct asus_wmi *asus = dev_get_drvdata(dev);
+	u8 new_mode;
+	int result;
+	int err;
 
 	result = kstrtou8(buf, 10, &new_mode);
 	if (result < 0)
@@ -2136,7 +2152,15 @@ static ssize_t throttle_thermal_policy_store(struct device *dev,
 		return -EINVAL;
 
 	asus->throttle_thermal_policy_mode = new_mode;
-	throttle_thermal_policy_write(asus);
+	err = throttle_thermal_policy_write(asus);
+	if (err)
+		return err;
+
+	/*
+	 * Ensure that platform_profile updates userspace with the change to ensure
+	 * that platform_profile and throttle_thermal_policy_mode are in sync
+	 */
+	platform_profile_notify();
 
 	return count;
 }
@@ -2144,6 +2168,103 @@ 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;
+	}
+
+	asus->throttle_thermal_policy_mode = tp;
+	return throttle_thermal_policy_write(asus);
+}
+
+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 +3025,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 +3118,9 @@ 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:
+	if (asus->platform_profile_support)
+		platform_profile_remove();
 fail_fan_boost_mode:
 fail_egpu_enable:
 fail_dgpu_disable:
@@ -3017,6 +3145,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 v3 1/1] asus-wmi: Add support for platform_profile

[Andy Shevchenko]

On Sat, Aug 14, 2021 at 7:33 AM 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.
>
> Because throttle_thermal_policy is used by platform_profile and is
> writeable separately to platform_profile any userspace changes to
> throttle_thermal_policy need to notify platform_profile.
>
> In future throttle_thermal_policy sysfs should be removed so that
> only one method controls the laptop power profile.

Some comments below.

...

> +       /*
> +        * Ensure that platform_profile updates userspace with the change to ensure
> +        * that platform_profile and throttle_thermal_policy_mode are in sync

Missed period here and in other multi-line comments.

> +        */

...

> +       /* 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;

This will be better in a form

    if (!..._available)
        return -ENODATA; // what -1 means?

    tp = ...;
    if (tp < 0)
        return tp;

...

> +       /* All possible toggles like throttle_thermal_policy here */
> +       if (!asus->throttle_thermal_policy_available) {
> +               return -1;

See above.

> +       }

...

> +       if (asus->throttle_thermal_policy_available) {
> +               pr_info("Using throttle_thermal_policy for platform_profile support\n");

Why pr_*()?

> +       } else {
> +               /*
> +                * Not an error if a component platform_profile relies on is unavailable
> +                * so early return, skipping the setup of platform_profile.
> +               */
> +               return 0;

Do it other way around,

/*
 * Comment
 */
if (!...)
  return 0;

> +       }


-- 
With Best Regards,
Andy Shevchenko

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

[Luke Jones]

Hi Andy, thanks for the feedback. All is addressed, and some inline 
comment/reply. New version incoming pending pr_info() vs dev_info()

On Sat, Aug 14 2021 at 12:40:39 +0300, Andy Shevchenko 
<andy.shevchenko@gmail.com> wrote:
> On Sat, Aug 14, 2021 at 7:33 AM 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.
>> 
>>  Because throttle_thermal_policy is used by platform_profile and is
>>  writeable separately to platform_profile any userspace changes to
>>  throttle_thermal_policy need to notify platform_profile.
>> 
>>  In future throttle_thermal_policy sysfs should be removed so that
>>  only one method controls the laptop power profile.
> 
> Some comments below.
> 
> ...
> 
>>  +       /*
>>  +        * Ensure that platform_profile updates userspace with the 
>> change to ensure
>>  +        * that platform_profile and throttle_thermal_policy_mode 
>> are in sync
> 
> Missed period here and in other multi-line comments.
> 
>>  +        */
> 
> ...
> 
>>  +       /* 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;
> 
> This will be better in a form
> 
>     if (!..._available)
>         return -ENODATA; // what -1 means?
> 

I wasn't sure what the best return was here. On your suggestion I've 
changed to ENODATA

>     tp = ...;
>     if (tp < 0)
>         return tp;
> 
> ...
> 
>>  +       /* All possible toggles like throttle_thermal_policy here */
>>  +       if (!asus->throttle_thermal_policy_available) {
>>  +               return -1;
> 
> See above.
> 
>>  +       }
> 
> ...
> 
>>  +       if (asus->throttle_thermal_policy_available) {
>>  +               pr_info("Using throttle_thermal_policy for 
>> platform_profile support\n");
> 
> Why pr_*()?

That seemed to be the convention? I see there is also dev_info(), so 
I've switched to that as it seems more appropriate.

> 
>>  +       } else {
>>  +               /*
>>  +                * Not an error if a component platform_profile 
>> relies on is unavailable
>>  +                * so early return, skipping the setup of 
>> platform_profile.
>>  +               */
>>  +               return 0;
> 
> Do it other way around,
> 
> /*
>  * Comment
>  */
> if (!...)
>   return 0;

Thanks, I think I was transliterating thought process to code as I went 
along.
All updated now.

> 
>>  +       }
> 
> 
> --
> With Best Regards,
> Andy Shevchenko

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

[Luke Jones]

A very quick question: should I be enabling platform_profile by default 
if asus_wmi is enabled in kconfig?

On Sat, Aug 14 2021 at 23:46:06 +1200, Luke Jones <luke@ljones.dev> 
wrote:
> Hi Andy, thanks for the feedback. All is addressed, and some inline 
> comment/reply. New version incoming pending pr_info() vs dev_info()
> 
> On Sat, Aug 14 2021 at 12:40:39 +0300, Andy Shevchenko 
> <andy.shevchenko@gmail.com> wrote:
>> On Sat, Aug 14, 2021 at 7:33 AM 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.
>>> 
>>>  Because throttle_thermal_policy is used by platform_profile and is
>>>  writeable separately to platform_profile any userspace changes to
>>>  throttle_thermal_policy need to notify platform_profile.
>>> 
>>>  In future throttle_thermal_policy sysfs should be removed so that
>>>  only one method controls the laptop power profile.
>> 
>> Some comments below.
>> 
>> ...
>> 
>>>  +       /*
>>>  +        * Ensure that platform_profile updates userspace with the 
>>> \x7f\x7fchange to ensure
>>>  +        * that platform_profile and throttle_thermal_policy_mode 
>>> \x7f\x7fare in sync
>> 
>> Missed period here and in other multi-line comments.
>> 
>>>  +        */
>> 
>> ...
>> 
>>>  +       /* 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;
>> 
>> This will be better in a form
>> 
>>     if (!..._available)
>>         return -ENODATA; // what -1 means?
>> 
> 
> I wasn't sure what the best return was here. On your suggestion I've 
> changed to ENODATA
> 
>>     tp = ...;
>>     if (tp < 0)
>>         return tp;
>> 
>> ...
>> 
>>>  +       /* All possible toggles like throttle_thermal_policy here 
>>> */
>>>  +       if (!asus->throttle_thermal_policy_available) {
>>>  +               return -1;
>> 
>> See above.
>> 
>>>  +       }
>> 
>> ...
>> 
>>>  +       if (asus->throttle_thermal_policy_available) {
>>>  +               pr_info("Using throttle_thermal_policy for 
>>> \x7f\x7fplatform_profile support\n");
>> 
>> Why pr_*()?
> 
> That seemed to be the convention? I see there is also dev_info(), so 
> I've switched to that as it seems more appropriate.
> 
>> 
>>>  +       } else {
>>>  +               /*
>>>  +                * Not an error if a component platform_profile 
>>> \x7f\x7frelies on is unavailable
>>>  +                * so early return, skipping the setup of 
>>> \x7f\x7fplatform_profile.
>>>  +               */
>>>  +               return 0;
>> 
>> Do it other way around,
>> 
>> /*
>>  * Comment
>>  */
>> if (!...)
>>   return 0;
> 
> Thanks, I think I was transliterating thought process to code as I 
> went along.
> All updated now.
> 
>> 
>>>  +       }
>> 
>> 
>> --
>> With Best Regards,
>> Andy Shevchenko
> 

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

[Andy Shevchenko]

On Sat, Aug 14, 2021 at 3:47 PM Luke Jones <luke@ljones.dev> wrote:
> A very quick question: should I be enabling platform_profile by default
> if asus_wmi is enabled in kconfig?

Very quick for you, but me. I dunno. I think the best thing is what
Hans can tell here.

Nevertheless, I think that this kind of default should be applied to
all modules (yes, I know there might be possible exceptions).

-- 
With Best Regards,
Andy Shevchenko

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

[Hans de Goede]

Hi,

On 8/14/21 2:47 PM, Luke Jones wrote:
> A very quick question: should I be enabling platform_profile by default if asus_wmi is enabled in kconfig?

You should add a "select ACPI_PLATFORM_PROFILE" to the Kconfig part for ASUS_WMI,
the PLATFORM_PROFILE support / common code is a library, so it needs to be selected
by the drivers which need it.

Regards,

Hans


> 
> On Sat, Aug 14 2021 at 23:46:06 +1200, Luke Jones <luke@ljones.dev> wrote:
>> Hi Andy, thanks for the feedback. All is addressed, and some inline comment/reply. New version incoming pending pr_info() vs dev_info()
>>
>> On Sat, Aug 14 2021 at 12:40:39 +0300, Andy Shevchenko <andy.shevchenko@gmail.com> wrote:
>>> On Sat, Aug 14, 2021 at 7:33 AM 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.
>>>>
>>>>  Because throttle_thermal_policy is used by platform_profile and is
>>>>  writeable separately to platform_profile any userspace changes to
>>>>  throttle_thermal_policy need to notify platform_profile.
>>>>
>>>>  In future throttle_thermal_policy sysfs should be removed so that
>>>>  only one method controls the laptop power profile.
>>>
>>> Some comments below.
>>>
>>> ...
>>>
>>>>  +       /*
>>>>  +        * Ensure that platform_profile updates userspace with the \x7f\x7fchange to ensure
>>>>  +        * that platform_profile and throttle_thermal_policy_mode \x7f\x7fare in sync
>>>
>>> Missed period here and in other multi-line comments.
>>>
>>>>  +        */
>>>
>>> ...
>>>
>>>>  +       /* 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;
>>>
>>> This will be better in a form
>>>
>>>     if (!..._available)
>>>         return -ENODATA; // what -1 means?
>>>
>>
>> I wasn't sure what the best return was here. On your suggestion I've changed to ENODATA
>>
>>>     tp = ...;
>>>     if (tp < 0)
>>>         return tp;
>>>
>>> ...
>>>
>>>>  +       /* All possible toggles like throttle_thermal_policy here */
>>>>  +       if (!asus->throttle_thermal_policy_available) {
>>>>  +               return -1;
>>>
>>> See above.
>>>
>>>>  +       }
>>>
>>> ...
>>>
>>>>  +       if (asus->throttle_thermal_policy_available) {
>>>>  +               pr_info("Using throttle_thermal_policy for \x7f\x7fplatform_profile support\n");
>>>
>>> Why pr_*()?
>>
>> That seemed to be the convention? I see there is also dev_info(), so I've switched to that as it seems more appropriate.
>>
>>>
>>>>  +       } else {
>>>>  +               /*
>>>>  +                * Not an error if a component platform_profile \x7f\x7frelies on is unavailable
>>>>  +                * so early return, skipping the setup of \x7f\x7fplatform_profile.
>>>>  +               */
>>>>  +               return 0;
>>>
>>> Do it other way around,
>>>
>>> /*
>>>  * Comment
>>>  */
>>> if (!...)
>>>   return 0;
>>
>> Thanks, I think I was transliterating thought process to code as I went along.
>> All updated now.
>>
>>>
>>>>  +       }
>>>
>>>
>>> -- 
>>> With Best Regards,
>>> Andy Shevchenko
>>
> 
> 

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

[Andy Shevchenko]

On Sat, Aug 14, 2021 at 2:46 PM Luke Jones <luke@ljones.dev> wrote:
> On Sat, Aug 14 2021 at 12:40:39 +0300, Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
> > On Sat, Aug 14, 2021 at 7:33 AM Luke D. Jones <luke@ljones.dev> wrote:

...

> >>  +               pr_info("Using throttle_thermal_policy for
> >> platform_profile support\n");
> >
> > Why pr_*()?
>
> That seemed to be the convention? I see there is also dev_info(), so
> I've switched to that as it seems more appropriate.

The rule of thumb is if you have the device the message belongs to,
use dev_*(), if it's really stuff before we know anything about
devices, use pr_*(). In some cases if you have ACPI handle, you may
use acpi_handle_*().

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v3 0/1] asus-wmi: add platform_profile support

[Luke Jones]

On Sat, Aug 14 2021 at 16:31:02 +1200, Luke D. Jones <luke@ljones.dev> 
wrote:
> Changelog:
> - V2
>   + Correctly unregister from platform_profile if
>     throttle_thermal_policy fails
>   + Do platform_profile_notify() in both 
> throttle_thermal_policy_store()
>     and in throttle_thermal_policy_switch_next()
>   + Remove unnecessary prep for possible fan-boost modes as this
>     doesn't match expected platform_profile behaviour
> - V3
>   + Add missing declaration for err in
>     throttle_thermal_policy_switch_next
> 
> Luke D. Jones (1):
>   asus-wmi: Add support for platform_profile
> 
>  drivers/platform/x86/asus-wmi.c | 139 
> +++++++++++++++++++++++++++++++-
>  1 file changed, 135 insertions(+), 4 deletions(-)
> 
> --
> 2.31.1

Hi,

I teested the patch again and it appears that the 
platform_profile_notify() in both throttle_thermal_policy_store() and 
throttle_thermal_policy_switch_next() updates the 
/sys/firmware/acpi/platform_profile sysfs path fine, but userspace 
isn't updated?

The way I'm checking is:
1. echo 1 |sudo tee 
/sys/devices/platform/asus-nb-wmi/throttle_thermal_policy
2. cat -p /sys/firmware/acpi/platform_profile
   - performance (updated correctly by platform_profile_notify)
3. Check gnome-settings, not updated.

Doing `echo "performance" |sudo tee 
/sys/firmware/acpi/platform_profile` updates both 
throttle_thermal_policy and userspace as expected. I'm wondering if 
I've missed something?

Cheers,
Luke.

Re: [PATCH v3 0/1] asus-wmi: add platform_profile support

[Hans de Goede]

Hi,

On 8/14/21 9:51 AM, Luke Jones wrote:
> 
> 
> On Sat, Aug 14 2021 at 16:31:02 +1200, Luke D. Jones <luke@ljones.dev> wrote:
>> Changelog:
>> - V2
>>   + Correctly unregister from platform_profile if
>>     throttle_thermal_policy fails
>>   + Do platform_profile_notify() in both throttle_thermal_policy_store()
>>     and in throttle_thermal_policy_switch_next()
>>   + Remove unnecessary prep for possible fan-boost modes as this
>>     doesn't match expected platform_profile behaviour
>> - V3
>>   + Add missing declaration for err in
>>     throttle_thermal_policy_switch_next
>>
>> Luke D. Jones (1):
>>   asus-wmi: Add support for platform_profile
>>
>>  drivers/platform/x86/asus-wmi.c | 139 +++++++++++++++++++++++++++++++-
>>  1 file changed, 135 insertions(+), 4 deletions(-)
>>
>> -- 
>> 2.31.1
> 
> Hi,
> 
> I teested the patch again and it appears that the platform_profile_notify() in both throttle_thermal_policy_store() and throttle_thermal_policy_switch_next() updates the /sys/firmware/acpi/platform_profile sysfs path fine, but userspace isn't updated?
> 
> The way I'm checking is:
> 1. echo 1 |sudo tee /sys/devices/platform/asus-nb-wmi/throttle_thermal_policy
> 2. cat -p /sys/firmware/acpi/platform_profile
>   - performance (updated correctly by platform_profile_notify)
> 3. Check gnome-settings, not updated.
> 
> Doing `echo "performance" |sudo tee /sys/firmware/acpi/platform_profile` updates both throttle_thermal_policy and userspace as expected. I'm wondering if I've missed something?

If you add a printk where you call platform_profile_notify() and you see that
happening, then you are likely seeing a userspace bug. Possibly your
power-profile-daemon is simply a bit old and therefor does not support
the combination of profiles which asus-wmi offers, IIRC it falls back to
using intel-pstate in that case.

You could try building the latest power-profile-daemon from git and run
it in verbose mode. If it sees the changes and the control-panel applet is
still not updating then I would not worry about that. The userspace code
is still somewhat new and I'm not sure which version your distro is
running and how well it is keeping up with gnome-updates.

Regards,

Hans

Re: [PATCH v3 0/1] asus-wmi: add platform_profile support

[Bastien Nocera]

On Sun, 2021-08-15 at 15:48 +0200, Hans de Goede wrote:
> Hi,
> 
> On 8/14/21 9:51 AM, Luke Jones wrote:
> > 
> > 
> > On Sat, Aug 14 2021 at 16:31:02 +1200, Luke D. Jones
> > <luke@ljones.dev> wrote:
> > > Changelog:
> > > - V2
> > >   + Correctly unregister from platform_profile if
> > >     throttle_thermal_policy fails
> > >   + Do platform_profile_notify() in both
> > > throttle_thermal_policy_store()
> > >     and in throttle_thermal_policy_switch_next()
> > >   + Remove unnecessary prep for possible fan-boost modes as this
> > >     doesn't match expected platform_profile behaviour
> > > - V3
> > >   + Add missing declaration for err in
> > >     throttle_thermal_policy_switch_next
> > > 
> > > Luke D. Jones (1):
> > >   asus-wmi: Add support for platform_profile
> > > 
> > >  drivers/platform/x86/asus-wmi.c | 139
> > > +++++++++++++++++++++++++++++++-
> > >  1 file changed, 135 insertions(+), 4 deletions(-)
> > > 
> > > -- 
> > > 2.31.1
> > 
> > Hi,
> > 
> > I teested the patch again and it appears that the
> > platform_profile_notify() in both throttle_thermal_policy_store() and
> > throttle_thermal_policy_switch_next() updates the
> > /sys/firmware/acpi/platform_profile sysfs path fine, but userspace
> > isn't updated?
> > 
> > The way I'm checking is:
> > 1. echo 1 |sudo tee /sys/devices/platform/asus-nb-
> > wmi/throttle_thermal_policy
> > 2. cat -p /sys/firmware/acpi/platform_profile
> >   - performance (updated correctly by platform_profile_notify)
> > 3. Check gnome-settings, not updated.
> > 
> > Doing `echo "performance" |sudo tee
> > /sys/firmware/acpi/platform_profile` updates both
> > throttle_thermal_policy and userspace as expected. I'm wondering if
> > I've missed something?
> 
> If you add a printk where you call platform_profile_notify() and you
> see that
> happening, then you are likely seeing a userspace bug. Possibly your
> power-profile-daemon is simply a bit old and therefor does not support
> the combination of profiles which asus-wmi offers, IIRC it falls back
> to
> using intel-pstate in that case.

Support for the quiet profile is only available in git:
https://gitlab.freedesktop.org/hadess/power-profiles-daemon/-/commit/c9b646025d9f155509a6cda1c292bfd120daeb9e

You can apply the patch on top of 0.9.0 if you want to use your
distribution's packages or something.

There's debugging info in the README should that be necessary.

> You could try building the latest power-profile-daemon from git and run
> it in verbose mode. If it sees the changes and the control-panel applet
> is
> still not updating then I would not worry about that. The userspace
> code
> is still somewhat new and I'm not sure which version your distro is
> running and how well it is keeping up with gnome-updates.
> 
> Regards,
> 
> Hans
> 

Re: [PATCH v3 0/1] asus-wmi: add platform_profile support

[Luke Jones]

On Sun, Aug 15 2021 at 15:48:49 +0200, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi,
> 
> On 8/14/21 9:51 AM, Luke Jones wrote:
>> 
>> 
>>  On Sat, Aug 14 2021 at 16:31:02 +1200, Luke D. Jones 
>> <luke@ljones.dev> wrote:
>>>  Changelog:
>>>  - V2
>>>    + Correctly unregister from platform_profile if
>>>      throttle_thermal_policy fails
>>>    + Do platform_profile_notify() in both 
>>> throttle_thermal_policy_store()
>>>      and in throttle_thermal_policy_switch_next()
>>>    + Remove unnecessary prep for possible fan-boost modes as this
>>>      doesn't match expected platform_profile behaviour
>>>  - V3
>>>    + Add missing declaration for err in
>>>      throttle_thermal_policy_switch_next
>>> 
>>>  Luke D. Jones (1):
>>>    asus-wmi: Add support for platform_profile
>>> 
>>>   drivers/platform/x86/asus-wmi.c | 139 
>>> +++++++++++++++++++++++++++++++-
>>>   1 file changed, 135 insertions(+), 4 deletions(-)
>>> 
>>>  --
>>>  2.31.1
>> 
>>  Hi,
>> 
>>  I teested the patch again and it appears that the 
>> platform_profile_notify() in both throttle_thermal_policy_store() 
>> and throttle_thermal_policy_switch_next() updates the 
>> /sys/firmware/acpi/platform_profile sysfs path fine, but userspace 
>> isn't updated?
>> 
>>  The way I'm checking is:
>>  1. echo 1 |sudo tee 
>> /sys/devices/platform/asus-nb-wmi/throttle_thermal_policy
>>  2. cat -p /sys/firmware/acpi/platform_profile
>>    - performance (updated correctly by platform_profile_notify)
>>  3. Check gnome-settings, not updated.
>> 
>>  Doing `echo "performance" |sudo tee 
>> /sys/firmware/acpi/platform_profile` updates both 
>> throttle_thermal_policy and userspace as expected. I'm wondering if 
>> I've missed something?
> 
> If you add a printk where you call platform_profile_notify() and you 
> see that
> happening, then you are likely seeing a userspace bug. Possibly your
> power-profile-daemon is simply a bit old and therefor does not support
> the combination of profiles which asus-wmi offers, IIRC it falls back 
> to
> using intel-pstate in that case.

It's possible that it's a userspace bug then. The power-profile-daemon 
I'm using is fresh from git (0.9+). To be clear updating via 
/sys/firmware/acpi/platform_profile works perfectly fine and 
power-profile-daemon updates etc. But if I do platform_profile_notify() 
then it doesn't seem to be updated. Nevertheless I will finalise the 
patch as it is and submit for merging and we can go from there.

> 
> You could try building the latest power-profile-daemon from git and 
> run
> it in verbose mode. If it sees the changes and the control-panel 
> applet is
> still not updating then I would not worry about that. The userspace 
> code
> is still somewhat new and I'm not sure which version your distro is
> running and how well it is keeping up with gnome-updates.
> 
> Regards,
> 
> Hans
> 

Re: [PATCH v3 0/1] asus-wmi: add platform_profile support

[Hans de Goede]

Hi,

On 8/16/21 1:00 AM, Luke Jones wrote:
> 
> 
> On Sun, Aug 15 2021 at 15:48:49 +0200, Hans de Goede <hdegoede@redhat.com> wrote:
>> Hi,
>>
>> On 8/14/21 9:51 AM, Luke Jones wrote:
>>>
>>>
>>>  On Sat, Aug 14 2021 at 16:31:02 +1200, Luke D. Jones <luke@ljones.dev> wrote:
>>>>  Changelog:
>>>>  - V2
>>>>    + Correctly unregister from platform_profile if
>>>>      throttle_thermal_policy fails
>>>>    + Do platform_profile_notify() in both throttle_thermal_policy_store()
>>>>      and in throttle_thermal_policy_switch_next()
>>>>    + Remove unnecessary prep for possible fan-boost modes as this
>>>>      doesn't match expected platform_profile behaviour
>>>>  - V3
>>>>    + Add missing declaration for err in
>>>>      throttle_thermal_policy_switch_next
>>>>
>>>>  Luke D. Jones (1):
>>>>    asus-wmi: Add support for platform_profile
>>>>
>>>>   drivers/platform/x86/asus-wmi.c | 139 +++++++++++++++++++++++++++++++-
>>>>   1 file changed, 135 insertions(+), 4 deletions(-)
>>>>
>>>>  --
>>>>  2.31.1
>>>
>>>  Hi,
>>>
>>>  I teested the patch again and it appears that the platform_profile_notify() in both throttle_thermal_policy_store() and throttle_thermal_policy_switch_next() updates the /sys/firmware/acpi/platform_profile sysfs path fine, but userspace isn't updated?
>>>
>>>  The way I'm checking is:
>>>  1. echo 1 |sudo tee /sys/devices/platform/asus-nb-wmi/throttle_thermal_policy
>>>  2. cat -p /sys/firmware/acpi/platform_profile
>>>    - performance (updated correctly by platform_profile_notify)
>>>  3. Check gnome-settings, not updated.
>>>
>>>  Doing `echo "performance" |sudo tee /sys/firmware/acpi/platform_profile` updates both throttle_thermal_policy and userspace as expected. I'm wondering if I've missed something?
>>
>> If you add a printk where you call platform_profile_notify() and you see that
>> happening, then you are likely seeing a userspace bug. Possibly your
>> power-profile-daemon is simply a bit old and therefor does not support
>> the combination of profiles which asus-wmi offers, IIRC it falls back to
>> using intel-pstate in that case.
> 
> It's possible that it's a userspace bug then. The power-profile-daemon I'm using is fresh from git (0.9+). To be clear updating via /sys/firmware/acpi/platform_profile works perfectly fine and power-profile-daemon updates etc. But if I do platform_profile_notify() then it doesn't seem to be updated. Nevertheless I will finalise the patch as it is and submit for merging and we can go from there.

This is weird, I wonder if there is a difference in glib versions used, where one does
include polling for POLLPRI on sysfs files in GFileMonitor and the other only uses
inotify which only catches changes from another userspace write ?

Regards,

Hans