Dear Guennadi Liakhovetski, In message <[EMAIL PROTECTED]> you wrote: > > This union replaces the typedef env_t, which was also defined in fw_env.c. > Thus I was not fixing the issue you describe above, which I fully agree > with - the tool and u-boot should ideally use the same definition from a > common header. I just did not address this issue in this patch series.
This is not correct. You are creating this issue. So far, U-Boot and fw_env used to use the same (or at least equivalent) definitions. And I have to admit that I'm not a friend of using unions. They are a great way to obfuscate code. > > I think both sets of functions should use the same set of definitions > > (yes, I am aware that this requires chnages to "include/environ- > > ment.h"). > > Would be good, yes, the question is - do we want to do this now and do we > want to do this in the scope of this patch series? If there is no real need to change it now, then leave it unchanged? > > Please stick with the typedef. ... > and Linux explicitly discourages typedef. They also produce warnings by > checkpatch.sh. If this is also different in U-Boot, no problem, can change > back. Ah! And why didn't you mention this in the patch? For a reviewer it is very important to understand why you are modifying code. > > > - /* Update CRC */ > > > - environment.crc = crc32 (0, (uint8_t*) environment.data, ENV_SIZE); > > > + /* > > > + * Update CRC: it is at the same location with and without the > > > + * redundant environment > > > + */ > > > + environment.image->single.crc = crc32 (0, (uint8_t *) environment.data, > > > + ENV_SIZE); > > > > The comment is wrong. Without redundant environment, the CRC is at > > offset 4 from the start of the environment storage, and with > > redundancy it's at offset 5. This is definitely not the same location. > > I think, CRC is always at offset 0, then follows the (optional) flag byte, Yes, you are right. > and then comes the data. I also see this with binary dumps of the > environment. Otherwise what takes the first four bytes? Or did you mean > the data which CRC is calculated is at offset 4 or 5? I think, the comment > is correct. And we see the obfuscation caused by using a union. > > Also, the code does not match the commend, since you access > > "single.crc" which has no relation to redundant environment > > at all. > > That's exactly the reason - the comment explains, that the crc is at the > same location, so, you can access it using single.crc in both cases. This is ugly, plain ugly. > > Looking at the rest of the code I see no improvement in using the > > union versus what we did before - the code is even longer now, and > > (slightly) less readable to me. > > I need it to be able to copy the whole environment image, including the > crc and the optional flag with one read / write / memcpy operation. And Hm... why do you need to do this in a single read operation? Where's the difference between reading it all at once or in several smaller chunks? > this is also an advantage even on NOR - no need for two or three syscalls, > the whole image is read / written with one syscall. A big win, really ;-) 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: [EMAIL PROTECTED] How long does it take a DEC field service engineer to change a lightbulb? It depends on how many bad ones he brought with him. _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de http://lists.denx.de/mailman/listinfo/u-boot
