From: Luke Jones <firstname.lastname@example.org>
To: Hans de Goede <email@example.com>
Cc: Bastien Nocera <firstname.lastname@example.org>,
Subject: Re: [PATCH] asus-wmi: Add support for platform_profile
Date: Fri, 13 Aug 2021 21:42:10 +1200 [thread overview]
Message-ID: <AYURXQ.K273M0JTASMR@ljones.dev> (raw)
I'll try to follow along here...
On Fri, Aug 13 2021 at 10:44:07 +0200, Hans de Goede
> On 8/13/21 9:40 AM, Hans de Goede wrote:
>> On 8/13/21 9:13 AM, Luke Jones wrote:
>>> On Fri, Aug 13 2021 at 09:03:04 +0200, Hans de Goede
>>> <email@example.com> wrote:
>>>> 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
>>>>> struct platform_profile_handler *handler =
>>>>> &asus->platform_profile_handler; // added
>>>>> u8 new_mode = asus->throttle_thermal_policy_mode + 1;
>>>>> 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);
>>>>> 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
>>>> (we can define a new EV_KEY_.... code for this) and then let
>>>> 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
>>>> 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
>>>> So the new throttle_thermal_policy_switch_next() would look like
>>>> static int throttle_thermal_policy_switch_next(struct asus_wmi
>>>> 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.
>>> 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
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.
next prev 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
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).