From mboxrd@z Thu Jan 1 00:00:00 1970 From: Thomas Gleixner Subject: Re: [PATCH] genirq: Respect IRQCHIP_SKIP_SET_WAKE in irq_chip_set_wake_parent() Date: Thu, 21 Mar 2019 10:26:26 +0100 (CET) Message-ID: References: <20190315200759.139479-1-swboyd@chromium.org> Mime-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Return-path: In-Reply-To: <20190315200759.139479-1-swboyd@chromium.org> Sender: linux-kernel-owner@vger.kernel.org To: Stephen Boyd Cc: linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org, Lina Iyer , Marc Zyngier List-Id: linux-gpio@vger.kernel.org On Fri, 15 Mar 2019, Stephen Boyd wrote: > This function returns an error if a child interrupt controller calls > irq_chip_set_wake_parent() but that parent interrupt controller has the > IRQCHIP_SKIP_SET_WAKE flag. Let's return 0 for success instead because > there isn't anything to do. > > There's also the possibility that a parent indicates that we should skip > it, but the grandparent has an .irq_set_wake callback. Let's iterate > through the parent chain as long as the IRQCHIP_SKIP_SET_WAKE flag isn't > set so we can find the first parent that needs to handle the wake > configuration. This fixes a problem on my Qualcomm sdm845 device where > I'm trying to enable wake on an irq from the gpio controller that's a > child of the qcom pdc interrupt controller. The qcom pdc interrupt > controller has the IRQCHIP_SKIP_SET_WAKE flag set, and so does the > grandparent (ARM GIC), causing this function to return a failure because > the parent controller doesn't have the .irq_set_wake callback set. It took me some time to distangle that changelog.... and I don't think that this is the right thing to do. set_irq_wake_real() returns 0 when the topmost chip has SKIP_SET_WAKE set. So let's assume we have the following chains: chip A -> chip B chip A -> chip B -> chip C chip A has SKIP_SET_WAKE not set chip B has SKIP_SET_WAKE set chip C has SKIP_SET_WAKE not set and invokes irq_chip_set_wake_parent() Now assume we have interrupt X connected to chip B and interrupt Y connected to chip C. If irq_set_wake() is called for interrupt X, then the function returns without trying to invoke the set_wake() callback of chip A. If irq_set_wake() is called for interrupt Y, irq_chip_set_wake_parent() is invoked from chip C which then skips chip B, but tries to invoke the callback on chip A. That's inconsistent and changes the existing behaviour. So IMO, the right thing to do is to return 0 from irq_chip_set_wake_parent() when the parent has SKIP_SET_WAKE set and not to try to follow the whole chain. That should fix your problem nicely w/o changing behaviour. Thanks, tglx ---- diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index 3faef4a77f71..51128bea3846 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -1449,6 +1449,10 @@ int irq_chip_set_vcpu_affinity_parent(struct irq_data *data, void *vcpu_info) int irq_chip_set_wake_parent(struct irq_data *data, unsigned int on) { data = data->parent_data; + + if (data->chip->flags & IRQCHIP_SKIP_SET_WAKE) + return 0; + if (data->chip->irq_set_wake) return data->chip->irq_set_wake(data, on); From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.0 required=3.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,HEADER_FROM_DIFFERENT_DOMAINS,INCLUDES_PATCH,MAILING_LIST_MULTI, SPF_PASS,URIBL_BLOCKED autolearn=unavailable autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 2191CC43381 for ; Thu, 21 Mar 2019 09:26:46 +0000 (UTC) Received: from bombadil.infradead.org (bombadil.infradead.org [198.137.202.133]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.kernel.org (Postfix) with ESMTPS id E57BC218A2 for ; Thu, 21 Mar 2019 09:26:45 +0000 (UTC) Authentication-Results: mail.kernel.org; dkim=pass (2048-bit key) header.d=lists.infradead.org header.i=@lists.infradead.org header.b="c78xoIwy" DMARC-Filter: OpenDMARC Filter v1.3.2 mail.kernel.org E57BC218A2 Authentication-Results: mail.kernel.org; dmarc=none (p=none dis=none) header.from=linutronix.de Authentication-Results: mail.kernel.org; spf=none smtp.mailfrom=linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20170209; h=Sender: Content-Transfer-Encoding:Content-Type:Cc:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:References:Message-ID: In-Reply-To:Subject:To:From:Date:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=szgNblDjXNSo6YEQKukDl7IVRkvc+rle13vPZitW31o=; b=c78xoIwyYY4Dcr f4iAA9oeUoy8ZK53I27TpzG+uh7AntRsVZZPVMxu0CbPw0pQLsrL31yF6hz+fO0YA5JEtilf5RwWU 2kgfmLNsHYuMjYi59d13dQRQKOEii7sAkdJTedttBNrEXG0LIlPMStPSgJMs2oY1IYt6AvlzeqMwo iYP+o9UtEbzLeSULQcU7hUaCXQPZgl315j//LDpZMLjBk0Z/D903jpUSBIFcpf7UnMclwa31h0G4G +wERPVQJthOyhZNWGXiLgMjc8VYC29Cg3FbYkAdn2eu7LUmfibPTKqZr7BltZzfWJwP9jKgCdwbDG tC1ReoC3EjVBktnXZsmg==; Received: from localhost ([127.0.0.1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.90_1 #2 (Red Hat Linux)) id 1h6tyE-0007vm-UL; Thu, 21 Mar 2019 09:26:42 +0000 Received: from galois.linutronix.de ([2a01:7a0:2:106d:700::1]) by bombadil.infradead.org with esmtps (Exim 4.90_1 #2 (Red Hat Linux)) id 1h6tyA-0007uv-Gs for linux-arm-kernel@lists.infradead.org; Thu, 21 Mar 2019 09:26:40 +0000 Received: from p5492e2fc.dip0.t-ipconnect.de ([84.146.226.252] helo=nanos) by Galois.linutronix.de with esmtpsa (TLS1.2:DHE_RSA_AES_256_CBC_SHA256:256) (Exim 4.80) (envelope-from ) id 1h6ty5-0008SI-4o; Thu, 21 Mar 2019 10:26:33 +0100 Date: Thu, 21 Mar 2019 10:26:26 +0100 (CET) From: Thomas Gleixner To: Stephen Boyd Subject: Re: [PATCH] genirq: Respect IRQCHIP_SKIP_SET_WAKE in irq_chip_set_wake_parent() In-Reply-To: <20190315200759.139479-1-swboyd@chromium.org> Message-ID: References: <20190315200759.139479-1-swboyd@chromium.org> User-Agent: Alpine 2.21 (DEB 202 2017-01-01) MIME-Version: 1.0 X-Linutronix-Spam-Score: -1.0 X-Linutronix-Spam-Level: - X-Linutronix-Spam-Status: No , -1.0 points, 5.0 required, ALL_TRUSTED=-1, SHORTCIRCUIT=-0.0001 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20190321_022638_705796_64431DD8 X-CRM114-Status: GOOD ( 19.49 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.21 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Cc: Marc Zyngier , linux-gpio@vger.kernel.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Lina Iyer Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+infradead-linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Fri, 15 Mar 2019, Stephen Boyd wrote: > This function returns an error if a child interrupt controller calls > irq_chip_set_wake_parent() but that parent interrupt controller has the > IRQCHIP_SKIP_SET_WAKE flag. Let's return 0 for success instead because > there isn't anything to do. > > There's also the possibility that a parent indicates that we should skip > it, but the grandparent has an .irq_set_wake callback. Let's iterate > through the parent chain as long as the IRQCHIP_SKIP_SET_WAKE flag isn't > set so we can find the first parent that needs to handle the wake > configuration. This fixes a problem on my Qualcomm sdm845 device where > I'm trying to enable wake on an irq from the gpio controller that's a > child of the qcom pdc interrupt controller. The qcom pdc interrupt > controller has the IRQCHIP_SKIP_SET_WAKE flag set, and so does the > grandparent (ARM GIC), causing this function to return a failure because > the parent controller doesn't have the .irq_set_wake callback set. It took me some time to distangle that changelog.... and I don't think that this is the right thing to do. set_irq_wake_real() returns 0 when the topmost chip has SKIP_SET_WAKE set. So let's assume we have the following chains: chip A -> chip B chip A -> chip B -> chip C chip A has SKIP_SET_WAKE not set chip B has SKIP_SET_WAKE set chip C has SKIP_SET_WAKE not set and invokes irq_chip_set_wake_parent() Now assume we have interrupt X connected to chip B and interrupt Y connected to chip C. If irq_set_wake() is called for interrupt X, then the function returns without trying to invoke the set_wake() callback of chip A. If irq_set_wake() is called for interrupt Y, irq_chip_set_wake_parent() is invoked from chip C which then skips chip B, but tries to invoke the callback on chip A. That's inconsistent and changes the existing behaviour. So IMO, the right thing to do is to return 0 from irq_chip_set_wake_parent() when the parent has SKIP_SET_WAKE set and not to try to follow the whole chain. That should fix your problem nicely w/o changing behaviour. Thanks, tglx ---- diff --git a/kernel/irq/chip.c b/kernel/irq/chip.c index 3faef4a77f71..51128bea3846 100644 --- a/kernel/irq/chip.c +++ b/kernel/irq/chip.c @@ -1449,6 +1449,10 @@ int irq_chip_set_vcpu_affinity_parent(struct irq_data *data, void *vcpu_info) int irq_chip_set_wake_parent(struct irq_data *data, unsigned int on) { data = data->parent_data; + + if (data->chip->flags & IRQCHIP_SKIP_SET_WAKE) + return 0; + if (data->chip->irq_set_wake) return data->chip->irq_set_wake(data, on); _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel