From: Alexander Gordeev <agordeev@linux.ibm.com> To: Kefeng Wang <wangkefeng.wang@huawei.com> Cc: Baoquan He <bhe@redhat.com>, linux-kernel@vger.kernel.org, linux-mm@kvack.org, akpm@linux-foundation.org, hch@infradead.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 01/11] mm/ioremap: change the return value of io[re|un]map_allowed Date: Sat, 6 Aug 2022 10:29:23 +0200 [thread overview] Message-ID: <Yu4mYxpV0GWRTjQp@li-4a3a4a4c-28e5-11b2-a85c-a8d192c6f089.ibm.com> (raw) In-Reply-To: <832b38ca-064e-0ab8-cd62-337d0d83d471@huawei.com> On Sat, Aug 06, 2022 at 10:29:03AM +0800, Kefeng Wang wrote: > > On 2022/8/4 23:42, Alexander Gordeev wrote: > > On Mon, Aug 01, 2022 at 10:40:19PM +0800, Baoquan He wrote: > > > > Hi Baoquan, > > > > > --- a/arch/arm64/mm/ioremap.c > > > +++ b/arch/arm64/mm/ioremap.c > > > @@ -3,19 +3,20 @@ > > > #include <linux/mm.h> > > > #include <linux/io.h> > > > -bool ioremap_allowed(phys_addr_t phys_addr, size_t size, unsigned long prot) > > > +void __iomem *ioremap_allowed(phys_addr_t phys_addr, size_t size, unsigned long prot) > > > { > > > unsigned long last_addr = phys_addr + size - 1; > > > + int ret = -EINVAL; > > If ret variable is really needed? > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > > index 72974cb81343..d72eb310fb3c 100644 > > > --- a/include/asm-generic/io.h > > > +++ b/include/asm-generic/io.h > > > @@ -967,26 +967,27 @@ static inline void iounmap(volatile void __iomem *addr) > > > /* > > > * Arch code can implement the following two hooks when using GENERIC_IOREMAP > > > * ioremap_allowed() return a bool, > > > - * - true means continue to remap > > > - * - false means skip remap and return directly > > > + * - IS_ERR means return an error > > > + * - NULL means continue to remap > > > + * - a non-NULL, non-IS_ERR pointer is returned directly > > If ioremap_allowed() returns a valid pointer, then the function name > > is not as precise anymore. > > Maybe use arch_ioremap/unmap as before, or some better name. > > > > > > @@ -28,8 +29,11 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, > > > phys_addr -= offset; > > > size = PAGE_ALIGN(size + offset); > > > - if (!ioremap_allowed(phys_addr, size, prot)) > > > + base = ioremap_allowed(phys_addr, size, prot); > > > + if (IS_ERR(base)) > > > return NULL; > > > + else if (base) > > > + return base; > > It is probably just me, but the base name bit misleading here. > We could reuse vaddr, not add new base. vaddr name is wrong AFAICT. ioremap_allowed() returns __iomem address, not the virtual one. > > > > > @@ -50,9 +54,9 @@ EXPORT_SYMBOL(ioremap_prot); > > > void iounmap(volatile void __iomem *addr) > > > { > > > - void *vaddr = (void *)((unsigned long)addr & PAGE_MASK); > > > + void __iomem *vaddr = (void __iomem *)((unsigned long)addr & PAGE_MASK); Same here. > > > - if (!iounmap_allowed(vaddr)) > > > + if (iounmap_allowed(vaddr)) > > I guess, iounmap_allowed() should accept void __iomem *, not void *. > > Then addr needs to be passed to iounmap_allowed() not vaddr. > > The following is_vmalloc_addr() and vunmap() in iounmap() use void *, > > so we could simply use void* for iounmap_allowed(). iounmap_allowed() accepts void __iomem * and I that looks correct to me. Passing void * on the other hand means you pass a pointer that in theory differs from what architecture previously returned with ioremap_allowed() and "knows" nothing about. I think you need to pass addr to iounmap_allowed() as is.
WARNING: multiple messages have this Message-ID (diff)
From: Alexander Gordeev <agordeev@linux.ibm.com> To: Kefeng Wang <wangkefeng.wang@huawei.com> Cc: Baoquan He <bhe@redhat.com>, linux-kernel@vger.kernel.org, linux-mm@kvack.org, akpm@linux-foundation.org, hch@infradead.org, linux-arm-kernel@lists.infradead.org Subject: Re: [PATCH 01/11] mm/ioremap: change the return value of io[re|un]map_allowed Date: Sat, 6 Aug 2022 10:29:23 +0200 [thread overview] Message-ID: <Yu4mYxpV0GWRTjQp@li-4a3a4a4c-28e5-11b2-a85c-a8d192c6f089.ibm.com> (raw) In-Reply-To: <832b38ca-064e-0ab8-cd62-337d0d83d471@huawei.com> On Sat, Aug 06, 2022 at 10:29:03AM +0800, Kefeng Wang wrote: > > On 2022/8/4 23:42, Alexander Gordeev wrote: > > On Mon, Aug 01, 2022 at 10:40:19PM +0800, Baoquan He wrote: > > > > Hi Baoquan, > > > > > --- a/arch/arm64/mm/ioremap.c > > > +++ b/arch/arm64/mm/ioremap.c > > > @@ -3,19 +3,20 @@ > > > #include <linux/mm.h> > > > #include <linux/io.h> > > > -bool ioremap_allowed(phys_addr_t phys_addr, size_t size, unsigned long prot) > > > +void __iomem *ioremap_allowed(phys_addr_t phys_addr, size_t size, unsigned long prot) > > > { > > > unsigned long last_addr = phys_addr + size - 1; > > > + int ret = -EINVAL; > > If ret variable is really needed? > > > > > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > > > index 72974cb81343..d72eb310fb3c 100644 > > > --- a/include/asm-generic/io.h > > > +++ b/include/asm-generic/io.h > > > @@ -967,26 +967,27 @@ static inline void iounmap(volatile void __iomem *addr) > > > /* > > > * Arch code can implement the following two hooks when using GENERIC_IOREMAP > > > * ioremap_allowed() return a bool, > > > - * - true means continue to remap > > > - * - false means skip remap and return directly > > > + * - IS_ERR means return an error > > > + * - NULL means continue to remap > > > + * - a non-NULL, non-IS_ERR pointer is returned directly > > If ioremap_allowed() returns a valid pointer, then the function name > > is not as precise anymore. > > Maybe use arch_ioremap/unmap as before, or some better name. > > > > > > @@ -28,8 +29,11 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, > > > phys_addr -= offset; > > > size = PAGE_ALIGN(size + offset); > > > - if (!ioremap_allowed(phys_addr, size, prot)) > > > + base = ioremap_allowed(phys_addr, size, prot); > > > + if (IS_ERR(base)) > > > return NULL; > > > + else if (base) > > > + return base; > > It is probably just me, but the base name bit misleading here. > We could reuse vaddr, not add new base. vaddr name is wrong AFAICT. ioremap_allowed() returns __iomem address, not the virtual one. > > > > > @@ -50,9 +54,9 @@ EXPORT_SYMBOL(ioremap_prot); > > > void iounmap(volatile void __iomem *addr) > > > { > > > - void *vaddr = (void *)((unsigned long)addr & PAGE_MASK); > > > + void __iomem *vaddr = (void __iomem *)((unsigned long)addr & PAGE_MASK); Same here. > > > - if (!iounmap_allowed(vaddr)) > > > + if (iounmap_allowed(vaddr)) > > I guess, iounmap_allowed() should accept void __iomem *, not void *. > > Then addr needs to be passed to iounmap_allowed() not vaddr. > > The following is_vmalloc_addr() and vunmap() in iounmap() use void *, > > so we could simply use void* for iounmap_allowed(). iounmap_allowed() accepts void __iomem * and I that looks correct to me. Passing void * on the other hand means you pass a pointer that in theory differs from what architecture previously returned with ioremap_allowed() and "knows" nothing about. I think you need to pass addr to iounmap_allowed() as is. _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
next prev parent reply other threads:[~2022-08-06 8:30 UTC|newest] Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top 2022-08-01 14:40 [PATCH 00/11] mm: ioremap: Convert architectures to take GENERIC_IOREMAP way Baoquan He 2022-08-01 14:40 ` Baoquan He 2022-08-01 14:40 ` [PATCH 01/11] mm/ioremap: change the return value of io[re|un]map_allowed Baoquan He 2022-08-01 14:40 ` Baoquan He 2022-08-04 15:42 ` Alexander Gordeev 2022-08-04 15:42 ` Alexander Gordeev 2022-08-06 2:29 ` Kefeng Wang 2022-08-06 2:29 ` Kefeng Wang 2022-08-06 8:29 ` Alexander Gordeev [this message] 2022-08-06 8:29 ` Alexander Gordeev 2022-08-07 1:42 ` Baoquan He 2022-08-07 1:42 ` Baoquan He 2022-08-07 1:51 ` Baoquan He 2022-08-07 1:51 ` Baoquan He 2022-08-07 1:58 ` Baoquan He 2022-08-07 1:58 ` Baoquan He 2022-08-01 14:40 ` [PATCH 02/11] mm: ioremap: fixup the physical address Baoquan He 2022-08-01 14:40 ` Baoquan He 2022-08-04 16:02 ` Alexander Gordeev 2022-08-04 16:02 ` Alexander Gordeev 2022-08-07 2:11 ` Baoquan He 2022-08-07 2:11 ` Baoquan He 2022-08-20 0:49 ` Baoquan He 2022-08-20 0:49 ` Baoquan He 2022-08-01 14:40 ` [PATCH 03/11] mm: ioremap: allow ARCH to have its own ioremap definition Baoquan He 2022-08-01 14:40 ` Baoquan He 2022-08-01 14:40 ` [PATCH 04/11] arc: mm: Convert to GENERIC_IOREMAP Baoquan He 2022-08-01 14:40 ` Baoquan He 2022-08-01 14:40 ` Baoquan He 2022-08-01 14:40 ` [PATCH 05/11] hexagon: " Baoquan He 2022-08-01 14:40 ` Baoquan He 2022-08-01 14:40 ` [PATCH 06/11] ia64: " Baoquan He 2022-08-01 14:40 ` Baoquan He 2022-08-01 14:40 ` Baoquan He 2022-08-01 14:40 ` [PATCH 07/11] openrisc: " Baoquan He 2022-08-01 14:40 ` Baoquan He 2022-08-01 14:40 ` Baoquan He 2022-08-01 14:40 ` [PATCH 08/11] parisc: " Baoquan He 2022-08-01 14:40 ` Baoquan He 2022-08-01 14:40 ` [PATCH 09/11] s390: " Baoquan He 2022-08-01 14:40 ` Baoquan He 2022-08-01 14:40 ` [PATCH 10/11] sh: " Baoquan He 2022-08-01 14:40 ` Baoquan He 2022-08-01 14:40 ` [PATCH 11/11] xtensa: " Baoquan He 2022-08-01 14:40 ` Baoquan He
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --in-reply-to=Yu4mYxpV0GWRTjQp@li-4a3a4a4c-28e5-11b2-a85c-a8d192c6f089.ibm.com \ --to=agordeev@linux.ibm.com \ --cc=akpm@linux-foundation.org \ --cc=bhe@redhat.com \ --cc=hch@infradead.org \ --cc=linux-arm-kernel@lists.infradead.org \ --cc=linux-kernel@vger.kernel.org \ --cc=linux-mm@kvack.org \ --cc=wangkefeng.wang@huawei.com \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: linkBe sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.