| Message ID | 20220427121413.168468-3-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Cleanup ioremap() and support ioremap_prot() | expand |
On Wed, 27 Apr 2022 20:14:11 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > Add special check hook for architecture to verify addr, size > or prot when ioremap() or iounmap(), which will make the generic > ioremap more useful. > > ... > > --- a/include/asm-generic/io.h > +++ b/include/asm-generic/io.h > @@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr) > #elif defined(CONFIG_GENERIC_IOREMAP) > #include <linux/pgtable.h> > > +bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot); > +bool arch_iounmap_check(void __iomem *addr); Pet peeve. The word "check" is a poor one. I gives no sense of what the function is checking and it gives no sense of how the function's return value relates to the thing which it checks. Maybe it returns 0 on success and -EINVAL on failure. Don't know! Don't you think that better names would be io_remap_ok(), io_remap_valid(), io_remap_allowed(), etc? Other than that, Acked-by: Andrew Morton <akpm@linux-foundation.org>
On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > @@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr) > #elif defined(CONFIG_GENERIC_IOREMAP) > #include <linux/pgtable.h> > > +bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot); > +bool arch_iounmap_check(void __iomem *addr); > + > void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot); > void iounmap(volatile void __iomem *addr); > > diff --git a/mm/ioremap.c b/mm/ioremap.c > index 522ef899c35f..d1117005dcc7 100644 > --- a/mm/ioremap.c > +++ b/mm/ioremap.c > @@ -11,6 +11,16 @@ > #include <linux/io.h> > #include <linux/export.h> > > +bool __weak arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot) > +{ > + return true; > +} > + > +bool __weak arch_iounmap_check(void __iomem *addr) > +{ > + return true; > +} > + I don't really like the weak functions. The normal way to do this in asm-generic headers is to have something like #ifndef arch_ioremap_check static inline bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot) { return true; } #endif and then in architectures that actually do some checking, have these bits in asm/io.h bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot); #define arch_ioremap_check arch_ioremap_check (or alternatively an extern declaration, if the implementation is nontrivial) It may be worth pointing out that either way requires including asm-generic/io.h, which most architectures don't. This is probably fine, as only csky, riscv and now arm64 use CONFIG_GENERIC_IOREMAP, and we can probably require that any further architectures using this symbol also have to use asm-generic/io.h. Arnd
On Wed, 27 Apr 2022 20:20:30 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > @@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr) > > #elif defined(CONFIG_GENERIC_IOREMAP) > > #include <linux/pgtable.h> > > > > +bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot); > > +bool arch_iounmap_check(void __iomem *addr); > > + > > void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot); > > void iounmap(volatile void __iomem *addr); > > > > diff --git a/mm/ioremap.c b/mm/ioremap.c > > index 522ef899c35f..d1117005dcc7 100644 > > --- a/mm/ioremap.c > > +++ b/mm/ioremap.c > > @@ -11,6 +11,16 @@ > > #include <linux/io.h> > > #include <linux/export.h> > > > > +bool __weak arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot) > > +{ > > + return true; > > +} > > + > > +bool __weak arch_iounmap_check(void __iomem *addr) > > +{ > > + return true; > > +} > > + > > I don't really like the weak functions. How come? They work quite nicely here? > The normal way to do this Is a lot more fuss.
On Wed, Apr 27, 2022 at 8:25 PM Andrew Morton <akpm@linux-foundation.org> wrote: > On Wed, 27 Apr 2022 20:20:30 +0200 Arnd Bergmann <arnd@arndb.de> wrote: > > On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > > > > > +bool __weak arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot) > > > +{ > > > + return true; > > > +} > > > + > > > +bool __weak arch_iounmap_check(void __iomem *addr) > > > +{ > > > + return true; > > > +} > > > + > > > > I don't really like the weak functions. > > How come? They work quite nicely here? I find them rather confusing, mostly because it is less clear whether the fallback function is used in a given configuration, or a replacement one is present. This is a bigger problem in some subsystems than others, and the main place I don't like is the drivers/pci/ subsystem. A number of the uses there should be driver specific but happen to be implemented by architectures instead. Maybe I'm just projecting that onto other uses, but I definitely have a bad feeling about them here. Arnd
On 2022/4/28 1:04, Andrew Morton wrote: > On Wed, 27 Apr 2022 20:14:11 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >> Add special check hook for architecture to verify addr, size >> or prot when ioremap() or iounmap(), which will make the generic >> ioremap more useful. >> >> ... >> >> --- a/include/asm-generic/io.h >> +++ b/include/asm-generic/io.h >> @@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr) >> #elif defined(CONFIG_GENERIC_IOREMAP) >> #include <linux/pgtable.h> >> >> +bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot); >> +bool arch_iounmap_check(void __iomem *addr); > Pet peeve. The word "check" is a poor one. I gives no sense of what > the function is checking and it gives no sense of how the function's > return value relates to the thing which it checks. > > Maybe it returns 0 on success and -EINVAL on failure. Don't know! > > Don't you think that better names would be io_remap_ok(), > io_remap_valid(), io_remap_allowed(), etc? Will use arch_ioremap/unmap_allowed(), and I'd like to keep return bool for now if there is no special requirements. > > > Other than that, > > Acked-by: Andrew Morton <akpm@linux-foundation.org> Thanks. > .
On 2022/4/28 2:20, Arnd Bergmann wrote: > On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: >> @@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr) >> #elif defined(CONFIG_GENERIC_IOREMAP) >> #include <linux/pgtable.h> >> >> +bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot); >> +bool arch_iounmap_check(void __iomem *addr); >> + >> void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot); >> void iounmap(volatile void __iomem *addr); >> >> diff --git a/mm/ioremap.c b/mm/ioremap.c >> index 522ef899c35f..d1117005dcc7 100644 >> --- a/mm/ioremap.c >> +++ b/mm/ioremap.c >> @@ -11,6 +11,16 @@ >> #include <linux/io.h> >> #include <linux/export.h> >> >> +bool __weak arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot) >> +{ >> + return true; >> +} >> + >> +bool __weak arch_iounmap_check(void __iomem *addr) >> +{ >> + return true; >> +} >> + > I don't really like the weak functions. The normal way to do this in > asm-generic headers > is to have something like > > #ifndef arch_ioremap_check > static inline bool arch_ioremap_check(phys_addr_t addr, size_t size, > unsigned long prot) > { > return true; > } > #endif > > and then in architectures that actually do some checking, have these > bits in asm/io.h > > bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot); > #define arch_ioremap_check arch_ioremap_check Ok, I could use this way, and keep consistent with others definitions in asm/io.h > (or alternatively an extern declaration, if the implementation is nontrivial) > > It may be worth pointing out that either way requires including > asm-generic/io.h, > which most architectures don't. This is probably fine, as only csky, riscv and > now arm64 use CONFIG_GENERIC_IOREMAP, and we can probably require > that any further architectures using this symbol also have to use > asm-generic/io.h. It looks the arch is already include it, $ git grep "asm-generic/io.h" arch/ arch/arc/include/asm/io.h:#include <asm-generic/io.h> arch/arm/include/asm/io.h:#include <asm-generic/io.h> arch/arm64/include/asm/io.h:#include <asm-generic/io.h> arch/csky/include/asm/io.h:#include <asm-generic/io.h> arch/h8300/include/asm/io.h:#include <asm-generic/io.h> arch/ia64/include/asm/io.h:#include <asm-generic/io.h> arch/m68k/include/asm/io.h:#include <asm-generic/io.h> arch/m68k/include/asm/io_no.h: * that behavior here first before we include asm-generic/io.h. arch/microblaze/include/asm/io.h:#include <asm-generic/io.h> arch/nios2/include/asm/io.h:#include <asm-generic/io.h> arch/openrisc/include/asm/io.h:#include <asm-generic/io.h> arch/powerpc/include/asm/io.h:#include <asm-generic/io.h> arch/riscv/include/asm/io.h:#include <asm-generic/io.h> arch/s390/include/asm/io.h:#include <asm-generic/io.h> arch/sparc/include/asm/io_32.h:#include <asm-generic/io.h> arch/um/include/asm/io.h:#include <asm-generic/io.h> arch/x86/include/asm/io.h:#include <asm-generic/io.h> arch/xtensa/include/asm/io.h:#include <asm-generic/io.h> > Arnd > > .
On Thu, Apr 28, 2022 at 8:20 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > On 2022/4/28 2:20, Arnd Bergmann wrote: > > On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > > > bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot); > > #define arch_ioremap_check arch_ioremap_check > Ok, I could use this way, and keep consistent with others definitions in > asm/io.h > > (or alternatively an extern declaration, if the implementation is nontrivial) > > > > It may be worth pointing out that either way requires including > > asm-generic/io.h, > > which most architectures don't. This is probably fine, as only csky, riscv and > > now arm64 use CONFIG_GENERIC_IOREMAP, and we can probably require > > that any further architectures using this symbol also have to use > > asm-generic/io.h. > > It looks the arch is already include it, > > $ git grep "asm-generic/io.h" arch/ > > arch/arc/include/asm/io.h:#include <asm-generic/io.h> > arch/arm/include/asm/io.h:#include <asm-generic/io.h> > arch/arm64/include/asm/io.h:#include <asm-generic/io.h> > arch/csky/include/asm/io.h:#include <asm-generic/io.h> > arch/h8300/include/asm/io.h:#include <asm-generic/io.h> > arch/ia64/include/asm/io.h:#include <asm-generic/io.h> > arch/m68k/include/asm/io.h:#include <asm-generic/io.h> > arch/m68k/include/asm/io_no.h: * that behavior here first before we > include asm-generic/io.h. > arch/microblaze/include/asm/io.h:#include <asm-generic/io.h> > arch/nios2/include/asm/io.h:#include <asm-generic/io.h> > arch/openrisc/include/asm/io.h:#include <asm-generic/io.h> > arch/powerpc/include/asm/io.h:#include <asm-generic/io.h> > arch/riscv/include/asm/io.h:#include <asm-generic/io.h> > arch/s390/include/asm/io.h:#include <asm-generic/io.h> > arch/sparc/include/asm/io_32.h:#include <asm-generic/io.h> > arch/um/include/asm/io.h:#include <asm-generic/io.h> > arch/x86/include/asm/io.h:#include <asm-generic/io.h> > arch/xtensa/include/asm/io.h:#include <asm-generic/io.h> Right, it's mostly the older architectures that never started using asm-generic/io.h: $ git grep -L asm-generic/io.h arch/*/include/asm/io.h arch/alpha/include/asm/io.h arch/hexagon/include/asm/io.h arch/mips/include/asm/io.h arch/parisc/include/asm/io.h arch/sh/include/asm/io.h arch/sparc/include/asm/io.h # it is used on sparc32 That's actually less than I expected, and most of these are not seeing a lot of upstream work any more. Arnd
On Thu, Apr 28, 2022 at 02:16:39PM +0800, Kefeng Wang wrote: > > Pet peeve. The word "check" is a poor one. I gives no sense of what > > the function is checking and it gives no sense of how the function's > > return value relates to the thing which it checks. > > > > Maybe it returns 0 on success and -EINVAL on failure. Don't know! > > > > Don't you think that better names would be io_remap_ok(), > > io_remap_valid(), io_remap_allowed(), etc? > > Will use arch_ioremap/unmap_allowed(), and I'd like to keep return bool > > for now if there is no special requirements. Actually, there are a few architectures that can successfully ioreamp without setting up new ptes, e.g. mips. So I think I'd name them just arch_ioremap and arch_iounmap, and return a "void __Ń–omem *" from arch_ioremap, where: - IS_ERR means return an error - NULL means continue to remap - a non-NULL, non-IS_ERR pointer is directly returned.
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h index 7ce93aaf69f8..26924fded7c3 100644 --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -964,6 +964,9 @@ static inline void iounmap(volatile void __iomem *addr) #elif defined(CONFIG_GENERIC_IOREMAP) #include <linux/pgtable.h> +bool arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot); +bool arch_iounmap_check(void __iomem *addr); + void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot); void iounmap(volatile void __iomem *addr); diff --git a/mm/ioremap.c b/mm/ioremap.c index 522ef899c35f..d1117005dcc7 100644 --- a/mm/ioremap.c +++ b/mm/ioremap.c @@ -11,6 +11,16 @@ #include <linux/io.h> #include <linux/export.h> +bool __weak arch_ioremap_check(phys_addr_t addr, size_t size, unsigned long prot) +{ + return true; +} + +bool __weak arch_iounmap_check(void __iomem *addr) +{ + return true; +} + void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot) { unsigned long offset, vaddr; @@ -27,6 +37,9 @@ void __iomem *ioremap_prot(phys_addr_t addr, size_t size, unsigned long prot) addr -= offset; size = PAGE_ALIGN(size + offset); + if (!arch_ioremap_check(addr, size, prot)) + return NULL; + area = get_vm_area_caller(size, VM_IOREMAP, __builtin_return_address(0)); if (!area) @@ -45,6 +58,11 @@ EXPORT_SYMBOL(ioremap_prot); void iounmap(volatile void __iomem *addr) { - vunmap((void *)((unsigned long)addr & PAGE_MASK)); + void *vaddr = (void *)((unsigned long)addr & PAGE_MASK); + + if (!arch_iounmap_check(vaddr)) + return; + + vunmap(vaddr); } EXPORT_SYMBOL(iounmap);
Add special check hook for architecture to verify addr, size or prot when ioremap() or iounmap(), which will make the generic ioremap more useful. Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- include/asm-generic/io.h | 3 +++ mm/ioremap.c | 20 +++++++++++++++++++- 2 files changed, 22 insertions(+), 1 deletion(-)