[PATCH 1/3] gpio: mxc: Support module build

[PATCH 1/3] gpio: mxc: Support module build

[Anson Huang]

Change config to tristate, add module device table, module author,
description and license to support module build for i.MX GPIO driver.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 drivers/gpio/Kconfig    | 2 +-
 drivers/gpio/gpio-mxc.c | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/drivers/gpio/Kconfig b/drivers/gpio/Kconfig
index 05e0801..4d09ec5 100644
--- a/drivers/gpio/Kconfig
+++ b/drivers/gpio/Kconfig
@@ -397,7 +397,7 @@ config GPIO_MVEBU
 	select REGMAP_MMIO
 
 config GPIO_MXC
-	def_bool y
+	tristate "i.MX GPIO support"
 	depends on ARCH_MXC || COMPILE_TEST
 	select GPIO_GENERIC
 	select GENERIC_IRQ_CHIP
diff --git a/drivers/gpio/gpio-mxc.c b/drivers/gpio/gpio-mxc.c
index 64278a4..643f4c55 100644
--- a/drivers/gpio/gpio-mxc.c
+++ b/drivers/gpio/gpio-mxc.c
@@ -15,6 +15,7 @@
 #include <linux/irq.h>
 #include <linux/irqdomain.h>
 #include <linux/irqchip/chained_irq.h>
+#include <linux/module.h>
 #include <linux/platform_device.h>
 #include <linux/slab.h>
 #include <linux/syscore_ops.h>
@@ -158,6 +159,7 @@ static const struct of_device_id mxc_gpio_dt_ids[] = {
 	{ .compatible = "fsl,imx7d-gpio", .data = &mxc_gpio_devtype[IMX35_GPIO], },
 	{ /* sentinel */ }
 };
+MODULE_DEVICE_TABLE(of, mxc_gpio_dt_ids);
 
 /*
  * MX2 has one interrupt *for all* gpio ports. The list is used
@@ -604,3 +606,7 @@ static int __init gpio_mxc_init(void)
 	return platform_driver_register(&mxc_gpio_driver);
 }
 subsys_initcall(gpio_mxc_init);
+
+MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>");
+MODULE_DESCRIPTION("i.MX GPIO Driver");
+MODULE_LICENSE("GPL");
-- 
2.7.4

[PATCH 2/3] arm64: defconfig: Build in CONFIG_GPIO_MXC by default

[Anson Huang]

i.MX GPIO is NOT default enabled now, so select CONFIG_GPIO_MXC
as built-in manually.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 arch/arm64/configs/defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/configs/defconfig b/arch/arm64/configs/defconfig
index 278e78f..6817d01 100644
--- a/arch/arm64/configs/defconfig
+++ b/arch/arm64/configs/defconfig
@@ -487,6 +487,7 @@ CONFIG_GPIO_PCA953X=y
 CONFIG_GPIO_PCA953X_IRQ=y
 CONFIG_GPIO_BD9571MWV=m
 CONFIG_GPIO_MAX77620=y
+CONFIG_GPIO_MXC=y
 CONFIG_POWER_AVS=y
 CONFIG_QCOM_CPR=y
 CONFIG_ROCKCHIP_IODOMAIN=y
-- 
2.7.4

[PATCH 3/3] ARM: imx_v6_v7_defconfig: Build in CONFIG_GPIO_MXC by default

[Anson Huang]

i.MX GPIO is NOT default enabled now, so select CONFIG_GPIO_MXC
as built-in manually.

Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
 arch/arm/configs/imx_v6_v7_defconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/configs/imx_v6_v7_defconfig b/arch/arm/configs/imx_v6_v7_defconfig
index 38968b8..4956fc8 100644
--- a/arch/arm/configs/imx_v6_v7_defconfig
+++ b/arch/arm/configs/imx_v6_v7_defconfig
@@ -225,6 +225,7 @@ CONFIG_GPIO_PCA953X=y
 CONFIG_GPIO_PCF857X=y
 CONFIG_GPIO_STMPE=y
 CONFIG_GPIO_74X164=y
+CONFIG_GPIO_MXC=y
 CONFIG_POWER_RESET=y
 CONFIG_POWER_RESET_SYSCON=y
 CONFIG_POWER_RESET_SYSCON_POWEROFF=y
-- 
2.7.4

Re: [PATCH 3/3] ARM: imx_v6_v7_defconfig: Build in CONFIG_GPIO_MXC by default

[Andreas Kemnade]

[-- Attachment #1: Type: text/plain, Size: 405 bytes --]

Hi,

On Wed,  8 Jul 2020 07:25:23 +0800
Anson Huang <Anson.Huang@nxp.com> wrote:

> i.MX GPIO is NOT default enabled now, so select CONFIG_GPIO_MXC
> as built-in manually.
> 
> Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> ---
>  arch/arm/configs/imx_v6_v7_defconfig | 1 +
>  1 file changed, 1 insertion(+)
> 
shouldn't this be done also in the multi_v7_defconfig?

Regards,
Andreas

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

RE: [PATCH 3/3] ARM: imx_v6_v7_defconfig: Build in CONFIG_GPIO_MXC by default

[Anson Huang]

Hi, Andreas


> Subject: Re: [PATCH 3/3] ARM: imx_v6_v7_defconfig: Build in
> CONFIG_GPIO_MXC by default
> 
> Hi,
> 
> On Wed,  8 Jul 2020 07:25:23 +0800
> Anson Huang <Anson.Huang@nxp.com> wrote:
> 
> > i.MX GPIO is NOT default enabled now, so select CONFIG_GPIO_MXC as
> > built-in manually.
> >
> > Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
> > ---
> >  arch/arm/configs/imx_v6_v7_defconfig | 1 +
> >  1 file changed, 1 insertion(+)
> >
> shouldn't this be done also in the multi_v7_defconfig?

OK, I will add it in V2.

Thanks,
Anson

Re: [PATCH 1/3] gpio: mxc: Support module build

[Linus Walleij]

On Wed, Jul 8, 2020 at 1:28 AM Anson Huang <Anson.Huang@nxp.com> wrote:

>  subsys_initcall(gpio_mxc_init);
> +
> +MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>");
> +MODULE_DESCRIPTION("i.MX GPIO Driver");
> +MODULE_LICENSE("GPL");

You are making this modualrizable but keeping the subsys_initcall(),
which doesn't make very much sense. It is obviously not necessary
to do this probe at subsys_initcall() time, right?

Take this opportunity to convert the driver to use
module_platform_driver() as well.

Yours,
Linus Walleij

RE: [PATCH 1/3] gpio: mxc: Support module build

[Anson Huang]

Hi, Linus

> Subject: Re: [PATCH 1/3] gpio: mxc: Support module build
> 
> On Wed, Jul 8, 2020 at 1:28 AM Anson Huang <Anson.Huang@nxp.com>
> wrote:
> 
> >  subsys_initcall(gpio_mxc_init);
> > +
> > +MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>");
> > +MODULE_DESCRIPTION("i.MX GPIO Driver"); MODULE_LICENSE("GPL");
> 
> You are making this modualrizable but keeping the subsys_initcall(), which
> doesn't make very much sense. It is obviously not necessary to do this probe
> at subsys_initcall() time, right?
>

If building it as module, the subsys_initcall() will be equal to module_init(), I keep
it unchanged is because I try to make it identical when built-in, since most of the
config will still have it built-in, except the Android GKI support. Does it make sense?
 
> Take this opportunity to convert the driver to use
> module_platform_driver() as well.

If you think it has to be or it is better to use module_platform_driver(), I will do
it in V2.

thanks,
Anson

RE: [PATCH 1/3] gpio: mxc: Support module build

[Anson Huang]

Hi, Linus

> Subject: RE: [PATCH 1/3] gpio: mxc: Support module build
> 
> Hi, Linus
> 
> > Subject: Re: [PATCH 1/3] gpio: mxc: Support module build
> >
> > On Wed, Jul 8, 2020 at 1:28 AM Anson Huang <Anson.Huang@nxp.com>
> > wrote:
> >
> > >  subsys_initcall(gpio_mxc_init);
> > > +
> > > +MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>");
> > > +MODULE_DESCRIPTION("i.MX GPIO Driver"); MODULE_LICENSE("GPL");
> >
> > You are making this modualrizable but keeping the subsys_initcall(),
> > which doesn't make very much sense. It is obviously not necessary to
> > do this probe at subsys_initcall() time, right?
> >
> 
> If building it as module, the subsys_initcall() will be equal to module_init(), I
> keep it unchanged is because I try to make it identical when built-in, since
> most of the config will still have it built-in, except the Android GKI support.
> Does it make sense?
> 
> > Take this opportunity to convert the driver to use
> > module_platform_driver() as well.
> 
> If you think it has to be or it is better to use module_platform_driver(), I will do
> it in V2.

I tried to replace the subsys_initcall() with module_platform_driver(), but met issue
about " register_syscore_ops(&mxc_gpio_syscore_ops);" which is called in gpio_mxc_init()
function, this function should be called ONLY once, moving it to .probe function is NOT
working, so we may need to keep the gpio_mxc_init(), that is another reason that we may
need to keep subsys_initcall()?

Thanks,
Anson

Re: [PATCH 1/3] gpio: mxc: Support module build

[Linus Walleij]

On Wed, Jul 15, 2020 at 4:44 AM Anson Huang <anson.huang@nxp.com> wrote:

> > Subject: RE: [PATCH 1/3] gpio: mxc: Support module build
> >
> > Hi, Linus
> >
> > > Subject: Re: [PATCH 1/3] gpio: mxc: Support module build
> > >
> > > On Wed, Jul 8, 2020 at 1:28 AM Anson Huang <Anson.Huang@nxp.com>
> > > wrote:
> > >
> > > >  subsys_initcall(gpio_mxc_init);
> > > > +
> > > > +MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>");
> > > > +MODULE_DESCRIPTION("i.MX GPIO Driver"); MODULE_LICENSE("GPL");
> > >
> > > You are making this modualrizable but keeping the subsys_initcall(),
> > > which doesn't make very much sense. It is obviously not necessary to
> > > do this probe at subsys_initcall() time, right?
> > >
> >
> > If building it as module, the subsys_initcall() will be equal to module_init(), I
> > keep it unchanged is because I try to make it identical when built-in, since
> > most of the config will still have it built-in, except the Android GKI support.
> > Does it make sense?
> >
> > > Take this opportunity to convert the driver to use
> > > module_platform_driver() as well.
> >
> > If you think it has to be or it is better to use module_platform_driver(), I will do
> > it in V2.
>
> I tried to replace the subsys_initcall() with module_platform_driver(), but met issue
> about " register_syscore_ops(&mxc_gpio_syscore_ops);" which is called in gpio_mxc_init()
> function, this function should be called ONLY once, moving it to .probe function is NOT
> working, so we may need to keep the gpio_mxc_init(), that is another reason that we may
> need to keep subsys_initcall()?

This looks a bit dangerous to keep like this while allowing this
code to be used from a module.

What happens if you insmod and rmmod this a few times, really?
How is this tested?

This is not really modularized if that isn't working, just that modprobing
once works isn't real modularization IMO, it seems more like a
quick and dirty way to get Androids GKI somewhat working with the module
while not properly making the module a module.

You need input from the driver maintainers on how to handle this.

Yours,
Linus Walleij

RE: [PATCH 1/3] gpio: mxc: Support module build

[Anson Huang]

Hi, Linus

> Subject: Re: [PATCH 1/3] gpio: mxc: Support module build
> 
> On Wed, Jul 15, 2020 at 4:44 AM Anson Huang <anson.huang@nxp.com>
> wrote:
> 
> > > Subject: RE: [PATCH 1/3] gpio: mxc: Support module build
> > >
> > > Hi, Linus
> > >
> > > > Subject: Re: [PATCH 1/3] gpio: mxc: Support module build
> > > >
> > > > On Wed, Jul 8, 2020 at 1:28 AM Anson Huang <Anson.Huang@nxp.com>
> > > > wrote:
> > > >
> > > > >  subsys_initcall(gpio_mxc_init);
> > > > > +
> > > > > +MODULE_AUTHOR("Shawn Guo <shawn.guo@linaro.org>");
> > > > > +MODULE_DESCRIPTION("i.MX GPIO Driver");
> MODULE_LICENSE("GPL");
> > > >
> > > > You are making this modualrizable but keeping the
> > > > subsys_initcall(), which doesn't make very much sense. It is
> > > > obviously not necessary to do this probe at subsys_initcall() time, right?
> > > >
> > >
> > > If building it as module, the subsys_initcall() will be equal to
> > > module_init(), I keep it unchanged is because I try to make it
> > > identical when built-in, since most of the config will still have it built-in,
> except the Android GKI support.
> > > Does it make sense?
> > >
> > > > Take this opportunity to convert the driver to use
> > > > module_platform_driver() as well.
> > >
> > > If you think it has to be or it is better to use
> > > module_platform_driver(), I will do it in V2.
> >
> > I tried to replace the subsys_initcall() with
> > module_platform_driver(), but met issue about "
> > register_syscore_ops(&mxc_gpio_syscore_ops);" which is called in
> > gpio_mxc_init() function, this function should be called ONLY once,
> > moving it to .probe function is NOT working, so we may need to keep the
> gpio_mxc_init(), that is another reason that we may need to keep
> subsys_initcall()?
> 
> This looks a bit dangerous to keep like this while allowing this code to be used
> from a module.
> 
> What happens if you insmod and rmmod this a few times, really?
> How is this tested?
> 
> This is not really modularized if that isn't working, just that modprobing once
> works isn't real modularization IMO, it seems more like a quick and dirty way
> to get Androids GKI somewhat working with the module while not properly
> making the module a module.
> 
> You need input from the driver maintainers on how to handle this.

As far as I know, some general/critical modules are NOT supporting rmmod, like
clk, pinctrl, gpio etc., and I am NOT sure whether Android GKI need to support
rmmod for these system-wide-used module, I will ask them for more detail about
this.

The requirement I received is to support loadable module, but so far no hard requirement
to support module remove for gpio driver, so, is it OK to add it step by step, and this patch
series ONLY to support module build and one time modprobe?  

Thanks,
Anson

Re: [PATCH 1/3] gpio: mxc: Support module build

[Linus Walleij]

Greg, John,

we need some guidance here. See below.

On Thu, Jul 16, 2020 at 4:38 PM Anson Huang <anson.huang@nxp.com> wrote:
> [Me]
> > On Wed, Jul 15, 2020 at 4:44 AM Anson Huang <anson.huang@nxp.com>

> > > I tried to replace the subsys_initcall() with
> > > module_platform_driver(), but met issue about "
> > > register_syscore_ops(&mxc_gpio_syscore_ops);" which is called in
> > > gpio_mxc_init() function, this function should be called ONLY once,
> > > moving it to .probe function is NOT working, so we may need to keep the
> > > gpio_mxc_init(), that is another reason that we may need to keep
> > > subsys_initcall()?
> >
> > This looks a bit dangerous to keep like this while allowing this code to be used
> > from a module.
> >
> > What happens if you insmod and rmmod this a few times, really?
> > How is this tested?
> >
> > This is not really modularized if that isn't working, just that modprobing once
> > works isn't real modularization IMO, it seems more like a quick and dirty way
> > to get Androids GKI somewhat working with the module while not properly
> > making the module a module.
> >
> > You need input from the driver maintainers on how to handle this.
>
> As far as I know, some general/critical modules are NOT supporting rmmod, like
> clk, pinctrl, gpio etc., and I am NOT sure whether Android GKI need to support
> rmmod for these system-wide-used module, I will ask them for more detail about
> this.
>
> The requirement I received is to support loadable module, but so far no hard requirement
> to support module remove for gpio driver, so, is it OK to add it step by step, and this patch
> series ONLY to support module build and one time modprobe?

While I am a big fan of the Android GKI initiative this needs to be aligned
with the Linux core maintainers, so let's ask Greg. I am also paging
John Stultz on this: he is close to this action.

They both know the Android people very well.

So there is a rationale like this going on: in order to achieve GKI goals
and have as much as possible of the Linux kernel stashed into loadable
kernel modules, it has been elevated to modus operandi amongst
the developers pushing this change that it is OK to pile up a load of
modules that cannot ever be unloaded.

This is IIUC regardless of whether all consumers of the module are
actually gone: it would be OK to say make it impossible to rmmod
a clk driver even of zero clocks from that driver is in use. So it is not
dependency-graph problem, it is a "load once, never remove" approach.

This rationale puts me as subsystem maintainer in an unpleasant spot:
it is really hard to tell case-to-case whether that change really is a
technical advantage for the kernel per se or whether it is done for the
greater ecosystem of Android.

Often I would say it makes it possible to build a smaller kernel vmlinux
so OK that is an advantage. On the other hand I have an inkling that I
should be pushing developers to make sure that rmmod works.

As a minimum requirement I would expect this to be marked by

struct device_driver {
   (...)
    /* This module absolutely cannot be unbound */
   .suppress_bind_attrs = true;
};

So that noone would be able to try to unbind this (could even be an
attack vector!)

What is our broader reasoning when it comes to this? (I might have
missed some mail thread here.)

Yours,
Linus Walleij

Re: [PATCH 1/3] gpio: mxc: Support module build

[Greg KH]

On Fri, Jul 17, 2020 at 02:01:16PM +0200, Linus Walleij wrote:
> Greg, John,
> 
> we need some guidance here. See below.
> 
> On Thu, Jul 16, 2020 at 4:38 PM Anson Huang <anson.huang@nxp.com> wrote:
> > [Me]
> > > On Wed, Jul 15, 2020 at 4:44 AM Anson Huang <anson.huang@nxp.com>
> 
> > > > I tried to replace the subsys_initcall() with
> > > > module_platform_driver(), but met issue about "
> > > > register_syscore_ops(&mxc_gpio_syscore_ops);" which is called in
> > > > gpio_mxc_init() function, this function should be called ONLY once,
> > > > moving it to .probe function is NOT working, so we may need to keep the
> > > > gpio_mxc_init(), that is another reason that we may need to keep
> > > > subsys_initcall()?
> > >
> > > This looks a bit dangerous to keep like this while allowing this code to be used
> > > from a module.
> > >
> > > What happens if you insmod and rmmod this a few times, really?
> > > How is this tested?
> > >
> > > This is not really modularized if that isn't working, just that modprobing once
> > > works isn't real modularization IMO, it seems more like a quick and dirty way
> > > to get Androids GKI somewhat working with the module while not properly
> > > making the module a module.
> > >
> > > You need input from the driver maintainers on how to handle this.
> >
> > As far as I know, some general/critical modules are NOT supporting rmmod, like
> > clk, pinctrl, gpio etc., and I am NOT sure whether Android GKI need to support
> > rmmod for these system-wide-used module, I will ask them for more detail about
> > this.
> >
> > The requirement I received is to support loadable module, but so far no hard requirement
> > to support module remove for gpio driver, so, is it OK to add it step by step, and this patch
> > series ONLY to support module build and one time modprobe?
> 
> While I am a big fan of the Android GKI initiative this needs to be aligned
> with the Linux core maintainers, so let's ask Greg. I am also paging
> John Stultz on this: he is close to this action.
> 
> They both know the Android people very well.
> 
> So there is a rationale like this going on: in order to achieve GKI goals
> and have as much as possible of the Linux kernel stashed into loadable
> kernel modules, it has been elevated to modus operandi amongst
> the developers pushing this change that it is OK to pile up a load of
> modules that cannot ever be unloaded.

Why can't the module be unloaded?  Is it just because they never
implement the proper "remove all resources allocated" logic in a remove
function, or something else?

> This is IIUC regardless of whether all consumers of the module are
> actually gone: it would be OK to say make it impossible to rmmod
> a clk driver even of zero clocks from that driver is in use. So it is not
> dependency-graph problem, it is a "load once, never remove" approach.

Sounds like a "lazy" approach :)

> This rationale puts me as subsystem maintainer in an unpleasant spot:
> it is really hard to tell case-to-case whether that change really is a
> technical advantage for the kernel per se or whether it is done for the
> greater ecosystem of Android.
> 
> Often I would say it makes it possible to build a smaller kernel vmlinux
> so OK that is an advantage. On the other hand I have an inkling that I
> should be pushing developers to make sure that rmmod works.

I can see where a number of modules just can not ever be removed because
of resources and not being able to properly tear down, but that doesn't
mean that a driver author shouldn't at least try, right?

> As a minimum requirement I would expect this to be marked by
> 
> struct device_driver {
>    (...)
>     /* This module absolutely cannot be unbound */
>    .suppress_bind_attrs = true;
> };

No, that's not what bind/unbind is really for.  That's a per-subsystem
choice as to if you want to allow devices to be added/removed from
drivers at runtime.  It has nothing to do with module load/unload.

> So that noone would be able to try to unbind this (could even be an
> attack vector!)
> 
> What is our broader reasoning when it comes to this? (I might have
> missed some mail thread here.)

Android is just finally pushing vendors to get their code upstream,
which is a good thing to see.  And building things as a module is an
even better thing as now it is finally allowing arm64 systems to be
built to support more than one specific hardware platform at runtime.

So moving drivers to modules is good.  If a module can be removed, even
better, but developers should not be lazy and just flat out not try at
all to make their code unloadable if at all possible.

Does that help?

thanks,

greg k-h

Re: [PATCH 1/3] gpio: mxc: Support module build

[Geert Uytterhoeven]

Hi Greg,

On Fri, Jul 17, 2020 at 2:14 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> Android is just finally pushing vendors to get their code upstream,
> which is a good thing to see.  And building things as a module is an
> even better thing as now it is finally allowing arm64 systems to be
> built to support more than one specific hardware platform at runtime.

Can you please stop spreading this FUD?

As I said before, Arm64 kernels have supported more than one specific
hardware platform at runtime from the beginning of the arm64 port
(assumed the needed platform support has been enabled in the kernel
 config, of course).
Even most arm32 kernels support this, since the introduction of the
CONFIG_ARCH_MULTIPLATFORM option.  In fact every recently
introduced arm32 platform is usually v7, and must conform to this.
The sole exceptions are very old platforms, and the v4/v5/v6/v7 split
due to architectural issues (the latter still support clusters of
platforms in a single kernel).

Thank you, and have a nice weekend!

Gr{oetje,eeting}s,

                        Geert

-- 
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/3] gpio: mxc: Support module build

[Greg KH]

On Fri, Jul 17, 2020 at 02:27:43PM +0200, Geert Uytterhoeven wrote:
> Hi Greg,
> 
> On Fri, Jul 17, 2020 at 2:14 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > Android is just finally pushing vendors to get their code upstream,
> > which is a good thing to see.  And building things as a module is an
> > even better thing as now it is finally allowing arm64 systems to be
> > built to support more than one specific hardware platform at runtime.
> 
> Can you please stop spreading this FUD?

For many many SoCs today, this is not true.  Their drivers are required
to be built in and will not work as modules, as we are seeing the
patches try to fix.

> As I said before, Arm64 kernels have supported more than one specific
> hardware platform at runtime from the beginning of the arm64 port
> (assumed the needed platform support has been enabled in the kernel
>  config, of course).
> Even most arm32 kernels support this, since the introduction of the
> CONFIG_ARCH_MULTIPLATFORM option.  In fact every recently
> introduced arm32 platform is usually v7, and must conform to this.
> The sole exceptions are very old platforms, and the v4/v5/v6/v7 split
> due to architectural issues (the latter still support clusters of
> platforms in a single kernel).

I think the confusion here is that this really does not work well, if at
all, on most "high end" SoC chips due to the collection of different
things all of the vendors ship to their customers.  This is the work
that is trying to be fixed up here.

And look at the driver core work for many driver subsystems to be fixed
up just to get a single kernel image to work on multiple platforms.
Just because older ones did it, doesn't mean it actually works today :)

thanks,

greg k-h

Re: [PATCH 1/3] gpio: mxc: Support module build

[Arnd Bergmann]

On Fri, Jul 17, 2020 at 3:21 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Jul 17, 2020 at 02:27:43PM +0200, Geert Uytterhoeven wrote:
> > Hi Greg,
> >
> > On Fri, Jul 17, 2020 at 2:14 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > > Android is just finally pushing vendors to get their code upstream,
> > > which is a good thing to see.  And building things as a module is an
> > > even better thing as now it is finally allowing arm64 systems to be
> > > built to support more than one specific hardware platform at runtime.
> >
> > Can you please stop spreading this FUD?
>
> For many many SoCs today, this is not true.  Their drivers are required
> to be built in and will not work as modules, as we are seeing the
> patches try to fix.

There are two different points here:

a) having drivers as loadable modules: I think everyone agrees this
is a good thing in general. Having more of them makes smaller kernels,
which is good. arm64 is no different from arm32 and powerpc here,
and probably a bit better than x86, which requires all platform specific
core code (PC, numachip, UV, ScaleMP, ...) to be built-in.

b) supporting multiple hardware platforms at runtime: this is totally
unrelated to the platform specific drivers being loadable modules.
arm64 is a little better here than arm32 and powerpc, which need more
than one configuration to support all hardware, about the same as
x86 or s390 and much better than most others that have to chose
a machine at compile time.

> > As I said before, Arm64 kernels have supported more than one specific
> > hardware platform at runtime from the beginning of the arm64 port
> > (assumed the needed platform support has been enabled in the kernel
> >  config, of course).
> > Even most arm32 kernels support this, since the introduction of the
> > CONFIG_ARCH_MULTIPLATFORM option.  In fact every recently
> > introduced arm32 platform is usually v7, and must conform to this.
> > The sole exceptions are very old platforms, and the v4/v5/v6/v7 split
> > due to architectural issues (the latter still support clusters of
> > platforms in a single kernel).
>
> I think the confusion here is that this really does not work well, if at
> all, on most "high end" SoC chips due to the collection of different
> things all of the vendors ship to their customers.  This is the work
> that is trying to be fixed up here.
>
> And look at the driver core work for many driver subsystems to be fixed
> up just to get a single kernel image to work on multiple platforms.
> Just because older ones did it, doesn't mean it actually works today :)

Can you give a specific example? The only problem I'm aware of for
those SoCs is drivers being outside of the mainline kernel. Clearly
having support for loadable modules helps SoC vendors because it
allows them to support a new platform with an existing binary kernel
by shipping third-party driver modules, but for stuff that is already
in mainline, we could in theory support all hardware in a single gigantic
binary kernel with no support for loadable modules at all.

      Arnd

Re: [PATCH 1/3] gpio: mxc: Support module build

[Greg KH]

On Fri, Jul 17, 2020 at 03:54:49PM +0200, Arnd Bergmann wrote:
> > And look at the driver core work for many driver subsystems to be fixed
> > up just to get a single kernel image to work on multiple platforms.
> > Just because older ones did it, doesn't mean it actually works today :)
> 
> Can you give a specific example? The only problem I'm aware of for
> those SoCs is drivers being outside of the mainline kernel. Clearly
> having support for loadable modules helps SoC vendors because it
> allows them to support a new platform with an existing binary kernel
> by shipping third-party driver modules, but for stuff that is already
> in mainline, we could in theory support all hardware in a single gigantic
> binary kernel with no support for loadable modules at all.

That did not work for many drivers for some reason, look at all the work
Saravana had to do in the driver core and device tree code for it to
happen correctly over the past year.

thanks,

greg k-h

Re: [PATCH 1/3] gpio: mxc: Support module build

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1646 bytes --]

On Fri, Jul 17, 2020 at 04:13:44PM +0200, Greg KH wrote:
> On Fri, Jul 17, 2020 at 03:54:49PM +0200, Arnd Bergmann wrote:

> > > And look at the driver core work for many driver subsystems to be fixed
> > > up just to get a single kernel image to work on multiple platforms.
> > > Just because older ones did it, doesn't mean it actually works today :)

> > Can you give a specific example? The only problem I'm aware of for
> > those SoCs is drivers being outside of the mainline kernel. Clearly
> > having support for loadable modules helps SoC vendors because it
> > allows them to support a new platform with an existing binary kernel
> > by shipping third-party driver modules, but for stuff that is already
> > in mainline, we could in theory support all hardware in a single gigantic
> > binary kernel with no support for loadable modules at all.

> That did not work for many drivers for some reason, look at all the work
> Saravana had to do in the driver core and device tree code for it to
> happen correctly over the past year.

Could you be more specific about these issues?  I'm aware of his work
around probe ordering but that's not at all arch specific, the same
issues affect every architecture, so doesn't seem to be what you're
talking about.

arm64 has never supported anything other than a multiplatform kernel,
and the actively maintained 32 bit platforms have supported one for more
than half a decade at this point.  CI systems keep managing to test
these kernels, distributions seem to keep managing to ship them and
users appear able to install and use them so it doesn't seem quite so
fundamentally broken as all that.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 1/3] gpio: mxc: Support module build

[Arnd Bergmann]

On Fri, Jul 17, 2020 at 2:16 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> On Fri, Jul 17, 2020 at 02:01:16PM +0200, Linus Walleij wrote:
> > While I am a big fan of the Android GKI initiative this needs to be aligned
> > with the Linux core maintainers, so let's ask Greg. I am also paging
> > John Stultz on this: he is close to this action.
> >
> > They both know the Android people very well.
> >
> > So there is a rationale like this going on: in order to achieve GKI goals
> > and have as much as possible of the Linux kernel stashed into loadable
> > kernel modules, it has been elevated to modus operandi amongst
> > the developers pushing this change that it is OK to pile up a load of
> > modules that cannot ever be unloaded.
>
> Why can't the module be unloaded?  Is it just because they never
> implement the proper "remove all resources allocated" logic in a remove
> function, or something else?

For the core kernel parts, it's usually for the lack of tracking of who
is using the resource provided by the driver, as the subsystems tend
to be written around x86's "everything is built-in" model.

For instance, a PCIe host bridge might rely on the IOMMU, a
clock controller, an interrupt controller, a pin controller and a reset
controller. The host bridge can still be probed at reduced functionality
if some of these are missing, or it can use deferred probing when
some others are missing at probe time.

If we want all of drivers to be unloaded again, we need to do one
of two things:

a) track dependencies, so that removing one of the devices
    underneath leads to everything depending on it to get removed
    as well or will be notified about it going away and can stop using
    it. This is the model used in the network subsystem, where
    any ethernet driver can be unloaded and everything using the
    device gets torn down.

b) use reference counting on the device or (at the minimum)
    try_module_get()/module_put() calls for all such resources
    so as long as the pci host bridge is there, so none of the devices
    it uses will go away when they are still used.

Traditionally, we would have considered the PCIe host bridge to
be a fundamental part of the system, implying that everything it
uses is also fundamental, and there was no need to track
usage at all, just to ensure the probing is done in the right order.

> > As a minimum requirement I would expect this to be marked by
> >
> > struct device_driver {
> >    (...)
> >     /* This module absolutely cannot be unbound */
> >    .suppress_bind_attrs = true;
> > };
>
> No, that's not what bind/unbind is really for.  That's a per-subsystem
> choice as to if you want to allow devices to be added/removed from
> drivers at runtime.  It has nothing to do with module load/unload.

It's a one-way dependency: If we can't allow the device to be
unbound, then we also should not allow module unloading because
that forces an unbind.

      Arnd

Re: [PATCH 1/3] gpio: mxc: Support module build

[Greg KH]

On Fri, Jul 17, 2020 at 03:02:54PM +0200, Arnd Bergmann wrote:
> On Fri, Jul 17, 2020 at 2:16 PM Greg KH <gregkh@linuxfoundation.org> wrote:
> > On Fri, Jul 17, 2020 at 02:01:16PM +0200, Linus Walleij wrote:
> > > While I am a big fan of the Android GKI initiative this needs to be aligned
> > > with the Linux core maintainers, so let's ask Greg. I am also paging
> > > John Stultz on this: he is close to this action.
> > >
> > > They both know the Android people very well.
> > >
> > > So there is a rationale like this going on: in order to achieve GKI goals
> > > and have as much as possible of the Linux kernel stashed into loadable
> > > kernel modules, it has been elevated to modus operandi amongst
> > > the developers pushing this change that it is OK to pile up a load of
> > > modules that cannot ever be unloaded.
> >
> > Why can't the module be unloaded?  Is it just because they never
> > implement the proper "remove all resources allocated" logic in a remove
> > function, or something else?
> 
> For the core kernel parts, it's usually for the lack of tracking of who
> is using the resource provided by the driver, as the subsystems tend
> to be written around x86's "everything is built-in" model.
> 
> For instance, a PCIe host bridge might rely on the IOMMU, a
> clock controller, an interrupt controller, a pin controller and a reset
> controller. The host bridge can still be probed at reduced functionality
> if some of these are missing, or it can use deferred probing when
> some others are missing at probe time.
> 
> If we want all of drivers to be unloaded again, we need to do one
> of two things:
> 
> a) track dependencies, so that removing one of the devices
>     underneath leads to everything depending on it to get removed
>     as well or will be notified about it going away and can stop using
>     it. This is the model used in the network subsystem, where
>     any ethernet driver can be unloaded and everything using the
>     device gets torn down.
> 
> b) use reference counting on the device or (at the minimum)
>     try_module_get()/module_put() calls for all such resources
>     so as long as the pci host bridge is there, so none of the devices
>     it uses will go away when they are still used.
> 
> Traditionally, we would have considered the PCIe host bridge to
> be a fundamental part of the system, implying that everything it
> uses is also fundamental, and there was no need to track
> usage at all, just to ensure the probing is done in the right order.

Yeah, ick, for IOMMU and stuff like this, no, load it once and never
unload it makes much more sense.

Just know how to dynamically load the specific driver out of a
collection of them, and all should be fine.

> > > As a minimum requirement I would expect this to be marked by
> > >
> > > struct device_driver {
> > >    (...)
> > >     /* This module absolutely cannot be unbound */
> > >    .suppress_bind_attrs = true;
> > > };
> >
> > No, that's not what bind/unbind is really for.  That's a per-subsystem
> > choice as to if you want to allow devices to be added/removed from
> > drivers at runtime.  It has nothing to do with module load/unload.
> 
> It's a one-way dependency: If we can't allow the device to be
> unbound, then we also should not allow module unloading because
> that forces an unbind.

Ok, then turn that off for the subsystems this does not support, no
objection from me.  It's just a fun hack that people use for testing out
drivers on new devices, and for virtual devices.

thanks,

greg k-h

Re: [PATCH 1/3] gpio: mxc: Support module build

[Linus Walleij]

On Fri, Jul 17, 2020 at 2:14 PM Greg KH <gregkh@linuxfoundation.org> wrote:

> So moving drivers to modules is good.  If a module can be removed, even
> better, but developers should not be lazy and just flat out not try at
> all to make their code unloadable if at all possible.
>
> Does that help?

Yeah it confirms my intuitive maintenance approach: developer submits
modularization patch, I will be a bit inquisitive and "can't you attempt
to make this thing unload too" and if they conclude that that is
an unfathomable effort I will likely merge it anyway as very likely
the kernel looks better after than before provided all build and test
coverage stays the same as well.

Thanks!
Linus Walleij

Re: [PATCH 1/3] gpio: mxc: Support module build

[John Stultz]

On Fri, Jul 17, 2020 at 5:01 AM Linus Walleij <linus.walleij@linaro.org> wrote:
>
> Greg, John,
>
> we need some guidance here. See below.
>
> On Thu, Jul 16, 2020 at 4:38 PM Anson Huang <anson.huang@nxp.com> wrote:
> > [Me]
> > > On Wed, Jul 15, 2020 at 4:44 AM Anson Huang <anson.huang@nxp.com>
>
> > > > I tried to replace the subsys_initcall() with
> > > > module_platform_driver(), but met issue about "
> > > > register_syscore_ops(&mxc_gpio_syscore_ops);" which is called in
> > > > gpio_mxc_init() function, this function should be called ONLY once,
> > > > moving it to .probe function is NOT working, so we may need to keep the
> > > > gpio_mxc_init(), that is another reason that we may need to keep
> > > > subsys_initcall()?
> > >
> > > This looks a bit dangerous to keep like this while allowing this code to be used
> > > from a module.
> > >
> > > What happens if you insmod and rmmod this a few times, really?
> > > How is this tested?
> > >
> > > This is not really modularized if that isn't working, just that modprobing once
> > > works isn't real modularization IMO, it seems more like a quick and dirty way
> > > to get Androids GKI somewhat working with the module while not properly
> > > making the module a module.
> > >
> > > You need input from the driver maintainers on how to handle this.
> >
> > As far as I know, some general/critical modules are NOT supporting rmmod, like
> > clk, pinctrl, gpio etc., and I am NOT sure whether Android GKI need to support
> > rmmod for these system-wide-used module, I will ask them for more detail about
> > this.
> >
> > The requirement I received is to support loadable module, but so far no hard requirement
> > to support module remove for gpio driver, so, is it OK to add it step by step, and this patch
> > series ONLY to support module build and one time modprobe?
>
> While I am a big fan of the Android GKI initiative this needs to be aligned
> with the Linux core maintainers, so let's ask Greg. I am also paging
> John Stultz on this: he is close to this action.
>
> They both know the Android people very well.
>
> So there is a rationale like this going on: in order to achieve GKI goals
> and have as much as possible of the Linux kernel stashed into loadable
> kernel modules, it has been elevated to modus operandi amongst
> the developers pushing this change that it is OK to pile up a load of
> modules that cannot ever be unloaded.
>
> This is IIUC regardless of whether all consumers of the module are
> actually gone: it would be OK to say make it impossible to rmmod
> a clk driver even of zero clocks from that driver is in use. So it is not
> dependency-graph problem, it is a "load once, never remove" approach.
>
> This rationale puts me as subsystem maintainer in an unpleasant spot:
> it is really hard to tell case-to-case whether that change really is a
> technical advantage for the kernel per se or whether it is done for the
> greater ecosystem of Android.
>
> Often I would say it makes it possible to build a smaller kernel vmlinux
> so OK that is an advantage. On the other hand I have an inkling that I
> should be pushing developers to make sure that rmmod works.
>
> As a minimum requirement I would expect this to be marked by
>
> struct device_driver {
>    (...)
>     /* This module absolutely cannot be unbound */
>    .suppress_bind_attrs = true;
> };
>
> So that noone would be able to try to unbind this (could even be an
> attack vector!)
>
> What is our broader reasoning when it comes to this? (I might have
> missed some mail thread here.)

Sorry for being a little late here, was out for a few days.

So yea, wrt to some of the Android GKI related efforts I've been
involved with, loading once and not unloading is fine for the usage
model.

I can understand it being a bit ugly compared to drivers with proper
unloading support, and I think for most new driver submissions,
maintainers can reasonably push to see proper unloading being
implemented.

But there are some pragmatic cases with low-level drivers (as you
mentioned: clk, pinctrl, gpio, etc) where sorting out the unloading is
particularly complicated, or there is some missing infrastructure, and
in those cases being able to load a "permanent" module seems to me
like a clear benefit.  After all, it seems a bit strange to enforce
that drivers be unloadable when the same code was considered ok to be
merged as a built-in.

So I think there's a reasonable case for the preference order to be:
"built-in" < "permanent module" < "unloadable module".

And of course, it can be more complicated, as enabling a driver to be
a module can have rippling effects on other code that may call into
it. But I think maintainers have the best sense of how to draw the
line there.

thanks
-john