On Thu, 21 Nov 2024 at 19:04, Mario Limonciello
<mario.limonciello@xxxxxxx> wrote:
>
> On 11/21/2024 11:22, Antheas Kapenekakis wrote:
> > Unfortunately, some modern standby systems, including the ROG Ally, rely
> > on a delay between modern standby transitions. Add a quirk table for
> > introducing delays between modern standby transitions, and quirk the
> > ROG Ally on "Display Off", which needs a bit of time to turn off its
> > controllers prior to suspending.
> >
> > Signed-off-by: Antheas Kapenekakis <lkml@xxxxxxxxxxx>
> > ---
> > drivers/acpi/x86/s2idle.c | 56 +++++++++++++++++++++++++++++++++++++++
> > 1 file changed, 56 insertions(+)
> >
> > diff --git a/drivers/acpi/x86/s2idle.c b/drivers/acpi/x86/s2idle.c
> > index d389c57d2963..504e6575d7ad 100644
> > --- a/drivers/acpi/x86/s2idle.c
> > +++ b/drivers/acpi/x86/s2idle.c
> > @@ -18,6 +18,7 @@
> > #include <linux/acpi.h>
> > #include <linux/device.h>
> > #include <linux/dmi.h>
> > +#include <linux/delay.h>
> > #include <linux/suspend.h>
> >
> > #include "../sleep.h"
> > @@ -91,11 +92,50 @@ struct lpi_device_constraint_amd {
> > int min_dstate;
> > };
> >
> > +struct s2idle_delay_quirks {
> > + int delay_display_off;
> > + int delay_sleep_entry;
> > + int delay_sleep_exit;
> > + int delay_display_on;
> > +};
>
> Historically these "kinds" of quirks are kept in drivers/acpi/x86/utils.c.
>
> Could it be moved there? Or perhaps stored in the ASUS drivers and
> callbacks?
Yes, it could definitely be moved there.
> This feels cleaner if you used "struct acpi_s2idle_dev_ops" and
> callbacks. More below.
I can convert the quirk into 4 callbacks and put it there if it is
better. But note that the exit delays are added before the firmware
call and the entry delays are added after. I guess this makes sense as
a callback form as well.
> > +
> > +/*
> > + * The ROG Ally series disconnects its controllers on Display Off and performs
> > + * a fancy shutdown sequence, which requires around half a second to complete.
> > + * If the power is cut earlier by entering it into D3, the original Ally unit
> > + * might not disconnect its XInput MCU, causing excess battery drain, and the
> > + * Ally X will make the controller restart post-suspend. In addition, the EC
> > + * of the device rarely (1/20 attempts) may get stuck asserting PROCHOT after
> > + * suspend (for various reasons), so split the delay between Display Off and
> > + * Sleep Entry.
> > + */
> > +static const struct s2idle_delay_quirks rog_ally_quirks = {
> > + .delay_display_off = 350,
> > + .delay_sleep_entry = 150,
> > +};
>
> Is this delay still needed with Ally MCU 319 that has the fixes from ASUS?
>
> I'm suspecting not, which means this quirk should be made more narrow IMO.
Yes, it is definitely needed. I have had at least two users break the
new firmware when removing the whole quirk. The controller shuts down
uncleanly and performs a restart/can break its RGB after suspend. The
new firmware was mostly tested with the old quirk in place.
I have not tried removing the quirk myself on the new firmware.
Perhaps I should.
> In the various ASUS drivers you can lookup the MCU firmware version.
> Those drivers can do acpi_register_lps0_dev() when the older firmware is
> present and use the callbacks. If the newer firmware is there less code
> to worry about.
>
> This also would mean less static quirk tables in the kernel tree.
I would prefer to avoid bringing in other drivers in this or depending
on their functionality, as in this case it is also not needed. I guess
as a callback form this can be somewhat flexible.
> > +
> > +static const struct dmi_system_id s2idle_delay_quirks[] = {
> > + {
> > + .matches = {
> > + DMI_MATCH(DMI_BOARD_NAME, "RC71L"),
> > + },
> > + .driver_data = (void *)&rog_ally_quirks
> > + },
> > + {
> > + .matches = {
> > + DMI_MATCH(DMI_BOARD_NAME, "RC72L"),
> > + },
> > + .driver_data = (void *)&rog_ally_quirks
> > + },
> > + {}
> > +};
> > +
> > static LIST_HEAD(lps0_s2idle_devops_head);
> >
> > static struct lpi_constraints *lpi_constraints_table;
> > static int lpi_constraints_table_size;
> > static int rev_id;
> > +struct s2idle_delay_quirks *delay_quirks;
> >
> > #define for_each_lpi_constraint(entry) \
> > for (int i = 0; \
> > @@ -566,6 +606,9 @@ static int acpi_s2idle_display_off(void)
> > acpi_sleep_run_lps0_dsm(ACPI_LPS0_DISPLAY_OFF,
> > lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
> >
> > + if (delay_quirks && delay_quirks->delay_display_off)
> > + msleep(delay_quirks->delay_display_off);
> > +
> > acpi_scan_lock_release();
> >
> > return 0;
> > @@ -587,6 +630,9 @@ static int acpi_s2idle_sleep_entry(void)
> > acpi_sleep_run_lps0_dsm(ACPI_LPS0_SLEEP_ENTRY,
> > lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
> >
> > + if (delay_quirks && delay_quirks->delay_sleep_entry)
> > + msleep(delay_quirks->delay_sleep_entry);
> > +
> > acpi_scan_lock_release();
> >
> > return 0;
> > @@ -627,6 +673,9 @@ static int acpi_s2idle_sleep_exit(void)
> > acpi_scan_lock_acquire();
> >
> > /* Modern Standby Sleep Exit */
> > + if (delay_quirks && delay_quirks->delay_sleep_exit)
> > + msleep(delay_quirks->delay_sleep_exit);
> > +
> > if (lps0_dsm_func_mask_microsoft > 0)
> > acpi_sleep_run_lps0_dsm(ACPI_LPS0_SLEEP_EXIT,
> > lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
> > @@ -648,6 +697,9 @@ static int acpi_s2idle_display_on(void)
> > acpi_scan_lock_acquire();
> >
> > /* Display on */
> > + if (delay_quirks && delay_quirks->delay_display_on)
> > + msleep(delay_quirks->delay_display_on);
> > +
> > if (lps0_dsm_func_mask_microsoft > 0)
> > acpi_sleep_run_lps0_dsm(ACPI_LPS0_DISPLAY_ON,
> > lps0_dsm_func_mask_microsoft, lps0_dsm_guid_microsoft);
> > @@ -760,6 +812,10 @@ int acpi_register_lps0_dev(struct acpi_s2idle_dev_ops *arg)
> >
> > sleep_flags = lock_system_sleep();
> > list_add(&arg->list_node, &lps0_s2idle_devops_head);
> > + const struct dmi_system_id *s2idle_sysid = dmi_first_match(
> > + s2idle_delay_quirks
> > + );
> > + delay_quirks = s2idle_sysid ? s2idle_sysid->driver_data : NULL;
> > unlock_system_sleep(sleep_flags);
> >
> > return 0;
>
