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
signature.asc
Description: PGP signature
_______________________________________________ U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
