Re: [PATCH] asus-wmi: Add support for platform_profile

[Hans de Goede]

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