| Message ID | 20171106133450.482858-1-arnd@arndb.de |
|---|---|
| State | New |
| Headers | show |
| Series | clocksource/drivers/timer-of: mark timer_of_exit as __init | expand |
On Mon, 6 Nov 2017, Arnd Bergmann wrote: > The newly added function triggers a harmless Kbuild warning because > of a missing annotation: > > WARNING: vmlinux.o(.text+0x448098): Section mismatch in reference from the function timer_of_exit() to the function .init.text:timer_clk_exit() > The function timer_of_exit() references > the function __init timer_clk_exit(). > This is often because timer_of_exit lacks a __init > annotation or the annotation of timer_clk_exit is wrong. > > The function is only called from other __init functions, so it > can safely be marked as __init as well. Hmm. I don't see any caller at all. From the intention of the patch I assume this isn't designed for using from init functions, so we rather have to remove the __init annotations from the called functions. Sudeep posted a patch which does that: https://lkml.kernel.org/r/1509979716-10646-1-git-send-email-sudeep.holla@arm.com Though I rather would know whether this function is going to be used at all and what the intention of this patch was. Benjamin???? Thanks, tglx
On Sun, Nov 12, 2017 at 10:16 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > On Mon, 6 Nov 2017, Arnd Bergmann wrote: >> The newly added function triggers a harmless Kbuild warning because >> of a missing annotation: >> >> WARNING: vmlinux.o(.text+0x448098): Section mismatch in reference from the function timer_of_exit() to the function .init.text:timer_clk_exit() >> The function timer_of_exit() references >> the function __init timer_clk_exit(). >> This is often because timer_of_exit lacks a __init >> annotation or the annotation of timer_clk_exit is wrong. >> >> The function is only called from other __init functions, so it >> can safely be marked as __init as well. > > Hmm. I don't see any caller at all. From the intention of the patch I > assume this isn't designed for using from init functions, so we rather have > to remove the __init annotations from the called functions. > > Sudeep posted a patch which does that: > > https://lkml.kernel.org/r/1509979716-10646-1-git-send-email-sudeep.holla@arm.com > > Though I rather would know whether this function is going to be used at > all and what the intention of this patch was. > > Benjamin???? My interpretation was that timer drivers are still supposed to be unregistered at module unload time, but that you might use the new timer_of_exit() in the failure path of whatever function calls timer_of_init() successfully when something fails in the next step. Sudeep's interpretation also makes sense, I had not thought of that, but I now found the patch that adds a user in an init function: https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1519644.html It seems I guessed right and Sudeep guessed wrong (both by pure chance I admit). Both patches solve the problem, Sudeep's version is a little more robust in case we ever add a caller in an __exit function (which I think is currently not allowed), while mine saves a little bit of memory and matches the current usage better. Arnd
On Sun, 12 Nov 2017, Arnd Bergmann wrote: > On Sun, Nov 12, 2017 at 10:16 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > > On Mon, 6 Nov 2017, Arnd Bergmann wrote: > >> The newly added function triggers a harmless Kbuild warning because > >> of a missing annotation: > >> > >> WARNING: vmlinux.o(.text+0x448098): Section mismatch in reference from the function timer_of_exit() to the function .init.text:timer_clk_exit() > >> The function timer_of_exit() references > >> the function __init timer_clk_exit(). > >> This is often because timer_of_exit lacks a __init > >> annotation or the annotation of timer_clk_exit is wrong. > >> > >> The function is only called from other __init functions, so it > >> can safely be marked as __init as well. > > > > Hmm. I don't see any caller at all. From the intention of the patch I > > assume this isn't designed for using from init functions, so we rather have > > to remove the __init annotations from the called functions. > > > > Sudeep posted a patch which does that: > > > > https://lkml.kernel.org/r/1509979716-10646-1-git-send-email-sudeep.holla@arm.com > > > > Though I rather would know whether this function is going to be used at > > all and what the intention of this patch was. > > > > Benjamin???? > > My interpretation was that timer drivers are still supposed to be unregistered > at module unload time, but that you might use the new timer_of_exit() > in the failure path of whatever function calls timer_of_init() successfully > when something fails in the next step. > > Sudeep's interpretation also makes sense, I had not thought of that, but > I now found the patch that adds a user in an init function: > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1519644.html > > It seems I guessed right and Sudeep guessed wrong (both by pure chance > I admit). Both patches solve the problem, Sudeep's version is a little > more robust in case we ever add a caller in an __exit function (which I > think is currently not allowed), while mine saves a little bit of memory > and matches the current usage better. Right, but if the only use case is the cleanup in an error path, then the function name is a misnomer. Thanks, tglx
2017-11-12 23:27 GMT+01:00 Thomas Gleixner <tglx@linutronix.de>: > On Sun, 12 Nov 2017, Arnd Bergmann wrote: >> On Sun, Nov 12, 2017 at 10:16 PM, Thomas Gleixner <tglx@linutronix.de> wrote: >> > On Mon, 6 Nov 2017, Arnd Bergmann wrote: >> >> The newly added function triggers a harmless Kbuild warning because >> >> of a missing annotation: >> >> >> >> WARNING: vmlinux.o(.text+0x448098): Section mismatch in reference from the function timer_of_exit() to the function .init.text:timer_clk_exit() >> >> The function timer_of_exit() references >> >> the function __init timer_clk_exit(). >> >> This is often because timer_of_exit lacks a __init >> >> annotation or the annotation of timer_clk_exit is wrong. >> >> >> >> The function is only called from other __init functions, so it >> >> can safely be marked as __init as well. >> > >> > Hmm. I don't see any caller at all. From the intention of the patch I >> > assume this isn't designed for using from init functions, so we rather have >> > to remove the __init annotations from the called functions. >> > >> > Sudeep posted a patch which does that: >> > >> > https://lkml.kernel.org/r/1509979716-10646-1-git-send-email-sudeep.holla@arm.com >> > >> > Though I rather would know whether this function is going to be used at >> > all and what the intention of this patch was. >> > >> > Benjamin???? >> >> My interpretation was that timer drivers are still supposed to be unregistered >> at module unload time, but that you might use the new timer_of_exit() >> in the failure path of whatever function calls timer_of_init() successfully >> when something fails in the next step. >> >> Sudeep's interpretation also makes sense, I had not thought of that, but >> I now found the patch that adds a user in an init function: >> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1519644.html >> >> It seems I guessed right and Sudeep guessed wrong (both by pure chance >> I admit). Both patches solve the problem, Sudeep's version is a little >> more robust in case we ever add a caller in an __exit function (which I >> think is currently not allowed), while mine saves a little bit of memory >> and matches the current usage better. > > Right, but if the only use case is the cleanup in an error path, then the > function name is a misnomer. > > Thanks, > > tglx The function is suppose to be called when you need to undo what have been done in timer_of_init(). That could happen in error case or when removing the module. Until now this function isn't called yet because it was part on my series to update stm32 timer and onky this patch has been merged. Benjamin
On Sun, Nov 12, 2017 at 11:24:56PM +0100, Arnd Bergmann wrote: > On Sun, Nov 12, 2017 at 10:16 PM, Thomas Gleixner <tglx@linutronix.de> wrote: > > On Mon, 6 Nov 2017, Arnd Bergmann wrote: > >> The newly added function triggers a harmless Kbuild warning because > >> of a missing annotation: > >> > >> WARNING: vmlinux.o(.text+0x448098): Section mismatch in reference from the function timer_of_exit() to the function .init.text:timer_clk_exit() > >> The function timer_of_exit() references > >> the function __init timer_clk_exit(). > >> This is often because timer_of_exit lacks a __init > >> annotation or the annotation of timer_clk_exit is wrong. > >> > >> The function is only called from other __init functions, so it > >> can safely be marked as __init as well. > > > > Hmm. I don't see any caller at all. From the intention of the patch I > > assume this isn't designed for using from init functions, so we rather have > > to remove the __init annotations from the called functions. > > > > Sudeep posted a patch which does that: > > > > https://lkml.kernel.org/r/1509979716-10646-1-git-send-email-sudeep.holla@arm.com > > > > Though I rather would know whether this function is going to be used at > > all and what the intention of this patch was. > > > > Benjamin???? > > My interpretation was that timer drivers are still supposed to be unregistered > at module unload time, but that you might use the new timer_of_exit() > in the failure path of whatever function calls timer_of_init() successfully > when something fails in the next step. > > Sudeep's interpretation also makes sense, I had not thought of that, but > I now found the patch that adds a user in an init function: > https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1519644.html > > It seems I guessed right and Sudeep guessed wrong (both by pure chance > I admit). Ah OK, I just went by name that it will be called by some exit/remove function. > Both patches solve the problem, Sudeep's version is a little > more robust in case we ever add a caller in an __exit function (which I > think is currently not allowed), while mine saves a little bit of memory > and matches the current usage better. > Agreed, may be if we add users which is called from init functions, the warning should disappear. Also as tglx suggested, we could rename if it's just used from init function error/exit paths. -- Regards, Sudeep
On 13/11/2017 11:11, Sudeep Holla wrote: > On Sun, Nov 12, 2017 at 11:24:56PM +0100, Arnd Bergmann wrote: >> On Sun, Nov 12, 2017 at 10:16 PM, Thomas Gleixner <tglx@linutronix.de> wrote: >>> On Mon, 6 Nov 2017, Arnd Bergmann wrote: >>>> The newly added function triggers a harmless Kbuild warning because >>>> of a missing annotation: >>>> >>>> WARNING: vmlinux.o(.text+0x448098): Section mismatch in reference from the function timer_of_exit() to the function .init.text:timer_clk_exit() >>>> The function timer_of_exit() references >>>> the function __init timer_clk_exit(). >>>> This is often because timer_of_exit lacks a __init >>>> annotation or the annotation of timer_clk_exit is wrong. >>>> >>>> The function is only called from other __init functions, so it >>>> can safely be marked as __init as well. >>> >>> Hmm. I don't see any caller at all. From the intention of the patch I >>> assume this isn't designed for using from init functions, so we rather have >>> to remove the __init annotations from the called functions. >>> >>> Sudeep posted a patch which does that: >>> >>> https://lkml.kernel.org/r/1509979716-10646-1-git-send-email-sudeep.holla@arm.com >>> >>> Though I rather would know whether this function is going to be used at >>> all and what the intention of this patch was. >>> >>> Benjamin???? >> >> My interpretation was that timer drivers are still supposed to be unregistered >> at module unload time, but that you might use the new timer_of_exit() >> in the failure path of whatever function calls timer_of_init() successfully >> when something fails in the next step. >> >> Sudeep's interpretation also makes sense, I had not thought of that, but >> I now found the patch that adds a user in an init function: >> https://www.mail-archive.com/linux-kernel@vger.kernel.org/msg1519644.html >> >> It seems I guessed right and Sudeep guessed wrong (both by pure chance >> I admit). > > Ah OK, I just went by name that it will be called by some exit/remove > function. > >> Both patches solve the problem, Sudeep's version is a little >> more robust in case we ever add a caller in an __exit function (which I >> think is currently not allowed), while mine saves a little bit of memory >> and matches the current usage better. >> > > Agreed, may be if we add users which is called from init functions, the > warning should disappear. Also as tglx suggested, we could rename if it's > just used from init function error/exit paths. The drivers are not compiled as module AFAICT, the function will be called in the init error path. -- <http://www.linaro.org/> Linaro.org │ Open source software for ARM SoCs Follow Linaro: <http://www.facebook.com/pages/Linaro> Facebook | <http://twitter.com/#!/linaroorg> Twitter | <http://www.linaro.org/linaro-blog/> Blog
On Mon, 13 Nov 2017, Daniel Lezcano wrote: > On 13/11/2017 11:11, Sudeep Holla wrote: > > Agreed, may be if we add users which is called from init functions, the > > warning should disappear. Also as tglx suggested, we could rename if it's > > just used from init function error/exit paths. > > The drivers are not compiled as module AFAICT, the function will be > called in the init error path. Ok, if the consensus is that this is used for common cleanup in __init error handling, then please can someone submit a new version of this which renames the function to something like timer_of_cleanup() and add the __init annotation to it? And while at it please add a kernel doc comment to that effect. Thanks, tglx
On 13/11/17 14:58, Daniel Lezcano wrote: > On 13/11/2017 11:11, Sudeep Holla wrote: [..] >> >> Agreed, may be if we add users which is called from init functions, the >> warning should disappear. Also as tglx suggested, we could rename if it's >> just used from init function error/exit paths. > > The drivers are not compiled as module AFAICT, the function will be > called in the init error path. > Understood, IMO better to rename the functions as cleanup or something similar. exit made it sound differently and since there were no users, I assumed it to be used in remove/exit functions. Sorry for the noise. -- Regards, Sudeep
diff --git a/drivers/clocksource/timer-of.c b/drivers/clocksource/timer-of.c index 7c64a5c1bfc1..e301fdb1286e 100644 --- a/drivers/clocksource/timer-of.c +++ b/drivers/clocksource/timer-of.c @@ -177,7 +177,7 @@ int __init timer_of_init(struct device_node *np, struct timer_of *to) return ret; } -void timer_of_exit(struct timer_of *to) +void __init timer_of_exit(struct timer_of *to) { if (to->flags & TIMER_OF_IRQ) timer_irq_exit(&to->of_irq);
The newly added function triggers a harmless Kbuild warning because of a missing annotation: WARNING: vmlinux.o(.text+0x448098): Section mismatch in reference from the function timer_of_exit() to the function .init.text:timer_clk_exit() The function timer_of_exit() references the function __init timer_clk_exit(). This is often because timer_of_exit lacks a __init annotation or the annotation of timer_clk_exit is wrong. The function is only called from other __init functions, so it can safely be marked as __init as well. Fixes: f48729a999ee ("clocksource/drivers/timer-of: Add timer_of_exit function") Signed-off-by: Arnd Bergmann <arnd@arndb.de> --- drivers/clocksource/timer-of.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) -- 2.9.0