| Message ID | 20240922172258.48435-4-lkml@antheas.dev (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | acpi/x86: s2idle: move Display off/on calls outside suspend (fixes ROG Ally suspend) | expand |
On 22/09/24 19: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 (i.e., entering DRIPS). > > Reported-by: Denis Benato <benato.denis96@gmail.com> I told you privately that as stated here: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes Reported-By tags are to be followed by a Closes tag stating that the issue is solved. What has been presented to me today was not about solving bugs, but changing how they manifests and therefore permission was not granted to you to represent me as satisfied with the work as it would be wrong to assume issues are solved and this is worth merging. Furthermore everybody can see my answers to your v1 patch and there is no need to attach a Reported-by when the issue has been reported publicly and is not resolved. I will add my Tested-off-by by myself whenever I will be fully satisfied by work presented: in the current state I am not. > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > --- > include/linux/suspend.h | 5 +++++ > kernel/power/suspend.c | 41 +++++++++++++++++++++++++++++++++++++++++ > 2 files changed, 46 insertions(+) > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h > index 8f33249cc067..d7e2a4d8ab0c 100644 > --- a/include/linux/suspend.h > +++ b/include/linux/suspend.h > @@ -144,6 +144,11 @@ struct platform_s2idle_ops { > int (*display_on)(void); > }; > > +struct platform_s2idle_quirks { > + int delay_display_off; > + int delay_display_on; > +}; > + > #ifdef CONFIG_SUSPEND > extern suspend_state_t pm_suspend_target_state; > extern suspend_state_t mem_sleep_current; > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c > index 610f8ecaeebd..af2abdd2f8c3 100644 > --- a/kernel/power/suspend.c > +++ b/kernel/power/suspend.c > @@ -11,6 +11,7 @@ > > #include <linux/string.h> > #include <linux/delay.h> > +#include <linux/dmi.h> > #include <linux/errno.h> > #include <linux/init.h> > #include <linux/console.h> > @@ -61,6 +62,30 @@ static DECLARE_SWAIT_QUEUE_HEAD(s2idle_wait_head); > enum s2idle_states __read_mostly s2idle_state; > static DEFINE_RAW_SPINLOCK(s2idle_lock); > > +// The ROG Ally series disconnects its controllers on Display Off, without > +// holding a lock, introducing a race condition. Add a delay to allow the > +// controller to disconnect cleanly prior to suspend. > +static const struct platform_s2idle_quirks rog_ally_quirks = { > + .delay_display_off = 500, > +}; > + > +static const struct dmi_system_id platform_s2idle_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 > + }, > + {} > +}; > + > + > /** > * pm_suspend_default_s2idle - Check if suspend-to-idle is the default suspend. > * > @@ -589,12 +614,26 @@ static int enter_state(suspend_state_t state) > if (state == PM_SUSPEND_TO_IDLE) > s2idle_begin(); > > + /* > + * Windows transitions between Modern Standby states slowly, over multiple > + * seconds. Certain manufacturers may rely on this, introducing race > + * conditions. Until Linux can support modern standby, add the relevant > + * delays between transitions here. > + */ > + const struct dmi_system_id *s2idle_sysid = dmi_first_match( > + platform_s2idle_quirks > + ); > + const struct platform_s2idle_quirks *s2idle_quirks = s2idle_sysid ? > + s2idle_sysid->driver_data : NULL; > + > /* > * Linux does not have the concept of a "Screen Off" state, so call > * the platform functions for Display On/Off prior to the suspend > * sequence, mirroring Windows which calls them outside of it as well. > */ > platform_suspend_display_off(); > + if (s2idle_quirks && s2idle_quirks->delay_display_off) > + msleep(s2idle_quirks->delay_display_off); > > if (sync_on_suspend_enabled) { > trace_suspend_resume(TPS("sync_filesystems"), 0, true); > @@ -624,6 +663,8 @@ static int enter_state(suspend_state_t state) > Unlock: > mutex_unlock(&system_transition_mutex); > > + if (s2idle_quirks && s2idle_quirks->delay_display_on) > + msleep(s2idle_quirks->delay_display_on); > platform_suspend_display_on(); > return error; > }
Hi Denis, I did not add a Tested-by for this reason. The Report-by is to credit you for bringing this into my attention, I thought you were ok with this. If not, I will remove it in the next revision. Best, Antheas On Sun, 22 Sept 2024 at 19:55, Denis Benato <benato.denis96@gmail.com> wrote: > > On 22/09/24 19: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 (i.e., entering DRIPS). > > > > Reported-by: Denis Benato <benato.denis96@gmail.com> > > I told you privately that as stated here: https://www.kernel.org/doc/html/latest/process/submitting-patches.html#using-reported-by-tested-by-reviewed-by-suggested-by-and-fixes > Reported-By tags are to be followed by a Closes tag stating that the issue is solved. > > What has been presented to me today was not about solving bugs, but changing how they manifests and therefore permission was not granted to you to represent me as satisfied with the work as it would be wrong to assume issues are solved and this is worth merging. > > Furthermore everybody can see my answers to your v1 patch and there is no need to attach a Reported-by when the issue has been reported publicly and is not resolved. > > I will add my Tested-off-by by myself whenever I will be fully satisfied by work presented: in the current state I am not. > > > Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> > > --- > > include/linux/suspend.h | 5 +++++ > > kernel/power/suspend.c | 41 +++++++++++++++++++++++++++++++++++++++++ > > 2 files changed, 46 insertions(+) > > > > diff --git a/include/linux/suspend.h b/include/linux/suspend.h > > index 8f33249cc067..d7e2a4d8ab0c 100644 > > --- a/include/linux/suspend.h > > +++ b/include/linux/suspend.h > > @@ -144,6 +144,11 @@ struct platform_s2idle_ops { > > int (*display_on)(void); > > }; > > > > +struct platform_s2idle_quirks { > > + int delay_display_off; > > + int delay_display_on; > > +}; > > + > > #ifdef CONFIG_SUSPEND > > extern suspend_state_t pm_suspend_target_state; > > extern suspend_state_t mem_sleep_current; > > diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c > > index 610f8ecaeebd..af2abdd2f8c3 100644 > > --- a/kernel/power/suspend.c > > +++ b/kernel/power/suspend.c > > @@ -11,6 +11,7 @@ > > > > #include <linux/string.h> > > #include <linux/delay.h> > > +#include <linux/dmi.h> > > #include <linux/errno.h> > > #include <linux/init.h> > > #include <linux/console.h> > > @@ -61,6 +62,30 @@ static DECLARE_SWAIT_QUEUE_HEAD(s2idle_wait_head); > > enum s2idle_states __read_mostly s2idle_state; > > static DEFINE_RAW_SPINLOCK(s2idle_lock); > > > > +// The ROG Ally series disconnects its controllers on Display Off, without > > +// holding a lock, introducing a race condition. Add a delay to allow the > > +// controller to disconnect cleanly prior to suspend. > > +static const struct platform_s2idle_quirks rog_ally_quirks = { > > + .delay_display_off = 500, > > +}; > > + > > +static const struct dmi_system_id platform_s2idle_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 > > + }, > > + {} > > +}; > > + > > + > > /** > > * pm_suspend_default_s2idle - Check if suspend-to-idle is the default suspend. > > * > > @@ -589,12 +614,26 @@ static int enter_state(suspend_state_t state) > > if (state == PM_SUSPEND_TO_IDLE) > > s2idle_begin(); > > > > + /* > > + * Windows transitions between Modern Standby states slowly, over multiple > > + * seconds. Certain manufacturers may rely on this, introducing race > > + * conditions. Until Linux can support modern standby, add the relevant > > + * delays between transitions here. > > + */ > > + const struct dmi_system_id *s2idle_sysid = dmi_first_match( > > + platform_s2idle_quirks > > + ); > > + const struct platform_s2idle_quirks *s2idle_quirks = s2idle_sysid ? > > + s2idle_sysid->driver_data : NULL; > > + > > /* > > * Linux does not have the concept of a "Screen Off" state, so call > > * the platform functions for Display On/Off prior to the suspend > > * sequence, mirroring Windows which calls them outside of it as well. > > */ > > platform_suspend_display_off(); > > + if (s2idle_quirks && s2idle_quirks->delay_display_off) > > + msleep(s2idle_quirks->delay_display_off); > > > > if (sync_on_suspend_enabled) { > > trace_suspend_resume(TPS("sync_filesystems"), 0, true); > > @@ -624,6 +663,8 @@ static int enter_state(suspend_state_t state) > > Unlock: > > mutex_unlock(&system_transition_mutex); > > > > + if (s2idle_quirks && s2idle_quirks->delay_display_on) > > + msleep(s2idle_quirks->delay_display_on); > > platform_suspend_display_on(); > > return error; > > } >
diff --git a/include/linux/suspend.h b/include/linux/suspend.h index 8f33249cc067..d7e2a4d8ab0c 100644 --- a/include/linux/suspend.h +++ b/include/linux/suspend.h @@ -144,6 +144,11 @@ struct platform_s2idle_ops { int (*display_on)(void); }; +struct platform_s2idle_quirks { + int delay_display_off; + int delay_display_on; +}; + #ifdef CONFIG_SUSPEND extern suspend_state_t pm_suspend_target_state; extern suspend_state_t mem_sleep_current; diff --git a/kernel/power/suspend.c b/kernel/power/suspend.c index 610f8ecaeebd..af2abdd2f8c3 100644 --- a/kernel/power/suspend.c +++ b/kernel/power/suspend.c @@ -11,6 +11,7 @@ #include <linux/string.h> #include <linux/delay.h> +#include <linux/dmi.h> #include <linux/errno.h> #include <linux/init.h> #include <linux/console.h> @@ -61,6 +62,30 @@ static DECLARE_SWAIT_QUEUE_HEAD(s2idle_wait_head); enum s2idle_states __read_mostly s2idle_state; static DEFINE_RAW_SPINLOCK(s2idle_lock); +// The ROG Ally series disconnects its controllers on Display Off, without +// holding a lock, introducing a race condition. Add a delay to allow the +// controller to disconnect cleanly prior to suspend. +static const struct platform_s2idle_quirks rog_ally_quirks = { + .delay_display_off = 500, +}; + +static const struct dmi_system_id platform_s2idle_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 + }, + {} +}; + + /** * pm_suspend_default_s2idle - Check if suspend-to-idle is the default suspend. * @@ -589,12 +614,26 @@ static int enter_state(suspend_state_t state) if (state == PM_SUSPEND_TO_IDLE) s2idle_begin(); + /* + * Windows transitions between Modern Standby states slowly, over multiple + * seconds. Certain manufacturers may rely on this, introducing race + * conditions. Until Linux can support modern standby, add the relevant + * delays between transitions here. + */ + const struct dmi_system_id *s2idle_sysid = dmi_first_match( + platform_s2idle_quirks + ); + const struct platform_s2idle_quirks *s2idle_quirks = s2idle_sysid ? + s2idle_sysid->driver_data : NULL; + /* * Linux does not have the concept of a "Screen Off" state, so call * the platform functions for Display On/Off prior to the suspend * sequence, mirroring Windows which calls them outside of it as well. */ platform_suspend_display_off(); + if (s2idle_quirks && s2idle_quirks->delay_display_off) + msleep(s2idle_quirks->delay_display_off); if (sync_on_suspend_enabled) { trace_suspend_resume(TPS("sync_filesystems"), 0, true); @@ -624,6 +663,8 @@ static int enter_state(suspend_state_t state) Unlock: mutex_unlock(&system_transition_mutex); + if (s2idle_quirks && s2idle_quirks->delay_display_on) + msleep(s2idle_quirks->delay_display_on); platform_suspend_display_on(); return error; }
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 (i.e., entering DRIPS). Reported-by: Denis Benato <benato.denis96@gmail.com> Signed-off-by: Antheas Kapenekakis <lkml@antheas.dev> --- include/linux/suspend.h | 5 +++++ kernel/power/suspend.c | 41 +++++++++++++++++++++++++++++++++++++++++ 2 files changed, 46 insertions(+)