[U-Boot] [PATCH] Revert "env: net: Move eth_parse_enetaddr() to net.c/h"

[Ondřej Jirman]

Hi Joe,

On Fri, Sep 13, 2019 at 07:38:20PM +0000, Joe Hershberger wrote:
> Hi Ondřej,
> 
> On Fri, Sep 13, 2019 at 2:12 PM Ondřej Jirman <megous at megous.com> wrote:
> >
> > On Fri, Sep 13, 2019 at 10:58:14AM -0600, Simon Glass wrote:
> > > Hi,
> > >
> > > On Fri, 13 Sep 2019 at 08:07, Heinrich Schuchardt <xypron.glpk at gmx.de> wrote:
> > > >
> > > > On 9/13/19 1:48 PM, Ondřej Jirman wrote:
> > > > > On Fri, Sep 13, 2019 at 01:19:18PM +0200, Heinrich Schuchardt wrote:
> > > > >> On 9/13/19 1:48 AM, megous at megous.com wrote:
> > > > >>> From: Ondrej Jirman <megous at megous.com>
> > > > >>>
> > > > >>> The reverted patch causes linking error with disabled CONFIG_NET:
> > > > >>>
> > > > >>>    cmd/built-in.o: In function `eth_env_get_enetaddr':
> > > > >>>    u-boot-v2019.10/cmd/nvedit.c:363: undefined reference to `eth_parse_enetaddr'
> > > > >>>
> > > > >>> Function setup_environment() in board/sunxi/board.c calls
> > > > >>> eth_env_set_enetaddr() to setup stable mac address for ethernet interfaces.
> > > > >>>
> > > > >>> This needs to be implemented and succeed even if net is disabled in u-boot,
> > > > >>> as it ensures Linux will not generate random MAC addresses, and picks the
> > > > >>> ones provided by u-boot via DT. See fdt_fixup_ethernet().
> > > > >>>
> > > > >>> This feature is independent of the whole network stack and network drivers
> > > > >>> in u-boot.
> > > > >>>
> > > > >>> This revert fixes the linking error.
> > > > >>>
> > > > >>> Signed-off-by: Ondrej Jirman <megous at megous.com>
> > > > >>> ---
> > > > >>>   cmd/nvedit.c           | 12 ++++++++++++
> > > > >>>   include/env_internal.h | 11 +++++++++++
> > > > >>>   include/net.h          | 11 -----------
> > > > >>>   net/net.c              | 12 ------------
> > > > >>>   4 files changed, 23 insertions(+), 23 deletions(-)
> > > > >>>
> > > > >>> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> > > > >>> index 1cb0bc1460..399f6d6ce1 100644
> > > > >>> --- a/cmd/nvedit.c
> > > > >>> +++ b/cmd/nvedit.c
> > > > >>> @@ -358,6 +358,18 @@ ulong env_get_hex(const char *varname, ulong default_val)
> > > > >>>     return value;
> > > > >>>   }
> > > > >>>
> > > > >>> +void eth_parse_enetaddr(const char *addr, uint8_t *enetaddr)
> > > > >>> +{
> > > > >>> +   char *end;
> > > > >>> +   int i;
> > > > >>> +
> > > > >>> +   for (i = 0; i < 6; ++i) {
> > > > >>> +           enetaddr[i] = addr ? simple_strtoul(addr, &end, 16) : 0;
> > > > >>> +           if (addr)
> > > > >>> +                   addr = (*end) ? end + 1 : end;
> > > > >>> +   }
> > > > >>> +}
> > > > >>> +
> > > > >>>   int eth_env_get_enetaddr(const char *name, uint8_t *enetaddr)
> > > > >>>   {
> > > > >>>     eth_parse_enetaddr(env_get(name), enetaddr);
> > > > >>> diff --git a/include/env_internal.h b/include/env_internal.h
> > > > >>> index b1ddcb5adf..27eb5bd1e7 100644
> > > > >>> --- a/include/env_internal.h
> > > > >>> +++ b/include/env_internal.h
> > > > >>
> > > > >> Please, don't move the definition to env_internal.h but to env.h as
> > > > >> board/renesas/sh7753evb/sh7753evb.c and others are using
> > > > >> eth_parse_enetaddr().
> > > > >>
> > > > >> env_internal.h explicitly states "It should not be included by board files".
> > > > >>
> > > > >> Please, execute Travis CI tests to ensure you do not break any other board.
> > > > >
> > > > > I haven't found any documentation in the tree or on the u-boot website on
> > > > > how to do that.
> > > > >
> > > > > regards,
> > > > >       o.
> > > >
> > > > Create a repository with U-Boot on Github (e.g. fork
> > > > https://github.com/trini/u-boot). Logon in https://travis-ci.org/ with
> > > > your Github account. Allow Travis to access the U-Boot repository. In
> > > > settings select "Build pushed requests". Push your commit to Github. Now
> > > > Travis should start building (takes about 4 hours).
> > > >
> > > > The file telling Travis what to do is in .travis.yml in U-Boot.
> > > >
> > >
> > > I think it would be better to do a patch to move this function into a
> > > common/ file, and add a new config to test for this case.
> > >
> > > I understand the desire for a revert, but no build was broken due to
> > > the original patch, and the expected behaviour was not obvious.
> >
> > My build was broken, where previously it worked with the same config
> > on v2019.07. Otherwise I wouldn't complain. Or are u-boot builds
> > not meant to be user configurable, at least at the high level, like
> > disabling usb/net, or other big subystems that may be unnecessary and
> > take a lot of space and boot time?
> 
> It sounds like your board / build config is not in the mainline tree,
> so there is no way Simon could have known it would break you, and it
> didn't break the existing boards, hence his comment. I strongly
> encourage you to send a series adding your config so that it has an
> opportunity to be build tested.

I'm using orangepi_pc_defconfig. It's mainline.

I just disable a few things, like USB and NET. That's enough for it to
break the build.

I don't think my minimalistic config would be proper as a defconfig for that
particular board. Anyway, the kernel has feature that generates random
configs for revealing these kinds of issues.

regards,
	o.

> > I'm more used to the Linux kernel, where it's sort of expected that
> > random configurations at least build, if not boot.
> 
> No, that is the expectation here too, and generally we accomplish that.
> 
> > I don't mind having local patches for my specific configurations, if
> > expectations for straying from defconfig are different for u-boot.
> >
> > regards,
> >         o.
> >
> > > Regards,
> > > Simon
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > https://lists.denx.de/listinfo/u-boot