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

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

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