Re: [PATCH u-boot-mvebu v3 05/18] arm: mvebu: turris_omnia: Implement getting board information from MCU

Stefan Roese
2024-03-28T12:54:00Z

On 3/28/24 12:17, Marek Behún wrote:
> On Thu, 28 Mar 2024 10:56:01 +0100
> Stefan Roese <sr@denx.de> wrote:
>
>> On 3/27/24 17:23, Marek Behún wrote:
>>> Implement reading board serial number, first MAC address and board
>>> version from MCU. MCU supports board information if the FEAT_BOARD_INFO
>>> feature bit is set in MCU features.
>>>
>>> Prefer getting board information from MCU if supported, fallback to
>>> Atmel SHA chip.
>>>
>>> Signed-off-by: Marek Behún <kabel@kernel.org>
>>
>> Minor comment below. Other than this:
>>
>> Reviewed-by: Stefan Roese <sr@denx.de>
>>
>> Thanks,
>> Stefan
>>
>>> ---
>>> board/CZ.NIC/turris_atsha_otp.c | 27 +------
>>> board/CZ.NIC/turris_omnia/Makefile | 2 +-
>>> board/CZ.NIC/turris_omnia/turris_omnia.c | 94 +++++++++++++++++++++++-
>>> 3 files changed, 93 insertions(+), 30 deletions(-)
>>>
>>> diff --git a/board/CZ.NIC/turris_atsha_otp.c b/board/CZ.NIC/turris_atsha_otp.c
>>> index a29fe36231..85eebcdf18 100644
>>> --- a/board/CZ.NIC/turris_atsha_otp.c
>>> +++ b/board/CZ.NIC/turris_atsha_otp.c
>>> @@ -11,6 +11,7 @@
>>> #include <atsha204a-i2c.h>
>>>
>>> #include "turris_atsha_otp.h"
>>> +#include "turris_common.h"
>>>
>>> #define TURRIS_ATSHA_OTP_VERSION 0
>>> #define TURRIS_ATSHA_OTP_SERIAL 1
>>> @@ -32,26 +33,6 @@ static struct udevice *get_atsha204a_dev(void)
>>> return dev;
>>> }
>>>
>>> -static void increment_mac(u8 *mac)
>>> -{
>>> - int i;
>>> -
>>> - for (i = 5; i >= 3; i--) {
>>> - mac[i] += 1;
>>> - if (mac[i])
>>> - break;
>>> - }
>>> -}
>>> -
>>> -static void set_mac_if_invalid(int i, u8 *mac)
>>> -{
>>> - u8 oldmac[6];
>>> -
>>> - if (is_valid_ethaddr(mac) &&
>>> - !eth_env_get_enetaddr_by_index("eth", i, oldmac))
>>> - eth_env_set_enetaddr_by_index("eth", i, mac);
>>> -}
>>> -
>>> int turris_atsha_otp_init_mac_addresses(int first_idx)
>>> {
>>> struct udevice *dev = get_atsha204a_dev();
>>> @@ -84,11 +65,7 @@ int turris_atsha_otp_init_mac_addresses(int first_idx)
>>> mac[4] = mac1[2];
>>> mac[5] = mac1[3];
>>>
>>> - set_mac_if_invalid((first_idx + 0) % 3, mac);
>>> - increment_mac(mac);
>>> - set_mac_if_invalid((first_idx + 1) % 3, mac);
>>> - increment_mac(mac);
>>> - set_mac_if_invalid((first_idx + 2) % 3, mac);
>>> + turris_init_mac_addresses(first_idx, mac);
>>>
>>> return 0;
>>> }
>>> diff --git a/board/CZ.NIC/turris_omnia/Makefile b/board/CZ.NIC/turris_omnia/Makefile
>>> index dc39b44ae1..341378b4e5 100644
>>> --- a/board/CZ.NIC/turris_omnia/Makefile
>>> +++ b/board/CZ.NIC/turris_omnia/Makefile
>>> @@ -2,4 +2,4 @@
>>> #
>>> # Copyright (C) 2017 Marek Behún <kabel@kernel.org>
>>>
>>> -obj-y := turris_omnia.o ../turris_atsha_otp.o
>>> +obj-y := turris_omnia.o ../turris_atsha_otp.o ../turris_common.o
>>> diff --git a/board/CZ.NIC/turris_omnia/turris_omnia.c b/board/CZ.NIC/turris_omnia/turris_omnia.c
>>> index 6dfde5ee7a..f63640ad64 100644
>>> --- a/board/CZ.NIC/turris_omnia/turris_omnia.c
>>> +++ b/board/CZ.NIC/turris_omnia/turris_omnia.c
>>> @@ -18,6 +18,7 @@
>>> #include <asm/io.h>
>>> #include <asm/arch/cpu.h>
>>> #include <asm/arch/soc.h>
>>> +#include <asm/unaligned.h>
>>> #include <dm/uclass.h>
>>> #include <dt-bindings/gpio/gpio.h>
>>> #include <fdt_support.h>
>>> @@ -25,12 +26,14 @@
>>> #include <time.h>
>>> #include <turris-omnia-mcu-interface.h>
>>> #include <linux/bitops.h>
>>> +#include <linux/bitrev.h>
>>> #include <linux/delay.h>
>>> #include <u-boot/crc.h>
>>>
>>> #include "../drivers/ddr/marvell/a38x/ddr3_init.h"
>>> #include <../serdes/a38x/high_speed_env_spec.h>
>>> #include "../turris_atsha_otp.h"
>>> +#include "../turris_common.h"
>>>
>>> DECLARE_GLOBAL_DATA_PTR;
>>>
>>> @@ -186,6 +189,70 @@ static bool omnia_mcu_has_feature(u32 feature)
>>> return feature & features;
>>> }
>>>
>>> +static u32 omnia_mcu_crc32(const void *p, size_t len)
>>> +{
>>> + u32 val, crc = 0;
>>> +
>>> + compiletime_assert(!(len % 4), "length has to be a multiple of 4");
>>> +
>>> + while (len) {
>>> + val = bitrev32(get_unaligned_le32(p));
>>> + crc = crc32(crc, (void *)&val, 4);
>>> + p += 4;
>>> + len -= 4;
>>> + }
>>> +
>>> + return ~bitrev32(crc);
>>> +}
>>> +
>>> +/* Can only be called after relocation, since it needs cleared BSS */
>>> +static int omnia_mcu_board_info(char *serial, u8 *mac, char *version)
>>> +{
>>> + static u8 reply[17];
>>> + static bool cached;
>>> +
>>> + if (!cached) {
>>> + u8 csum;
>>> + int ret;
>>> +
>>> + ret = omnia_mcu_read(CMD_BOARD_INFO_GET, reply, sizeof(reply));
>>> + if (ret)
>>> + return ret;
>>> +
>>> + if (reply[0] != 16)
>>> + return -EBADMSG;
>>> +
>>> + csum = reply[16];
>>> + reply[16] = 0;
>>> +
>>> + if ((omnia_mcu_crc32(&reply[1], 16) & 0xff) != csum)
>>> + return -EBADMSG;
>>> +
>>> + cached = true;
>>> + }
>>> +
>>> + if (serial) {
>>> + const char *serial_env;
>>> +
>>> + serial_env = env_get("serial#");
>>> + if (serial_env && strlen(serial_env) == 16) {
>>> + strcpy(serial, serial_env);
>>
>> Usage of strcpy() is discouraged AFAIK.
>
> Yeah, it would make sense to use strncpy, but the potential leak is
> prevented by strlen() check.

strncpy is also not optimal. Please see here if you are interested:

https://www.geeksforgeeks.org/why-strcpy-and-strncpy-are-not-safe-to-use/

AFAIU, there is no pressing need to change this implementation here
though right now. I just wanted to make this comment.

Thanks,
Stefan