| Message ID | 20190415155509.3565087-1-arnd@arndb.de |
|---|---|
| State | Accepted |
| Commit | 9a75bd18a85bec5d6d0006a3dba6ff78f65d8fe3 |
| Headers | show |
| Series | ipmi: avoid atomic_inc in exit function | expand |
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.
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
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
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
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;
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(-) -- 2.20.0