| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <30293382-2287-45a2-9269-55d547432085@amd.com>
Date: Mon, 27 Nov 2023 14:14:23 -0600
From: Mario Limonciello <mario.limonciello@....com>
To: "Luke D. Jones" <luke@...nes.dev>, hdegoede@...hat.com
Cc: ilpo.jarvinen@...ux.intel.com, corentin.chary@...il.com,
platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG
Ally before suspend
On 11/26/2023 17:05, Luke D. Jones wrote:
> ASUS have worked around an issue in XInput where it doesn't support USB
> selective suspend, which causes suspend issues in Windows. They worked
> around this by adjusting the MCU firmware to disable the USB0 hub when
> the screen is switched off during the Microsoft DSM suspend path in ACPI.
>
> The issue we have with this however is one of timing - the call the tells
> the MCU to this isn't able to complete before suspend is done so we call
> this in a prepare() and add a small msleep() to ensure it is done. This
> must be done before the screen is switched off to prevent a variety of
> possible races.
Right now the way that Linux handles the LPS0 calls is that they're all
back to back. Luke did try to inject a delay after the LPS0 calls were
done but before it went to sleep but this wasn't sufficient.
Another "potential" way to solve this problem from Linux may be to
actually glue the LPS0 screen off call to when DRM actually has eDP
turned off.
Making such a change would essentially push back the "screen off" LPS0
command to when the user has run 'systemctl suspend' (or an action that
did this) because the compositor usually turns it off with DPMS at this
time.
This is a much bigger change though and *much more ripe for breakage*.
So I think in may be worth leaving a TODO comment to look into doing
that in the future.
If that ever happens; it's possible that this change could be reverted too.
>
> Further to this the MCU powersave option must also be disabled as it can
> cause a number of issues such as:
> - unreliable resume connection of N-Key
> - complete loss of N-Key if the power is plugged in while suspended
> Disabling the powersave option prevents this.
>
> Without this the MCU is unable to initialise itself correctly on resume.
initialize
>
> Signed-off-by: Luke D. Jones <luke@...nes.dev>
I think it would be good to add a Closes: tag to the AMD Gitlab issue
that this was discussed within as well as any other public references
you know about.
Additionally as Phoenix APU support goes back as far as kernel 6.1 and
this is well contained to only run on the ROG I suggest to CC stable so
that people can use the ROG on that LTS kernel or later.
> ---
> drivers/platform/x86/asus-wmi.c | 50 ++++++++++++++++++++++
> include/linux/platform_data/x86/asus-wmi.h | 3 ++
> 2 files changed, 53 insertions(+)
>
> diff --git a/drivers/platform/x86/asus-wmi.c b/drivers/platform/x86/asus-wmi.c
> index 6a79f16233ab..4ba33dfebfd4 100644
> --- a/drivers/platform/x86/asus-wmi.c
> +++ b/drivers/platform/x86/asus-wmi.c
> @@ -16,6 +16,7 @@
> #include <linux/acpi.h>
> #include <linux/backlight.h>
> #include <linux/debugfs.h>
> +#include <linux/delay.h>
> #include <linux/dmi.h>
> #include <linux/fb.h>
> #include <linux/hwmon.h>
> @@ -132,6 +133,11 @@ module_param(fnlock_default, bool, 0444);
> #define ASUS_SCREENPAD_BRIGHT_MAX 255
> #define ASUS_SCREENPAD_BRIGHT_DEFAULT 60
>
> +/* Controls the power state of the USB0 hub on ROG Ally which input is on */
> +#define ASUS_USB0_PWR_EC0_CSEE "\\_SB.PCI0.SBRG.EC0.CSEE"
> +/* 300ms so far seems to produce a reliable result on AC and battery */
> +#define ASUS_USB0_PWR_EC0_CSEE_WAIT 300
> +
> static const char * const ashs_ids[] = { "ATK4001", "ATK4002", NULL };
>
> static int throttle_thermal_policy_write(struct asus_wmi *);
> @@ -300,6 +306,9 @@ struct asus_wmi {
>
> bool fnlock_locked;
>
> + /* The ROG Ally device requires the MCU USB device be disconnected before suspend */
> + bool ally_mcu_usb_switch;
> +
> struct asus_wmi_debug debug;
>
> struct asus_wmi_driver *driver;
> @@ -4488,6 +4497,8 @@ static int asus_wmi_add(struct platform_device *pdev)
> asus->nv_temp_tgt_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_NV_THERM_TARGET);
> asus->panel_overdrive_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_PANEL_OD);
> asus->mini_led_mode_available = asus_wmi_dev_is_present(asus, ASUS_WMI_DEVID_MINI_LED_MODE);
> + asus->ally_mcu_usb_switch = acpi_has_method(NULL, ASUS_USB0_PWR_EC0_CSEE)
> + && dmi_match(DMI_BOARD_NAME, "RC71L");
>
> err = fan_boost_mode_check_present(asus);
> if (err)
> @@ -4654,6 +4665,43 @@ static int asus_hotk_resume(struct device *device)
> asus_wmi_fnlock_update(asus);
>
> asus_wmi_tablet_mode_get_state(asus);
> +
> + return 0;
> +}
> +
> +static int asus_hotk_resume_early(struct device *device)
> +{
> + struct asus_wmi *asus = dev_get_drvdata(device);
> +
> + if (asus->ally_mcu_usb_switch) {
> + if (ACPI_FAILURE(acpi_execute_simple_method(NULL, ASUS_USB0_PWR_EC0_CSEE, 0xB8)))
> + dev_err(device, "ROG Ally MCU failed to connect USB dev\n");
> + else
> + msleep(ASUS_USB0_PWR_EC0_CSEE_WAIT);
> + }
> + return 0;
> +}
> +
> +static int asus_hotk_prepare(struct device *device)
> +{
> + struct asus_wmi *asus = dev_get_drvdata(device);
> + int result, err;
> +
> + if (asus->ally_mcu_usb_switch) {
> + /* When powersave is enabled it causes many issues with resume of USB hub */
> + result = asus_wmi_get_devstate_simple(asus, ASUS_WMI_DEVID_MCU_POWERSAVE);
> + if (result == 1) {
> + dev_warn(device, "MCU powersave enabled, disabling to prevent resume issues");
> + err = asus_wmi_set_devstate(ASUS_WMI_DEVID_MCU_POWERSAVE, 0, &result);
> + if (err || result != 1)
> + dev_err(device, "Failed to set MCU powersave mode: %d\n", err);
> + }
> + /* sleep required to ensure USB0 is disabled before sleep continues */
> + if (ACPI_FAILURE(acpi_execute_simple_method(NULL, ASUS_USB0_PWR_EC0_CSEE, 0xB7)))
> + dev_err(device, "ROG Ally MCU failed to disconnect USB dev\n");
> + else
> + msleep(ASUS_USB0_PWR_EC0_CSEE_WAIT);
> + }
> return 0;
> }
>
> @@ -4701,6 +4749,8 @@ static const struct dev_pm_ops asus_pm_ops = {
> .thaw = asus_hotk_thaw,
> .restore = asus_hotk_restore,
> .resume = asus_hotk_resume,
> + .resume_early = asus_hotk_resume_early,
> + .prepare = asus_hotk_prepare,
Have you experimented with only using the prepare() call or only the
resume_early() call? Are both really needed?
> };
>
> /* Registration ***************************************************************/
> diff --git a/include/linux/platform_data/x86/asus-wmi.h b/include/linux/platform_data/x86/asus-wmi.h
> index 63e630276499..ab1c7deff118 100644
> --- a/include/linux/platform_data/x86/asus-wmi.h
> +++ b/include/linux/platform_data/x86/asus-wmi.h
> @@ -114,6 +114,9 @@
> /* Charging mode - 1=Barrel, 2=USB */
> #define ASUS_WMI_DEVID_CHARGE_MODE 0x0012006C
>
> +/* MCU powersave mode */
> +#define ASUS_WMI_DEVID_MCU_POWERSAVE 0x001200E2
> +
> /* epu is connected? 1 == true */
> #define ASUS_WMI_DEVID_EGPU_CONNECTED 0x00090018
> /* egpu on/off */
Powered by blists - more mailing lists