at86rf2xx: do not try to send when device is busy by miri64 · Pull Request #11264 · RIOT-OS/RIOT

miri64 · 2019-03-25T16:02:23Z

Contribution description

Otherwise, a race condition might be triggered where data that was just
received (which is the reason the device might be busy) is overridden
by the sent data (see #11256).

Testing procedure

Run the tests in #11256 ~~together with #11263~~ [the test doesn't include gnrc_netif so merging #11263 would be pointless] (I did not do this myself yet, this is why I marked it as WIP).

Issues/PRs references

This prevents the race condition show-cased in #11256.
Together with #11263 the sent packets are tried to be sent later instead of just being dropped.

Depends on #11263

Basis for #11068

miri64 · 2019-07-12T12:27:07Z

Rebased and added #11263 as a dependency when gnrc_netif is also included.

miri64 · 2019-08-30T13:04:45Z

Rebased to current master and dependencies.

miri64 · 2019-09-02T07:50:58Z

Maybe this will be fixed less hacky with #12069. Until this is confirmed, I keep this open.

miri64 · 2019-09-25T09:50:04Z

Rebased to current #11263

Otherwise, a race condition might be triggered where data that was just received (which is the reason the device might be busy) is overridden by the sent data. (cherry picked from commit a7d26f1, see RIOT-OS#11264)

Otherwise, there can happen a state transition between the busy check and the setting do TX_ARET_ON. (cherry picked from commit 41ee023, see RIOT-OS#11264)

(cherry picked from commit c8c2df4, see RIOT-OS#11264)

miri64 · 2019-11-11T07:57:22Z

Rebased to current master and dependencies

miri64 · 2020-01-21T09:58:18Z

Rebased to current master and dependencies to resolve #11263's conflict with #13065.

benpicco · 2020-02-17T16:32:34Z

Can this be closed now that #12728 is merged?

miri64 · 2020-02-19T13:17:44Z

If #11068 is able to perform without this PR, yes. Let me check this in the coming days.

fjmolinas · 2020-04-14T07:07:34Z

If #11068 is able to perform without this PR, yes. Let me check this in the coming days.

ping :)

miri64 · 2020-04-14T07:37:57Z

Sorry, still did not find the time to test this :(. Will see if I manage today.

miri64 · 2020-04-14T11:14:42Z

I tested with #11068 rebased to master and without gnrc_netif_pktq and also tested as it is now. I used ping6 -s 120 -i 750 -c 4800 beef::1 over static routes with a single hop.

Rebased to master:

2020-04-14 12:02:40,266 # --- beef::1 PING statistics ---
2020-04-14 12:02:40,272 # 4800 packets transmitted, 423 packets received, 91% packet loss
2020-04-14 12:02:40,276 # round-trip min/avg/max = 84.702/111.848/142.180 ms

In the current version:

2020-04-14 13:10:52,562 # --- beef::1 PING statistics ---
2020-04-14 13:10:52,567 # 4800 packets transmitted, 1265 packets received, 73% packet loss
2020-04-14 13:10:52,572 # round-trip min/avg/max = 75.163/111.254/153.087 ms

So yes, in some form or another, this PR is still needed. However, as I said multiple times before: this should be made configurable and based on a MAC protocol on top. As it is now it basically just requires a minimal MAC queue on top, which might not work for all devices.

Otherwise, a race condition might be triggered where data that was just received (which is the reason the device might be busy) is overridden by the sent data.

Otherwise, there can happen a state transition between the busy check and the setting do TX_ARET_ON.

miri64 · 2020-09-23T10:31:03Z

Rebased to current master, mainly to reflect the current state. We should discuss in #14954 how to proceed with this in netdev. I will adapt this PR then accordingly.

jia200x · 2020-09-23T11:05:57Z

I think there's still a case of race condition: If the user calls the send function while receiving and at the same moment RX_BUSY goes to RX, the send function will pass through and we will see the same issue :/

I think the proper way to do it would be to either save the IRQ request somewhere or use features like the Frame Buffer Empty Indicator (section 11.7 of datasheet) or the SPI_CMD_MODE to read the IRQ_STATUS register (section 6.3.1)

miri64 · 2020-10-28T14:50:43Z

IMHO with the radio HAL now merged, it doesn't make much sense to keep this PR. Shout if you disagree.

miri64 added Type: bug The issue reports a bug / The PR fixes a bug (including spelling errors) Area: network Area: Networking State: WIP State: The PR is still work-in-progress and its code is not in its final presentable form yet Area: drivers Area: Device drivers labels Mar 25, 2019

miri64 requested review from PeterKietzmann and kaspar030 March 25, 2019 16:02

This was referenced Mar 25, 2019

gnrc_netif: add packet to queue when device is busy #11263

Merged

gnrc_netif: fix DEBUG output on error #11285

Merged

gnrc_sixlowpan_frag: initial import of minimal forwarding #11068

Merged

miri64 force-pushed the at86rf2xx/enh/busy-ret branch from 5bf30ed to 63f9573 Compare July 12, 2019 12:26

jia200x mentioned this pull request Aug 23, 2019

at86rf2xx: fix race conditions and PHY states representation #12069

Closed

miri64 force-pushed the at86rf2xx/enh/busy-ret branch from 63f9573 to 25fee2d Compare August 30, 2019 13:04

miri64 force-pushed the at86rf2xx/enh/busy-ret branch from 25fee2d to c8c2df4 Compare September 25, 2019 09:49

miri64 added a commit to 5G-I3/RIOT-public that referenced this pull request Sep 25, 2019

at86rf2xx: make gnrc_netif_pktq a dependency with GNRC

870656f

(cherry picked from commit c8c2df4, see RIOT-OS#11264)

miri64 mentioned this pull request Oct 22, 2019

cdcecm: early exit on inactive usb interface #12534

Merged

miri64 force-pushed the at86rf2xx/enh/busy-ret branch from c8c2df4 to dead52c Compare November 11, 2019 07:56

miri64 mentioned this pull request Dec 6, 2019

tests: add tests for netdev flooding race-condition #11256

Open

miri64 force-pushed the at86rf2xx/enh/busy-ret branch from dead52c to 0656a72 Compare January 21, 2020 09:57

miri64 and others added 3 commits September 23, 2020 11:51

at86rf2xx: do not try to send when device is busy

9d1731f

Otherwise, a race condition might be triggered where data that was just received (which is the reason the device might be busy) is overridden by the sent data.

at86rf2xx: unroll tx_prepare in netdev->driver->send

0ae2296

Otherwise, there can happen a state transition between the busy check and the setting do TX_ARET_ON.

at86rf2xx: make gnrc_netif_pktq a dependency with GNRC

d91e63c

miri64 force-pushed the at86rf2xx/enh/busy-ret branch from 0656a72 to d91e63c Compare September 23, 2020 10:30

miri64 requested review from daniel-k, jia200x and smlng as code owners September 23, 2020 10:30

miri64 closed this Oct 28, 2020

miri64 deleted the at86rf2xx/enh/busy-ret branch October 28, 2020 14:50

Conversation

miri64 commented Mar 25, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Contribution description

Testing procedure

Issues/PRs references

Uh oh!

miri64 commented Jul 12, 2019

Uh oh!

miri64 commented Aug 30, 2019

Uh oh!

miri64 commented Sep 2, 2019

Uh oh!

miri64 commented Sep 25, 2019

Uh oh!

miri64 commented Nov 11, 2019

Uh oh!

miri64 commented Jan 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

benpicco commented Feb 17, 2020

Uh oh!

miri64 commented Feb 19, 2020

Uh oh!

fjmolinas commented Apr 14, 2020

Uh oh!

miri64 commented Apr 14, 2020

Uh oh!

miri64 commented Apr 14, 2020

Uh oh!

miri64 commented Sep 23, 2020

Uh oh!

jia200x commented Sep 23, 2020

Uh oh!

miri64 commented Oct 28, 2020

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

miri64 commented Mar 25, 2019 •

edited

Loading

miri64 commented Jan 21, 2020 •

edited

Loading