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 09:40:25 +0200	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>


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


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



  reply	other threads:[~2021-08-13  7:40 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 [this message]
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
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).