From mboxrd@z Thu Jan 1 00:00:00 1970 From: Wolfgang Denk Date: Fri, 25 Oct 2019 09:06:44 +0200 Subject: [U-Boot] [PATCH v5 00/19] efi_loader: non-volatile variables support In-Reply-To: <20191023065332.GE10448@linaro.org> References: <20190905082133.18996-1-takahiro.akashi@linaro.org> <20191001062829.GA18778@linaro.org> <20191023065332.GE10448@linaro.org> Message-ID: <20191025070644.4F34D2400D6@gemini.denx.de> List-Id: MIME-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit To: u-boot@lists.denx.de Dear Takahiro, In message <20191023065332.GE10448@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... > > > # 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. 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. > > > * 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? 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... > > > * 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. 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? > > > * 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? > > > * 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? > > > * 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? > > > * 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? 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: wd at 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_