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

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

[Paul Gortmaker]

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

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

[Peter Rosin]

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?

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

Anyway, I'll add this to the queue, and fold it if I happen to rebase.
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");
> 

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

[Paul Gortmaker]

[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");
> > 
> 

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

[Peter Rosin]

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

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@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");
>>>
>>

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

[Paul Gortmaker]

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

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

[Peter Rosin]

On 2017-03-09 01:32, Paul Gortmaker wrote:
*snip* *snip*

> 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

The mux subsystem may be needed for some boards and not for others, and
boards not needing it could benefit from the de-bloating. So, a modular
core is probably desired, methinks...

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

Yes, of course. I was confused for a bit. Possibly because at some previous
revisions the mux core did depend on one of the drivers (mux-gpio) and that
driver did export a hook function that the core used. That never felt clean
and in the end it was dropped. At that time, the mux-gpio driver was also
made non-modular to prevent the circular dependency. I suppose the same thing
could have been accomplished with a third "library" module that both the
core and the driver could have accessed...

> 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
CPU:0 PID: 1 Comm: swapper Not tainted 4.11-rc1+ #1243
Hardware name: Atmel SAMA5
[<c010d24c>] (unwind_backtrace) from [<c010b1ec>] (show_stack+0x10/0x14)
[<c010b1ec>] (show_stack) from [<c0115290>] (__warn+0xe0/0xf8)
[<c0115290>] (__warn) from [<c01152e0>] (warn_slowpath_fmt+0x38/0x48)
[<c01152e0>] (warn_slowpath_fmt) from [<c034715c>] (class_find_device+0xac/0xb8)
[<c034715c>] (class_find_device) from [<c0434230>] (mux_control_get+0xa0/0x1bc)
[<c0434230>] (mux_control_get) from [<c0434394>] (devm_mux_control_get+0x3c/0x78)
[<c0434394>] (devm_mux_control_get) from [<c04060f0>] (i2c_mux_probe+0x44/0x294)
[<c04060f0>] (i2c_mux_probe) from [<c03475ac>] (platform_drv_probe+0x4c/0xb0)
[<c03475ac>] (platform_drv_probe) from [<c0345e30>] (driver_probe_device+0x26c/0x464)
[<c0345e30>] (driver_probe_device) from [<c0346128>] (__driver_attach+0x100/0x11c)
[<c0346128>] (__driver_attach) from [<c034403c>] (bus_for_each_dev+0x6c/0xa0)
[<c034403c>] (bus_for_each_dev) from [<c0344a94>] (bus_add_driver+0x1c8/0x260)
[<c0344a94>] (bus_add_driver) from [<c0346a7c>] (driver_register+0x78/0xf8)
[<c0346a7c>] (driver_register) from [<c0800d50>] (do_one_initcall+0xb0/0x15c)
[<c0800d50>] (do_one_initcall) from [<c0800f38>] (kernel_init_freeable+0x13c/0x1d8)
[<c0800f38>] (kernel_init_freeable) from [<c057ab60>] (kernel_init+0x8/0x10c)
[<c057ab60>] (kernel_init) from [<c0107fb8>] (ret_from_fork+0x14/0x3c)
---[ end trace d97274a16af7ef1c ]---

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?

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

Ah, but in this case it's the owner of the mux class, not the mux-core
itself.

Cheers,
peda

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

[Peter Rosin]

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
> CPU:0 PID: 1 Comm: swapper Not tainted 4.11-rc1+ #1243
> Hardware name: Atmel SAMA5
> [<c010d24c>] (unwind_backtrace) from [<c010b1ec>] (show_stack+0x10/0x14)
> [<c010b1ec>] (show_stack) from [<c0115290>] (__warn+0xe0/0xf8)
> [<c0115290>] (__warn) from [<c01152e0>] (warn_slowpath_fmt+0x38/0x48)
> [<c01152e0>] (warn_slowpath_fmt) from [<c034715c>] (class_find_device+0xac/0xb8)
> [<c034715c>] (class_find_device) from [<c0434230>] (mux_control_get+0xa0/0x1bc)
> [<c0434230>] (mux_control_get) from [<c0434394>] (devm_mux_control_get+0x3c/0x78)
> [<c0434394>] (devm_mux_control_get) from [<c04060f0>] (i2c_mux_probe+0x44/0x294)
> [<c04060f0>] (i2c_mux_probe) from [<c03475ac>] (platform_drv_probe+0x4c/0xb0)
> [<c03475ac>] (platform_drv_probe) from [<c0345e30>] (driver_probe_device+0x26c/0x464)
> [<c0345e30>] (driver_probe_device) from [<c0346128>] (__driver_attach+0x100/0x11c)
> [<c0346128>] (__driver_attach) from [<c034403c>] (bus_for_each_dev+0x6c/0xa0)
> [<c034403c>] (bus_for_each_dev) from [<c0344a94>] (bus_add_driver+0x1c8/0x260)
> [<c0344a94>] (bus_add_driver) from [<c0346a7c>] (driver_register+0x78/0xf8)
> [<c0346a7c>] (driver_register) from [<c0800d50>] (do_one_initcall+0xb0/0x15c)
> [<c0800d50>] (do_one_initcall) from [<c0800f38>] (kernel_init_freeable+0x13c/0x1d8)
> [<c0800f38>] (kernel_init_freeable) from [<c057ab60>] (kernel_init+0x8/0x10c)
> [<c057ab60>] (kernel_init) from [<c0107fb8>] (ret_from_fork+0x14/0x3c)
> ---[ end trace d97274a16af7ef1c ]---
> 
> 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.

Cheers,
peda

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

[Paul Gortmaker]

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