Re: [PATCH] platform/x86: bitland-mifs-wmi: Don't abort suspend when platform profile access fails

[Armin Wolf <W_Armin@gmx.de>]

Am 11.09.26 um 20:21 schrieb Chris Taraszka:

> On a Xiaomi Book Pro 14 (BIOS XMAPT4B0P0909), bitland_mifs_wmi_suspend()
> intermittently fails to read the platform profile and returns -EINVAL to
> the PM core, which aborts the entire system suspend:
>
>    bitland-mifs-wmi B60BFB48-...-4: PM: dpm_run_callback():
>      bitland_mifs_wmi_suspend [bitland_mifs_wmi] returns -22
>    bitland-mifs-wmi B60BFB48-...-4: PM: failed to suspend: error -22
>    PM: Some devices failed to suspend, or early wake event detected
>
> The WMI method call itself succeeds. The WMI core reports its own
> failures as -EIO, -ENOMSG, -ENODATA or -EPROTO, so the -EINVAL comes from
> laptop_profile_get() when the SystemPerMode value returned by the
> firmware is not one of the four documented modes.
>
> How often this happens depends on firmware state. On a v7.2.2 boot the
> query failed persistently on battery: /sys/power/suspend_stats reported
> 4628 failures against 2 successes, both of those on AC. Because logind
> re-issues the suspend while the lid stays closed, the machine looped
> awake and drained half the battery over one 8h42m lid-closed period at
> ~7-8 W. On v7.3-rc2, whose driver, WMI core and platform_profile code are
> unchanged from v7.2, the query has so far failed only right after boot,
> when power-profiles-daemon first reads the profile.
>
> Commit d3666875c75e ("platform/x86: bitland-mifs-wmi: Fix NULL pointer
> dereference during suspend/resume") added a !data->pp_dev guard, but
> that only covers the event device. On the control device pp_dev is
> valid, so the guard does not apply and the error is returned verbatim.
>
> Saving the platform profile is best-effort. Failing to read it should
> not keep the system awake, so warn and continue instead, in line with
> that commit skipping profile operations rather than failing the
> transition. Do the same on resume: laptop_profile_set() returns
> -EOPNOTSUPP for the performance and full-speed modes without DC power
> (see Documentation/wmi/devices/bitland-mifs-wmi.rst), so a profile saved
> on DC power cannot be restored after unplugging during suspend, which
> would otherwise mark the resume as failed.

Hi,

i think the root cause for this is that your device uses a different set of platform profiles.
Can you share the output of "acpidump"?

Thanks,
Armin Wolf

> Fixes: dc1ec4fa86b2 ("platform/x86: bitland-mifs-wmi: Add new Bitland MIFS WMI driver")
> Cc: stable@vger.kernel.org
> Signed-off-by: Chris Taraszka <chris@miget.com>
> ---
> Tested on the same machine with v7.3-rc2 plus this patch: 11 s2idle
> suspend cycles, including lid-closed suspends on battery of 7h42m, 4.9h
> and 75.8h, and repeated pm_test=devices cycles on battery all completed
> without errors. The failing firmware state could not be reproduced on
> demand on v7.3-rc2, so the new warnings have not been observed firing
> during a suspend; the failures described above were recorded on v7.2.2
> without this patch.
>
>   drivers/platform/x86/bitland-mifs-wmi.c | 17 +++++++++++++----
>   1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/platform/x86/bitland-mifs-wmi.c b/drivers/platform/x86/bitland-mifs-wmi.c
> index 3a37318..788ef14 100644
> --- a/drivers/platform/x86/bitland-mifs-wmi.c
> +++ b/drivers/platform/x86/bitland-mifs-wmi.c
> @@ -305,22 +305,31 @@ static int bitland_mifs_wmi_suspend(struct device *dev)
>   		return 0;
>   
>   	ret = laptop_profile_get(data->pp_dev, &profile);
> -	if (ret == 0)
> -		data->saved_profile = profile;
> +	if (ret) {
> +		dev_warn(dev, "Failed to save platform profile: %d\n", ret);
> +		return 0;
> +	}
>   
> -	return ret;
> +	data->saved_profile = profile;
> +
> +	return 0;
>   }
>   
>   static int bitland_mifs_wmi_resume(struct device *dev)
>   {
>   	struct bitland_mifs_wmi_data *data = dev_get_drvdata(dev);
> +	int ret;
>   
>   	/* Skip event device */
>   	if (!data->pp_dev)
>   		return 0;
>   
>   	dev_dbg(dev, "Resuming, restoring profile %d\n", data->saved_profile);
> -	return laptop_profile_set(dev, data->saved_profile);
> +	ret = laptop_profile_set(dev, data->saved_profile);
> +	if (ret)
> +		dev_warn(dev, "Failed to restore platform profile: %d\n", ret);
> +
> +	return 0;
>   }
>   
>   static DEFINE_SIMPLE_DEV_PM_OPS(bitland_mifs_wmi_pm_ops,