lkml.org 
[lkml]   [2022]   [Aug]   [22]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    From
    SubjectRe: [PATCH v2 01/11] mm/ioremap: change the return value of io[re|un]map_allowed and rename
    Date


    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@huawei.com/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@redhat.com>
    > ---
    > 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))
    \
     
     \ /
      Last update: 2022-08-22 08:27    [W:7.872 / U:0.124 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site