Re: [PATCH v3] locking/local_lock: s/l/__l/ and s/tl/__tl/ to reduce risk of shadowing

[Waiman Long <llong@redhat.com>]

On 10/9/25 6:39 AM, Sebastian Andrzej Siewior wrote:
> From: Vincent Mailhol <mailhol@kernel.org>
>
> The Linux kernel coding style advises to avoid common variable
> names in function-like macros to reduce the risk of collisions.
>
> Throughout local_lock_internal.h, several macros use the rather common
> variable names 'l' and 'tl'. This already resulted in an actual
> collision: the __local_lock_acquire() function like macro is currently
> shadowing the parameter 'l' of the:
>
>    class_##_name##_t class_##_name##_constructor(_type *l)
>
> function factory from linux/cleanup.h.
>
> Rename the variable 'l' to '__l' and the variable 'tl' to '__tl'
> throughout the file to fix the current name collision and to prevent
> future ones.
>
> [ bigeasy: Rebase, update all l and tl instances in macros ]
>
> Signed-off-by: Vincent Mailhol <mailhol@kernel.org>
> Signed-off-by: Sebastian Andrzej Siewior <bigeasy@linutronix.de>
> ---
>
> Looks harmless enough to make everyone happy.
>
> v2…v3:
>    - Rebase
>
> v1…v2: https://lore.kernel.org/all/20250925-local_lock_internal_fix_shadow-v2-1-d3b85ee775a4@kernel.org/
>
>    - __lock conflicted with an existing definition in lockdep.c. Use
>      instead __l (and also, to keep things consistent, use __tl instead
>      of tl for the trylock).
>
>    - Apply the renaming to the entire file and not just to
>      __local_lock_acquire().
>
>    - Rewrite the patch description accordingly.
>
> v1: https://lore.kernel.org/r/20250923-local_lock_internal_fix_shadow-v1-1-14e313c88a46@kernel.org
>
>   include/linux/local_lock_internal.h | 62 ++++++++++++++---------------
>   1 file changed, 31 insertions(+), 31 deletions(-)
>
> diff --git a/include/linux/local_lock_internal.h b/include/linux/local_lock_internal.h
> index a4dc479157b5c..8f82b4eb542f2 100644
> --- a/include/linux/local_lock_internal.h
> +++ b/include/linux/local_lock_internal.h
> @@ -99,18 +99,18 @@ do {								\
>   
>   #define __local_lock_acquire(lock)					\
>   	do {								\
> -		local_trylock_t *tl;					\
> -		local_lock_t *l;					\
> +		local_trylock_t *__tl;					\
> +		local_lock_t *__l;					\
>   									\
> -		l = (local_lock_t *)(lock);				\
> -		tl = (local_trylock_t *)l;				\
> +		__l = (local_lock_t *)(lock);				\
> +		__tl = (local_trylock_t *)__l;				\
>   		_Generic((lock),					\
>   			local_trylock_t *: ({				\
> -				lockdep_assert(tl->acquired == 0);	\
> -				WRITE_ONCE(tl->acquired, 1);		\
> +				lockdep_assert(__tl->acquired == 0);	\
> +				WRITE_ONCE(__tl->acquired, 1);		\
>   			}),						\
>   			local_lock_t *: (void)0);			\
> -		local_lock_acquire(l);					\
> +		local_lock_acquire(__l);				\
>   	} while (0)
>   
>   #define __local_lock(lock)					\
> @@ -133,36 +133,36 @@ do {								\
>   
>   #define __local_trylock(lock)					\
>   	({							\
> -		local_trylock_t *tl;				\
> +		local_trylock_t *__tl;				\
>   								\
>   		preempt_disable();				\
> -		tl = (lock);					\
> -		if (READ_ONCE(tl->acquired)) {			\
> +		__tl = (lock);					\
> +		if (READ_ONCE(__tl->acquired)) {		\
>   			preempt_enable();			\
> -			tl = NULL;				\
> +			__tl = NULL;				\
>   		} else {					\
> -			WRITE_ONCE(tl->acquired, 1);		\
> +			WRITE_ONCE(__tl->acquired, 1);		\
>   			local_trylock_acquire(			\
> -				(local_lock_t *)tl);		\
> +				(local_lock_t *)__tl);		\
>   		}						\
> -		!!tl;						\
> +		!!__tl;						\
>   	})
>   
>   #define __local_trylock_irqsave(lock, flags)			\
>   	({							\
> -		local_trylock_t *tl;				\
> +		local_trylock_t *__tl;				\
>   								\
>   		local_irq_save(flags);				\
> -		tl = (lock);					\
> -		if (READ_ONCE(tl->acquired)) {			\
> +		__tl = (lock);					\
> +		if (READ_ONCE(__tl->acquired)) {		\
>   			local_irq_restore(flags);		\
> -			tl = NULL;				\
> +			__tl = NULL;				\
>   		} else {					\
> -			WRITE_ONCE(tl->acquired, 1);		\
> +			WRITE_ONCE(__tl->acquired, 1);		\
>   			local_trylock_acquire(			\
> -				(local_lock_t *)tl);		\
> +				(local_lock_t *)__tl);		\
>   		}						\
> -		!!tl;						\
> +		!!__tl;						\
>   	})
>   
>   /* preemption or migration must be disabled before calling __local_lock_is_locked */
> @@ -170,16 +170,16 @@ do {								\
>   
>   #define __local_lock_release(lock)					\
>   	do {								\
> -		local_trylock_t *tl;					\
> -		local_lock_t *l;					\
> +		local_trylock_t *__tl;					\
> +		local_lock_t *__l;					\
>   									\
> -		l = (local_lock_t *)(lock);				\
> -		tl = (local_trylock_t *)l;				\
> -		local_lock_release(l);					\
> +		__l = (local_lock_t *)(lock);				\
> +		__tl = (local_trylock_t *)__l;				\
> +		local_lock_release(__l);				\
>   		_Generic((lock),					\
>   			local_trylock_t *: ({				\
> -				lockdep_assert(tl->acquired == 1);	\
> -				WRITE_ONCE(tl->acquired, 0);		\
> +				lockdep_assert(__tl->acquired == 1);	\
> +				WRITE_ONCE(__tl->acquired, 0);		\
>   			}),						\
>   			local_lock_t *: (void)0);			\
>   	} while (0)
> @@ -223,12 +223,12 @@ typedef spinlock_t local_trylock_t;
>   #define INIT_LOCAL_LOCK(lockname) __LOCAL_SPIN_LOCK_UNLOCKED((lockname))
>   #define INIT_LOCAL_TRYLOCK(lockname) __LOCAL_SPIN_LOCK_UNLOCKED((lockname))
>   
> -#define __local_lock_init(l)					\
> +#define __local_lock_init(__l)					\
>   	do {							\
> -		local_spin_lock_init((l));			\
> +		local_spin_lock_init((__l));			\
>   	} while (0)
>   
> -#define __local_trylock_init(l)			__local_lock_init(l)
> +#define __local_trylock_init(__l)			__local_lock_init(__l)
>   
>   #define __local_lock(__lock)					\
>   	do {							\

Make sense.

Acked-by: Waiman Long <longman@redhat.com>