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 43933ECAAA1 for ; Mon, 12 Sep 2022 02:57:51 +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:In-Reply-To:MIME-Version:References: Message-ID:Subject:Cc: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=7Sj1sD28zqFQNa3pwoAu06CxL9M9BrWPPGIrE8brKlI=; b=Xk5fKOEF563shJ LkUQ0U/O/TpJvBttVqk8qWgiar2OTCdgusejihXcsW60Xyz5yt0DRuHjEXPFYtEeqj0syJECP0u+s jO4MeTAYAA/hSvpfcGqQsRYJI0pURyEuUxi3fQ5z3jlEdpqGXzi08k3NC01UCzEQ2c66M4/vO0JmJ WKM3I84FE8zvMbHHxnNTq1y5eTcPMTYxcsCzGmT3utf/C9+/4U3uI49RCoGmQGL7f9gIZICCV8Q/o jNZkBQMizpvqB4Uy2B0yiQwuq3Mtg/jGxQSAKTr2dJRPPjym/RA7Q54F2u32Th/vSkB41bWtRGZ/u OAUqU6O1x4FKVu/Invrw==; Received: from localhost ([::1] helo=bombadil.infradead.org) by bombadil.infradead.org with esmtp (Exim 4.94.2 #2 (Red Hat Linux)) id 1oXZcN-006SkY-AX; Mon, 12 Sep 2022 02:56:15 +0000 Received: from us-smtp-delivery-124.mimecast.com ([170.10.129.124]) by bombadil.infradead.org with esmtps (Exim 4.94.2 #2 (Red Hat Linux)) id 1oXZcI-006Sgx-Ht for linux-arm-kernel@lists.infradead.org; Mon, 12 Sep 2022 02:56:14 +0000 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1662951367; 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=QNx0pkhpCOCmN1r/bDO5Y2HzdH/DnCo328+m7ItQR3I=; b=KC3SlhVHvucgh4sBhjLJaOZPZUYu8rFuHLhZhnlO0pqtPudVz9slUjgLpVFSYzuYf7vRp0 HJa5VoG4AU55BHw/K9r7R8VB5SJMVyZFjI30Zc70V1i1R1MYdL6pw8Z2qIau2v/EAxjco4 O5SWe4dRdNfcFfmpRUaovwO4RUhm8BI= Received: from mimecast-mx02.redhat.com (mx3-rdu2.redhat.com [66.187.233.73]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.2, cipher=TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384) id us-mta-159-LfkHz_sqP8i2v-ih7rxMbg-1; Sun, 11 Sep 2022 22:56:03 -0400 X-MC-Unique: LfkHz_sqP8i2v-ih7rxMbg-1 Received: from smtp.corp.redhat.com (int-mx06.intmail.prod.int.rdu2.redhat.com [10.11.54.6]) (using TLSv1.2 with cipher AECDH-AES256-SHA (256/256 bits)) (No client certificate requested) by mimecast-mx02.redhat.com (Postfix) with ESMTPS id 4FDF429AA381; Mon, 12 Sep 2022 02:56:03 +0000 (UTC) Received: from localhost (ovpn-12-21.pek2.redhat.com [10.72.12.21]) by smtp.corp.redhat.com (Postfix) with ESMTPS id 6087C2166B26; Mon, 12 Sep 2022 02:56:01 +0000 (UTC) Date: Mon, 12 Sep 2022 10:55:58 +0800 From: Baoquan He To: Christophe Leroy Cc: "linux-kernel@vger.kernel.org" , "hch@infradead.org" , "linux-mm@kvack.org" , "akpm@linux-foundation.org" , "agordeev@linux.ibm.com" , "wangkefeng.wang@huawei.com" , "linux-arm-kernel@lists.infradead.org" Subject: Re: [PATCH v2 02/11] mm: ioremap: fixup the physical address and page prot Message-ID: References: <20220820003125.353570-1-bhe@redhat.com> <20220820003125.353570-3-bhe@redhat.com> <54b7afcc-056d-7f33-6858-d451a3222c70@csgroup.eu> <8df89136-a7f2-9b66-d522-a4fb9860bf22@csgroup.eu> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: X-Scanned-By: MIMEDefang 2.78 on 10.11.54.6 X-CRM114-Version: 20100106-BlameMichelson ( TRE 0.8.0 (BSD) ) MR-646709E3 X-CRM114-CacheID: sfid-20220911_195610_936553_985D899D X-CRM114-Status: GOOD ( 42.94 ) 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 Hi Christophe, On 08/28/22 at 07:10pm, Baoquan He wrote: > On 08/23/22 at 07:03pm, Christophe Leroy wrote: ...... > > >>>> Is it really the best approach ? Wouldn't it be better to have helpers > > >>>> to do that, those helpers being called by the ioremap_prot(), instead of > > >>>> doing it inside the arch_ioremap() function ? > > >>> > > >>> This is suggested too by Alexander during his v1 reviewing. I tried, but > > >>> feel the current way taken in this patchset is better. Because not all > > >>> architecutres need the address fix up, only parisc, and only few need > > >>> adjust the 'prot'. Introducing other helpers seems too much, that only > > >>> increases the complexity of of ioremap() and the generic GENERIC_IOREMAP > > >>> method for people to understand and take. > > >> > > >> I can't understand. Why is it difficult to do something like: > > >> > > >> #ifndef ioremap_adjust_prot > > >> static inline unsigned long ioremap_adjust_prot(unsigned long flags) > > >> { > > >> return flags; > > >> } > > >> #endif > > >> > > >> Then for arc you do > > >> > > >> static inline unsigned long ioremap_adjust_prot(unsigned long flags) > > >> { > > >> return pgprot_val(pgprot_noncached(__pgprot(flags))); > > >> } > > >> #define ioremap_adjust_prot ioremap_adjust_prot > > > > > > My thinking is we have four things to do in the added hookers. > > > 1) check if we should do ioremap on ARCHes. If not, return NULL from > > > ioremap_prot(); > > > 2) handling the mapping io address specifically on ARCHes, e.g arc, > > > ia64, s390; > > > 3) the original physical address passed into ioremap_prot() need be > > > fixed up, e.g arc; > > > 4) the 'prot' passed into ioremap_prot() need be adjusted, e.g on arc > > > and xtensa. > > > > > > With Kefeng's patches, the case 1) is handled with introduced > > > ioremap_allowed()/iounmap_allowed(). In this patchset, what I do is > > > rename the hooks as arch_ioremap() and arch_iounmap(), then put case 1), > > > 2), 3), 4) handling into arch_ioremap(). Adding helpers to cover each > > > case is not difficult from my side. I worry that as time goes by, those > > > several hooks my cause issue, e.g if a new adjustment need be done, > > > should we introduce a new helper or make do with the existed hook; how > > > > > > When I investigated this, one arch_ioremap() looks not complicated > > > since not all ARCHes need cover all above 4 cases. That's why I finally > > > choose one hook. I am open to new idea, please let me know if we should > > > change it to introduce several different helpers. > > > > > > > A new idea that would have my preference would be to do just like we did > > with arch_get_unmapped_area(). Look at > > https://elixir.bootlin.com/linux/v6.0-rc1/source/arch/powerpc/mm/book3s64/slice.c#L638 > > and https://elixir.bootlin.com/linux/v6.0-rc1/source/mm/mmap.c#L2131 > > > > Instead of having the generic that calls the arch specific, make it the > > other way round, have the arch specific call the generic after doing its > > specialties. > > This sounds good. I made a draft patch to change code in generic code > part, just showing what it looks like. > > Both arch_ioremap() way and the arch sepcific call the generic way look > good to me. Just it will take less effort for me to continue the > arch_ioremap() way. I would like to hear Christoph's opinion since he > introduced the GENERIC_IOREMAP method and suggested the earlier > arch_ioremap() way and change in this patchset. I will make another round change and post. Since Christoph doesn't reply, I would like to continue with the existing arch_ioremap/arch_iounmap() hooks way if you don't have strong opinion on the new idea to reintroduce ioremap(). > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > index 68a8117b30fa..4bc3e18c475f 100644 > --- a/include/asm-generic/io.h > +++ b/include/asm-generic/io.h > @@ -1047,35 +1047,18 @@ static inline void iounmap(volatile void __iomem *addr) > #elif defined(CONFIG_GENERIC_IOREMAP) > #include > > -/* > - * Arch code can implement the following two hooks when using GENERIC_IOREMAP > - * arch_ioremap() return a bool, > - * - IS_ERR means return an error > - * - NULL means continue to remap > - * - a non-NULL, non-IS_ERR pointer is returned directly > - * arch_iounmap() return a bool, > - * - 0 means continue to vunmap > - * - error code means skip vunmap and return directly > - */ > -#ifndef arch_ioremap > -#define arch_ioremap arch_ioremap > -static inline void __iomem *arch_ioremap(phys_addr_t *paddr, size_t size, > - unsigned long *prot_val) > -{ > - return NULL; > -} > -#endif > +void __iomem * > +generic_ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot); > > -#ifndef arch_iounmap > -#define arch_iounmap arch_iounmap > -static inline int arch_iounmap(void __iomem *addr) > +#ifndef ioremap_prot > +#define ioremap_prot ioremap_prot > +void __iomem * > +ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot); > { > - return 0; > + return generic_ioremap_prot(phys_addr, size, prot); > } > #endif > > -void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, > - unsigned long prot); > void iounmap(volatile void __iomem *addr); > > #ifndef ioremap > diff --git a/mm/ioremap.c b/mm/ioremap.c > index 7914b5cf5b78..87d51003dee6 100644 > --- a/mm/ioremap.c > +++ b/mm/ioremap.c > @@ -11,8 +11,8 @@ > #include > #include > > -void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, > - unsigned long prot) > +void __iomem * > +generic_ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot) > { > unsigned long offset, vaddr; > phys_addr_t last_addr; > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel