From: Hans de Goede <email@example.com>
To: Luke Jones <firstname.lastname@example.org>
Cc: Bastien Nocera <email@example.com>,
Subject: Re: [PATCH] asus-wmi: Add support for platform_profile
Date: Fri, 13 Aug 2021 12:05:20 +0200 [thread overview]
Message-ID: <firstname.lastname@example.org> (raw)
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 <email@example.com> 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
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.
next prev parent 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
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).