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