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 2BF49C3DA45 for ; Sat, 13 Jul 2024 19:56:56 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=lists.infradead.org; s=bombadil.20210309; h=Sender:List-Subscribe:List-Help :List-Post:List-Archive:List-Unsubscribe:List-Id:Content-Type:MIME-Version: Message-ID:Date:References:In-Reply-To:Subject:Cc:To:From:Reply-To: Content-Transfer-Encoding:Content-ID:Content-Description:Resent-Date: Resent-From:Resent-Sender:Resent-To:Resent-Cc:Resent-Message-ID:List-Owner; bh=gRPdv/ftOLQuVeXznl0lu98TbjXcYSwLjTuncwy+WfI=; b=FB09ZucPhkkXZAuMBjImQ4WjXq c7CGLu2GYMfEAYvDN8gdCioRdLMGQ/hKGmOkJ2KHvLZHFNfZMHGz6UpSdAELSd2qkMVBfjSks5UU0 LPOxTzcRbjrD8pFg7TGCnko+HSUi5/euh7F24K5b7u41LGwGNXg2g8U6rKRoRznXKgcd6hJkqYvFW lOGRzUpy/VRJIPCHXUSYQXhNr7z49D2JDPU13TdAmaKd4AcLpPZGZzfJbrahehHwGQlyD9GqL3fsz IcSH2DGXMfxJrEbhh7AAJK/EXE+IG42Gfmi+9ACJOqTZK6AvZzeqVgD+44u7Fs+jXij4N9zsESRtl QcXbtZ8Q==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.97.1 #2 (Red Hat Linux)) id 1sSirH-000000036To-23xO; Sat, 13 Jul 2024 19:56:39 +0000 Received: from galois.linutronix.de ([193.142.43.55]) by bombadil.infradead.org with esmtps (Exim 4.97.1 #2 (Red Hat Linux)) id 1sSiqx-000000036Pf-2URL for linux-arm-kernel@lists.infradead.org; Sat, 13 Jul 2024 19:56:22 +0000 From: Thomas Gleixner DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020; t=1720900577; 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=gRPdv/ftOLQuVeXznl0lu98TbjXcYSwLjTuncwy+WfI=; b=MzsURIn1hxXiP39jY6qaf6Kw64T3nTcB6KjMuZ57f0iUpKpnSlJpCtWXET6zpsPo/b5SB3 0gt5OtGSr/KYdxMoDw73ULKZMaVsnGAOgDDg8UDBd3czcL2Nr3kzEcLSkrqXizKpy6v6qV D5i1g1SiErlYPP2+in9iEwrfw82TeeZVtGe02Uxd7dVvEfcBWMDwxi3NSoqqr7wan3nTKx U6/poGJLwFeB+cD/XWY9wYUSY3V+F3ywaiMObzj/bcAMOR9qROixkXrycheVKMAqdL/XwS za9oRMbjjgccMB0FebenhbZMf4yCtph63aLrFoc0+hyryx1yOdJTJTk/h3O7qg== DKIM-Signature: v=1; a=ed25519-sha256; c=relaxed/relaxed; d=linutronix.de; s=2020e; t=1720900577; 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=gRPdv/ftOLQuVeXznl0lu98TbjXcYSwLjTuncwy+WfI=; b=YvThdUVPdGiWN25ynNS8ChfwwK3EhshdhosV3nk2ahuT8EybEU1IkiOQe/OIxboDqvtu5Y /ZUlAPd032O/HoDg== To: Bjorn Helgaas Cc: Marek =?utf-8?Q?Beh=C3=BAn?= , Lorenzo Pieralisi , Krzysztof =?utf-8?Q?Wilczy=C5=84ski?= , Bjorn Helgaas , Andrew Lunn , Gregory Clement , Thomas Petazzoni , Rob Herring , linux-pci@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Manivannan Sadhasivam , Pali =?utf-8?Q?Roh=C3=A1r?= , Marc Zyngier Subject: Re: [PATCH v2] PCI: mvebu: Dispose INTx IRQs before to removing INTx domain In-Reply-To: <20240713193200.GA374767@bhelgaas> References: <20240713193200.GA374767@bhelgaas> Date: Sat, 13 Jul 2024 21:56:16 +0200 Message-ID: <878qy5rrq7.ffs@tglx> MIME-Version: 1.0 Content-Type: text/plain X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20240713_125619_799159_EB91E461 X-CRM114-Status: GOOD ( 22.44 ) 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: , Sender: "linux-arm-kernel" Errors-To: linux-arm-kernel-bounces+linux-arm-kernel=archiver.kernel.org@lists.infradead.org On Sat, Jul 13 2024 at 14:32, Bjorn Helgaas wrote: > On Sat, Jul 13, 2024 at 12:33:29PM +0200, Thomas Gleixner wrote: >> > There are 20+ drivers in drivers/pci/controller, and I don't see >> > irq_dispose_mapping() usage similar to this elsewhere. Does that mean >> > most or all of the other drivers have a similar defect? >> >> Right. >> >> But the real question is why is such a mapping not torn down by the >> entity (device, bridge, whatever) which set it up in the first place? > > Marek/Pali, the commit log mentions a crash when unloading. Do you > have a pointer to any details? Maybe there's a driver there that we > can fix? Pali already explained it in his reply to me, but for some stupid reason (my fault hitting reply instead of reply-all) this did not make the list. Let me replay the discussion: >>> Pali: >>> I'm not expert neither. But IIRC it is because endpoint drivers do not >>> call irq_dispose_mapping at their own for shared IRQs. And this is how >>> shared PCI INTA-D interrupts are implemented in the kernel. First >>> endpoint driver (e.g. wifi card) request for shared interrupt and kernel >>> automatically creates irq mapping if it does not exist. Second endpoint >>> driver (e.g. second wifi card) request for shared interrupt and kernel >>> just returns existing mapping. And when the first endpoint driver is >>> releasing its own irq handler it do not dispose irq mapping because it >>> may be shared with other endpoint drivers (in this case with second wifi >>> card). Seems that the owner of these shared mappings is the PCI >>> controller driver and it has to do dispose them on its own removal (when >>> releasing the domain for shared PCI IRQs). >> tglx: >> Working around this in a particular PCI controller driver is just wrong >> then. This wants a common cleanup function so all affected drivers which >> remove the INTX irqdomain can be fixed up without copy & pasta of the >> very same code. Something like the below and then change the >> irq_domain_remove() call for the INTX domain to use that function. There was some additional back and forth but the actual patch which can be used for both INTX and other, e.g. error reporting, domains is below. Sorry for taking this offlist unintenitonally. The background of all this is that initially PCI[e] controllers were not removable/modular. Later on the whole modularization effort created this problem. Thanks, tglx --- --- a/drivers/pci/irq.c +++ b/drivers/pci/irq.c @@ -278,3 +278,19 @@ int __weak pcibios_alloc_irq(struct pci_ void __weak pcibios_free_irq(struct pci_dev *dev) { } + +#ifdef CONFIG_IRQDOMAIN +void pci_remove_irqdomain(struct irqdomain *domain, unsigned int nr_irqs) +{ + /* + * Comment explaining the oddity of this. + */ + for (unsigned int i = 0; j < nr_irqs; i++) { + int virq = irq_find_mapping(domain, i); + + if (virq > 0) + irq_dispose_mapping(virq); + } + irq_domain_remove(domain); +} +#endif