[U-Boot] [PATCH] net: Always build the string_to_enetaddr() helper

[U-Boot] [PATCH] net: Always build the string_to_enetaddr() helper

[Joe Hershberger]

Part of the env cleanup moved this out of the environment code and into
the net code. However, this helper is sometimes needed even when the net
stack isn't included.

Move the helper to lib/net_utils.c like it's similarly-purposed
string_to_ip(). Also rename the moved function to similar naming.

Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
Reported-by: Ondrej Jirman <megous@megous.com>

---

 arch/arm/mach-tegra/cboot.c         |  2 +-
 board/renesas/sh7752evb/sh7752evb.c |  2 +-
 board/renesas/sh7753evb/sh7753evb.c |  2 +-
 board/renesas/sh7757lcr/sh7757lcr.c |  4 ++--
 cmd/ethsw.c                         |  2 +-
 cmd/nvedit.c                        |  2 +-
 doc/README.enetaddr                 |  4 ++--
 include/net.h                       | 24 +++++++++++++-----------
 lib/net_utils.c                     | 15 +++++++++++++++
 net/eth-uclass.c                    |  2 +-
 net/eth_legacy.c                    |  2 +-
 net/net.c                           | 12 ------------
 12 files changed, 39 insertions(+), 34 deletions(-)

diff --git a/arch/arm/mach-tegra/cboot.c b/arch/arm/mach-tegra/cboot.c
index 0433081c6c..0762144ecf 100644
--- a/arch/arm/mach-tegra/cboot.c
+++ b/arch/arm/mach-tegra/cboot.c
@@ -495,7 +495,7 @@ static int cboot_get_ethaddr_legacy(const void *fdt, uint8_t mac[ETH_ALEN])
 		return -ENOENT;
 	}
 
-	eth_parse_enetaddr(prop, mac);
+	string_to_enetaddr(prop, mac);
 
 	if (!is_valid_ethaddr(mac)) {
 		printf("Invalid MAC address: %s\n", prop);
diff --git a/board/renesas/sh7752evb/sh7752evb.c b/board/renesas/sh7752evb/sh7752evb.c
index d0b850f35d..b4292b22e7 100644
--- a/board/renesas/sh7752evb/sh7752evb.c
+++ b/board/renesas/sh7752evb/sh7752evb.c
@@ -93,7 +93,7 @@ static void set_mac_to_sh_giga_eth_register(int channel, char *mac_string)
 	unsigned char mac[6];
 	unsigned long val;
 
-	eth_parse_enetaddr(mac_string, mac);
+	string_to_enetaddr(mac_string, mac);
 
 	if (!channel)
 		ether = GETHER0_MAC_BASE;
diff --git a/board/renesas/sh7753evb/sh7753evb.c b/board/renesas/sh7753evb/sh7753evb.c
index e1bed7dcc3..5aebb041b0 100644
--- a/board/renesas/sh7753evb/sh7753evb.c
+++ b/board/renesas/sh7753evb/sh7753evb.c
@@ -100,7 +100,7 @@ static void set_mac_to_sh_giga_eth_register(int channel, char *mac_string)
 	unsigned char mac[6];
 	unsigned long val;
 
-	eth_parse_enetaddr(mac_string, mac);
+	string_to_enetaddr(mac_string, mac);
 
 	if (!channel)
 		ether = GETHER0_MAC_BASE;
diff --git a/board/renesas/sh7757lcr/sh7757lcr.c b/board/renesas/sh7757lcr/sh7757lcr.c
index d2671202e9..662c435f7a 100644
--- a/board/renesas/sh7757lcr/sh7757lcr.c
+++ b/board/renesas/sh7757lcr/sh7757lcr.c
@@ -140,7 +140,7 @@ static void set_mac_to_sh_eth_register(int channel, char *mac_string)
 	unsigned char mac[6];
 	unsigned long val;
 
-	eth_parse_enetaddr(mac_string, mac);
+	string_to_enetaddr(mac_string, mac);
 
 	if (!channel)
 		ether = ETHER0_MAC_BASE;
@@ -159,7 +159,7 @@ static void set_mac_to_sh_giga_eth_register(int channel, char *mac_string)
 	unsigned char mac[6];
 	unsigned long val;
 
-	eth_parse_enetaddr(mac_string, mac);
+	string_to_enetaddr(mac_string, mac);
 
 	if (!channel)
 		ether = GETHER0_MAC_BASE;
diff --git a/cmd/ethsw.c b/cmd/ethsw.c
index 8846805799..8d271ce1f3 100644
--- a/cmd/ethsw.c
+++ b/cmd/ethsw.c
@@ -864,7 +864,7 @@ static int keyword_match_mac_addr(enum ethsw_keyword_id key_id, int argc,
 		return 0;
 	}
 
-	eth_parse_enetaddr(argv[*argc_nr + 1], parsed_cmd->ethaddr);
+	string_to_enetaddr(argv[*argc_nr + 1], parsed_cmd->ethaddr);
 
 	if (is_broadcast_ethaddr(parsed_cmd->ethaddr)) {
 		memset(parsed_cmd->ethaddr, 0xFF, sizeof(parsed_cmd->ethaddr));
diff --git a/cmd/nvedit.c b/cmd/nvedit.c
index 1cb0bc1460..1e4b225a94 100644
--- a/cmd/nvedit.c
+++ b/cmd/nvedit.c
@@ -360,7 +360,7 @@ ulong env_get_hex(const char *varname, ulong default_val)
 
 int eth_env_get_enetaddr(const char *name, uint8_t *enetaddr)
 {
-	eth_parse_enetaddr(env_get(name), enetaddr);
+	string_to_enetaddr(env_get(name), enetaddr);
 	return is_valid_ethaddr(enetaddr);
 }
 
diff --git a/doc/README.enetaddr b/doc/README.enetaddr
index f926485986..5baa9f2179 100644
--- a/doc/README.enetaddr
+++ b/doc/README.enetaddr
@@ -76,12 +76,12 @@ To assist in the management of these layers, a few helper functions exist.  You
 should use these rather than attempt to do any kind of parsing/manipulation
 yourself as many common errors have arisen in the past.
 
-	* void eth_parse_enetaddr(const char *addr, uchar *enetaddr);
+	* void string_to_enetaddr(const char *addr, uchar *enetaddr);
 
 Convert a string representation of a MAC address to the binary version.
 char *addr = "00:11:22:33:44:55";
 uchar enetaddr[6];
-eth_parse_enetaddr(addr, enetaddr);
+string_to_enetaddr(addr, enetaddr);
 /* enetaddr now equals { 0x00, 0x11, 0x22, 0x33, 0x44, 0x55 } */
 
 	* int eth_env_get_enetaddr(char *name, uchar *enetaddr);
diff --git a/include/net.h b/include/net.h
index 75a16e4c8f..d054c5f694 100644
--- a/include/net.h
+++ b/include/net.h
@@ -825,6 +825,19 @@ static inline void net_random_ethaddr(uchar *addr)
 	addr[0] |= 0x02;	/* set local assignment bit (IEEE802) */
 }
 
+/**
+ * string_to_enetaddr() - Parse a MAC address
+ *
+ * Convert a string MAC address
+ *
+ * Implemented in lib/net_utils.c (built unconditionally)
+ *
+ * @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 string_to_enetaddr(const char *addr, uint8_t *enetaddr);
+
 /* Convert an IP address to a string */
 void ip_to_string(struct in_addr x, char *s);
 
@@ -875,15 +888,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/lib/net_utils.c b/lib/net_utils.c
index 9fb9d4a4b0..ed5044c3de 100644
--- a/lib/net_utils.c
+++ b/lib/net_utils.c
@@ -41,3 +41,18 @@ struct in_addr string_to_ip(const char *s)
 	addr.s_addr = htonl(addr.s_addr);
 	return addr;
 }
+
+void string_to_enetaddr(const char *addr, uint8_t *enetaddr)
+{
+	char *end;
+	int i;
+
+	if (!enetaddr)
+		return;
+
+	for (i = 0; i < 6; ++i) {
+		enetaddr[i] = addr ? simple_strtoul(addr, &end, 16) : 0;
+		if (addr)
+			addr = (*end) ? end + 1 : end;
+	}
+}
diff --git a/net/eth-uclass.c b/net/eth-uclass.c
index 3bd98b01ad..9fe4096120 100644
--- a/net/eth-uclass.c
+++ b/net/eth-uclass.c
@@ -227,7 +227,7 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op,
 		switch (op) {
 		case env_op_create:
 		case env_op_overwrite:
-			eth_parse_enetaddr(value, pdata->enetaddr);
+			string_to_enetaddr(value, pdata->enetaddr);
 			eth_write_hwaddr(dev);
 			break;
 		case env_op_delete:
diff --git a/net/eth_legacy.c b/net/eth_legacy.c
index 41f5263526..5d6b0d7d7f 100644
--- a/net/eth_legacy.c
+++ b/net/eth_legacy.c
@@ -117,7 +117,7 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op,
 			switch (op) {
 			case env_op_create:
 			case env_op_overwrite:
-				eth_parse_enetaddr(value, dev->enetaddr);
+				string_to_enetaddr(value, dev->enetaddr);
 				eth_write_hwaddr(dev, "eth", dev->index);
 				break;
 			case env_op_delete:
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;
-	}
-}
-- 
2.11.0

[U-Boot] [PATCH] net: Always build the string_to_enetaddr() helper

[Heinrich Schuchardt]

On 9/14/19 2:40 AM, Joe Hershberger wrote:
> Part of the env cleanup moved this out of the environment code and into
> the net code. However, this helper is sometimes needed even when the net
> stack isn't included.
>
> Move the helper to lib/net_utils.c like it's similarly-purposed
> string_to_ip(). Also rename the moved function to similar naming.
>
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> Reported-by: Ondrej Jirman <megous@megous.com>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>

>
> ---
>
>   arch/arm/mach-tegra/cboot.c         |  2 +-
>   board/renesas/sh7752evb/sh7752evb.c |  2 +-
>   board/renesas/sh7753evb/sh7753evb.c |  2 +-
>   board/renesas/sh7757lcr/sh7757lcr.c |  4 ++--
>   cmd/ethsw.c                         |  2 +-
>   cmd/nvedit.c                        |  2 +-
>   doc/README.enetaddr                 |  4 ++--
>   include/net.h                       | 24 +++++++++++++-----------
>   lib/net_utils.c                     | 15 +++++++++++++++
>   net/eth-uclass.c                    |  2 +-
>   net/eth_legacy.c                    |  2 +-
>   net/net.c                           | 12 ------------
>   12 files changed, 39 insertions(+), 34 deletions(-)
>
> diff --git a/arch/arm/mach-tegra/cboot.c b/arch/arm/mach-tegra/cboot.c
> index 0433081c6c..0762144ecf 100644
> --- a/arch/arm/mach-tegra/cboot.c
> +++ b/arch/arm/mach-tegra/cboot.c
> @@ -495,7 +495,7 @@ static int cboot_get_ethaddr_legacy(const void *fdt, uint8_t mac[ETH_ALEN])
>   		return -ENOENT;
>   	}
>
> -	eth_parse_enetaddr(prop, mac);
> +	string_to_enetaddr(prop, mac);
>
>   	if (!is_valid_ethaddr(mac)) {
>   		printf("Invalid MAC address: %s\n", prop);
> diff --git a/board/renesas/sh7752evb/sh7752evb.c b/board/renesas/sh7752evb/sh7752evb.c
> index d0b850f35d..b4292b22e7 100644
> --- a/board/renesas/sh7752evb/sh7752evb.c
> +++ b/board/renesas/sh7752evb/sh7752evb.c
> @@ -93,7 +93,7 @@ static void set_mac_to_sh_giga_eth_register(int channel, char *mac_string)
>   	unsigned char mac[6];
>   	unsigned long val;
>
> -	eth_parse_enetaddr(mac_string, mac);
> +	string_to_enetaddr(mac_string, mac);
>
>   	if (!channel)
>   		ether = GETHER0_MAC_BASE;
> diff --git a/board/renesas/sh7753evb/sh7753evb.c b/board/renesas/sh7753evb/sh7753evb.c
> index e1bed7dcc3..5aebb041b0 100644
> --- a/board/renesas/sh7753evb/sh7753evb.c
> +++ b/board/renesas/sh7753evb/sh7753evb.c
> @@ -100,7 +100,7 @@ static void set_mac_to_sh_giga_eth_register(int channel, char *mac_string)
>   	unsigned char mac[6];
>   	unsigned long val;
>
> -	eth_parse_enetaddr(mac_string, mac);
> +	string_to_enetaddr(mac_string, mac);
>
>   	if (!channel)
>   		ether = GETHER0_MAC_BASE;
> diff --git a/board/renesas/sh7757lcr/sh7757lcr.c b/board/renesas/sh7757lcr/sh7757lcr.c
> index d2671202e9..662c435f7a 100644
> --- a/board/renesas/sh7757lcr/sh7757lcr.c
> +++ b/board/renesas/sh7757lcr/sh7757lcr.c
> @@ -140,7 +140,7 @@ static void set_mac_to_sh_eth_register(int channel, char *mac_string)
>   	unsigned char mac[6];
>   	unsigned long val;
>
> -	eth_parse_enetaddr(mac_string, mac);
> +	string_to_enetaddr(mac_string, mac);
>
>   	if (!channel)
>   		ether = ETHER0_MAC_BASE;
> @@ -159,7 +159,7 @@ static void set_mac_to_sh_giga_eth_register(int channel, char *mac_string)
>   	unsigned char mac[6];
>   	unsigned long val;
>
> -	eth_parse_enetaddr(mac_string, mac);
> +	string_to_enetaddr(mac_string, mac);
>
>   	if (!channel)
>   		ether = GETHER0_MAC_BASE;
> diff --git a/cmd/ethsw.c b/cmd/ethsw.c
> index 8846805799..8d271ce1f3 100644
> --- a/cmd/ethsw.c
> +++ b/cmd/ethsw.c
> @@ -864,7 +864,7 @@ static int keyword_match_mac_addr(enum ethsw_keyword_id key_id, int argc,
>   		return 0;
>   	}
>
> -	eth_parse_enetaddr(argv[*argc_nr + 1], parsed_cmd->ethaddr);
> +	string_to_enetaddr(argv[*argc_nr + 1], parsed_cmd->ethaddr);
>
>   	if (is_broadcast_ethaddr(parsed_cmd->ethaddr)) {
>   		memset(parsed_cmd->ethaddr, 0xFF, sizeof(parsed_cmd->ethaddr));
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 1cb0bc1460..1e4b225a94 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -360,7 +360,7 @@ ulong env_get_hex(const char *varname, ulong default_val)
>
>   int eth_env_get_enetaddr(const char *name, uint8_t *enetaddr)
>   {
> -	eth_parse_enetaddr(env_get(name), enetaddr);
> +	string_to_enetaddr(env_get(name), enetaddr);
>   	return is_valid_ethaddr(enetaddr);
>   }
>
> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
> index f926485986..5baa9f2179 100644
> --- a/doc/README.enetaddr
> +++ b/doc/README.enetaddr
> @@ -76,12 +76,12 @@ To assist in the management of these layers, a few helper functions exist.  You
>   should use these rather than attempt to do any kind of parsing/manipulation
>   yourself as many common errors have arisen in the past.
>
> -	* void eth_parse_enetaddr(const char *addr, uchar *enetaddr);
> +	* void string_to_enetaddr(const char *addr, uchar *enetaddr);
>
>   Convert a string representation of a MAC address to the binary version.
>   char *addr = "00:11:22:33:44:55";
>   uchar enetaddr[6];
> -eth_parse_enetaddr(addr, enetaddr);
> +string_to_enetaddr(addr, enetaddr);
>   /* enetaddr now equals { 0x00, 0x11, 0x22, 0x33, 0x44, 0x55 } */
>
>   	* int eth_env_get_enetaddr(char *name, uchar *enetaddr);
> diff --git a/include/net.h b/include/net.h
> index 75a16e4c8f..d054c5f694 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -825,6 +825,19 @@ static inline void net_random_ethaddr(uchar *addr)
>   	addr[0] |= 0x02;	/* set local assignment bit (IEEE802) */
>   }
>
> +/**
> + * string_to_enetaddr() - Parse a MAC address
> + *
> + * Convert a string MAC address
> + *
> + * Implemented in lib/net_utils.c (built unconditionally)
> + *
> + * @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 string_to_enetaddr(const char *addr, uint8_t *enetaddr);
> +
>   /* Convert an IP address to a string */
>   void ip_to_string(struct in_addr x, char *s);
>
> @@ -875,15 +888,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/lib/net_utils.c b/lib/net_utils.c
> index 9fb9d4a4b0..ed5044c3de 100644
> --- a/lib/net_utils.c
> +++ b/lib/net_utils.c
> @@ -41,3 +41,18 @@ struct in_addr string_to_ip(const char *s)
>   	addr.s_addr = htonl(addr.s_addr);
>   	return addr;
>   }
> +
> +void string_to_enetaddr(const char *addr, uint8_t *enetaddr)
> +{
> +	char *end;
> +	int i;
> +
> +	if (!enetaddr)
> +		return;
> +
> +	for (i = 0; i < 6; ++i) {
> +		enetaddr[i] = addr ? simple_strtoul(addr, &end, 16) : 0;
> +		if (addr)
> +			addr = (*end) ? end + 1 : end;
> +	}
> +}
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index 3bd98b01ad..9fe4096120 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -227,7 +227,7 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op,
>   		switch (op) {
>   		case env_op_create:
>   		case env_op_overwrite:
> -			eth_parse_enetaddr(value, pdata->enetaddr);
> +			string_to_enetaddr(value, pdata->enetaddr);
>   			eth_write_hwaddr(dev);
>   			break;
>   		case env_op_delete:
> diff --git a/net/eth_legacy.c b/net/eth_legacy.c
> index 41f5263526..5d6b0d7d7f 100644
> --- a/net/eth_legacy.c
> +++ b/net/eth_legacy.c
> @@ -117,7 +117,7 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op,
>   			switch (op) {
>   			case env_op_create:
>   			case env_op_overwrite:
> -				eth_parse_enetaddr(value, dev->enetaddr);
> +				string_to_enetaddr(value, dev->enetaddr);
>   				eth_write_hwaddr(dev, "eth", dev->index);
>   				break;
>   			case env_op_delete:
> 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;
> -	}
> -}
>

[U-Boot] [PATCH] net: Always build the string_to_enetaddr() helper

[Ondřej Jirman]

Hi,

On Fri, Sep 13, 2019 at 07:40:22PM -0500, Joe Hershberger wrote:
> Part of the env cleanup moved this out of the environment code and into
> the net code. However, this helper is sometimes needed even when the net
> stack isn't included.
> 
> Move the helper to lib/net_utils.c like it's similarly-purposed
> string_to_ip(). Also rename the moved function to similar naming.
> 
> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> Reported-by: Ondrej Jirman <megous@megous.com>

I've tested the patch and it works, but I'be found other related issue, where
u-boot thinks %pM will format a MAC address string, but it does just
print out the pointer due to relevant functions being gated by CONFIG_CMD_NET
guard in lib/vsprintf.c.

The gating should probably be done so that it panics/halts the u-boot if gated
pointer flags are used by u-boot code, because that will clearly be incorrect,
without calling code ever knowing. This way the user will know that something
is wrong and will have to fix the code.

I noticed be cause produced mac address is wrong.

Quick fix is to remove the CONFIG_CMD_NET gate.

regards,
	o.

> 
> ---
> 
>  arch/arm/mach-tegra/cboot.c         |  2 +-
>  board/renesas/sh7752evb/sh7752evb.c |  2 +-
>  board/renesas/sh7753evb/sh7753evb.c |  2 +-
>  board/renesas/sh7757lcr/sh7757lcr.c |  4 ++--
>  cmd/ethsw.c                         |  2 +-
>  cmd/nvedit.c                        |  2 +-
>  doc/README.enetaddr                 |  4 ++--
>  include/net.h                       | 24 +++++++++++++-----------
>  lib/net_utils.c                     | 15 +++++++++++++++
>  net/eth-uclass.c                    |  2 +-
>  net/eth_legacy.c                    |  2 +-
>  net/net.c                           | 12 ------------
>  12 files changed, 39 insertions(+), 34 deletions(-)
> 
> diff --git a/arch/arm/mach-tegra/cboot.c b/arch/arm/mach-tegra/cboot.c
> index 0433081c6c..0762144ecf 100644
> --- a/arch/arm/mach-tegra/cboot.c
> +++ b/arch/arm/mach-tegra/cboot.c
> @@ -495,7 +495,7 @@ static int cboot_get_ethaddr_legacy(const void *fdt, uint8_t mac[ETH_ALEN])
>  		return -ENOENT;
>  	}
>  
> -	eth_parse_enetaddr(prop, mac);
> +	string_to_enetaddr(prop, mac);
>  
>  	if (!is_valid_ethaddr(mac)) {
>  		printf("Invalid MAC address: %s\n", prop);
> diff --git a/board/renesas/sh7752evb/sh7752evb.c b/board/renesas/sh7752evb/sh7752evb.c
> index d0b850f35d..b4292b22e7 100644
> --- a/board/renesas/sh7752evb/sh7752evb.c
> +++ b/board/renesas/sh7752evb/sh7752evb.c
> @@ -93,7 +93,7 @@ static void set_mac_to_sh_giga_eth_register(int channel, char *mac_string)
>  	unsigned char mac[6];
>  	unsigned long val;
>  
> -	eth_parse_enetaddr(mac_string, mac);
> +	string_to_enetaddr(mac_string, mac);
>  
>  	if (!channel)
>  		ether = GETHER0_MAC_BASE;
> diff --git a/board/renesas/sh7753evb/sh7753evb.c b/board/renesas/sh7753evb/sh7753evb.c
> index e1bed7dcc3..5aebb041b0 100644
> --- a/board/renesas/sh7753evb/sh7753evb.c
> +++ b/board/renesas/sh7753evb/sh7753evb.c
> @@ -100,7 +100,7 @@ static void set_mac_to_sh_giga_eth_register(int channel, char *mac_string)
>  	unsigned char mac[6];
>  	unsigned long val;
>  
> -	eth_parse_enetaddr(mac_string, mac);
> +	string_to_enetaddr(mac_string, mac);
>  
>  	if (!channel)
>  		ether = GETHER0_MAC_BASE;
> diff --git a/board/renesas/sh7757lcr/sh7757lcr.c b/board/renesas/sh7757lcr/sh7757lcr.c
> index d2671202e9..662c435f7a 100644
> --- a/board/renesas/sh7757lcr/sh7757lcr.c
> +++ b/board/renesas/sh7757lcr/sh7757lcr.c
> @@ -140,7 +140,7 @@ static void set_mac_to_sh_eth_register(int channel, char *mac_string)
>  	unsigned char mac[6];
>  	unsigned long val;
>  
> -	eth_parse_enetaddr(mac_string, mac);
> +	string_to_enetaddr(mac_string, mac);
>  
>  	if (!channel)
>  		ether = ETHER0_MAC_BASE;
> @@ -159,7 +159,7 @@ static void set_mac_to_sh_giga_eth_register(int channel, char *mac_string)
>  	unsigned char mac[6];
>  	unsigned long val;
>  
> -	eth_parse_enetaddr(mac_string, mac);
> +	string_to_enetaddr(mac_string, mac);
>  
>  	if (!channel)
>  		ether = GETHER0_MAC_BASE;
> diff --git a/cmd/ethsw.c b/cmd/ethsw.c
> index 8846805799..8d271ce1f3 100644
> --- a/cmd/ethsw.c
> +++ b/cmd/ethsw.c
> @@ -864,7 +864,7 @@ static int keyword_match_mac_addr(enum ethsw_keyword_id key_id, int argc,
>  		return 0;
>  	}
>  
> -	eth_parse_enetaddr(argv[*argc_nr + 1], parsed_cmd->ethaddr);
> +	string_to_enetaddr(argv[*argc_nr + 1], parsed_cmd->ethaddr);
>  
>  	if (is_broadcast_ethaddr(parsed_cmd->ethaddr)) {
>  		memset(parsed_cmd->ethaddr, 0xFF, sizeof(parsed_cmd->ethaddr));
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 1cb0bc1460..1e4b225a94 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -360,7 +360,7 @@ ulong env_get_hex(const char *varname, ulong default_val)
>  
>  int eth_env_get_enetaddr(const char *name, uint8_t *enetaddr)
>  {
> -	eth_parse_enetaddr(env_get(name), enetaddr);
> +	string_to_enetaddr(env_get(name), enetaddr);
>  	return is_valid_ethaddr(enetaddr);
>  }
>  
> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
> index f926485986..5baa9f2179 100644
> --- a/doc/README.enetaddr
> +++ b/doc/README.enetaddr
> @@ -76,12 +76,12 @@ To assist in the management of these layers, a few helper functions exist.  You
>  should use these rather than attempt to do any kind of parsing/manipulation
>  yourself as many common errors have arisen in the past.
>  
> -	* void eth_parse_enetaddr(const char *addr, uchar *enetaddr);
> +	* void string_to_enetaddr(const char *addr, uchar *enetaddr);
>  
>  Convert a string representation of a MAC address to the binary version.
>  char *addr = "00:11:22:33:44:55";
>  uchar enetaddr[6];
> -eth_parse_enetaddr(addr, enetaddr);
> +string_to_enetaddr(addr, enetaddr);
>  /* enetaddr now equals { 0x00, 0x11, 0x22, 0x33, 0x44, 0x55 } */
>  
>  	* int eth_env_get_enetaddr(char *name, uchar *enetaddr);
> diff --git a/include/net.h b/include/net.h
> index 75a16e4c8f..d054c5f694 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -825,6 +825,19 @@ static inline void net_random_ethaddr(uchar *addr)
>  	addr[0] |= 0x02;	/* set local assignment bit (IEEE802) */
>  }
>  
> +/**
> + * string_to_enetaddr() - Parse a MAC address
> + *
> + * Convert a string MAC address
> + *
> + * Implemented in lib/net_utils.c (built unconditionally)
> + *
> + * @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 string_to_enetaddr(const char *addr, uint8_t *enetaddr);
> +
>  /* Convert an IP address to a string */
>  void ip_to_string(struct in_addr x, char *s);
>  
> @@ -875,15 +888,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/lib/net_utils.c b/lib/net_utils.c
> index 9fb9d4a4b0..ed5044c3de 100644
> --- a/lib/net_utils.c
> +++ b/lib/net_utils.c
> @@ -41,3 +41,18 @@ struct in_addr string_to_ip(const char *s)
>  	addr.s_addr = htonl(addr.s_addr);
>  	return addr;
>  }
> +
> +void string_to_enetaddr(const char *addr, uint8_t *enetaddr)
> +{
> +	char *end;
> +	int i;
> +
> +	if (!enetaddr)
> +		return;
> +
> +	for (i = 0; i < 6; ++i) {
> +		enetaddr[i] = addr ? simple_strtoul(addr, &end, 16) : 0;
> +		if (addr)
> +			addr = (*end) ? end + 1 : end;
> +	}
> +}
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index 3bd98b01ad..9fe4096120 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -227,7 +227,7 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op,
>  		switch (op) {
>  		case env_op_create:
>  		case env_op_overwrite:
> -			eth_parse_enetaddr(value, pdata->enetaddr);
> +			string_to_enetaddr(value, pdata->enetaddr);
>  			eth_write_hwaddr(dev);
>  			break;
>  		case env_op_delete:
> diff --git a/net/eth_legacy.c b/net/eth_legacy.c
> index 41f5263526..5d6b0d7d7f 100644
> --- a/net/eth_legacy.c
> +++ b/net/eth_legacy.c
> @@ -117,7 +117,7 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op,
>  			switch (op) {
>  			case env_op_create:
>  			case env_op_overwrite:
> -				eth_parse_enetaddr(value, dev->enetaddr);
> +				string_to_enetaddr(value, dev->enetaddr);
>  				eth_write_hwaddr(dev, "eth", dev->index);
>  				break;
>  			case env_op_delete:
> 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;
> -	}
> -}
> -- 
> 2.11.0
> 

[U-Boot] [PATCH] net: Always build the string_to_enetaddr() helper

[Tom Rini]

On Sat, Sep 14, 2019 at 04:05:44PM +0200, Ondřej Jirman wrote:
> Hi,
> 
> On Fri, Sep 13, 2019 at 07:40:22PM -0500, Joe Hershberger wrote:
> > Part of the env cleanup moved this out of the environment code and into
> > the net code. However, this helper is sometimes needed even when the net
> > stack isn't included.
> > 
> > Move the helper to lib/net_utils.c like it's similarly-purposed
> > string_to_ip(). Also rename the moved function to similar naming.
> > 
> > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> > Reported-by: Ondrej Jirman <megous@megous.com>
> 
> I've tested the patch and it works, but I'be found other related issue, where
> u-boot thinks %pM will format a MAC address string, but it does just
> print out the pointer due to relevant functions being gated by CONFIG_CMD_NET
> guard in lib/vsprintf.c.
> 
> The gating should probably be done so that it panics/halts the u-boot if gated
> pointer flags are used by u-boot code, because that will clearly be incorrect,
> without calling code ever knowing. This way the user will know that something
> is wrong and will have to fix the code.

I'm not in favor of panic because of calling an unimplemented print
format character.  I guess we'll need to see what the size increase is
on un-guarding these formats and go from there.

-- 
Tom
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: not available
URL: <http://lists.denx.de/pipermail/u-boot/attachments/20190914/6495bf43/attachment.sig>

[U-Boot] [PATCH] net: Always build the string_to_enetaddr() helper

[Joe Hershberger]

On Sat, Sep 14, 2019 at 1:32 PM Tom Rini <trini@konsulko.com> wrote:
>
> On Sat, Sep 14, 2019 at 04:05:44PM +0200, Ondřej Jirman wrote:
> > Hi,
> >
> > On Fri, Sep 13, 2019 at 07:40:22PM -0500, Joe Hershberger wrote:
> > > Part of the env cleanup moved this out of the environment code and into
> > > the net code. However, this helper is sometimes needed even when the net
> > > stack isn't included.
> > >
> > > Move the helper to lib/net_utils.c like it's similarly-purposed
> > > string_to_ip(). Also rename the moved function to similar naming.
> > >
> > > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> > > Reported-by: Ondrej Jirman <megous@megous.com>
> >
> > I've tested the patch and it works, but I'be found other related issue, where
> > u-boot thinks %pM will format a MAC address string, but it does just
> > print out the pointer due to relevant functions being gated by CONFIG_CMD_NET
> > guard in lib/vsprintf.c.
> >
> > The gating should probably be done so that it panics/halts the u-boot if gated
> > pointer flags are used by u-boot code, because that will clearly be incorrect,
> > without calling code ever knowing. This way the user will know that something
> > is wrong and will have to fix the code.
>
> I'm not in favor of panic because of calling an unimplemented print
> format character.  I guess we'll need to see what the size increase is
> on un-guarding these formats and go from there.

I'll look into it. I'm also not in favor of a panic.

-Joe

[U-Boot] [PATCH] net: Always build the string_to_enetaddr() helper

[Simon Goldschmidt]

Joe Hershberger <joe.hershberger@ni.com> schrieb am Sa., 14. Sep. 2019,
20:46:

> On Sat, Sep 14, 2019 at 1:32 PM Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sat, Sep 14, 2019 at 04:05:44PM +0200, Ondřej Jirman wrote:
> > > Hi,
> > >
> > > On Fri, Sep 13, 2019 at 07:40:22PM -0500, Joe Hershberger wrote:
> > > > Part of the env cleanup moved this out of the environment code and
> into
> > > > the net code. However, this helper is sometimes needed even when the
> net
> > > > stack isn't included.
> > > >
> > > > Move the helper to lib/net_utils.c like it's similarly-purposed
> > > > string_to_ip(). Also rename the moved function to similar naming.
> > > >
> > > > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> > > > Reported-by: Ondrej Jirman <megous@megous.com>
> > >
> > > I've tested the patch and it works, but I'be found other related
> issue, where
> > > u-boot thinks %pM will format a MAC address string, but it does just
> > > print out the pointer due to relevant functions being gated by
> CONFIG_CMD_NET
> > > guard in lib/vsprintf.c.
> > >
> > > The gating should probably be done so that it panics/halts the u-boot
> if gated
> > > pointer flags are used by u-boot code, because that will clearly be
> incorrect,
> > > without calling code ever knowing. This way the user will know that
> something
> > > is wrong and will have to fix the code.
> >
> > I'm not in favor of panic because of calling an unimplemented print
> > format character.  I guess we'll need to see what the size increase is
> > on un-guarding these formats and go from there.
>
> I'll look into it. I'm also not in favor of a panic.
>

In lwIP, we're using macros for such format characters. Would it work to do
that here and make the compiler complain about an undefined symbol of the
macro for this extended format character isn't defined?

Regards,
Simon

[U-Boot] [PATCH] net: Always build the string_to_enetaddr() helper

[Joe Hershberger]

Hi Simon,

On Sat, Sep 14, 2019 at 1:55 PM Simon Goldschmidt
<simon.k.r.goldschmidt@gmail.com> wrote:
>
> Joe Hershberger <joe.hershberger@ni.com> schrieb am Sa., 14. Sep. 2019,
> 20:46:
>
> > On Sat, Sep 14, 2019 at 1:32 PM Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sat, Sep 14, 2019 at 04:05:44PM +0200, Ondřej Jirman wrote:
> > > > Hi,
> > > >
> > > > On Fri, Sep 13, 2019 at 07:40:22PM -0500, Joe Hershberger wrote:
> > > > > Part of the env cleanup moved this out of the environment code and
> > into
> > > > > the net code. However, this helper is sometimes needed even when the
> > net
> > > > > stack isn't included.
> > > > >
> > > > > Move the helper to lib/net_utils.c like it's similarly-purposed
> > > > > string_to_ip(). Also rename the moved function to similar naming.
> > > > >
> > > > > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> > > > > Reported-by: Ondrej Jirman <megous@megous.com>
> > > >
> > > > I've tested the patch and it works, but I'be found other related
> > issue, where
> > > > u-boot thinks %pM will format a MAC address string, but it does just
> > > > print out the pointer due to relevant functions being gated by
> > CONFIG_CMD_NET
> > > > guard in lib/vsprintf.c.
> > > >
> > > > The gating should probably be done so that it panics/halts the u-boot
> > if gated
> > > > pointer flags are used by u-boot code, because that will clearly be
> > incorrect,
> > > > without calling code ever knowing. This way the user will know that
> > something
> > > > is wrong and will have to fix the code.
> > >
> > > I'm not in favor of panic because of calling an unimplemented print
> > > format character.  I guess we'll need to see what the size increase is
> > > on un-guarding these formats and go from there.
> >
> > I'll look into it. I'm also not in favor of a panic.
> >
>
> In lwIP, we're using macros for such format characters. Would it work to do
> that here and make the compiler complain about an undefined symbol of the
> macro for this extended format character isn't defined?

Maybe... Though, if we don't successfully police the usage of the
macro, it won't help. I'd like to evaluate the code-size impact and
maybe just always include it.

-Joe

> Regards,
> Simon
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] net: Always build the string_to_enetaddr() helper

[Simon Goldschmidt]

Joe Hershberger <joe.hershberger@ni.com> schrieb am Di., 17. Sep. 2019,
23:22:

> Hi Simon,
>
> On Sat, Sep 14, 2019 at 1:55 PM Simon Goldschmidt
> <simon.k.r.goldschmidt@gmail.com> wrote:
> >
> > Joe Hershberger <joe.hershberger@ni.com> schrieb am Sa., 14. Sep. 2019,
> > 20:46:
> >
> > > On Sat, Sep 14, 2019 at 1:32 PM Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Sat, Sep 14, 2019 at 04:05:44PM +0200, Ondřej Jirman wrote:
> > > > > Hi,
> > > > >
> > > > > On Fri, Sep 13, 2019 at 07:40:22PM -0500, Joe Hershberger wrote:
> > > > > > Part of the env cleanup moved this out of the environment code
> and
> > > into
> > > > > > the net code. However, this helper is sometimes needed even when
> the
> > > net
> > > > > > stack isn't included.
> > > > > >
> > > > > > Move the helper to lib/net_utils.c like it's similarly-purposed
> > > > > > string_to_ip(). Also rename the moved function to similar naming.
> > > > > >
> > > > > > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> > > > > > Reported-by: Ondrej Jirman <megous@megous.com>
> > > > >
> > > > > I've tested the patch and it works, but I'be found other related
> > > issue, where
> > > > > u-boot thinks %pM will format a MAC address string, but it does
> just
> > > > > print out the pointer due to relevant functions being gated by
> > > CONFIG_CMD_NET
> > > > > guard in lib/vsprintf.c.
> > > > >
> > > > > The gating should probably be done so that it panics/halts the
> u-boot
> > > if gated
> > > > > pointer flags are used by u-boot code, because that will clearly be
> > > incorrect,
> > > > > without calling code ever knowing. This way the user will know that
> > > something
> > > > > is wrong and will have to fix the code.
> > > >
> > > > I'm not in favor of panic because of calling an unimplemented print
> > > > format character.  I guess we'll need to see what the size increase
> is
> > > > on un-guarding these formats and go from there.
> > >
> > > I'll look into it. I'm also not in favor of a panic.
> > >
> >
> > In lwIP, we're using macros for such format characters. Would it work to
> do
> > that here and make the compiler complain about an undefined symbol of the
> > macro for this extended format character isn't defined?
>
> Maybe... Though, if we don't successfully police the usage of the
> macro, it won't help. I'd like to evaluate the code-size impact and
> maybe just always include it.
>

Well, if you ensure the macro is only defined when the corresponding printf
code is enabled, I guess it does help.

Such macros might also help in the numerous cases where tiny printf doesn't
understand a formatter in spl, but that's a different issue, although
related.

OTOH, here, always including it (unless compiling for tiny printf) might be
ok as well...

Regards,
Simon


> -Joe
>

[U-Boot] [PATCH] net: Always build the string_to_enetaddr() helper

[Ondřej Jirman]

On Sat, Sep 14, 2019 at 02:29:11PM -0400, Tom Rini wrote:
> On Sat, Sep 14, 2019 at 04:05:44PM +0200, Ondřej Jirman wrote:
> > Hi,
> > 
> > On Fri, Sep 13, 2019 at 07:40:22PM -0500, Joe Hershberger wrote:
> > > Part of the env cleanup moved this out of the environment code and into
> > > the net code. However, this helper is sometimes needed even when the net
> > > stack isn't included.
> > > 
> > > Move the helper to lib/net_utils.c like it's similarly-purposed
> > > string_to_ip(). Also rename the moved function to similar naming.
> > > 
> > > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> > > Reported-by: Ondrej Jirman <megous@megous.com>
> > 
> > I've tested the patch and it works, but I'be found other related issue, where
> > u-boot thinks %pM will format a MAC address string, but it does just
> > print out the pointer due to relevant functions being gated by CONFIG_CMD_NET
> > guard in lib/vsprintf.c.
> > 
> > The gating should probably be done so that it panics/halts the u-boot if gated
> > pointer flags are used by u-boot code, because that will clearly be incorrect,
> > without calling code ever knowing. This way the user will know that something
> > is wrong and will have to fix the code.
> 
> I'm not in favor of panic because of calling an unimplemented print
> format character.  I guess we'll need to see what the size increase is
> on un-guarding these formats and go from there.

OTOH, u-boot doesn't use snprintf everywhere, and uses sprintf quite liberally,
so:

  char buf[exepected_size_for_format_X];

  sprintf(buf, "%pX", some_pointer);

may conceivably overflow the buffer, because u-boot will format a generic
pointer there instead of what the developer expected (based on config
option, the dev may never tried disabling), so only some users may be affected
by silent or not so silent stack corruption.

I think this is unlikely atm, because all formats I inspected, seem to
produce longer strings than the 64-bit generic pointer formatting.

That was why I suggested the panic(), because it may not be just a superficial
formatting issue.

Anyway, you definitely want a loud error, rather than silently
passing incorrect/unexpected data somewhere where it matters, like DTB.

regards,
	o.



> -- 
> Tom



> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] net: Always build the string_to_enetaddr() helper

[Ondřej Jirman]

Hello, 

On Tue, Sep 17, 2019 at 09:22:20PM +0000, Joe Hershberger wrote:
> Hi Simon,
> 
> On Sat, Sep 14, 2019 at 1:55 PM Simon Goldschmidt
> <simon.k.r.goldschmidt@gmail.com> wrote:
> >
> > Joe Hershberger <joe.hershberger@ni.com> schrieb am Sa., 14. Sep. 2019,
> > 20:46:
> >
> > > On Sat, Sep 14, 2019 at 1:32 PM Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Sat, Sep 14, 2019 at 04:05:44PM +0200, Ondřej Jirman wrote:
> > > > > Hi,
> > > > >
> > > > > On Fri, Sep 13, 2019 at 07:40:22PM -0500, Joe Hershberger wrote:
> > > > > > Part of the env cleanup moved this out of the environment code and
> > > into
> > > > > > the net code. However, this helper is sometimes needed even when the
> > > net
> > > > > > stack isn't included.
> > > > > >
> > > > > > Move the helper to lib/net_utils.c like it's similarly-purposed
> > > > > > string_to_ip(). Also rename the moved function to similar naming.
> > > > > >
> > > > > > Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> > > > > > Reported-by: Ondrej Jirman <megous@megous.com>
> > > > >
> > > > > I've tested the patch and it works, but I'be found other related
> > > issue, where
> > > > > u-boot thinks %pM will format a MAC address string, but it does just
> > > > > print out the pointer due to relevant functions being gated by
> > > CONFIG_CMD_NET
> > > > > guard in lib/vsprintf.c.
> > > > >
> > > > > The gating should probably be done so that it panics/halts the u-boot
> > > if gated
> > > > > pointer flags are used by u-boot code, because that will clearly be
> > > incorrect,
> > > > > without calling code ever knowing. This way the user will know that
> > > something
> > > > > is wrong and will have to fix the code.
> > > >
> > > > I'm not in favor of panic because of calling an unimplemented print
> > > > format character.  I guess we'll need to see what the size increase is
> > > > on un-guarding these formats and go from there.
> > >
> > > I'll look into it. I'm also not in favor of a panic.
> > >
> >
> > In lwIP, we're using macros for such format characters. Would it work to do
> > that here and make the compiler complain about an undefined symbol of the
> > macro for this extended format character isn't defined?
> 
> Maybe... Though, if we don't successfully police the usage of the
> macro, it won't help. I'd like to evaluate the code-size impact and
> maybe just always include it.

How about a one-time console warning if unsupported format modifier is used
at run-time, if panic is too aggressive?

Anything that will hint at the error, would be nice.

regards,
	o.

> -Joe
> 
> > Regards,
> > Simon
> > _______________________________________________
> > U-Boot mailing list
> > U-Boot at lists.denx.de
> > https://lists.denx.de/listinfo/u-boot
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot

[U-Boot] [PATCH] net: Always build the string_to_enetaddr() helper

[Ondřej Jirman]

Hello,

On Fri, Sep 13, 2019 at 07:40:22PM -0500, Joe Hershberger wrote:
> Part of the env cleanup moved this out of the environment code and into
> the net code. However, this helper is sometimes needed even when the net
> stack isn't included.
> 
> Move the helper to lib/net_utils.c like it's similarly-purposed
> string_to_ip(). Also rename the moved function to similar naming.

it looks like this patch was forgotten and not merged yet. The issue
in the comments is independent of this change. Is anything more needed
to get this merged?

regards,
	o.

> Signed-off-by: Joe Hershberger <joe.hershberger@ni.com>
> Reported-by: Ondrej Jirman <megous@megous.com>
> 
> ---
> 
>  arch/arm/mach-tegra/cboot.c         |  2 +-
>  board/renesas/sh7752evb/sh7752evb.c |  2 +-
>  board/renesas/sh7753evb/sh7753evb.c |  2 +-
>  board/renesas/sh7757lcr/sh7757lcr.c |  4 ++--
>  cmd/ethsw.c                         |  2 +-
>  cmd/nvedit.c                        |  2 +-
>  doc/README.enetaddr                 |  4 ++--
>  include/net.h                       | 24 +++++++++++++-----------
>  lib/net_utils.c                     | 15 +++++++++++++++
>  net/eth-uclass.c                    |  2 +-
>  net/eth_legacy.c                    |  2 +-
>  net/net.c                           | 12 ------------
>  12 files changed, 39 insertions(+), 34 deletions(-)
> 
> diff --git a/arch/arm/mach-tegra/cboot.c b/arch/arm/mach-tegra/cboot.c
> index 0433081c6c..0762144ecf 100644
> --- a/arch/arm/mach-tegra/cboot.c
> +++ b/arch/arm/mach-tegra/cboot.c
> @@ -495,7 +495,7 @@ static int cboot_get_ethaddr_legacy(const void *fdt, uint8_t mac[ETH_ALEN])
>  		return -ENOENT;
>  	}
>  
> -	eth_parse_enetaddr(prop, mac);
> +	string_to_enetaddr(prop, mac);
>  
>  	if (!is_valid_ethaddr(mac)) {
>  		printf("Invalid MAC address: %s\n", prop);
> diff --git a/board/renesas/sh7752evb/sh7752evb.c b/board/renesas/sh7752evb/sh7752evb.c
> index d0b850f35d..b4292b22e7 100644
> --- a/board/renesas/sh7752evb/sh7752evb.c
> +++ b/board/renesas/sh7752evb/sh7752evb.c
> @@ -93,7 +93,7 @@ static void set_mac_to_sh_giga_eth_register(int channel, char *mac_string)
>  	unsigned char mac[6];
>  	unsigned long val;
>  
> -	eth_parse_enetaddr(mac_string, mac);
> +	string_to_enetaddr(mac_string, mac);
>  
>  	if (!channel)
>  		ether = GETHER0_MAC_BASE;
> diff --git a/board/renesas/sh7753evb/sh7753evb.c b/board/renesas/sh7753evb/sh7753evb.c
> index e1bed7dcc3..5aebb041b0 100644
> --- a/board/renesas/sh7753evb/sh7753evb.c
> +++ b/board/renesas/sh7753evb/sh7753evb.c
> @@ -100,7 +100,7 @@ static void set_mac_to_sh_giga_eth_register(int channel, char *mac_string)
>  	unsigned char mac[6];
>  	unsigned long val;
>  
> -	eth_parse_enetaddr(mac_string, mac);
> +	string_to_enetaddr(mac_string, mac);
>  
>  	if (!channel)
>  		ether = GETHER0_MAC_BASE;
> diff --git a/board/renesas/sh7757lcr/sh7757lcr.c b/board/renesas/sh7757lcr/sh7757lcr.c
> index d2671202e9..662c435f7a 100644
> --- a/board/renesas/sh7757lcr/sh7757lcr.c
> +++ b/board/renesas/sh7757lcr/sh7757lcr.c
> @@ -140,7 +140,7 @@ static void set_mac_to_sh_eth_register(int channel, char *mac_string)
>  	unsigned char mac[6];
>  	unsigned long val;
>  
> -	eth_parse_enetaddr(mac_string, mac);
> +	string_to_enetaddr(mac_string, mac);
>  
>  	if (!channel)
>  		ether = ETHER0_MAC_BASE;
> @@ -159,7 +159,7 @@ static void set_mac_to_sh_giga_eth_register(int channel, char *mac_string)
>  	unsigned char mac[6];
>  	unsigned long val;
>  
> -	eth_parse_enetaddr(mac_string, mac);
> +	string_to_enetaddr(mac_string, mac);
>  
>  	if (!channel)
>  		ether = GETHER0_MAC_BASE;
> diff --git a/cmd/ethsw.c b/cmd/ethsw.c
> index 8846805799..8d271ce1f3 100644
> --- a/cmd/ethsw.c
> +++ b/cmd/ethsw.c
> @@ -864,7 +864,7 @@ static int keyword_match_mac_addr(enum ethsw_keyword_id key_id, int argc,
>  		return 0;
>  	}
>  
> -	eth_parse_enetaddr(argv[*argc_nr + 1], parsed_cmd->ethaddr);
> +	string_to_enetaddr(argv[*argc_nr + 1], parsed_cmd->ethaddr);
>  
>  	if (is_broadcast_ethaddr(parsed_cmd->ethaddr)) {
>  		memset(parsed_cmd->ethaddr, 0xFF, sizeof(parsed_cmd->ethaddr));
> diff --git a/cmd/nvedit.c b/cmd/nvedit.c
> index 1cb0bc1460..1e4b225a94 100644
> --- a/cmd/nvedit.c
> +++ b/cmd/nvedit.c
> @@ -360,7 +360,7 @@ ulong env_get_hex(const char *varname, ulong default_val)
>  
>  int eth_env_get_enetaddr(const char *name, uint8_t *enetaddr)
>  {
> -	eth_parse_enetaddr(env_get(name), enetaddr);
> +	string_to_enetaddr(env_get(name), enetaddr);
>  	return is_valid_ethaddr(enetaddr);
>  }
>  
> diff --git a/doc/README.enetaddr b/doc/README.enetaddr
> index f926485986..5baa9f2179 100644
> --- a/doc/README.enetaddr
> +++ b/doc/README.enetaddr
> @@ -76,12 +76,12 @@ To assist in the management of these layers, a few helper functions exist.  You
>  should use these rather than attempt to do any kind of parsing/manipulation
>  yourself as many common errors have arisen in the past.
>  
> -	* void eth_parse_enetaddr(const char *addr, uchar *enetaddr);
> +	* void string_to_enetaddr(const char *addr, uchar *enetaddr);
>  
>  Convert a string representation of a MAC address to the binary version.
>  char *addr = "00:11:22:33:44:55";
>  uchar enetaddr[6];
> -eth_parse_enetaddr(addr, enetaddr);
> +string_to_enetaddr(addr, enetaddr);
>  /* enetaddr now equals { 0x00, 0x11, 0x22, 0x33, 0x44, 0x55 } */
>  
>  	* int eth_env_get_enetaddr(char *name, uchar *enetaddr);
> diff --git a/include/net.h b/include/net.h
> index 75a16e4c8f..d054c5f694 100644
> --- a/include/net.h
> +++ b/include/net.h
> @@ -825,6 +825,19 @@ static inline void net_random_ethaddr(uchar *addr)
>  	addr[0] |= 0x02;	/* set local assignment bit (IEEE802) */
>  }
>  
> +/**
> + * string_to_enetaddr() - Parse a MAC address
> + *
> + * Convert a string MAC address
> + *
> + * Implemented in lib/net_utils.c (built unconditionally)
> + *
> + * @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 string_to_enetaddr(const char *addr, uint8_t *enetaddr);
> +
>  /* Convert an IP address to a string */
>  void ip_to_string(struct in_addr x, char *s);
>  
> @@ -875,15 +888,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/lib/net_utils.c b/lib/net_utils.c
> index 9fb9d4a4b0..ed5044c3de 100644
> --- a/lib/net_utils.c
> +++ b/lib/net_utils.c
> @@ -41,3 +41,18 @@ struct in_addr string_to_ip(const char *s)
>  	addr.s_addr = htonl(addr.s_addr);
>  	return addr;
>  }
> +
> +void string_to_enetaddr(const char *addr, uint8_t *enetaddr)
> +{
> +	char *end;
> +	int i;
> +
> +	if (!enetaddr)
> +		return;
> +
> +	for (i = 0; i < 6; ++i) {
> +		enetaddr[i] = addr ? simple_strtoul(addr, &end, 16) : 0;
> +		if (addr)
> +			addr = (*end) ? end + 1 : end;
> +	}
> +}
> diff --git a/net/eth-uclass.c b/net/eth-uclass.c
> index 3bd98b01ad..9fe4096120 100644
> --- a/net/eth-uclass.c
> +++ b/net/eth-uclass.c
> @@ -227,7 +227,7 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op,
>  		switch (op) {
>  		case env_op_create:
>  		case env_op_overwrite:
> -			eth_parse_enetaddr(value, pdata->enetaddr);
> +			string_to_enetaddr(value, pdata->enetaddr);
>  			eth_write_hwaddr(dev);
>  			break;
>  		case env_op_delete:
> diff --git a/net/eth_legacy.c b/net/eth_legacy.c
> index 41f5263526..5d6b0d7d7f 100644
> --- a/net/eth_legacy.c
> +++ b/net/eth_legacy.c
> @@ -117,7 +117,7 @@ static int on_ethaddr(const char *name, const char *value, enum env_op op,
>  			switch (op) {
>  			case env_op_create:
>  			case env_op_overwrite:
> -				eth_parse_enetaddr(value, dev->enetaddr);
> +				string_to_enetaddr(value, dev->enetaddr);
>  				eth_write_hwaddr(dev, "eth", dev->index);
>  				break;
>  			case env_op_delete:
> 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;
> -	}
> -}
> -- 
> 2.11.0
> 
> _______________________________________________
> U-Boot mailing list
> U-Boot at lists.denx.de
> https://lists.denx.de/listinfo/u-boot