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

[Hans de Goede]

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@xxxxxxxxxx> 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.

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.

###

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