[U-Boot] [PATCH] Revert "env: net: Move eth_parse_enetaddr() to net.c/h"
From: Ondrej Jirman megous@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@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 @@ -273,6 +273,17 @@ struct env_driver {
extern struct hsearch_data env_htab;
+/** + * eth_parse_enetaddr() - Parse a MAC address + * + * Convert a string MAC address + * + * @addr: MAC address in aa:bb:cc:dd:ee:ff format, where each part is a 2-digit + * hex value + * @enetaddr: Place to put MAC address (6 bytes) + */ +void eth_parse_enetaddr(const char *addr, uint8_t *enetaddr); + #endif /* DO_DEPS_ONLY */
#endif /* _ENV_INTERNAL_H_ */ diff --git a/include/net.h b/include/net.h index 75a16e4c8f..e208cc43a0 100644 --- a/include/net.h +++ b/include/net.h @@ -875,15 +875,4 @@ int update_tftp(ulong addr, char *interface, char *devstring);
/**********************************************************************/
-/** - * eth_parse_enetaddr() - Parse a MAC address - * - * Convert a string MAC address - * - * @addr: MAC address in aa:bb:cc:dd:ee:ff format, where each part is a 2-digit - * hex value - * @enetaddr: Place to put MAC address (6 bytes) - */ -void eth_parse_enetaddr(const char *addr, uint8_t *enetaddr); - #endif /* __NET_H__ */ diff --git a/net/net.c b/net/net.c index ded86e7456..4d2b7ead3b 100644 --- a/net/net.c +++ b/net/net.c @@ -1628,15 +1628,3 @@ ushort env_get_vlan(char *var) { return string_to_vlan(env_get(var)); } - -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; - } -}
On 9/13/19 1:48 AM, megous@megous.com wrote:
From: Ondrej Jirman megous@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@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.
Best regards
Heinrich
@@ -273,6 +273,17 @@ struct env_driver {
extern struct hsearch_data env_htab;
+/**
- eth_parse_enetaddr() - Parse a MAC address
- Convert a string MAC address
- @addr: MAC address in aa:bb:cc:dd:ee:ff format, where each part is a 2-digit
- hex value
- @enetaddr: Place to put MAC address (6 bytes)
- */
+void eth_parse_enetaddr(const char *addr, uint8_t *enetaddr);
#endif /* DO_DEPS_ONLY */
#endif /* _ENV_INTERNAL_H_ */
diff --git a/include/net.h b/include/net.h index 75a16e4c8f..e208cc43a0 100644 --- a/include/net.h +++ b/include/net.h @@ -875,15 +875,4 @@ int update_tftp(ulong addr, char *interface, char *devstring);
/**********************************************************************/
-/**
- eth_parse_enetaddr() - Parse a MAC address
- Convert a string MAC address
- @addr: MAC address in aa:bb:cc:dd:ee:ff format, where each part is a 2-digit
- hex value
- @enetaddr: Place to put MAC address (6 bytes)
- */
-void eth_parse_enetaddr(const char *addr, uint8_t *enetaddr);
- #endif /* __NET_H__ */
diff --git a/net/net.c b/net/net.c index ded86e7456..4d2b7ead3b 100644 --- a/net/net.c +++ b/net/net.c @@ -1628,15 +1628,3 @@ ushort env_get_vlan(char *var) { return string_to_vlan(env_get(var)); }
-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;- }
-}
On Fri, Sep 13, 2019 at 01:19:18PM +0200, Heinrich Schuchardt wrote:
On 9/13/19 1:48 AM, megous@megous.com wrote:
From: Ondrej Jirman megous@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@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.
Best regards
Heinrich
@@ -273,6 +273,17 @@ struct env_driver {
extern struct hsearch_data env_htab;
+/**
- eth_parse_enetaddr() - Parse a MAC address
- Convert a string MAC address
- @addr: MAC address in aa:bb:cc:dd:ee:ff format, where each part is a 2-digit
- hex value
- @enetaddr: Place to put MAC address (6 bytes)
- */
+void eth_parse_enetaddr(const char *addr, uint8_t *enetaddr);
#endif /* DO_DEPS_ONLY */
#endif /* _ENV_INTERNAL_H_ */
diff --git a/include/net.h b/include/net.h index 75a16e4c8f..e208cc43a0 100644 --- a/include/net.h +++ b/include/net.h @@ -875,15 +875,4 @@ int update_tftp(ulong addr, char *interface, char *devstring);
/**********************************************************************/
-/**
- eth_parse_enetaddr() - Parse a MAC address
- Convert a string MAC address
- @addr: MAC address in aa:bb:cc:dd:ee:ff format, where each part is a 2-digit
- hex value
- @enetaddr: Place to put MAC address (6 bytes)
- */
-void eth_parse_enetaddr(const char *addr, uint8_t *enetaddr);
- #endif /* __NET_H__ */
diff --git a/net/net.c b/net/net.c index ded86e7456..4d2b7ead3b 100644 --- a/net/net.c +++ b/net/net.c @@ -1628,15 +1628,3 @@ ushort env_get_vlan(char *var) { return string_to_vlan(env_get(var)); }
-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;- }
-}
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@megous.com wrote:
From: Ondrej Jirman megous@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@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.
Best regards
Heinrich
Hi,
On Fri, 13 Sep 2019 at 08:07, Heinrich Schuchardt xypron.glpk@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@megous.com wrote:
From: Ondrej Jirman megous@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@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.
Regards, Simon
On 9/13/19 6:58 PM, Simon Glass wrote:
Hi,
On Fri, 13 Sep 2019 at 08:07, Heinrich Schuchardt xypron.glpk@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@megous.com wrote:
From: Ondrej Jirman megous@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@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.
Do we really need a CONFIG? Doesn't the linker take care of eliminating unused global functions?
Regards
Heinrich
I understand the desire for a revert, but no build was broken due to the original patch, and the expected behaviour was not obvious.
Regards, Simon
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@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@megous.com wrote:
From: Ondrej Jirman megous@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@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?
I'm more used to the Linux kernel, where it's sort of expected that random configurations at least build, if not boot.
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
Hi Ondřej,
On Fri, Sep 13, 2019 at 2:12 PM Ondřej Jirman megous@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@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@megous.com wrote:
From: Ondrej Jirman megous@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@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@lists.denx.de https://lists.denx.de/listinfo/u-boot
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@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@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@megous.com wrote: > From: Ondrej Jirman megous@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@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@lists.denx.de https://lists.denx.de/listinfo/u-boot
On Fri, Sep 13, 2019 at 2:53 PM Ondřej Jirman megous@megous.com wrote:
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@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@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@megous.com wrote: >> From: Ondrej Jirman megous@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@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.
Clearly the point is that the actual problematic config is not mainline.
I don't think my minimalistic config would be proper as a defconfig for that particular board.
I was not suggesting to replace it, simply to add a minimal one. There are plenty of examples of boards with several defconfigs.
Anyway, the kernel has feature that generates random configs for revealing these kinds of issues.
Are you suggesting that you can port this to U-Boot so we can test in a similar way?
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@lists.denx.de https://lists.denx.de/listinfo/u-boot
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
On Fri, Sep 13, 2019 at 08:00:50PM +0000, Joe Hershberger wrote:
On Fri, Sep 13, 2019 at 2:53 PM Ondřej Jirman megous@megous.com wrote:
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.
Clearly the point is that the actual problematic config is not mainline.
I don't think my minimalistic config would be proper as a defconfig for that particular board.
I was not suggesting to replace it, simply to add a minimal one. There are plenty of examples of boards with several defconfigs.
Interesting, I may add one then. Not sure what sunxi maintainer will think of that, but if it has value for testing, why not. Probably just one minimal config would have caught this, so I guess it has some value.
Thanks for suggestion.
Anyway, the kernel has feature that generates random configs for revealing these kinds of issues.
Are you suggesting that you can port this to U-Boot so we can test in a similar way?
It's a Kconfig feature, you can already use it. Try make randconfig inside u-boot.
regards, o.
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@lists.denx.de https://lists.denx.de/listinfo/u-boot
U-Boot mailing list U-Boot@lists.denx.de https://lists.denx.de/listinfo/u-boot
Hi Ondřej,
On Fri, 13 Sep 2019 at 14:11, Ondřej Jirman megous@megous.com wrote:
On Fri, Sep 13, 2019 at 08:00:50PM +0000, Joe Hershberger wrote:
On Fri, Sep 13, 2019 at 2:53 PM Ondřej Jirman megous@megous.com wrote:
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.
Clearly the point is that the actual problematic config is not mainline.
I don't think my minimalistic config would be proper as a defconfig for that particular board.
I was not suggesting to replace it, simply to add a minimal one. There are plenty of examples of boards with several defconfigs.
Interesting, I may add one then. Not sure what sunxi maintainer will think of that, but if it has value for testing, why not. Probably just one minimal config would have caught this, so I guess it has some value.
Thanks for suggestion.
Anyway, the kernel has feature that generates random configs for revealing these kinds of issues.
Are you suggesting that you can port this to U-Boot so we can test in a similar way?
It's a Kconfig feature, you can already use it. Try make randconfig inside u-boot.
Another suggestion that might be better: add a new sandbox_nonet build. I suspect this would throw up quite a few issues. Also since it is more generic the build coverage would likely be better.
Regards, Simon
participants (5)
-
Heinrich Schuchardt -
Joe Hershberger -
megous＠megous.com -
Ondřej Jirman
-
Simon Glass