| Message ID | 1594164323-14920-1-git-send-email-Anson.Huang@nxp.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] gpio: mxc: Support module build | expand |
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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.
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
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");
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(-)