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

[David Laight]

On Sat, 25 Apr 2026 13:57:37 -0700
Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx> wrote:

> On Sun, 22 Mar 2026 14:40:32 +0000 David Laight <david.laight.linux@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxx>
> 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@xxxxxxxxx>
> Closes: https://lore.kernel.org/20260307092119.20733-1-chmh0624@xxxxxxxxx [1]
> Cc: Arnd Bergmann <arnd@xxxxxxxx>
> Cc: Mark Salter <msalter@xxxxxxxxxx>
> Cc: David Laight <david.laight.linux@xxxxxxxxx>
> Signed-off-by: Andrew Morton <akpm@xxxxxxxxxxxxxxxxxxxx>
> ---
> 
>  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
> _
>