Message-ID: <f0de01e8-fea5-38b4-8711-db84232744df@axentia.se>
Date:   Thu, 9 Mar 2017 10:39:27 +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-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

Powered by blists - more mailing lists