Message-ID: <0945642d-0f21-3b92-d90a-6999e1d47900@axentia.se>
Date:   Wed, 8 Mar 2017 16:32:29 +0100
From:   Peter Rosin <peda@...ntia.se>
To:     Paul Gortmaker <paul.gortmaker@...driver.com>
CC:     <linux-kernel@...r.kernel.org>, Jonathan Cameron <jic23@...nel.org>
Subject: Re: [PATCH] mux-core: make it explicitly non-modular

On 2017-03-08 15:38, Paul Gortmaker wrote:
> [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.

Ahhh, insmod etc, that was a different millennia. I've been pretty much
away from Linux for a looong while...

>> 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@...-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@...-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).

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.

> 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?

> 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,
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?

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

Cheers,
peda

> --
> 
>> Thanks!
>>
>> Cheers,
>> peda
>>
>>> Cc: Peter Rosin <peda@...ntia.se>
>>> Cc: Jonathan Cameron <jic23@...nel.org>
>>> Signed-off-by: Paul Gortmaker <paul.gortmaker@...driver.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@...ntia.se");
>>> -MODULE_LICENSE("GPL v2");
>>>
>>

Powered by blists - more mailing lists