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


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.

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.



  parent reply	other threads:[~2021-08-13  8:44 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 [this message]
2021-08-13  8:46           ` Hans de Goede
2021-08-13  9:42           ` Luke Jones
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).