Google Groups no longer supports new Usenet posts or subscriptions. Historical content remains viewable.
Dismiss

[PATCH] [firmware_class] Fix compile with no builtin firmware

29 views
Skip to first unread message

Solomon Peachy

unread,
Nov 20, 2012, 9:50:03 AM11/20/12
to
When compiling the firmware loader, the builtin firmware functions were
erroneously compiled as they were wrapped with CONFIG_FW_LOADER instead
of CONFIG_FIRMWARE_IN_KERNEL. This is normally harmless, except when
there was actually no firmware to compile into the kernel, causing the
build to fail with a linking error:

drivers/built-in.o: In function `release_firmware':
(.text+0x192e2): undefined reference to `__end_builtin_fw'
drivers/built-in.o: In function `release_firmware':
(.text+0x19304): undefined reference to `__end_builtin_fw'
drivers/built-in.o: In function `_request_firmware':
firmware_class.c:(.text+0x1986c): undefined reference to `__end_builtin_fw'
firmware_class.c:(.text+0x19886): undefined reference to `__end_builtin_fw'
firmware_class.c:(.text+0x19a98): undefined reference to `__end_builtin_fw'
drivers/built-in.o: In function `release_firmware':
(.text+0x192dc): undefined reference to `__start_builtin_fw'
drivers/built-in.o: In function `_request_firmware':
firmware_class.c:(.text+0x19860): undefined reference to `__start_builtin_fw'
firmware_class.c:(.text+0x19a8a): undefined reference to `__start_builtin_fw'

This trivial patch fixes this oversight.

Signed-off-by: Solomon Peachy <pi...@shaftnet.org>
CC: sta...@vger.kernel.org
---
drivers/base/firmware_class.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8945f4e..3474e7f 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -38,7 +38,7 @@ MODULE_LICENSE("GPL");

/* Builtin firmware support */

-#ifdef CONFIG_FW_LOADER
+#ifdef CONFIG_FIRMWARE_IN_KERNEL

extern struct builtin_fw __start_builtin_fw[];
extern struct builtin_fw __end_builtin_fw[];
--
1.7.11.7

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/

Ming Lei

unread,
Nov 20, 2012, 11:10:02 AM11/20/12
to
On Tue, Nov 20, 2012 at 10:45 PM, Solomon Peachy <pi...@shaftnet.org> wrote:
> When compiling the firmware loader, the builtin firmware functions were
> erroneously compiled as they were wrapped with CONFIG_FW_LOADER instead
> of CONFIG_FIRMWARE_IN_KERNEL. This is normally harmless, except when
> there was actually no firmware to compile into the kernel, causing the
> build to fail with a linking error:
>
> drivers/built-in.o: In function `release_firmware':
> (.text+0x192e2): undefined reference to `__end_builtin_fw'
> drivers/built-in.o: In function `release_firmware':
> (.text+0x19304): undefined reference to `__end_builtin_fw'
> drivers/built-in.o: In function `_request_firmware':
> firmware_class.c:(.text+0x1986c): undefined reference to `__end_builtin_fw'
> firmware_class.c:(.text+0x19886): undefined reference to `__end_builtin_fw'
> firmware_class.c:(.text+0x19a98): undefined reference to `__end_builtin_fw'
> drivers/built-in.o: In function `release_firmware':
> (.text+0x192dc): undefined reference to `__start_builtin_fw'
> drivers/built-in.o: In function `_request_firmware':
> firmware_class.c:(.text+0x19860): undefined reference to `__start_builtin_fw'
> firmware_class.c:(.text+0x19a8a): undefined reference to `__start_builtin_fw'

I have tried to reproduce the compile failure but not succeed, could you share
your .config?

>
> This trivial patch fixes this oversight.
>
> Signed-off-by: Solomon Peachy <pi...@shaftnet.org>
> CC: sta...@vger.kernel.org
> ---
> drivers/base/firmware_class.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> index 8945f4e..3474e7f 100644
> --- a/drivers/base/firmware_class.c
> +++ b/drivers/base/firmware_class.c
> @@ -38,7 +38,7 @@ MODULE_LICENSE("GPL");
>
> /* Builtin firmware support */
>
> -#ifdef CONFIG_FW_LOADER
> +#ifdef CONFIG_FIRMWARE_IN_KERNEL

This might not be correct when EXTRA_FIRMWARE is set and
CONFIG_FIRMWARE_IN_KERNEL is unset.

Thanks,
--
Ming Lei

Solomon Peachy

unread,
Nov 20, 2012, 11:20:01 AM11/20/12
to
On Wed, Nov 21, 2012 at 12:01:40AM +0800, Ming Lei wrote:
> > drivers/built-in.o: In function `release_firmware':
> > (.text+0x192e2): undefined reference to `__end_builtin_fw'
> > drivers/built-in.o: In function `release_firmware':
> > (.text+0x19304): undefined reference to `__end_builtin_fw'
> > drivers/built-in.o: In function `_request_firmware':
> > firmware_class.c:(.text+0x1986c): undefined reference to `__end_builtin_fw'
> > firmware_class.c:(.text+0x19886): undefined reference to `__end_builtin_fw'
> > firmware_class.c:(.text+0x19a98): undefined reference to `__end_builtin_fw'
> > drivers/built-in.o: In function `release_firmware':
> > (.text+0x192dc): undefined reference to `__start_builtin_fw'
> > drivers/built-in.o: In function `_request_firmware':
> > firmware_class.c:(.text+0x19860): undefined reference to `__start_builtin_fw'
> > firmware_class.c:(.text+0x19a8a): undefined reference to `__start_builtin_fw'
>
> I have tried to reproduce the compile failure but not succeed, could you share
> your .config?

The corresponding .config is attached. Note that it is for a uClinux
3.3.0-uc0 kernel.

Even in the absence of the compile error, I believe the patch is correct.

- Solomon
--
Solomon Peachy pizza at shaftnet dot org
Melbourne, FL ^^ (mail/jabber/gtalk) ^^
Quidquid latine dictum sit, altum viditur.
.config

Greg KH

unread,
Nov 20, 2012, 11:40:03 AM11/20/12
to
On Tue, Nov 20, 2012 at 11:10:11AM -0500, Solomon Peachy wrote:
> On Wed, Nov 21, 2012 at 12:01:40AM +0800, Ming Lei wrote:
> > > drivers/built-in.o: In function `release_firmware':
> > > (.text+0x192e2): undefined reference to `__end_builtin_fw'
> > > drivers/built-in.o: In function `release_firmware':
> > > (.text+0x19304): undefined reference to `__end_builtin_fw'
> > > drivers/built-in.o: In function `_request_firmware':
> > > firmware_class.c:(.text+0x1986c): undefined reference to `__end_builtin_fw'
> > > firmware_class.c:(.text+0x19886): undefined reference to `__end_builtin_fw'
> > > firmware_class.c:(.text+0x19a98): undefined reference to `__end_builtin_fw'
> > > drivers/built-in.o: In function `release_firmware':
> > > (.text+0x192dc): undefined reference to `__start_builtin_fw'
> > > drivers/built-in.o: In function `_request_firmware':
> > > firmware_class.c:(.text+0x19860): undefined reference to `__start_builtin_fw'
> > > firmware_class.c:(.text+0x19a8a): undefined reference to `__start_builtin_fw'
> >
> > I have tried to reproduce the compile failure but not succeed, could you share
> > your .config?
>
> The corresponding .config is attached. Note that it is for a uClinux
> 3.3.0-uc0 kernel.

Lots of things have changed in the firmware code since 3.3.0, can you
retest this on the 3.7-rc6 tree?

Also, when sending patches, please cc: the proper maintainers (the
scripts/get_maintainers.pl tool can help you with that.)

thanks,

greg k-h

Solomon Peachy

unread,
Nov 20, 2012, 1:20:01 PM11/20/12
to
On Tue, Nov 20, 2012 at 08:33:09AM -0800, Greg KH wrote:
> > The corresponding .config is attached. Note that it is for a uClinux
> > 3.3.0-uc0 kernel.
>
> Lots of things have changed in the firmware code since 3.3.0, can you
> retest this on the 3.7-rc6 tree?

Not easily; My employer is contracted to do some driver porting and
we're stuck with the kernel the client provided. However, the patch is
still relevant for upstream, because the underlying problem still
exists:

* The #ifdef wraps code that pertains solely to built-in firmware, (ie
CONFIG_FIRMWARE_IN_KERNEL) and has an #else path for when it's disabled.
* There is no point in a CONFIG_FW_LOADER test inside firmware_class.c
when the file isn't even compiled unless CONFIG_FW_LOADER is defined.

Perhaps the compile problem is solved in newer kernels (by always
generating an empty builtin firmware list?) but the #ifdef is still
incorrect.

> Also, when sending patches, please cc: the proper maintainers (the
> scripts/get_maintainers.pl tool can help you with that.)

Ooops, sorry. I'll double-check that next time.

Ming Lei

unread,
Nov 20, 2012, 8:40:02 PM11/20/12
to
On Tue, 20 Nov 2012 13:12:03 -0500
Solomon Peachy <pi...@shaftnet.org> wrote:

> On Tue, Nov 20, 2012 at 08:33:09AM -0800, Greg KH wrote:
> > > The corresponding .config is attached. Note that it is for a uClinux
> > > 3.3.0-uc0 kernel.
> >
> > Lots of things have changed in the firmware code since 3.3.0, can you
> > retest this on the 3.7-rc6 tree?

Solomon, I can't duplicate the build failure with your .config on 3.7-rc5-next.

>
> Not easily; My employer is contracted to do some driver porting and
> we're stuck with the kernel the client provided. However, the patch is
> still relevant for upstream, because the underlying problem still
> exists:
>
> * The #ifdef wraps code that pertains solely to built-in firmware, (ie
> CONFIG_FIRMWARE_IN_KERNEL) and has an #else path for when it's disabled.
> * There is no point in a CONFIG_FW_LOADER test inside firmware_class.c
> when the file isn't even compiled unless CONFIG_FW_LOADER is defined.

Enabling CONFIG_EXTRA_FIRMWARE still can make one firmware built in kernel
even though CONFIG_FIRMWARE_IN_KERNEL isn't defined, so your patch will break
this case.

>
> Perhaps the compile problem is solved in newer kernels (by always
> generating an empty builtin firmware list?) but the #ifdef is still
> incorrect.

Looks the problem hasn't been reported before.


Thanks,
--
Ming Lei

Solomon Peachy

unread,
Nov 21, 2012, 9:10:02 AM11/21/12
to
On Wed, Nov 21, 2012 at 09:35:28AM +0800, Ming Lei wrote:
> Solomon, I can't duplicate the build failure with your .config on 3.7-rc5-next.

Okay, so it's since been fixed.

> > * The #ifdef wraps code that pertains solely to built-in firmware, (ie
> > CONFIG_FIRMWARE_IN_KERNEL) and has an #else path for when it's disabled.
> > * There is no point in a CONFIG_FW_LOADER test inside firmware_class.c
> > when the file isn't even compiled unless CONFIG_FW_LOADER is defined.
>
> Enabling CONFIG_EXTRA_FIRMWARE still can make one firmware built in kernel
> even though CONFIG_FIRMWARE_IN_KERNEL isn't defined, so your patch will break
> this case.

So... isn't the logical solution here to make CONFIG_EXTRA_FIRMARE
depend on (or enable) CONFIG_FIRMWARE_IN_KERNEL? After all, the two are
apparently related.

I can update my patch to include this, and rewrite the commit message so
it's relevant to modern kernels, or I can just drop this and forget the
whole affair.

Ming Lei

unread,
Nov 22, 2012, 3:10:03 PM11/22/12
to
On Wed, Nov 21, 2012 at 10:01 PM, Solomon Peachy <pi...@shaftnet.org> wrote:
> On Wed, Nov 21, 2012 at 09:35:28AM +0800, Ming Lei wrote:
>> Solomon, I can't duplicate the build failure with your .config on 3.7-rc5-next.
>
> Okay, so it's since been fixed.
>
>> > * The #ifdef wraps code that pertains solely to built-in firmware, (ie
>> > CONFIG_FIRMWARE_IN_KERNEL) and has an #else path for when it's disabled.
>> > * There is no point in a CONFIG_FW_LOADER test inside firmware_class.c
>> > when the file isn't even compiled unless CONFIG_FW_LOADER is defined.
>>
>> Enabling CONFIG_EXTRA_FIRMWARE still can make one firmware built in kernel
>> even though CONFIG_FIRMWARE_IN_KERNEL isn't defined, so your patch will break
>> this case.
>
> So... isn't the logical solution here to make CONFIG_EXTRA_FIRMARE
> depend on (or enable) CONFIG_FIRMWARE_IN_KERNEL? After all, the two are
> apparently related.

No, it is not related closely, CONFIG_FIRMWARE_IN_KERNEL means
that all in-kernel-tree firmware blobs should be included in kernel binary,
but CONFIG_EXTRA_FIRMARE means that one additional firmware
image will be put into kernel binary.

>
> I can update my patch to include this, and rewrite the commit message so
> it's relevant to modern kernels, or I can just drop this and forget the
> whole affair.

Considered that there is no your problem in -linus tree or -next tree and
the current code works for long time, maybe it is better to not touch the code.
Or suggest you to study this kind of config options and firmware/Makefie first,
then figure out one proper patch.

Solomon Peachy

unread,
Nov 22, 2012, 4:10:02 PM11/22/12
to
On Thu, Nov 22, 2012 at 09:45:23AM +0800, Ming Lei wrote:
> No, it is not related closely, CONFIG_FIRMWARE_IN_KERNEL means
> that all in-kernel-tree firmware blobs should be included in kernel binary,
> but CONFIG_EXTRA_FIRMARE means that one additional firmware
> image will be put into kernel binary.

Okay.

> Considered that there is no your problem in -linus tree or -next tree
> and the current code works for long time, maybe it is better to not
> touch the code. Or suggest you to study this kind of config options
> and firmware/Makefie first, then figure out one proper patch.

Given what you've told me (i.e. support for loading "builtin" firmware is
always required), I propose this patch instead:

diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
index 8945f4e..d291e15 100644
--- a/drivers/base/firmware_class.c
+++ b/drivers/base/firmware_class.c
@@ -38,8 +38,6 @@ MODULE_LICENSE("GPL");

/* Builtin firmware support */

-#ifdef CONFIG_FW_LOADER
-
extern struct builtin_fw __start_builtin_fw[];
extern struct builtin_fw __end_builtin_fw[];

@@ -69,19 +67,6 @@ static bool fw_is_builtin_firmware(const struct firmware *fw)
return false;
}

-#else /* Module case - no builtin firmware support */
-
-static inline bool fw_get_builtin_firmware(struct firmware *fw, const char *name)
-{
- return false;
-}
-
-static inline bool fw_is_builtin_firmware(const struct firmware *fw)
-{
- return false;
-}
-#endif
-
enum {
FW_STATUS_LOADING,
FW_STATUS_DONE,


The empty stub functions can never be compiled, as firmware_class.c
isn't compiled when CONFIG_FW_LOADER isn't enabled. So let's just nuke
this unused code entirely.

Ming Lei

unread,
Nov 22, 2012, 5:10:02 PM11/22/12
to
On Thu, Nov 22, 2012 at 10:15 AM, Solomon Peachy <pi...@shaftnet.org> wrote:
> On Thu, Nov 22, 2012 at 09:45:23AM +0800, Ming Lei wrote:
>> No, it is not related closely, CONFIG_FIRMWARE_IN_KERNEL means
>> that all in-kernel-tree firmware blobs should be included in kernel binary,
>> but CONFIG_EXTRA_FIRMARE means that one additional firmware
>> image will be put into kernel binary.
>
> Okay.
>
>> Considered that there is no your problem in -linus tree or -next tree
>> and the current code works for long time, maybe it is better to not
>> touch the code. Or suggest you to study this kind of config options
>> and firmware/Makefie first, then figure out one proper patch.
>
> Given what you've told me (i.e. support for loading "builtin" firmware is
> always required), I propose this patch instead:

Sorry, I didn't tell you builtin firmware is always required, :-(
But looks this patch is fine for me.
0 new messages