Patchwork [U-Boot] tools/envcrc: fix compile breakage

login
register
mail settings
Submitter Igor Grinberg
Date Nov. 28, 2011, 7:57 a.m.
Message ID <1322467058-30532-1-git-send-email-grinberg@compulab.co.il>
Download mbox | patch
Permalink /patch/127934/
State Accepted
Delegated to: Wolfgang Denk
Headers show

Comments

Igor Grinberg - Nov. 28, 2011, 7:57 a.m.
When ENV_IS_EMBEDDED is not set, but CONFIG_BUILD_ENVCRC is set,
the environment.h file does not get included resulting in unrecognized
env_t type.
Fix this by moving the include directive.

Reported-by: Mike Frysinger <vapier@gentoo.org>
Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
---
 tools/envcrc.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)
Wolfgang Denk - Nov. 28, 2011, 8:07 a.m.
Dear Igor Grinberg,

In message <1322467058-30532-1-git-send-email-grinberg@compulab.co.il> you wrote:
> When ENV_IS_EMBEDDED is not set, but CONFIG_BUILD_ENVCRC is set,
> the environment.h file does not get included resulting in unrecognized
> env_t type.
> Fix this by moving the include directive.

Hm... the fix seems wrong to me.  What happens now if ENV_IS_EMBEDDED
is set, but CONFIG_BUILD_ENVCRC is _not_ set?

Did this change pass a MAKEALL for example for ppc?

Best regards,

Wolfgang Denk
Wolfgang Denk - Nov. 28, 2011, 8:18 a.m.
Dear Igor Grinberg,

In message <1322467058-30532-1-git-send-email-grinberg@compulab.co.il> you wrote:
> When ENV_IS_EMBEDDED is not set, but CONFIG_BUILD_ENVCRC is set,
> the environment.h file does not get included resulting in unrecognized
> env_t type.
> Fix this by moving the include directive.

...
> -extern uint32_t crc32 (uint32_t, const unsigned char *, unsigned int);
...
> +extern uint32_t crc32 (uint32_t, const unsigned char *, unsigned int);

Also, instead of moving this line, you should rather insert something
like "#include <u-boot/crc.h>" ...

Best regards,

Wolfgang Denk
Igor Grinberg - Nov. 28, 2011, 8:53 a.m.
Hi Wolfgang,

On 11/28/11 10:18, Wolfgang Denk wrote:
> Dear Igor Grinberg,
> 
> In message <1322467058-30532-1-git-send-email-grinberg@compulab.co.il> you wrote:
>> When ENV_IS_EMBEDDED is not set, but CONFIG_BUILD_ENVCRC is set,
>> the environment.h file does not get included resulting in unrecognized
>> env_t type.
>> Fix this by moving the include directive.
> 
> ...
>> -extern uint32_t crc32 (uint32_t, const unsigned char *, unsigned int);
> ...
>> +extern uint32_t crc32 (uint32_t, const unsigned char *, unsigned int);
> 
> Also, instead of moving this line, you should rather insert something
> like "#include <u-boot/crc.h>" ...

I had no intend to clean this file,
but I can do this change in a separate patch.
Igor Grinberg - Nov. 28, 2011, 9:04 a.m.
Hi Wolfgang,

On 11/28/11 10:07, Wolfgang Denk wrote:
> Dear Igor Grinberg,
> 
> In message <1322467058-30532-1-git-send-email-grinberg@compulab.co.il> you wrote:
>> When ENV_IS_EMBEDDED is not set, but CONFIG_BUILD_ENVCRC is set,
>> the environment.h file does not get included resulting in unrecognized
>> env_t type.
>> Fix this by moving the include directive.
> 
> Hm... the fix seems wrong to me.  What happens now if ENV_IS_EMBEDDED
> is set, but CONFIG_BUILD_ENVCRC is _not_ set?

Well, you should look into that file...
it says:
#if defined(ENV_IS_EMBEDDED) && !defined(CONFIG_BUILD_ENVCRC)
# define CONFIG_BUILD_ENVCRC 1
#endif

So this will not be a problem.
As for the logic of it... I don't know what was the original intend.

> 
> Did this change pass a MAKEALL for example for ppc?

I don't have the tool chain for ppc.
I think the original fix has been tested by Stefano.
Is there a requirement to have all the supported
architectures tool chains, for submitting patches?

Stefano, Mike, can you, please, test it on PPC and Blackfin?

Thanks
Wolfgang Denk - Nov. 28, 2011, 3:15 p.m.
Dear Igor Grinberg,

In message <4ED34E9B.7040200@compulab.co.il> you wrote:
> 
> > Hm... the fix seems wrong to me.  What happens now if ENV_IS_EMBEDDED
> > is set, but CONFIG_BUILD_ENVCRC is _not_ set?
> 
> Well, you should look into that file...

It's faster for me to ask you to do that :-)

> > Did this change pass a MAKEALL for example for ppc?
> 
> I don't have the tool chain for ppc.
> I think the original fix has been tested by Stefano.
> Is there a requirement to have all the supported
> architectures tool chains, for submitting patches?

Not for all, but http://www.denx.de/wiki/U-Boot/Patches says "Please
also run MAKEALL for _at_least_one_other_architecture_ than the one you
made your modifications in."

Best regards,

Wolfgang Denk
Mike Frysinger - Nov. 28, 2011, 6:52 p.m.
On Mon, Nov 28, 2011 at 04:04, Igor Grinberg wrote:
> I don't have the tool chain for ppc.

http://dev.gentoo.org/~vapier/u-boot/
-mike
Mike Frysinger - Nov. 29, 2011, 4:25 a.m.
On Monday 28 November 2011 02:57:38 Igor Grinberg wrote:
> When ENV_IS_EMBEDDED is not set, but CONFIG_BUILD_ENVCRC is set,
> the environment.h file does not get included resulting in unrecognized
> env_t type.
> Fix this by moving the include directive.

seems to work for me now; thanks!
Tested-by: Mike Frysinger <vapier@gentoo.org>
-mike
Igor Grinberg - Nov. 29, 2011, 6:35 a.m.
On 11/28/11 20:52, Mike Frysinger wrote:
> On Mon, Nov 28, 2011 at 04:04, Igor Grinberg wrote:
>> I don't have the tool chain for ppc.
> 
> http://dev.gentoo.org/~vapier/u-boot/

10x Mike
Igor Grinberg - Nov. 29, 2011, 6:38 a.m.
On 11/28/11 17:15, Wolfgang Denk wrote:
> Dear Igor Grinberg,
> 
> In message <4ED34E9B.7040200@compulab.co.il> you wrote:
>>
>>> Hm... the fix seems wrong to me.  What happens now if ENV_IS_EMBEDDED
>>> is set, but CONFIG_BUILD_ENVCRC is _not_ set?
>>
>> Well, you should look into that file...
> 
> It's faster for me to ask you to do that :-)

I have already done that (before sending the patch) as soon as I saw
the email from Mike, reporting the problem.

> 
>>> Did this change pass a MAKEALL for example for ppc?
>>
>> I don't have the tool chain for ppc.
>> I think the original fix has been tested by Stefano.
>> Is there a requirement to have all the supported
>> architectures tool chains, for submitting patches?
> 
> Not for all, but http://www.denx.de/wiki/U-Boot/Patches says "Please
> also run MAKEALL for _at_least_one_other_architecture_ than the one you
> made your modifications in."

10x for looking into that for me ;-)
Igor Grinberg - Nov. 29, 2011, 6:41 a.m.
On 11/29/11 06:25, Mike Frysinger wrote:
> On Monday 28 November 2011 02:57:38 Igor Grinberg wrote:
>> When ENV_IS_EMBEDDED is not set, but CONFIG_BUILD_ENVCRC is set,
>> the environment.h file does not get included resulting in unrecognized
>> env_t type.
>> Fix this by moving the include directive.
> 
> seems to work for me now; thanks!
> Tested-by: Mike Frysinger <vapier@gentoo.org>

10x Mike - it is for Blackfin, right?

Wolfgang, can you apply the fix now?
Igor Grinberg - Dec. 5, 2011, 10:48 a.m.
ping!

This fixes a compile breakage and
IMO should be applied before 2011.12 is out.
Can someone, please apply it?

On 11/28/11 09:57, Igor Grinberg wrote:
> When ENV_IS_EMBEDDED is not set, but CONFIG_BUILD_ENVCRC is set,
> the environment.h file does not get included resulting in unrecognized
> env_t type.
> Fix this by moving the include directive.
> 
> Reported-by: Mike Frysinger <vapier@gentoo.org>
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> ---
>  tools/envcrc.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/tools/envcrc.c b/tools/envcrc.c
> index 111d9f6..51e3f54 100644
> --- a/tools/envcrc.c
> +++ b/tools/envcrc.c
> @@ -61,7 +61,6 @@
>  #endif	/* CONFIG_ENV_IS_IN_FLASH */
>  
>  #if defined(ENV_IS_EMBEDDED) && !defined(CONFIG_BUILD_ENVCRC)
> -# include <environment.h>
>  # define CONFIG_BUILD_ENVCRC 1
>  #endif
>  
> @@ -74,13 +73,14 @@
>  #define ENV_SIZE (CONFIG_ENV_SIZE - ENV_HEADER_SIZE)
>  
>  
> -extern uint32_t crc32 (uint32_t, const unsigned char *, unsigned int);
> -
>  #ifdef CONFIG_BUILD_ENVCRC
> +# include <environment.h>
>  extern unsigned int env_size;
>  extern env_t environment;
>  #endif	/* CONFIG_BUILD_ENVCRC */
>  
> +extern uint32_t crc32 (uint32_t, const unsigned char *, unsigned int);
> +
>  int main (int argc, char **argv)
>  {
>  #ifdef CONFIG_BUILD_ENVCRC
Wolfgang Denk - Dec. 6, 2011, 9:11 p.m.
Dear Igor Grinberg,

In message <1322467058-30532-1-git-send-email-grinberg@compulab.co.il> you wrote:
> When ENV_IS_EMBEDDED is not set, but CONFIG_BUILD_ENVCRC is set,
> the environment.h file does not get included resulting in unrecognized
> env_t type.
> Fix this by moving the include directive.
> 
> Reported-by: Mike Frysinger <vapier@gentoo.org>
> Signed-off-by: Igor Grinberg <grinberg@compulab.co.il>
> ---
>  tools/envcrc.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)

Applied, thanks.

Best regards,

Wolfgang Denk

Patch

diff --git a/tools/envcrc.c b/tools/envcrc.c
index 111d9f6..51e3f54 100644
--- a/tools/envcrc.c
+++ b/tools/envcrc.c
@@ -61,7 +61,6 @@ 
 #endif	/* CONFIG_ENV_IS_IN_FLASH */
 
 #if defined(ENV_IS_EMBEDDED) && !defined(CONFIG_BUILD_ENVCRC)
-# include <environment.h>
 # define CONFIG_BUILD_ENVCRC 1
 #endif
 
@@ -74,13 +73,14 @@ 
 #define ENV_SIZE (CONFIG_ENV_SIZE - ENV_HEADER_SIZE)
 
 
-extern uint32_t crc32 (uint32_t, const unsigned char *, unsigned int);
-
 #ifdef CONFIG_BUILD_ENVCRC
+# include <environment.h>
 extern unsigned int env_size;
 extern env_t environment;
 #endif	/* CONFIG_BUILD_ENVCRC */
 
+extern uint32_t crc32 (uint32_t, const unsigned char *, unsigned int);
+
 int main (int argc, char **argv)
 {
 #ifdef CONFIG_BUILD_ENVCRC