Re: [PATCH v2 1/1] platform/x86: asus-wmi: disable USB0 hub on ROG Ally before suspend

[Luke Jones <luke@xxxxxxxxxx>]

On Mon, Nov 27 2023 at 11:39:09 PM +01:00:00, Hans de Goede 
<hdegoede@xxxxxxxxxx> 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@xxxxxxxxxx> 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@xxxxxxxxxx>
> > >
> > >  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@xxxxxxxxxx/
>
> 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@xxxxxxxxxx>
> > >
> > >  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 */