[PATCH 0/3] video/fbdev: avoid module usage in non-modular sparc code

[PATCH 0/3] video/fbdev: avoid module usage in non-modular sparc code

[Paul Gortmaker]

This series of commits is a part of a larger project to ensure
people don't reference modular support functions in non-modular
code.  Overall there was roughly 5k lines of dead code in the
kernel due to this.  So far we've fixed several areas, like tty,
x86, net, ... and we continue to work on other areas.

There are several reasons to not use module support for code that
can never be built as a module, but the big ones are:

 (1) it is easy to accidentally write unused module_exit and remove code
 (2) it can be misleading when reading the source, thinking it can be
     modular when the Makefile and/or Kconfig prohibit it
 (3) it requires the include of the module.h header file which in turn
     includes nearly everything else, thus adding to CPP overhead.
 (4) it gets copied/replicated into other code and spreads like weeds.

For the video fb subsystem, I have about 40 commits in my personal
testing queue, but rather than overwhelm maintainers with a large
patch series, I'm looking for subsets within that to keep the Cc on
the series and the 00/N boiler-plate to a sane amount.

To that end, the sparc specific fb drivers is a nice small subset
that can be used to test the waters in the video subsystem, to ensure
the overall goals here are OK with everyone maintaining fb code.

The disallow of unbind might seem like overkill, but for drivers that
had a ".remove" function, it is the only way we can block people from
abusing existing infrastructure in order to continue getting at that
otherwise dead function call that we want to remove.

We toss out a net amount of about 80 lines of dead code here in just
three commits; not groundbreaking, but a step in the right direction.

Build tested for sparc64, allmodconfig; but with CONFIG_FB=y as the
drivers here all depend on this specific value to get enabled.

Paul.
---

Cc: "David S. Miller" <davem@davemloft.net>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: sparclinux@vger.kernel.org

Paul Gortmaker (3):
  drivers/video: make fbdev/sunxvr500.c explicitly non-modular
  drivers/video: make fbdev/sunxvr1000.c explicitly non-modular
  drivers/video: make fbdev/sunxvr2500.c explicitly non-modular

 drivers/video/fbdev/sunxvr1000.c | 42 +++++++---------------------------------
 drivers/video/fbdev/sunxvr2500.c | 39 +++++++------------------------------
 drivers/video/fbdev/sunxvr500.c  | 42 +++++++---------------------------------
 3 files changed, 21 insertions(+), 102 deletions(-)

-- 
2.6.1

[PATCH 1/3] drivers/video: make fbdev/sunxvr500.c explicitly non-modular

[Paul Gortmaker]

The Kconfig currently controlling compilation of this code is:

config FB_XVR500
        bool "Sun XVR-500 3DLABS Wildcat 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.

We explicitly disallow a driver unbind, since that doesn't have a
sensible use case anyway, and it allows us to drop the ".remove"
code for non-modular drivers.

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

We don't replace module.h with init.h since the file already has that.

We also delete the MODULE_LICENSE tag etc. since all that information
was (or is now) contained at the top of the file in the comments.

Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: sparclinux@vger.kernel.org
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/video/fbdev/sunxvr500.c | 42 +++++++----------------------------------
 1 file changed, 7 insertions(+), 35 deletions(-)

diff --git a/drivers/video/fbdev/sunxvr500.c b/drivers/video/fbdev/sunxvr500.c
index 387350d004df..dc0d886e4e7e 100644
--- a/drivers/video/fbdev/sunxvr500.c
+++ b/drivers/video/fbdev/sunxvr500.c
@@ -1,9 +1,10 @@
-/* sunxvr500.c: Sun 3DLABS XVR-500 Expert3D driver for sparc64 systems
+/* sunxvr500.c: Sun 3DLABS XVR-500 Expert3D fb driver for sparc64 systems
+ *
+ * License: GPL
  *
  * Copyright (C) 2007 David S. Miller (davem@davemloft.net)
  */
 
-#include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/fb.h>
 #include <linux/pci.h>
@@ -392,25 +393,6 @@ err_out:
 	return err;
 }
 
-static void e3d_pci_unregister(struct pci_dev *pdev)
-{
-	struct fb_info *info = pci_get_drvdata(pdev);
-	struct e3d_info *ep = info->par;
-
-	unregister_framebuffer(info);
-
-	iounmap(ep->ramdac);
-	iounmap(ep->fb_base);
-
-	pci_release_region(pdev, 0);
-	pci_release_region(pdev, 1);
-
-	fb_dealloc_cmap(&info->cmap);
-        framebuffer_release(info);
-
-	pci_disable_device(pdev);
-}
-
 static struct pci_device_id e3d_pci_table[] = {
 	{	PCI_DEVICE(PCI_VENDOR_ID_3DLABS, 0x7a0),	},
 	{	PCI_DEVICE(0x1091, 0x7a0),			},
@@ -434,10 +416,12 @@ static struct pci_device_id e3d_pci_table[] = {
 };
 
 static struct pci_driver e3d_driver = {
+	.driver = {
+		.suppress_bind_attrs = true,
+	},
 	.name		= "e3d",
 	.id_table	= e3d_pci_table,
 	.probe		= e3d_pci_register,
-	.remove		= e3d_pci_unregister,
 };
 
 static int __init e3d_init(void)
@@ -447,16 +431,4 @@ static int __init e3d_init(void)
 
 	return pci_register_driver(&e3d_driver);
 }
-
-static void __exit e3d_exit(void)
-{
-	pci_unregister_driver(&e3d_driver);
-}
-
-module_init(e3d_init);
-module_exit(e3d_exit);
-
-MODULE_DESCRIPTION("framebuffer driver for Sun XVR-500 graphics");
-MODULE_AUTHOR("David S. Miller <davem@davemloft.net>");
-MODULE_VERSION("1.0");
-MODULE_LICENSE("GPL");
+device_initcall(e3d_init);
-- 
2.6.1

[PATCH 2/3] drivers/video: make fbdev/sunxvr1000.c explicitly non-modular

[Paul Gortmaker]

The Kconfig currently controlling compilation of this code is:

config FB_XVR1000
        bool "Sun XVR-1000 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 explicitly disallow a driver unbind, since that doesn't have a
sensible use case anyway, and it allows us to drop the ".remove"
code for non-modular drivers.

We don't replace module.h with init.h since the file already has that.
Also note that MODULE_DEVICE_TABLE is a no-op for non-modular code.

We also delete the MODULE_LICENSE tag etc. since all that information
was (or is now) contained at the top of the file in the comments.

Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: sparclinux@vger.kernel.org
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/video/fbdev/sunxvr1000.c | 42 +++++++---------------------------------
 1 file changed, 7 insertions(+), 35 deletions(-)

diff --git a/drivers/video/fbdev/sunxvr1000.c b/drivers/video/fbdev/sunxvr1000.c
index 08879bdfad35..fb37f6e05391 100644
--- a/drivers/video/fbdev/sunxvr1000.c
+++ b/drivers/video/fbdev/sunxvr1000.c
@@ -1,9 +1,10 @@
-/* sunxvr1000.c: Sun XVR-1000 driver for sparc64 systems
+/* sunxvr1000.c: Sun XVR-1000 fb driver for sparc64 systems
+ *
+ * License: GPL
  *
  * Copyright (C) 2010 David S. Miller (davem@davemloft.net)
  */
 
-#include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/fb.h>
 #include <linux/init.h>
@@ -173,36 +174,19 @@ err_out:
 	return err;
 }
 
-static int gfb_remove(struct platform_device *op)
-{
-	struct fb_info *info = dev_get_drvdata(&op->dev);
-	struct gfb_info *gp = info->par;
-
-	unregister_framebuffer(info);
-
-	iounmap(gp->fb_base);
-
-	of_iounmap(&op->resource[6], gp->fb_base, gp->fb_size);
-
-        framebuffer_release(info);
-
-	return 0;
-}
-
 static const struct of_device_id gfb_match[] = {
 	{
 		.name = "SUNW,gfb",
 	},
 	{},
 };
-MODULE_DEVICE_TABLE(of, ffb_match);
 
 static struct platform_driver gfb_driver = {
 	.probe		= gfb_probe,
-	.remove		= gfb_remove,
 	.driver = {
-		.name		= "gfb",
-		.of_match_table	= gfb_match,
+		.name			= "gfb",
+		.of_match_table		= gfb_match,
+		.suppress_bind_attrs	= true,
 	},
 };
 
@@ -213,16 +197,4 @@ static int __init gfb_init(void)
 
 	return platform_driver_register(&gfb_driver);
 }
-
-static void __exit gfb_exit(void)
-{
-	platform_driver_unregister(&gfb_driver);
-}
-
-module_init(gfb_init);
-module_exit(gfb_exit);
-
-MODULE_DESCRIPTION("framebuffer driver for Sun XVR-1000 graphics");
-MODULE_AUTHOR("David S. Miller <davem@davemloft.net>");
-MODULE_VERSION("1.0");
-MODULE_LICENSE("GPL");
+device_initcall(gfb_init);
-- 
2.6.1

[PATCH 3/3] drivers/video: make fbdev/sunxvr2500.c explicitly non-modular

[Paul Gortmaker]

The Kconfig currently controlling compilation of this code is:

config FB_XVR2500
        bool "Sun XVR-2500 3DLABS Wildcat 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 explicitly disallow a driver unbind, since that doesn't have a
sensible use case anyway, and it allows us to drop the ".remove"
code for non-modular drivers.

We don't replace module.h with init.h since the file already has that.

We also delete the MODULE_LICENSE tag etc. since all that information
was (or is now) contained at the top of the file in the comments.

Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: sparclinux@vger.kernel.org
Cc: linux-fbdev@vger.kernel.org
Signed-off-by: Paul Gortmaker <paul.gortmaker@windriver.com>
---
 drivers/video/fbdev/sunxvr2500.c | 39 +++++++--------------------------------
 1 file changed, 7 insertions(+), 32 deletions(-)

diff --git a/drivers/video/fbdev/sunxvr2500.c b/drivers/video/fbdev/sunxvr2500.c
index 843b6bab0483..1a053292f2eb 100644
--- a/drivers/video/fbdev/sunxvr2500.c
+++ b/drivers/video/fbdev/sunxvr2500.c
@@ -1,9 +1,10 @@
-/* s3d.c: Sun 3DLABS XVR-2500 et al. driver for sparc64 systems
+/* sunxvr2500.c: Sun 3DLABS XVR-2500 et al. fb driver for sparc64 systems
+ *
+ * License: GPL
  *
  * Copyright (C) 2007 David S. Miller (davem@davemloft.net)
  */
 
-#include <linux/module.h>
 #include <linux/kernel.h>
 #include <linux/fb.h>
 #include <linux/pci.h>
@@ -219,22 +220,6 @@ err_out:
 	return err;
 }
 
-static void s3d_pci_unregister(struct pci_dev *pdev)
-{
-	struct fb_info *info = pci_get_drvdata(pdev);
-	struct s3d_info *sp = info->par;
-
-	unregister_framebuffer(info);
-
-	iounmap(sp->fb_base);
-
-	pci_release_region(pdev, 1);
-
-        framebuffer_release(info);
-
-	pci_disable_device(pdev);
-}
-
 static struct pci_device_id s3d_pci_table[] = {
 	{	PCI_DEVICE(PCI_VENDOR_ID_3DLABS, 0x002c),	},
 	{	PCI_DEVICE(PCI_VENDOR_ID_3DLABS, 0x002d),	},
@@ -248,10 +233,12 @@ static struct pci_device_id s3d_pci_table[] = {
 };
 
 static struct pci_driver s3d_driver = {
+	.driver = {
+		.suppress_bind_attrs = true,
+	},
 	.name		= "s3d",
 	.id_table	= s3d_pci_table,
 	.probe		= s3d_pci_register,
-	.remove		= s3d_pci_unregister,
 };
 
 static int __init s3d_init(void)
@@ -261,16 +248,4 @@ static int __init s3d_init(void)
 
 	return pci_register_driver(&s3d_driver);
 }
-
-static void __exit s3d_exit(void)
-{
-	pci_unregister_driver(&s3d_driver);
-}
-
-module_init(s3d_init);
-module_exit(s3d_exit);
-
-MODULE_DESCRIPTION("framebuffer driver for Sun XVR-2500 graphics");
-MODULE_AUTHOR("David S. Miller <davem@davemloft.net>");
-MODULE_VERSION("1.0");
-MODULE_LICENSE("GPL");
+device_initcall(s3d_init);
-- 
2.6.1

Re: [PATCH 0/3] video/fbdev: avoid module usage in non-modular sparc code

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1741 bytes --]



On 22/02/16 05:13, Paul Gortmaker wrote:
> This series of commits is a part of a larger project to ensure
> people don't reference modular support functions in non-modular
> code.  Overall there was roughly 5k lines of dead code in the
> kernel due to this.  So far we've fixed several areas, like tty,
> x86, net, ... and we continue to work on other areas.
> 
> There are several reasons to not use module support for code that
> can never be built as a module, but the big ones are:
> 
>  (1) it is easy to accidentally write unused module_exit and remove code
>  (2) it can be misleading when reading the source, thinking it can be
>      modular when the Makefile and/or Kconfig prohibit it
>  (3) it requires the include of the module.h header file which in turn
>      includes nearly everything else, thus adding to CPP overhead.
>  (4) it gets copied/replicated into other code and spreads like weeds.

I think all the "normal" drivers should be compilable as modules. If
they can't, it should be fixed. However, we have lots of old drivers for
which it may be difficult to get anyone to fix them or test them.

Then again, with that kind of drivers it may be better to just let them
be, if there's no big reason to start cleaning them up. In other words,
if the driver is old and not actively developed/maintained, compiles
fine, presumably works fine, doesn't obstruct any other development,
then why touch it at all...

While doing this, did you just go forward removing the module support,
or did you check if it would be trivial to make the driver build as a
module? I wouldn't be surprised if in some cases all that would need to
be done is change the Kconfig's bool to tristate.

 Tomi


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

Re: [PATCH 0/3] video/fbdev: avoid module usage in non-modular sparc code

[Paul Gortmaker]

[Re: [PATCH 0/3] video/fbdev: avoid module usage in non-modular sparc code] On 26/02/2016 (Fri 12:58) Tomi Valkeinen wrote:

> 
> 
> On 22/02/16 05:13, Paul Gortmaker wrote:
> > This series of commits is a part of a larger project to ensure
> > people don't reference modular support functions in non-modular
> > code.  Overall there was roughly 5k lines of dead code in the
> > kernel due to this.  So far we've fixed several areas, like tty,
> > x86, net, ... and we continue to work on other areas.
> > 
> > There are several reasons to not use module support for code that
> > can never be built as a module, but the big ones are:
> > 
> >  (1) it is easy to accidentally write unused module_exit and remove code
> >  (2) it can be misleading when reading the source, thinking it can be
> >      modular when the Makefile and/or Kconfig prohibit it
> >  (3) it requires the include of the module.h header file which in turn
> >      includes nearly everything else, thus adding to CPP overhead.
> >  (4) it gets copied/replicated into other code and spreads like weeds.
> 
> I think all the "normal" drivers should be compilable as modules. If
> they can't, it should be fixed. However, we have lots of old drivers for
> which it may be difficult to get anyone to fix them or test them.

A counter point would be that if an old driver has remained non-modular
for all these years, then clearly there is no demand for adding a new
modular implementation at this point in time.

> 
> Then again, with that kind of drivers it may be better to just let them
> be, if there's no big reason to start cleaning them up. In other words,
> if the driver is old and not actively developed/maintained, compiles
> fine, presumably works fine, doesn't obstruct any other development,
> then why touch it at all...

The main reason is listed as #4 above -- if we keep drivers around that
reflect a disconnect between Kconfig and code, the same mistake gets
copied into more and more new drivers as they are created.

If the argument was to not go in and rewrite core code for legacy
drivers, I'd agree with that, but that isn't what is happening here.
In a lot of these type changes, where the only change is to replace
module_init with device initcall, the object files are identical.

> While doing this, did you just go forward removing the module support,
> or did you check if it would be trivial to make the driver build as a
> module? I wouldn't be surprised if in some cases all that would need to
> be done is change the Kconfig's bool to tristate.

In some cases, where people have explicitly asked for it, indicating
that they (as author) intended it to be tristate, and support it as
such, yes.

But overall, with the number and diversity of the drivers involved, I
did not want to just go around enabling new functionality here and there
on hardware I don't know intimately.  So instead the changes are like
what you see here: the runtime and functionality remains faithful to
what it was, while resolving issues #1 through #4 above.

If subsystem maintainers would rather have blanket tristate coversions
and whatever changes are required to make it compile and modpost, and
are OK to assume things will just work, then that could be an option...

Paul.
--

> 
>  Tomi
> 

Re: [PATCH 0/3] video/fbdev: avoid module usage in non-modular sparc code,Re: [PATCH 0/3] video/fbdev: avoid module usage in non-modular sparc code

[David Miller]

From: Tomi Valkeinen <tomi.valkeinen@ti.com>
Date: Fri, 26 Feb 2016 12:58:00 +0200

> While doing this, did you just go forward removing the module support,
> or did you check if it would be trivial to make the driver build as a
> module? I wouldn't be surprised if in some cases all that would need to
> be done is change the Kconfig's bool to tristate.

I think what he is doing is reasonable, and none of these sparc drivers
have any reason to be built modular.  You need them to have any bootup
console whatsoever, so they have to be built in statically to be
usable at all.

I plan to apply this patch series to my sparc tree as-is.

Re: [PATCH 0/3] video/fbdev: avoid module usage in non-modular sparc code

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 1585 bytes --]

On 26/02/16 15:58, Paul Gortmaker wrote:

> A counter point would be that if an old driver has remained non-modular
> for all these years, then clearly there is no demand for adding a new
> modular implementation at this point in time.

True. Then again, I think fbdev drivers are almost always used as
built-in to get the console up and running early. For fbdev I see the
module support mostly as a way to improve the code quality and to
simplify development and testing.

> The main reason is listed as #4 above -- if we keep drivers around that
> reflect a disconnect between Kconfig and code, the same mistake gets
> copied into more and more new drivers as they are created.

Yep, but the same could be said about having drivers without module
support too =).

In any case, I don't accept new fbdev drivers except in special cases,
so fbdev drivers' value as examples is not that much.

> If the argument was to not go in and rewrite core code for legacy
> drivers, I'd agree with that, but that isn't what is happening here.
> In a lot of these type changes, where the only change is to replace
> module_init with device initcall, the object files are identical.

Yes, the patches look simple enough. Ensuring they would work as modules
would be riskier.

> If subsystem maintainers would rather have blanket tristate coversions
> and whatever changes are required to make it compile and modpost, and
> are OK to assume things will just work, then that could be an option...

Nope, I think these are fine. I'll queue them up for 4.6.

 Tomi


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

Re: [PATCH 0/3] video/fbdev: avoid module usage in non-modular sparc code,Re: [PATCH 0/3] video/fbdev: avoid module usage in non-modular sparc code

[Tomi Valkeinen]

[-- Attachment #1.1: Type: text/plain, Size: 959 bytes --]

On 26/02/16 19:21, David Miller wrote:
> From: Tomi Valkeinen <tomi.valkeinen@ti.com>
> Date: Fri, 26 Feb 2016 12:58:00 +0200
> 
>> While doing this, did you just go forward removing the module support,
>> or did you check if it would be trivial to make the driver build as a
>> module? I wouldn't be surprised if in some cases all that would need to
>> be done is change the Kconfig's bool to tristate.
> 
> I think what he is doing is reasonable, and none of these sparc drivers
> have any reason to be built modular.  You need them to have any bootup
> console whatsoever, so they have to be built in statically to be
> usable at all.

I disagree. Having drivers buildable as modules is always good, even if
the normal use case would be to have them as built-in.

> I plan to apply this patch series to my sparc tree as-is.

They are fbdev patches without any dependency to sparc afaics. So let's
apply them via fbdev tree.

 Tomi


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