From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id 6AFF02248AC; Thu, 6 Feb 2025 09:04:24 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1738832665; cv=none; b=k5f2ZJqcWKC85MLfzBZ0AhrPTS426OTe2Wr/+JYWP6SAlakVPGEq2aLFoF64o19iUTkkn2fI5wGB/IXD5QMojn1lvYrokHPPNE0VK8bYzuYXhQ4vZBhLDi5tN4nPNFcrAyKHs7umcl5uJAkOH2xpIEIyRFrZSKd6jzzmf7Y8/Qk= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1738832665; c=relaxed/simple; bh=8eG7UI0hfNWaK2mXqVw5c7deZzv7yKrvlpZtUeX6xgg=; h=Date:Message-ID:From:To:Cc:Subject:In-Reply-To:References: MIME-Version:Content-Type; b=h4lTtzeMiY8MOZ1CMDJHkKAavAS8WORkNJl02zGm4xz1TtBJRr3JOgOXUZ9sCnbx1AGQrhjn/7Xbe8fwWjlBcaYx0C/3ZAS6D8sCR1AB/RxzMxMMzhKuYqmn+QpWLXPD4P+alzsR22N7xfQQLlUa60WozTcMt3YmclbFE0XRhcs= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=CE1JmQ18; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="CE1JmQ18" Received: by smtp.kernel.org (Postfix) with ESMTPSA id CA6CFC4CEDD; Thu, 6 Feb 2025 09:04:24 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1738832664; bh=8eG7UI0hfNWaK2mXqVw5c7deZzv7yKrvlpZtUeX6xgg=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=CE1JmQ18aDBlvGcwxkqEg7l1m7hdhTlmxC/Rt9EvPBJTNPVq4OvliUkuddk0ZZG/u CgToeBS0lriBfdZzuqOFNwUVM12L7oRInzTwgYeld64fFtqogOQaDPjCH7tmU4WXQp UqaBJ8pnWuWXahnm3KmmcTBBdxRmRfjLyhdEALFibRx4IM2bsJNx+ARU1LhrBWMuCw pqLzCNkvHTi+0Xcso4/8ZGXqrxHyNRoqQjKadxBB6yeKntWgiArn5krn9xympsgn4z TE5lJSvIEDLFuO2gwa1EUjt097VHw5SXr3Xj764+7opLuPWWJ1r/MjH/vClXpFHMS0 SFp/VKZQ+7zjg== Received: from 82-132-233-249.dab.02.net ([82.132.233.249] helo=wait-a-minute.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1tfxo5-0013cQ-BO; Thu, 06 Feb 2025 09:04:22 +0000 Date: Thu, 06 Feb 2025 09:04:00 +0000 Message-ID: <87ed0btpfj.wl-maz@kernel.org> From: Marc Zyngier To: Brian Norris Cc: Jingoo Han , Manivannan Sadhasivam , linux-pci@vger.kernel.org, Rob Herring , Lorenzo Pieralisi , Krzysztof =?UTF-8?B?V2lsY3p5xYRza2k=?= , Bjorn Helgaas , linux-kernel@vger.kernel.org, Brian Norris Subject: Re: [PATCH v2] PCI: dwc: Use level-triggered handler for MSI IRQs In-Reply-To: <20250205151635.v2.1.Id60295bee6aacf44aa3664e702012cb4710529c3@changeid> References: <20250205151635.v2.1.Id60295bee6aacf44aa3664e702012cb4710529c3@changeid> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.4 (x86_64-pc-linux-gnu) MULE/6.0 (HANACHIRUSATO) Precedence: bulk X-Mailing-List: linux-kernel@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=US-ASCII X-SA-Exim-Connect-IP: 82.132.233.249 X-SA-Exim-Rcpt-To: briannorris@chromium.org, jingoohan1@gmail.com, manivannan.sadhasivam@linaro.org, linux-pci@vger.kernel.org, robh@kernel.org, lpieralisi@kernel.org, kw@linux.com, bhelgaas@google.com, linux-kernel@vger.kernel.org, briannorris@google.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false On Wed, 05 Feb 2025 23:16:36 +0000, Brian Norris wrote: > > From: Brian Norris > > Per Synopsis's documentation [1], the msi_ctrl_int signal is > level-triggered, not edge-triggered. > > The use of handle_edge_trigger() was chosen in commit 7c5925afbc58 > ("PCI: dwc: Move MSI IRQs allocation to IRQ domains hierarchical API"), > which actually dropped preexisting use of handle_level_trigger(). > Looking at the patch history, this change was only made by request: > > Subject: Re: [PATCH v6 1/9] PCI: dwc: Add IRQ chained API support > https://lore.kernel.org/all/04d3d5b6-9199-218d-476f-c77d04b8d2e7@arm.com/ > > "Are you sure about this "handle_level_irq"? MSIs are definitely edge > triggered, not level." > > However, while the underlying MSI protocol is edge-triggered in a sense, > the DesignWare IP is actually level-triggered. You are confusing two things: - MSIs are edge triggered. No ifs, no buts. That's because you can't "unwrite" something. Even the so-called level-triggered MSIs are build on a pair of edges (one up, one down). - The DisgustWare IP multiplexes MSIs onto a single interrupt, and *latches* them, presenting a level sensitive signal *for the latch*. Not for the MSIs themselves. > > So, let's switch back to level-triggered. > > In many cases, the distinction doesn't really matter here, because this > signal is hidden behind another (chained) top-level IRQ which can be > masked on its own. But it's still wise to manipulate this interrupt line > according to its actual specification -- specifically, to mask it while > we handle it. The distinction absolutely matters, because you are not dealing with the actual MSIs, but with a latch. > > [1] From: > > DesignWare Cores PCI Express RP Controller Reference Manual > Version 6.00a, June 2022 > Section 2.89 MSI Interface Signals > > msi_ctrl_int is described as: > > "Asserted when an MSI interrupt is pending. De-asserted when there is > no MSI interrupt pending. > ... > Active State: High (level)" > > It also points at the databook for more info. One relevant excerpt from > the databook: > > DesignWare Cores PCI Express Controller Databook > Version 6.00a, June 2022 > Section 3.9.2.3 iMSI-RX: Integrated MSI Receiver [AXI Bridge] > > "When any status bit remains set, then msi_ctrl_int remains asserted. > The interrupt status register provides a status bit for up to 32 > interrupt vectors per Endpoint. When the decoded interrupt vector is > enabled but is masked, then the controller sets the corresponding bit > in interrupt status register but the it [sic] does not assert the > top-level controller output msi_ctrl_int. Key word: *output*. That is the level-triggered line. Not the MSIs, which are *input* signals to the mux. > > Signed-off-by: Brian Norris > Signed-off-by: Brian Norris > --- > > Changes in v2: > * add documentation references > > drivers/pci/controller/dwc/pcie-designware-host.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/drivers/pci/controller/dwc/pcie-designware-host.c b/drivers/pci/controller/dwc/pcie-designware-host.c > index ffaded8f2df7..89a1207754d3 100644 > --- a/drivers/pci/controller/dwc/pcie-designware-host.c > +++ b/drivers/pci/controller/dwc/pcie-designware-host.c > @@ -198,7 +198,7 @@ static int dw_pcie_irq_domain_alloc(struct irq_domain *domain, > for (i = 0; i < nr_irqs; i++) > irq_domain_set_info(domain, virq + i, bit + i, > pp->msi_irq_chip, > - pp, handle_edge_irq, > + pp, handle_level_irq, > NULL, NULL); > > return 0; I don't buy this, at least not without further justification based on the programming model of the mux. It also breaks the semantics of interrupt being made pending while we were handling them (retrigger being one). M. -- Without deviation from the norm, progress is not possible.