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

[Ilpo Järvinen]

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@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.

Yes, I think it is okay to make it configurable first and then look 
separately into making it default on.

-- 
 i.