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

[Wolfgang Denk <wd@denx.de>]

Dear Takahiro,

In message <20191025075645.GJ10448@linaro.org> you wrote:
>
> 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.

Can you please at least comment on the size impact?  How much does
the code size grow on everage, especially for SPL?


> 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.

To be honest: when I saw your monster patch series which basically
touches every piece of code in U-Boot, I felt a strong temptation to
just send a NAK and be done with it.  But I know this would not be
fair.  But to be able to say yes I would need days to review the
code and probably run some tests myself, and I don;t have that time.

So I can neither say yes or no, sorry.

> > 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?

This is a point which is important to me.  We need at least a few
"Tested-by" credits...

> > > > > * 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 needs to be done to have this - say - for U-Boot context?

> > What prevented you from implementing a solution to works for all of
> > us?

?

> > > > > 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.

I think you misunderstand. If we just need the same pointer in all
functions dealing with the environment, there are at least two ways
to implement this: we can add it as an argument to each and every
function call; this will blow up code size and also impact execution
speed.  Or we can add it to some (private or public) data structure
that is visible everywhere.  In the simplest case we could add such
a pointer to the global data (GD) structure as I suggested in my
previous mail.  this would allow you to use basically the same code
as now, but without needing to change all the argument lists.  In
the result, you could drop your modifications of all common and
board specififc files.  The code changes would be concentrated on
the environment code, and it should be anle to submit bisectable
patches again.

Yes, global variables have disadvantages, too, but does it not make
sense here?  I think we will have only one active environment
context at any time in U-Boot, so this seems to be at least a lesser
evil than the zillion of changes of all call arguments.


> > > > > * 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.

It might be difficult to find willing testers under such
circumstances.  I would not want to run code with serious known
bugs.

> > > > > * 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."

This does not answer my question.  Are you just refering to the
general problem that a write to the persistent storage area might
fail, which has ever been present and which is inherently
unfixxable, or does your new code add any additional problems of
this kind?

> > 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.

I think this is a new feature that needs to be tested.  You focus on
UEFI + FAT and I don't know if UEFI + non-FAT makes sense, but at
least (1) non-UEFI + FAT and (2) non-UEFI + non-FAT are
configurations which must be tested - absolute minimum is at least
one example implementation. My suggestion would be to use something
that is not file system based, but using plain storage, say a board
which has the environment in SPI NOR flash.

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
Our OS who art in CPU, UNIX be thy name.
Thy programs run, thy syscalls done,
In kernel as it is in user!