| Message ID | 20240402022607.34625-9-luke@ljones.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | asus-wmi: add new features, clean up, fixes | expand |
On Tue, 2 Apr 2024, Luke D. Jones wrote: > Add support for an MCU powersave WMI call. This is intended to set the > MCU in to a low-power mode when sleeping. This mode can cut sleep power > use by around half. > > Signed-off-by: Luke D. Jones <luke@ljones.dev> Hi, I fail to follow the logic of the patch here. This patch makes it configurable which is not bad in itself but what is the reason why a user would not always want to cut sleep power usage down? So this sounds like a feature that the user wants always enabled if available. So what are the downsides of enabling it if it's supported?
On Wednesday, 3 April 2024 12:01:15 AM NZDT Ilpo Järvinen wrote: > On Tue, 2 Apr 2024, Luke D. Jones wrote: > > Add support for an MCU powersave WMI call. This is intended to set the > > MCU in to a low-power mode when sleeping. This mode can cut sleep power > > use by around half. > > > > Signed-off-by: Luke D. Jones <luke@ljones.dev> > > Hi, > > I fail to follow the logic of the patch here. This patch makes it > configurable which is not bad in itself but what is the reason why a user > would not always want to cut sleep power usage down? So this sounds like a > feature that the user wants always enabled if available. > > So what are the downsides of enabling it if it's supported? Honestly, I'm not sure. Previously it was a source of issues but with recent bios and more work in the various gaming-handheld distros it's much less of a problem. The issue before was that the MCU would appear every second suspend due to the suspend sequence being more parallel compared to windows being sequential - the acpi calls related to this would "unplug" the USB devices that are related to the n-key (keyboard, same internal dev as laptops) and not complete it before suspend. And then on resume it was unreliable. I worked around this by calling the unplug very early, and trying to "plug" super early also so that subsystems wouldn't notice the absence of the device and create new devices + paths on add. Most of the requirement for that came from some userspace programs unable to handle the unplug/plug part, but also bad device state occurring. But now with the forced wait for the device to finish its task, and then the forced wait before letting it add itself back everything is fine - although it does mean everything sees a device properly unplugged and plugged. All of the above is to say that the "powersave" function was also part of the issue as delayed things further and we couldn't add the device back before subsystems noticed. Distros like bazzite and chimeraOS are now using this patch, and I'd like to leave it to them to set a default for now. If it turns out everything is indeed safe as houses then we can adjust the kernel default. A side-note I think is that because there is a forced wait time due to unable to use the right acpi path, the old excuse of "but users might want the device to wake faster by turning off powersave" doesn't really apply now. I will discuss with the main stakeholders, but for now can we accept as is? If changes are required we'll get them done before the merge window. > > > --- > > > > .../ABI/testing/sysfs-platform-asus-wmi | 11 +++- > > drivers/platform/x86/asus-wmi.c | 50 +++++++++++++++++++ > > 2 files changed, 60 insertions(+), 1 deletion(-) > > > > diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi > > b/Documentation/ABI/testing/sysfs-platform-asus-wmi index > > 41b92e53e88a..28144371a0f1 100644 > > --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi > > +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi > > @@ -202,4 +202,13 @@ Contact: "Luke Jones" <luke@ljones.dev> > > > > Description: > > Set if the BIOS POST sound is played on boot. > > > > * 0 - False, > > > > - * 1 - True > > \ No newline at end of file > > + * 1 - True > > + > > +What: /sys/devices/platform/<platform>/mcu_powersave > > +Date: Apr 2024 > > +KernelVersion: 6.10 > > +Contact: "Luke Jones" <luke@ljones.dev> > > +Description: > > + Set if the MCU can go in to low-power mode on system sleep > > + * 0 - False, > > + * 1 - True > > diff --git a/drivers/platform/x86/asus-wmi.c > > b/drivers/platform/x86/asus-wmi.c index ddf568ef8c5e..cf872eed0986 100644 > > --- a/drivers/platform/x86/asus-wmi.c > > +++ b/drivers/platform/x86/asus-wmi.c > > @@ -1292,6 +1292,53 @@ static ssize_t nv_temp_target_show(struct device > > *dev,> > > } > > static DEVICE_ATTR_RW(nv_temp_target); > > > > +/* Ally MCU Powersave > > ********************************************************/ +static ssize_t > > mcu_powersave_show(struct device *dev, > > + struct device_attribute *attr, char *buf) > > +{ > > + struct asus_wmi *asus = dev_get_drvdata(dev); > > + int result; > > + > > + result = asus_wmi_get_devstate_simple(asus, > > ASUS_WMI_DEVID_MCU_POWERSAVE); + if (result < 0) > > + return result; > > + > > + return sysfs_emit(buf, "%d\n", result); > > +} > > + > > +static ssize_t mcu_powersave_store(struct device *dev, > > + struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + int result, err; > > + u32 enable; > > + > > + struct asus_wmi *asus = dev_get_drvdata(dev); > > + > > + result = kstrtou32(buf, 10, &enable); > > + if (result) > > + return result; > > + > > + if (enable > 1) > > + return -EINVAL; > > + > > + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MCU_POWERSAVE, enable, > > &result); + if (err) { > > + pr_warn("Failed to set MCU powersave: %d\n", err); > > + return err; > > + } > > + > > + if (result > 1) { > > + pr_warn("Failed to set MCU powersave (result): 0x%x\n", result); > > + return -EIO; > > + } > > + > > + sysfs_notify(&asus->platform_device->dev.kobj, NULL, "mcu_powersave"); > > + > > + return count; > > +} > > +static DEVICE_ATTR_RW(mcu_powersave); > > + > > > > /* Battery > > ********************************************************************/ > > > > /* The battery maximum charging percentage */ > > > > @@ -4299,6 +4346,7 @@ static struct attribute *platform_attributes[] = { > > > > &dev_attr_ppt_platform_sppt.attr, > > &dev_attr_nv_dynamic_boost.attr, > > &dev_attr_nv_temp_target.attr, > > > > + &dev_attr_mcu_powersave.attr, > > > > &dev_attr_boot_sound.attr, > > &dev_attr_panel_od.attr, > > &dev_attr_mini_led_mode.attr, > > > > @@ -4352,6 +4400,8 @@ static umode_t asus_sysfs_is_visible(struct kobject > > *kobj,> > > devid = ASUS_WMI_DEVID_NV_DYN_BOOST; > > > > else if (attr == &dev_attr_nv_temp_target.attr) > > > > devid = ASUS_WMI_DEVID_NV_THERM_TARGET; > > > > + else if (attr == &dev_attr_mcu_powersave.attr) > > + devid = ASUS_WMI_DEVID_MCU_POWERSAVE; > > > > else if (attr == &dev_attr_boot_sound.attr) > > > > devid = ASUS_WMI_DEVID_BOOT_SOUND; > > > > else if (attr == &dev_attr_panel_od.attr)
On Wed, 3 Apr 2024, Luke Jones wrote: > On Wednesday, 3 April 2024 12:01:15 AM NZDT Ilpo Järvinen wrote: > > On Tue, 2 Apr 2024, Luke D. Jones wrote: > > > Add support for an MCU powersave WMI call. This is intended to set the > > > MCU in to a low-power mode when sleeping. This mode can cut sleep power > > > use by around half. > > > > > > Signed-off-by: Luke D. Jones <luke@ljones.dev> > > > > Hi, > > > > I fail to follow the logic of the patch here. This patch makes it > > configurable which is not bad in itself but what is the reason why a user > > would not always want to cut sleep power usage down? So this sounds like a > > feature that the user wants always enabled if available. > > > > So what are the downsides of enabling it if it's supported? > > Honestly, I'm not sure. Previously it was a source of issues but with recent > bios and more work in the various gaming-handheld distros it's much less of a > problem. The issue before was that the MCU would appear every second suspend > due to the suspend sequence being more parallel compared to windows being > sequential - the acpi calls related to this would "unplug" the USB devices > that are related to the n-key (keyboard, same internal dev as laptops) and not > complete it before suspend. And then on resume it was unreliable. > > I worked around this by calling the unplug very early, and trying to "plug" > super early also so that subsystems wouldn't notice the absence of the device > and create new devices + paths on add. Most of the requirement for that came > from some userspace programs unable to handle the unplug/plug part, but also > bad device state occurring. > > But now with the forced wait for the device to finish its task, and then the > forced wait before letting it add itself back everything is fine - although it > does mean everything sees a device properly unplugged and plugged. > > All of the above is to say that the "powersave" function was also part of the > issue as delayed things further and we couldn't add the device back before > subsystems noticed. > > Distros like bazzite and chimeraOS are now using this patch, and I'd like to > leave it to them to set a default for now. If it turns out everything is > indeed safe as houses then we can adjust the kernel default. > > A side-note I think is that because there is a forced wait time due to unable > to use the right acpi path, the old excuse of "but users might want the device > to wake faster by turning off powersave" doesn't really apply now. > > I will discuss with the main stakeholders, but for now can we accept as is? If > changes are required we'll get them done before the merge window. Yes, I think it is okay to make it configurable first and then look separately into making it default on.
diff --git a/Documentation/ABI/testing/sysfs-platform-asus-wmi b/Documentation/ABI/testing/sysfs-platform-asus-wmi index 41b92e53e88a..28144371a0f1 100644 --- a/Documentation/ABI/testing/sysfs-platform-asus-wmi +++ b/Documentation/ABI/testing/sysfs-platform-asus-wmi @@ -202,4 +202,13 @@ Contact: "Luke Jones" <luke@ljones.dev> Description: Set if the BIOS POST sound is played on boot. * 0 - False, - * 1 - True \ No newline at end of file + * 1 - True + +What: /sys/devices/platform/<platform>/mcu_powersave +Date: Apr 2024 +KernelVersion: 6.10 +Contact: "Luke Jones" <luke@ljones.dev> +Description: + Set if the MCU can go in to low-power mode on system sleep + * 0 - False, + * 1 - True diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c index ddf568ef8c5e..cf872eed0986 100644 --- a/drivers/platform/x86/asus-wmi.c +++ b/drivers/platform/x86/asus-wmi.c @@ -1292,6 +1292,53 @@ static ssize_t nv_temp_target_show(struct device *dev, } static DEVICE_ATTR_RW(nv_temp_target); +/* Ally MCU Powersave ********************************************************/ +static ssize_t mcu_powersave_show(struct device *dev, + struct device_attribute *attr, char *buf) +{ + struct asus_wmi *asus = dev_get_drvdata(dev); + int result; + + result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MCU_POWERSAVE); + if (result < 0) + return result; + + return sysfs_emit(buf, "%d\n", result); +} + +static ssize_t mcu_powersave_store(struct device *dev, + struct device_attribute *attr, + const char *buf, size_t count) +{ + int result, err; + u32 enable; + + struct asus_wmi *asus = dev_get_drvdata(dev); + + result = kstrtou32(buf, 10, &enable); + if (result) + return result; + + if (enable > 1) + return -EINVAL; + + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MCU_POWERSAVE, enable, &result); + if (err) { + pr_warn("Failed to set MCU powersave: %d\n", err); + return err; + } + + if (result > 1) { + pr_warn("Failed to set MCU powersave (result): 0x%x\n", result); + return -EIO; + } + + sysfs_notify(&asus->platform_device->dev.kobj, NULL, "mcu_powersave"); + + return count; +} +static DEVICE_ATTR_RW(mcu_powersave); + /* Battery ********************************************************************/ /* The battery maximum charging percentage */ @@ -4299,6 +4346,7 @@ static struct attribute *platform_attributes[] = { &dev_attr_ppt_platform_sppt.attr, &dev_attr_nv_dynamic_boost.attr, &dev_attr_nv_temp_target.attr, + &dev_attr_mcu_powersave.attr, &dev_attr_boot_sound.attr, &dev_attr_panel_od.attr, &dev_attr_mini_led_mode.attr, @@ -4352,6 +4400,8 @@ static umode_t asus_sysfs_is_visible(struct kobject *kobj, devid = ASUS_WMI_DEVID_NV_DYN_BOOST; else if (attr == &dev_attr_nv_temp_target.attr) devid = ASUS_WMI_DEVID_NV_THERM_TARGET; + else if (attr == &dev_attr_mcu_powersave.attr) + devid = ASUS_WMI_DEVID_MCU_POWERSAVE; else if (attr == &dev_attr_boot_sound.attr) devid = ASUS_WMI_DEVID_BOOT_SOUND; else if (attr == &dev_attr_panel_od.attr)
Add support for an MCU powersave WMI call. This is intended to set the MCU in to a low-power mode when sleeping. This mode can cut sleep power use by around half. Signed-off-by: Luke D. Jones <luke@ljones.dev> --- .../ABI/testing/sysfs-platform-asus-wmi | 11 +++- drivers/platform/x86/asus-wmi.c | 50 +++++++++++++++++++ 2 files changed, 60 insertions(+), 1 deletion(-)