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

[Luke Jones <luke@ljones.dev>]

I'll try to follow along here...

On Fri, Aug 13 2021 at 10:44:07 +0200, Hans de Goede 
<hdegoede@redhat.com> wrote:
> Hi,
> 
> On 8/13/21 9:40 AM, Hans de Goede wrote:
>>  Hi,
>> 
>>  On 8/13/21 9:13 AM, Luke Jones wrote:
>>> 
>>> 
>>>  On Fri, Aug 13 2021 at 09:03:04 +0200, Hans de Goede 
>>> <hdegoede@redhat.com> 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;
>>>>> 
>>>>>   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 
>>>> 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
>> 
>>  <snip>
>> 
>>>  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 
/sys/bus/platform/devices/asus-nb-wmi/throttle_thermal_policy as 
described above.
Not to /sys/firmware/acpi/platform_profile

Cheers,
Luke.

> 
> ###
> 
> 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.
> 
> Regards,
> 
> Hans
>