On Wed, May 12, 2021 at 3:55 PM Hans de Goede <hdegoede@xxxxxxxxxx> 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@xxxxxxxxx>
> Cc: Maxim Mikityanskiy <maxtram95@xxxxxxxxx>
> Fixes: 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement irq_set_wake on Bay Trail")
> Signed-off-by: Hans de Goede <hdegoede@xxxxxxxxxx>
> ---
> 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
>
--
With Best Regards,
Andy Shevchenko
