| Message ID | 20220801144029.57829-2-bhe@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | mm: ioremap: Convert architectures to take GENERIC_IOREMAP way | expand |
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. > @@ -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. > @@ -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); > > - 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. > return;
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. > >> @@ -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); >> >> - 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(). > >> return; > .
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.
On 08/06/22 at 10:29am, Alexander Gordeev wrote: > 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. Thanks a lot for reviewing, both. Here, I tend to agree with Alexander. ioremap_allowed() returns __iomem*. How about naming it io_addr here. While I don't have strong opinion about it, reusing vaddr and casting it to (__iomem*) when return is also OK to me. > > > > > > > > @@ -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. OK, I will change to pass (__iomem*) to iounmap_allowed() directly. Thanks.
On 08/06/22 at 10:29am, Kefeng Wang wrote: ...snip... > > > 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. Looks good to me. Or ioremap_check() which is a generic name, and usually xxx_check() can hanlde many things.
On 08/04/22 at 05:42pm, 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? There are two places where -EINVAL need be returned. I can remove variable ret, and return -EINVAL directly if there's concern. Thanks again for looking into this series.
diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h index 87dd42d74afe..9c56a077b687 100644 --- a/arch/arm64/include/asm/io.h +++ b/arch/arm64/include/asm/io.h @@ -164,7 +164,7 @@ extern void __memset_io(volatile void __iomem *, int, size_t); * I/O memory mapping functions. */ -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); #define ioremap_allowed ioremap_allowed #define _PAGE_IOREMAP PROT_DEVICE_nGnRE diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c index c5af103d4ad4..49467c781283 100644 --- 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; /* Don't allow outside PHYS_MASK */ if (last_addr & ~PHYS_MASK) - return false; + return IOMEM_ERR_PTR(ret); /* Don't allow RAM to be mapped. */ if (WARN_ON(pfn_is_map_memory(__phys_to_pfn(phys_addr)))) - return false; + return IOMEM_ERR_PTR(ret); - return true; + return NULL; } /* 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 * iounmap_allowed() return a bool, - * - true means continue to vunmap - * - false means skip vunmap and return directly + * - 0 means continue to vunmap + * - error code means skip vunmap and return directly */ #ifndef ioremap_allowed #define ioremap_allowed ioremap_allowed -static inline bool ioremap_allowed(phys_addr_t phys_addr, size_t size, +static inline void __iomem *ioremap_allowed(phys_addr_t phys_addr, size_t size, unsigned long prot) { - return true; + return NULL; } #endif #ifndef iounmap_allowed #define iounmap_allowed iounmap_allowed -static inline bool iounmap_allowed(void *addr) +static inline int iounmap_allowed(void *addr) { - return true; + return 0; } #endif diff --git a/mm/ioremap.c b/mm/ioremap.c index 8652426282cc..b16ee9debea2 100644 --- a/mm/ioremap.c +++ b/mm/ioremap.c @@ -17,6 +17,7 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long offset, vaddr; phys_addr_t last_addr; struct vm_struct *area; + void __iomem *base; /* Disallow wrap-around or zero size */ last_addr = phys_addr + size - 1; @@ -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; area = get_vm_area_caller(size, VM_IOREMAP, __builtin_return_address(0)); @@ -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); - if (!iounmap_allowed(vaddr)) + if (iounmap_allowed(vaddr)) return; if (is_vmalloc_addr(vaddr))
In some architectures, there are arch specifici io address mapping handling when calling ioremap() or ioremap_prot(), e.g, arc, ia64, openrisc, s390, sh. In oder to convert them to take GENERIC_IOREMAP method, we need change the return value of ioremap_allowed() and iounmap_allowed() as below. === ioremap_allowed() return a bool, - IS_ERR means return an error - NULL means continue to remap - a non-NULL, non-IS_ERR pointer is returned directly iounmap_allowed() return a bool, - 0 means continue to vunmap - error code means skip vunmap and return directly This is taken from Kefeng's below old patch. Christoph suggested the return value because he foresaw the doablity of converting to take GENERIC_IOREMAP on more architectures. - [PATCH v3 4/6] mm: ioremap: Add arch_ioremap/iounmap() - https://lore.kernel.org/all/20220519082552.117736-5-wangkefeng.wang@huawei.com/T/#u This is preparation for later patch, no functionality change. Signed-off-by: Baoquan He <bhe@redhat.com> --- arch/arm64/include/asm/io.h | 2 +- arch/arm64/mm/ioremap.c | 9 +++++---- include/asm-generic/io.h | 17 +++++++++-------- mm/ioremap.c | 10 +++++++--- 4 files changed, 22 insertions(+), 16 deletions(-)