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 <[email protected]>
CC: [email protected]
---
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
On Tue, Nov 20, 2012 at 10:45 PM, Solomon Peachy <[email protected]> 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 <[email protected]>
> CC: [email protected]
> ---
> 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
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.
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
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.
- Solomon
--
Solomon Peachy pizza at shaftnet dot org
Melbourne, FL ^^ (mail/jabber/gtalk) ^^
Quidquid latine dictum sit, altum viditur.
On Tue, 20 Nov 2012 13:12:03 -0500
Solomon Peachy <[email protected]> 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
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.
- Solomon
--
Solomon Peachy pizza at shaftnet dot org
Melbourne, FL ^^ (mail/jabber/gtalk) ^^
Quidquid latine dictum sit, altum viditur.
On Wed, Nov 21, 2012 at 10:01 PM, Solomon Peachy <[email protected]> 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.
Thanks,
--
Ming Lei
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.
- Solomon
--
Solomon Peachy pizza at shaftnet dot org
Melbourne, FL ^^ (mail/jabber/gtalk) ^^
Quidquid latine dictum sit, altum viditur.
On Thu, Nov 22, 2012 at 10:15 AM, Solomon Peachy <[email protected]> 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, :-(
>
> 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.
But looks this patch is fine for me.
Thanks,
--
Ming Lei