James Bottomley recently discovered that we have
{request,release}_firmware() dummies for the case of the actual
functions not being available and has a fix for the bug that was
actually causing build errors for built-in users with
CONFIG_FW_LOADER=m.
But now missing selects on FW_LOADER are no longer visible at
compile-time at all and can become runtime problems.
FW_LOADER is infrastructure with relatively small codesize we can
safely enable for everyone, and only for people who really need small
kernels (and can be expected to know what they are doing) it matters
being able to disable it.
This patch therefore always sets FW_LOADER=y and allows users only to
disable it with EMBEDDED=y.
As a bonus, we can then get rid of all "select FW_LOADER" plus the due
to it required "depends on HOTPLUG" which removes some complexity from
our Kconfig files.
Signed-off-by: Adrian Bunk <[email protected]>
---
This patch has been sent on:
- 2 Jun 2008
462ee1ceb263f523b6f4e3bd30a0f63810f05c67 diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index d7da109..629e255 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -27,8 +27,9 @@ config PREVENT_FIRMWARE_BUILD
If unsure say Y here.
config FW_LOADER
- tristate "Userspace firmware loading support"
+ tristate "Userspace firmware loading support" if EMBEDDED
depends on HOTPLUG
+ default y
---help---
This option is provided for the case where no in-kernel-tree modules
require userspace firmware loading support, but a module built outside
On Tue, Jun 10, 2008 at 07:04:08PM +0300, Adrian Bunk wrote:
> James Bottomley recently discovered that we have
> {request,release}_firmware() dummies for the case of the actual
> functions not being available and has a fix for the bug that was
> actually causing build errors for built-in users with
> CONFIG_FW_LOADER=m.
>
> But now missing selects on FW_LOADER are no longer visible at
> compile-time at all and can become runtime problems.
>
> FW_LOADER is infrastructure with relatively small codesize we can
> safely enable for everyone, and only for people who really need small
> kernels (and can be expected to know what they are doing) it matters
> being able to disable it.
>
> This patch therefore always sets FW_LOADER=y and allows users only to
> disable it with EMBEDDED=y.
>
> As a bonus, we can then get rid of all "select FW_LOADER" plus the due
> to it required "depends on HOTPLUG" which removes some complexity from
> our Kconfig files.
Well, we can't get rid of that if EMBEDDED is set, right?
You sent this as an RFC before, I thought people said to just fix up all
of the dependancies with drivers that needed FW_LOADER to be enabled,
that would be easier, right?
thanks,
greg k-h
On Tue, Jun 10, 2008 at 09:24:50AM -0700, Greg KH wrote:
> On Tue, Jun 10, 2008 at 07:04:08PM +0300, Adrian Bunk wrote:
> > James Bottomley recently discovered that we have
> > {request,release}_firmware() dummies for the case of the actual
> > functions not being available and has a fix for the bug that was
> > actually causing build errors for built-in users with
> > CONFIG_FW_LOADER=m.
> >
> > But now missing selects on FW_LOADER are no longer visible at
> > compile-time at all and can become runtime problems.
> >
> > FW_LOADER is infrastructure with relatively small codesize we can
> > safely enable for everyone, and only for people who really need small
> > kernels (and can be expected to know what they are doing) it matters
> > being able to disable it.
> >
> > This patch therefore always sets FW_LOADER=y and allows users only to
> > disable it with EMBEDDED=y.
> >
> > As a bonus, we can then get rid of all "select FW_LOADER" plus the due
> > to it required "depends on HOTPLUG" which removes some complexity from
> > our Kconfig files.
>
> Well, we can't get rid of that if EMBEDDED is set, right?
No, if EMBEDDED is set and HOTPLUG is not set you will not be able to
enable FW_LOADER (the "depends on HOTPLUG" for FW_LOADER has to stay,
but all the options that currently select FW_LOADER no longer need the
dependency).
> You sent this as an RFC before, I thought people said to just fix up all
> of the dependancies with drivers that needed FW_LOADER to be enabled,
> that would be easier, right?
As far as I know there were zero answers to my RFC.
What are you referring to?
> thanks,
>
> greg k-h
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
On Tue, Jun 10, 2008 at 09:12:06PM +0300, Adrian Bunk wrote:
> On Tue, Jun 10, 2008 at 09:24:50AM -0700, Greg KH wrote:
> > On Tue, Jun 10, 2008 at 07:04:08PM +0300, Adrian Bunk wrote:
> > > James Bottomley recently discovered that we have
> > > {request,release}_firmware() dummies for the case of the actual
> > > functions not being available and has a fix for the bug that was
> > > actually causing build errors for built-in users with
> > > CONFIG_FW_LOADER=m.
> > >
> > > But now missing selects on FW_LOADER are no longer visible at
> > > compile-time at all and can become runtime problems.
> > >
> > > FW_LOADER is infrastructure with relatively small codesize we can
> > > safely enable for everyone, and only for people who really need small
> > > kernels (and can be expected to know what they are doing) it matters
> > > being able to disable it.
> > >
> > > This patch therefore always sets FW_LOADER=y and allows users only to
> > > disable it with EMBEDDED=y.
> > >
> > > As a bonus, we can then get rid of all "select FW_LOADER" plus the due
> > > to it required "depends on HOTPLUG" which removes some complexity from
> > > our Kconfig files.
> >
> > Well, we can't get rid of that if EMBEDDED is set, right?
>
> No, if EMBEDDED is set and HOTPLUG is not set you will not be able to
> enable FW_LOADER (the "depends on HOTPLUG" for FW_LOADER has to stay,
> but all the options that currently select FW_LOADER no longer need the
> dependency).
Ok, but what about the point that the options that are wanting FW_LOADER
in that situation? I know EMBEDDED is tough to get right as you can
shoot yourself in the foot very easily, but this seems like we are going
to make it even harder to use properly.
> > You sent this as an RFC before, I thought people said to just fix up all
> > of the dependancies with drivers that needed FW_LOADER to be enabled,
> > that would be easier, right?
>
> As far as I know there were zero answers to my RFC.
>
> What are you referring to?
Sorry, I thought that was sent to the RFC, I think it was to a thread
even before that.
I don't like this change for the above reason at this point.
thanks,
greg k-h
On Tue, Jun 10, 2008 at 02:28:59PM -0700, Greg KH wrote:
> On Tue, Jun 10, 2008 at 09:12:06PM +0300, Adrian Bunk wrote:
> > On Tue, Jun 10, 2008 at 09:24:50AM -0700, Greg KH wrote:
> > > On Tue, Jun 10, 2008 at 07:04:08PM +0300, Adrian Bunk wrote:
> > > > James Bottomley recently discovered that we have
> > > > {request,release}_firmware() dummies for the case of the actual
> > > > functions not being available and has a fix for the bug that was
> > > > actually causing build errors for built-in users with
> > > > CONFIG_FW_LOADER=m.
> > > >
> > > > But now missing selects on FW_LOADER are no longer visible at
> > > > compile-time at all and can become runtime problems.
> > > >
> > > > FW_LOADER is infrastructure with relatively small codesize we can
> > > > safely enable for everyone, and only for people who really need small
> > > > kernels (and can be expected to know what they are doing) it matters
> > > > being able to disable it.
> > > >
> > > > This patch therefore always sets FW_LOADER=y and allows users only to
> > > > disable it with EMBEDDED=y.
> > > >
> > > > As a bonus, we can then get rid of all "select FW_LOADER" plus the due
> > > > to it required "depends on HOTPLUG" which removes some complexity from
> > > > our Kconfig files.
> > >
> > > Well, we can't get rid of that if EMBEDDED is set, right?
> >
> > No, if EMBEDDED is set and HOTPLUG is not set you will not be able to
> > enable FW_LOADER (the "depends on HOTPLUG" for FW_LOADER has to stay,
> > but all the options that currently select FW_LOADER no longer need the
> > dependency).
>
> Ok, but what about the point that the options that are wanting FW_LOADER
> in that situation? I know EMBEDDED is tough to get right as you can
> shoot yourself in the foot very easily, but this seems like we are going
> to make it even harder to use properly.
Currently missing selects can result in a build error.
After James' patch to fix the dummy functions (that should have worked
from day 1) it will instead become a runtime problem.
And when the select gets added the dependency on HOTPLUG often gets
forgotten.
If you worry about "EMBEDDED is tough to get right" a bigger problem
would be the linux-tiny patches to add new config options - each might
save a few kB when disabled but also offers a new way to shoot yourself
into the foot.
I do care more about:
- that users for the common CONFIG_EMBEDDED=n case don't run into
unexpected problems (due to a missing select on FW_LOADER) and
- that we remove complexity from our kconfig dependencies so that
they break less often.
I'm counting 6 commits that fix bugs in this area introduced since
2.6.25 (sic) under drivers/media/ alone.
And Davids firmware patches offer even more opportunities for people to
forget the dependency on HOTPLUG (which will break only in the
CONFIG_EMBEDDED=y case...). [1]
> > > You sent this as an RFC before, I thought people said to just fix up all
> > > of the dependancies with drivers that needed FW_LOADER to be enabled,
> > > that would be easier, right?
> >
> > As far as I know there were zero answers to my RFC.
> >
> > What are you referring to?
>
> Sorry, I thought that was sent to the RFC, I think it was to a thread
> even before that.
>...
If it was in the "build issue #503 ..." thread it was even me who
said it - before I had the idea of hiding FW_LOADER behind EMBEDDED.
> thanks,
>
> greg k-h
cu
Adrian
[1] and before anyone restarts the "kconfig is broken - select should
follow dependencies" FUD: I've seen much talk and zero code for
this, and in my opinion the problem is much more difficult than
it appears at first sight
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
Adrian Bunk wrote:
>
> [1] and before anyone restarts the "kconfig is broken - select should
> follow dependencies" FUD: I've seen much talk and zero code for
> this, and in my opinion the problem is much more difficult than
> it appears at first sight
>
I had this crazy idea, Perhaps it would be easier to reverse the problem.
Introduce a kind of Lazy_Enable mode. Which means config is enabled but
gets it's use_count bumped up every time someone is dependent on it.
At the end, for optimization, any Lazy_Enable with zero count, is dropped.
Is that at all possible in current infrastructure?
(I admit I never looked at the source)
Boaz
On Wed, Jun 11, 2008 at 12:20:05PM +0300, Boaz Harrosh wrote:
> Adrian Bunk wrote:
> >
> > [1] and before anyone restarts the "kconfig is broken - select should
> > follow dependencies" FUD: I've seen much talk and zero code for
> > this, and in my opinion the problem is much more difficult than
> > it appears at first sight
>
> I had this crazy idea, Perhaps it would be easier to reverse the problem.
> Introduce a kind of Lazy_Enable mode. Which means config is enabled but
> gets it's use_count bumped up every time someone is dependent on it.
> At the end, for optimization, any Lazy_Enable with zero count, is dropped.
> Is that at all possible in current infrastructure?
> (I admit I never looked at the source)
Wouldn't your "Lazy_Enable" do the same as "select" already does?
> Boaz
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed
Adrian Bunk wrote:
> On Wed, Jun 11, 2008 at 12:20:05PM +0300, Boaz Harrosh wrote:
>> Adrian Bunk wrote:
>>> [1] and before anyone restarts the "kconfig is broken - select should
>>> follow dependencies" FUD: I've seen much talk and zero code for
>>> this, and in my opinion the problem is much more difficult than
>>> it appears at first sight
>> I had this crazy idea, Perhaps it would be easier to reverse the problem.
>> Introduce a kind of Lazy_Enable mode. Which means config is enabled but
>> gets it's use_count bumped up every time someone is dependent on it.
>> At the end, for optimization, any Lazy_Enable with zero count, is dropped.
>> Is that at all possible in current infrastructure?
>> (I admit I never looked at the source)
>
> Wouldn't your "Lazy_Enable" do the same as "select" already does?
>
No, I mean, for example, ISCSI_TCP will just "depends" on SCSI_ISCSI_ATTRS but
SCSI_ISCSI_ATTRS is Lazy_Enable. So a user can always choose ISCSI_TCP if he
wants to. At the end if there are no users for SCSI_ISCSI_ATTRS it is dropped.
Lazy_Enable is for these modules that say: "I have no purpose of my own
but only serve other needs"
Theoretically it looks like a simpler problem. (Pull vs Push)
>> Boaz
>
> cu
> Adrian
>
Just my $0.017
Boaz
On Wed, Jun 11, 2008 at 11:23:20AM +0300, Adrian Bunk wrote:
> On Tue, Jun 10, 2008 at 02:28:59PM -0700, Greg KH wrote:
> > On Tue, Jun 10, 2008 at 09:12:06PM +0300, Adrian Bunk wrote:
> > > On Tue, Jun 10, 2008 at 09:24:50AM -0700, Greg KH wrote:
> > > > On Tue, Jun 10, 2008 at 07:04:08PM +0300, Adrian Bunk wrote:
> > > > > James Bottomley recently discovered that we have
> > > > > {request,release}_firmware() dummies for the case of the actual
> > > > > functions not being available and has a fix for the bug that was
> > > > > actually causing build errors for built-in users with
> > > > > CONFIG_FW_LOADER=m.
> > > > >
> > > > > But now missing selects on FW_LOADER are no longer visible at
> > > > > compile-time at all and can become runtime problems.
> > > > >
> > > > > FW_LOADER is infrastructure with relatively small codesize we can
> > > > > safely enable for everyone, and only for people who really need small
> > > > > kernels (and can be expected to know what they are doing) it matters
> > > > > being able to disable it.
> > > > >
> > > > > This patch therefore always sets FW_LOADER=y and allows users only to
> > > > > disable it with EMBEDDED=y.
> > > > >
> > > > > As a bonus, we can then get rid of all "select FW_LOADER" plus the due
> > > > > to it required "depends on HOTPLUG" which removes some complexity from
> > > > > our Kconfig files.
> > > >
> > > > Well, we can't get rid of that if EMBEDDED is set, right?
> > >
> > > No, if EMBEDDED is set and HOTPLUG is not set you will not be able to
> > > enable FW_LOADER (the "depends on HOTPLUG" for FW_LOADER has to stay,
> > > but all the options that currently select FW_LOADER no longer need the
> > > dependency).
> >
> > Ok, but what about the point that the options that are wanting FW_LOADER
> > in that situation? I know EMBEDDED is tough to get right as you can
> > shoot yourself in the foot very easily, but this seems like we are going
> > to make it even harder to use properly.
>
> Currently missing selects can result in a build error.
>
> After James' patch to fix the dummy functions (that should have worked
> from day 1) it will instead become a runtime problem.
Ah, yes, that should be fine then.
Can you bounce me the patch again?
thanks,
greg k-h
On Wed, Jun 11, 2008 at 11:20:40AM -0700, Greg KH wrote:
> On Wed, Jun 11, 2008 at 11:23:20AM +0300, Adrian Bunk wrote:
> > On Tue, Jun 10, 2008 at 02:28:59PM -0700, Greg KH wrote:
> > > On Tue, Jun 10, 2008 at 09:12:06PM +0300, Adrian Bunk wrote:
> > > > On Tue, Jun 10, 2008 at 09:24:50AM -0700, Greg KH wrote:
> > > > > On Tue, Jun 10, 2008 at 07:04:08PM +0300, Adrian Bunk wrote:
> > > > > > James Bottomley recently discovered that we have
> > > > > > {request,release}_firmware() dummies for the case of the actual
> > > > > > functions not being available and has a fix for the bug that was
> > > > > > actually causing build errors for built-in users with
> > > > > > CONFIG_FW_LOADER=m.
> > > > > >
> > > > > > But now missing selects on FW_LOADER are no longer visible at
> > > > > > compile-time at all and can become runtime problems.
> > > > > >
> > > > > > FW_LOADER is infrastructure with relatively small codesize we can
> > > > > > safely enable for everyone, and only for people who really need small
> > > > > > kernels (and can be expected to know what they are doing) it matters
> > > > > > being able to disable it.
> > > > > >
> > > > > > This patch therefore always sets FW_LOADER=y and allows users only to
> > > > > > disable it with EMBEDDED=y.
> > > > > >
> > > > > > As a bonus, we can then get rid of all "select FW_LOADER" plus the due
> > > > > > to it required "depends on HOTPLUG" which removes some complexity from
> > > > > > our Kconfig files.
> > > > >
> > > > > Well, we can't get rid of that if EMBEDDED is set, right?
> > > >
> > > > No, if EMBEDDED is set and HOTPLUG is not set you will not be able to
> > > > enable FW_LOADER (the "depends on HOTPLUG" for FW_LOADER has to stay,
> > > > but all the options that currently select FW_LOADER no longer need the
> > > > dependency).
> > >
> > > Ok, but what about the point that the options that are wanting FW_LOADER
> > > in that situation? I know EMBEDDED is tough to get right as you can
> > > shoot yourself in the foot very easily, but this seems like we are going
> > > to make it even harder to use properly.
> >
> > Currently missing selects can result in a build error.
> >
> > After James' patch to fix the dummy functions (that should have worked
> > from day 1) it will instead become a runtime problem.
>
> Ah, yes, that should be fine then.
>
> Can you bounce me the patch again?
done
> thanks,
>
> greg k-h
cu
Adrian
--
"Is there not promise of rain?" Ling Tan asked suddenly out
of the darkness. There had been need of rain for many days.
"Only a promise," Lao Er said.
Pearl S. Buck - Dragon Seed