Messages in this thread | | | From | "Rafael J. Wysocki" <> | Subject | Re: [PATCH][RFC] ACPI / PM: Fix incorrect wakeup irq setting before suspend-to-idle | Date | Fri, 25 Sep 2015 03:23:48 +0200 |
| |
On Monday, August 10, 2015 10:11:26 AM Chen Yu wrote: > For ACPI compatible system, SCI(ACPI System Control > Interrupt) is used to wake system up from suspend-to-idle. > Once CPU is woken up by SCI, interrupt handler will > firstly checks if current interrupt is legal to wake up > the whole system, thus irq_pm_check_wakeup is invoked > to validate the irq number. However, before suspend-to-idle, > acpi_gbl_FADT.sci_interrupt is marked rather than actual > irq number in acpi_freeze_prepare, this might lead to unable > to wake up the system. > > This patch fixes this problem by marking the irq number > return by acpi_gsi_to_irq as IRQD_WAKEUP_STATE, rather than > marking the acpi_gbl_FADT.sci_interrupt. > > Signed-off-by: Chen Yu <yu.c.chen@intel.com>
That would only really matter if GPE devices were used, but I've never seen a system using them in practice, so this is more of a theoretical issue.
> --- > drivers/acpi/osl.c | 5 ++++- > drivers/acpi/sleep.c | 20 ++++++++++++++++++-- > drivers/acpi/sleep.h | 5 +++++ > 3 files changed, 27 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/osl.c b/drivers/acpi/osl.c > index 3b8963f..8e1420a 100644 > --- a/drivers/acpi/osl.c > +++ b/drivers/acpi/osl.c > @@ -49,6 +49,7 @@ > #include <asm/uaccess.h> > > #include "internal.h" > +#include "sleep.h" > > #define _COMPONENT ACPI_OS_SERVICES > ACPI_MODULE_NAME("osl"); > @@ -850,7 +851,9 @@ acpi_os_install_interrupt_handler(u32 gsi, acpi_osd_handler handler, > gsi); > return AE_OK; > } > - > +#ifdef CONFIG_SUSPEND > + set_wake_irq_freeze(irq); > +#endif
Please don't use #ifdefs in function bodies. You can use IS_ENABLED() for that.
> acpi_irq_handler = handler; > acpi_irq_context = context; > if (request_irq(irq, acpi_irq, IRQF_SHARED, "acpi", acpi_irq)) { > diff --git a/drivers/acpi/sleep.c b/drivers/acpi/sleep.c > index 2f0d4db..9e7b54e 100644 > --- a/drivers/acpi/sleep.c > +++ b/drivers/acpi/sleep.c > @@ -620,6 +620,22 @@ static const struct platform_suspend_ops acpi_suspend_ops_old = { > .end = acpi_pm_end, > .recover = acpi_pm_finish, > }; > +static int wake_irq_freeze = -EINVAL;
There may be more than one of these in theory.
> + > +int get_wake_irq_freeze(void) > +{ > + if (IS_ERR_VALUE(wake_irq_freeze)) > + return acpi_gbl_FADT.sci_interrupt; > + else > + return wake_irq_freeze;
That would look better this way IMO:
return IS_ERR_VALUE(wake_irq_freeze) ? acpi_gbl_FADT.sci_interrupt : wake_irq_freeze;
> +} > +EXPORT_SYMBOL_GPL(get_wake_irq_freeze); > + > +void set_wake_irq_freeze(unsigned int irq) > +{ > + wake_irq_freeze = (int)irq; > +} > +EXPORT_SYMBOL_GPL(set_wake_irq_freeze); > > static int acpi_freeze_begin(void) > { > @@ -632,14 +648,14 @@ static int acpi_freeze_prepare(void) > acpi_enable_wakeup_devices(ACPI_STATE_S0); > acpi_enable_all_wakeup_gpes(); > acpi_os_wait_events_complete(); > - enable_irq_wake(acpi_gbl_FADT.sci_interrupt); > + enable_irq_wake(get_wake_irq_freeze()); > return 0; > } > > static void acpi_freeze_restore(void) > { > acpi_disable_wakeup_devices(ACPI_STATE_S0); > - disable_irq_wake(acpi_gbl_FADT.sci_interrupt); > + disable_irq_wake(get_wake_irq_freeze()); > acpi_enable_all_runtime_gpes(); > } > > diff --git a/drivers/acpi/sleep.h b/drivers/acpi/sleep.h > index c797ffa..eca4fda 100644 > --- a/drivers/acpi/sleep.h > +++ b/drivers/acpi/sleep.h > @@ -6,3 +6,8 @@ extern struct list_head acpi_wakeup_device_list; > extern struct mutex acpi_device_lock; > > extern void acpi_resume_power_resources(void); > + > +#ifdef CONFIG_SUSPEND > +extern int get_wake_irq_freeze(void); > +extern void set_wake_irq_freeze(unsigned int irq); > +#endif
Is the #ifdef needed here at all?
Thanks, Rafael
| |