[OpenWrt-Devel] [PATCH 1/3] gemini: Make a per-board case for ethernet MAC

[Christian Lamparter]

Hello, 

I have a few suggestions inline below.

On Friday, May 24, 2019 11:27:17 PM CEST Linus Walleij wrote:
> The DNS-313 isn't the only special board so let's bite the
> bullet and create a case ladder in preparation for DIR-685.
> 
> Signed-off-by: Linus Walleij <linus.walleij at linaro.org>
> ---
>  .../lib/preinit/05_set_ether_mac_gemini       | 30 ++++++++++++-------
>  1 file changed, 19 insertions(+), 11 deletions(-)
> 
> diff --git a/target/linux/gemini/base-files/lib/preinit/05_set_ether_mac_gemini b/target/linux/gemini/base-files/lib/preinit/05_set_ether_mac_gemini
> index 1ce5c8067ef0..ebd3ae0f55c5 100644
> --- a/target/linux/gemini/base-files/lib/preinit/05_set_ether_mac_gemini
> +++ b/target/linux/gemini/base-files/lib/preinit/05_set_ether_mac_gemini
> @@ -1,6 +1,25 @@
>  #!/bin/sh
>  
>  set_ether_mac() {
> +
> +	. /lib/functions.sh
> +
> +	case $(board_name) in
> +	dlink,dns-313)
> +		# The DNS-313 has a special field in its RedBoot
> +		# binary that we need to check
> +		CONFIG_PARTITION="$(grep "RedBoot" /proc/mtd | cut -d: -f1)"
> +		if [ ! -z $CONFIG_PARTITION ] ; then
This looks familiar. From afar, this is almost like
package/base-files/files/lib/functions.sh's find_mtd_part with an extra check.

> +			DEVID="$(dd if=/dev/mtdblock0 bs=1 skip=119508 count=7 2>/dev/null)"
I think find_mtd_part's result from above would be the perfect input
for dd if=... 
> +			if [ "x$DEVID" = "xdns-313" ] ; then
> +				MAC1="$(dd if=/dev/mtdblock0 bs=1 skip=119540 count=6 2>/dev/null | hexdump -n6 -e '/1 ":%02X"' | sed s/^://g)"
Isn't this like package/base-files/files/lib/functions/system.sh's
mtd_get_mac_binary function?

What does 

. /lib/functions.sh
. /lib/functions/system.sh
echo $(mtd_get_mac_binary RedBoot 119540)

produce? it should be the same MAC.

if it is you could use the get_mac_binary with the find_mtd_part from above
and get the mac this way.

> +				ifconfig eth0 hw ether $MAC1 2>/dev/null
I guess while we are at it, why not change it to
"ip link set dev eth0 address $MAC1"

in case the ifconfig deprecation ever materializes.

> +				return 0
> +			fi
> +		fi
> +		;;
> +	esac
> +
>  	# Most devices have a standard "VCTL" partition
>  	CONFIG_PARTITION="$(grep "VCTL" /proc/mtd | cut -d: -f1)"
>  	if [ ! -z $CONFIG_PARTITION ] ; then
> @@ -12,17 +31,6 @@ set_ether_mac() {
>  		return 0
>  	fi
>  
> -	# The DNS-313 has a special field in its RedBoot
> -	# binary that we need to check
> -	CONFIG_PARTITION="$(grep "RedBoot" /proc/mtd | cut -d: -f1)"
> -	if [ ! -z $CONFIG_PARTITION ] ; then
> -		DEVID="$(dd if=/dev/mtdblock0 bs=1 skip=119508 count=7 2>/dev/null)"
> -		if [ "x$DEVID" = "xdns-313" ] ; then
> -			MAC1="$(dd if=/dev/mtdblock0 bs=1 skip=119540 count=6 2>/dev/null | hexdump -n6 -e '/1 ":%02X"' | sed s/^://g)"
> -			ifconfig eth0 hw ether $MAC1 2>/dev/null
> -			return 0
> -		fi
> -	fi
>  }
>  
>  boot_hook_add preinit_main set_ether_mac
> 





_______________________________________________
openwrt-devel mailing list
openwrt-devel at lists.openwrt.org
https://lists.openwrt.org/mailman/listinfo/openwrt-devel