Messages in this thread | | | Date | Sat, 6 Aug 2022 10:29:03 +0800 | Subject | Re: [PATCH 01/11] mm/ioremap: change the return value of io[re|un]map_allowed | From | Kefeng Wang <> |
| |
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; > .
| |