Re: [PATCH v4] char/nvram: Remove redundant nvram_mutex

[Tellakula Yeswanth Krishna <yeswanth@linux.ibm.com>]

On 30/03/26 4:05 pm, Venkat Rao Bagalkote wrote:
> The global nvram_mutex in drivers/char/nvram.c is redundant and unused,
> and this triggers compiler warnings on some configurations.
>
> All platform-specific nvram operations already provide their own internal
> synchronization, meaning the wrapper-level mutex does not provide any
> additional safety.
>
> Remove the nvram_mutex definition along with all remaining lock/unlock
> users across PPC32, x86, and m68k code paths, and rely entirely on the
> per-architecture nvram implementations for locking.
>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Venkat Rao Bagalkote <venkat88@linux.ibm.com>
> ---
Without Fix
===============
make -j 33 -s && make modules_install && make install
In file included from ./include/linux/seqlock.h:20,
                  from ./include/linux/mmzone.h:17,
                  from ./include/linux/gfp.h:7,
                  from ./include/linux/umh.h:4,
                  from ./include/linux/kmod.h:9,
                  from ./include/linux/module.h:18,
                  from drivers/char/nvram.c:34:
drivers/char/nvram.c:56:21: warning: 'nvram_mutex' defined but not used 
[-Wunused-variable]
    56 | static DEFINE_MUTEX(nvram_mutex);
       |                     ^~~~~~~~~~~
./include/linux/mutex.h:87:22: note: in definition of macro 'DEFINE_MUTEX'
    87 |         struct mutex mutexname = __MUTEX_INITIALIZER(mutexname)
       |                      ^~~~~~~~~




With this patch issue is fixed


Please add below tag

yeswanth <yeswanth@linux.ibm.com>


Thanks,

Yeswanth Krishna

> v4:
>    - Remove all remaining nvram_mutex call sites, completing the mutex removal
>
> v3:
>    - Removed global nvram_mutex definition
>
>   drivers/char/nvram.c | 16 +++-------------
>   1 file changed, 3 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/char/nvram.c b/drivers/char/nvram.c
> index 9eff426a9286..e89cc1f1c89e 100644
> --- a/drivers/char/nvram.c
> +++ b/drivers/char/nvram.c
> @@ -53,7 +53,6 @@
>   #include <asm/nvram.h>
>   #endif
>   
> -static DEFINE_MUTEX(nvram_mutex);
>   static DEFINE_SPINLOCK(nvram_state_lock);
>   static int nvram_open_cnt;	/* #times opened */
>   static int nvram_open_mode;	/* special open modes */
> @@ -310,11 +309,8 @@ static long nvram_misc_ioctl(struct file *file, unsigned int cmd,
>   		break;
>   #ifdef CONFIG_PPC32
>   	case IOC_NVRAM_SYNC:
> -		if (ppc_md.nvram_sync != NULL) {
> -			mutex_lock(&nvram_mutex);
> +		if (ppc_md.nvram_sync)
>   			ppc_md.nvram_sync();
> -			mutex_unlock(&nvram_mutex);
> -		}
>   		ret = 0;
>   		break;
>   #endif
> @@ -324,11 +320,8 @@ static long nvram_misc_ioctl(struct file *file, unsigned int cmd,
>   		if (!capable(CAP_SYS_ADMIN))
>   			return -EACCES;
>   
> -		if (arch_nvram_ops.initialize != NULL) {
> -			mutex_lock(&nvram_mutex);
> +		if (arch_nvram_ops.initialize)
>   			ret = arch_nvram_ops.initialize();
> -			mutex_unlock(&nvram_mutex);
> -		}
>   		break;
>   	case NVRAM_SETCKS:
>   		/* just set checksum, contents unchanged (maybe useful after
> @@ -336,11 +329,8 @@ static long nvram_misc_ioctl(struct file *file, unsigned int cmd,
>   		if (!capable(CAP_SYS_ADMIN))
>   			return -EACCES;
>   
> -		if (arch_nvram_ops.set_checksum != NULL) {
> -			mutex_lock(&nvram_mutex);
> +		if (arch_nvram_ops.set_checksum)
>   			ret = arch_nvram_ops.set_checksum();
> -			mutex_unlock(&nvram_mutex);
> -		}
>   		break;
>   #endif /* CONFIG_X86 || CONFIG_M68K */
>   	}