[2.6 patch] always enable FW_LOADER unless EMBEDDED=y

[2.6 patch] always enable FW_LOADER unless EMBEDDED=y

[Adrian Bunk]

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 <bunk@kernel.org>

---

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

Re: [2.6 patch] always enable FW_LOADER unless EMBEDDED=y

[Greg KH]

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

Re: [2.6 patch] always enable FW_LOADER unless EMBEDDED=y

[Adrian Bunk]

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

Re: [2.6 patch] always enable FW_LOADER unless EMBEDDED=y

[Greg KH]

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

Re: [2.6 patch] always enable FW_LOADER unless EMBEDDED=y

[Adrian Bunk]

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

Re: [2.6 patch] always enable FW_LOADER unless EMBEDDED=y

[Boaz Harrosh]

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

Re: [2.6 patch] always enable FW_LOADER unless EMBEDDED=y

[Adrian Bunk]

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

Re: [2.6 patch] always enable FW_LOADER unless EMBEDDED=y

[Boaz Harrosh]

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

Re: [2.6 patch] always enable FW_LOADER unless EMBEDDED=y

[Greg KH]

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

Re: [2.6 patch] always enable FW_LOADER unless EMBEDDED=y

[Adrian Bunk]

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