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

[Olliver Schinagl]

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