[PATCH v5] char/nvram: Remove redundant nvram_mutex

[PATCH v5] char/nvram: Remove redundant nvram_mutex

[Venkat Rao Bagalkote]

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.

Reviewed-by: Arnd Bergmann <arnd@arndb.de>
Suggested-by: Arnd Bergmann <arnd@arndb.de>
Tested-by: Tellakula Yeswanth Krishna <yeswanth@linux.ibm.com>
Signed-off-by: Venkat Rao Bagalkote <venkat88@linux.ibm.com>
---
Changes since v4:
- No code changes
- Resent after v7.1-rc1 as suggested by Arnd Bergmann

 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 */
 	}
-- 
2.45.2

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

[Venkat]

Hi,

Gentle ping on this patch.

This removes the unused global nvram_mutex and relies on the
existing per-architecture synchronization, as suggested earlier.

I’ve re-tested the change, and everything continues to work as expected.
No issues observed in validation.

Please let me know if any further changes are needed.

Thanks,
Venkat


> On 28 Apr 2026, at 11:45 AM, Venkat Rao Bagalkote <venkat88@linux.ibm.com> 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.
> 
> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
> Suggested-by: Arnd Bergmann <arnd@arndb.de>
> Tested-by: Tellakula Yeswanth Krishna <yeswanth@linux.ibm.com>
> Signed-off-by: Venkat Rao Bagalkote <venkat88@linux.ibm.com>
> ---
> Changes since v4:
> - No code changes
> - Resent after v7.1-rc1 as suggested by Arnd Bergmann
> 
> 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 */
> }
> -- 
> 2.45.2
> 

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

[Christophe Leroy (CS GROUP)]

Le 14/05/2026 à 05:57, Venkat a écrit :
> 
> Hi,
> 
> Gentle ping on this patch.
> 
> This removes the unused global nvram_mutex and relies on the
> existing per-architecture synchronization, as suggested earlier.
> 
> I’ve re-tested the change, and everything continues to work as expected.
> No issues observed in validation.
> 
> Please let me know if any further changes are needed.
> 
> Thanks,
> Venkat
> 
> 
>> On 28 Apr 2026, at 11:45 AM, Venkat Rao Bagalkote <venkat88@linux.ibm.com> wrote:
>>
>> The global nvram_mutex in drivers/char/nvram.c is redundant and unused,

Redundant with what ?

It is _used_, at least in nvram_misc_ioctl()


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

Indeed, this is what it is redundant with, I would say that first thing 
in the message.

I think it would also be worth providing the history from Arnd from 
here: 
https://patchwork.ozlabs.org/project/linuxppc-dev/patch/20260323072422.25730-1-venkat88@linux.ibm.com/#3667538


>>
>> 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.
>>
>> Reviewed-by: Arnd Bergmann <arnd@arndb.de>
>> Suggested-by: Arnd Bergmann <arnd@arndb.de>
>> Tested-by: Tellakula Yeswanth Krishna <yeswanth@linux.ibm.com>
>> Signed-off-by: Venkat Rao Bagalkote <venkat88@linux.ibm.com>

With the above changes, Reviewed-by: Christophe Leroy (CS GROUP) 
<chleroy@kernel.org>

>> ---
>> Changes since v4:
>> - No code changes
>> - Resent after v7.1-rc1 as suggested by Arnd Bergmann
>>
>> 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 */
>> }
>> -- 
>> 2.45.2
>>
>