Hi Wolfgang,
On Fri, Oct 25, 2019 at 09:06:44AM +0200, Wolfgang Denk wrote:
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...
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: wd@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_