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


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 <> wrote:


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



  reply	other threads:[~2021-08-13 10:05 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
2021-08-13 10:05             ` Hans de Goede [this message]
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).