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

[Joe Hershberger]

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 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