Received: by 2002:a25:1985:0:0:0:0:0 with SMTP id 127csp1746120ybz; Thu, 16 Apr 2020 14:57:06 -0700 (PDT) X-Google-Smtp-Source: APiQypIKbXma2uTNWQr1TRwWSiMRChFWGizJ+6Z2dR/31hKnb5NSLtrVujyzb2H/l78PzPY9xfY0 X-Received: by 2002:a17:906:f295:: with SMTP id gu21mr110600ejb.83.1587074226616; Thu, 16 Apr 2020 14:57:06 -0700 (PDT) ARC-Seal: i=1; a=rsa-sha256; t=1587074226; cv=none; d=google.com; s=arc-20160816; b=OfI/zrVAiKTLW75h63885pHhzMv/debgm1VntjZYkOcePNi9ejQ6FQp9R2r4mHTaTc Dqi0J24kAL9Ahimh8KrS9l0EFeShtFxY3Es1rIjDvI1bQf/wj8MDM2m16IpHD9uFUaV5 PmnxnsfxRlz9bGrWmHPFP8+mfII9FuTaj8NrzBp5Z+mcZp0yv9dQd47yhXe2Qt9ujW3h CZk+rrXbJ8H1TgJb28gkBOw6zvzkyL37TkgFw8Bt4324BySmbXdLyTiEdsCX3aplI14G bScgfmR+Xw3PJ9hD2A+meRUB0N4a/gGF8FRVJUcIyLKzx4RgqGo3l7pQhP9pZsa3OD2J 5qPQ== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:sender:content-transfer-encoding:cc:to:subject :message-id:date:from:in-reply-to:references:mime-version :dkim-signature; bh=rAFWRmCauzapDqY6zF6wYDHpb+eX5UR9B/4Z3NEvnTc=; b=MPwwdFCrBqKC0BTdciU6eGBcRQVJI+Y7vfqdwgyuaUQPP5MOu4c61eTmIw9WCu8um9 qbI1Xv41evkrm8TUsX47pw3TBxKxMVo8xF23mdOLBlWUelxOhzCg5lMuo8fEPie9eNeO 9Xyqw9ZjIO2pLhwHAUjI3mbkSnKD6qNb3uiH0xb2DzJyMd023PzXgfNH0mVH9aUi66BR qnDcQutwT37U8lFbNZiHZCVDeRWcX9jy0jko4NnY94o/xqpHPhKTvscYKwFjp1mloa2+ UN6zF8xItQ3kzd16QdcQzlN3J4/tBVeBU1JOMqjlt0WtoU3DAZVXQpoB4isEjVeUAj91 uDOg== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=CNfYTVuK; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Return-Path: Received: from vger.kernel.org (vger.kernel.org. [23.128.96.18]) by mx.google.com with ESMTP id j23si7520153ejm.361.2020.04.16.14.56.43; Thu, 16 Apr 2020 14:57:06 -0700 (PDT) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18; Authentication-Results: mx.google.com; dkim=pass header.i=@gmail.com header.s=20161025 header.b=CNfYTVuK; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=QUARANTINE dis=NONE) header.from=gmail.com Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1728690AbgDPVzF (ORCPT + 99 others); Thu, 16 Apr 2020 17:55:05 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:50216 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-FAIL-OK-FAIL) by vger.kernel.org with ESMTP id S1728646AbgDPVzE (ORCPT ); Thu, 16 Apr 2020 17:55:04 -0400 Received: from mail-wr1-x443.google.com (mail-wr1-x443.google.com [IPv6:2a00:1450:4864:20::443]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id B9387C061A0C; Thu, 16 Apr 2020 14:55:03 -0700 (PDT) Received: by mail-wr1-x443.google.com with SMTP id k1so438573wrx.4; Thu, 16 Apr 2020 14:55:03 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20161025; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=rAFWRmCauzapDqY6zF6wYDHpb+eX5UR9B/4Z3NEvnTc=; b=CNfYTVuKIBIp6tZgQC803INlAGosjrnkwkNgVhBDqENWuNDr9mf4jy8DRNAGzy1rLJ s6pUD8o0PoziHcCdGenejLs5ji4NtbucA0Eace9Ru1Fr9SgytsHeMpwstkZukWu3VNws Wrm4g8cDoc17wSb5J+ItL0tQfL2a/x+0x8gLW3LHYaZ1A4MQTCTzF8jRu5DPCtdVFVfF TmejhNxo9ziq/n0Jh3ZxzqMbhy2kDutTBe2wDY7zwk+cslfkmD02o6j9mdUzadthe5dB lp37SHR8mi1NfGEro4U4JG+EMIL/yuNPE/d9EJDLGOIMlG3TcdZ4/2wxIm2Pxj7FCfa9 hA8w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=rAFWRmCauzapDqY6zF6wYDHpb+eX5UR9B/4Z3NEvnTc=; b=EL/9HOhHsgb3kJHL57n9LWK6igU0pHdtXuMHDwJyldXH57Fwc+2bHcFpjhDDzKE/U8 t7ub32pG/I7LXia9iJJZy2c9y8YAanRp1r8ptqBx5anfTbXlvRO7BMu/bdVjp+cHriXY VrL8w9uUobBm31P0T/Vulmzb2MZdLoLXfxElPSh/IrsxnGXbDTzEkVXUsMJSZz5M5kGB nZnXA27EAvx8B7ebol6ghv/Mg2POTgclj3d6xUAT7/W2Si4b6Gc0S2hFHhib3Svkd8eE Chj1eQG2A6xJKbHFnmKscUhojqBJk9rdew6INUKpheSqwI3XWf+/9GCBxMAj6EA2HwXb 3MtA== X-Gm-Message-State: AGi0PubwXqgTdTsdueD2y4u5FEkIiB6o8HsTwGs+oCxEuBSs+PbOZu1g AwWlHD7lWmOcB5ARXljL7EfhvuUjDi6+tnxsLcY= X-Received: by 2002:adf:f146:: with SMTP id y6mr361227wro.132.1587074102376; Thu, 16 Apr 2020 14:55:02 -0700 (PDT) MIME-Version: 1.0 References: <20200407213058.62870-1-hdegoede@redhat.com> In-Reply-To: From: Maxim Mikityanskiy Date: Fri, 17 Apr 2020 00:54:36 +0300 Message-ID: Subject: Re: [PATCH] platform/x86: intel_int0002_vgpio: Only bind to the INT0002 dev when using s2idle To: Hans de Goede Cc: Darren Hart , Andy Shevchenko , Platform Driver , Linux Kernel Mailing List , "5 . 3+" Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable Sender: linux-kernel-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Apr 14, 2020 at 4:24 PM Hans de Goede wrote: > > Hi, > > On 4/8/20 2:11 PM, Maxim Mikityanskiy wrote: > > On Wed, Apr 8, 2020 at 12:31 AM Hans de Goede wro= te: > >> > >> Commit 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implemen= t > >> irq_set_wake on Bay Trail") stopped passing irq_set_wake requests on t= o > >> 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 wakeu= p > >> 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 t= o > >> 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 causin= g > >> 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 th= is > >> 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 u= sed. > >> 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 =E2=80=94 all seem to work fine. So, if appro= priate: > > > > Tested-by: Maxim Mikityanskiy > > 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 =E2=80=94 does it never happen on Braswell? What are the reproduct= ion > > 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@localhost ~]# 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 > >> Cc: 5.3+ # 5.3+ > >> Fixes: 871f1f2bcb01 ("platform/x86: intel_int0002_vgpio: Only implemen= t irq_set_wake on Bay Trail") > >> Signed-off-by: Hans de Goede > >> --- > >> 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/plat= form/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 =3D { > >> .irq_set_wake =3D int0002_irq_set_wake, > >> }; > >> > >> -static struct irq_chip int0002_cht_irqchip =3D { > >> - .name =3D DRV_NAME, > >> - .irq_ack =3D int0002_irq_ack, > >> - .irq_mask =3D int0002_irq_mask, > >> - .irq_unmask =3D int0002_irq_unmask, > >> - /* > >> - * No set_wake, on CHT the IRQ is typically shared with the AC= PI SCI > >> - * and we don't want to mess with the ACPI SCI irq settings. > >> - */ > >> - .flags =3D IRQCHIP_SKIP_SET_WAKE, > >> -}; > >> - > >> static const struct x86_cpu_id int0002_cpu_ids[] =3D { > >> INTEL_CPU_FAM6(ATOM_SILVERMONT, int0002_byt_irqchip), /* Va= lleyview, Bay Trail */ > >> - INTEL_CPU_FAM6(ATOM_AIRMONT, int0002_cht_irqchip), /* Bra= swell, Cherry Trail */ > >> + INTEL_CPU_FAM6(ATOM_AIRMONT, int0002_byt_irqchip), /* Bra= swell, 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 =3D platform_get_irq(pdev, 0); > >> if (irq < 0) > >> return irq; > >> -- > >> 2.26.0 > >> > > >