Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ieee802154/submac: avoid race condition between RX_DONE and ACK_TIMEOUT #16964

Merged
merged 3 commits into from
Oct 11, 2021

Conversation

jia200x
Copy link
Member

@jia200x jia200x commented Oct 7, 2021

Contribution description

This PR makes adapts the documentation of ieee802154_submac_ack_timer_fired to prevent execution when ieee802154_submac_ack_timer_cancel was called. Since the SubMAC does not impose any rules on how it should be processed, this leaves the responsibility to the upper layer.

This also adapts netdev_submac_ieee802154 and tests/ieee802154_submac accordingly.

Testing procedure

It's very hard to trigger, but the consequences of this bug are more visible when using socket_zep (see #16932 (comment))

Anyway, here's the test output for tests/ieee802154_submac and examples/gnrc_lorawan.

> 2021-10-07 14:47:50,777 # Tx complete
2021-10-07 14:47:50,907 # txtsnd 00:04:25:19:18:01:BD:DB 100
> 2021-10-07 14:47:50,912 # Tx complete
txtsnd 00:04:25:19:18:01:BD:DA 100
2021-10-07 14:47:58,642 # txtsnd 00:04:25:19:18:01:BD:DA 100
> 2021-10-07 14:47:58,676 # No ACK
txtsnd 00:04:25:19:18:01:BD:DB 100
2021-10-07 14:48:00,072 # txtsnd 00:04:25:19:18:01:BD:DB 100
> 2021-10-07 14:48:00,078 # Tx complete
2021-10-07 14:59:45,788 # 1032 bytes from fe80::ac8d:fee1:20bc:404d%6: icmp_seq=999 ttl=64 rssi=-71 dBm time=140.303 ms
2021-10-07 14:59:46,645 # 
2021-10-07 14:59:46,649 # --- fe80::ac8d:fee1:20bc:404d PING statistics ---
2021-10-07 14:59:46,653 # 1000 packets transmitted, 980 packets received, 2% packet loss
2021-10-07 14:59:46,658 # round-trip min/avg/max = 132.505/147.343/169.626 ms
> ifconfig
2021-10-07 15:19:26,346 # ifconfig
2021-10-07 15:19:26,353 # Iface  6  HWaddr: 3D:DB  Channel: 26  Page: 0  NID: 0x23  PHY: O-QPSK 
2021-10-07 15:19:26,354 #           
2021-10-07 15:19:26,358 #           Long HWaddr: 00:04:25:19:18:01:BD:DB 
2021-10-07 15:19:26,365 #            TX-Power: 0dBm  State: IDLE  max. Retrans.: 3  CSMA Retries: 4 

Issues/PRs references

Discovered by @benpicco in #16932 (comment)

@jia200x jia200x requested review from miri64 and benpicco October 7, 2021 13:20
@jia200x
Copy link
Member Author

jia200x commented Oct 7, 2021

I guess this should be backported

@github-actions github-actions bot added Area: drivers Area: Device drivers Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework labels Oct 7, 2021
@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 Oct 7, 2021
Copy link
Contributor

@benpicco benpicco left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fixes the problem I see with socket_zep and the code makes sense.
When the ACK timeout is canceled, we should clear the flag in case it already happened.

@benpicco benpicco added CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch and removed CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR labels Oct 11, 2021
@benpicco benpicco added this to the Release 2021.10 milestone Oct 11, 2021
@benpicco benpicco merged commit 57f0dc9 into RIOT-OS:master Oct 11, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Area: drivers Area: Device drivers Area: network Area: Networking Area: sys Area: System Area: tests Area: tests and testing framework CI: ready for build If set, CI server will compile all applications for all available boards for the labeled PR Process: needs backport Integration Process: The PR is required to be backported to a release or feature branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants