Message-ID: <20140120032757.GA761@windriver.com>
Date:	Sun, 19 Jan 2014 22:27:57 -0500
From:	Paul Gortmaker <paul.gortmaker@...driver.com>
To:	Geert Uytterhoeven <geert@...ux-m68k.org>
CC:	"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
	Linux/m68k <linux-m68k@...r.kernel.org>
Subject: Re: don't use module_init in non-modular ... (was: Re: [PATCH] m68k:
 don't use module_init in non-modular mvme16x/rtc.c code)

[Re: don't use module_init in non-modular ... (was: Re: [PATCH] m68k: don't use module_init in non-modular mvme16x/rtc.c code)] On 19/01/2014 (Sun 10:40) Geert Uytterhoeven wrote:

> Hi Paul,
> 
> On Thu, Jan 16, 2014 at 11:15 PM, Paul Gortmaker
> <paul.gortmaker@...driver.com> wrote:
> > The rtc.o is built for obj-y, i.e. always built in.  It will
> > never be modular, so using module_init as an alias for __initcall
> > can be somewhat misleading.
> >
> > Fix this up now, so that we can relocate module_init from
> > init.h into module.h in the future.  If we don't do this, we'd
> > have to add module.h to obviously non-modular code, and that
> > would be a worse thing.
> 
> The word "module" has different meanings: it can be a "loadable kernel module",
> or just a "code module". include/linux/init.h seems to agree with this:

I think for most people, "module" means an actual "foo.ko" that can be
fed to insmod.  And it is generated by code that is controlled by a
tristate config.  Otherwise, sure "init/main.c" is a "code module" and
so is every C file, making the distinction meaningless.  Further....

>     /**
>      * module_init() - driver initialization entry point
>      * @x: function to be run at kernel boot time or module insertion
>      *
>      * module_init() will either be called during do_initcalls() (if
>      * builtin) or at module insertion time (if a module).  There can only
>      * be one per module.

...I don't see how you can use the above comments to imply agreement
with your interpretation.  The above refers to what is done with
tristate (i.e. modular) code in the =y case and the =m case.  I'd be
reluctant to think it meant anything about non-modular code in general.
Moving this block inside of module.h helps clarify that, as well.

>      */
>     #define module_init(x)  __initcall(x);
> 
> I can understand you want to restrict "module_init()" to real loadable
> modules, but "device_initcall()" sounds like a real bad name or this.

It is an existing name, it is part of the infrastructure added to
replace the non-prioritized __initcall, and what is wrong with a
non-modular device driver calling device_initcall()?  I don't see the
badness.  Seems like quite a good fit, actually.  Just like having
non-modular specific arch code calling arch_initcall() etc. etc.

> 
> Furthermore, many places that contain always compiled-in code and
> currently only use module_init() should probably start using module_exit()
> as well, to do the proper cleanups to make kexec work.

Always compiled in code that uses module_init() blocks us from ever
properly making use of the prioritied initcall levels, because they
all land in one bucket.  See the mm and kernel commits making use of
priority levels in mm/ and kernel/ in akpm's mmotm tree for examples.

kexec: BenH started an interesting thread about it and what it needs for
a new powerpc target; it is an interesting read, but what kexec does in
the future will be largely independent of what I'm trying to do here.
Things like putting a device into a quiescent state vs removing the
actual driver and its associated memory allocations - the former would
be a new kexec specific callback.

Thanks for the feedback, but I'm not sure I'm following or agreeing
with your reservations about making these fairly simple and
straighforward clarifications/cleanups.  I'm hoping perhaps in the above
I've had some luck in clarifying why they are useful and sensible.

Paul.
--

> 
> So I'm not really convinced this is the way we want to go...
> 
> > Note that direct use of __initcall is discouraged, vs. one
> > of the priority categorized subgroups.  As __initcall gets
> > mapped onto device_initcall, our use of device_initcall
> > directly in this change means that the runtime impact is
> > zero -- it will remain at level 6 in initcall ordering.
> >
> > Cc: Geert Uytterhoeven <geert@...ux-m68k.org>
> > Cc: linux-m68k@...ts.linux-m68k.org
> > Signed-off-by: Paul Gortmaker <paul.gortmaker@...driver.com>
> >
> > diff --git a/arch/m68k/mvme16x/rtc.c b/arch/m68k/mvme16x/rtc.c
> > index 6ef7a81a3b12..1755e2f7137d 100644
> > --- a/arch/m68k/mvme16x/rtc.c
> > +++ b/arch/m68k/mvme16x/rtc.c
> > @@ -161,4 +161,4 @@ static int __init rtc_MK48T08_init(void)
> >         printk(KERN_INFO "MK48T08 Real Time Clock Driver v%s\n", RTC_VERSION);
> >         return misc_register(&rtc_dev);
> >  }
> > -module_init(rtc_MK48T08_init);
> > +device_initcall(rtc_MK48T08_init);
> 
> Gr{oetje,eeting}s,
> 
>                         Geert
> 
> --
> Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@...ux-m68k.org
> 
> In personal conversations with technical people, I call myself a hacker. But
> when I'm talking to journalists I just say "programmer" or something like that.
>                                 -- Linus Torvalds
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists