archive mirror
 help / color / mirror / Atom feed
From: Luke Jones <>
To: Hans de Goede <>
Cc: Bastien Nocera <>,,
Subject: Re: [PATCH] asus-wmi: Add support for platform_profile
Date: Fri, 13 Aug 2021 21:42:10 +1200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

I'll try to follow along here...

On Fri, Aug 13 2021 at 10:44:07 +0200, Hans de Goede 
<> 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 
>>> <> 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;
>>>>>   // 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


> ###
> 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

  parent reply	other threads:[~2021-08-13  9:42 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-13  2:42 [PATCH] asus-wmi: Add support for platform_profile Luke D. Jones
2021-08-13  5:27 ` Luke Jones
2021-08-13  7:03   ` Hans de Goede
2021-08-13  7:13     ` Luke Jones
2021-08-13  7:40       ` Hans de Goede
2021-08-13  8:27         ` Luke Jones
2021-08-13  8:57           ` Hans de Goede
2021-08-13  8:44         ` Hans de Goede
2021-08-13  8:46           ` Hans de Goede
2021-08-13  9:42           ` Luke Jones [this message]
2021-08-13 10:05             ` Hans de Goede
2021-08-13  6:47 ` Hans de Goede
2021-08-13  7:20   ` Luke Jones

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).