[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)
>  

[PATCH v2] asm-generic: convert __set_fixmap_offset() to static inline

[Min-Hsun Chang]

The current macro implementation of __set_fixmap_offset() uses a
hardcoded identifier `________addr` within a statement expression.
This can lead to variable shadowing and potential naming conflicts.

Furthermore, the macro expands its `idx` and `phys` arguments twice.
This double evaluation is dangerous if arguments with side effects
(e.g., i++) are passed to the macro.

Convert the macro to a static inline function as suggested by David.
This improves type safety, avoids shadowing, and ensures each
argument is evaluated exactly once.

Signed-off-by: Min-Hsun Chang <chmh0624@gmail.com>
---
Changes in v2:
  - Convert the macro to a static inline function instead of just 
    renaming the local variable (suggested by David Laight).
  - This addresses both variable shadowing and double evaluation 
    issues pointed out during v1 review.
  - Link to v1: https://lore.kernel.org/all/20260307092119.20733-1-chmh0624@gmail.com/
---
 include/asm-generic/fixmap.h | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/include/asm-generic/fixmap.h b/include/asm-generic/fixmap.h
index 29cab7947980..541b8db35af2 100644
--- a/include/asm-generic/fixmap.h
+++ b/include/asm-generic/fixmap.h
@@ -71,13 +71,13 @@ 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;							\
-})
+static inline unsigned long __set_fixmap_offset(unsigned int 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)
-- 
2.43.0