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

From: Luke Jones
Date: Fri Aug 13 2021 - 05:42:30 EST

I'll try to follow along here...

On Fri, Aug 13 2021 at 10:44:07 +0200, Hans de Goede <hdegoede@xxxxxxxxxx> 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@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.

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