asm-generic: remove a broken and needless ifdef conditional

[PATCH v2] asm-generic: remove a broken and needless ifdef conditional

Posted by Lukas Bulwahn 3 years, 8 months ago

Commit 527701eda5f1 ("lib: Add a generic version of devmem_is_allowed()")
introduces the config symbol GENERIC_LIB_DEVMEM_IS_ALLOWED, but then
falsely refers to CONFIG_GENERIC_DEVMEM_IS_ALLOWED (note the missing LIB
in the reference) in ./include/asm-generic/io.h.

Luckily, ./scripts/checkkconfigsymbols.py warns on non-existing configs:

GENERIC_DEVMEM_IS_ALLOWED
Referencing files: include/asm-generic/io.h

The actual fix, though, is simply to not to make this function declaration
dependent on any kernel config. For architectures that intend to use
the generic version, the arch's 'select GENERIC_LIB_DEVMEM_IS_ALLOWED' will
lead to picking the function definition, and for other architectures, this
function is simply defined elsewhere.

The wrong '#ifndef' on a non-existing config symbol also always had the
same effect (although more by mistake than by intent). So, there is no
functional change.

Remove this broken and needless ifdef conditional.

Fixes: 527701eda5f1 ("lib: Add a generic version of devmem_is_allowed()")
Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
---
v1: https://lore.kernel.org/all/20211006145859.9564-1-lukas.bulwahn@gmail.com/

Arnd, please pick this v2 for your asm-generic branch. 


 include/asm-generic/io.h | 2 --
 1 file changed, 2 deletions(-)

diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
index ce4c90601300..a68f8fbf423b 100644
--- a/include/asm-generic/io.h
+++ b/include/asm-generic/io.h
@@ -1221,9 +1221,7 @@ static inline void memcpy_toio(volatile void __iomem *addr, const void *buffer,
 }
 #endif
 
-#ifndef CONFIG_GENERIC_DEVMEM_IS_ALLOWED
 extern int devmem_is_allowed(unsigned long pfn);
-#endif
 
 #endif /* __KERNEL__ */
 
-- 
2.17.1

Re: [PATCH v2] asm-generic: remove a broken and needless ifdef conditional

Posted by Luis Chamberlain 3 years, 8 months ago

On Fri, Jul 22, 2022 at 01:07:11PM +0200, Lukas Bulwahn wrote:
> Commit 527701eda5f1 ("lib: Add a generic version of devmem_is_allowed()")
> introduces the config symbol GENERIC_LIB_DEVMEM_IS_ALLOWED, but then
> falsely refers to CONFIG_GENERIC_DEVMEM_IS_ALLOWED (note the missing LIB
> in the reference) in ./include/asm-generic/io.h.
> 
> Luckily, ./scripts/checkkconfigsymbols.py warns on non-existing configs:
> 
> GENERIC_DEVMEM_IS_ALLOWED
> Referencing files: include/asm-generic/io.h
> 
> The actual fix, though, is simply to not to make this function declaration
> dependent on any kernel config. For architectures that intend to use
> the generic version, the arch's 'select GENERIC_LIB_DEVMEM_IS_ALLOWED' will
> lead to picking the function definition, and for other architectures, this
> function is simply defined elsewhere.
> 
> The wrong '#ifndef' on a non-existing config symbol also always had the
> same effect (although more by mistake than by intent). So, there is no
> functional change.
> 
> Remove this broken and needless ifdef conditional.
> 
> Fixes: 527701eda5f1 ("lib: Add a generic version of devmem_is_allowed()")
> Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> ---

There is only one architecture which defines this as a static inline.
Should that architecture include asm-generic/io.h, or if it already
does, it may end up with a compilation error.

So if arch/s390/include/asm/page.h can end up including asm-generic/io.h
or if any file including for s390 can include its arch page.h and
asm-generic/io.h then we'll still need the guard.

This may compile today for s390 because s390 may not use asm-generic/io.h.

So why not just keep the guard and correct this as intended?

  Luis

> v1: https://lore.kernel.org/all/20211006145859.9564-1-lukas.bulwahn@gmail.com/
> 
> Arnd, please pick this v2 for your asm-generic branch. 
> 
> 
>  include/asm-generic/io.h | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/include/asm-generic/io.h b/include/asm-generic/io.h
> index ce4c90601300..a68f8fbf423b 100644
> --- a/include/asm-generic/io.h
> +++ b/include/asm-generic/io.h
> @@ -1221,9 +1221,7 @@ static inline void memcpy_toio(volatile void __iomem *addr, const void *buffer,
>  }
>  #endif
>  
> -#ifndef CONFIG_GENERIC_DEVMEM_IS_ALLOWED
>  extern int devmem_is_allowed(unsigned long pfn);
> -#endif
>  
>  #endif /* __KERNEL__ */
>  
> -- 
> 2.17.1
>

Re: [PATCH v2] asm-generic: remove a broken and needless ifdef conditional

Posted by Arnd Bergmann 3 years, 8 months ago

On Fri, Jul 22, 2022 at 4:53 PM Luis Chamberlain <mcgrof@kernel.org> wrote:
>
> On Fri, Jul 22, 2022 at 01:07:11PM +0200, Lukas Bulwahn wrote:
> > Commit 527701eda5f1 ("lib: Add a generic version of devmem_is_allowed()")
> > introduces the config symbol GENERIC_LIB_DEVMEM_IS_ALLOWED, but then
> > falsely refers to CONFIG_GENERIC_DEVMEM_IS_ALLOWED (note the missing LIB
> > in the reference) in ./include/asm-generic/io.h.
> >
> > Luckily, ./scripts/checkkconfigsymbols.py warns on non-existing configs:
> >
> > GENERIC_DEVMEM_IS_ALLOWED
> > Referencing files: include/asm-generic/io.h
> >
> > The actual fix, though, is simply to not to make this function declaration
> > dependent on any kernel config. For architectures that intend to use
> > the generic version, the arch's 'select GENERIC_LIB_DEVMEM_IS_ALLOWED' will
> > lead to picking the function definition, and for other architectures, this
> > function is simply defined elsewhere.
> >
> > The wrong '#ifndef' on a non-existing config symbol also always had the
> > same effect (although more by mistake than by intent). So, there is no
> > functional change.
> >
> > Remove this broken and needless ifdef conditional.
> >
> > Fixes: 527701eda5f1 ("lib: Add a generic version of devmem_is_allowed()")
> > Signed-off-by: Lukas Bulwahn <lukas.bulwahn@gmail.com>
> > ---
>
> There is only one architecture which defines this as a static inline.
> Should that architecture include asm-generic/io.h, or if it already
> does, it may end up with a compilation error.
>
> So if arch/s390/include/asm/page.h can end up including asm-generic/io.h
> or if any file including for s390 can include its arch page.h and
> asm-generic/io.h then we'll still need the guard.
>
> This may compile today for s390 because s390 may not use asm-generic/io.h.
>

It looks like we never concluded this thread. I double-checked this
on s390 and confirmed that it has both the inline definition and the
extern declaration visible and this does not cause problems on any of
the compilers I tried (gcc-5 and gcc-12).

Generally speaking there is not really a need to hide extern declarations
behind an ifdef.

> So why not just keep the guard and correct this as intended?

That was what the original patch from Lukas did, but it caused regressions
because configurations actually relied on the declaration even when the
Kconfig option is disabled.

The current version of the patch was in linux-next for a while now without
causing extra issues, so I'm sending it out in my final fixes pull request
for 5.19.

      Arnd