| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-Id: <R1ORXQ.WGLIPS8I54X63@ljones.dev>
Date: Fri, 13 Aug 2021 19:13:03 +1200
From: Luke Jones <luke@...nes.dev>
To: Hans de Goede <hdegoede@...hat.com>
Cc: linux-kernel@...r.kernel.org, hadess@...ess.net,
platform-driver-x86@...r.kernel.org
Subject: Re: [PATCH] asus-wmi: Add support for platform_profile
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
>
>
>> On Fri, Aug 13 2021 at 14:42:01 +1200, Luke D. Jones
>> <luke@...nes.dev> wrote:
>>> Add initial support for platform_profile where the support is
>>> based on availability of ASUS_THROTTLE_THERMAL_POLICY.
>>>
>>> Signed-off-by: Luke D. Jones <luke@...nes.dev>
>>> ---
>>> drivers/platform/x86/asus-wmi.c | 112
>>> ++++++++++++++++++++++++++++++++
>>> 1 file changed, 112 insertions(+)
>>>
>>> diff --git a/drivers/platform/x86/asus-wmi.c
>>> b/drivers/platform/x86/asus-wmi.c
>>> index 90a6a0d00deb..62260043c15c 100644
>>> --- a/drivers/platform/x86/asus-wmi.c
>>> +++ b/drivers/platform/x86/asus-wmi.c
>>> @@ -26,6 +26,7 @@
>>> #include <linux/rfkill.h>
>>> #include <linux/pci.h>
>>> #include <linux/pci_hotplug.h>
>>> +#include <linux/platform_profile.h>
>>> #include <linux/power_supply.h>
>>> #include <linux/hwmon.h>
>>> #include <linux/hwmon-sysfs.h>
>>> @@ -219,6 +220,9 @@ struct asus_wmi {
>>> bool throttle_thermal_policy_available;
>>> u8 throttle_thermal_policy_mode;
>>>
>>> + struct platform_profile_handler platform_profile_handler;
>>> + bool platform_profile_support;
>>> +
>>> // The RSOC controls the maximum charging percentage.
>>> bool battery_rsoc_available;
>>>
>>> @@ -2144,6 +2148,106 @@ static ssize_t
>>> throttle_thermal_policy_store(struct device *dev,
>>> // Throttle thermal policy: 0 - default, 1 - overboost, 2 - silent
>>> static DEVICE_ATTR_RW(throttle_thermal_policy);
>>>
>>> +/* Platform profile
>>> ***********************************************************/
>>> +static int platform_profile_get(struct platform_profile_handler
>>> *pprof,
>>> + enum platform_profile_option *profile)
>>> +{
>>> + struct asus_wmi *asus;
>>> + int tp;
>>> +
>>> + asus = container_of(pprof, struct asus_wmi,
>>> platform_profile_handler);
>>> +
>>> + /* All possible toggles like throttle_thermal_policy here */
>>> + if (asus->throttle_thermal_policy_available) {
>>> + tp = asus->throttle_thermal_policy_mode;
>>> + } else {
>>> + return -1;
>>> + }
>>> +
>>> + if (tp < 0)
>>> + return tp;
>>> +
>>> + switch (tp) {
>>> + case ASUS_THROTTLE_THERMAL_POLICY_DEFAULT:
>>> + *profile = PLATFORM_PROFILE_BALANCED;
>>> + break;
>>> + case ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST:
>>> + *profile = PLATFORM_PROFILE_PERFORMANCE;
>>> + break;
>>> + case ASUS_THROTTLE_THERMAL_POLICY_SILENT:
>>> + *profile = PLATFORM_PROFILE_QUIET;
>>> + break;
>>> + default:
>>> + return -EINVAL;
>>> + }
>>> +
>>> + return 0;
>>> +}
>>> +
>>> +static int platform_profile_set(struct platform_profile_handler
>>> *pprof,
>>> + enum platform_profile_option profile)
>>> +{
>>> + struct asus_wmi *asus;
>>> + int tp;
>>> +
>>> + asus = container_of(pprof, struct asus_wmi,
>>> platform_profile_handler);
>>> +
>>> + /* All possible toggles like throttle_thermal_policy here */
>>> + if (!asus->throttle_thermal_policy_available) {
>>> + return -1;
>>> + }
>>> +
>>> + switch (profile) {
>>> + case PLATFORM_PROFILE_PERFORMANCE:
>>> + tp = ASUS_THROTTLE_THERMAL_POLICY_OVERBOOST;
>>> + break;
>>> + case PLATFORM_PROFILE_BALANCED:
>>> + tp = ASUS_THROTTLE_THERMAL_POLICY_DEFAULT;
>>> + break;
>>> + case PLATFORM_PROFILE_QUIET:
>>> + tp = ASUS_THROTTLE_THERMAL_POLICY_SILENT;
>>> + break;
>>> + default:
>>> + return -EOPNOTSUPP;
>>> + }
>>> +
>>> + if (asus->throttle_thermal_policy_available) {
>>> + asus->throttle_thermal_policy_mode = tp;
>>> + return throttle_thermal_policy_write(asus);
>>> + }
>>> + return 0;
>>> +}
>>> +
>>> +static int platform_profile_setup(struct asus_wmi *asus)
>>> +{
>>> + int err;
>>> +
>>> + if (asus->throttle_thermal_policy_available) {
>>> + pr_info("Using throttle_thermal_policy for
>>> platform_profile support\n");
>>> + } else {
>>> + /*
>>> + * Not an error if a component platform_profile relies on
>>> is unavailable
>>> + * so early return, skipping the setup of
>>> platform_profile.
>>> + */
>>> + return 0;
>>> + }
>>> + asus->platform_profile_handler.profile_get =
>>> platform_profile_get;
>>> + asus->platform_profile_handler.profile_set =
>>> platform_profile_set;
>>> +
>>> + set_bit(PLATFORM_PROFILE_QUIET,
>>> asus->platform_profile_handler.choices);
>>> + set_bit(PLATFORM_PROFILE_BALANCED,
>>> + asus->platform_profile_handler.choices);
>>> + set_bit(PLATFORM_PROFILE_PERFORMANCE,
>>> + asus->platform_profile_handler.choices);
>>> +
>>> + err =
>>> platform_profile_register(&asus->platform_profile_handler);
>>> + if (err)
>>> + return err;
>>> +
>>> + asus->platform_profile_support = true;
>>> + return 0;
>>> +}
>>> +
>>> /* Backlight
>>> ******************************************************************/
>>>
>>> static int read_backlight_power(struct asus_wmi *asus)
>>> @@ -2904,6 +3008,10 @@ static int asus_wmi_add(struct
>>> platform_device *pdev)
>>> else
>>> throttle_thermal_policy_set_default(asus);
>>>
>>> + err = platform_profile_setup(asus);
>>> + if (err)
>>> + goto fail_platform_profile_setup;
>>> +
>>> err = panel_od_check_present(asus);
>>> if (err)
>>> goto fail_panel_od;
>>> @@ -2993,6 +3101,7 @@ static int asus_wmi_add(struct
>>> platform_device *pdev)
>>> asus_wmi_sysfs_exit(asus->platform_device);
>>> fail_sysfs:
>>> fail_throttle_thermal_policy:
>>> +fail_platform_profile_setup:
>>> fail_fan_boost_mode:
>>> fail_egpu_enable:
>>> fail_dgpu_disable:
>>> @@ -3017,6 +3126,9 @@ static int asus_wmi_remove(struct
>>> platform_device *device)
>>> asus_fan_set_auto(asus);
>>> asus_wmi_battery_exit(asus);
>>>
>>> + if (asus->platform_profile_support)
>>> + platform_profile_remove();
>>> +
>>> kfree(asus);
>>> return 0;
>>> }
>>> --
>>> 2.31.1
>>>
>>
>>
>
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().
I will submit new version after completing other feedback.
Many Thanks,
Luke.
Powered by blists - more mailing lists