[U-Boot] [PATCH 0/6] Support NAND in fw_printenv/fw_setenv

[Wolfgang Denk <wd@denx.de>]

Dear Guennadi Liakhovetski,

In message <Pine.LNX.4.64.0809020104080.8933@axis700.grange> you wrote:
> 
> As you know, in the tool we have to decide at run-time whether we are 
> dealing  with a single environment copy or with current / redundant 
> configuration. With NAND support when _writing_ environment to NAND you 
> have to write page at a time, which means, at this point we _must_ have 
> the image in a contiguous buffer in RAM. And the image can have one of the 

Agreed so far.

> two possible formats - with and without the "flags" byte. In principle I 
> see only three possibilities to implement this:
> 
> (a) work with arbitrary non-contiguous data in RAM as before, and copy it 
> into an additional buffer just before writing to NAND
> 
> advantage: can keep the current struct
> 
> disadvantage: extra malloc

Agreed.

> (b) use a plain data buffer, and, if needed, use the first byte in it for 
> flags
> 
> advantage: no extra malloc, no (explicit) union
> 
> disadvantage: confusing, have to work with byte-offsets instead of struct 
> / union members, and, in fact, this is the same as using a union, just 
> implicit, calculating byte-offsets manually, instead of letting the 
> compiler do it

Agreed that this is even worse than a union.

> (c) use a union
> 
> advantage: clean access to all environment fields without the use of 
> byte-offsets
> 
> disadvantage: slightly more complex code
> 
> Please, just tell me which of these three you would prefer, or maybe there 

None of them, I think.

> is a fourth possibility I am still overseeing.

The  two  cases  (redundant  versus  non-redundant  env)   are   well
separated,  and  known  early  (after  parsing the config file, i. e.
before any processing of environment data).

How about defining two structs,  one  without  the  flag  byte  (non-
redundant   env),  and  another  one  with  the  flag  byte  included
(redundant env). Then just use a pointer of the correct type (either
first or second struct) to access the data.

> You also asked about the extra "char *data" pointer in the struct 
> environment, whether there is no danger that a different compiler version 
> will break it. This pointer uses no magic - it is just a plain simple 
> pointer, I use it to point to data inside the union to avoid having to 
> check every time whether we have the redundant environment or not. So, I 
> check it only once at initialisation time, set this pointer, and then just 
> use it to access the data buffer inside the image (union). No magic here.

But it's bogus. Now you have data[] in the union, *plus* in the
struct. You have it twice.

> > > 4. fix MTD_OLD
> > > 
> > > Would we still need this with NAND-only tool?
> > 
> > Yes of course we need it. I will not accept such thing as a NAND-only tool.
> 
> Ok. But NAND-support is not needed with MTD_OLD? So, if it cannot be 
> compiled with older kernels, we may just disable it per ifdef?

Do we really need to ifdef this? 

> How would you like to make such a replacement then? If I produce a patch 
> just from the current state to the final state, I think, it will look 
> worse than the broken-down patch-series. Otherwise we could remove the 

It will not. See my previous statistics. It will be  less  than  half
the size of your split patches.


Best regards,

Wolfgang Denk

-- 
DENX Software Engineering GmbH,     MD: Wolfgang Denk & Detlev Zundel
HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany
Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: wd at denx.de
This is now.  Later is later.