[PATCH] ipmi: avoid atomic_inc in exit function

[PATCH] ipmi: avoid atomic_inc in exit function

[Arnd Bergmann]

This causes a link failure on ARM in certain configurations,
when we reference each atomic operation from .alt.smp.init in
order to patch out atomics on non-SMP systems:

`.exit.text' referenced in section `.alt.smp.init' of drivers/char/ipmi/ipmi_msghandler.o: defined in discarded section `.exit.text' of drivers/char/ipmi/ipmi_msghandler.o

In this case, we can trivially replace the atomic_inc() with
an atomic_set() that has the same effect and does not require
a fixup.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/char/ipmi/ipmi_msghandler.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/char/ipmi/ipmi_msghandler.c b/drivers/char/ipmi/ipmi_msghandler.c
index e8ba67834746..c48198eef510 100644
--- a/drivers/char/ipmi/ipmi_msghandler.c
+++ b/drivers/char/ipmi/ipmi_msghandler.c
@@ -5164,7 +5164,7 @@ static void __exit cleanup_ipmi(void)
 		 * avoids problems with race conditions removing the timer
 		 * here.
 		 */
-		atomic_inc(&stop_operation);
+		atomic_set(&stop_operation, 1);
 		del_timer_sync(&ipmi_timer);
 
 		initialized = false;
-- 
2.20.0

Re: [PATCH] ipmi: avoid atomic_inc in exit function

[Christoph Hellwig]

On Mon, Apr 15, 2019 at 05:55:00PM +0200, Arnd Bergmann wrote:
> This causes a link failure on ARM in certain configurations,
> when we reference each atomic operation from .alt.smp.init in
> order to patch out atomics on non-SMP systems:
> 
> `.exit.text' referenced in section `.alt.smp.init' of drivers/char/ipmi/ipmi_msghandler.o: defined in discarded section `.exit.text' of drivers/char/ipmi/ipmi_msghandler.o
> 
> In this case, we can trivially replace the atomic_inc() with
> an atomic_set() that has the same effect and does not require
> a fixup.

I'd rather fіx the arm section management.  Using atomic in exit
routines is perfectly valid, and it would seem odd to forbid it.

Re: [PATCH] ipmi: avoid atomic_inc in exit function

[Corey Minyard]

On Mon, Apr 15, 2019 at 09:40:22AM -0700, Christoph Hellwig wrote:
> On Mon, Apr 15, 2019 at 05:55:00PM +0200, Arnd Bergmann wrote:
> > This causes a link failure on ARM in certain configurations,
> > when we reference each atomic operation from .alt.smp.init in
> > order to patch out atomics on non-SMP systems:
> > 
> > `.exit.text' referenced in section `.alt.smp.init' of drivers/char/ipmi/ipmi_msghandler.o: defined in discarded section `.exit.text' of drivers/char/ipmi/ipmi_msghandler.o
> > 
> > In this case, we can trivially replace the atomic_inc() with
> > an atomic_set() that has the same effect and does not require
> > a fixup.
> 
> I'd rather fіx the arm section management.  Using atomic in exit
> routines is perfectly valid, and it would seem odd to forbid it.

That was my first thought, too.  It's kind of hard to believe that
the IPMI driver is the only thing that does an atomic_inc() in the
exit code.

-corey

Re: [PATCH] ipmi: avoid atomic_inc in exit function

[Arnd Bergmann]

On Mon, Apr 15, 2019 at 7:39 PM Corey Minyard <minyard@acm.org> wrote:
>
> On Mon, Apr 15, 2019 at 09:40:22AM -0700, Christoph Hellwig wrote:
> > On Mon, Apr 15, 2019 at 05:55:00PM +0200, Arnd Bergmann wrote:
> > > This causes a link failure on ARM in certain configurations,
> > > when we reference each atomic operation from .alt.smp.init in
> > > order to patch out atomics on non-SMP systems:
> > >
> > > `.exit.text' referenced in section `.alt.smp.init' of drivers/char/ipmi/ipmi_msghandler.o: defined in discarded section `.exit.text' of drivers/char/ipmi/ipmi_msghandler.o
> > >
> > > In this case, we can trivially replace the atomic_inc() with
> > > an atomic_set() that has the same effect and does not require
> > > a fixup.
> >
> > I'd rather fіx the arm section management.  Using atomic in exit
> > routines is perfectly valid, and it would seem odd to forbid it.
>
> That was my first thought, too.  It's kind of hard to believe that
> the IPMI driver is the only thing that does an atomic_inc() in the
> exit code.

That's what I had thought as well at first, and I carried a patch
to work around this by not dropping the .text.exit section on ARM
when SMP patching is enabled for a few years. I never sent this
because that can waste a significant amount of kernel memory,
and I knew the warning is harmless.

When revisiting it now, I found that this one was the only instance
I ever hit. It seems to be that using atomics in module_exit() is
indeed odd, because the function is rarely concurrent with anything
else.

        Arnd

Re: [PATCH] ipmi: avoid atomic_inc in exit function

[Corey Minyard]

On Mon, Apr 15, 2019 at 09:00:46PM +0200, Arnd Bergmann wrote:
> On Mon, Apr 15, 2019 at 7:39 PM Corey Minyard <minyard@acm.org> wrote:
> >
> > On Mon, Apr 15, 2019 at 09:40:22AM -0700, Christoph Hellwig wrote:
> > > On Mon, Apr 15, 2019 at 05:55:00PM +0200, Arnd Bergmann wrote:
> > > > This causes a link failure on ARM in certain configurations,
> > > > when we reference each atomic operation from .alt.smp.init in
> > > > order to patch out atomics on non-SMP systems:
> > > >
> > > > `.exit.text' referenced in section `.alt.smp.init' of drivers/char/ipmi/ipmi_msghandler.o: defined in discarded section `.exit.text' of drivers/char/ipmi/ipmi_msghandler.o
> > > >
> > > > In this case, we can trivially replace the atomic_inc() with
> > > > an atomic_set() that has the same effect and does not require
> > > > a fixup.
> > >
> > > I'd rather fіx the arm section management.  Using atomic in exit
> > > routines is perfectly valid, and it would seem odd to forbid it.
> >
> > That was my first thought, too.  It's kind of hard to believe that
> > the IPMI driver is the only thing that does an atomic_inc() in the
> > exit code.
> 
> That's what I had thought as well at first, and I carried a patch
> to work around this by not dropping the .text.exit section on ARM
> when SMP patching is enabled for a few years. I never sent this
> because that can waste a significant amount of kernel memory,
> and I knew the warning is harmless.
> 
> When revisiting it now, I found that this one was the only instance
> I ever hit. It seems to be that using atomics in module_exit() is
> indeed odd, because the function is rarely concurrent with anything
> else.

I've added the change to my tree; it actually makes a little more
sense, so I'm ok with it.

I guess it's up to you to deal with any new ones that happen in
the future ;-).

-corey