Re: [PATCH 1/1] platform/x86: intel_int0002_vgpio: Only call enable_irq_wake() when using s2idle

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux