[PATCH] clocksource/drivers/timer-of: mark timer_of_exit as __init

[PATCH] clocksource/drivers/timer-of: mark timer_of_exit as __init

[Arnd Bergmann]

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(-)

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);
-- 
2.9.0

Re: [PATCH] clocksource/drivers/timer-of: mark timer_of_exit as __init

[Thomas Gleixner]

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

Re: [PATCH] clocksource/drivers/timer-of: mark timer_of_exit as __init

[Arnd Bergmann]

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

Re: [PATCH] clocksource/drivers/timer-of: mark timer_of_exit as __init

[Thomas Gleixner]

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

Re: [PATCH] clocksource/drivers/timer-of: mark timer_of_exit as __init

[Benjamin Gaignard]

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

Re: [PATCH] clocksource/drivers/timer-of: mark timer_of_exit as __init

[Sudeep Holla]

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

Re: [PATCH] clocksource/drivers/timer-of: mark timer_of_exit as __init

[Daniel Lezcano]

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

Re: [PATCH] clocksource/drivers/timer-of: mark timer_of_exit as __init

[Thomas Gleixner]

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

Re: [PATCH] clocksource/drivers/timer-of: mark timer_of_exit as __init

[Sudeep Holla]

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