Message ID | 20210813024201.9518-1-luke@ljones.dev (mailing list archive) |
---|---|
State | Changes Requested, archived |
Headers | show |
Series | asus-wmi: Add support for platform_profile | expand |
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'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. Regards, Luke. On Fri, Aug 13 2021 at 14:42:01 +1200, Luke D. Jones <luke@ljones.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@ljones.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 Luke, Thank you for this patch, it is great to see more and more laptop models getting support for the standard platform_profile userspace API. On 8/13/21 4:42 AM, Luke D. Jones 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@ljones.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) { You already check for asus->throttle_thermal_policy_available a couple of lines higher in this function and you return when it is not available ... I guess this was intended to also have a second if () for fan-boost mode ? Since you are not adding support for fan-boost mode at this time, it would be best (IMHO) to drop this second if for now and just take this path unconditionally. > + 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: I think you need to add this: if (asus->platform_profile_support) platform_profile_remove(); here so that if later fail-exit paths are taken the platform_profile support gets unregistered again. > 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; > } > Regards, Hans
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@ljones.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@ljones.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 >> > >
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 > > >> On Fri, Aug 13 2021 at 14:42:01 +1200, Luke D. Jones >> <luke@ljones.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@ljones.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.
On Fri, Aug 13 2021 at 08:47:56 +0200, Hans de Goede <hdegoede@redhat.com> wrote: > Hi Luke, > > Thank you for this patch, it is great to see more and more laptop > models getting support for the standard platform_profile userspace > API. > > On 8/13/21 4:42 AM, Luke D. Jones 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@ljones.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) { > > You already check for asus->throttle_thermal_policy_available a couple > of lines higher in this function and you return when it is not > available ... > > I guess this was intended to also have a second if () for > fan-boost mode ? Yes, sorry I missed that. I'd initially looked at adding fan-boost also but as it is *fans only* I didn't see the value in adding it. I've also not seen any use of this in the wild bar one laptop which was likely to be retired many months ago. > > Since you are not adding support for fan-boost mode at this time, > it would be best (IMHO) to drop this second if for now and > just take this path unconditionally. > Adjusted now. > >> + 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: > > I think you need to add this: > > if (asus->platform_profile_support) > platform_profile_remove(); > > here so that if later fail-exit paths are taken the platform_profile > support gets unregistered again. Good catch, thanks! :) Next version will come after some testing. > >> 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; >> } >> > > Regards, > > Hans >
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
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.
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@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. 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. 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. ### 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
p.s. <snip> Note that inotify (and thus GFileMonitor) does not really work on sysfs files. Interesting email thread about this here: https://linux-fsdevel.vger.kernel.narkive.com/u0qmXPFK/inotify-sysfs
Hi, On 8/13/21 10:27 AM, Luke Jones wrote: > > > 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. We definitely want the notify when the change is done by the kernel itself through the hotkey, which would not happen when the notify call gets moved to store. The question which I had was if we should also notify on store-s (writes to the sysfs file from userspace processes). The current drivers don't do a notify on store, but that seems wrong. Also see my other somewhat long email. Thinking more about this I think we should actually do the notify from the platform_profile_store() function inside drivers/acpi/platform_profile.c, that way all drivers will consistently do the notify after a store. This does mean that the asus-wmi code should only call platform_profile_notify() in the hotkey path as I suggested in my original proposal, so that we don't call notify twice for a single store. ### So summarizing my current thoughts on this: 1. The asus-wmi code should only call platform_profile_notify() in the hotkey path, to be consistent with other existing drivers. 2. We should document the POLLPRI stuff in Documentation/ABI/testing/sysfs-platform_profile This documentation should say that POLLPRI will be raised on any changes, independent of those changes coming from a userspace write; or coming from another source (typically a hotkey triggered profile change handled either directly by the embedded-controller or fully handled inside the kernel). 3. We should make platform_profile_store() function inside drivers/acpi/platform_profile.c call platform_profile_notify() on a successful store to make all the drivers consistently do a notify after a store. And I think it might make sense to fold 2. + 3. into a single patch. I'll put writing that patch on my to do list. Regards, Hans
I'll try to follow along here... On Fri, Aug 13 2021 at 10:44:07 +0200, Hans de Goede <hdegoede@redhat.com> 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@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. > > 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 >
Hi, On 8/13/21 11:42 AM, Luke Jones wrote: > I'll try to follow along here... > > On Fri, Aug 13 2021 at 10:44:07 +0200, Hans de Goede <hdegoede@redhat.com> wrote: <snip> >>> 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. 2. is something which we may do in a year or so, we have a strict no breaking userspace policy in the kernel; so people should be able to drop in a new kernel without updating asusctl and things should keep working. Which means that we are stuck with the /sys/bus/platform/devices/asus-nb-wmi/throttle_thermal_policy file for at least a year. So we need to 1, call platform_profily_notify on writes to /sys/bus/platform/devices/asus-nb-wmi/throttle_thermal_policy and on changes because of the hotkey, while not calling it from the profile_set callback. >> 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 Ah I see, yes I agree platform_profile_notify() should be called from the store handler for /sys/bus/platform/devices/asus-nb-wmi/throttle_thermal_policy. Basically it should be called for _all_ changes not done through the profile_set callback. Regards, Hans
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; }
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@ljones.dev> --- drivers/platform/x86/asus-wmi.c | 112 ++++++++++++++++++++++++++++++++ 1 file changed, 112 insertions(+)