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]

 



Hi,

On 5/12/21 3:20 PM, Andy Shevchenko wrote:
> 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?

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@xxxxxxxxx>

Thank you.

Regards,

Hans


> 
>> 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
>>
> 
> 




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

  Powered by Linux