On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <[email protected]> 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 <[email protected]> wrote:
> On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <[email protected]> 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 <[email protected]> wrote:
> On Wed, 27 Apr 2022 20:20:30 +0200 Arnd Bergmann <[email protected]> wrote:
> > On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <[email protected]> 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 2:20, Arnd Bergmann wrote:
> On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <[email protected]> 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 <[email protected]> wrote:
> On 2022/4/28 2:20, Arnd Bergmann wrote:
> > On Wed, Apr 27, 2022 at 2:14 PM Kefeng Wang <[email protected]> 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