[v2,13/15] env: Mark env_get_location as weak

Message ID ac37d27bda88fdf53f4c670f4457e9dca44447ea.1516094113.git-series.maxime.ripard@free-electrons.com
State New
Headers show
Series
  • env: Multiple env support and env transition for sunxi
Related show

Commit Message

Maxime Ripard Jan. 16, 2018, 9:16 a.m.
Allow boards and architectures to override the default environment lookup
code by overriding env_get_location.

Reviewed-by: Andre Przywara <andre.przywara@arm.com>
Reviewed-by: Lukasz Majewski <lukma@denx.de>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 env/env.c | 20 +++++++++++++++++++-
 1 file changed, 19 insertions(+), 1 deletion(-)

Comments

Simon Glass Jan. 17, 2018, 10:07 p.m. | #1
Hi Maxime,

On 16 January 2018 at 01:16, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Allow boards and architectures to override the default environment lookup
> code by overriding env_get_location.
>
> Reviewed-by: Andre Przywara <andre.przywara@arm.com>
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/env.c | 20 +++++++++++++++++++-
>  1 file changed, 19 insertions(+), 1 deletion(-)
>

I still don't really understand why this needs to be a weak function.
If the board knows the priority order, can it not put it into
global_data? We could have a little u8 array of 4 items with a
terminator?

> diff --git a/env/env.c b/env/env.c
> index 75da2b921804..6d0ab8ebe1a4 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -79,7 +79,25 @@ static void env_set_inited(enum env_location location)
>
>  static enum env_location env_load_location;
>
> -static enum env_location env_get_location(enum env_operation op, int prio)
> +/**
> + * env_get_location() - Returns the best env location for a board
> + * @op: operations performed on the environment
> + * @prio: priority between the multiple environments, 0 being the
> + *        highest priority
> + *
> + * This will return the preferred environment for the given
> + * priority. This is overridable by boards if they need to.
> + *
> + * All implementations are free to use the operation, the priority and
> + * any other data relevant to their choice, but must take into account
> + * the fact that the lowest prority (0) is the most important location
> + * in the system. The following locations should be returned by order
> + * of descending priorities, from the highest to the lowest priority.
> + *
> + * Returns:
> + * an enum env_location value on success, a negative error code otherwise
> + */
> +__weak enum env_location env_get_location(enum env_operation op, int prio)
>  {
>         switch (op) {
>         case ENVOP_GET_CHAR:
> --
> git-series 0.9.1

Regards,
Simon
Maxime Ripard Jan. 18, 2018, 5:21 p.m. | #2
Hi Simon,

On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote:
> On 16 January 2018 at 01:16, Maxime Ripard

> <maxime.ripard@free-electrons.com> wrote:

> > Allow boards and architectures to override the default environment lookup

> > code by overriding env_get_location.

> >

> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>

> > Reviewed-by: Lukasz Majewski <lukma@denx.de>

> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

> > ---

> >  env/env.c | 20 +++++++++++++++++++-

> >  1 file changed, 19 insertions(+), 1 deletion(-)

> >

> 

> I still don't really understand why this needs to be a weak function.

> If the board knows the priority order, can it not put it into

> global_data? We could have a little u8 array of 4 items with a

> terminator?


Sure that would be simpler, but that would also prevent us from doing
"smart" things based on data other than just whether the previous
environment is usable. Things based for example on a GPIO state, or a
custom algorithm to transition (or duplicate) the environment.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Simon Glass Jan. 22, 2018, 12:29 a.m. | #3
Hi Maxime,

On 18 January 2018 at 10:21, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi Simon,
>
> On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote:
>> On 16 January 2018 at 01:16, Maxime Ripard
>> <maxime.ripard@free-electrons.com> wrote:
>> > Allow boards and architectures to override the default environment lookup
>> > code by overriding env_get_location.
>> >
>> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>> > Reviewed-by: Lukasz Majewski <lukma@denx.de>
>> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> > ---
>> >  env/env.c | 20 +++++++++++++++++++-
>> >  1 file changed, 19 insertions(+), 1 deletion(-)
>> >
>>
>> I still don't really understand why this needs to be a weak function.
>> If the board knows the priority order, can it not put it into
>> global_data? We could have a little u8 array of 4 items with a
>> terminator?
>
> Sure that would be simpler, but that would also prevent us from doing
> "smart" things based on data other than just whether the previous
> environment is usable. Things based for example on a GPIO state, or a
> custom algorithm to transition (or duplicate) the environment.

In that case the board could read the GPIO state, or the algorithm,
and then set up the value.

Basically I am saying it could set up the priority order in advance of
it being needed, rather than having a callback.

Regards,
Simon
Maxime Ripard Jan. 22, 2018, 12:46 p.m. | #4
Hi,

On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote:
> > On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote:

> >> On 16 January 2018 at 01:16, Maxime Ripard

> >> <maxime.ripard@free-electrons.com> wrote:

> >> > Allow boards and architectures to override the default environment lookup

> >> > code by overriding env_get_location.

> >> >

> >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>

> >> > Reviewed-by: Lukasz Majewski <lukma@denx.de>

> >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

> >> > ---

> >> >  env/env.c | 20 +++++++++++++++++++-

> >> >  1 file changed, 19 insertions(+), 1 deletion(-)

> >> >

> >>

> >> I still don't really understand why this needs to be a weak function.

> >> If the board knows the priority order, can it not put it into

> >> global_data? We could have a little u8 array of 4 items with a

> >> terminator?

> >

> > Sure that would be simpler, but that would also prevent us from doing

> > "smart" things based on data other than just whether the previous

> > environment is usable. Things based for example on a GPIO state, or a

> > custom algorithm to transition (or duplicate) the environment.

> 

> In that case the board could read the GPIO state, or the algorithm,

> and then set up the value.

> 

> Basically I am saying it could set up the priority order in advance of

> it being needed, rather than having a callback.


Aren't we kind of stuck here?

On the previous iterations, we already discussed this and Tom
eventually told he was in favour of __weak functions, and the
discussion stopped there. I assumed you were ok with it.

I'd really want to move forward on that. This is something that is
really biting us *now* and I'd hate to miss yet another merge window
because of debates like this.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Tom Rini Jan. 22, 2018, 12:49 p.m. | #5
On Mon, Jan 22, 2018 at 01:46:46PM +0100, Maxime Ripard wrote:
> Hi,

> 

> On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote:

> > > On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote:

> > >> On 16 January 2018 at 01:16, Maxime Ripard

> > >> <maxime.ripard@free-electrons.com> wrote:

> > >> > Allow boards and architectures to override the default environment lookup

> > >> > code by overriding env_get_location.

> > >> >

> > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>

> > >> > Reviewed-by: Lukasz Majewski <lukma@denx.de>

> > >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

> > >> > ---

> > >> >  env/env.c | 20 +++++++++++++++++++-

> > >> >  1 file changed, 19 insertions(+), 1 deletion(-)

> > >> >

> > >>

> > >> I still don't really understand why this needs to be a weak function.

> > >> If the board knows the priority order, can it not put it into

> > >> global_data? We could have a little u8 array of 4 items with a

> > >> terminator?

> > >

> > > Sure that would be simpler, but that would also prevent us from doing

> > > "smart" things based on data other than just whether the previous

> > > environment is usable. Things based for example on a GPIO state, or a

> > > custom algorithm to transition (or duplicate) the environment.

> > 

> > In that case the board could read the GPIO state, or the algorithm,

> > and then set up the value.

> > 

> > Basically I am saying it could set up the priority order in advance of

> > it being needed, rather than having a callback.

> 

> Aren't we kind of stuck here?

> 

> On the previous iterations, we already discussed this and Tom

> eventually told he was in favour of __weak functions, and the

> discussion stopped there. I assumed you were ok with it.

> 

> I'd really want to move forward on that. This is something that is

> really biting us *now* and I'd hate to miss yet another merge window

> because of debates like this.


Yes, I think this is where we want things to be weak, with a reasonable
default.  If we start to see that "everyone" does the same thing by and
large we can re-evaluate.

-- 
Tom
Maxime Ripard Jan. 22, 2018, 3:57 p.m. | #6
On Mon, Jan 22, 2018 at 07:49:41AM -0500, Tom Rini wrote:
> On Mon, Jan 22, 2018 at 01:46:46PM +0100, Maxime Ripard wrote:

> > Hi,

> > 

> > On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote:

> > > > On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote:

> > > >> On 16 January 2018 at 01:16, Maxime Ripard

> > > >> <maxime.ripard@free-electrons.com> wrote:

> > > >> > Allow boards and architectures to override the default environment lookup

> > > >> > code by overriding env_get_location.

> > > >> >

> > > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>

> > > >> > Reviewed-by: Lukasz Majewski <lukma@denx.de>

> > > >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

> > > >> > ---

> > > >> >  env/env.c | 20 +++++++++++++++++++-

> > > >> >  1 file changed, 19 insertions(+), 1 deletion(-)

> > > >> >

> > > >>

> > > >> I still don't really understand why this needs to be a weak function.

> > > >> If the board knows the priority order, can it not put it into

> > > >> global_data? We could have a little u8 array of 4 items with a

> > > >> terminator?

> > > >

> > > > Sure that would be simpler, but that would also prevent us from doing

> > > > "smart" things based on data other than just whether the previous

> > > > environment is usable. Things based for example on a GPIO state, or a

> > > > custom algorithm to transition (or duplicate) the environment.

> > > 

> > > In that case the board could read the GPIO state, or the algorithm,

> > > and then set up the value.

> > > 

> > > Basically I am saying it could set up the priority order in advance of

> > > it being needed, rather than having a callback.

> > 

> > Aren't we kind of stuck here?

> > 

> > On the previous iterations, we already discussed this and Tom

> > eventually told he was in favour of __weak functions, and the

> > discussion stopped there. I assumed you were ok with it.

> > 

> > I'd really want to move forward on that. This is something that is

> > really biting us *now* and I'd hate to miss yet another merge window

> > because of debates like this.

> 

> Yes, I think this is where we want things to be weak, with a reasonable

> default.  If we start to see that "everyone" does the same thing by and

> large we can re-evaluate.


Ok.

I've fixed the bug I mentionned the other day on IRC, should I send a PR?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Tom Rini Jan. 22, 2018, 4:36 p.m. | #7
On Mon, Jan 22, 2018 at 04:57:41PM +0100, Maxime Ripard wrote:
> On Mon, Jan 22, 2018 at 07:49:41AM -0500, Tom Rini wrote:

> > On Mon, Jan 22, 2018 at 01:46:46PM +0100, Maxime Ripard wrote:

> > > Hi,

> > > 

> > > On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote:

> > > > > On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote:

> > > > >> On 16 January 2018 at 01:16, Maxime Ripard

> > > > >> <maxime.ripard@free-electrons.com> wrote:

> > > > >> > Allow boards and architectures to override the default environment lookup

> > > > >> > code by overriding env_get_location.

> > > > >> >

> > > > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>

> > > > >> > Reviewed-by: Lukasz Majewski <lukma@denx.de>

> > > > >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

> > > > >> > ---

> > > > >> >  env/env.c | 20 +++++++++++++++++++-

> > > > >> >  1 file changed, 19 insertions(+), 1 deletion(-)

> > > > >> >

> > > > >>

> > > > >> I still don't really understand why this needs to be a weak function.

> > > > >> If the board knows the priority order, can it not put it into

> > > > >> global_data? We could have a little u8 array of 4 items with a

> > > > >> terminator?

> > > > >

> > > > > Sure that would be simpler, but that would also prevent us from doing

> > > > > "smart" things based on data other than just whether the previous

> > > > > environment is usable. Things based for example on a GPIO state, or a

> > > > > custom algorithm to transition (or duplicate) the environment.

> > > > 

> > > > In that case the board could read the GPIO state, or the algorithm,

> > > > and then set up the value.

> > > > 

> > > > Basically I am saying it could set up the priority order in advance of

> > > > it being needed, rather than having a callback.

> > > 

> > > Aren't we kind of stuck here?

> > > 

> > > On the previous iterations, we already discussed this and Tom

> > > eventually told he was in favour of __weak functions, and the

> > > discussion stopped there. I assumed you were ok with it.

> > > 

> > > I'd really want to move forward on that. This is something that is

> > > really biting us *now* and I'd hate to miss yet another merge window

> > > because of debates like this.

> > 

> > Yes, I think this is where we want things to be weak, with a reasonable

> > default.  If we start to see that "everyone" does the same thing by and

> > large we can re-evaluate.

> 

> Ok.

> 

> I've fixed the bug I mentionned the other day on IRC, should I send a PR?


Lets give Simon a chance to provide any other feedback here, or another
argument to convince me that no, we don't want to have this abstracted
by a weak function but instead ..., thanks!

-- 
Tom
Simon Glass Jan. 22, 2018, 4:48 p.m. | #8
Hi,

On 22 January 2018 at 09:36, Tom Rini <trini@konsulko.com> wrote:
> On Mon, Jan 22, 2018 at 04:57:41PM +0100, Maxime Ripard wrote:
>> On Mon, Jan 22, 2018 at 07:49:41AM -0500, Tom Rini wrote:
>> > On Mon, Jan 22, 2018 at 01:46:46PM +0100, Maxime Ripard wrote:
>> > > Hi,
>> > >
>> > > On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote:
>> > > > > On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote:
>> > > > >> On 16 January 2018 at 01:16, Maxime Ripard
>> > > > >> <maxime.ripard@free-electrons.com> wrote:
>> > > > >> > Allow boards and architectures to override the default environment lookup
>> > > > >> > code by overriding env_get_location.
>> > > > >> >
>> > > > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>
>> > > > >> > Reviewed-by: Lukasz Majewski <lukma@denx.de>
>> > > > >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> > > > >> > ---
>> > > > >> >  env/env.c | 20 +++++++++++++++++++-
>> > > > >> >  1 file changed, 19 insertions(+), 1 deletion(-)
>> > > > >> >
>> > > > >>
>> > > > >> I still don't really understand why this needs to be a weak function.
>> > > > >> If the board knows the priority order, can it not put it into
>> > > > >> global_data? We could have a little u8 array of 4 items with a
>> > > > >> terminator?
>> > > > >
>> > > > > Sure that would be simpler, but that would also prevent us from doing
>> > > > > "smart" things based on data other than just whether the previous
>> > > > > environment is usable. Things based for example on a GPIO state, or a
>> > > > > custom algorithm to transition (or duplicate) the environment.
>> > > >
>> > > > In that case the board could read the GPIO state, or the algorithm,
>> > > > and then set up the value.
>> > > >
>> > > > Basically I am saying it could set up the priority order in advance of
>> > > > it being needed, rather than having a callback.
>> > >
>> > > Aren't we kind of stuck here?
>> > >
>> > > On the previous iterations, we already discussed this and Tom
>> > > eventually told he was in favour of __weak functions, and the
>> > > discussion stopped there. I assumed you were ok with it.
>> > >
>> > > I'd really want to move forward on that. This is something that is
>> > > really biting us *now* and I'd hate to miss yet another merge window
>> > > because of debates like this.
>> >
>> > Yes, I think this is where we want things to be weak, with a reasonable
>> > default.  If we start to see that "everyone" does the same thing by and
>> > large we can re-evaluate.
>>
>> Ok.
>>
>> I've fixed the bug I mentionned the other day on IRC, should I send a PR?
>
> Lets give Simon a chance to provide any other feedback here, or another
> argument to convince me that no, we don't want to have this abstracted
> by a weak function but instead ..., thanks!

I suspect there is a reason why this is better than what I propose.
Perhaps when I try it out it will become apparent.

So let's go ahead and revisit later if we have new information.

Reviewed-by: Simon Glass <sjg@chromium.org>

Regards,
Simon
Tom Rini Jan. 22, 2018, 5:08 p.m. | #9
On Mon, Jan 22, 2018 at 09:48:26AM -0700, Simon Glass wrote:
> Hi,

> 

> On 22 January 2018 at 09:36, Tom Rini <trini@konsulko.com> wrote:

> > On Mon, Jan 22, 2018 at 04:57:41PM +0100, Maxime Ripard wrote:

> >> On Mon, Jan 22, 2018 at 07:49:41AM -0500, Tom Rini wrote:

> >> > On Mon, Jan 22, 2018 at 01:46:46PM +0100, Maxime Ripard wrote:

> >> > > Hi,

> >> > >

> >> > > On Sun, Jan 21, 2018 at 05:29:56PM -0700, Simon Glass wrote:

> >> > > > > On Wed, Jan 17, 2018 at 03:07:58PM -0700, Simon Glass wrote:

> >> > > > >> On 16 January 2018 at 01:16, Maxime Ripard

> >> > > > >> <maxime.ripard@free-electrons.com> wrote:

> >> > > > >> > Allow boards and architectures to override the default environment lookup

> >> > > > >> > code by overriding env_get_location.

> >> > > > >> >

> >> > > > >> > Reviewed-by: Andre Przywara <andre.przywara@arm.com>

> >> > > > >> > Reviewed-by: Lukasz Majewski <lukma@denx.de>

> >> > > > >> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

> >> > > > >> > ---

> >> > > > >> >  env/env.c | 20 +++++++++++++++++++-

> >> > > > >> >  1 file changed, 19 insertions(+), 1 deletion(-)

> >> > > > >> >

> >> > > > >>

> >> > > > >> I still don't really understand why this needs to be a weak function.

> >> > > > >> If the board knows the priority order, can it not put it into

> >> > > > >> global_data? We could have a little u8 array of 4 items with a

> >> > > > >> terminator?

> >> > > > >

> >> > > > > Sure that would be simpler, but that would also prevent us from doing

> >> > > > > "smart" things based on data other than just whether the previous

> >> > > > > environment is usable. Things based for example on a GPIO state, or a

> >> > > > > custom algorithm to transition (or duplicate) the environment.

> >> > > >

> >> > > > In that case the board could read the GPIO state, or the algorithm,

> >> > > > and then set up the value.

> >> > > >

> >> > > > Basically I am saying it could set up the priority order in advance of

> >> > > > it being needed, rather than having a callback.

> >> > >

> >> > > Aren't we kind of stuck here?

> >> > >

> >> > > On the previous iterations, we already discussed this and Tom

> >> > > eventually told he was in favour of __weak functions, and the

> >> > > discussion stopped there. I assumed you were ok with it.

> >> > >

> >> > > I'd really want to move forward on that. This is something that is

> >> > > really biting us *now* and I'd hate to miss yet another merge window

> >> > > because of debates like this.

> >> >

> >> > Yes, I think this is where we want things to be weak, with a reasonable

> >> > default.  If we start to see that "everyone" does the same thing by and

> >> > large we can re-evaluate.

> >>

> >> Ok.

> >>

> >> I've fixed the bug I mentionned the other day on IRC, should I send a PR?

> >

> > Lets give Simon a chance to provide any other feedback here, or another

> > argument to convince me that no, we don't want to have this abstracted

> > by a weak function but instead ..., thanks!

> 

> I suspect there is a reason why this is better than what I propose.

> Perhaps when I try it out it will become apparent.

> 

> So let's go ahead and revisit later if we have new information.

> 

> Reviewed-by: Simon Glass <sjg@chromium.org>


Thanks!  Maxime, please re-spin with the bugfix (or wait another day or
two for other feedback), repost and I'll take it in Thurs/Fri or so.

-- 
Tom

Patch

diff --git a/env/env.c b/env/env.c
index 75da2b921804..6d0ab8ebe1a4 100644
--- a/env/env.c
+++ b/env/env.c
@@ -79,7 +79,25 @@  static void env_set_inited(enum env_location location)
 
 static enum env_location env_load_location;
 
-static enum env_location env_get_location(enum env_operation op, int prio)
+/**
+ * env_get_location() - Returns the best env location for a board
+ * @op: operations performed on the environment
+ * @prio: priority between the multiple environments, 0 being the
+ *        highest priority
+ *
+ * This will return the preferred environment for the given
+ * priority. This is overridable by boards if they need to.
+ *
+ * All implementations are free to use the operation, the priority and
+ * any other data relevant to their choice, but must take into account
+ * the fact that the lowest prority (0) is the most important location
+ * in the system. The following locations should be returned by order
+ * of descending priorities, from the highest to the lowest priority.
+ *
+ * Returns:
+ * an enum env_location value on success, a negative error code otherwise
+ */
+__weak enum env_location env_get_location(enum env_operation op, int prio)
 {
 	switch (op) {
 	case ENVOP_GET_CHAR: