cpufreq: mediatek: allow modular build

cpufreq: mediatek: allow modular build

[Arnd Bergmann]

The newly merged cpufreq-mt8173 driver breaks the ARM allmodconfig
build because of a dependency on the cpu-cooling infrastructure that
may be built as a loadable module:

drivers/built-in.o: In function `mtk_cpufreq_ready':
binder.c:(.text+0x324c8c): undefined reference to `of_cpufreq_cooling_register'
drivers/built-in.o: In function `mtk_cpufreq_exit':
binder.c:(.text+0x324ea0): undefined reference to `cpufreq_cooling_unregister'

This works around the issue by allowing this driver to be built
as a module as well, and adding a dependency on THERMAL that prevents
it from being built-in when the cpu-cooling driver is a module.

This is not perfect because there is still a case where THERMAL=m
and CPU_COOLING=n that should allow us to have this driver built-in
as well, but I decided to follow existing practice in other drivers
here, and that case seems irrelevant in practice.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
I have not checked if someone else has already sent a patch for this,
just ignore mine if the issue has been fixed already.

diff --git a/drivers/cpufreq/Kconfig.arm b/drivers/cpufreq/Kconfig.arm
index 77aa34eae92c..28844bdf026d 100644
--- a/drivers/cpufreq/Kconfig.arm
+++ b/drivers/cpufreq/Kconfig.arm
@@ -131,8 +131,8 @@ config ARM_KIRKWOOD_CPUFREQ
 	  SoCs.
 
 config ARM_MT8173_CPUFREQ
-	bool "Mediatek MT8173 CPUFreq support"
-	depends on ARCH_MEDIATEK && REGULATOR
+	tristate "Mediatek MT8173 CPUFreq support"
+	depends on ARCH_MEDIATEK && REGULATOR && THERMAL
 	select PM_OPP
 	help
 	  This adds the CPUFreq driver support for Mediatek MT8173 SoC.
diff --git a/drivers/cpufreq/mt8173-cpufreq.c b/drivers/cpufreq/mt8173-cpufreq.c
index 49caed293a3b..2131e1a81be9 100644
--- a/drivers/cpufreq/mt8173-cpufreq.c
+++ b/drivers/cpufreq/mt8173-cpufreq.c
@@ -17,6 +17,7 @@
 #include <linux/cpu_cooling.h>
 #include <linux/cpufreq.h>
 #include <linux/cpumask.h>
+#include <linux/module.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/pm_opp.h>
@@ -524,4 +525,5 @@ static int mt8173_cpufreq_driver_init(void)
 
 	return 0;
 }
-device_initcall(mt8173_cpufreq_driver_init);
+module_init(mt8173_cpufreq_driver_init);
+MODULE_LICENSE("GPL v2");

cpufreq: mediatek: allow modular build

[Viresh Kumar]

On 11-09-15, 10:15, Arnd Bergmann wrote:
> The newly merged cpufreq-mt8173 driver breaks the ARM allmodconfig
> build because of a dependency on the cpu-cooling infrastructure that
> may be built as a loadable module:
> 
> drivers/built-in.o: In function `mtk_cpufreq_ready':
> binder.c:(.text+0x324c8c): undefined reference to `of_cpufreq_cooling_register'
> drivers/built-in.o: In function `mtk_cpufreq_exit':
> binder.c:(.text+0x324ea0): undefined reference to `cpufreq_cooling_unregister'
> 
> This works around the issue by allowing this driver to be built
> as a module as well, and adding a dependency on THERMAL that prevents
> it from being built-in when the cpu-cooling driver is a module.
> 
> This is not perfect because there is still a case where THERMAL=m
> and CPU_COOLING=n that should allow us to have this driver built-in
> as well, but I decided to follow existing practice in other drivers
> here, and that case seems irrelevant in practice.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> ---
> I have not checked if someone else has already sent a patch for this,
> just ignore mine if the issue has been fixed already.

5269e7067cd6 ("cpufreq: Add ARM_MT8173_CPUFREQ dependency on THERMAL")

in Rafael's tree. Its a bit different, so have a look.
-- 
viresh

cpufreq: mediatek: allow modular build

[Arnd Bergmann]

On Friday 11 September 2015 13:47:53 Viresh Kumar wrote:
> On 11-09-15, 10:15, Arnd Bergmann wrote:
> > The newly merged cpufreq-mt8173 driver breaks the ARM allmodconfig
> > build because of a dependency on the cpu-cooling infrastructure that
> > may be built as a loadable module:
> > 
> > drivers/built-in.o: In function `mtk_cpufreq_ready':
> > binder.c:(.text+0x324c8c): undefined reference to `of_cpufreq_cooling_register'
> > drivers/built-in.o: In function `mtk_cpufreq_exit':
> > binder.c:(.text+0x324ea0): undefined reference to `cpufreq_cooling_unregister'
> > 
> > This works around the issue by allowing this driver to be built
> > as a module as well, and adding a dependency on THERMAL that prevents
> > it from being built-in when the cpu-cooling driver is a module.
> > 
> > This is not perfect because there is still a case where THERMAL=m
> > and CPU_COOLING=n that should allow us to have this driver built-in
> > as well, but I decided to follow existing practice in other drivers
> > here, and that case seems irrelevant in practice.
> > 
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > ---
> > I have not checked if someone else has already sent a patch for this,
> > just ignore mine if the issue has been fixed already.
> 
> 5269e7067cd6 ("cpufreq: Add ARM_MT8173_CPUFREQ dependency on THERMAL")
> 
> in Rafael's tree. Its a bit different, so have a look.

That fix looks correct to me too, thanks for the quick reply!

In my approach, I decided to allow the driver to be a module, as that
seems nicer for multi_v7_defconfig, but I now see that there are
several other drivers that can only be built-in, so if we decided to
make that the general strategy we should change them all.

	Arnd

cpufreq: mediatek: allow modular build

[Viresh Kumar]

On 11-09-15, 10:22, Arnd Bergmann wrote:
> In my approach, I decided to allow the driver to be a module, as that
> seems nicer for multi_v7_defconfig, but I now see that there are
> several other drivers that can only be built-in, so if we decided to
> make that the general strategy we should change them all.

And we need to do that with a proper module_exit() function, otherwise
we are really adding a BUG. Which you just did with your patch :)

-- 
viresh

cpufreq: mediatek: allow modular build

[Arnd Bergmann]

On Friday 11 September 2015 13:55:36 Viresh Kumar wrote:
> On 11-09-15, 10:22, Arnd Bergmann wrote:
> > In my approach, I decided to allow the driver to be a module, as that
> > seems nicer for multi_v7_defconfig, but I now see that there are
> > several other drivers that can only be built-in, so if we decided to
> > make that the general strategy we should change them all.
> 
> And we need to do that with a proper module_exit() function, otherwise
> we are really adding a BUG. Which you just did with your patch 

I don't consider that a bug: a module with just an init function and
no exit function can be loaded once and never unloaded, which is not
nice for debugging, but is otherwise fully functional.

	Arnd

cpufreq: mediatek: allow modular build

[Viresh Kumar]

On 11-09-15, 10:36, Arnd Bergmann wrote:
> I don't consider that a bug: a module with just an init function and
> no exit function can be loaded once and never unloaded, which is not
> nice for debugging, but is otherwise fully functional.

For me, there are two essential things that a module has to support:
- hotplug, which is just fine.
- hot-unplug, which will pass as well, but without freeing resources..

:)

-- 
viresh

cpufreq: mediatek: allow modular build

[Russell King - ARM Linux]

On Fri, Sep 11, 2015 at 02:08:06PM +0530, Viresh Kumar wrote:
> On 11-09-15, 10:36, Arnd Bergmann wrote:
> > I don't consider that a bug: a module with just an init function and
> > no exit function can be loaded once and never unloaded, which is not
> > nice for debugging, but is otherwise fully functional.
> 
> For me, there are two essential things that a module has to support:
> - hotplug, which is just fine.
> - hot-unplug, which will pass as well, but without freeing resources..

Well, that's got nothing to do with not having a module_exit().

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

cpufreq: mediatek: allow modular build

[Viresh Kumar]

On 11-09-15, 10:34, Russell King - ARM Linux wrote:
> Well, that's got nothing to do with not having a module_exit().

Yeah, so we don't necessarily need a module_exit(), but the module
should have freed the resources with module unplug. And that was
missing with Arnd's patch..

Or did I misinterpret your comment ?

-- 
viresh

cpufreq: mediatek: allow modular build

[Russell King - ARM Linux]

On Fri, Sep 11, 2015 at 03:10:03PM +0530, Viresh Kumar wrote:
> On 11-09-15, 10:34, Russell King - ARM Linux wrote:
> > Well, that's got nothing to do with not having a module_exit().
> 
> Yeah, so we don't necessarily need a module_exit(), but the module
> should have freed the resources with module unplug. And that was
> missing with Arnd's patch..

Not module "unplug" (I've never heard it called that before).  A module
with a module_init() but no module_exit() can only be added to a running
kernel, and never removed.  That's intentional behaviour.

However, if you're talking about hot-unplug, presumably you're talking
about _CPU_ hot-unplug, and that's something the code should definitely
handle irrespective of whether it's built-in or a module.

The two issues are entirely separate.

-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

cpufreq: mediatek: allow modular build

[Viresh Kumar]

On 11-09-15, 10:58, Russell King - ARM Linux wrote:
> Not module "unplug" (I've never heard it called that before).  A module
> with a module_init() but no module_exit() can only be added to a running
> kernel, and never removed.  That's intentional behaviour.

Ah, I see. I wasn't sure that a module with no module_exit() can't be
removed.

> However, if you're talking about hot-unplug, presumably you're talking
> about _CPU_ hot-unplug, and that's something the code should definitely
> handle irrespective of whether it's built-in or a module.
> 
> The two issues are entirely separate.

Bah, all this time I meant module insertion/removal, sorry :(

-- 
viresh

cpufreq: mediatek: allow modular build

[Russell King - ARM Linux]

On Fri, Sep 11, 2015 at 03:39:16PM +0530, Viresh Kumar wrote:
> On 11-09-15, 10:58, Russell King - ARM Linux wrote:
> > Not module "unplug" (I've never heard it called that before).  A module
> > with a module_init() but no module_exit() can only be added to a running
> > kernel, and never removed.  That's intentional behaviour.
> 
> Ah, I see. I wasn't sure that a module with no module_exit() can't be
> removed.

Yes, but it's not that simple.

A module with no module_init() and no module_exit() is what's called a
library module, which can be inserted, and later removed provided no
other module depends on anything the library module exports.

A module with a module_init() but no module_exit() is one which can be
inserted, but never removed.

A module with a module_init() and a module_exit() can be inserted, and
later removed in much the same way as the library module mentioned above.

See kernel/module.c:

        /* If it has an init func, it must have an exit func to unload */
        if (mod->init && !mod->exit) {
                forced = try_force_unload(flags);
                if (!forced) {
                        /* This module can't be removed */
                        ret = -EBUSY;
                        goto out;
                }
        }

and

        if (mod->init != NULL && mod->exit == NULL) {
                printed_something = 1;
                seq_puts(m, "[permanent],");
        }


-- 
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

cpufreq: mediatek: allow modular build

[Viresh Kumar]

On 11-09-15, 11:13, Russell King - ARM Linux wrote:
> Yes, but it's not that simple.
> 
> A module with no module_init() and no module_exit() is what's called a
> library module, which can be inserted, and later removed provided no
> other module depends on anything the library module exports.
> 
> A module with a module_init() but no module_exit() is one which can be
> inserted, but never removed.
> 
> A module with a module_init() and a module_exit() can be inserted, and
> later removed in much the same way as the library module mentioned above.
> 
> See kernel/module.c:
> 
>         /* If it has an init func, it must have an exit func to unload */
>         if (mod->init && !mod->exit) {
>                 forced = try_force_unload(flags);
>                 if (!forced) {
>                         /* This module can't be removed */
>                         ret = -EBUSY;
>                         goto out;
>                 }
>         }
> 
> and
> 
>         if (mod->init != NULL && mod->exit == NULL) {
>                 printed_something = 1;
>                 seq_puts(m, "[permanent],");
>         }

Thanks for that, really appreciate it.

And now I realize that Arnd was correct to start with :)

-- 
viresh