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

[Tellakula Yeswanth Krishna]

Please add this tag


Tested-by: yeswanth <yeswanth@xxxxxxxxxxxxx>

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@xxxxxxxx>
> > Signed-off-by: Venkat Rao Bagalkote <venkat88@xxxxxxxxxxxxx>
> > ---
> 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@xxxxxxxxxxxxx>
>
>
> 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 */
> >       }