Hi, 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 <hdegoede@xxxxxxxxxx> wrote: <snip> >>> 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 /sys/bus/platform/devices/asus-nb-wmi/throttle_thermal_policy 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. Regards, Hans
