[PATCH 0/5] drivers/tty: make more bool drivers explicitly non-modular

[PATCH 0/5] drivers/tty: make more bool drivers explicitly non-modular

[Paul Gortmaker]

This second set of patches to drivers/tty steps outside of the serial
dir, and an improved auditing finds two more serial drivers pretending
to be modular that really are not.

The reasoning for doing this is the same as the first set[1] of patches
and is largely copied below:

  In the previous merge window, we made changes to allow better
  delineation between modular and non-modular code in commit
  0fd972a7d91d6e15393c449492a04d94c0b89351 ("module: relocate module_init
  from init.h to module.h").  This allows us to now ensure module code
  looks modular and non-modular code does not accidentally look modular
  without suffering build breakage.
  
  Here we target code that is, by nature of their Kconfig settings, only
  available to be built-in, but implicitly presenting itself as being
  possibly modular by way of using modular headers, macros, and functions.
  
  The goal here is to remove that illusion of modularity from these
  drivers, but in a way that leaves the actual runtime unchanged.
  In doing so, we remove code that has never been tested and adds
  no value to the tree.  And we begin the process of expecting a
  level of consistency between the Kconfig of a driver and the code
  that the driver uses.
  
Build tested for allyesconfig on x86_64, and ARM for lpc81xx, and powerpc
for hvc_console and mpsc, layered onto tty/tty-next as a baseline.

Paul.

[1] https://lkml.kernel.org/r/1437530538-5078-1-git-send-email-paul.gortmaker@windriver.com
--

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Joachim Eastwood <manabian@gmail.com>
Cc: linux-arm-kernel@lists.infradead.org
Cc: linuxppc-dev@lists.ozlabs.org
Cc: linux-serial@vger.kernel.org

Paul Gortmaker (5):
  drivers/tty: make pty.c slightly more explicitly non-modular
  drivers/tty: make sysrq.c slightly more explicitly non-modular
  drivers/tty: make hvc_console.c explicitly non-modular
  drivers/tty: make serial/mpsc.c driver explicitly non-modular
  drivers/tty: make serial 8250_lpc18xx.c explicitly non-modular

 drivers/tty/hvc/hvc_console.c          | 18 +----------------
 drivers/tty/pty.c                      |  7 +++++--
 drivers/tty/serial/8250/8250_lpc18xx.c | 19 ++++--------------
 drivers/tty/serial/mpsc.c              | 36 +++-------------------------------
 drivers/tty/sysrq.c                    |  6 +++++-
 5 files changed, 18 insertions(+), 68 deletions(-)

-- 
2.5.0

[PATCH 4/5] drivers/tty: make serial/mpsc.c driver explicitly non-modular

[Paul Gortmaker]

The Kconfig for this driver is currently:

config SERIAL_MPSC
        bool "Marvell MPSC serial port support"

...meaning that it currently is not being built as a module by anyone.
Lets remove the modular code that is essentially orphaned, so that
when reading the driver there is no doubt it is builtin-only.

Since module_init translates to device_initcall in the non-modular
case, the init ordering remains unchanged with this commit.

We leave some tags like MODULE_AUTHOR for documentation purposes.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: linux-serial@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/tty/serial/mpsc.c | 36 +++---------------------------------
 1 file changed, 3 insertions(+), 33 deletions(-)

diff --git a/drivers/tty/serial/mpsc.c b/drivers/tty/serial/mpsc.c
index 82bb6d1fe23b..edb32e3f1e84 100644
--- a/drivers/tty/serial/mpsc.c
+++ b/drivers/tty/serial/mpsc.c
@@ -55,8 +55,6 @@
 #define SUPPORT_SYSRQ
 #endif
 
-#include <linux/module.h>
-#include <linux/moduleparam.h>
 #include <linux/tty.h>
 #include <linux/tty_flip.h>
 #include <linux/ioport.h>
@@ -2108,24 +2106,8 @@ static int mpsc_drv_probe(struct platform_device *dev)
 	return rc;
 }
 
-static int mpsc_drv_remove(struct platform_device *dev)
-{
-	pr_debug("mpsc_drv_exit: Removing MPSC %d\n", dev->id);
-
-	if (dev->id < MPSC_NUM_CTLRS) {
-		uart_remove_one_port(&mpsc_reg, &mpsc_ports[dev->id].port);
-		mpsc_release_port((struct uart_port *)
-				&mpsc_ports[dev->id].port);
-		mpsc_drv_unmap_regs(&mpsc_ports[dev->id]);
-		return 0;
-	} else {
-		return -ENODEV;
-	}
-}
-
 static struct platform_driver mpsc_driver = {
 	.probe	= mpsc_drv_probe,
-	.remove	= mpsc_drv_remove,
 	.driver	= {
 		.name	= MPSC_CTLR_NAME,
 	},
@@ -2156,22 +2138,10 @@ static int __init mpsc_drv_init(void)
 
 	return rc;
 }
+device_initcall(mpsc_drv_init);
 
-static void __exit mpsc_drv_exit(void)
-{
-	platform_driver_unregister(&mpsc_driver);
-	platform_driver_unregister(&mpsc_shared_driver);
-	uart_unregister_driver(&mpsc_reg);
-	memset(mpsc_ports, 0, sizeof(mpsc_ports));
-	memset(&mpsc_shared_regs, 0, sizeof(mpsc_shared_regs));
-}
-
-module_init(mpsc_drv_init);
-module_exit(mpsc_drv_exit);
-
+/*
 MODULE_AUTHOR("Mark A. Greer <mgreer@mvista.com>");
 MODULE_DESCRIPTION("Generic Marvell MPSC serial/UART driver");
-MODULE_VERSION(MPSC_VERSION);
 MODULE_LICENSE("GPL");
-MODULE_ALIAS_CHARDEV_MAJOR(MPSC_MAJOR);
-MODULE_ALIAS("platform:" MPSC_CTLR_NAME);
+*/
-- 
2.5.0

[PATCH 5/5] drivers/tty: make serial 8250_lpc18xx.c explicitly non-modular

[Paul Gortmaker]

The Kconfig currently controlling compilation of this code is:

8250/Kconfig:config SERIAL_8250_LPC18XX
8250/Kconfig:        bool "NXP LPC18xx/43xx serial port support"

...meaning that it currently is not being built as a module by anyone.

Lets remove the modular code that is essentially orphaned, so that
when reading the driver there is no doubt it is builtin-only.

Since module_platform_driver() uses the same init level priority as
builtin_platform_driver() the init ordering remains unchanged with
this commit.

Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code.

We leave some tags like MODULE_AUTHOR for documentation purposes.

Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Joachim Eastwood <manabian@gmail.com>
Cc: linux-serial@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/tty/serial/8250/8250_lpc18xx.c | 19 ++++---------------
 1 file changed, 4 insertions(+), 15 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_lpc18xx.c b/drivers/tty/serial/8250/8250_lpc18xx.c
index 99cd478851ff..f5112bee1ab6 100644
--- a/drivers/tty/serial/8250/8250_lpc18xx.c
+++ b/drivers/tty/serial/8250/8250_lpc18xx.c
@@ -15,7 +15,7 @@
 
 #include <linux/clk.h>
 #include <linux/io.h>
-#include <linux/module.h>
+#include <linux/init.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 
@@ -198,33 +198,22 @@ dis_clk_reg:
 	return ret;
 }
 
-static int lpc18xx_serial_remove(struct platform_device *pdev)
-{
-	struct lpc18xx_uart_data *data = platform_get_drvdata(pdev);
-
-	serial8250_unregister_port(data->line);
-	clk_disable_unprepare(data->clk_uart);
-	clk_disable_unprepare(data->clk_reg);
-
-	return 0;
-}
-
 static const struct of_device_id lpc18xx_serial_match[] = {
 	{ .compatible = "nxp,lpc1850-uart" },
 	{ },
 };
-MODULE_DEVICE_TABLE(of, lpc18xx_serial_match);
 
 static struct platform_driver lpc18xx_serial_driver = {
 	.probe  = lpc18xx_serial_probe,
-	.remove = lpc18xx_serial_remove,
 	.driver = {
 		.name = "lpc18xx-uart",
 		.of_match_table = lpc18xx_serial_match,
 	},
 };
-module_platform_driver(lpc18xx_serial_driver);
+builtin_platform_driver(lpc18xx_serial_driver);
 
+/*
 MODULE_AUTHOR("Joachim Eastwood <manabian@gmail.com>");
 MODULE_DESCRIPTION("Serial port driver NXP LPC18xx/43xx devices");
 MODULE_LICENSE("GPL v2");
+*/
-- 
2.5.0

Re: [PATCH 5/5] drivers/tty: make serial 8250_lpc18xx.c explicitly non-modular

[Joachim Eastwood]

Hi Paul,

On 9 August 2015 at 02:51, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
> The Kconfig currently controlling compilation of this code is:
>
> 8250/Kconfig:config SERIAL_8250_LPC18XX
> 8250/Kconfig:        bool "NXP LPC18xx/43xx serial port support"
>
> ...meaning that it currently is not being built as a module by anyone.
>
> Lets remove the modular code that is essentially orphaned, so that
> when reading the driver there is no doubt it is builtin-only.

The bool in KConfig is more or less a copy-paste mistake on my side. I
really meant for this driver to be buildable as module and it builds
just fine with 'm'.

Want me send a patch changing it to tristate or will you handle it?


regards,
Joachim Eastwood

Re: [PATCH 5/5] drivers/tty: make serial 8250_lpc18xx.c explicitly non-modular

[Paul Gortmaker]

[Re: [PATCH 5/5] drivers/tty: make serial 8250_lpc18xx.c explicitly non-modular] On 10/08/2015 (Mon 20:42) Joachim Eastwood wrote:

> Hi Paul,
> 
> On 9 August 2015 at 02:51, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
> > The Kconfig currently controlling compilation of this code is:
> >
> > 8250/Kconfig:config SERIAL_8250_LPC18XX
> > 8250/Kconfig:        bool "NXP LPC18xx/43xx serial port support"
> >
> > ...meaning that it currently is not being built as a module by anyone.
> >
> > Lets remove the modular code that is essentially orphaned, so that
> > when reading the driver there is no doubt it is builtin-only.
> 
> The bool in KConfig is more or less a copy-paste mistake on my side. I
> really meant for this driver to be buildable as module and it builds
> just fine with 'm'.
> 
> Want me send a patch changing it to tristate or will you handle it?

I will do a v2 in the next day or so with collected ACKs and this patch
replaced, unless Greg jumps in and indicates he'd like something
different.

Paul.
--

> 
> 
> regards, Joachim Eastwood

Re: [PATCH 5/5] drivers/tty: make serial 8250_lpc18xx.c explicitly non-modular

[Greg Kroah-Hartman]

On Tue, Aug 11, 2015 at 02:05:44AM -0400, Paul Gortmaker wrote:
> [Re: [PATCH 5/5] drivers/tty: make serial 8250_lpc18xx.c explicitly non-modular] On 10/08/2015 (Mon 20:42) Joachim Eastwood wrote:
> 
> > Hi Paul,
> > 
> > On 9 August 2015 at 02:51, Paul Gortmaker <paul.gortmaker@windriver.com> wrote:
> > > The Kconfig currently controlling compilation of this code is:
> > >
> > > 8250/Kconfig:config SERIAL_8250_LPC18XX
> > > 8250/Kconfig:        bool "NXP LPC18xx/43xx serial port support"
> > >
> > > ...meaning that it currently is not being built as a module by anyone.
> > >
> > > Lets remove the modular code that is essentially orphaned, so that
> > > when reading the driver there is no doubt it is builtin-only.
> > 
> > The bool in KConfig is more or less a copy-paste mistake on my side. I
> > really meant for this driver to be buildable as module and it builds
> > just fine with 'm'.
> > 
> > Want me send a patch changing it to tristate or will you handle it?
> 
> I will do a v2 in the next day or so with collected ACKs and this patch
> replaced, unless Greg jumps in and indicates he'd like something
> different.

Nope, that would be fine with me, thanks.

greg k-h

[PATCH 0/5] drivers/tty: make more bool drivers explicitly non-modular

[Paul Gortmaker]

This next set of patches to drivers/tty fixes up more drivers that would
perhaps appear modular when looking at the code, but are controlled by
a bool Kconfig.  We make sure they don't look modular and get rid
of module related code in the drivers that would never be used.

The one common thread here for all the patches is that we also
scrap the .remove functions which would only be used for module
unload (impossible) and driver unbind.  For the drivers here, there
doesn't seem to be a sensible unbind use case (vs. e.g. a multiport
PCI ethernet driver where one port is unbound and passed through to
a kvm guest or similar).  Hence we just explicitly disallow any
driver unbind operations to help prevent root from doing something
illogical to the machine that they could have done previously.

We've already done this for drivers/tty/serial/mpsc.c previously.

Build tested for allmodconfig on ARM64 and powerpc for tty/tty-testing.

Paul.

---

Cc: Andy Gross <agross-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Cc: Anton Blanchard <anton-eUNUBHrolfbYtjvyW6yDsg@public.gmane.org>
Cc: David Brown <davidb-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Cc: Greg Kroah-Hartman <gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org>
Cc: Jiri Slaby <jslaby-IBi9RG/b67k@public.gmane.org>
Cc: Kumar Gala <galak-sgV2jX0FEOL9JmXXK+q4OQ@public.gmane.org>
Cc: Matthias Brugger <matthias.bgg-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org>
Cc: Michael Ellerman <mpe-Gsx/Oe8HsFggBc27wqDAHg@public.gmane.org>
Cc: Nicolas Ferre <nicolas.ferre-AIFe0yeh4nAAvxtiuMwx3w@public.gmane.org>
Cc: linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Cc: linux-arm-msm-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-mediatek-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org
Cc: linuxppc-dev-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org
Cc: linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
Cc: linux-soc-u79uwXL29TY76Z2rM5mHXA@public.gmane.org

Paul Gortmaker (5):
  drivers/tty: make hvc/hvc_vio.c explicitly non-modular
  drivers/tty: make ehv_bytechan.c explicitly non-modular
  drivers/tty/serial: make serial/atmel_serial.c explicitly non-modular
  drivers/tty/serial: make msm_serial.c explicitly non-modular
  drivers/tty/serial: make 8250/8250_mtk.c explicitly non-modular

 drivers/tty/ehv_bytechan.c         | 40 +++------------------------------
 drivers/tty/hvc/hvc_vio.c          | 29 ++++--------------------
 drivers/tty/serial/8250/8250_mtk.c | 35 ++++++-----------------------
 drivers/tty/serial/atmel_serial.c  | 45 ++++----------------------------------
 drivers/tty/serial/msm_serial.c    | 30 ++++---------------------
 5 files changed, 22 insertions(+), 157 deletions(-)

-- 
2.6.1

Re: [PATCH 0/5] drivers/tty: make more bool drivers explicitly non-modular

[Alexandre Belloni]

On 18/10/2015 at 18:21:13 -0400, Paul Gortmaker wrote :
> The one common thread here for all the patches is that we also
> scrap the .remove functions which would only be used for module
> unload (impossible) and driver unbind.  For the drivers here, there
> doesn't seem to be a sensible unbind use case (vs. e.g. a multiport
> PCI ethernet driver where one port is unbound and passed through to
> a kvm guest or similar).  Hence we just explicitly disallow any
> driver unbind operations to help prevent root from doing something
> illogical to the machine that they could have done previously.
> 
> We've already done this for drivers/tty/serial/mpsc.c previously.
> 
> Build tested for allmodconfig on ARM64 and powerpc for tty/tty-testing.
> 

So, how does this actually build test atmel_serial?

A proper solution would be to actually make it a tristate and allow
building as a module. I think it currently fails because of
console_initcall() but that is certainly fixable.


-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 0/5] drivers/tty: make more bool drivers explicitly non-modular

[Paul Gortmaker]

[Re: [PATCH 0/5] drivers/tty: make more bool drivers explicitly non-modular] On 20/10/2015 (Tue 17:10) Alexandre Belloni wrote:

> On 18/10/2015 at 18:21:13 -0400, Paul Gortmaker wrote :
> > The one common thread here for all the patches is that we also
> > scrap the .remove functions which would only be used for module
> > unload (impossible) and driver unbind.  For the drivers here, there
> > doesn't seem to be a sensible unbind use case (vs. e.g. a multiport
> > PCI ethernet driver where one port is unbound and passed through to
> > a kvm guest or similar).  Hence we just explicitly disallow any
> > driver unbind operations to help prevent root from doing something
> > illogical to the machine that they could have done previously.
> > 
> > We've already done this for drivers/tty/serial/mpsc.c previously.
> > 
> > Build tested for allmodconfig on ARM64 and powerpc for tty/tty-testing.
> > 
> 
> So, how does this actually build test atmel_serial?

Not sure why this should be a surprise;  I build test it exactly like this:

paul@builder-02:~/git/linux-head$ echo $ARCH
arm64
paul@builder-02:~/git/linux-head$ echo $CROSS_COMPILE 
aarch64-linux-gnu-
paul@builder-02:~/git/linux-head$ make O=../arm-build/  drivers/tty/serial/atmel_serial.o
make[1]: Entering directory '/home/paul/git/arm-build'
arch/arm64/Makefile:25: LSE atomics not supported by binutils
  CHK     include/config/kernel.release
  Using /home/paul/git/linux-head as source for kernel
  GEN     ./Makefile
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h

[...]

  HOSTCC  scripts/sign-file
  HOSTCC  scripts/extract-cert
  CC      drivers/tty/serial/atmel_serial.o
make[1]: Leaving directory '/home/paul/git/arm-build'
paul@builder-02:~/git/linux-head$ 

It did build; no warning/error.  Would you call it an invalid build test?

> 
> A proper solution would be to actually make it a tristate and allow
> building as a module. I think it currently fails because of
> console_initcall() but that is certainly fixable.

Well, as per other threads on this topic, if people want to extend
the functionality to support tristate, then great.  But please do
not confuse that with existing functionality which is clearly non
modular in this case.

Thanks,
Paul.
--

> 
> 
> -- 
> Alexandre Belloni, Free Electrons
> Embedded Linux, Kernel and Android engineering
> http://free-electrons.com

Re: [PATCH 0/5] drivers/tty: make more bool drivers explicitly non-modular

[Alexandre Belloni]

On 20/10/2015 at 20:20:07 -0400, Paul Gortmaker wrote :
> [Re: [PATCH 0/5] drivers/tty: make more bool drivers explicitly non-modular] On 20/10/2015 (Tue 17:10) Alexandre Belloni wrote:
> 
> > On 18/10/2015 at 18:21:13 -0400, Paul Gortmaker wrote :
> > > The one common thread here for all the patches is that we also
> > > scrap the .remove functions which would only be used for module
> > > unload (impossible) and driver unbind.  For the drivers here, there
> > > doesn't seem to be a sensible unbind use case (vs. e.g. a multiport
> > > PCI ethernet driver where one port is unbound and passed through to
> > > a kvm guest or similar).  Hence we just explicitly disallow any
> > > driver unbind operations to help prevent root from doing something
> > > illogical to the machine that they could have done previously.
> > > 
> > > We've already done this for drivers/tty/serial/mpsc.c previously.
> > > 
> > > Build tested for allmodconfig on ARM64 and powerpc for tty/tty-testing.
> > > 
> > 
> > So, how does this actually build test atmel_serial?
> 
> Not sure why this should be a surprise;  I build test it exactly like this:
> 

CONFIG_SERIAL_ATMEL is not selected by allmodconfig on arm64 or powerpc
so this is not explaining how you build tested atmel_serial.

> paul@builder-02:~/git/linux-head$ echo $ARCH
> arm64
> paul@builder-02:~/git/linux-head$ echo $CROSS_COMPILE 
> aarch64-linux-gnu-
> paul@builder-02:~/git/linux-head$ make O=../arm-build/  drivers/tty/serial/atmel_serial.o
> make[1]: Entering directory '/home/paul/git/arm-build'
> arch/arm64/Makefile:25: LSE atomics not supported by binutils
>   CHK     include/config/kernel.release
>   Using /home/paul/git/linux-head as source for kernel
>   GEN     ./Makefile
>   CHK     include/generated/uapi/linux/version.h
>   CHK     include/generated/utsrelease.h
> 
> [...]
> 
>   HOSTCC  scripts/sign-file
>   HOSTCC  scripts/extract-cert
>   CC      drivers/tty/serial/atmel_serial.o
> make[1]: Leaving directory '/home/paul/git/arm-build'
> paul@builder-02:~/git/linux-head$ 
> 
> It did build; no warning/error.  Would you call it an invalid build test?
> 

What you describe is a different test. I end up with 4 warnings when
doing that on my machine.

-- 
Alexandre Belloni, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

Re: [PATCH 0/5] drivers/tty: make more bool drivers explicitly non-modular

[Paul Gortmaker]

[[PATCH 0/5] drivers/tty: make more bool drivers explicitly non-modular] On 18/10/2015 (Sun 18:21) Paul Gortmaker wrote:

> This next set of patches to drivers/tty fixes up more drivers that would
> perhaps appear modular when looking at the code, but are controlled by
> a bool Kconfig.  We make sure they don't look modular and get rid
> of module related code in the drivers that would never be used.

[...]

> Paul Gortmaker (5):
>   drivers/tty: make hvc/hvc_vio.c explicitly non-modular
>   drivers/tty: make ehv_bytechan.c explicitly non-modular
>   drivers/tty/serial: make serial/atmel_serial.c explicitly non-modular
>   drivers/tty/serial: make msm_serial.c explicitly non-modular
>   drivers/tty/serial: make 8250/8250_mtk.c explicitly non-modular

Hi Greg,

You've merged the #3 atmel patch and #5 the 8250_mtk patch; the msm_serial
patch (#4) is no longer applicable since the authors changed it to tristate.

That leaves the top two patches; both for PPC hardware.  Did you skip
those because you wanted them pushed through the powerpc tree?

If so, that is fine, I can do that.  Just let me know.

Thanks,
Paul.
--

> 
>  drivers/tty/ehv_bytechan.c         | 40 +++------------------------------
>  drivers/tty/hvc/hvc_vio.c          | 29 ++++--------------------
>  drivers/tty/serial/8250/8250_mtk.c | 35 ++++++-----------------------
>  drivers/tty/serial/atmel_serial.c  | 45 ++++----------------------------------
>  drivers/tty/serial/msm_serial.c    | 30 ++++---------------------
>  5 files changed, 22 insertions(+), 157 deletions(-)
> 
> -- 
> 2.6.1
>