Re: [PATCH] asm-generic: replace ________addr with __UNIQUE_ID(addr)

David Laight
2026-04-25T22:01:37Z

On Sat, 25 Apr 2026 13:57:37 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Sun, 22 Mar 2026 14:40:32 +0000 David Laight <david.laight.linux@gmail.com> wrote:
>
> > > -#define __set_fixmap_offset(idx, phys, flags) \
> > > -({ \
> > > - unsigned long ________addr; \
> > > - __set_fixmap(idx, phys, flags); \
> > > - ________addr = fix_to_virt(idx) + ((phys) & (PAGE_SIZE - 1)); \
> > > - ________addr; \
> > > +#define ___set_fixmap_offset(idx, phys, flags, uniq) \
> > > +({ \
> > > + unsigned long uniq; \
> > > + __set_fixmap(idx, phys, flags); \
> > > + uniq = fix_to_virt(idx) + ((phys) & (PAGE_SIZE - 1)); \
> > > + uniq; \
> >
> > You don't need a variable to hold the result at all.
> >
> > The real problem with this define is that both idx and phys are
> > expanded twice.
>
> The real problem with this define is that it's a define. Why oh why do
> we keep doing this to ourselves?

Sometimes #defines generate better code because they are expanded earlier,
and sometimes you want type-agnostic 'functions'.
But neither is true here.

But I think I'd go for 'always_inline'.
Sometimes the compilers make silly decisions.

David

>
>
> What's wrong with the below?
>
> /* Return a pointer with offset calculated */
> static inline unsigned long
> __set_fixmap_offset(enum fixed_addresses idx, phys_addr_t phys, pgprot_t flags)
> {
> __set_fixmap(idx, phys, flags);
> return fix_to_virt(idx) + (phys & (PAGE_SIZE - 1));
> }
>
> static inline unsigned long
> set_fixmap_offset(enum fixed_addresses idx, phys_addr_t phys)
> {
> return __set_fixmap_offset(idx, phys, FIXMAP_PAGE_NORMAL);
> }
>
> /*
> * Some hardware wants to get fixmapped without caching.
> */
> static inline void
> set_fixmap_nocache(enum fixed_addresses idx, phys_addr_t phys)
> {
> __set_fixmap(idx, phys, FIXMAP_PAGE_NOCACHE);
> }
>
> static inline void
> set_fixmap_offset_nocache(enum fixed_addresses idx, phys_addr_t phys)
> {
> __set_fixmap_offset(idx, phys, FIXMAP_PAGE_NOCACHE);
> }
>
>
> I'll toss the below into mm.git, shall send it to Arnd if nothing blows
> up.
>
>
> From: Andrew Morton <akpm@linux-foundation.org>
> Subject: include/asm-generic/fixmap.h: reimplement nasty macros in C
> Date: Sat Apr 25 01:42:28 PM PDT 2026
>
> Min-Hsun Chang reports[1] "the macro __set_fixmap_offset() uses a
> hardcoded identifier ________addr, which can lead to variable name
> shadowing if a caller happens to use the same name in its scope."
>
> As is usual with macro messes, the answer is to reimplement everything in
> C.
>
> Reported-by: Min-Hsun Chang <chmh0624@gmail.com>
> Closes: https://lore.kernel.org/20260307092119.20733-1-chmh0624@gmail.com [1]
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Mark Salter <msalter@redhat.com>
> Cc: David Laight <david.laight.linux@gmail.com>
> Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
> ---
>
> include/asm-generic/fixmap.h | 34 ++++++++++++++++++++-------------
> 1 file changed, 21 insertions(+), 13 deletions(-)
>
> --- a/include/asm-generic/fixmap.h~a
> +++ a/include/asm-generic/fixmap.h
> @@ -71,25 +71,33 @@ static inline unsigned long virt_to_fix(
> #endif
>
> /* Return a pointer with offset calculated */
> -#define __set_fixmap_offset(idx, phys, flags) \
> -({ \
> - unsigned long ________addr; \
> - __set_fixmap(idx, phys, flags); \
> - ________addr = fix_to_virt(idx) + ((phys) & (PAGE_SIZE - 1)); \
> - ________addr; \
> -})
> +static inline unsigned long
> +__set_fixmap_offset(enum fixed_addresses idx, phys_addr_t phys, pgprot_t flags)
> +{
> + __set_fixmap(idx, phys, flags);
> + return fix_to_virt(idx) + (phys & (PAGE_SIZE - 1));
> +}
>
> -#define set_fixmap_offset(idx, phys) \
> - __set_fixmap_offset(idx, phys, FIXMAP_PAGE_NORMAL)
> +static inline unsigned long
> +set_fixmap_offset(enum fixed_addresses idx, phys_addr_t phys)
> +{
> + return __set_fixmap_offset(idx, phys, FIXMAP_PAGE_NORMAL);
> +}
>
> /*
> * Some hardware wants to get fixmapped without caching.
> */
> -#define set_fixmap_nocache(idx, phys) \
> - __set_fixmap(idx, phys, FIXMAP_PAGE_NOCACHE)
> +static inline void
> +set_fixmap_nocache(enum fixed_addresses idx, phys_addr_t phys)
> +{
> + __set_fixmap(idx, phys, FIXMAP_PAGE_NOCACHE);
> +}
>
> -#define set_fixmap_offset_nocache(idx, phys) \
> - __set_fixmap_offset(idx, phys, FIXMAP_PAGE_NOCACHE)
> +static inline void
> +set_fixmap_offset_nocache(enum fixed_addresses idx, phys_addr_t phys)
> +{
> + __set_fixmap_offset(idx, phys, FIXMAP_PAGE_NOCACHE);
> +}
>
> /*
> * Some fixmaps are for IO
> _
>