Message-Id: <4HRRXQ.EDZWS3NOU3J32@ljones.dev>
Date:   Fri, 13 Aug 2021 20:27:04 +1200
From:   Luke Jones <luke@...nes.dev>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Bastien Nocera <hadess@...ess.net>, linux-kernel@...r.kernel.org,
        platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH] asus-wmi: Add support for platform_profile



On Fri, Aug 13 2021 at 09:40:25 +0200, Hans de Goede 
<hdegoede@...hat.com> 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@...hat.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.
> 
> 2. Once we have an answer to 1. we need to documented the
> expected behavior in Documentation/ABI/testing/sysfs-platform_profile
> 
> 3. If we go for doing a notify on any change, then we need
> to update the existing drivers to do this.
> 
> Regards,
> 
> Hans

My thinking for it was ensuring that a process that wrote to 
/sys/devices/platform/asus-nb-wmi/throttle_thermal_policy would force 
an update across all. I think perhaps I should move the notify to 
throttle_thermal_policy_store() instead which only activates when the 
path is written.

Cheers,
Luke.

Powered by blists - more mailing lists