Dear James Yang,
In message alpine.LRH.2.00.1301041608190.3906@ra8135-ec1.am.freescale.net you wrote:
Hm... this adds a special case and as such increases complexity - and what is the benefit for you?
The purpose is to avoid having to allocate memory for getenv_f() to
What exactly is the problem of adding a dynamic variable on the stack? This is a way cheaper operation than adding the code here...
work. While the unmodified getenv_f() does let me do that if I pass len=0, it has the side effect of printing a warning message that the buffer is too small. I want to avoid this message from being printed as well.
Then just provide a big enough buffer. You don't bother about a few bytes of stack space, do you? They cost you nothing...
Part 7 of the patchset runs at a point where memory can only be allocated from the stack. The stack is in cache, so any available RAM is precious. The function that calls getenv_f() calls another
This argument backfires - because if you detect that the variable is set, then you will call fsl_ddr_interactive(), which then will alocate a buffer (char buffer2[CONFIG_SYS_CBSIZE]) and call getenv_f() again, now for real.
Actually you now need TWO such buffers - see this snippet from your patch 7/7:
unsigned long long ddrsize; const char *prompt = "FSL DDR>"; char buffer[CONFIG_SYS_CBSIZE]; + char buffer2[CONFIG_SYS_CBSIZE]; + char *p = NULL; char *argv[CONFIG_SYS_MAXARGS + 1]; /* NULL terminated */ int argc; unsigned int next_step = STEP_GET_SPD;
I. e. before one such buffer was sufficient, now you need two - if you care about memory, then dumpt his patch, and leave the code as is - both your code and your stack footprint will be smaller.
function, so allocating a buffer with an unmodified getenv_f() would require the buffer to persist in the calling function's stack frame uselessly. That buffer is of size CONFIG_SYS_CBSIZE, which is either 256 or 1024, so I wouldn't call it non-critical.
But you do this anyway, just in another part of the code. ANd there you even need two such buffers now!
I imagine that with the modified getenv_f(), other pre-relocation features could be written to utilize the detection of environment variables in a similar fashion. This patch set by itself should not be considered as the sole usage case.
Well, the use case you present shows that while the idea sounds good initially, the results tend to be worse than the existing code.
You did not convince me that the addition is a good idea.
The description was not written to be a top-down procedural description. Maybe reordering like this will make it seem more correct?
This will not remove the inconsistent behaviour of returning a 1 in one case, indepoendent of the actual length of the value, and the length in another case. And there is no need for such an inconsistency.
if (!buf)return 1;I tend to NAK this part.
Would it be acceptable if it returns 0 instead? The reason I chose 1 is because all of the 100+ existing usages of getenv_f() check only for return value > 0. I was trying to make it consistent with all of those existing usage cases.
Why don't you implement consistent behaviour and always return the correct length of the variable value, and -1 if the variable does not exist?
Best regards,
Wolfgang Denk