[12/14] env: Mark env_get_location as weak

Message ID 537a3581d52c026ce964762a38cf65faed7c175c.1511864667.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 Nov. 28, 2017, 10:24 a.m.
Allow boards and architectures to override the default environment lookup
code by overriding env_get_location.

Reviewed-by: Lukasz Majewski <lukma@denx.de>
Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
---
 env/env.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Andre Przywara Dec. 5, 2017, 10:14 a.m. | #1
Hi,

On 28/11/17 10:24, Maxime Ripard wrote:
> Allow boards and architectures to override the default environment lookup
> code by overriding env_get_location.
> 
> Reviewed-by: Lukasz Majewski <lukma@denx.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>

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

Cheers,
Andre.

> ---
>  env/env.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/env/env.c b/env/env.c
> index b4d8886e7a69..9b8b38cf3c2b 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -62,7 +62,7 @@ static enum env_location env_locations[] = {
>  
>  static enum env_location env_load_location;
>  
> -static enum env_location env_get_location(enum env_operation op, int prio)
> +__weak enum env_location env_get_location(enum env_operation op, int prio)
>  {
>  	switch (op) {
>  	case ENVO_GET_CHAR:
>
Simon Glass Dec. 29, 2017, 3:13 a.m. | #2
Hi Maxime,

On 28 November 2017 at 03:24, 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: Lukasz Majewski <lukma@denx.de>
> Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
> ---
>  env/env.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/env/env.c b/env/env.c
> index b4d8886e7a69..9b8b38cf3c2b 100644
> --- a/env/env.c
> +++ b/env/env.c
> @@ -62,7 +62,7 @@ static enum env_location env_locations[] = {
>
>  static enum env_location env_load_location;
>
> -static enum env_location env_get_location(enum env_operation op, int prio)
> +__weak enum env_location env_get_location(enum env_operation op, int prio)

Is it possible instead to make this deterministic, so that the board
sets up the location during boot?

>  {
>         switch (op) {
>         case ENVO_GET_CHAR:
> --
> git-series 0.9.1
> _______________________________________________
> U-Boot mailing list
> U-Boot@lists.denx.de
> https://lists.denx.de/listinfo/u-boot

Regards,
Simon
Maxime Ripard Jan. 5, 2018, 9:29 a.m. | #3
Hi Simon,

On Thu, Dec 28, 2017 at 08:13:42PM -0700, Simon Glass wrote:
> Hi Maxime,

> 

> On 28 November 2017 at 03:24, 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: Lukasz Majewski <lukma@denx.de>

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

> > ---

> >  env/env.c | 2 +-

> >  1 file changed, 1 insertion(+), 1 deletion(-)

> >

> > diff --git a/env/env.c b/env/env.c

> > index b4d8886e7a69..9b8b38cf3c2b 100644

> > --- a/env/env.c

> > +++ b/env/env.c

> > @@ -62,7 +62,7 @@ static enum env_location env_locations[] = {

> >

> >  static enum env_location env_load_location;

> >

> > -static enum env_location env_get_location(enum env_operation op, int prio)

> > +__weak enum env_location env_get_location(enum env_operation op, int prio)

> 

> Is it possible instead to make this deterministic, so that the board

> sets up the location during boot?


I'm not really sure what you mean here. The default implementation is
deterministic, and the board implementations should make sure that it
is if it's of any value to them.

Do you want me to add a comment to make that clearer?

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Simon Glass Jan. 8, 2018, 4:52 a.m. | #4
Hi Maxime,

On 5 January 2018 at 02:29, Maxime Ripard
<maxime.ripard@free-electrons.com> wrote:
> Hi Simon,
>
> On Thu, Dec 28, 2017 at 08:13:42PM -0700, Simon Glass wrote:
>> Hi Maxime,
>>
>> On 28 November 2017 at 03:24, 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: Lukasz Majewski <lukma@denx.de>
>> > Signed-off-by: Maxime Ripard <maxime.ripard@free-electrons.com>
>> > ---
>> >  env/env.c | 2 +-
>> >  1 file changed, 1 insertion(+), 1 deletion(-)
>> >
>> > diff --git a/env/env.c b/env/env.c
>> > index b4d8886e7a69..9b8b38cf3c2b 100644
>> > --- a/env/env.c
>> > +++ b/env/env.c
>> > @@ -62,7 +62,7 @@ static enum env_location env_locations[] = {
>> >
>> >  static enum env_location env_load_location;
>> >
>> > -static enum env_location env_get_location(enum env_operation op, int prio)
>> > +__weak enum env_location env_get_location(enum env_operation op, int prio)
>>
>> Is it possible instead to make this deterministic, so that the board
>> sets up the location during boot?
>
> I'm not really sure what you mean here. The default implementation is
> deterministic, and the board implementations should make sure that it
> is if it's of any value to them.
>
> Do you want me to add a comment to make that clearer?

I mean that the board presumably knows the location, so could set it
up (perhaps in global_data). Then env_get_location() can use it.

Making functions weak means everything becomes non-deterministic -
e.g. I cannot tell from the code what it is going to do.

>
> Thanks!
> Maxime
>
> --
> Maxime Ripard, Free Electrons
> Embedded Linux and Kernel engineering
> http://free-electrons.com

Regards,
Simon
Maxime Ripard Jan. 9, 2018, 1:10 p.m. | #5
Hi,

On Sun, Jan 07, 2018 at 09:52:23PM -0700, Simon Glass wrote:
> >> On 28 November 2017 at 03:24, 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: Lukasz Majewski <lukma@denx.de>

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

> >> > ---

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

> >> >  1 file changed, 1 insertion(+), 1 deletion(-)

> >> >

> >> > diff --git a/env/env.c b/env/env.c

> >> > index b4d8886e7a69..9b8b38cf3c2b 100644

> >> > --- a/env/env.c

> >> > +++ b/env/env.c

> >> > @@ -62,7 +62,7 @@ static enum env_location env_locations[] = {

> >> >

> >> >  static enum env_location env_load_location;

> >> >

> >> > -static enum env_location env_get_location(enum env_operation op, int prio)

> >> > +__weak enum env_location env_get_location(enum env_operation op, int prio)

> >>

> >> Is it possible instead to make this deterministic, so that the board

> >> sets up the location during boot?

> >

> > I'm not really sure what you mean here. The default implementation is

> > deterministic, and the board implementations should make sure that it

> > is if it's of any value to them.

> >

> > Do you want me to add a comment to make that clearer?

> 

> I mean that the board presumably knows the location, so could set it

> up (perhaps in global_data). Then env_get_location() can use it.


Well, it's pretty much what happens when you only have a single
environment selected in Kconfig, except that you don't need to change
anything in all the boards, which would be the case with your solution
I guess?

> Making functions weak means everything becomes non-deterministic -

> e.g. I cannot tell from the code what it is going to do.


One of the point of this serie is to allow boards to select the
environment locations based on data of their choice, so it's kind of
expected that that function should be overriden I guess?

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com
Tom Rini Jan. 9, 2018, 4:05 p.m. | #6
On Tue, Jan 09, 2018 at 02:10:47PM +0100, Maxime Ripard wrote:
> Hi,

> 

> On Sun, Jan 07, 2018 at 09:52:23PM -0700, Simon Glass wrote:

> > >> On 28 November 2017 at 03:24, 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: Lukasz Majewski <lukma@denx.de>

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

> > >> > ---

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

> > >> >  1 file changed, 1 insertion(+), 1 deletion(-)

> > >> >

> > >> > diff --git a/env/env.c b/env/env.c

> > >> > index b4d8886e7a69..9b8b38cf3c2b 100644

> > >> > --- a/env/env.c

> > >> > +++ b/env/env.c

> > >> > @@ -62,7 +62,7 @@ static enum env_location env_locations[] = {

> > >> >

> > >> >  static enum env_location env_load_location;

> > >> >

> > >> > -static enum env_location env_get_location(enum env_operation op, int prio)

> > >> > +__weak enum env_location env_get_location(enum env_operation op, int prio)

> > >>

> > >> Is it possible instead to make this deterministic, so that the board

> > >> sets up the location during boot?

> > >

> > > I'm not really sure what you mean here. The default implementation is

> > > deterministic, and the board implementations should make sure that it

> > > is if it's of any value to them.

> > >

> > > Do you want me to add a comment to make that clearer?

> > 

> > I mean that the board presumably knows the location, so could set it

> > up (perhaps in global_data). Then env_get_location() can use it.

> 

> Well, it's pretty much what happens when you only have a single

> environment selected in Kconfig, except that you don't need to change

> anything in all the boards, which would be the case with your solution

> I guess?

> 

> > Making functions weak means everything becomes non-deterministic -

> > e.g. I cannot tell from the code what it is going to do.

> 

> One of the point of this serie is to allow boards to select the

> environment locations based on data of their choice, so it's kind of

> expected that that function should be overriden I guess?


I am generally a fan of using __weak functions as well, with the weak
version being clear about what an overriding version is supposed to do,
when it needs to be overridden.

-- 
Tom
Maxime Ripard Jan. 11, 2018, 3:35 p.m. | #7
Hi,

On Tue, Jan 09, 2018 at 11:05:51AM -0500, Tom Rini wrote:
> On Tue, Jan 09, 2018 at 02:10:47PM +0100, Maxime Ripard wrote:

> > Hi,

> > 

> > On Sun, Jan 07, 2018 at 09:52:23PM -0700, Simon Glass wrote:

> > > >> On 28 November 2017 at 03:24, 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: Lukasz Majewski <lukma@denx.de>

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

> > > >> > ---

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

> > > >> >  1 file changed, 1 insertion(+), 1 deletion(-)

> > > >> >

> > > >> > diff --git a/env/env.c b/env/env.c

> > > >> > index b4d8886e7a69..9b8b38cf3c2b 100644

> > > >> > --- a/env/env.c

> > > >> > +++ b/env/env.c

> > > >> > @@ -62,7 +62,7 @@ static enum env_location env_locations[] = {

> > > >> >

> > > >> >  static enum env_location env_load_location;

> > > >> >

> > > >> > -static enum env_location env_get_location(enum env_operation op, int prio)

> > > >> > +__weak enum env_location env_get_location(enum env_operation op, int prio)

> > > >>

> > > >> Is it possible instead to make this deterministic, so that the board

> > > >> sets up the location during boot?

> > > >

> > > > I'm not really sure what you mean here. The default implementation is

> > > > deterministic, and the board implementations should make sure that it

> > > > is if it's of any value to them.

> > > >

> > > > Do you want me to add a comment to make that clearer?

> > > 

> > > I mean that the board presumably knows the location, so could set it

> > > up (perhaps in global_data). Then env_get_location() can use it.

> > 

> > Well, it's pretty much what happens when you only have a single

> > environment selected in Kconfig, except that you don't need to change

> > anything in all the boards, which would be the case with your solution

> > I guess?

> > 

> > > Making functions weak means everything becomes non-deterministic -

> > > e.g. I cannot tell from the code what it is going to do.

> > 

> > One of the point of this serie is to allow boards to select the

> > environment locations based on data of their choice, so it's kind of

> > expected that that function should be overriden I guess?

> 

> I am generally a fan of using __weak functions as well, with the weak

> version being clear about what an overriding version is supposed to do,

> when it needs to be overridden.


I'll add a comment then to detail what is expected from this function,
and any function that might override it.

Thanks!
Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux and Kernel engineering
http://free-electrons.com

Patch

diff --git a/env/env.c b/env/env.c
index b4d8886e7a69..9b8b38cf3c2b 100644
--- a/env/env.c
+++ b/env/env.c
@@ -62,7 +62,7 @@  static enum env_location env_locations[] = {
 
 static enum env_location env_load_location;
 
-static enum env_location env_get_location(enum env_operation op, int prio)
+__weak enum env_location env_get_location(enum env_operation op, int prio)
 {
 	switch (op) {
 	case ENVO_GET_CHAR: