Skip to content

net/ieee802154_security: remove radio hal dependency and cleanup#15909

Merged
miri64 merged 2 commits intoRIOT-OS:masterfrom
leandrolanzieri:pr/ieee802154_security_radio_hal
Feb 5, 2021
Merged

net/ieee802154_security: remove radio hal dependency and cleanup#15909
miri64 merged 2 commits intoRIOT-OS:masterfrom
leandrolanzieri:pr/ieee802154_security_radio_hal

Conversation

@leandrolanzieri
Copy link
Copy Markdown
Contributor

Contribution description

This PR does two things:

  1. Removes duplicated definitions that were added to the radio HAL, which are already defined in ieee802154_security.h
    The shortcut functions are removed from the radio HAL. After a discussion with @jia200x, a ieee802154_sec_context_t will likely find a place on the newly introduced submac later on. For now, as radios that use the radio HAL are still accessed via the netdev_ieee802154_submac, the current status will still work. This allows to remove an avoidable dependency and reduce code duplication.

  2. Makes the default implementations of the cipher operations of ieee802154_security private
    This makes the API of the module simpler, as the fallback implementations are kept private. Now the default cipher operations driver holds NULL pointers, which indicates to use the fallback implementation. Documentation on this has been added.
    ROM usage seems not to be negatively impacted by the change. In fact, while compiling examples/gnrc_networking using ieee802154_security for samr21-xpro, the text section is reduced by 4 bytes.

Testing procedure

Same procedure as for #15150:

You can test the implementation with examples/gnrc_networking, if you add USEMODULE += ieee802154_security and try ping6. It is supposed to work with any 802.15.4 radio. If you have an at86rf2xx with SPI, you can additionally add USEMODULE += at86rf2xx_aes_spi, if you want to utilize the transceiver´s hardware crypto module.
I tested the implementation on two nucleo-f767zi with an at86rf233.

Issues/PRs references

#15150

@leandrolanzieri leandrolanzieri added Area: network Area: Networking Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation Area: security Area: Security-related libraries and subsystems labels Feb 2, 2021
@maribu
Copy link
Copy Markdown
Member

maribu commented Feb 2, 2021

lgtm. Maybe @fabian18 could also take a look, since he authored the code.

@fabian18
Copy link
Copy Markdown
Contributor

fabian18 commented Feb 2, 2021

First of all it sill works with and without hardware support. 👍
Setting the callbacks to NULL instead to default values was also taken into consideration in the original PR. #15150 (comment)
I´d prefer that the callback is not NULL but set to the default callback because it avoids an if() {} else {}.
But the if() {} else {} can elide a virtual function call.
I am fine with both.

@jia200x
Copy link
Copy Markdown
Member

jia200x commented Feb 2, 2021

I´d prefer that the callback is not NULL but set to the default callback because it avoids an if() {} else {}.
But the if() {} else {} can elide a virtual function call.

I tend to agree that avoiding an if-else could look cleaner, but I see some cons:

  • If the compiler is not able to optimize the virtual function calls, the codesize is bigger. Also, if-else can be optimized out explicitly with function calls.
  • It forces to write a north-bound API where only a south-bound API is required (e.g in the future we can write the ieee802154_sec_dev_t as a generic hwcrypto object)

@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

So, how should we move forward? Any strong opinion against keeping the conditional?

@fabian18
Copy link
Copy Markdown
Contributor

fabian18 commented Feb 4, 2021

I have no objections, going with the if() {} else {}.

master:

no hardware support:

ping6 fe80::a421:98bf:f2ef:37f8 -s 64 -c 100
2021-02-04 20:34:19,277 # --- fe80::a421:98bf:f2ef:37f8 PING statistics ---
2021-02-04 20:34:19,283 # 100 packets transmitted, 100 packets received, 0% packet loss
2021-02-04 20:34:19,288 # round-trip min/avg/max = 12.885/16.195/55.831 ms

with hardware support:

ping6 fe80::a421:98bf:f2ef:37f8 -s 64 -c 100
2021-02-04 20:37:51,593 # --- fe80::a421:98bf:f2ef:37f8 PING statistics ---
2021-02-04 20:37:51,597 # 100 packets transmitted, 100 packets received, 0% packet loss
2021-02-04 20:37:51,601 # round-trip min/avg/max = 21.058/23.587/31.023 ms

this branch:

no hardware support:

ping6 fe80::a421:98bf:f2ef:37f8 -s 64 -c 100
2021-02-04 20:45:32,718 # --- fe80::a421:98bf:f2ef:37f8 PING statistics ---
2021-02-04 20:45:32,723 # 100 packets transmitted, 100 packets received, 0% packet loss
2021-02-04 20:45:32,727 # round-trip min/avg/max = 12.831/16.074/45.000 ms

with hardware support:

ping6 fe80::a421:98bf:f2ef:37f8 -s 64 -c 100
2021-02-04 20:42:10,764 # --- fe80::a421:98bf:f2ef:37f8 PING statistics ---
2021-02-04 20:42:10,769 # 100 packets transmitted, 100 packets received, 0% packet loss
2021-02-04 20:42:10,774 # round-trip min/avg/max = 21.195/24.019/31.200 ms

@benpicco benpicco added the CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR label Feb 5, 2021
@miri64 miri64 merged commit 492000f into RIOT-OS:master Feb 5, 2021
@leandrolanzieri leandrolanzieri deleted the pr/ieee802154_security_radio_hal branch February 5, 2021 16:20
@leandrolanzieri
Copy link
Copy Markdown
Contributor Author

Thanks for the review!

@kaspar030 kaspar030 added this to the Release 2021.04 milestone Apr 23, 2021
@leandrolanzieri leandrolanzieri added the Release notes: ignored Set on PRs that have been considered for inclusion in the current release's notes but were minor. label Apr 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Area: network Area: Networking Area: security Area: Security-related libraries and subsystems CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Release notes: ignored Set on PRs that have been considered for inclusion in the current release's notes but were minor. Type: cleanup The issue proposes a clean-up / The PR cleans-up parts of the codebase / documentation

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants