| Message ID | 20220519082552.117736-5-wangkefeng.wang@huawei.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | arm64: Cleanup ioremap() and support ioremap_prot() | expand |
On Thu, 19 May 2022 16:25:50 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > Add special hook for architecture to verify or setup addr, size > or prot when ioremap() or iounmap(), which will make the generic > ioremap more useful. > > arch_ioremap() return a pointer, > - IS_ERR means return an error > - NULL means continue to remap > - a non-NULL, non-IS_ERR pointer is directly returned > arch_iounmap() return a int value, > - 0 means continue to vunmap > - error code means skip vunmap and return directly > > ... > > --- a/include/asm-generic/io.h > +++ b/include/asm-generic/io.h > @@ -964,6 +964,30 @@ static inline void iounmap(volatile void __iomem *addr) > #elif defined(CONFIG_GENERIC_IOREMAP) > #include <linux/pgtable.h> > > +/* > + * Arch code can implement the following two special hooks when using GENERIC_IOREMAP > + * arch_ioremap() return a pointer, > + * - 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 int, > + * - 0 means continue to vunmap > + * - error code means skip vunmap and return directly > + */ > +#ifndef arch_ioremap > +static inline void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot) > +{ > + return NULL; > +} Maybe should do #define arch_ioremap arch_ioremap here > +#endif > + > +#ifndef arch_iounmap > +static inline int arch_iounmap(void __iomem *addr) > +{ > + return 0; > +} and here. It shouldn't matter a lot because this file has inclusion guards. However it seems tidier and perhaps other code will want to know whether this was defined. Dunno. Otherwise, Acked-by: Andrew Morton <akpm@linux-foundation.org> Please take this patch and [2/6] and [3/6] via the appropriate arm tree.
On 2022/5/20 2:52, Andrew Morton wrote: > On Thu, 19 May 2022 16:25:50 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >> Add special hook for architecture to verify or setup addr, size >> or prot when ioremap() or iounmap(), which will make the generic >> ioremap more useful. >> >> arch_ioremap() return a pointer, >> - IS_ERR means return an error >> - NULL means continue to remap >> - a non-NULL, non-IS_ERR pointer is directly returned >> arch_iounmap() return a int value, >> - 0 means continue to vunmap >> - error code means skip vunmap and return directly >> >> ... >> >> --- a/include/asm-generic/io.h >> +++ b/include/asm-generic/io.h >> @@ -964,6 +964,30 @@ static inline void iounmap(volatile void __iomem *addr) >> #elif defined(CONFIG_GENERIC_IOREMAP) >> #include <linux/pgtable.h> >> >> +/* >> + * Arch code can implement the following two special hooks when using GENERIC_IOREMAP >> + * arch_ioremap() return a pointer, >> + * - 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 int, >> + * - 0 means continue to vunmap >> + * - error code means skip vunmap and return directly >> + */ >> +#ifndef arch_ioremap >> +static inline void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot) >> +{ >> + return NULL; >> +} > Maybe should do > > #define arch_ioremap arch_ioremap > > here > >> +#endif >> + >> +#ifndef arch_iounmap >> +static inline int arch_iounmap(void __iomem *addr) >> +{ >> + return 0; >> +} > and here. > > It shouldn't matter a lot because this file has inclusion guards. > However it seems tidier and perhaps other code will want to know > whether this was defined. Dunno. > Oh, forget to add the define part, thanks Andrew. Hi Catalin, could you help to involve them when taking them, many thanks. > Otherwise, > > Acked-by: Andrew Morton <akpm@linux-foundation.org> > > Please take this patch and [2/6] and [3/6] via the appropriate arm tree. > > .
On 5/20/22 00:22, Andrew Morton wrote: > On Thu, 19 May 2022 16:25:50 +0800 Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >> Add special hook for architecture to verify or setup addr, size >> or prot when ioremap() or iounmap(), which will make the generic >> ioremap more useful. >> >> arch_ioremap() return a pointer, >> - IS_ERR means return an error >> - NULL means continue to remap >> - a non-NULL, non-IS_ERR pointer is directly returned >> arch_iounmap() return a int value, >> - 0 means continue to vunmap >> - error code means skip vunmap and return directly >> >> ... >> >> --- a/include/asm-generic/io.h >> +++ b/include/asm-generic/io.h >> @@ -964,6 +964,30 @@ static inline void iounmap(volatile void __iomem *addr) >> #elif defined(CONFIG_GENERIC_IOREMAP) >> #include <linux/pgtable.h> >> >> +/* >> + * Arch code can implement the following two special hooks when using GENERIC_IOREMAP >> + * arch_ioremap() return a pointer, >> + * - 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 int, >> + * - 0 means continue to vunmap >> + * - error code means skip vunmap and return directly >> + */ >> +#ifndef arch_ioremap >> +static inline void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot) >> +{ >> + return NULL; >> +} > > Maybe should do > > #define arch_ioremap arch_ioremap > > here > >> +#endif >> + >> +#ifndef arch_iounmap >> +static inline int arch_iounmap(void __iomem *addr) >> +{ >> + return 0; >> +} > > and here. > > It shouldn't matter a lot because this file has inclusion guards. > However it seems tidier and perhaps other code will want to know > whether this was defined. Dunno. +1, agreed.
Hi Kefeng,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on arm64/for-next/core]
[also build test WARNING on arnd-asm-generic/master akpm-mm/mm-everything linus/master v5.18 next-20220523]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/intel-lab-lkp/linux/commits/Kefeng-Wang/arm64-Cleanup-ioremap-and-support-ioremap_prot/20220519-161853
base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core
config: csky-randconfig-s032-20220522 (https://download.01.org/0day-ci/archive/20220524/202205240657.BXxrhbgp-lkp@intel.com/config)
compiler: csky-linux-gcc (GCC) 11.3.0
reproduce:
wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
chmod +x ~/bin/make.cross
# apt-get install sparse
# sparse version: v0.6.4-dirty
# https://github.com/intel-lab-lkp/linux/commit/eb83d30756d3b279ca58791d343dc821291818d0
git remote add linux-review https://github.com/intel-lab-lkp/linux
git fetch --no-tags linux-review Kefeng-Wang/arm64-Cleanup-ioremap-and-support-ioremap_prot/20220519-161853
git checkout eb83d30756d3b279ca58791d343dc821291818d0
# save the config file
mkdir build_dir && cp config build_dir/.config
COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=csky SHELL=/bin/bash
If you fix the issue, kindly add following tag where applicable
Reported-by: kernel test robot <lkp@intel.com>
sparse warnings: (new ones prefixed by >>)
>> mm/ioremap.c:59:16: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const *addr @@ got void [noderef] __iomem *vaddr @@
mm/ioremap.c:59:16: sparse: expected void const *addr
mm/ioremap.c:59:16: sparse: got void [noderef] __iomem *vaddr
vim +59 mm/ioremap.c
51
52 void iounmap(volatile void __iomem *addr)
53 {
54 void __iomem *vaddr = (void __iomem *)((unsigned long)addr & PAGE_MASK);
55
56 if (arch_iounmap(vaddr))
57 return;
58
> 59 vunmap(vaddr);
On 2022/5/24 6:17, kernel test robot wrote: > Hi Kefeng, > > Thank you for the patch! Perhaps something to improve: > > [auto build test WARNING on arm64/for-next/core] > [also build test WARNING on arnd-asm-generic/master akpm-mm/mm-everything linus/master v5.18 next-20220523] > [If your patch is applied to the wrong git tree, kindly drop us a note. > And when submitting patch, we suggest to use '--base' as documented in > https://git-scm.com/docs/git-format-patch] > > url: https://github.com/intel-lab-lkp/linux/commits/Kefeng-Wang/arm64-Cleanup-ioremap-and-support-ioremap_prot/20220519-161853 > base: https://git.kernel.org/pub/scm/linux/kernel/git/arm64/linux.git for-next/core > config: csky-randconfig-s032-20220522 (https://download.01.org/0day-ci/archive/20220524/202205240657.BXxrhbgp-lkp@intel.com/config) > compiler: csky-linux-gcc (GCC) 11.3.0 > reproduce: > wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross > chmod +x ~/bin/make.cross > # apt-get install sparse > # sparse version: v0.6.4-dirty > # https://github.com/intel-lab-lkp/linux/commit/eb83d30756d3b279ca58791d343dc821291818d0 > git remote add linux-review https://github.com/intel-lab-lkp/linux > git fetch --no-tags linux-review Kefeng-Wang/arm64-Cleanup-ioremap-and-support-ioremap_prot/20220519-161853 > git checkout eb83d30756d3b279ca58791d343dc821291818d0 > # save the config file > mkdir build_dir && cp config build_dir/.config > COMPILER_INSTALL_PATH=$HOME/0day COMPILER=gcc-11.3.0 make.cross C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' O=build_dir ARCH=csky SHELL=/bin/bash > > If you fix the issue, kindly add following tag where applicable > Reported-by: kernel test robot <lkp@intel.com> > > > sparse warnings: (new ones prefixed by >>) >>> mm/ioremap.c:59:16: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const *addr @@ got void [noderef] __iomem *vaddr @@ > mm/ioremap.c:59:16: sparse: expected void const *addr > mm/ioremap.c:59:16: sparse: got void [noderef] __iomem *vaddr > > vim +59 mm/ioremap.c > > 51 > 52 void iounmap(volatile void __iomem *addr) > 53 { > 54 void __iomem *vaddr = (void __iomem *)((unsigned long)addr & PAGE_MASK); > 55 > 56 if (arch_iounmap(vaddr)) > 57 return; > 58 > > 59 vunmap(vaddr); 1) Will add generic "arch_ioremap/arch_iounmap define" 2) and change this to vunmap((void *)vaddr); >
On Tue, May 24, 2022 at 11:48 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >>> mm/ioremap.c:59:16: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const *addr @@ got void [noderef] __iomem *vaddr @@ > > mm/ioremap.c:59:16: sparse: expected void const *addr > > mm/ioremap.c:59:16: sparse: got void [noderef] __iomem *vaddr > > > > vim +59 mm/ioremap.c > > > > 51 > > 52 void iounmap(volatile void __iomem *addr) > > 53 { > > 54 void __iomem *vaddr = (void __iomem *)((unsigned long)addr & PAGE_MASK); > > 55 > > 56 if (arch_iounmap(vaddr)) > > 57 return; > > 58 > > > 59 vunmap(vaddr); > > 1) Will add generic "arch_ioremap/arch_iounmap define" > > 2) and change this to vunmap((void *)vaddr); I think this need an extra __force to actually suppress the sparse warning, as in vunmap((void __force *)vaddr); Using __force is usually wrong, this is one of the exceptions, so maybe add a comment as well. Arnd
On Thu, May 19, 2022 at 10:25 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > > Add special hook for architecture to verify or setup addr, size > or prot when ioremap() or iounmap(), which will make the generic > ioremap more useful. > > arch_ioremap() return a pointer, > - IS_ERR means return an error > - NULL means continue to remap > - a non-NULL, non-IS_ERR pointer is directly returned > arch_iounmap() return a int value, > - 0 means continue to vunmap > - error code means skip vunmap and return directly > > Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> I don't really like interfaces that mix error pointers and NULL pointer returns. Would it be possible to have a special error code other than NULL for the fallback case? arnd
On 2022/5/24 20:35, Arnd Bergmann wrote: > On Tue, May 24, 2022 at 11:48 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: >>>>> mm/ioremap.c:59:16: sparse: sparse: incorrect type in argument 1 (different address spaces) @@ expected void const *addr @@ got void [noderef] __iomem *vaddr @@ >>> mm/ioremap.c:59:16: sparse: expected void const *addr >>> mm/ioremap.c:59:16: sparse: got void [noderef] __iomem *vaddr >>> >>> vim +59 mm/ioremap.c >>> >>> 51 >>> 52 void iounmap(volatile void __iomem *addr) >>> 53 { >>> 54 void __iomem *vaddr = (void __iomem *)((unsigned long)addr & PAGE_MASK); >>> 55 >>> 56 if (arch_iounmap(vaddr)) >>> 57 return; >>> 58 >>> > 59 vunmap(vaddr); >> 1) Will add generic "arch_ioremap/arch_iounmap define" >> >> 2) and change this to vunmap((void *)vaddr); > I think this need an extra __force to actually suppress the sparse > warning, as in > > vunmap((void __force *)vaddr); > > Using __force is usually wrong, this is one of the exceptions, so > maybe add a comment > as well. Right, I found this too， and using ___force in local, will update, thank. > > Arnd > > .
On 2022/5/24 20:37, Arnd Bergmann wrote: > On Thu, May 19, 2022 at 10:25 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: >> Add special hook for architecture to verify or setup addr, size >> or prot when ioremap() or iounmap(), which will make the generic >> ioremap more useful. >> >> arch_ioremap() return a pointer, >> - IS_ERR means return an error >> - NULL means continue to remap >> - a non-NULL, non-IS_ERR pointer is directly returned >> arch_iounmap() return a int value, >> - 0 means continue to vunmap >> - error code means skip vunmap and return directly >> >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > I don't really like interfaces that mix error pointers and NULL pointer > returns. > > Would it be possible to have a special error code other than NULL > for the fallback case? I don't find a good error code, maybe ENOTSUPP, any better suggestion? > > arnd > > .
On Tue, May 24, 2022 at 4:32 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > On 2022/5/24 20:37, Arnd Bergmann wrote: > > On Thu, May 19, 2022 at 10:25 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: > >> Add special hook for architecture to verify or setup addr, size > >> or prot when ioremap() or iounmap(), which will make the generic > >> ioremap more useful. > >> > >> arch_ioremap() return a pointer, > >> - IS_ERR means return an error > >> - NULL means continue to remap > >> - a non-NULL, non-IS_ERR pointer is directly returned > >> arch_iounmap() return a int value, > >> - 0 means continue to vunmap > >> - error code means skip vunmap and return directly > >> > >> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> > > I don't really like interfaces that mix error pointers and NULL pointer > > returns. > > > > Would it be possible to have a special error code other than NULL > > for the fallback case? > > I don't find a good error code, maybe ENOTSUPP, any better suggestion? I had another look at the resulting arm64 function, and it appears that you never actually return a non-error pointer here. If I didn't miss anything, I think the best way would be to change the return type to just indicate success or an error code, and drop the case of returning the actual result, and changing the function name accordingly. Would that work, or do you actually require returning an __iomem token from somewhere else? Arnd
On 2022/5/24 22:47, Arnd Bergmann wrote: > On Tue, May 24, 2022 at 4:32 PM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: >> On 2022/5/24 20:37, Arnd Bergmann wrote: >>> On Thu, May 19, 2022 at 10:25 AM Kefeng Wang <wangkefeng.wang@huawei.com> wrote: >>>> Add special hook for architecture to verify or setup addr, size >>>> or prot when ioremap() or iounmap(), which will make the generic >>>> ioremap more useful. >>>> >>>> arch_ioremap() return a pointer, >>>> - IS_ERR means return an error >>>> - NULL means continue to remap >>>> - a non-NULL, non-IS_ERR pointer is directly returned >>>> arch_iounmap() return a int value, >>>> - 0 means continue to vunmap >>>> - error code means skip vunmap and return directly >>>> >>>> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> >>> I don't really like interfaces that mix error pointers and NULL pointer >>> returns. >>> >>> Would it be possible to have a special error code other than NULL >>> for the fallback case? >> I don't find a good error code, maybe ENOTSUPP, any better suggestion? > I had another look at the resulting arm64 function, and it appears that > you never actually return a non-error pointer here. If I didn't miss anything, > I think the best way would be to change the return type to just indicate > success or an error code, and drop the case of returning the actual result, > and changing the function name accordingly. > > Would that work, or do you actually require returning an __iomem > token from somewhere else? Christoph suggested in the first version, https://lore.kernel.org/linux-arm-kernel/Ymq2uX%2FY15HlIpo7@infradead.org/ > Arnd > > .
On Tue, May 24, 2022 at 10:53:54PM +0800, Kefeng Wang wrote: > Christoph suggested in the first version, > > https://lore.kernel.org/linux-arm-kernel/Ymq2uX%2FY15HlIpo7@infradead.org/ Yeah, mostly to eventually also covers mips. But if this causes problems now please just dumb it down and we can revisit the interface when actually needed.
On 2022/5/31 14:13, Christoph Hellwig wrote: > On Tue, May 24, 2022 at 10:53:54PM +0800, Kefeng Wang wrote: >> Christoph suggested in the first version, >> >> https://lore.kernel.org/linux-arm-kernel/Ymq2uX%2FY15HlIpo7@infradead.org/ > Yeah, mostly to eventually also covers mips. But if this causes > problems now please just dumb it down and we can revisit the interface > when actually needed. > . OK， will rethink the return value and resend after 5.19-rc1， thanks all.
diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h index e6ffa2519f08..b60f7151e1d6 100644 --- a/include/asm-generic/io.h +++ b/include/asm-generic/io.h @@ -964,6 +964,30 @@ static inline void iounmap(volatile void __iomem *addr) #elif defined(CONFIG_GENERIC_IOREMAP) #include <linux/pgtable.h> +/* + * Arch code can implement the following two special hooks when using GENERIC_IOREMAP + * arch_ioremap() return a pointer, + * - 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 int, + * - 0 means continue to vunmap + * - error code means skip vunmap and return directly + */ +#ifndef arch_ioremap +static inline void __iomem *arch_ioremap(phys_addr_t phys_addr, size_t size, unsigned long prot) +{ + return NULL; +} +#endif + +#ifndef arch_iounmap +static inline int arch_iounmap(void __iomem *addr) +{ + return 0; +} +#endif + void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long prot); void iounmap(volatile void __iomem *addr); diff --git a/mm/ioremap.c b/mm/ioremap.c index 7cb9996b0c12..fac7f23b8c4b 100644 --- a/mm/ioremap.c +++ b/mm/ioremap.c @@ -16,6 +16,7 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long pro 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; @@ -27,8 +28,13 @@ void __iomem *ioremap_prot(phys_addr_t phys_addr, size_t size, unsigned long pro phys_addr -= offset; size = PAGE_ALIGN(size + offset); - area = get_vm_area_caller(size, VM_IOREMAP, - __builtin_return_address(0)); + base = arch_ioremap(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)); if (!area) return NULL; vaddr = (unsigned long)area->addr; @@ -45,6 +51,11 @@ EXPORT_SYMBOL(ioremap_prot); void iounmap(volatile void __iomem *addr) { - vunmap((void *)((unsigned long)addr & PAGE_MASK)); + void __iomem *vaddr = (void __iomem *)((unsigned long)addr & PAGE_MASK); + + if (arch_iounmap(vaddr)) + return; + + vunmap(vaddr); } EXPORT_SYMBOL(iounmap);
Add special hook for architecture to verify or setup addr, size or prot when ioremap() or iounmap(), which will make the generic ioremap more useful. arch_ioremap() return a pointer, - IS_ERR means return an error - NULL means continue to remap - a non-NULL, non-IS_ERR pointer is directly returned arch_iounmap() return a int value, - 0 means continue to vunmap - error code means skip vunmap and return directly Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com> --- include/asm-generic/io.h | 24 ++++++++++++++++++++++++ mm/ioremap.c | 17 ++++++++++++++--- 2 files changed, 38 insertions(+), 3 deletions(-)