| Message ID | 1404802679-24019-2-git-send-email-tianyu.lan@intel.com (mailing list archive) |
|---|---|
| State | RFC, archived |
| Headers | show |
On Tuesday, July 08, 2014 02:57:58 PM Lan Tianyu wrote: > The button wakeup GPEs are enabled unconditionally in the current world by > commit 2a5d24(ACPI / Wakeup: Enable button GPEs unconditionally during > initialization). Because button's GPE methods needs to be run to clear > GPE status on some machines when there is GPE interrupt. If not, it will > cause machines resume immediately after being suspended since GPE status > isn't cleared correctly. > > But if there is no GPE method for button wakeup GPE, these GPEs should not > be enabled since nothing needs to be done when they are triggered and this > also causes LID GPE storm on Lenovo Ideapad y560p. > > This patch is to check Button GPE method and enable it if there is associated > GPE method. Part of the problem is that we call acpi_setup_gpe_for_wake() for buttons too and it sets ACPI_GPE_DISPATCH_NOTIFY unconditionally. While we could address this the way you're proposing, it seems a bit less than straightforward so to speak. Let me think more about how to address that. > Reference: https://bugzilla.kernel.org/show_bug.cgi?id=61051 > Reported-by: James Tocknell<aragilar@gmail.com> > Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> > --- > drivers/acpi/wakeup.c | 11 ++++++++--- > 1 file changed, 8 insertions(+), 3 deletions(-) > > diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c > index 1638401..5b20ae4 100644 > --- a/drivers/acpi/wakeup.c > +++ b/drivers/acpi/wakeup.c > @@ -86,9 +86,14 @@ int __init acpi_wakeup_device_init(void) > struct acpi_device, > wakeup_list); > if (device_can_wakeup(&dev->dev)) { > - /* Button GPEs are supposed to be always enabled. */ > - acpi_enable_gpe(dev->wakeup.gpe_device, > - dev->wakeup.gpe_number); > + /* > + * Button GPEs are supposed to be always enabled if > + * they have associated GPE methods. > + */ > + if (ACPI_SUCCESS(acpi_check_gpe_method( > + dev->wakeup.gpe_device, dev->wakeup.gpe_number))) > + acpi_enable_gpe(dev->wakeup.gpe_device, > + dev->wakeup.gpe_number); > device_set_wakeup_enable(&dev->dev, true); > } > } >
On 2014?07?08? 21:26, Rafael J. Wysocki wrote: > On Tuesday, July 08, 2014 02:57:58 PM Lan Tianyu wrote: >> The button wakeup GPEs are enabled unconditionally in the current world by >> commit 2a5d24(ACPI / Wakeup: Enable button GPEs unconditionally during >> initialization). Because button's GPE methods needs to be run to clear >> GPE status on some machines when there is GPE interrupt. If not, it will >> cause machines resume immediately after being suspended since GPE status >> isn't cleared correctly. >> >> But if there is no GPE method for button wakeup GPE, these GPEs should not >> be enabled since nothing needs to be done when they are triggered and this >> also causes LID GPE storm on Lenovo Ideapad y560p. >> >> This patch is to check Button GPE method and enable it if there is associated >> GPE method. > > Part of the problem is that we call acpi_setup_gpe_for_wake() for buttons too > and it sets ACPI_GPE_DISPATCH_NOTIFY unconditionally. Yes, this is why the GPE wasn't disabled by CA. From my opinion, original reason of enabling button wakeup GPE in the runtime is that it may have associated GPE method and the method will notify button driver and clear some hardware status. But if it didn't have GPE method, I don't why we need to enable the GPE. I check some machines and they use EC Query event to notify button driver. > While we could address > this the way you're proposing, it seems a bit less than straightforward so to > speak. > > Let me think more about how to address that. Ok. I get it. > >> Reference: https://bugzilla.kernel.org/show_bug.cgi?id=61051 >> Reported-by: James Tocknell<aragilar@gmail.com> >> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> >> --- >> drivers/acpi/wakeup.c | 11 ++++++++--- >> 1 file changed, 8 insertions(+), 3 deletions(-) >> >> diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c >> index 1638401..5b20ae4 100644 >> --- a/drivers/acpi/wakeup.c >> +++ b/drivers/acpi/wakeup.c >> @@ -86,9 +86,14 @@ int __init acpi_wakeup_device_init(void) >> struct acpi_device, >> wakeup_list); >> if (device_can_wakeup(&dev->dev)) { >> - /* Button GPEs are supposed to be always enabled. */ >> - acpi_enable_gpe(dev->wakeup.gpe_device, >> - dev->wakeup.gpe_number); >> + /* >> + * Button GPEs are supposed to be always enabled if >> + * they have associated GPE methods. >> + */ >> + if (ACPI_SUCCESS(acpi_check_gpe_method( >> + dev->wakeup.gpe_device, dev->wakeup.gpe_number))) >> + acpi_enable_gpe(dev->wakeup.gpe_device, >> + dev->wakeup.gpe_number); >> device_set_wakeup_enable(&dev->dev, true); >> } >> } >> >
On Wednesday, July 09, 2014 11:08:42 AM Lan Tianyu wrote: > On 2014?07?08? 21:26, Rafael J. Wysocki wrote: > > On Tuesday, July 08, 2014 02:57:58 PM Lan Tianyu wrote: > >> The button wakeup GPEs are enabled unconditionally in the current world by > >> commit 2a5d24(ACPI / Wakeup: Enable button GPEs unconditionally during > >> initialization). Because button's GPE methods needs to be run to clear > >> GPE status on some machines when there is GPE interrupt. If not, it will > >> cause machines resume immediately after being suspended since GPE status > >> isn't cleared correctly. > >> > >> But if there is no GPE method for button wakeup GPE, these GPEs should not > >> be enabled since nothing needs to be done when they are triggered and this > >> also causes LID GPE storm on Lenovo Ideapad y560p. > >> > >> This patch is to check Button GPE method and enable it if there is associated > >> GPE method. > > > > Part of the problem is that we call acpi_setup_gpe_for_wake() for buttons too > > and it sets ACPI_GPE_DISPATCH_NOTIFY unconditionally. > > Yes, this is why the GPE wasn't disabled by CA. From my opinion, > original reason of enabling button wakeup GPE in the runtime is that it > may have associated GPE method and the method will notify button driver > and clear some hardware status. But if it didn't have GPE method, I > don't why we need to enable the GPE. I think you're right, but "is there a handler method for that GPE present?" is a bit too low-level question to ask from outside of ACPICA. We should be asking "is there any kind of handling registered for it?" if anything, but in my opinion the cleanest approach would be to modify acpi_enable_gpe() to return an error code if there's no handler/method/notify for the GPE in question. The patch I attached to the BZ entry doesn't do that just yet, because I'm wondering whether or not it will make the ACPICA's auto-disabling trigger. :-) Rafael -- To unsubscribe from this list: send the line "unsubscribe linux-acpi" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/acpi/wakeup.c b/drivers/acpi/wakeup.c index 1638401..5b20ae4 100644 --- a/drivers/acpi/wakeup.c +++ b/drivers/acpi/wakeup.c @@ -86,9 +86,14 @@ int __init acpi_wakeup_device_init(void) struct acpi_device, wakeup_list); if (device_can_wakeup(&dev->dev)) { - /* Button GPEs are supposed to be always enabled. */ - acpi_enable_gpe(dev->wakeup.gpe_device, - dev->wakeup.gpe_number); + /* + * Button GPEs are supposed to be always enabled if + * they have associated GPE methods. + */ + if (ACPI_SUCCESS(acpi_check_gpe_method( + dev->wakeup.gpe_device, dev->wakeup.gpe_number))) + acpi_enable_gpe(dev->wakeup.gpe_device, + dev->wakeup.gpe_number); device_set_wakeup_enable(&dev->dev, true); } }
The button wakeup GPEs are enabled unconditionally in the current world by commit 2a5d24(ACPI / Wakeup: Enable button GPEs unconditionally during initialization). Because button's GPE methods needs to be run to clear GPE status on some machines when there is GPE interrupt. If not, it will cause machines resume immediately after being suspended since GPE status isn't cleared correctly. But if there is no GPE method for button wakeup GPE, these GPEs should not be enabled since nothing needs to be done when they are triggered and this also causes LID GPE storm on Lenovo Ideapad y560p. This patch is to check Button GPE method and enable it if there is associated GPE method. Reference: https://bugzilla.kernel.org/show_bug.cgi?id=61051 Reported-by: James Tocknell<aragilar@gmail.com> Signed-off-by: Lan Tianyu <tianyu.lan@intel.com> --- drivers/acpi/wakeup.c | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-)