| Message ID | 1292630381-23022-2-git-send-email-yorksun@freescale.com |
|---|---|
| State | Changes Requested |
| Headers | show |
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
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
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
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
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
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
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
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
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
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
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
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
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) {
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(-)