Hi Wolfgang, On Fri, Oct 25, 2019 at 09:06:44AM +0200, Wolfgang Denk wrote: > Dear Takahiro, > > In message <20191023065332.ge10...@linaro.org> you wrote: > > > > This is my second ping. > > Could you please take time to review this patch? > > Sorry, I'm afraid I will not find the time to review any such > monster patch series any time soon. I hope Joe (added to Cc:) > has more resources available. > > Only a few comments below...
I won't and cannot make replies on every comment that you gave me below because we are very different at some basic points and other comments are just details. > > > > # In version 5 of this patch set, the implementation is changed again. > > > > # > > > > # I believe that this is NOT intrusive, and that my approach here is NOT > > > > # selfish at all. If Wolfgang doesn't accept this approach, however, > > 371 files changed, 3690 insertions(+), 2337 deletions(-) > > I don't know what your scales are, but for me such a patch is > extremely invasive. It affects a zillion of files in common code > plus a ton of board specific files. > > I did not find any information about the size impact or if the > modified code continues to build for all boards - I remember we have > a number of board with tight resources here and there. > > You should at least provide some information how much bigger the new > code gets. > > From a quick glance I think the patches are not cleanly separated - > you cannot change interfaces for the implementation in one step and > for the callers in another, as this breaks bisectability. As you noticed below, I'm aware of that. So first I wanted to know if you agree to my *basic* approach or not, not details, in order to go further, but still don't see yes or no below. > > My biggest concern is that such a highly invasive change cannot be > simply rubberstamped in a code review - I think this also needs > runtime testing on at least a significant number of the affected > boards. We should try to get help from at least some board > maintainers - maybe you should ask for help for such testing n the > board maintainers mailing list? > > > Please do not misunderstand me - I am not trying to block any of > this - I understand and appreciate the huge amount of efforts you > have put into this. But I feel this needs not only careful review, > but also actual testing on as many of the effected boards as > possible, and I simply don't have time for that. Okay. > > > > > * To access (get or set) a variable, associated context must be > > > > presented. > > > > So, almost of all existing env interfaces are changed to accept one > > > > extra argument, ctx. > > > > (I believe that this is Wolfgang's *requirement*.) > > I wonder if we really need to change all interfaces. I fear the > size impact might bite us. I only had a glimpse at the actual code, > but it seemed to me as if we were just pssing the same information > around everywhere. Could we not use GD nstead, for example? > > > > > * Non-volatile feature is not implemented in a general form and must be > > > > implemented by users in their sub-systems. > > I don't understand what this means, or why such a decision was made. > Which sub-systems do you have in mind here? UEFI sub-system/library. > What prevented you from implementing a solution to works for all of > us? > > > > > In version 4, U-Boot environment's attributes are extended to support > > > > non-volatile (or auto-save capability), but Wolfgang rejected > > > > my approach. > > > > As modifying attributes for this purpose would cause bunch of > > > > incompatibility issues (as far as I said in my cover letter and > > > > the discussions in ML), I would prefer a much simple approach. > > I think we still have a different opinion here, but I'm lacking time > for a thorough readding of the new code, so I hold back. I hope > that Joe can have a closer look... You don't have to worry about my previous versions. Just focus on the current v5. > > > > > * Each backing storage driver must be converted to be aligned with > > > > new env interfaces to handle multiple contexts in parallel and > > > > provide context-specific Kconfig configurations for driver parameters. > > > > > > > > In this version, only FAT file system and flash devices are supported, > > > > but it is quite straightforward to modify other drivers. > > If I see this correctly, there is a fundamental change in the > implementation before: Up to now, the environment seize on external > storage has been a compile time constant (CONFIG_ENV_SIZE). > > Now this value gets computed, and I'm not even sure if this is a > contant at run time. Yes, it's constant for "U-Boot environment" (== U-Boot variables) context. But for other sus-systems, in my case UEFI, the total size of storage for (UEFI) variables may be different, but still constant. > This scares me. Does this not break compatibility? How do you > upgrade a system from an older version of U-Boot to one with your > patches? > > > > > Known issues/restriction/TODO: > > > > * The current form of patchset is not 'bisect'able. > > > > Not to break 'bisect,' all the patches in this patch set must be > > > > put into a single commit when merging. > > > > (This can be mitigated by modifying/splitting Patch#18/#19 though.) > > OK, so you are aware of this problem. > > I must admit that I really hate this. If you could avoid all the API > changes, this would solve this problem, wouldn't it? "Avoid all the API changes" is an approach that I took in all my previous versions, but you *denied* it. That is: I proposed an approach in which the existing interfaces, env_get/set(), were maintained for existing users/sub-systems. Only new users who want to enjoy merits from new "context" feature may use new *extended* interfaces, env_[get|set]_ext(), in my case UEFI. As you *denied* it, this version (v5) is an inevitable result. Don't take me wrong, but I think that you made inconsistent comments. > > > > * Unfortunately, this code fails to read U-Boot environment from flash > > > > at boot time due to incomprehensible memory corruption. > > > > See murky workaround, which is marked as FIXME, in env/flash.c. > > Argh. This is a killing point, isn't it? > > You don't seriously expect to have patches which cause > "incomprehensible memory corruption" to be included into mainline? It will be just a matter of time for debugging. > > > > * The whole area of storage will be saved at *every* update of > > > > one UEFI variable. It should be optimized if possible. > > This is only true for UEFI variables, right? Yes. > > > > * An error during "save" operation may cause inconsistency between > > > > cache (hash table) and the storage. > > > > -> This is not UEFI specific though. > > Is this a new problem, or do you just mention this here for > completeness? We always had this issue, didn't we? As I said, "this is not UEFI specific." > > > > * I cannot test all the platforms affected by this patchset. > > Sure, so please seek help from the board maintainers. > > And please provide size statistics. > > > > > To enable this feature for example with FAT file system, the following > > > > configs must be enabled: > > > > CONFIG_ENV_IS_IN_FAT > > > > CONFIG_ENV_FAT_INTERFACE > > > > CONFIG_ENV_EFI_FAT_DEVICE_AND_PART > > > > CONFIG_ENV_EFI_FAT_FILE > > How much testing can be done on boards that don't use FAT to store > the environment? As I said, > > > > In this version, only FAT file system and flash devices are supported, > > > > but it is quite straightforward to modify other drivers. If you don't think my patch is not qualified for a "PATCH" for some reason, I will sent one as "RFC" from the next version. I don't care. Thanks, -Takahiro Akashi > > Sorry that I can't be of any better help here... > > Best regards, > > Wolfgang Denk > > -- > DENX Software Engineering GmbH, Managing Director: Wolfgang Denk > HRB 165235 Munich, Office: Kirchenstr.5, D-82194 Groebenzell, Germany > Phone: (+49)-8142-66989-10 Fax: (+49)-8142-66989-80 Email: w...@denx.de > 'What shall we do?' said Twoflower. 'Panic?' said Rincewind hope- > fully. He always held that panic was the best means of survival; back > in the olden days, his theory went, people faced with hungry sabre- > toothed tigers could be divided very simply in those who panicked and > those who stood there saying 'What a magnificent brute!' or 'Here, > pussy.' - Terry Pratchett, _The Light Fantastic_ _______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
