Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1344123 > unrolled thread

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

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 0/3] video/fbdev: avoid module usage in non-modular sparc  code Tomi Valkeinen <tomi.valkeinen@ti.com> - 2016-02-26 12:00 +0100
    Re: [PATCH 0/3] video/fbdev: avoid module usage in non-modular sparc  code Paul Gortmaker <paul.gortmaker@windriver.com> - 2016-02-26 15:00 +0100
      Re: [PATCH 0/3] video/fbdev: avoid module usage in non-modular sparc  code Tomi Valkeinen <tomi.valkeinen@ti.com> - 2016-02-29 09:50 +0100
    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 <davem@davemloft.net> - 2016-02-26 18:30 +0100
      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 <tomi.valkeinen@ti.com> - 2016-02-29 10:00 +0100

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [next] | [standalone]

#1344267

[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
> 

[toc] | [prev] | [next] | [standalone]

#1345578

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1344483 — 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

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.

[toc] | [prev] | [next] | [standalone]

#1345580 — 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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web