Le 20/08/2022 à 02:31, Baoquan He a écrit : > 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 hook ioremap_allowed() and iounmap_allowed(). > Meanwhile, rename them to arch_ioremap() and arch_iounmap() to reflect > their current behaviour. Please don't just say you need to change the return value. Explain why. And why does it need a name change ? The new name suggests that what was simply a check function becomes now a function doing the job. Is that the intention ? > > === > arch_ioremap() return a bool, It is not a bool. A bool is either true or false. > - 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, Same here, not a bool either. > - 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@xxxxxxxxxx/T/#u > > While at it, the invocation of arch_ioremap() need be moved to the > beginning of ioremap_prot() because architectures like sh, openrisc, > ia64, need do the ARCH specific io address mapping on the original > physical address. And in the later patch, the address fix up code > in arch_ioremap() also need be done on the original addre on some > architectures. > > This is preparation for later patch, no functionality change. No functionnal change, really ? > > Signed-off-by: Baoquan He <bhe@xxxxxxxxxx> > --- > arch/arm64/include/asm/io.h | 4 ++-- > arch/arm64/mm/ioremap.c | 15 ++++++++++----- > include/asm-generic/io.h | 29 +++++++++++++++-------------- > mm/ioremap.c | 12 ++++++++---- > 4 files changed, 35 insertions(+), 25 deletions(-) > > diff --git a/arch/arm64/include/asm/io.h b/arch/arm64/include/asm/io.h > index 877495a0fd0c..dd7e1c2dc86c 100644 > --- a/arch/arm64/include/asm/io.h > +++ b/arch/arm64/include/asm/io.h > @@ -139,8 +139,8 @@ 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); > -#define ioremap_allowed ioremap_allowed > +void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot); > +#define arch_ioremap arch_ioremap > > #define _PAGE_IOREMAP PROT_DEVICE_nGnRE > > diff --git a/arch/arm64/mm/ioremap.c b/arch/arm64/mm/ioremap.c > index c5af103d4ad4..b0f4cea86f0e 100644 > --- a/arch/arm64/mm/ioremap.c > +++ b/arch/arm64/mm/ioremap.c > @@ -3,19 +3,24 @@ > #include <linux/mm.h> > #include <linux/io.h> > > -bool ioremap_allowed(phys_addr_t phys_addr, size_t size, unsigned long prot) > +void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot) > { > - unsigned long last_addr = phys_addr + size - 1; > + unsigned long last_addr, offset; > + > + offset = phys_addr & (~PAGE_MASK); > + phys_addr -= offset; > + size = PAGE_ALIGN(size + offset); > + last_addr = phys_addr + size - 1; > > /* Don't allow outside PHYS_MASK */ > if (last_addr & ~PHYS_MASK) > - return false; > + return IOMEM_ERR_PTR(-EINVAL); > > /* 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(-EINVAL); > > - return true; > + return NULL; > } > > /* > diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h > index a68f8fbf423b..7b6bfb62ef80 100644 > --- a/include/asm-generic/io.h > +++ b/include/asm-generic/io.h > @@ -1049,27 +1049,28 @@ 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 > - * iounmap_allowed() return a bool, > - * - true means continue to vunmap > - * - false means skip vunmap and return directly > + * 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 ioremap_allowed > -#define ioremap_allowed ioremap_allowed > -static inline bool ioremap_allowed(phys_addr_t phys_addr, size_t size, > +#ifndef arch_ioremap > +#define arch_ioremap arch_ioremap > +static inline void __iomem *arch_ioremap(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) > +#ifndef arch_iounmap > +#define arch_iounmap arch_iounmap > +static inline int arch_iounmap(void __iomem *addr) > { > - return true; > + return 0; > } > #endif > > diff --git a/mm/ioremap.c b/mm/ioremap.c > index 8652426282cc..99fde69becc7 100644 > --- a/mm/ioremap.c > +++ b/mm/ioremap.c > @@ -17,6 +17,13 @@ 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 *ioaddr; > + > + ioaddr = arch_ioremap(phys_addr, size, prot); > + if (IS_ERR(ioaddr)) > + return NULL; > + else if (ioaddr) > + return ioaddr; > > /* Disallow wrap-around or zero size */ > last_addr = phys_addr + size - 1; > @@ -28,9 +35,6 @@ 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)) > - return NULL; > - > area = get_vm_area_caller(size, VM_IOREMAP, > __builtin_return_address(0)); > if (!area) > @@ -52,7 +56,7 @@ void iounmap(volatile void __iomem *addr) > { > void *vaddr = (void *)((unsigned long)addr & PAGE_MASK); > > - if (!iounmap_allowed(vaddr)) > + if (arch_iounmap((void __iomem *)addr)) > return; > > if (is_vmalloc_addr(vaddr))
