Received: by 2002:a05:7412:419a:b0:f3:1519:9f41 with SMTP id i26csp4752748rdh; Wed, 29 Nov 2023 09:38:11 -0800 (PST) X-Google-Smtp-Source: AGHT+IHalc/vhoXQcr9NxtV0rb8OdwDbVJN6gf6ByhwLFPRwe2K5eSUoJ3vf4Vi45Rp4xYYoS5k4 X-Received: by 2002:a05:6a00:3a17:b0:6bd:2c0a:e82 with SMTP id fj23-20020a056a003a1700b006bd2c0a0e82mr22429330pfb.7.1701279491017; Wed, 29 Nov 2023 09:38:11 -0800 (PST) ARC-Seal: i=1; a=rsa-sha256; t=1701279491; cv=none; d=google.com; s=arc-20160816; b=tat3firdU04PRAb/0IZ1+tRgRIPOSL+jUY+OkV6WhXJ6e9Av4khbnnE+ADhwsT/4yL 9PVFf2YjIKmKLNNd+M4viL0iIO4n/OW28RMNd35V2BuDzS3Pt9us1ZlHDUuyLddslscH Ap03KR2RaPoov2L9xXjXFUyObPmGQeTB2HJYm0+Y/OmlmXXc52eI20/uvS6OMoaADp76 871j5lVMyXYYQvnzOzFQUzjC13dCWpNAsxRou1kiRp/nOTVnZuSus9+lPlQ7Z+EisDAs 1Yu1Oz0zpNs7n1VeyZ6Ey8fCw7I5nSBs52AcbVJn0/QiaDYvPsG9IwGbayJ5N6Fx2XOn rmgA== ARC-Message-Signature: i=1; a=rsa-sha256; c=relaxed/relaxed; d=google.com; s=arc-20160816; h=list-id:precedence:content-transfer-encoding:subject:cc:to:from :date:references:in-reply-to:message-id:mime-version:user-agent :feedback-id:dkim-signature; bh=g7Dwl/yvBVmqD7aOBty7w1MPh982nepDmPiRm+Q1g/0=; fh=iY8cDVbwK9y1/9jK2weIE5VrB3MActfwEi2E38c/VBc=; b=ZELk4lhiLP6cXgMQqtqYGvvMYyjgXWIvtnqUjiyuZArrE1yO6g6nKpRTlKKM5GF86W Lza1jiNSQp+3FFU5vO/aOO2gps4EvAA0DDAq9lA0z05uEFvFZiRJinjZNs/yz/o79kCt F24FaH8LIciq8Dj7j+Tdljgjl5reifaAIa283/pUWOG4IXXRZqna/ifc+H/I0iW9x30C LiTXEOWFAm/i0rTrci0/TgoH/PMVtWY3kTk57C1crJNtjbCpL+19T29NEsHI0pMWADj4 25HNbVeH1lUSJSX+lTjWwnNypx+gQU2MzHSCAZwO+nPA81BCYPInNLIVEEp1CsRfczlX Forw== ARC-Authentication-Results: i=1; mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=bhfR2UoU; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Return-Path: Received: from pete.vger.email (pete.vger.email. [23.128.96.36]) by mx.google.com with ESMTPS id f11-20020a056a00238b00b006cb8cbc9bb8si14570499pfc.284.2023.11.29.09.38.10 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Wed, 29 Nov 2023 09:38:10 -0800 (PST) Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) client-ip=23.128.96.36; Authentication-Results: mx.google.com; dkim=pass header.i=@kernel.org header.s=k20201202 header.b=bhfR2UoU; spf=pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.36 as permitted sender) smtp.mailfrom=linux-kernel-owner@vger.kernel.org; dmarc=pass (p=NONE sp=NONE dis=NONE) header.from=kernel.org Received: from out1.vger.email (depot.vger.email [IPv6:2620:137:e000::3:0]) by pete.vger.email (Postfix) with ESMTP id EE36780A998F; Wed, 29 Nov 2023 09:38:07 -0800 (PST) X-Virus-Status: Clean X-Virus-Scanned: clamav-milter 0.103.11 at pete.vger.email Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S231623AbjK2Rhv (ORCPT + 99 others); Wed, 29 Nov 2023 12:37:51 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:51930 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S229509AbjK2Rhu (ORCPT ); Wed, 29 Nov 2023 12:37:50 -0500 Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 85CCBF4 for ; Wed, 29 Nov 2023 09:37:56 -0800 (PST) Received: by smtp.kernel.org (Postfix) with ESMTPSA id 79736C433C8; Wed, 29 Nov 2023 17:37:55 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1701279476; bh=lJyy3cA15cbgMVL8KhADRDQs1MK00XO4WULr9ZayULc=; h=In-Reply-To:References:Date:From:To:Cc:Subject:From; b=bhfR2UoUKb5fr2G4p6VFK2S1Ppt0/o9rie3j3bZVYpyR2uZ46MOtrpChm5gG/YHeY d4e84r9qTNpZf6MFWEQLfje1QtIJNTswysbw6//VOLD7l+9lm24649zNRlptTpLij6 nDSqQWc7EEPJxL3z6dUS49Kc3apEOlwMi8328GYBhLyjX2DoqmLM6PET9Kv32emYCb lj0Syp2wSuClxWEMtnsRaZbZlOaaparuflWmh9lkxNjGMOzpMVf1BkWN+9I8H5SMaC yGUOEvtTWKPjdjkXdEO1e9GZcEC8GXaT3XH+S2PXNhJmVOafj6LF1zEgwjSnP3+UyC S+ivGMiHIwnJA== Received: from compute5.internal (compute5.nyi.internal [10.202.2.45]) by mailauth.nyi.internal (Postfix) with ESMTP id 5BA0727C0054; Wed, 29 Nov 2023 12:37:54 -0500 (EST) Received: from imap51 ([10.202.2.101]) by compute5.internal (MEProxy); Wed, 29 Nov 2023 12:37:54 -0500 X-ME-Sender: X-ME-Proxy-Cause: gggruggvucftvghtrhhoucdtuddrgedvkedrudeihedguddtudcutefuodetggdotefrod ftvfcurfhrohhfihhlvgemucfhrghsthforghilhdpqfgfvfdpuffrtefokffrpgfnqfgh necuuegrihhlohhuthemuceftddtnecusecvtfgvtghiphhivghnthhsucdlqddutddtmd enucfjughrpefofgggkfgjfhffhffvvefutgfgsehtqhertderreejnecuhfhrohhmpedf tehrnhguuceuvghrghhmrghnnhdfuceorghrnhgusehkvghrnhgvlhdrohhrgheqnecugg ftrfgrthhtvghrnhepvddttdffjeefgeeggfelfefggefhheeffefftefggfelgeduveev tefhfeejveeknecuvehluhhsthgvrhfuihiivgeptdenucfrrghrrghmpehmrghilhhfrh homheprghrnhguodhmvghsmhhtphgruhhthhhpvghrshhonhgrlhhithihqdduvdekhedu jedtvdegqddvkeejtddtvdeigedqrghrnhgupeepkhgvrhhnvghlrdhorhhgsegrrhhnug gsrdguvg X-ME-Proxy: Feedback-ID: i36794607:Fastmail Received: by mailuser.nyi.internal (Postfix, from userid 501) id 3F299B60089; Wed, 29 Nov 2023 12:37:52 -0500 (EST) X-Mailer: MessagingEngine.com Webmail Interface User-Agent: Cyrus-JMAP/3.9.0-alpha0-1234-gac66594aae-fm-20231122.001-gac66594a MIME-Version: 1.0 Message-Id: <8c45a36e-60f9-49ee-aa77-aaba8ac5e62f@app.fastmail.com> In-Reply-To: References: <20231120215945.52027-2-pstanner@redhat.com> <20231120215945.52027-6-pstanner@redhat.com> Date: Wed, 29 Nov 2023 18:37:25 +0100 From: "Arnd Bergmann" To: "Philipp Stanner" , "Danilo Krummrich" , "Bjorn Helgaas" , "Andrew Morton" , "Randy Dunlap" , "Jason Gunthorpe" , "Eric Auger" , "Kent Overstreet" , "Niklas Schnelle" , "Neil Brown" , "John Sanpe" , "Dave Jiang" , "Yury Norov" , "Kees Cook" , "Masami Hiramatsu" , "David Gow" , "Herbert Xu" , "Thomas Gleixner" , "wuqiang.matt" , "Jason Baron" , "Ben Dooks" Cc: linux-kernel@vger.kernel.org, linux-pci@vger.kernel.org Subject: Re: [PATCH 4/4] lib/iomap.c: improve comment about pci anomaly Content-Type: text/plain;charset=utf-8 Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-1.2 required=5.0 tests=DKIMWL_WL_HIGH,DKIM_SIGNED, DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,MAILING_LIST_MULTI, SPF_HELO_NONE,SPF_PASS,T_SCC_BODY_TEXT_LINE autolearn=unavailable autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on pete.vger.email Precedence: bulk List-ID: X-Mailing-List: linux-kernel@vger.kernel.org X-Greylist: Sender passed SPF test, not delayed by milter-greylist-4.6.4 (pete.vger.email [0.0.0.0]); Wed, 29 Nov 2023 09:38:08 -0800 (PST) On Wed, Nov 29, 2023, at 11:16, Philipp Stanner wrote: > On Fri, 2023-11-24 at 20:08 +0100, Danilo Krummrich wrote: >> >> lib/pci_iomap.c contains another definition of pci_iounmap() which is >> guarded by ARCH_WANTS_GENERIC_PCI_IOUNMAP to prevent multiple >> definitions >> in case either GENERIC_IOMAP is set or the architecture already >> defined >> pci_iounmap(). > > To clarify that, here's the relevant excerpt from include/asm- > generic/io.h: > > #ifndef CONFIG_GENERIC_IOMAP > #ifndef pci_iounmap > #define ARCH_WANTS_GENERIC_PCI_IOUNMAP > #endif > #endif Right, this was added fairly recently in an effort to unify the architectures that can share a simple implementation based on the way that modern PCI host bridges on non-x86 work. >> What's the purpose of not having set ARCH_HAS_GENERIC_IOPORT_MAP >> producing >> an empty definition of pci_iounmap() though [1]? >>=20 >> And more generally, is there any other (subtle) logic behind this? > > That's indeed also very hard to understand for me, because you'd expect > that if pci_iomap() exists (and does something), pci_iounmap() should > also exist and, of course, unmapp the memory again. Right, I think that was a leak introduced in 316e8d79a095 ("pci_iounmap'2: Electric Boogaloo: try to make sense of it all") that should be fixed like --- a/lib/pci_iomap.c +++ b/lib/pci_iomap.c @@ -170,8 +170,8 @@ void pci_iounmap(struct pci_dev *dev, void __iomem *= p) =20 if (addr >=3D start && addr < start + IO_SPACE_LIMIT) return; - iounmap(p); #endif + iounmap(p); } EXPORT_SYMBOL(pci_iounmap); i.e. architectures without port I/O just call iounmap() but those that support the normal ioport_map() have to skip iounmap() for ports in that special PIO range. > Regarding the last point, a number of architectures define their own > ioport_map(): > > arch/alpha/kernel/io.c, line 684 (as a function) > arch/arc/include/asm/io.h, line 27 (as a function) > arch/arm/mm/iomap.c, line 19 (as a function) > arch/m68k/include/asm/kmap.h, line 60 (as a function) > arch/parisc/lib/iomap.c, line 504 (as a function) > arch/powerpc/kernel/iomap.c, line 14 (as a function) > arch/s390/include/asm/io.h, line 38 (as a function) > arch/sh/kernel/ioport.c, line 24 (as a function) > arch/sparc/lib/iomap.c, line 10 (as a function) > > I grepped through those archs and as I see it, none of those specify an > empty pci_iomap() that could be a counterpart to the potentially empty > pci_iounmap() in lib/pci_iomap.c I'm trying to unwind what you are saying here, and there are two separate issues: - an empty unmap() function still makes sense if the map() function just returns a usable pointer like the asm-generic version of ioport_map(), it only has to be non-empty if the map function allocates a resource that has to be freed later, like the page table entries for most ioremap() implementations. - pci_iounmap() in lib/pci_iomap.c being empty is probably just a bug >> From what I can tell looking at the header, I think we can >> just remove the "#elif defined(CONFIG_GENERIC_PCI_IOMAP)" >> bit entirely, as it no longer serves the purpose it originally >> had. > > So it seems that the empty unmap-function in pci_iomap.c is the left- > over counterpart of those mapping functions always returning NULL. no > @Arnd: > Your code draft > > void pci_iounmap(struct pci_dev *dev, void __iomem * addr) > { > #ifdef CONFIG_HAS_IOPORT > if (iomem_is_ioport(addr)) { > ioport_unmap(addr); > return; > } > #endif > iounmap(addr) > } > > seems to agree with that: There will never be the need for an empty > function that does nothing. Correct? Agreed, while arch/sparc/ currently has an empty pci_iounmap(), that is just because the normal iounmap() on that architecture is also empty, given that all MMIO memory is always mapped. >> > { >> > #ifdef CONFIG_HAS_IOPORT >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 if (iomem_is_ioport(addr= )) { >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 ioport_unmap(addr); >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2= =A0=C2=A0=C2=A0 return; >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 } >> > #endif >> > =C2=A0=C2=A0=C2=A0=C2=A0=C2=A0=C2=A0 iounmap(addr) >> > } >> >=20 >> > and then define iomem_is_ioport() in lib/iomap.c for x86, >> > while defining it in asm-generic/io.h for the rest, >> > with an override in asm/io.h for those architectures >> > that need a custom inb(). >>=20 >> So, that would be similar to IO_COND(), right? What would we need >> inb() for in this context? In general, any architecture that has a custom inb() also needs a custom ioport_map() and iomem_is_ioport() in this scheme, while the "normal" architectures like arm/arm64 and riscv should be able to just use the asm-generic version. IO_COND() is really specific to those architectures that rely on the rather misnamed GENERIC_IOMAP for implementing ioport_map(). Arnd