Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1594715 > unrolled thread

[PATCH] mux-core: make it explicitly non-modular

Back to article view | Back to linux.kernel

Contents

  [PATCH] mux-core: make it explicitly non-modular Paul Gortmaker <paul.gortmaker@windriver.com> - 2017-03-08 00:00 +0100
    Re: [PATCH] mux-core: make it explicitly non-modular Paul Gortmaker <paul.gortmaker@windriver.com> - 2017-03-08 15:40 +0100
      Re: [PATCH] mux-core: make it explicitly non-modular Paul Gortmaker <paul.gortmaker@windriver.com> - 2017-03-09 01:40 +0100
        Re: [PATCH] mux-core: make it explicitly non-modular Paul Gortmaker <paul.gortmaker@windriver.com> - 2017-03-09 23:40 +0100

#1594715 — [PATCH] mux-core: make it explicitly non-modular

The Kconfig currently controlling compilation of this code is:

drivers/mux/Kconfig:menuconfig MULTIPLEXER
drivers/mux/Kconfig:    bool "Multiplexer subsystem"

...meaning that it currently is not being built as a module by anyone.

Lets remove the couple traces of modular infrastructure use, so that
when reading the driver there is no doubt it is builtin-only.

Hence we delete the MODULE_LICENSE tag etc. since all that information
is already contained at the top of the file in the comments.

Cc: Peter Rosin <peda@axentia.se>
Cc: Jonathan Cameron <jic23@kernel.org>
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---

[feel free to fold this change into the original addition commit if
 you happen to be rebasing for some other reason... ]

 drivers/mux/mux-core.c | 6 +-----
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
index 46088a0f9677..7b4af6370e37 100644
--- a/drivers/mux/mux-core.c
+++ b/drivers/mux/mux-core.c
@@ -15,7 +15,7 @@
 #include <linux/device.h>
 #include <linux/err.h>
 #include <linux/idr.h>
-#include <linux/module.h>
+#include <linux/init.h>
 #include <linux/mux.h>
 #include <linux/of.h>
 #include <linux/of_platform.h>
@@ -408,7 +408,3 @@ void devm_mux_control_put(struct device *dev, struct mux_control *mux)
 EXPORT_SYMBOL_GPL(devm_mux_control_put);
 
 subsys_initcall(mux_init);
-
-MODULE_DESCRIPTION("Multiplexer subsystem");
-MODULE_AUTHOR("Peter Rosin <peda@axentia.se");
-MODULE_LICENSE("GPL v2");
-- 
2.11.0

[toc] | [next] | [standalone]

#1595225

[Re: [PATCH] mux-core: make it explicitly non-modular] On 08/03/2017 (Wed 10:38) Peter Rosin wrote:

> On 2017-03-07 23:41, Paul Gortmaker wrote:
> > The Kconfig currently controlling compilation of this code is:
> > 
> > drivers/mux/Kconfig:menuconfig MULTIPLEXER
> > drivers/mux/Kconfig:    bool "Multiplexer subsystem"
> > 
> > ...meaning that it currently is not being built as a module by anyone.
> > 
> > Lets remove the couple traces of modular infrastructure use, so that
> > when reading the driver there is no doubt it is builtin-only.
> > 
> > Hence we delete the MODULE_LICENSE tag etc. since all that information
> > is already contained at the top of the file in the comments.
> 
> Hi Paul,
> 
> Yup, it is confirmed, I don't really know what I'm doing. In particular
> when in comes to modules... I did wonder about this when I wrote the
> code and one specific thing I wondered about was how module loading is
> triggered.
> 
> I can imagine that calling a function that happens to be exported from
> a module triggers its loading and that failure to load the module leads
> to an oops. But I don't know if that is even remotely correct? Is it?

No, that would be pretty user unfriendly.  When you "insmod" a module,
the kernel checks just like a linker, that all the functions it wants to
use are available.  If they are not, then the kernel fails to load it.
But it fails gracefully with a list of the unresolved symbols.

Obviously managing all the module interdependencies manually would be
tedious, so that is what what "depmod" does.  Then loading of a module
is usually done with "modprobe", which will consult the depmod data and
then "insmod" the required modules in the needed order.

> Is there a short answer? Or what should I read for a longer one?

Well, now that you know it won't oops from a module needing a module,
perhaps you do now want to make the core support modular?  If I look in
my ARM build, I see:

    paul@yow-lpgnfs-02:~/git/arm-build/drivers/mux$ ls -l
    total 96
    -rw-rw-r-- 1 paul paul   399 Mar  7 16:41 built-in.mod.c
    -rw-rw-r-- 1 paul paul 15999 Mar  7 16:41 built-in.o
    -rw-rw-r-- 1 paul paul     0 Mar  7 16:12 modules.builtin
    -rw-rw-r-- 1 paul paul    65 Mar  7 16:41 modules.order
    -rw-rw-r-- 1 paul paul  8324 Mar  7 16:49 mux-adg792a.ko
    -rw-rw-r-- 1 paul paul  1566 Mar  7 16:46 mux-adg792a.mod.c
    -rw-rw-r-- 1 paul paul  3552 Mar  7 16:47 mux-adg792a.mod.o
    -rw-rw-r-- 1 paul paul  6824 Mar  7 16:41 mux-adg792a.o
    -rw-rw-r-- 1 paul paul 20472 Mar  7 16:41 mux-core.o
    -rw-rw-r-- 1 paul paul  7680 Mar  7 16:49 mux-gpio.ko
    -rw-rw-r-- 1 paul paul  1550 Mar  7 16:46 mux-gpio.mod.c
    -rw-rw-r-- 1 paul paul  3408 Mar  7 16:47 mux-gpio.mod.o
    -rw-rw-r-- 1 paul paul  6360 Mar  7 16:41 mux-gpio.o
    paul@yow-lpgnfs-02:~/git/arm-build/drivers/mux$ 

Note that mux-core doesn't have a .ko (modular variant) and also you can
confirm with nm that everything in mux-core.o is in built-in.o (in this
case the nm outputs are virtually identical).

But if you do make that a module too, you will need the MODULE_LICENSE
tag.  The kernel also checks that license compatibility is maintained ;
i.e. a proprietary module can't use functions from another module doing
EXPORT_SYMBOL_GPL.

> Anyway, I'll add this to the queue, and fold it if I happen to rebase.

Oh, and speaking of EXPORT_SYMBOL, I should have added <linux/export.h>
to your file with the patch I sent, since it does do exports.

Hope that helps,
Paul.
--

> Thanks!
> 
> Cheers,
> peda
> 
> > Cc: Peter Rosin <peda@axentia.se>
> > Cc: Jonathan Cameron <jic23@kernel.org>
> > Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
> > ---
> > 
> > [feel free to fold this change into the original addition commit if
> >  you happen to be rebasing for some other reason... ]
> > 
> >  drivers/mux/mux-core.c | 6 +-----
> >  1 file changed, 1 insertion(+), 5 deletions(-)
> > 
> > diff --git a/drivers/mux/mux-core.c b/drivers/mux/mux-core.c
> > index 46088a0f9677..7b4af6370e37 100644
> > --- a/drivers/mux/mux-core.c
> > +++ b/drivers/mux/mux-core.c
> > @@ -15,7 +15,7 @@
> >  #include <linux/device.h>
> >  #include <linux/err.h>
> >  #include <linux/idr.h>
> > -#include <linux/module.h>
> > +#include <linux/init.h>
> >  #include <linux/mux.h>
> >  #include <linux/of.h>
> >  #include <linux/of_platform.h>
> > @@ -408,7 +408,3 @@ void devm_mux_control_put(struct device *dev, struct mux_control *mux)
> >  EXPORT_SYMBOL_GPL(devm_mux_control_put);
> >  
> >  subsys_initcall(mux_init);
> > -
> > -MODULE_DESCRIPTION("Multiplexer subsystem");
> > -MODULE_AUTHOR("Peter Rosin <peda@axentia.se");
> > -MODULE_LICENSE("GPL v2");
> > 
> 

[toc] | [prev] | [next] | [standalone]

#1595601

[Re: [PATCH] mux-core: make it explicitly non-modular] On 08/03/2017 (Wed 16:32) Peter Rosin wrote:

> On 2017-03-08 15:38, Paul Gortmaker wrote:

[...]

> > 
> > Note that mux-core doesn't have a .ko (modular variant) and also you can
> > confirm with nm that everything in mux-core.o is in built-in.o (in this
> > case the nm outputs are virtually identical).
> 
> Yes, I was aware that the mux core was not set up to be built as a module,
> and that was intentional. But the only reason for that was that I thought
> subsystem cores were never modular. The module related remains that
> you removed were just that, remains from other code used as a template.

So, I used to think the same -- core subsystems were not modules.  But
what has happened is that we've got all these different ARM boards, and
they have board specific PCIe drivers/setup and board specific DMA
drivers.  Add to that the desire to have a distro like kernel for a
group of boards, and you get a situation where people want to boot the
absolute minimum vmlinux to support the core and RAM, and then load PCIe
and DMA drivers from a ramdisk/initrd as the specific board is detected.

The goal of not having a bloated vmlinux and a higher resource threshold
needed just for booting is reasonably valid I think.  But at the same
time, if the end target will never be part of a shared vmlinux image,
but always part of a board specific ".config" build, then the effort to
make such core subsystems modular is rather pointless.  Hence it isn't
sensible to have a blanket rule/policy -- it needs looking case by case.

If the mux subsystem isn't board specific (and at a glance, it seems
like it is not) then it might be the former and not the latter.  But I
leave that call up to you.

> > But if you do make that a module too, you will need the MODULE_LICENSE
> > tag.  The kernel also checks that license compatibility is maintained ;
> > i.e. a proprietary module can't use functions from another module doing
> > EXPORT_SYMBOL_GPL.
> > 
> >> Anyway, I'll add this to the queue, and fold it if I happen to rebase.
> > 
> > Oh, and speaking of EXPORT_SYMBOL, I should have added <linux/export.h>
> > to your file with the patch I sent, since it does do exports.
> 
> Ok, I assume that it should be included by the individual drivers as well?

Not really -- drivers, as leaf nodes in the overall system rarely do any
EXPORT_SYMBOL operations.  It is the exporters and not the consumers who
should explicitly include that header.

> > Hope that helps,
> > Paul.
> 
> Yup, it helps, but it doesn't really help with making the call if it
> should be possible to build the mux core as module or not. What things
> are not possible to build as modules? E.g. there's the mux_ida thing,

See above comments re: sharing vs. board specific.  You should then be
better able to make the call.

> will that work as expected if the mux core is modular (and possibly
> unloaded)? Isn't there a risk that names will be reused and confusing
> and cause bugs? Or is there a way to block a module from being unloaded?

Per the above use case (minimal vmlinux) the unload isn't so important,
and in many cases it may be racy or simply not make sense.  If that is
the case, you don't need to provide a module_exit, and in the past I
think we used to bump the module use count at a successful load to
prevent unloading ; there might be a more elegant method now; google is
your friend here.  Also, I don't think name reuse/confusion is an issue.

> Hmmm, there's also the ".owner = THIS_MODULE" line in mux-core.c that
> seems related, is that line valid for non-modular "units"?

That is largely a legacy bit of (repeatedly copied) boiler plate.  If I
recall correctly, in a non-module it becomes ".owner = NULL".  And even
if you are a proper module, I think the higher level wrappers like
module_platform_driver() do the right thing with .owner so that drivers
don't have to duplicate such mundane boilerplate.  If you search git
history for THIS_MODULE, I think you will find batch removals from
drivers for where the per-driver boilerplate is simply redundant.

Paul.
--

> 
> Cheers,
> peda
> 

[toc] | [prev] | [next] | [standalone]

#1596438

[Re: [PATCH] mux-core: make it explicitly non-modular] On 09/03/2017 (Thu 13:01) Peter Rosin wrote:

> On 2017-03-09 10:39, Peter Rosin wrote:
> 
> *snip* *snip*
> 
> >> Per the above use case (minimal vmlinux) the unload isn't so important,
> >> and in many cases it may be racy or simply not make sense.  If that is
> >> the case, you don't need to provide a module_exit, and in the past I
> >> think we used to bump the module use count at a successful load to
> >> prevent unloading ; there might be a more elegant method now; google is
> >> your friend here.  Also, I don't think name reuse/confusion is an issue.
> > 
> > Ok, I thought it all boiled down to making the mux-core Kconfig a tristate
> > option and changing subsys_initcall(...) to module_init(...).
> > 
> > But if I do that, I cannot be sure that the mux-core has been initialized
> > before drivers and clients start to use it in the non-modular case (if
> > things are modules, the dependencies should ensure that the mux-core is
> > loaded/initialized before any users are loaded). But there is no
> > topological sorting going on for ordering init calls in the non-modular
> > case.
> > 
> > In short, I get:
> > 
> > WARNING: CPU: 0 PID: 1 at drivers/base/class.c:438: class_find_device+0xac/0xb8
> > class_find_device called for class 'mux' before it was initialized

[...]

> > 
> > So, it appears that I also have to determine if the core has been
> > initialized in all its entry points and return -EPROBE_DEFER (or something)
> > when not. I suppose I could instead initialize on-demand, but that seems
> > more difficult the do without races...
> > 
> > Am I missing something?
> 
> I did some digging and e.g. drivers/uwb was moved before drivers/usb in
> commit ae5d82cb8d60 ("uwb: build UWB before USB/WUSB") to resolve what
> appears to be a similar situation. I suppose I could just move mux up in
> drivers/Makefile so that it is before both i2c and iio.

So, you can "cheat" and leave it as subsys_initcall (and add a comment
why it is) and it will be subsys_initcall when built-in and module_init
when modular -- that should work too, and if you look around you'll
probably see many other existing instances of that going on.

init.h
   #ifndef MODULE
   [...]
   #define subsys_initcall(fn)             __define_initcall(fn, 4)

module.h
   #else /* MODULE */
   [...]
   #define subsys_initcall(fn)              module_init(fn)

Paul.
--

> 
> Cheers,
> peda
> 

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web