| 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
| ||
|
Date: Tue, 28 Nov 2023 13:57:42 +1300
From: Luke Jones <luke@...nes.dev>
To: Hans de Goede <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 Mon, Nov 27 2023 at 11:39:09 PM +01:00:00, Hans de Goede
<hdegoede@...hat.com> wrote:
> Hi,
>
> On 11/27/23 21:17, Luke Jones wrote:
>>
>>
>> On Mon, Nov 27 2023 at 09:53:13 AM +01:00:00, Hans de Goede
>> <hdegoede@...hat.com> wrote:
>>> Hi,
>>>
>>> On 11/27/23 00: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.
>>>>
>>>> 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.
>>>>
>>>> Signed-off-by: Luke D. Jones <luke@...nes.dev>
>>>
>>> Thanks, patch looks good to me, except that all the new lines
>>> seem to use 4 spaces rather then a tab char as indent.
>>
>> Apologies for the previous HTML email.
>> I must be going mad... are you sure? I've checked the patch file I
>> submitted. Run checkpatch on it. Checked my email copy, and checked
>> in lore... I can't see where space chars are?
>
> So I just checked the copy in patchwork:
>
> https://patchwork.kernel.org/project/platform-driver-x86/patch/20231126230521.125708-2-luke@ljones.dev/
>
> and you are rights, no 4 spaces there.
>
> Where as if you look further down in this reply, where the original
> patch is quoted the 4 spaces are right there, so now I'm wondering
> if maybe my mail client introduced the problem when I was replying ?
>
> (replies to other patches preserve the tabs just fine).
>
> So this is weird, but lets just forget about it, just some weird
> glitch ...
>
Sun flares and cosmic particles.
I'm comfortable with this being merged and doing through stable also. I
did many many more tests this morning, along with a half dozen other
people and this solution appears to be the only reliable option. When
ASUS provide a way to turn off the MCU unplug feature I will update
with a new patch (and add the TODO).
>
>
>
>>
>>>
>>> With that fixed you can add my:
>>>
>>> Reviewed-by: Hans de Goede <hdegoede@...hat.com>
>>>
>>> to the next version.
>>>
>>> Regards,
>>>
>>> Hans
>>>
>>>
>>>> ---
>>>> 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,
>>>> };
>>>>
>>>> /* 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