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

[Luke Jones]

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