Re: [PATCH v2 8/9] platform/x86: asus-wmi: Add support for MCU powersave

[Luke Jones]

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@xxxxxxxxxx>
> 
> 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@xxxxxxxxxx>
> > 
> >  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@xxxxxxxxxx>
> > +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)