* [PATCH] mux-core: make it explicitly non-modular @ 2017-03-07 22:41 Paul Gortmaker 2017-03-08 9:38 ` Peter Rosin 0 siblings, 1 reply; 8+ messages in thread From: Paul Gortmaker @ 2017-03-07 22:41 UTC (permalink / raw) To: linux-kernel; +Cc: Paul Gortmaker, Peter Rosin, Jonathan Cameron 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 ^ permalink raw reply related [flat|nested] 8+ messages in thread
* Re: [PATCH] mux-core: make it explicitly non-modular 2017-03-07 22:41 [PATCH] mux-core: make it explicitly non-modular Paul Gortmaker @ 2017-03-08 9:38 ` Peter Rosin 2017-03-08 14:38 ` Paul Gortmaker 0 siblings, 1 reply; 8+ messages in thread From: Peter Rosin @ 2017-03-08 9:38 UTC (permalink / raw) To: Paul Gortmaker, linux-kernel; +Cc: Jonathan Cameron 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"); > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mux-core: make it explicitly non-modular 2017-03-08 9:38 ` Peter Rosin @ 2017-03-08 14:38 ` Paul Gortmaker 2017-03-08 15:32 ` Peter Rosin 0 siblings, 1 reply; 8+ messages in thread From: Paul Gortmaker @ 2017-03-08 14:38 UTC (permalink / raw) To: Peter Rosin; +Cc: linux-kernel, Jonathan Cameron [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"); > > > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mux-core: make it explicitly non-modular 2017-03-08 14:38 ` Paul Gortmaker @ 2017-03-08 15:32 ` Peter Rosin 2017-03-09 0:32 ` Paul Gortmaker 0 siblings, 1 reply; 8+ messages in thread From: Peter Rosin @ 2017-03-08 15:32 UTC (permalink / raw) To: Paul Gortmaker; +Cc: linux-kernel, Jonathan Cameron 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"); >>> >> ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mux-core: make it explicitly non-modular 2017-03-08 15:32 ` Peter Rosin @ 2017-03-09 0:32 ` Paul Gortmaker 2017-03-09 9:39 ` Peter Rosin 0 siblings, 1 reply; 8+ messages in thread From: Paul Gortmaker @ 2017-03-09 0:32 UTC (permalink / raw) To: Peter Rosin; +Cc: linux-kernel, Jonathan Cameron [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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mux-core: make it explicitly non-modular 2017-03-09 0:32 ` Paul Gortmaker @ 2017-03-09 9:39 ` Peter Rosin 2017-03-09 12:01 ` Peter Rosin 0 siblings, 1 reply; 8+ messages in thread From: Peter Rosin @ 2017-03-09 9:39 UTC (permalink / raw) To: Paul Gortmaker; +Cc: linux-kernel, Jonathan Cameron 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mux-core: make it explicitly non-modular 2017-03-09 9:39 ` Peter Rosin @ 2017-03-09 12:01 ` Peter Rosin 2017-03-09 22:31 ` Paul Gortmaker 0 siblings, 1 reply; 8+ messages in thread From: Peter Rosin @ 2017-03-09 12:01 UTC (permalink / raw) To: Paul Gortmaker; +Cc: linux-kernel, Jonathan Cameron 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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH] mux-core: make it explicitly non-modular 2017-03-09 12:01 ` Peter Rosin @ 2017-03-09 22:31 ` Paul Gortmaker 0 siblings, 0 replies; 8+ messages in thread From: Paul Gortmaker @ 2017-03-09 22:31 UTC (permalink / raw) To: Peter Rosin; +Cc: linux-kernel, Jonathan Cameron [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 > ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2017-03-09 22:31 UTC | newest] Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-03-07 22:41 [PATCH] mux-core: make it explicitly non-modular Paul Gortmaker 2017-03-08 9:38 ` Peter Rosin 2017-03-08 14:38 ` Paul Gortmaker 2017-03-08 15:32 ` Peter Rosin 2017-03-09 0:32 ` Paul Gortmaker 2017-03-09 9:39 ` Peter Rosin 2017-03-09 12:01 ` Peter Rosin 2017-03-09 22:31 ` Paul Gortmaker
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).