[U-Boot] [PATCH v5 00/19] efi_loader: non-volatile variables support
wd at denx.de
Fri Oct 25 07:06:44 UTC 2019
In message <20191023065332.GE10448 at 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
>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
> > > 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
> > > 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
Sorry that I can't be of any better help here...
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_
More information about the U-Boot