[U-Boot] [PATCH 11/11] tools: Add tool to add crc8 to a mac address

[Simon Glass]

Hi Olliver, (is it one l or two?)

On 8 November 2016 at 08:54, Olliver Schinagl <oliver at schinagl.nl> wrote:
> This patch adds a little tool that takes a generic MAC address and
> generates a CRC byte for it. The output is the full MAC address without
> any separators, ready written into an EEPROM.
>
> Signed-off-by: Olliver Schinagl <o.schinagl at ultimaker.com>
> ---
>  tools/.gitignore        |  1 +
>  tools/Makefile          |  4 ++++
>  tools/gen_ethaddr_crc.c | 52 +++++++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 57 insertions(+)
>  create mode 100644 tools/gen_ethaddr_crc.c
>
> diff --git a/tools/.gitignore b/tools/.gitignore
> index cb1e722..0d1f076 100644
> --- a/tools/.gitignore
> +++ b/tools/.gitignore
> @@ -6,6 +6,7 @@
>  /fit_check_sign
>  /fit_info
>  /gen_eth_addr
> +/gen_ethaddr_crc
>  /ifdtool
>  /img2srec
>  /kwboot
> diff --git a/tools/Makefile b/tools/Makefile
> index 06afdb0..4879ded 100644
> --- a/tools/Makefile
> +++ b/tools/Makefile
> @@ -43,6 +43,10 @@ envcrc-objs := envcrc.o lib/crc32.o common/env_embedded.o lib/sha1.o
>  hostprogs-$(CONFIG_CMD_NET) += gen_eth_addr
>  HOSTCFLAGS_gen_eth_addr.o := -pedantic
>
> +hostprogs-$(CONFIG_CMD_NET) += gen_ethaddr_crc
> +gen_ethaddr_crc-objs := gen_ethaddr_crc.o lib/crc8.o
> +HOSTCFLAGS_gen_ethaddr_crc.o := -pedantic
> +
>  hostprogs-$(CONFIG_CMD_LOADS) += img2srec
>  HOSTCFLAGS_img2srec.o := -pedantic
>
> diff --git a/tools/gen_ethaddr_crc.c b/tools/gen_ethaddr_crc.c
> new file mode 100644
> index 0000000..9b5bdb0
> --- /dev/null
> +++ b/tools/gen_ethaddr_crc.c
> @@ -0,0 +1,52 @@
> +/*
> + * (C) Copyright 2016
> + * Olliver Schinagl <oliver at schinagl.nl>
> + *
> + * SPDX-License-Identifier:    GPL-2.0+
> + */
> +
> +#include <ctype.h>
> +#include <stdint.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <u-boot/crc.h>
> +
> +#define ARP_HLEN 6 /* Length of hardware address */

Is there no #define for this in standard headers?

> +
> +int main(int argc, char *argv[])
> +{
> +       uint_fast8_t i;
> +       uint8_t mac_addr[ARP_HLEN + 1] = { 0x00 };

Why do you need to init it?

> +
> +       if (argc < 2) {
> +               puts("Please supply a MAC address.");

How about a usage() function which gives generic help as well?

> +               return -1;
> +       }
> +
> +       if (!((strlen(argv[1]) == 12) || (strlen(argv[1]) == 17))) {
> +               puts("Please supply a valid MAC address with optionaly\n"

optionally. But do you mean 'Please supply a valid MAC address with
optional - or : separators?'

> +                    "dashes (-) or colons (:) as seperators.");

separators.

> +               return -1;
> +       }
> +
> +       i = 0;
> +       while (*argv[1] != '\0') {

How about putting this code in a separate function:

int process_mac(const char *max)

so you don't have to access argv[1] all the time. Also you can return
1 instead of -1 from main() on error.

> +               char nibble[2] = { 0x00, '\n' }; /* for strtol */
> +
> +               nibble[0] = *argv[1]++;
> +               if (isxdigit(nibble[0])) {
> +                       if (isupper(nibble[0]))
> +                               nibble[0] = tolower(nibble[0]);
> +                       mac_addr[i >> 1] |= strtol(nibble, NULL, 16) << ((i % 2) ? 0 : 4) & ((i % 2) ? 0x0f : 0xf0);

How about a nibble_to_hex() function here? This is strange-looking
code. I'm wondering what you are trying to do.

> +                       i++;
> +               }
> +       }
> +       mac_addr[ARP_HLEN] = crc8(0, mac_addr, ARP_HLEN);

I suggest putting this in a separate variable...

> +
> +       for (i = 0; i < ARP_HLEN + 1; i++)
> +               printf("%.2x", mac_addr[i]);
> +       putchar('\n');

and here: printf("%.2x\n", crc)

> +
> +       return 0;
> +}
> --
> 2.10.2
>

Regards,
Simon