[U-Boot] [PATCH 03/14] env: Pass additional parameters to the env lookup function

[Maxime Ripard]

Hi Andre,

On Tue, Dec 05, 2017 at 10:09:04AM +0000, Andre Przywara wrote:
> On 28/11/17 10:24, Maxime Ripard wrote:
> > In preparation for the multiple environment support, let's introduce two
> > new parameters to the environment driver lookup function: the priority and
> > operation.
> > 
> > The operation parameter is meant to identify, obviously, the operation you
> > might want to perform on the environment.
> 
> I wonder if this is a bit too generic. Wouldn't a simple:
> 	bool is_save_op (or some more clever name)
> be sufficient? At least this is actually all we need, if I understand
> the code correctly.
> But it's up to you, I can see advantages for both approaches.

I'm not really sure what you meant, but I'll try to explain the
various constraints we have:

  - We need to init all the environments, no matter their priority,
    without any particular ordering

  - We need to load the environments by descending order of priority,
    until a valid one is found. I guess get_char is in the same
    situation, but I'm not really sure what it's supposed to be doing.

  - We need to store the environments. Now, this gets tricky. The
    current code only implement two cases: you store the environment
    where you loaded it from, which is the current case, and forcing
    always the same order, like we did on sunxi.

    But you might envision without pulling your hairs too much a
    situation for example that we would have loaded our environment
    from a read-only location (either because it is physically
    read-only, or because you cannot modify it (ie. signed
    environments)). In this case, you will need to implement a
    fallback strategy that is probably going to be by board, probably
    along the line of trying to save in our environments by ascending
    order, starting with the environment we loaded from.

tl; dr: we might need that flexibility for future use cases that are
not clear yet.

> >  int env_load(void)
> >  {
> > -	struct env_driver *drv = env_driver_lookup();
> > -	int ret = 0;
> > +	struct env_driver *drv;
> > +	int prio;
> >  
> > -	if (!drv)
> > -		return -ENODEV;
> > -	if (!drv->load)
> > -		return 0;
> > -	ret = drv->load();
> > -	if (ret) {
> > -		debug("%s: Environment failed to load (err=%d)\n", __func__,
> > -		      ret);
> > -		return ret;
> > +	for (prio = 0; (drv = env_driver_lookup(ENVO_LOAD, prio)); prio++) {
> > +		int ret;
> > +
> > +		if (!drv->load)
> > +			continue;
> 
> Is this change to the algorithm acceptable? Formerly we returned 0 on
> finding a drv->load pointer to be NULL, now it's -ENODEV, plus we try to
> find other sources first.

I couldn't really find a better way to deal with this.

You're right that the current algorithm is that if we don't have a
driver, return ENODEV, and if we don't have a load function, return 0.

When dealing with multiple enviroments, you would obviously have to
return -ENODEV if you didn't find any, but I couldn't find a proper
way to decide when we would return 0: should we return 0 when no
environment has a load function? only the first one we encounter?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20171208/c6622849/attachment.sig>