[PATCH v4] char/nvram: Remove redundant nvram_mutex

[PATCH v4] 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.

Suggested-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Venkat Rao Bagalkote <venkat88@linux.ibm.com>
---
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 */
 	}
-- 
2.45.2

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

[Ritesh Harjani]

Venkat Rao Bagalkote <venkat88@linux.ibm.com> writes:

> 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>
> ---
> v4:
>   - Remove all remaining nvram_mutex call sites, completing the mutex removal
>

Let me cut paste the review from Sashiko here..

    Does this removal expose the underlying raw spinlock to concurrent userspace
    contention?
    Looking at ppc_md.nvram_sync() implementations like core99_nvram_sync() on
    PowerMac, the code acquires a raw spinlock (nv_lock) and performs hardware
    flash memory operations with polling loops and udelay() calls that can take
    hundreds of milliseconds to complete.
    Because IOC_NVRAM_SYNC does not require CAP_SYS_ADMIN, any user with access
    to the device can call this ioctl.
    Previously, nvram_mutex provided a sleepable barrier for concurrent
    IOC_NVRAM_SYNC callers. Without it, won't secondary callers spin on the raw
    spinlock with interrupts disabled for the entire duration of the first
    caller's slow flash I/O?
    Could this prolonged spinning with IRQs disabled completely freeze the
    waiting CPUs and trigger NMI watchdog timeouts or system lockups?

First of all the above problem is only being talked about PowerMAC and
not for x86 / m68k. In there I think, we just read/write few bytes under
the spinlock.

On PowerMac too, I don't think the above problem gives a reason, to keep
a redundant locking at a generic wrapper layer which can affects other
platforms/archs. And the comment in PowerMac code above nv_lock says:
    // XXX Turn that into a sem
    static DEFINE_RAW_SPINLOCK(nv_lock);

So, it looks like Sashiko review comment can be ignored, and the patch
looks right to me, which kills the redundant mutex lock from here. So as
for this patch please feel free to add...

Reviewed-by: Ritesh Harjani (IBM) <ritesh.list@gmail.com>

... But I would also let Chrisptophe comment from ppc32 / PowerMac perspective.

-ritesh

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

[Tellakula Yeswanth Krishna]

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

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

[Tellakula Yeswanth Krishna]

Please add this tag


Tested-by: yeswanth <yeswanth@linux.ibm.com>

On 02/04/26 9:33 pm, Tellakula Yeswanth Krishna wrote:
>
> 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 */
>>       }

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

[Venkat]

> On 7 Apr 2026, at 12:11 PM, Tellakula Yeswanth Krishna <yeswanth@linux.ibm.com> wrote:
> 
> Please add this tag
> 
> 
> Tested-by: yeswanth <yeswanth@linux.ibm.com>
> 
> On 02/04/26 9:33 pm, Tellakula Yeswanth Krishna wrote:
>> 
>> 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>
>>> ---

Hi Arnd,

Thanks for the earlier review and suggestion on this change.

I’m not entirely sure which tree would be the best home for this patch, 
given that the change touches common NVRAM code used across multiple architectures (PPC32, x86, and m68k). 

If there are no further comments or objections from others, would you be able to pick this up through your tree?

Please let me know if this should instead go via some other tree or maintainer.

Regards,
Venkat.
>> 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 */
>>>      }