net: l2: ieee802154: improved/fixed endianness and short address handling #50400
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ghost
requested review from
jciupis,
pfalcon,
rlubos and
tbursztyka
as code owners
ghost
marked this pull request as draft
rlubos
reviewed
Sep 21, 2022
Contributor
rlubos
left a comment
rlubos
left a comment
There was a problem hiding this comment.
Had a brief look, not much to complain, just a few nits. Looks pretty good!
ghost
mentioned this pull request
ghost
marked this pull request as ready for review
Author
|
@rlubos : Thanks for the review! I also fixed the remaining bugs today, that made the tests fail and I added extra test coverage for short address mode. So should be ready for final review. |
jciupis
previously approved these changes
Sep 28, 2022
tbursztyka
requested changes
Sep 28, 2022
Contributor
tbursztyka
left a comment
tbursztyka
left a comment
There was a problem hiding this comment.
Overall a good PR, but some commits are too big and include too many changes at once: please split them. Side note: try to keep consistent with the code style.
Author
Thank you @tbursztyka for this thorough review. Will look into it this weekend to address your concerns. |
added 7 commits
October 1, 2022 10:12
This changes just fixes an erroneous indentation from a previous patch. Signed-off-by: Florian Grandel <[email protected]>
The IEEE 802.15.4 radio driver encodes attributes in: * little endian for everything that is close to the protocol as IEEE 802.15.4 frames are little endian encoded. * mixed big and little endian in its configuration where extended addresses are being represented. These inconsistencies are unfortunate but cannot be easily fixed in a backwards compatible way so will be left untouched in this change. Endianness was almost nowhere documented which explains these inconsistencies and led to several bugs where assignments of different byte order are not converted (or sometimes converted, sometimes not). This change documents intended endianness within the realm of the IEEE 802.15.4 radio driver code. Conversion bugs are fixed in a separate commit. Signed-off-by: Florian Grandel <[email protected]>
To maintain POSIX compatibility, link-layer addresses are encoded in big endian in the IP stack and socket API. The intended endianness was however not documented everywhere which led to bugs and inconsistencies. The IEEE 802.15.4 L2 stack, for example, sometimes stores addresses in little endian in structures that intend them to be stored in POSIX-compliant big endian byte order. This change documents intended endianness within the realm of the IP and sockets stack. Conversion bugs are fixed in a separate commit. Signed-off-by: Florian Grandel <[email protected]>
The IEEE 802.15.4 L2 code stores representation of attributes like PAN id, short address and extended address in different encodings: * big endian for extended address and CPU byte order for everything else whenever such attributes enter user space (except for IP/socket link layer addresses which are always big endian - even in case of short addresses - to maintain POSIX compatibility). * little endian for everything that is close to the radio driver as IEEE 802.15.4 frames are little endian encoded. Endianness was almost nowhere documented which led to several bugs and inconsistencies where assignments of different byte order were not converted (or sometimes converted, sometimes not). This change documents endianness wherever possible within the realm of the IEEE 802.15.4 L2 code. Conversion bugs and inconsistencies that were revealed by the improved documentation will be fixed in a separate commit. Signed-off-by: Florian Grandel <[email protected]>
The WPAN-USB sample did not document endianness of some user space variables. As the IEEE 802.15.4 stack uses attributes in several different encodings, the endianness should be documented. Signed-off-by: Florian Grandel <[email protected]>
This is a preparatory change that fixes one aspect of short address handling before fixing endianness so that the endianness fix can be applied consistently in this method. Signed-off-by: Florian Grandel <[email protected]>
This changes fixes several bugs and inconsistencies in the IEEE 802.15.4 L2 implementation. These bugs were revealed while documenting intended endianness of driver, IP, socket and L2 attributes (see previous changes). Signed-off-by: Florian Grandel <[email protected]>
Author
|
@tbursztyka I addressed all your concerns except for creating a separate PR for the IP subsystem - see my comments. It would be great if you could answer my questions in the unresolved comments and/or approve the PR. |
added 5 commits
October 1, 2022 18:32
This change introduces an additional function into the API that allows to test and clear a net_if flag atomically. A similar function already exists to test and set a flag. So this change actually improves symmetry of the API. The change is required in later commits of this PR. Signed-off-by: Florian Grandel <[email protected]>
IEEE 802.15.4 short address support is incomplete in several places. This change improves short address support without claiming to fix it everywhere. Future iterations will have to continue where this change leaves off. The purpose of this change was to: * use the short address returned by association responses, * automatically bind IEEE 802.15.4 datagram sockets to the short address if available, * use the short address in outgoing packages where applicable, * improve validation of association/disassociation frames, * model association more closely to the spec by tying it to the existence of a short address in the MAC PIB thereby removing redundancy in the PIB (which makes race conditions less probable), * keep both, the short and extended addresses, of the coordinator. Signed-off-by: Florian Grandel <[email protected]>
Several attributes in the ieee802154_context struct may potentially be accessed from different threads and/or ISR context. Only some of these attributes were properly guarded against race conditions. This may not have been to problematic in the past but as other changes in this PR introduce additional attributes and mutate several attributes in a single atomic transaction, leaving such changes unprotected seems dangerous. This change therefore introduces systematic locking of the ieee802154_context structure. Signed-off-by: Florian Grandel <[email protected]>
This change re-orders attributes in struct ieee802154_context to completely optimize struct padding away. This is done in a way that does not impact readability. Signed-off-by: Florian Grandel <[email protected]>
The LLDN frame has been obsoleted in IEEE 802.15.4-2015f. This change removes it from the code, introduces frame types from current spec levels and updates the frame validation rules in accordance with the spec. Signed-off-by: Florian Grandel <[email protected]>
tbursztyka
approved these changes
Oct 7, 2022
Author
|
@tbursztyka Thanks for your valuable feedback and approval. @rlubos @jukkar Another approval seems to be required to unblock this PR. You already posted LGTM, seems like its just a missing button press? |
rlubos
approved these changes
Oct 12, 2022
jukkar
approved these changes
Oct 12, 2022
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR implements part one of the change announced in #50336 (comment):
It turned out that a neighbor table was not (yet) required for this preparatory change. The PR also fixes endianness in the IEEE 802.15.4 L2 stack as the work on this change revealed several inconsistencies and even bugs in byte order conversion.