| Message ID | 20210512125523.55215-2-hdegoede@redhat.com (mailing list archive) |
|---|---|
| State | Accepted, archived |
| Headers | show |
| Series | platform/x86: intel_int0002_vgpio: Only call enable_irq_wake() when using s2idle | expand |
On Wed, May 12, 2021 at 3:55 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Commit 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement > irq_set_wake on Bay Trail") stopped passing irq_set_wake requests on to > the parents IRQ because this was breaking suspend (causing immediate > wakeups) on an Asus E202SA. > > This workaround for the Asus E202SA is causing wakeup by USB keyboard to > not work on other devices with Airmont CPU cores such as the Medion Akoya > E1239T. In hindsight the problem with the Asus E202SA has nothing to do > with Silvermont vs Airmont CPU cores, so the differentiation between the > 2 types of CPU cores introduced by the previous fix is wrong. > > The real issue at hand is s2idle vs S3 suspend where the suspend is > mostly handled by firmware. The parent IRQ for the INT0002 device is shared > with the ACPI SCI and the real problem is that the INT0002 code should not > be messing with the wakeup settings of that IRQ when suspend/resume is > being handled by the firmware. > > Note that on systems which support both s2idle and S3 suspend, which > suspend method to use can be changed at runtime. > > This patch fixes both the Asus E202SA spurious wakeups issue as well as > the wakeup by USB keyboard not working on the Medion Akoya E1239T issue. > > These are both fixed by replacing the old workaround with delaying the > enable_irq_wake(parent_irq) call till system-suspend time and protecting > it with a !pm_suspend_via_firmware() check so that we still do not call > it on devices using firmware-based (S3) suspend such as the Asus E202SA. > Note rather then adding #ifdef CONFIG_PM_SLEEP, this commit simply adds > a "depends on PM_SLEEP" to the Kconfig since this drivers whole purpose > is to deal with wakeup events, so using it without CONFIG_PM_SLEEP makes > no sense. I like the new approach. One remark (or two :) is to the PM SLEEP thingy. Can we add a separate line for "depends on", so it will be easier to maintain in case we need to amend it somehow? Another one is to amend a helpline to reflect that the driver is dealing solely with wake events (I haven't checked the current text, so it might be already enforced). Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> > Cc: Maxim Mikityanskiy <maxtram95@gmail.com> > Fixes: 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement irq_set_wake on Bay Trail") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> > --- > drivers/platform/x86/Kconfig | 2 +- > drivers/platform/x86/intel_int0002_vgpio.c | 80 +++++++++++++++------- > 2 files changed, 57 insertions(+), 25 deletions(-) > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 4b67e74a747b..c2f608d5f1b7 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -713,7 +713,7 @@ config INTEL_HID_EVENT > > config INTEL_INT0002_VGPIO > tristate "Intel ACPI INT0002 Virtual GPIO driver" > - depends on GPIOLIB && ACPI > + depends on GPIOLIB && ACPI && PM_SLEEP > select GPIOLIB_IRQCHIP > help > Some peripherals on Bay Trail and Cherry Trail platforms signal a > diff --git a/drivers/platform/x86/intel_int0002_vgpio.c b/drivers/platform/x86/intel_int0002_vgpio.c > index 289c6655d425..569342aa8926 100644 > --- a/drivers/platform/x86/intel_int0002_vgpio.c > +++ b/drivers/platform/x86/intel_int0002_vgpio.c > @@ -51,6 +51,12 @@ > #define GPE0A_STS_PORT 0x420 > #define GPE0A_EN_PORT 0x428 > > +struct int0002_data { > + struct gpio_chip chip; > + int parent_irq; > + int wake_enable_count; > +}; > + > /* > * As this is not a real GPIO at all, but just a hack to model an event in > * ACPI the get / set functions are dummy functions. > @@ -98,14 +104,16 @@ static void int0002_irq_mask(struct irq_data *data) > static int int0002_irq_set_wake(struct irq_data *data, unsigned int on) > { > struct gpio_chip *chip = irq_data_get_irq_chip_data(data); > - struct platform_device *pdev = to_platform_device(chip->parent); > - int irq = platform_get_irq(pdev, 0); > + struct int0002_data *int0002 = container_of(chip, struct int0002_data, chip); > > - /* Propagate to parent irq */ > + /* > + * Applying of the wakeup flag to our parent IRQ is delayed till system > + * suspend, because we only want to do this when using s2idle. > + */ > if (on) > - enable_irq_wake(irq); > + int0002->wake_enable_count++; > else > - disable_irq_wake(irq); > + int0002->wake_enable_count--; > > return 0; > } > @@ -135,7 +143,7 @@ static bool int0002_check_wake(void *data) > return (gpe_sts_reg & GPE0A_PME_B0_STS_BIT); > } > > -static struct irq_chip int0002_byt_irqchip = { > +static struct irq_chip int0002_irqchip = { > .name = DRV_NAME, > .irq_ack = int0002_irq_ack, > .irq_mask = int0002_irq_mask, > @@ -143,21 +151,9 @@ static struct irq_chip int0002_byt_irqchip = { > .irq_set_wake = int0002_irq_set_wake, > }; > > -static struct irq_chip int0002_cht_irqchip = { > - .name = DRV_NAME, > - .irq_ack = int0002_irq_ack, > - .irq_mask = int0002_irq_mask, > - .irq_unmask = int0002_irq_unmask, > - /* > - * No set_wake, on CHT the IRQ is typically shared with the ACPI SCI > - * and we don't want to mess with the ACPI SCI irq settings. > - */ > - .flags = IRQCHIP_SKIP_SET_WAKE, > -}; > - > static const struct x86_cpu_id int0002_cpu_ids[] = { > - X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, &int0002_byt_irqchip), > - X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, &int0002_cht_irqchip), > + X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL), > + X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, NULL), > {} > }; > > @@ -172,8 +168,9 @@ static int int0002_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > const struct x86_cpu_id *cpu_id; > - struct gpio_chip *chip; > + struct int0002_data *int0002; > struct gpio_irq_chip *girq; > + struct gpio_chip *chip; > int irq, ret; > > /* Menlow has a different INT0002 device? <sigh> */ > @@ -185,10 +182,13 @@ static int int0002_probe(struct platform_device *pdev) > if (irq < 0) > return irq; > > - chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); > - if (!chip) > + int0002 = devm_kzalloc(dev, sizeof(*int0002), GFP_KERNEL); > + if (!int0002) > return -ENOMEM; > > + int0002->parent_irq = irq; > + > + chip = &int0002->chip; > chip->label = DRV_NAME; > chip->parent = dev; > chip->owner = THIS_MODULE; > @@ -214,7 +214,7 @@ static int int0002_probe(struct platform_device *pdev) > } > > girq = &chip->irq; > - girq->chip = (struct irq_chip *)cpu_id->driver_data; > + girq->chip = &int0002_irqchip; > /* This let us handle the parent IRQ in the driver */ > girq->parent_handler = NULL; > girq->num_parents = 0; > @@ -230,6 +230,7 @@ static int int0002_probe(struct platform_device *pdev) > > acpi_register_wakeup_handler(irq, int0002_check_wake, NULL); > device_init_wakeup(dev, true); > + dev_set_drvdata(dev, int0002); > return 0; > } > > @@ -240,6 +241,36 @@ static int int0002_remove(struct platform_device *pdev) > return 0; > } > > +static int int0002_suspend(struct device *dev) > +{ > + struct int0002_data *int0002 = dev_get_drvdata(dev); > + > + /* > + * The INT0002 parent IRQ is often shared with the ACPI GPE IRQ, don't > + * muck with it when firmware based suspend is used, otherwise we may > + * cause spurious wakeups from firmware managed suspend. > + */ > + if (!pm_suspend_via_firmware() && int0002->wake_enable_count) > + enable_irq_wake(int0002->parent_irq); > + > + return 0; > +} > + > +static int int0002_resume(struct device *dev) > +{ > + struct int0002_data *int0002 = dev_get_drvdata(dev); > + > + if (!pm_suspend_via_firmware() && int0002->wake_enable_count) > + disable_irq_wake(int0002->parent_irq); > + > + return 0; > +} > + > +static const struct dev_pm_ops int0002_pm_ops = { > + .suspend = int0002_suspend, > + .resume = int0002_resume, > +}; > + > static const struct acpi_device_id int0002_acpi_ids[] = { > { "INT0002", 0 }, > { }, > @@ -250,6 +281,7 @@ static struct platform_driver int0002_driver = { > .driver = { > .name = DRV_NAME, > .acpi_match_table = int0002_acpi_ids, > + .pm = &int0002_pm_ops, > }, > .probe = int0002_probe, > .remove = int0002_remove, > -- > 2.31.1 >
On Wed, May 12, 2021 at 2:56 PM Hans de Goede <hdegoede@redhat.com> wrote: > > Commit 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement > irq_set_wake on Bay Trail") stopped passing irq_set_wake requests on to > the parents IRQ because this was breaking suspend (causing immediate > wakeups) on an Asus E202SA. > > This workaround for the Asus E202SA is causing wakeup by USB keyboard to > not work on other devices with Airmont CPU cores such as the Medion Akoya > E1239T. In hindsight the problem with the Asus E202SA has nothing to do > with Silvermont vs Airmont CPU cores, so the differentiation between the > 2 types of CPU cores introduced by the previous fix is wrong. > > The real issue at hand is s2idle vs S3 suspend where the suspend is > mostly handled by firmware. The parent IRQ for the INT0002 device is shared > with the ACPI SCI and the real problem is that the INT0002 code should not > be messing with the wakeup settings of that IRQ when suspend/resume is > being handled by the firmware. > > Note that on systems which support both s2idle and S3 suspend, which > suspend method to use can be changed at runtime. > > This patch fixes both the Asus E202SA spurious wakeups issue as well as > the wakeup by USB keyboard not working on the Medion Akoya E1239T issue. > > These are both fixed by replacing the old workaround with delaying the > enable_irq_wake(parent_irq) call till system-suspend time and protecting > it with a !pm_suspend_via_firmware() check so that we still do not call > it on devices using firmware-based (S3) suspend such as the Asus E202SA. > > Note rather then adding #ifdef CONFIG_PM_SLEEP, this commit simply adds > a "depends on PM_SLEEP" to the Kconfig since this drivers whole purpose > is to deal with wakeup events, so using it without CONFIG_PM_SLEEP makes > no sense. > > Cc: Maxim Mikityanskiy <maxtram95@gmail.com> > Fixes: 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement irq_set_wake on Bay Trail") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> Makes sense to me. Reviewed-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > drivers/platform/x86/Kconfig | 2 +- > drivers/platform/x86/intel_int0002_vgpio.c | 80 +++++++++++++++------- > 2 files changed, 57 insertions(+), 25 deletions(-) > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 4b67e74a747b..c2f608d5f1b7 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -713,7 +713,7 @@ config INTEL_HID_EVENT > > config INTEL_INT0002_VGPIO > tristate "Intel ACPI INT0002 Virtual GPIO driver" > - depends on GPIOLIB && ACPI > + depends on GPIOLIB && ACPI && PM_SLEEP > select GPIOLIB_IRQCHIP > help > Some peripherals on Bay Trail and Cherry Trail platforms signal a > diff --git a/drivers/platform/x86/intel_int0002_vgpio.c b/drivers/platform/x86/intel_int0002_vgpio.c > index 289c6655d425..569342aa8926 100644 > --- a/drivers/platform/x86/intel_int0002_vgpio.c > +++ b/drivers/platform/x86/intel_int0002_vgpio.c > @@ -51,6 +51,12 @@ > #define GPE0A_STS_PORT 0x420 > #define GPE0A_EN_PORT 0x428 > > +struct int0002_data { > + struct gpio_chip chip; > + int parent_irq; > + int wake_enable_count; > +}; > + > /* > * As this is not a real GPIO at all, but just a hack to model an event in > * ACPI the get / set functions are dummy functions. > @@ -98,14 +104,16 @@ static void int0002_irq_mask(struct irq_data *data) > static int int0002_irq_set_wake(struct irq_data *data, unsigned int on) > { > struct gpio_chip *chip = irq_data_get_irq_chip_data(data); > - struct platform_device *pdev = to_platform_device(chip->parent); > - int irq = platform_get_irq(pdev, 0); > + struct int0002_data *int0002 = container_of(chip, struct int0002_data, chip); > > - /* Propagate to parent irq */ > + /* > + * Applying of the wakeup flag to our parent IRQ is delayed till system > + * suspend, because we only want to do this when using s2idle. > + */ > if (on) > - enable_irq_wake(irq); > + int0002->wake_enable_count++; > else > - disable_irq_wake(irq); > + int0002->wake_enable_count--; > > return 0; > } > @@ -135,7 +143,7 @@ static bool int0002_check_wake(void *data) > return (gpe_sts_reg & GPE0A_PME_B0_STS_BIT); > } > > -static struct irq_chip int0002_byt_irqchip = { > +static struct irq_chip int0002_irqchip = { > .name = DRV_NAME, > .irq_ack = int0002_irq_ack, > .irq_mask = int0002_irq_mask, > @@ -143,21 +151,9 @@ static struct irq_chip int0002_byt_irqchip = { > .irq_set_wake = int0002_irq_set_wake, > }; > > -static struct irq_chip int0002_cht_irqchip = { > - .name = DRV_NAME, > - .irq_ack = int0002_irq_ack, > - .irq_mask = int0002_irq_mask, > - .irq_unmask = int0002_irq_unmask, > - /* > - * No set_wake, on CHT the IRQ is typically shared with the ACPI SCI > - * and we don't want to mess with the ACPI SCI irq settings. > - */ > - .flags = IRQCHIP_SKIP_SET_WAKE, > -}; > - > static const struct x86_cpu_id int0002_cpu_ids[] = { > - X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, &int0002_byt_irqchip), > - X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, &int0002_cht_irqchip), > + X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL), > + X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, NULL), > {} > }; > > @@ -172,8 +168,9 @@ static int int0002_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > const struct x86_cpu_id *cpu_id; > - struct gpio_chip *chip; > + struct int0002_data *int0002; > struct gpio_irq_chip *girq; > + struct gpio_chip *chip; > int irq, ret; > > /* Menlow has a different INT0002 device? <sigh> */ > @@ -185,10 +182,13 @@ static int int0002_probe(struct platform_device *pdev) > if (irq < 0) > return irq; > > - chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); > - if (!chip) > + int0002 = devm_kzalloc(dev, sizeof(*int0002), GFP_KERNEL); > + if (!int0002) > return -ENOMEM; > > + int0002->parent_irq = irq; > + > + chip = &int0002->chip; > chip->label = DRV_NAME; > chip->parent = dev; > chip->owner = THIS_MODULE; > @@ -214,7 +214,7 @@ static int int0002_probe(struct platform_device *pdev) > } > > girq = &chip->irq; > - girq->chip = (struct irq_chip *)cpu_id->driver_data; > + girq->chip = &int0002_irqchip; > /* This let us handle the parent IRQ in the driver */ > girq->parent_handler = NULL; > girq->num_parents = 0; > @@ -230,6 +230,7 @@ static int int0002_probe(struct platform_device *pdev) > > acpi_register_wakeup_handler(irq, int0002_check_wake, NULL); > device_init_wakeup(dev, true); > + dev_set_drvdata(dev, int0002); > return 0; > } > > @@ -240,6 +241,36 @@ static int int0002_remove(struct platform_device *pdev) > return 0; > } > > +static int int0002_suspend(struct device *dev) > +{ > + struct int0002_data *int0002 = dev_get_drvdata(dev); > + > + /* > + * The INT0002 parent IRQ is often shared with the ACPI GPE IRQ, don't > + * muck with it when firmware based suspend is used, otherwise we may > + * cause spurious wakeups from firmware managed suspend. > + */ > + if (!pm_suspend_via_firmware() && int0002->wake_enable_count) > + enable_irq_wake(int0002->parent_irq); > + > + return 0; > +} > + > +static int int0002_resume(struct device *dev) > +{ > + struct int0002_data *int0002 = dev_get_drvdata(dev); > + > + if (!pm_suspend_via_firmware() && int0002->wake_enable_count) > + disable_irq_wake(int0002->parent_irq); > + > + return 0; > +} > + > +static const struct dev_pm_ops int0002_pm_ops = { > + .suspend = int0002_suspend, > + .resume = int0002_resume, > +}; > + > static const struct acpi_device_id int0002_acpi_ids[] = { > { "INT0002", 0 }, > { }, > @@ -250,6 +281,7 @@ static struct platform_driver int0002_driver = { > .driver = { > .name = DRV_NAME, > .acpi_match_table = int0002_acpi_ids, > + .pm = &int0002_pm_ops, > }, > .probe = int0002_probe, > .remove = int0002_remove, > -- > 2.31.1 >
Hi, On 5/12/21 3:20 PM, Andy Shevchenko wrote: > On Wed, May 12, 2021 at 3:55 PM Hans de Goede <hdegoede@redhat.com> wrote: >> >> Commit 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement >> irq_set_wake on Bay Trail") stopped passing irq_set_wake requests on to >> the parents IRQ because this was breaking suspend (causing immediate >> wakeups) on an Asus E202SA. >> >> This workaround for the Asus E202SA is causing wakeup by USB keyboard to >> not work on other devices with Airmont CPU cores such as the Medion Akoya >> E1239T. In hindsight the problem with the Asus E202SA has nothing to do >> with Silvermont vs Airmont CPU cores, so the differentiation between the >> 2 types of CPU cores introduced by the previous fix is wrong. >> >> The real issue at hand is s2idle vs S3 suspend where the suspend is >> mostly handled by firmware. The parent IRQ for the INT0002 device is shared >> with the ACPI SCI and the real problem is that the INT0002 code should not >> be messing with the wakeup settings of that IRQ when suspend/resume is >> being handled by the firmware. >> >> Note that on systems which support both s2idle and S3 suspend, which >> suspend method to use can be changed at runtime. >> >> This patch fixes both the Asus E202SA spurious wakeups issue as well as >> the wakeup by USB keyboard not working on the Medion Akoya E1239T issue. >> >> These are both fixed by replacing the old workaround with delaying the >> enable_irq_wake(parent_irq) call till system-suspend time and protecting >> it with a !pm_suspend_via_firmware() check so that we still do not call >> it on devices using firmware-based (S3) suspend such as the Asus E202SA. > >> Note rather then adding #ifdef CONFIG_PM_SLEEP, this commit simply adds >> a "depends on PM_SLEEP" to the Kconfig since this drivers whole purpose >> is to deal with wakeup events, so using it without CONFIG_PM_SLEEP makes >> no sense. > > I like the new approach. > One remark (or two :) is to the PM SLEEP thingy. Can we add a separate > line for "depends on", so it will be easier to maintain in case we > need to amend it somehow? The depends on is already using && and I prefer to keep it that way over splitting it into 3 separate depends on lines. > Another one is to amend a helpline to > reflect that the driver is dealing solely with wake events (I haven't > checked the current text, so it might be already enforced). The helptext already begins with this: "Some peripherals on Bay Trail and Cherry Trail platforms signal a Power Management Event (PME) to the Power Management Controller (PMC) to wakeup the system." Which IMHO makes it pretty clear this is all about wake events. > Reviewed-by: Andy Shevchenko <andy.shevchenko@gmail.com> Thank you. Regards, Hans > >> Cc: Maxim Mikityanskiy <maxtram95@gmail.com> >> Fixes: 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement irq_set_wake on Bay Trail") >> Signed-off-by: Hans de Goede <hdegoede@redhat.com> >> --- >> drivers/platform/x86/Kconfig | 2 +- >> drivers/platform/x86/intel_int0002_vgpio.c | 80 +++++++++++++++------- >> 2 files changed, 57 insertions(+), 25 deletions(-) >> >> diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig >> index 4b67e74a747b..c2f608d5f1b7 100644 >> --- a/drivers/platform/x86/Kconfig >> +++ b/drivers/platform/x86/Kconfig >> @@ -713,7 +713,7 @@ config INTEL_HID_EVENT >> >> config INTEL_INT0002_VGPIO >> tristate "Intel ACPI INT0002 Virtual GPIO driver" >> - depends on GPIOLIB && ACPI >> + depends on GPIOLIB && ACPI && PM_SLEEP >> select GPIOLIB_IRQCHIP >> help >> Some peripherals on Bay Trail and Cherry Trail platforms signal a >> diff --git a/drivers/platform/x86/intel_int0002_vgpio.c b/drivers/platform/x86/intel_int0002_vgpio.c >> index 289c6655d425..569342aa8926 100644 >> --- a/drivers/platform/x86/intel_int0002_vgpio.c >> +++ b/drivers/platform/x86/intel_int0002_vgpio.c >> @@ -51,6 +51,12 @@ >> #define GPE0A_STS_PORT 0x420 >> #define GPE0A_EN_PORT 0x428 >> >> +struct int0002_data { >> + struct gpio_chip chip; >> + int parent_irq; >> + int wake_enable_count; >> +}; >> + >> /* >> * As this is not a real GPIO at all, but just a hack to model an event in >> * ACPI the get / set functions are dummy functions. >> @@ -98,14 +104,16 @@ static void int0002_irq_mask(struct irq_data *data) >> static int int0002_irq_set_wake(struct irq_data *data, unsigned int on) >> { >> struct gpio_chip *chip = irq_data_get_irq_chip_data(data); >> - struct platform_device *pdev = to_platform_device(chip->parent); >> - int irq = platform_get_irq(pdev, 0); >> + struct int0002_data *int0002 = container_of(chip, struct int0002_data, chip); >> >> - /* Propagate to parent irq */ >> + /* >> + * Applying of the wakeup flag to our parent IRQ is delayed till system >> + * suspend, because we only want to do this when using s2idle. >> + */ >> if (on) >> - enable_irq_wake(irq); >> + int0002->wake_enable_count++; >> else >> - disable_irq_wake(irq); >> + int0002->wake_enable_count--; >> >> return 0; >> } >> @@ -135,7 +143,7 @@ static bool int0002_check_wake(void *data) >> return (gpe_sts_reg & GPE0A_PME_B0_STS_BIT); >> } >> >> -static struct irq_chip int0002_byt_irqchip = { >> +static struct irq_chip int0002_irqchip = { >> .name = DRV_NAME, >> .irq_ack = int0002_irq_ack, >> .irq_mask = int0002_irq_mask, >> @@ -143,21 +151,9 @@ static struct irq_chip int0002_byt_irqchip = { >> .irq_set_wake = int0002_irq_set_wake, >> }; >> >> -static struct irq_chip int0002_cht_irqchip = { >> - .name = DRV_NAME, >> - .irq_ack = int0002_irq_ack, >> - .irq_mask = int0002_irq_mask, >> - .irq_unmask = int0002_irq_unmask, >> - /* >> - * No set_wake, on CHT the IRQ is typically shared with the ACPI SCI >> - * and we don't want to mess with the ACPI SCI irq settings. >> - */ >> - .flags = IRQCHIP_SKIP_SET_WAKE, >> -}; >> - >> static const struct x86_cpu_id int0002_cpu_ids[] = { >> - X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, &int0002_byt_irqchip), >> - X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, &int0002_cht_irqchip), >> + X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL), >> + X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, NULL), >> {} >> }; >> >> @@ -172,8 +168,9 @@ static int int0002_probe(struct platform_device *pdev) >> { >> struct device *dev = &pdev->dev; >> const struct x86_cpu_id *cpu_id; >> - struct gpio_chip *chip; >> + struct int0002_data *int0002; >> struct gpio_irq_chip *girq; >> + struct gpio_chip *chip; >> int irq, ret; >> >> /* Menlow has a different INT0002 device? <sigh> */ >> @@ -185,10 +182,13 @@ static int int0002_probe(struct platform_device *pdev) >> if (irq < 0) >> return irq; >> >> - chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); >> - if (!chip) >> + int0002 = devm_kzalloc(dev, sizeof(*int0002), GFP_KERNEL); >> + if (!int0002) >> return -ENOMEM; >> >> + int0002->parent_irq = irq; >> + >> + chip = &int0002->chip; >> chip->label = DRV_NAME; >> chip->parent = dev; >> chip->owner = THIS_MODULE; >> @@ -214,7 +214,7 @@ static int int0002_probe(struct platform_device *pdev) >> } >> >> girq = &chip->irq; >> - girq->chip = (struct irq_chip *)cpu_id->driver_data; >> + girq->chip = &int0002_irqchip; >> /* This let us handle the parent IRQ in the driver */ >> girq->parent_handler = NULL; >> girq->num_parents = 0; >> @@ -230,6 +230,7 @@ static int int0002_probe(struct platform_device *pdev) >> >> acpi_register_wakeup_handler(irq, int0002_check_wake, NULL); >> device_init_wakeup(dev, true); >> + dev_set_drvdata(dev, int0002); >> return 0; >> } >> >> @@ -240,6 +241,36 @@ static int int0002_remove(struct platform_device *pdev) >> return 0; >> } >> >> +static int int0002_suspend(struct device *dev) >> +{ >> + struct int0002_data *int0002 = dev_get_drvdata(dev); >> + >> + /* >> + * The INT0002 parent IRQ is often shared with the ACPI GPE IRQ, don't >> + * muck with it when firmware based suspend is used, otherwise we may >> + * cause spurious wakeups from firmware managed suspend. >> + */ >> + if (!pm_suspend_via_firmware() && int0002->wake_enable_count) >> + enable_irq_wake(int0002->parent_irq); >> + >> + return 0; >> +} >> + >> +static int int0002_resume(struct device *dev) >> +{ >> + struct int0002_data *int0002 = dev_get_drvdata(dev); >> + >> + if (!pm_suspend_via_firmware() && int0002->wake_enable_count) >> + disable_irq_wake(int0002->parent_irq); >> + >> + return 0; >> +} >> + >> +static const struct dev_pm_ops int0002_pm_ops = { >> + .suspend = int0002_suspend, >> + .resume = int0002_resume, >> +}; >> + >> static const struct acpi_device_id int0002_acpi_ids[] = { >> { "INT0002", 0 }, >> { }, >> @@ -250,6 +281,7 @@ static struct platform_driver int0002_driver = { >> .driver = { >> .name = DRV_NAME, >> .acpi_match_table = int0002_acpi_ids, >> + .pm = &int0002_pm_ops, >> }, >> .probe = int0002_probe, >> .remove = int0002_remove, >> -- >> 2.31.1 >> > >
Hi, On 5/12/21 2:55 PM, Hans de Goede wrote: > Commit 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement > irq_set_wake on Bay Trail") stopped passing irq_set_wake requests on to > the parents IRQ because this was breaking suspend (causing immediate > wakeups) on an Asus E202SA. > > This workaround for the Asus E202SA is causing wakeup by USB keyboard to > not work on other devices with Airmont CPU cores such as the Medion Akoya > E1239T. In hindsight the problem with the Asus E202SA has nothing to do > with Silvermont vs Airmont CPU cores, so the differentiation between the > 2 types of CPU cores introduced by the previous fix is wrong. > > The real issue at hand is s2idle vs S3 suspend where the suspend is > mostly handled by firmware. The parent IRQ for the INT0002 device is shared > with the ACPI SCI and the real problem is that the INT0002 code should not > be messing with the wakeup settings of that IRQ when suspend/resume is > being handled by the firmware. > > Note that on systems which support both s2idle and S3 suspend, which > suspend method to use can be changed at runtime. > > This patch fixes both the Asus E202SA spurious wakeups issue as well as > the wakeup by USB keyboard not working on the Medion Akoya E1239T issue. > > These are both fixed by replacing the old workaround with delaying the > enable_irq_wake(parent_irq) call till system-suspend time and protecting > it with a !pm_suspend_via_firmware() check so that we still do not call > it on devices using firmware-based (S3) suspend such as the Asus E202SA. > > Note rather then adding #ifdef CONFIG_PM_SLEEP, this commit simply adds > a "depends on PM_SLEEP" to the Kconfig since this drivers whole purpose > is to deal with wakeup events, so using it without CONFIG_PM_SLEEP makes > no sense. > > Cc: Maxim Mikityanskiy <maxtram95@gmail.com> > Fixes: 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement irq_set_wake on Bay Trail") > Signed-off-by: Hans de Goede <hdegoede@redhat.com> I've added this to my review-hans branch now and I will also include this in the next pdx86-fixes pull-req for 5.13. Regards, Hans > --- > drivers/platform/x86/Kconfig | 2 +- > drivers/platform/x86/intel_int0002_vgpio.c | 80 +++++++++++++++------- > 2 files changed, 57 insertions(+), 25 deletions(-) > > diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig > index 4b67e74a747b..c2f608d5f1b7 100644 > --- a/drivers/platform/x86/Kconfig > +++ b/drivers/platform/x86/Kconfig > @@ -713,7 +713,7 @@ config INTEL_HID_EVENT > > config INTEL_INT0002_VGPIO > tristate "Intel ACPI INT0002 Virtual GPIO driver" > - depends on GPIOLIB && ACPI > + depends on GPIOLIB && ACPI && PM_SLEEP > select GPIOLIB_IRQCHIP > help > Some peripherals on Bay Trail and Cherry Trail platforms signal a > diff --git a/drivers/platform/x86/intel_int0002_vgpio.c b/drivers/platform/x86/intel_int0002_vgpio.c > index 289c6655d425..569342aa8926 100644 > --- a/drivers/platform/x86/intel_int0002_vgpio.c > +++ b/drivers/platform/x86/intel_int0002_vgpio.c > @@ -51,6 +51,12 @@ > #define GPE0A_STS_PORT 0x420 > #define GPE0A_EN_PORT 0x428 > > +struct int0002_data { > + struct gpio_chip chip; > + int parent_irq; > + int wake_enable_count; > +}; > + > /* > * As this is not a real GPIO at all, but just a hack to model an event in > * ACPI the get / set functions are dummy functions. > @@ -98,14 +104,16 @@ static void int0002_irq_mask(struct irq_data *data) > static int int0002_irq_set_wake(struct irq_data *data, unsigned int on) > { > struct gpio_chip *chip = irq_data_get_irq_chip_data(data); > - struct platform_device *pdev = to_platform_device(chip->parent); > - int irq = platform_get_irq(pdev, 0); > + struct int0002_data *int0002 = container_of(chip, struct int0002_data, chip); > > - /* Propagate to parent irq */ > + /* > + * Applying of the wakeup flag to our parent IRQ is delayed till system > + * suspend, because we only want to do this when using s2idle. > + */ > if (on) > - enable_irq_wake(irq); > + int0002->wake_enable_count++; > else > - disable_irq_wake(irq); > + int0002->wake_enable_count--; > > return 0; > } > @@ -135,7 +143,7 @@ static bool int0002_check_wake(void *data) > return (gpe_sts_reg & GPE0A_PME_B0_STS_BIT); > } > > -static struct irq_chip int0002_byt_irqchip = { > +static struct irq_chip int0002_irqchip = { > .name = DRV_NAME, > .irq_ack = int0002_irq_ack, > .irq_mask = int0002_irq_mask, > @@ -143,21 +151,9 @@ static struct irq_chip int0002_byt_irqchip = { > .irq_set_wake = int0002_irq_set_wake, > }; > > -static struct irq_chip int0002_cht_irqchip = { > - .name = DRV_NAME, > - .irq_ack = int0002_irq_ack, > - .irq_mask = int0002_irq_mask, > - .irq_unmask = int0002_irq_unmask, > - /* > - * No set_wake, on CHT the IRQ is typically shared with the ACPI SCI > - * and we don't want to mess with the ACPI SCI irq settings. > - */ > - .flags = IRQCHIP_SKIP_SET_WAKE, > -}; > - > static const struct x86_cpu_id int0002_cpu_ids[] = { > - X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, &int0002_byt_irqchip), > - X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, &int0002_cht_irqchip), > + X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL), > + X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, NULL), > {} > }; > > @@ -172,8 +168,9 @@ static int int0002_probe(struct platform_device *pdev) > { > struct device *dev = &pdev->dev; > const struct x86_cpu_id *cpu_id; > - struct gpio_chip *chip; > + struct int0002_data *int0002; > struct gpio_irq_chip *girq; > + struct gpio_chip *chip; > int irq, ret; > > /* Menlow has a different INT0002 device? <sigh> */ > @@ -185,10 +182,13 @@ static int int0002_probe(struct platform_device *pdev) > if (irq < 0) > return irq; > > - chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); > - if (!chip) > + int0002 = devm_kzalloc(dev, sizeof(*int0002), GFP_KERNEL); > + if (!int0002) > return -ENOMEM; > > + int0002->parent_irq = irq; > + > + chip = &int0002->chip; > chip->label = DRV_NAME; > chip->parent = dev; > chip->owner = THIS_MODULE; > @@ -214,7 +214,7 @@ static int int0002_probe(struct platform_device *pdev) > } > > girq = &chip->irq; > - girq->chip = (struct irq_chip *)cpu_id->driver_data; > + girq->chip = &int0002_irqchip; > /* This let us handle the parent IRQ in the driver */ > girq->parent_handler = NULL; > girq->num_parents = 0; > @@ -230,6 +230,7 @@ static int int0002_probe(struct platform_device *pdev) > > acpi_register_wakeup_handler(irq, int0002_check_wake, NULL); > device_init_wakeup(dev, true); > + dev_set_drvdata(dev, int0002); > return 0; > } > > @@ -240,6 +241,36 @@ static int int0002_remove(struct platform_device *pdev) > return 0; > } > > +static int int0002_suspend(struct device *dev) > +{ > + struct int0002_data *int0002 = dev_get_drvdata(dev); > + > + /* > + * The INT0002 parent IRQ is often shared with the ACPI GPE IRQ, don't > + * muck with it when firmware based suspend is used, otherwise we may > + * cause spurious wakeups from firmware managed suspend. > + */ > + if (!pm_suspend_via_firmware() && int0002->wake_enable_count) > + enable_irq_wake(int0002->parent_irq); > + > + return 0; > +} > + > +static int int0002_resume(struct device *dev) > +{ > + struct int0002_data *int0002 = dev_get_drvdata(dev); > + > + if (!pm_suspend_via_firmware() && int0002->wake_enable_count) > + disable_irq_wake(int0002->parent_irq); > + > + return 0; > +} > + > +static const struct dev_pm_ops int0002_pm_ops = { > + .suspend = int0002_suspend, > + .resume = int0002_resume, > +}; > + > static const struct acpi_device_id int0002_acpi_ids[] = { > { "INT0002", 0 }, > { }, > @@ -250,6 +281,7 @@ static struct platform_driver int0002_driver = { > .driver = { > .name = DRV_NAME, > .acpi_match_table = int0002_acpi_ids, > + .pm = &int0002_pm_ops, > }, > .probe = int0002_probe, > .remove = int0002_remove, >
diff --git a/drivers/platform/x86/Kconfig b/drivers/platform/x86/Kconfig index 4b67e74a747b..c2f608d5f1b7 100644 --- a/drivers/platform/x86/Kconfig +++ b/drivers/platform/x86/Kconfig @@ -713,7 +713,7 @@ config INTEL_HID_EVENT config INTEL_INT0002_VGPIO tristate "Intel ACPI INT0002 Virtual GPIO driver" - depends on GPIOLIB && ACPI + depends on GPIOLIB && ACPI && PM_SLEEP select GPIOLIB_IRQCHIP help Some peripherals on Bay Trail and Cherry Trail platforms signal a diff --git a/drivers/platform/x86/intel_int0002_vgpio.c b/drivers/platform/x86/intel_int0002_vgpio.c index 289c6655d425..569342aa8926 100644 --- a/drivers/platform/x86/intel_int0002_vgpio.c +++ b/drivers/platform/x86/intel_int0002_vgpio.c @@ -51,6 +51,12 @@ #define GPE0A_STS_PORT 0x420 #define GPE0A_EN_PORT 0x428 +struct int0002_data { + struct gpio_chip chip; + int parent_irq; + int wake_enable_count; +}; + /* * As this is not a real GPIO at all, but just a hack to model an event in * ACPI the get / set functions are dummy functions. @@ -98,14 +104,16 @@ static void int0002_irq_mask(struct irq_data *data) static int int0002_irq_set_wake(struct irq_data *data, unsigned int on) { struct gpio_chip *chip = irq_data_get_irq_chip_data(data); - struct platform_device *pdev = to_platform_device(chip->parent); - int irq = platform_get_irq(pdev, 0); + struct int0002_data *int0002 = container_of(chip, struct int0002_data, chip); - /* Propagate to parent irq */ + /* + * Applying of the wakeup flag to our parent IRQ is delayed till system + * suspend, because we only want to do this when using s2idle. + */ if (on) - enable_irq_wake(irq); + int0002->wake_enable_count++; else - disable_irq_wake(irq); + int0002->wake_enable_count--; return 0; } @@ -135,7 +143,7 @@ static bool int0002_check_wake(void *data) return (gpe_sts_reg & GPE0A_PME_B0_STS_BIT); } -static struct irq_chip int0002_byt_irqchip = { +static struct irq_chip int0002_irqchip = { .name = DRV_NAME, .irq_ack = int0002_irq_ack, .irq_mask = int0002_irq_mask, @@ -143,21 +151,9 @@ static struct irq_chip int0002_byt_irqchip = { .irq_set_wake = int0002_irq_set_wake, }; -static struct irq_chip int0002_cht_irqchip = { - .name = DRV_NAME, - .irq_ack = int0002_irq_ack, - .irq_mask = int0002_irq_mask, - .irq_unmask = int0002_irq_unmask, - /* - * No set_wake, on CHT the IRQ is typically shared with the ACPI SCI - * and we don't want to mess with the ACPI SCI irq settings. - */ - .flags = IRQCHIP_SKIP_SET_WAKE, -}; - static const struct x86_cpu_id int0002_cpu_ids[] = { - X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, &int0002_byt_irqchip), - X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, &int0002_cht_irqchip), + X86_MATCH_INTEL_FAM6_MODEL(ATOM_SILVERMONT, NULL), + X86_MATCH_INTEL_FAM6_MODEL(ATOM_AIRMONT, NULL), {} }; @@ -172,8 +168,9 @@ static int int0002_probe(struct platform_device *pdev) { struct device *dev = &pdev->dev; const struct x86_cpu_id *cpu_id; - struct gpio_chip *chip; + struct int0002_data *int0002; struct gpio_irq_chip *girq; + struct gpio_chip *chip; int irq, ret; /* Menlow has a different INT0002 device? <sigh> */ @@ -185,10 +182,13 @@ static int int0002_probe(struct platform_device *pdev) if (irq < 0) return irq; - chip = devm_kzalloc(dev, sizeof(*chip), GFP_KERNEL); - if (!chip) + int0002 = devm_kzalloc(dev, sizeof(*int0002), GFP_KERNEL); + if (!int0002) return -ENOMEM; + int0002->parent_irq = irq; + + chip = &int0002->chip; chip->label = DRV_NAME; chip->parent = dev; chip->owner = THIS_MODULE; @@ -214,7 +214,7 @@ static int int0002_probe(struct platform_device *pdev) } girq = &chip->irq; - girq->chip = (struct irq_chip *)cpu_id->driver_data; + girq->chip = &int0002_irqchip; /* This let us handle the parent IRQ in the driver */ girq->parent_handler = NULL; girq->num_parents = 0; @@ -230,6 +230,7 @@ static int int0002_probe(struct platform_device *pdev) acpi_register_wakeup_handler(irq, int0002_check_wake, NULL); device_init_wakeup(dev, true); + dev_set_drvdata(dev, int0002); return 0; } @@ -240,6 +241,36 @@ static int int0002_remove(struct platform_device *pdev) return 0; } +static int int0002_suspend(struct device *dev) +{ + struct int0002_data *int0002 = dev_get_drvdata(dev); + + /* + * The INT0002 parent IRQ is often shared with the ACPI GPE IRQ, don't + * muck with it when firmware based suspend is used, otherwise we may + * cause spurious wakeups from firmware managed suspend. + */ + if (!pm_suspend_via_firmware() && int0002->wake_enable_count) + enable_irq_wake(int0002->parent_irq); + + return 0; +} + +static int int0002_resume(struct device *dev) +{ + struct int0002_data *int0002 = dev_get_drvdata(dev); + + if (!pm_suspend_via_firmware() && int0002->wake_enable_count) + disable_irq_wake(int0002->parent_irq); + + return 0; +} + +static const struct dev_pm_ops int0002_pm_ops = { + .suspend = int0002_suspend, + .resume = int0002_resume, +}; + static const struct acpi_device_id int0002_acpi_ids[] = { { "INT0002", 0 }, { }, @@ -250,6 +281,7 @@ static struct platform_driver int0002_driver = { .driver = { .name = DRV_NAME, .acpi_match_table = int0002_acpi_ids, + .pm = &int0002_pm_ops, }, .probe = int0002_probe, .remove = int0002_remove,
Commit 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement irq_set_wake on Bay Trail") stopped passing irq_set_wake requests on to the parents IRQ because this was breaking suspend (causing immediate wakeups) on an Asus E202SA. This workaround for the Asus E202SA is causing wakeup by USB keyboard to not work on other devices with Airmont CPU cores such as the Medion Akoya E1239T. In hindsight the problem with the Asus E202SA has nothing to do with Silvermont vs Airmont CPU cores, so the differentiation between the 2 types of CPU cores introduced by the previous fix is wrong. The real issue at hand is s2idle vs S3 suspend where the suspend is mostly handled by firmware. The parent IRQ for the INT0002 device is shared with the ACPI SCI and the real problem is that the INT0002 code should not be messing with the wakeup settings of that IRQ when suspend/resume is being handled by the firmware. Note that on systems which support both s2idle and S3 suspend, which suspend method to use can be changed at runtime. This patch fixes both the Asus E202SA spurious wakeups issue as well as the wakeup by USB keyboard not working on the Medion Akoya E1239T issue. These are both fixed by replacing the old workaround with delaying the enable_irq_wake(parent_irq) call till system-suspend time and protecting it with a !pm_suspend_via_firmware() check so that we still do not call it on devices using firmware-based (S3) suspend such as the Asus E202SA. Note rather then adding #ifdef CONFIG_PM_SLEEP, this commit simply adds a "depends on PM_SLEEP" to the Kconfig since this drivers whole purpose is to deal with wakeup events, so using it without CONFIG_PM_SLEEP makes no sense. Cc: Maxim Mikityanskiy <maxtram95@gmail.com> Fixes: 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement irq_set_wake on Bay Trail") Signed-off-by: Hans de Goede <hdegoede@redhat.com> --- drivers/platform/x86/Kconfig | 2 +- drivers/platform/x86/intel_int0002_vgpio.c | 80 +++++++++++++++------- 2 files changed, 57 insertions(+), 25 deletions(-)