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 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 smtp.lore.kernel.org (Postfix) with ESMTPS id DEA86C4332F for ; Tue, 12 Dec 2023 19:33:20 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender: Content-Transfer-Encoding:Content-Type:List-Subscribe:List-Help:List-Post: List-Archive:List-Unsubscribe:List-Id:MIME-Version:Message-ID:Date:References :In-Reply-To:Subject:Cc:To:From:Reply-To:Content-ID:Content-Description: Resent-Date:Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID: List-Owner; bh=3ZjVX/nJn1bSBFZr1MIL5Jcd9uDs1Um6/RVlp2W2Td8=; b=B61u1N9hcdkQXx Y2RCg2dByReVc/QnPAobIxB3l44Kokm+l+FkQFqmCtLUsm6HnMYyL1NM9x8fz3qk+QAGyRWvAfZdo FGyiqWkfbDfqPTI+EoSFq+XRkJsA+QsYilNbhfV7c/N3SYly94fGy6j5XQu/ZxZo9EufSPvFcBqR/ aP91E+OdCvQzlpOE17NFGTAbgMBnLhQc0FjzopabcJbCgdxYtz0A8x1k/df9WzhyaJSCHct1Qo6o7 dodtqvsLs2HVshRlCLCYH4wgupnVKBfqq4137hWYYHJTXpARKUXXLVLvCBiqB0RlTrgaLEvKEk8ro /zA++tCSPBVAT3SeDyjA==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.96 #2 (Red Hat Linux)) id 1rD8Uz-00Cbx9-2v; Tue, 12 Dec 2023 19:32:57 +0000 Received: from galois.linutronix.de ([2a0a:51c0:0:12e:550::1]) by bombadil.infradead.org with esmtps (Exim 4.96 #2 (Red Hat Linux)) id 1rD8Ux-00Cbux-04 for linux-arm-kernel@lists.infradead.org; Tue, 12 Dec 2023 19:32:56 +0000 From: Thomas Gleixner DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1702409571; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=yAuQflHlpP7PQHRCxurGh8vd3qGPf7EP4mCw7mI4KPo=; b=pfPa+PgC45CXpOi9IvcThsAxhVtXutuSJaSBreWJQmzOBlJH16M51b2bMcjpbVbRDay/hP GiKvtTpwrl9dA0pTYXhgG5O0yXMsOCNC3oyY8Q9J04ILzUOyWQUeVB5wJzpOhgPa+gW2Je dhEk9ttMmhkS7CVo4GUV21hz8aABq9rp5slulmo5N6clvfy/h3D+dNkEuLKlLDjxLNbjoL DfpNWRgYZo1RZ0j8o+7bnBkZu3eODUUU77FBBs9Tg0RNZ6huPfPnDUOMMzJVoR4wD/0GoO KKjeIZbpOGMklxvmH8qYkPU0lblYu4YX+gv2TmvK8vmVyOkasYNVUzISPi2+bQ== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1702409571; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=yAuQflHlpP7PQHRCxurGh8vd3qGPf7EP4mCw7mI4KPo=; b=qqf0D/thwWl4/VatQWveBxWB+9Ei1hdZ4zfCGMXPgZFxuCV59ZD06fPGWd1NUTVm2wTNfe 4FKztjP2ODZOuPBg== To: Ben Wolsieffer Cc: linux-kernel@vger.kernel.org, linux-stm32@st-md-mailman.stormreply.com, linux-arm-kernel@lists.infradead.org, linux-gpio@vger.kernel.org, Maxime Coquelin , Alexandre Torgue , Linus Walleij Subject: Re: [PATCH 2/2] pinctrl: stm32: fix GPIO level interrupts In-Reply-To: References: <20231204203357.2897008-1-ben.wolsieffer@hefring.com> <20231204203357.2897008-3-ben.wolsieffer@hefring.com> <87ttosqvbq.ffs@tglx> Date: Tue, 12 Dec 2023 20:32:50 +0100 Message-ID: <87il53p671.ffs@tglx> MIME-Version: 1.0 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20231212_113255_213959_800AE7C1 X-CRM114-Status: GOOD ( 30.54 ) X-BeenThere: linux-arm-kernel@lists.infradead.org X-Mailman-Version: 2.1.34 Precedence: list List-Id: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org Ben! On Tue, Dec 12 2023 at 10:35, Ben Wolsieffer wrote: > On Fri, Dec 08, 2023 at 09:43:21PM +0100, Thomas Gleixner wrote: >> On Mon, Dec 04 2023 at 15:33, Ben Wolsieffer wrote: >> > The STM32 doesn't support GPIO level interrupts in hardware, so the >> > driver tries to emulate them using edge interrupts, by retriggering the >> > interrupt if necessary based on the pin state after the handler >> > finishes. >> > >> > Currently, this functionality does not work because the irqchip uses >> > handle_edge_irq(), which doesn't run the irq_eoi() or irq_unmask() >> > callbacks after handling the interrupt. This patch fixes this by using >> > handle_level_irq() for level interrupts, which causes irq_unmask() to be >> > called to retrigger the interrupt. >> >> This does not make any sense at all. irq_unmask() does not retrigger >> anything. It sets the corresponding bit in the mask register, not more >> not less. > > I don't think this is correct. I was referring to > stm32_gpio_irq_unmask(), which calls stm32_gpio_irq_trigger(), which in > turn (for level interrupts) checks the GPIO pin state and retriggers the > interrupt if necessary. Ah. That makes a lot more sense. Sorry that I missed that driver detail. The changelog could mention explicitely where the retrigger comes from. >> Switching to handle_level_irq() makes the following difference >> vs. handle_edge_irq() when an interrupt is handled (ignoring the inner >> loop): >> >> + irq_mask(); >> irq_ack(); >> .... >> handle(); >> .... >> + irq_unmask(); > > Yes, the additional call to irq_unmask() is the key difference here, as > that callback performs the retriggering for level interrupts. Sorry to be pedantic here. irq_unmask() is a function in the interrupt core code which eventually ends up invoking the irqchip::irq_unmask(). >> When the interrupt is raised again after irq_ack() while the handler is >> running, i.e. a full toggle from active to inactive and back to active >> where the back to active transition causes the edge detector to trigger, >> then: > > I don't see how this is relevant. The bug occurs with level interrupts > in the case where there are no new transitions while the handler is > running. For example, with a high level interrupt, if the pin is still > high after the handler finishes, the interrupt should be immediately > triggered again. Ah. That's the problem you are trying to solve. Now we are getting closer to a proper description :) >> But in fact the regular exti driver could do the same and just handle >> the two NVIC interrupts which need demultiplexing separately and let >> everything else go through the hierarchy without bells and whistles. > > This sounds reasonable to me. It did seem strange to me that the exti > and exti_h drivers used such different approaches, although I wasn't > aware of the reasons behind them. I think this refactoring is out of > scope of this bug fix though. Sure. It just occured to me while looking at this stuff.. Care to resend with a proper explanation in the changelog? Thanks, tglx _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel