Zephyr: drivers: hwinfo: shell command "hwinfo devid" output ignores endianness

[sslupsky]

Describe the bug
When using the "hwinfo devid" shell command to display the device id, the command outputs the device id but the bytes of each word comprising the ID appear to print in the wrong order. I believe this is because the print statements do not appear to consider endianness.

To Reproduce
Steps to reproduce the behavior:
Compile app with CONFIG_HWINFO and CONFIG_HWINFO_SHELL

Step A: From main on sam0 arch, print the device id:

u32_t dev_id[4];
length = hwinfo_get_device_id((u8_t *) dev_id, sizeof(dev_id));
LOG_INF("Device ID: 0x%08x%08x%08x%08x", dev_id[0], dev_id[1], dev_id[2], dev_id[3]);

Step B: Invoke the shell command from the console:
hwinfo devid

Step C: Compare results.

Expected behavior
Step A on sam0 produces:
Device ID: 0x2f89032e5050323339202020ff0a0c0f

Step B on sam0 produces:
Length: 16
ID: 0x2e03892f33325050202020390f0c0aff

Comparison shows the 32 bit words that comprise the ID are in the correct order. However, the 4 bytes that comprise each word are reversed.

Impact
Minor annoyance but could be a major issue for anyone using the ID as a UID for something like a firmware update, networking, etc.

Screenshots or console output
The code that outputs the id is:
https://github.com/zephyrproject-rtos/zephyr/blob/687d1040ac698d76b9e40de19906b27b59e31f49/drivers/hwinfo/hwinfo_shell.c#L32-L34

The SAMD21 data sheet describes the ID as follows:
_Each device has a unique 128-bit serial number which is a concatenation of four 32-bit words_

From the datasheet description, does this suggest the ID should be output using Big Endian format?

[alexanderwachter]

IMO it is not an issue. The Zephyr device-id is a sequence of bytes. We make a 1:1 copy from the registers in the flash module. You should interpret the device ID as a byte sequence because other devices don't interpret them as u32_t.

[sslupsky]

I agree with your assertion that the device ID should be interpreted as a byte sequence. However, that is not how the data is being read from the hardware. Rather, the bytes are read from the registers as integer values (u32_t) which inherently have endianness and then cast as a sequence of byte which potentially breaks the endianness of the integers.

Here are a couple examples of this taken from the sam0 and nordic drivers:

https://github.com/zephyrproject-rtos/zephyr/blob/687d1040ac698d76b9e40de19906b27b59e31f49/drivers/hwinfo/hwinfo_sam0.c#L11-L22

https://github.com/zephyrproject-rtos/zephyr/blob/687d1040ac698d76b9e40de19906b27b59e31f49/drivers/hwinfo/hwinfo_nrf.c#L12-L21

The data structure receiving contents of the hardware registers is declared as a struct with u32_t members. Each one of those members has endianness. When you simply copy them using memcpy, endianness is not accounted for. In this case, ARM is little endian so the words are byte reversed.

[alexanderwachter]

Using memcpy on the struct just makes a 1:1 copy. It does not change the endianness. The registers are interpreted as they were in a contiguous memory region somewhere in the registers.
You can choose how to interpret them, for example, pass a struct that is the same as in the driver and then you will interpret them with endianness. IMO we should not make an assumption on how to interpret the bytes in the shell.

[sslupsky]

Correct, memcpy makes a copy of the struct. However, the data types that _comprise_ the struct have endianness.

[carlescufi]

@alexanderwachter I think @sslupsky is right, the fields should be in the CPU endianness, will you send a patch? or @sslupsky would you consider sending one?

[pabigot]

@carlescufi I think that if the vendor has a preferred expression of the hardware identifier as formed from the raw data that should be followed. IOW if the hardware identifier for Nordic is low-word then high-word little-endian, maybe there's some swapping to be done to get an octet sequence that matches the intended one, e.g. for Bluetooth addresses.

I agree with @alexanderwachter that a memcpy of the raw bytes from the starting address is a legitimate interpretation of the hardware id, unless the vendor specifies that the value has a canonical representation. I don't know how Atmel expects people to interpret the hardware identifier values.

But for the purposes of this API the interpretation should be a sequence of octets. It should not be reversed when creating a representation, as noted in #24103.

[sslupsky]

I agree with @alexanderwachter that a memcpy of the raw bytes from the starting address is a legitimate interpretation of the hardware id, unless the vendor specifies that the value has a canonical representation.

@pabigot My point is what you described is not what is happening here. The driver does not use memcpy to read the raw bytes from the starting address. It first reads the data as u32_t and then uses memcpy to copy the data. This approach ensures there is endianness in the data.

[pabigot]

@pabigot My point is what you described is not what is happening here. The driver does not use memcpy to read the raw bytes from the starting address. It first reads the data as u32_t and then uses memcpy to copy the data. This approach ensures there is endianness in the data.

If it reads and writes u32_t in CPU endianness and then copies the bytes, it's equivalent to copying the bytes without involving u32_t. It may be the read is 32-bit only because the registers don't support byte access.

Nonetheless, I think we agree that if Nordic wants hwinfo to correctly represent the fact that the hardware identifier in this is a 64-bit value that is stored in a little-endian representation, then it's the responsibility of the SOC hwinfo implementation to reverse it so the unswapped byte order represents the expected byte sequence. Consumers of hwinfo data cannot be expected to do transformations on the bytes based on vendor-intended interpretations.

[sslupsky]

Agreed, consumers of the hwinfo data should not be expected to do transformations on the bytes. That is at the core of what I am trying to get to consensus with on this issue. The consensus on this determines if the root cause of the problem is:

1. the underlying driver(s), or
2. how the information is output with hwinfo_shell.c.

Your suggestion, and I believe @alexanderwachter said this too, is that the data structure should be a "sequence of bytes". I agree.

So, that confirmation then suggests that the driver(s) should be responsible for ensuring that the data structure is a sequence of bytes. That is not what the current sam0 and nordic drivers do. The drivers read the data as u32_t and then hwinfo_shell outputs the data as a sequence of bytes, which of course then has endianness of the underlying MCU, which in this case is Cortex M0 which is little endian.

This issue likely extends to all the other drivers in the tree where the MCU is little endian.

[sslupsky]

I think the change can be done efficiently using sys/byteorder.h as follows:
sam0:

diff --git a/drivers/hwinfo/hwinfo_sam0.c b/drivers/hwinfo/hwinfo_sam0.c
index 309989ad24..b3275035f0 100644
--- a/drivers/hwinfo/hwinfo_sam0.c
+++ b/drivers/hwinfo/hwinfo_sam0.c
@@ -9,6 +9,7 @@
 #include <soc.h>
 #include <drivers/hwinfo.h>
 #include <string.h>
+#include <sys/byteorder.h>

 struct sam0_uid {
        u32_t id[4];
@@ -18,10 +19,10 @@ ssize_t z_impl_hwinfo_get_device_id(u8_t *buffer, size_t length)
 {
        struct sam0_uid dev_id;

-       dev_id.id[0] = *(const u32_t *) DT_INST_REG_ADDR_BY_IDX(0, 0);
-       dev_id.id[1] = *(const u32_t *) DT_INST_REG_ADDR_BY_IDX(0, 1);
-       dev_id.id[2] = *(const u32_t *) DT_INST_REG_ADDR_BY_IDX(0, 2);
-       dev_id.id[3] = *(const u32_t *) DT_INST_REG_ADDR_BY_IDX(0, 3);
+       dev_id.id[0] = sys_cpu_to_be32(*(const u32_t *) DT_INST_REG_ADDR_BY_IDX(0, 0));
+       dev_id.id[1] = sys_cpu_to_be32(*(const u32_t *) DT_INST_REG_ADDR_BY_IDX(0, 1));
+       dev_id.id[2] = sys_cpu_to_be32(*(const u32_t *) DT_INST_REG_ADDR_BY_IDX(0, 2));
+       dev_id.id[3] = sys_cpu_to_be32(*(const u32_t *) DT_INST_REG_ADDR_BY_IDX(0, 3));

        if (length > sizeof(dev_id.id)) {
                length = sizeof(dev_id.id);

nordic:

diff --git a/drivers/hwinfo/hwinfo_nrf.c b/drivers/hwinfo/hwinfo_nrf.c
index 6c2822d891..d29334635f 100644
--- a/drivers/hwinfo/hwinfo_nrf.c
+++ b/drivers/hwinfo/hwinfo_nrf.c
@@ -8,6 +8,7 @@
 #include <drivers/hwinfo.h>
 #include <string.h>
 #include <hal/nrf_ficr.h>
+#include <sys/byteorder.h>

 struct nrf_uid {
        u32_t id[2];
@@ -17,8 +18,8 @@ ssize_t z_impl_hwinfo_get_device_id(u8_t *buffer, size_t length)
 {
        struct nrf_uid dev_id;

-       dev_id.id[0] = nrf_ficr_deviceid_get(NRF_FICR, 0);
-       dev_id.id[1] = nrf_ficr_deviceid_get(NRF_FICR, 1);
+       dev_id.id[0] = sys_cpu_to_be32(nrf_ficr_deviceid_get(NRF_FICR, 0));
+       dev_id.id[1] = sys_cpu_to_be32(nrf_ficr_deviceid_get(NRF_FICR, 1));

        if (length > sizeof(dev_id.id)) {
                length = sizeof(dev_id.id);

I have tested the sam0 driver and it works fine. Can you please test the nordic patch and let me know if it provides what you want? Do the words need to be swapped as well in the nordic driver?

[pabigot]

I believe (without testing) that Nordic must be:

+       dev_id.id[0] = sys_cpu_to_be32(nrf_ficr_deviceid_get(NRF_FICR, 1));
+       dev_id.id[1] = sys_cpu_to_be32(nrf_ficr_deviceid_get(NRF_FICR, 0));

as the stored value is a little-endian 64-bit integer. That's why reversing the entire 8-byte sequence is being done for USB.

[sslupsky]

@carlescufi submitted a PR for this:
https://github.com/zephyrproject-rtos/zephyr/pull/24203

[sslupsky]

I tested the sam0 but cannot test the nordic change. I conferred with @pabigot about the change to the nordic driver and it looks ok. It was a slightly different change than the sam0 driver because of the word size.