[U-Boot] [PATCH v5 00/19] efi_loader: non-volatile variables support

[Wolfgang Denk <wd@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_