platform-driver-x86.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Luke Jones <luke@ljones.dev>
To: Hans de Goede <hdegoede@redhat.com>
Cc: Bastien Nocera <hadess@hadess.net>,
	linux-kernel@vger.kernel.org,
	platform-driver-x86@vger.kernel.org
Subject: Re: [PATCH] asus-wmi: Add support for platform_profile
Date: Fri, 13 Aug 2021 20:27:04 +1200	[thread overview]
Message-ID: <4HRRXQ.EDZWS3NOU3J32@ljones.dev> (raw)
In-Reply-To: <5b503320-c1a3-2653-b269-ba8d40568edf@redhat.com>



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



  reply	other threads:[~2021-08-13  8:27 UTC|newest]

Thread overview: 13+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-08-13  2:42 [PATCH] asus-wmi: Add support for platform_profile Luke D. Jones
2021-08-13  5:27 ` Luke Jones
2021-08-13  7:03   ` Hans de Goede
2021-08-13  7:13     ` Luke Jones
2021-08-13  7:40       ` Hans de Goede
2021-08-13  8:27         ` Luke Jones [this message]
2021-08-13  8:57           ` Hans de Goede
2021-08-13  8:44         ` Hans de Goede
2021-08-13  8:46           ` Hans de Goede
2021-08-13  9:42           ` Luke Jones
2021-08-13 10:05             ` Hans de Goede
2021-08-13  6:47 ` Hans de Goede
2021-08-13  7:20   ` Luke Jones

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=4HRRXQ.EDZWS3NOU3J32@ljones.dev \
    --to=luke@ljones.dev \
    --cc=hadess@hadess.net \
    --cc=hdegoede@redhat.com \
    --cc=linux-kernel@vger.kernel.org \
    --cc=platform-driver-x86@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).