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

Andrew Morton
2026-04-25T20:57:38Z

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?

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
_