Date:   Fri, 17 Apr 2020 00:54:36 +0300
From:   Maxim Mikityanskiy <maxtram95@...il.com>
To:     Hans de Goede <hdegoede@...hat.com>
Cc:     Darren Hart <dvhart@...radead.org>,
        Andy Shevchenko <andy@...radead.org>,
        Platform Driver <platform-driver-x86@...r.kernel.org>,
        Linux Kernel Mailing List <linux-kernel@...r.kernel.org>,
        "5 . 3+" <stable@...r.kernel.org>
Subject: Re: [PATCH] platform/x86: intel_int0002_vgpio: Only bind to the
 INT0002 dev when using s2idle

On Tue, Apr 14, 2020 at 4:24 PM Hans de Goede <hdegoede@...hat.com> wrote:
>
> Hi,
>
> On 4/8/20 2:11 PM, Maxim Mikityanskiy wrote:
> > On Wed, Apr 8, 2020 at 12:31 AM Hans de Goede <hdegoede@...hat.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 this issue is mostly fine, on most Cherry Trail
> >> devices where we need the INT0002 device for wakeups by e.g. USB kbds,
> >> the parent IRQ is shared with the ACPI SCI and that is marked as wakeup
> >> anyways.
> >>
> >> But not on all devices, specifically on a Medion Akoya E1239T there is
> >> no SCI at all, and because the irq_set_wake request is not passed on to
> >> the parent IRQ, wake up by the builtin USB kbd does not work here.
> >>
> >> So the workaround for the Asus E202SA immediate wake problem is causing
> >> problems elsewhere; and in hindsight it is not the correct fix,
> >> the Asus E202SA uses Airmont CPU cores, but this does not mean it is a
> >> Cherry Trail based device, Brasswell uses Airmont CPU cores too and this
> >> actually is a Braswell device.
> >>
> >> Most (all?) Braswell devices use classic S3 mode suspend rather then
> >> s2idle suspend and in this case directly dealing with PME events as
> >> the INT0002 driver does likely is not the best idea, so that this is
> >> causing issues is not surprising.
> >>
> >> Replace the workaround of not passing irq_set_wake requests on to the
> >> parents IRQ, by not binding to the INT0002 device when s2idle is not used.
> >> This fixes USB kbd wakeups not working on some Cherry Trail devices,
> >> while still avoiding mucking with the wakeup flags on the Asus E202SA
> >> (and other Brasswell devices).
> >
> > I tested this patch over kernel 5.6.2 on Asus E202SA and didn't notice
> > any regressions. Wakeup by opening lid, by pressing a button on
> > keyboard, by USB keyboard — all seem to work fine. So, if appropriate:
> >
> > Tested-by: Maxim Mikityanskiy <maxtram95@...il.com>
>
> Thank you for testing this.
>
> > I have a question though. After your patch this driver will basically
> > be a no-op on my laptop. Does it mean I don't even need it in the
> > first place? What about the IRQ storm this driver is meant to deal
> > with — does it never happen on Braswell? What are the reproduction
> > steps to verify my hardware is not affected? I have that INT0002
> > device, so I'm worried it may cause issues if not bound to the driver.
>
> I do not expect Braswell platforms to suffer from the IRQ storm
> issue. That was something which I hit on a Cherry Trail based device.
>
> To test this, try waking up the device from suspend by an USB attached
> keyboard (this may not work, in that case wake it some other way).
>
> After this do:
>
> cat /proc/interrupts | grep " 9-fasteoi"
>
> This should output something like this:
>
> [root@...alhost ~]# cat /proc/interrupts | grep " 9-fasteoi"
>     9:          0          0          0          0   IO-APIC    9-fasteoi   acpi
>
> Repeat this a couple of times, of the numbers after the 9:
> increase (very) rapidly you have an interrupt storm. Likely
> they will either be fully unchanged or change very slowly.

Thanks a lot for the instructions. After a wakeup by USB keyboard, the
counter increased by a few hundred (compared to the value before
suspend), but there was no further growth, so it looks I'm safe.
(Tested with your patch, of course.)

> Note if nothing is output then IRQ 9 is not used on your
> model, then the INT0002 device cannot cause an interrupt storm.
>
> Regards,
>
> Hans
>
>
>
> >
> >> Cc: Maxim Mikityanskiy <maxtram95@...il.com>
> >> Cc: 5.3+ <stable@...r.kernel.org> # 5.3+
> >> Fixes: 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implement irq_set_wake on Bay Trail")
> >> Signed-off-by: Hans de Goede <hdegoede@...hat.com>
> >> ---
> >>   drivers/platform/x86/intel_int0002_vgpio.c | 18 +++++-------------
> >>   1 file changed, 5 insertions(+), 13 deletions(-)
> >>
> >> diff --git a/drivers/platform/x86/intel_int0002_vgpio.c b/drivers/platform/x86/intel_int0002_vgpio.c
> >> index 55f088f535e2..e8bec72d3823 100644
> >> --- a/drivers/platform/x86/intel_int0002_vgpio.c
> >> +++ b/drivers/platform/x86/intel_int0002_vgpio.c
> >> @@ -143,21 +143,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[] = {
> >>          INTEL_CPU_FAM6(ATOM_SILVERMONT, int0002_byt_irqchip),   /* Valleyview, Bay Trail  */
> >> -       INTEL_CPU_FAM6(ATOM_AIRMONT, int0002_cht_irqchip),      /* Braswell, Cherry Trail */
> >> +       INTEL_CPU_FAM6(ATOM_AIRMONT, int0002_byt_irqchip),      /* Braswell, Cherry Trail */
> >>          {}
> >>   };
> >>
> >> @@ -181,6 +169,10 @@ static int int0002_probe(struct platform_device *pdev)
> >>          if (!cpu_id)
> >>                  return -ENODEV;
> >>
> >> +       /* We only need to directly deal with PMEs when using s2idle */
> >> +       if (!pm_suspend_default_s2idle())
> >> +               return -ENODEV;
> >> +
> >>          irq = platform_get_irq(pdev, 0);
> >>          if (irq < 0)
> >>                  return irq;
> >> --
> >> 2.26.0
> >>
> >
>

Powered by blists - more mailing lists