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

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

[Min-Hsun Chang]

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.

Following the pattern in commit 44238e82670e ("rcu: Replace ________p1 and
_________p1 with __UNIQUE_ID(rcu)") and commit 589a9785ee3a ("min/max:
remove sparse warnings when they're nested"), replace the hardcoded
identifier with __UNIQUE_ID(addr).

Since __UNIQUE_ID() must be expanded once to remain consistent across
declaration, assignment, and return within the statement expression,
introduce a nested helper macro ___set_fixmap_offset.

Signed-off-by: Min-Hsun Chang <chmh0624@gmail.com>
---
 include/asm-generic/fixmap.h | 16 ++++++++++------
 1 file changed, 10 insertions(+), 6 deletions(-)

diff --git a/include/asm-generic/fixmap.h b/include/asm-generic/fixmap.h
index 29cab7947980..2d1e9a2c6a3b 100644
--- a/include/asm-generic/fixmap.h
+++ b/include/asm-generic/fixmap.h
@@ -16,6 +16,7 @@
 #define __ASM_GENERIC_FIXMAP_H
 
 #include <linux/bug.h>
+#include <linux/compiler.h>
 #include <linux/mm_types.h>
 
 #define __fix_to_virt(x)	(FIXADDR_TOP - ((x) << PAGE_SHIFT))
@@ -71,14 +72,17 @@ static inline unsigned long virt_to_fix(const unsigned long vaddr)
 #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;							\
+#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;                                                       \
 })
 
+#define __set_fixmap_offset(idx, phys, flags)                   \
+	___set_fixmap_offset(idx, phys, flags, __UNIQUE_ID(addr))
+
 #define set_fixmap_offset(idx, phys) \
 	__set_fixmap_offset(idx, phys, FIXMAP_PAGE_NORMAL)
 
-- 
2.50.1

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

[Min-Hsun Chang]

Hi Arnd,

I'm sending a gentle ping regarding this patch to replace the hardcoded
identifier in __set_fixmap_offset with __UNIQUE_ID(addr).

Please let me know if any changes are needed.

Thanks.

Min-Hsun Chang

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

[David Laight]

On Sat,  7 Mar 2026 17:21:19 +0800
Min-Hsun Chang <chmh0624@gmail.com> wrote:

> 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.
> 
> Following the pattern in commit 44238e82670e ("rcu: Replace ________p1 and
> _________p1 with __UNIQUE_ID(rcu)") and commit 589a9785ee3a ("min/max:
> remove sparse warnings when they're nested"), replace the hardcoded
> identifier with __UNIQUE_ID(addr).
> 
> Since __UNIQUE_ID() must be expanded once to remain consistent across
> declaration, assignment, and return within the statement expression,
> introduce a nested helper macro ___set_fixmap_offset.

I'm not sure that is really relevant here - nested use is probably
impossible.
It is almost certainly enough to prefix locals with an leading _
and an abbreviation of the macro name.

But see below. 


> 
> Signed-off-by: Min-Hsun Chang <chmh0624@gmail.com>
> ---
>  include/asm-generic/fixmap.h | 16 ++++++++++------
>  1 file changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/include/asm-generic/fixmap.h b/include/asm-generic/fixmap.h
> index 29cab7947980..2d1e9a2c6a3b 100644
> --- a/include/asm-generic/fixmap.h
> +++ b/include/asm-generic/fixmap.h
> @@ -16,6 +16,7 @@
>  #define __ASM_GENERIC_FIXMAP_H
>  
>  #include <linux/bug.h>
> +#include <linux/compiler.h>
>  #include <linux/mm_types.h>
>  
>  #define __fix_to_virt(x)	(FIXADDR_TOP - ((x) << PAGE_SHIFT))
> @@ -71,14 +72,17 @@ static inline unsigned long virt_to_fix(const unsigned long vaddr)
>  #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;							\
> +#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.
So you either need to copy those to locals or use a static inline
function - which should be ok here because the types are fixed.
(Assuming neither __set_fixmap() or fix_to_virt() are #defines
that modify their arguments.)

	David


>  })
>  
> +#define __set_fixmap_offset(idx, phys, flags)                   \
> +	___set_fixmap_offset(idx, phys, flags, __UNIQUE_ID(addr))
> +
>  #define set_fixmap_offset(idx, phys) \
>  	__set_fixmap_offset(idx, phys, FIXMAP_PAGE_NORMAL)
>  

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

[Andrew Morton]

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
_

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

[David Laight]

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
> _
> 

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

[Andrew Morton]

On Sat, 25 Apr 2026 23:01:34 +0100 David Laight <david.laight.linux@gmail.com> wrote:

> > > 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.

Gee, if `static inline' misbehaves then we have big problems!

What's special about the fixmap code anyway?  It's not exactly
fastpath.  Perhaps this stuff can simply be uninlined.

Pet peeves:

We use too many macros.

We inline too much stuff.

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

[David Laight]

On Sat, 25 Apr 2026 15:12:40 -0700
Andrew Morton <akpm@linux-foundation.org> wrote:

> On Sat, 25 Apr 2026 23:01:34 +0100 David Laight <david.laight.linux@gmail.com> wrote:
> 
> > > > 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.  
> 
> Gee, if `static inline' misbehaves then we have big problems!
> 
> What's special about the fixmap code anyway?  It's not exactly
> fastpath.  Perhaps this stuff can simply be uninlined.

Some of the inlines are trivial - just adding an extra parameter.
But this set would be simpler if the last function
(__native_set_fixmap() for x86) returned (address & ~PAGE_MASK)

> 
> Pet peeves:
> 
> We use too many macros.

Indeed, I get fed up of looking things up to find they are trivial.
Some of the 'helpers' do nothing for core readability.
Even things like BIT() can have unexpected consequences.

Not to say I haven't got the pre-processor to do odd things in the past.
But it is usually trying to avoid having to keep multiple definitions
aligned with each other.

> We inline too much stuff.

True - don't look at what strlcpy() can generate.
The inline code should just get the constants from the compiler and
then call the appropriate function.

As does the compiler.
pixpaper_panel_hw_init() repeatedly calls two static functions that
contain sleeps. They all get inlined bloating the code size and
exploding the stack when clang separately allocates all the buffers
in the called functions.

	David

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

[Andrew Morton]

On Sun, 26 Apr 2026 11:49:38 +0100 David Laight <david.laight.linux@gmail.com> wrote:

> On Sat, 25 Apr 2026 15:12:40 -0700
> Andrew Morton <akpm@linux-foundation.org> wrote:
> 
> > On Sat, 25 Apr 2026 23:01:34 +0100 David Laight <david.laight.linux@gmail.com> wrote:
> > 
> > > > > 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.  
> > 
> > Gee, if `static inline' misbehaves then we have big problems!
> > 
> > What's special about the fixmap code anyway?  It's not exactly
> > fastpath.  Perhaps this stuff can simply be uninlined.
> 
> Some of the inlines are trivial - just adding an extra parameter.
> But this set would be simpler if the last function
> (__native_set_fixmap() for x86) returned (address & ~PAGE_MASK)

I don't understand this?

> > 
> > Pet peeves:
> > 
> > We use too many macros.
> 
> Indeed, I get fed up of looking things up to find they are trivial.
> Some of the 'helpers' do nothing for core readability.
> Even things like BIT() can have unexpected consequences.

Sure.

> Not to say I haven't got the pre-processor to do odd things in the past.
> But it is usually trying to avoid having to keep multiple definitions
> aligned with each other.
> 
> > We inline too much stuff.
> 
> True - don't look at what strlcpy() can generate.
> The inline code should just get the constants from the compiler and
> then call the appropriate function.

I can't actually find an in-kernel strlcpy()?

> As does the compiler.
> pixpaper_panel_hw_init() repeatedly calls two static functions that
> contain sleeps. They all get inlined bloating the code size and
> exploding the stack when clang separately allocates all the buffers
> in the called functions.

Poor thing.  I guess compiler developers are more focused on userspace
code and aren't especially concerned about kernel's particular
requirements.  And that's understandable - it's down to us to persuade
them to add options to permit us to tune the compiler behavior.