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

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

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

On 8 November 2016 at 08:54, Olliver Schinagl <oliver@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@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@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

On Fri, Nov 11, 2016 at 10:18 AM, Simon Glass <sjg@chromium.org> wrote:
> Hi Olliver, (is it one l or two?)
>
> On 8 November 2016 at 08:54, Olliver Schinagl <oliver@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@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@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?

There is. ARP_HLEN is defined in include/net.h

Hey Simon, Joe,


On 15-11-16 04:31, Joe Hershberger wrote:
> On Fri, Nov 11, 2016 at 10:18 AM, Simon Glass <sjg@chromium.org> wrote:
>> Hi Olliver, (is it one l or two?)
>>
>> On 8 November 2016 at 08:54, Olliver Schinagl <oliver@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@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@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?
> There is. ARP_HLEN is defined in include/net.h
Yep, but including net.h then makes net.h very unhappy (lots of missing 
things, u32 to begin with) then you add those includes and the party 
continues with lots of other missing includes. I managed to get all 
those includes sorted at some point, but then using the functions 
initially suggested by Joe, caused a whole lot of other library and 
include files to be missing. So I didn't think it was worth the effort.

Thus I suggest, merge as is, and if someone wants to start cleaning it 
up (by fixing net.h) that would be awesome. The idea for this tool was 
to be able to quickly generate a crc8 code :)

Hey Simon,


On 11-11-16 17:18, Simon Glass wrote:
> Hi Olliver, (is it one l or two?)
>
> On 8 November 2016 at 08:54, Olliver Schinagl <oliver@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@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@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?
Because I did it in the net stuff as well, where I used the 
'is_zero_mac()'' to indicate things where not setup properly. I guess it 
can go here ...
>
>> +
>> +       if (argc < 2) {
>> +               puts("Please supply a MAC address.");
> How about a usage() function which gives generic help as well?
fair point, You caught me on being lazy :) I didn't think of it for such 
a simple tool. I'll put it in the next version.
>
>> +               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.
right, no prob.
>
>> +               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.
It is strange, and we even have a nice function in u-boot I belive, but 
it's a pain to get to add it to this, hence the manual way. I'll add 
some doc to the func as well.
>
>> +                       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)
yeah i do like the separate var for here.
>
>> +
>> +       return 0;
>> +}
>> --
>> 2.10.2
>>
> Regards,
> Simon