drivers: hwinfo: fix endianness for sam0 and nrf #24203

sslupsky · 2020-04-08T19:04:28Z

HWINFO drivers 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 memcpy the data to a
buffer. This ensures the data has the endianness of the
underlying MCU, which in this case is Cortex M0 which
is little endian.

This commit fixes the endianness so the data can be
interpreted as a "left to right sequence of bytes".

Fixes #23444
Fixes #24103

pabigot

Nordic code behaves as I expect, converting the 64-bit little-endian integer to a big-endian octet sequence.

Atmel code looks right by inspection given SAM D21 Family Data Sheet section 10.3.3:

Each device has a unique 128-bit serial number which is a concatenation of four 32-bit words contained at the following addresses.... The uniqueness of the serial number is guaranteed only when using all 128 bits.

alexanderwachter · 2020-04-09T13:35:17Z

This change has a huge impact for existing devices. They will change their ID after updating the Zephyr version! Please announce this change in the Mailing list first and maybe ask @carlescufi for a slot to discuss it in the API meeting.

carlescufi · 2020-04-09T14:55:23Z

@sslupsky here's what I suggest we do with this change:

Add an entry for this in the "API Changes" section of the 2.3 release notes in this PR
Send an email to the mailing list

carlescufi · 2020-04-09T14:55:49Z

DNM for now

needs docs

sslupsky · 2020-04-09T15:03:50Z

@carlescufi This PR corrects a bug in the existing implementation for the sam0 and nordic drivers. The change does have an impact on other zephyr components as @pabigot pointed our regarding the Bluetooth workaround for this bug. That workaround will have to be addressed. I can include this in the "API Changes" but is it correct to call this an "API change"?

There may be other drivers affected but some are ok. For instance, I took a quick look at the sam driver yesterday and that one appears to be ok.

pabigot

Since this was apparently not clear originally (so things like USB were making assumptions about how to interpret it):

The documentation for this API should be updated to make clear that the returned value is not supposed to be interpreted based on vendor-specific assumptions of byte order. It should express the identifier as a raw byte sequence, doing any endian conversion necessary so that a hex representation of the bytes produces the intended serial number.

sslupsky · 2020-04-09T15:12:33Z

Does sound ok for the API Changes:

HWINFO
- HWINFO drivers are responsible for ensuring that the ID data structure is a
  sequence of bytes. The returned ID value is not supposed to be interpreted
  based on vendor-specific assumptions of byte order. It should express the
  identifier as a raw byte sequence, doing any endian conversion necessary so
  that a hex representation of the bytes produces the intended serial number.

pabigot · 2020-04-09T15:15:52Z

Does sound ok for the API Changes:

That looks like release notes. The hwinfo.h header needs to include this information.

sslupsky · 2020-04-09T15:16:59Z

I added the text to hwinfo.h as well.

sslupsky · 2020-04-09T15:19:00Z

I will skinny the release notes to:

HWINFO
- HWINFO drivers are responsible for ensuring that the ID data structure is a
  sequence of bytes.

alexanderwachter · 2020-04-09T15:19:46Z

@carlescufi This PR corrects a bug in the existing implementation for the sam0 and nordic drivers.

IMO it is still not a bug since the device doesn't apply any structure on the ID. But I agree that tools in production may interpret the values differently. So changing the byte-order is ok for me.

The change does have an impact on other zephyr components as @pabigot pointed our regarding the Bluetooth workaround for this bug. That workaround will have to be addressed. I can include this in the "API Changes" but is it correct to call this an "API change"?

The changes have an impact on users that use the hwinfo API to identify their devices!
That's a big surprise for everyone if the device changes the HARDWARE ID after a SOFTWARE update,

There may be other drivers affected but some are ok. For instance, I took a quick look at the sam driver yesterday and that one appears to be ok.

As mentioned before, this is a public API that is used by other users too. AFAIK foundries.io uses it to identify devices.
If the impact of this "bug" isn't too bad, I would recommend not to fix it.

carlescufi · 2020-04-09T15:20:51Z

As mentioned before, this is a public API that is used by other users too. AFAIK foundries.io uses it to identify devices.
If the impact of this "bug" isn't too bad, I would recommend not to fix it.

Right, but the API was not stable until after the 2.2 release, so I see not problem in switching the endianness at this point. We have not pushed a release with the hwinfo API being stable, so I still support this change.

carlescufi · 2020-04-09T15:25:51Z

I added the text to hwinfo.h as well.

@sslupsky
have you pushed? I don't see it in the PR yet

sslupsky · 2020-04-09T15:27:59Z

I pushed the recommended changes.

carlescufi · 2020-04-09T15:42:01Z

doc/releases/release-notes-2.3.rst

@@ -21,6 +21,11 @@ No security vulnerabilities received.
 API Changes
 ***********

+* HWINFO
+
+  * HWINFO drivers are responsible for ensuring that the ID data structure is a


@sslupsky could you add a bit more info here? About the actual change in API behavior, so users know what to expect. Please also list which platforms in particular changed.

Ok. Clarified the behaviour and referenced the platforms changed.

pabigot

Looks good. As a follow-up we need to review all existing implementations to ensure that what they produce conforms to the "don't rearrange this" policy.

sslupsky · 2020-04-09T15:45:46Z

Which mailing list? users?

carlescufi · 2020-04-09T15:46:28Z

Which mailing list? users?

Yes, users@ and devel@ would be good actually. Thanks!

HWINFO drivers 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 memcpy the data to a buffer. This ensures the data has the endianness of the underlying MCU, which in this case is Cortex M0 which is little endian. This commit fixes the endianness so the data can be interpreted as a "left to right sequence of bytes". This commit updates the API doc to provide clarification of the data structure. Add to 2.3 release notes. Fixes zephyrproject-rtos#23444, zephyrproject-rtos#24103 Signed-off-by: Steven Slupsky <sslupsky@gmail.com>

sslupsky · 2020-04-09T16:20:18Z

@carlescufi Sent email to users@ and devel@

pabigot · 2020-04-10T17:33:38Z

To complete this task:

esp32 name is fixed and byte order updated in drivers: hwinfo: fix ESP32 implementation #24268
imxrt is updated in drivers: hwinfo: update NXP platforms for byte order consistency #24271
litex appears to be correct as it copies out the low byte of the 32-bit registers
mcux_sim is updated in drivers: hwinfo: update NXP platforms for byte order consistency #24271
nrf was fixed in drivers: hwinfo: fix endianness for sam0 and nrf #24203
sam is arguably correct as the serial number is text, though a case could be made that the leading NUL should be removed.
sam0 was fixed in drivers: hwinfo: fix endianness for sam0 and nrf #24203
stm32 is updated in drivers: hwinfo: update stm32 implementation byte order #24272

This covers all currently implemented targets.

alexanderwachter · 2020-04-11T07:22:26Z

Thanks for updating all drivers @pabigot

sslupsky requested a review from alexanderwachter as a code owner April 8, 2020 19:04

sslupsky mentioned this pull request Apr 8, 2020

drivers: hwinfo: shell command "hwinfo devid" output ignores endianness #23444

Closed

galak added platform: nRF Nordic nRFx platform: Microchip SAM Microchip SAM Platform (formerly Atmel SAM) labels Apr 9, 2020

pabigot self-requested a review April 9, 2020 01:02

pabigot previously approved these changes Apr 9, 2020

View reviewed changes

eobalski self-requested a review April 9, 2020 13:19

eobalski approved these changes Apr 9, 2020

View reviewed changes

eobalski added the area: USB Universal Serial Bus label Apr 9, 2020

alexanderwachter added the area: API Changes to public APIs label Apr 9, 2020

carlescufi approved these changes Apr 9, 2020

View reviewed changes

carlescufi added the DNM This PR should not be merged (Do Not Merge) label Apr 9, 2020

pabigot self-requested a review April 9, 2020 15:03

pabigot requested changes Apr 9, 2020

View reviewed changes

stephanosio added the Release Notes To be mentioned in the release notes label Apr 9, 2020

sslupsky force-pushed the hwinfo-fix-endianness branch from 73c87b5 to e54ba1c Compare April 9, 2020 15:27

sslupsky requested a review from nashif as a code owner April 9, 2020 15:27

alexanderwachter approved these changes Apr 9, 2020

View reviewed changes

zephyrbot added the area: Documentation label Apr 9, 2020

carlescufi reviewed Apr 9, 2020

View reviewed changes

pabigot approved these changes Apr 9, 2020

View reviewed changes

sslupsky force-pushed the hwinfo-fix-endianness branch from e54ba1c to 19cd3ef Compare April 9, 2020 15:56

carlescufi approved these changes Apr 9, 2020

View reviewed changes

carlescufi merged commit c54f7ec into zephyrproject-rtos:master Apr 9, 2020

carlescufi removed the DNM This PR should not be merged (Do Not Merge) label Apr 9, 2020

nandojve mentioned this pull request Apr 18, 2020

Convert SAM SoC drivers to DT_INST #23107

Closed

61 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

drivers: hwinfo: fix endianness for sam0 and nrf #24203

drivers: hwinfo: fix endianness for sam0 and nrf #24203

sslupsky commented Apr 8, 2020 •

edited

pabigot left a comment

alexanderwachter commented Apr 9, 2020

carlescufi commented Apr 9, 2020

carlescufi commented Apr 9, 2020

sslupsky commented Apr 9, 2020

pabigot left a comment

sslupsky commented Apr 9, 2020

pabigot commented Apr 9, 2020

sslupsky commented Apr 9, 2020

sslupsky commented Apr 9, 2020

alexanderwachter commented Apr 9, 2020

carlescufi commented Apr 9, 2020

carlescufi commented Apr 9, 2020

sslupsky commented Apr 9, 2020

carlescufi Apr 9, 2020

sslupsky Apr 9, 2020

pabigot left a comment

sslupsky commented Apr 9, 2020

carlescufi commented Apr 9, 2020

sslupsky commented Apr 9, 2020

pabigot commented Apr 10, 2020

alexanderwachter commented Apr 11, 2020

drivers: hwinfo: fix endianness for sam0 and nrf #24203

drivers: hwinfo: fix endianness for sam0 and nrf #24203

Conversation

sslupsky commented Apr 8, 2020 • edited

pabigot left a comment

Choose a reason for hiding this comment

alexanderwachter commented Apr 9, 2020

carlescufi commented Apr 9, 2020

carlescufi commented Apr 9, 2020

sslupsky commented Apr 9, 2020

pabigot left a comment

Choose a reason for hiding this comment

sslupsky commented Apr 9, 2020

pabigot commented Apr 9, 2020

sslupsky commented Apr 9, 2020

sslupsky commented Apr 9, 2020

alexanderwachter commented Apr 9, 2020

carlescufi commented Apr 9, 2020

carlescufi commented Apr 9, 2020

sslupsky commented Apr 9, 2020

carlescufi Apr 9, 2020

Choose a reason for hiding this comment

sslupsky Apr 9, 2020

Choose a reason for hiding this comment

pabigot left a comment

Choose a reason for hiding this comment

sslupsky commented Apr 9, 2020

carlescufi commented Apr 9, 2020

sslupsky commented Apr 9, 2020

pabigot commented Apr 10, 2020

alexanderwachter commented Apr 11, 2020

sslupsky commented Apr 8, 2020 •

edited