Patchwork [U-Boot,v2,2/9] make the hwconfig buffer deeper

login
register
mail settings
Submitter York Sun
Date Dec. 17, 2010, 11:59 p.m.
Message ID <1292630381-23022-2-git-send-email-yorksun@freescale.com>
Download mbox | patch
Permalink /patch/76041/
State Changes Requested
Headers show

Comments

York Sun - Dec. 17, 2010, 11:59 p.m.
To temporarily fix buffer issue when running at flash, use bigger buffer
to push down the stack deeper.

Signed-off-by: York Sun <yorksun@freescale.com>
---
 common/hwconfig.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Wolfgang Denk - Dec. 18, 2010, 10:25 p.m.
Dear York Sun,

In message <1292630381-23022-2-git-send-email-yorksun@freescale.com> you wrote:
> To temporarily fix buffer issue when running at flash, use bigger buffer
> to push down the stack deeper.
> 
> Signed-off-by: York Sun <yorksun@freescale.com>

Please explain 1) why this is needed and 2) what it has to do with
running "at" (from?) flash?

hwconfig is intended for simple h/w configurations.  In which case do
you need more than 128 characters for such settings?  And is this
really needed / reasonable?

Best regards,

Wolfgang Denk
York Sun - Jan. 3, 2011, 8:08 p.m.
On Sat, 2010-12-18 at 16:25 -0600, Wolfgang Denk wrote:
> Dear York Sun,
> 
> In message <1292630381-23022-2-git-send-email-yorksun@freescale.com> you wrote:
> > To temporarily fix buffer issue when running at flash, use bigger buffer
> > to push down the stack deeper.
> >
> > Signed-off-by: York Sun <yorksun@freescale.com>
> 
> Please explain 1) why this is needed and 2) what it has to do with
> running "at" (from?) flash?
> 
> hwconfig is intended for simple h/w configurations.  In which case do
> you need more than 128 characters for such settings?  And is this
> really needed / reasonable?

First of all, I don't need more than 128 characters for hwconfig. I
think the buggy code makes the buffer less usable.

If you look into the common/hwconfig.c, you will notice the _current_
code uses the stack as temporary memory to hold the variable. Even I
don't agree with this way, I don't have a quick fix either. There may be
two ways or more to fix it. One is to create wrapper functions to deal
with pre-relocation and post-relocation functions. Another way is to
allocate more space in gd. Which way do you think is better?

York
Wolfgang Denk - Jan. 3, 2011, 9:11 p.m.
Dear York Sun,

In message <1294085287.24386.52.camel@oslab-l1> you wrote:
>
> > Please explain 1) why this is needed and 2) what it has to do with
> > running "at" (from?) flash?
> > 
> > hwconfig is intended for simple h/w configurations.  In which case do
> > you need more than 128 characters for such settings?  And is this
> > really needed / reasonable?
> 
> First of all, I don't need more than 128 characters for hwconfig. I
> think the buggy code makes the buffer less usable.

Hm... I have no idea whish sort of "buggy code" you are referring to
here, but in this case we shoul start and fix that buggy code, right?

> If you look into the common/hwconfig.c, you will notice the _current_
> code uses the stack as temporary memory to hold the variable. Even I
> don't agree with this way, I don't have a quick fix either. There may be

What's wrong with putting it on the stack?  This prevents a permanent
allocation, so the memory can easily be reused once that function
returns.

> two ways or more to fix it. One is to create wrapper functions to deal
> with pre-relocation and post-relocation functions. Another way is to
> allocate more space in gd. Which way do you think is better?

You fail to explain why we should change anything when you "don't need
more than 128 characters for hwconfig" in the first place?

Best regards,

Wolfgang Denk
York Sun - Jan. 3, 2011, 9:26 p.m.
On Mon, 2011-01-03 at 22:11 +0100, Wolfgang Denk wrote:
> Dear York Sun,
> 
> In message <1294085287.24386.52.camel@oslab-l1> you wrote:
> >
> > > Please explain 1) why this is needed and 2) what it has to do with
> > > running "at" (from?) flash?
> > > 
> > > hwconfig is intended for simple h/w configurations.  In which case do
> > > you need more than 128 characters for such settings?  And is this
> > > really needed / reasonable?
> > 
> > First of all, I don't need more than 128 characters for hwconfig. I
> > think the buggy code makes the buffer less usable.
> 
> Hm... I have no idea whish sort of "buggy code" you are referring to
> here, but in this case we shoul start and fix that buggy code, right?

Making a buffer in a function's stack and make use of it out of the
scope, that's the buggy code I am referring. 

> 
> > If you look into the common/hwconfig.c, you will notice the _current_
> > code uses the stack as temporary memory to hold the variable. Even I
> > don't agree with this way, I don't have a quick fix either. There may be
> 
> What's wrong with putting it on the stack?  This prevents a permanent
> allocation, so the memory can easily be reused once that function
> returns.

How can the content be guaranteed? The stack can be trashed when the
function returns.

> 
> > two ways or more to fix it. One is to create wrapper functions to deal
> > with pre-relocation and post-relocation functions. Another way is to
> > allocate more space in gd. Which way do you think is better?
> 
> You fail to explain why we should change anything when you "don't need
> more than 128 characters for hwconfig" in the first place?

I don't need more than 128 characters. However, the stack is trashed by
other functions. I have to push the stack deeper to keep the content
unchanged. I am not happy with it.

York
Wolfgang Denk - Jan. 3, 2011, 9:47 p.m.
Dear York Sun,

In message <1294089991.24386.58.camel@oslab-l1> you wrote:
>
> > Hm... I have no idea whish sort of "buggy code" you are referring to
> > here, but in this case we shoul start and fix that buggy code, right?
> 
> Making a buffer in a function's stack and make use of it out of the
> scope, that's the buggy code I am referring. 

That is bad indeed, and needs to be fixed.

> > You fail to explain why we should change anything when you "don't need
> > more than 128 characters for hwconfig" in the first place?
> 
> I don't need more than 128 characters. However, the stack is trashed by
> other functions. I have to push the stack deeper to keep the content
> unchanged. I am not happy with it.

Now I understand what you mean.

But we do not need a bigger buffer, or store another copy of the whole
content of the "hwconfig" variable - we just need room to for the
return value of hwconfig_parse(), which usually should be (much)
smaller than the content of the "hwconfig" variable.

Best regards,

Wolfgang Denk
Kumar Gala - Jan. 7, 2011, 3:14 p.m.
On Jan 3, 2011, at 3:47 PM, Wolfgang Denk wrote:

> Dear York Sun,
> 
> In message <1294089991.24386.58.camel@oslab-l1> you wrote:
>> 
>>> Hm... I have no idea whish sort of "buggy code" you are referring to
>>> here, but in this case we shoul start and fix that buggy code, right?
>> 
>> Making a buffer in a function's stack and make use of it out of the
>> scope, that's the buggy code I am referring. 
> 
> That is bad indeed, and needs to be fixed.

Where is that happening?

>>> You fail to explain why we should change anything when you "don't need
>>> more than 128 characters for hwconfig" in the first place?
>> 
>> I don't need more than 128 characters. However, the stack is trashed by
>> other functions. I have to push the stack deeper to keep the content
>> unchanged. I am not happy with it.
> 
> Now I understand what you mean.
> 
> But we do not need a bigger buffer, or store another copy of the whole
> content of the "hwconfig" variable - we just need room to for the
> return value of hwconfig_parse(), which usually should be (much)
> smaller than the content of the "hwconfig" variable.

I agree with WD, I'm confused.  Can you provide the hwconfig string you are using that has issues with 128 but works at 256.

- k
York Sun - Jan. 7, 2011, 4:49 p.m.
On Fri, 2011-01-07 at 09:14 -0600, Kumar Gala wrote:
> On Jan 3, 2011, at 3:47 PM, Wolfgang Denk wrote:
> 
> > Dear York Sun,
> > 
> > In message <1294089991.24386.58.camel@oslab-l1> you wrote:
> >> 
> >>> Hm... I have no idea whish sort of "buggy code" you are referring to
> >>> here, but in this case we shoul start and fix that buggy code, right?
> >> 
> >> Making a buffer in a function's stack and make use of it out of the
> >> scope, that's the buggy code I am referring. 
> > 
> > That is bad indeed, and needs to be fixed.
> 
> Where is that happening?
> 
> >>> You fail to explain why we should change anything when you "don't need
> >>> more than 128 characters for hwconfig" in the first place?
> >> 
> >> I don't need more than 128 characters. However, the stack is trashed by
> >> other functions. I have to push the stack deeper to keep the content
> >> unchanged. I am not happy with it.
> > 
> > Now I understand what you mean.
> > 
> > But we do not need a bigger buffer, or store another copy of the whole
> > content of the "hwconfig" variable - we just need room to for the
> > return value of hwconfig_parse(), which usually should be (much)
> > smaller than the content of the "hwconfig" variable.
> 
> I agree with WD, I'm confused.  Can you provide the hwconfig string you are using that has issues with 128 but works at 256.
> 
> - k

fsl_ddr:ctlr_intlv=cacheline,bank_intlv=cs0_cs1_cs2_cs3,ecc=off,addr_hash=false

With the buffer size 128, the sub function gets
"ctlr_intlv=cacheline,bank_intlv=cs0_". It is not because the buffer is
not enough, but the stack has been trashed. I have debug log if you need
it.

York
Wolfgang Denk - Jan. 7, 2011, 5:50 p.m.
Dear Kumar Gala,

In message <740DF509-6661-4611-BE1B-BBB629012697@kernel.crashing.org> you wrote:
> 
> >> Making a buffer in a function's stack and make use of it out of the
> >> scope, that's the buggy code I am referring. 
> > 
> > That is bad indeed, and needs to be fixed.
>
> Where is that happening?

__hwconfig() calls hwconfig_parse() an passes a local stack buffer
(buf) as argument; hwconfig_parse() returns a pointer into this
buffer; then __hwconfig() returns the return value from
hwconfig_parse() - but here the local stack buffer gots out of scope.


Best regards,

Wolfgang Denk
Wolfgang Denk - Jan. 7, 2011, 5:52 p.m.
Dear York Sun,

In message <1294418957.8466.8.camel@oslab-l1> you wrote:
>
> fsl_ddr:ctlr_intlv=cacheline,bank_intlv=cs0_cs1_cs2_cs3,ecc=off,addr_hash=false
> 
> With the buffer size 128, the sub function gets
> "ctlr_intlv=cacheline,bank_intlv=cs0_". It is not because the buffer is
> not enough, but the stack has been trashed. I have debug log if you need
> it.

This is yet another problem, then.

Best regards,

Wolfgang Denk
Kumar Gala - Jan. 7, 2011, 11:23 p.m.
On Jan 7, 2011, at 11:52 AM, Wolfgang Denk wrote:

> Dear York Sun,
> 
> In message <1294418957.8466.8.camel@oslab-l1> you wrote:
>> 
>> fsl_ddr:ctlr_intlv=cacheline,bank_intlv=cs0_cs1_cs2_cs3,ecc=off,addr_hash=false
>> 
>> With the buffer size 128, the sub function gets
>> "ctlr_intlv=cacheline,bank_intlv=cs0_". It is not because the buffer is
>> not enough, but the stack has been trashed. I have debug log if you need
>> it.
> 
> This is yet another problem, then.

So I figured the problem out and need to rework the hwconfig API to fix it.

We need some new versions of the APIs that take a buffer for use early before boot.  

- k
Kumar Gala - Jan. 9, 2011, 9 p.m.
On Jan 7, 2011, at 5:23 PM, Kumar Gala wrote:

> 
> On Jan 7, 2011, at 11:52 AM, Wolfgang Denk wrote:
> 
>> Dear York Sun,
>> 
>> In message <1294418957.8466.8.camel@oslab-l1> you wrote:
>>> 
>>> fsl_ddr:ctlr_intlv=cacheline,bank_intlv=cs0_cs1_cs2_cs3,ecc=off,addr_hash=false
>>> 
>>> With the buffer size 128, the sub function gets
>>> "ctlr_intlv=cacheline,bank_intlv=cs0_". It is not because the buffer is
>>> not enough, but the stack has been trashed. I have debug log if you need
>>> it.
>> 
>> This is yet another problem, then.
> 
> So I figured the problem out and need to rework the hwconfig API to fix it.
> 
> We need some new versions of the APIs that take a buffer for use early before boot.  

I've posted:

http://patchwork.ozlabs.org/patch/78045/
http://patchwork.ozlabs.org/patch/78046/

Which should resolve the hwconfig issue and not require the patch to bump the size up.

- k
York Sun - Jan. 10, 2011, 6:39 p.m.
On Sun, 2011-01-09 at 15:00 -0600, Kumar Gala wrote:
> On Jan 7, 2011, at 5:23 PM, Kumar Gala wrote:
> 
> > 
> > On Jan 7, 2011, at 11:52 AM, Wolfgang Denk wrote:
> > 
> >> Dear York Sun,
> >> 
> >> In message <1294418957.8466.8.camel@oslab-l1> you wrote:
> >>> 
> >>> fsl_ddr:ctlr_intlv=cacheline,bank_intlv=cs0_cs1_cs2_cs3,ecc=off,addr_hash=false
> >>> 
> >>> With the buffer size 128, the sub function gets
> >>> "ctlr_intlv=cacheline,bank_intlv=cs0_". It is not because the buffer is
> >>> not enough, but the stack has been trashed. I have debug log if you need
> >>> it.
> >> 
> >> This is yet another problem, then.
> > 
> > So I figured the problem out and need to rework the hwconfig API to fix it.
> > 
> > We need some new versions of the APIs that take a buffer for use early before boot.  
> 
> I've posted:
> 
> http://patchwork.ozlabs.org/patch/78045/
> http://patchwork.ozlabs.org/patch/78046/
> 
> Which should resolve the hwconfig issue and not require the patch to bump the size up.

I have confirmed your patches. I will drop this patch and resubmit the
rest.

York

Patch

diff --git a/common/hwconfig.c b/common/hwconfig.c
index 3c9759f..1b33d95 100644
--- a/common/hwconfig.c
+++ b/common/hwconfig.c
@@ -71,7 +71,7 @@  next:
 const char *cpu_hwconfig __attribute__((weak));
 const char *board_hwconfig __attribute__((weak));
 
-#define HWCONFIG_PRE_RELOC_BUF_SIZE	128
+#define HWCONFIG_PRE_RELOC_BUF_SIZE	256
 
 static const char *__hwconfig(const char *opt, size_t *arglen)
 {