cpu/stm32/periph_eth: fix error handling in send()#15783
Merged
benpicco merged 2 commits intoRIOT-OS:masterfrom Jan 22, 2021
Merged
cpu/stm32/periph_eth: fix error handling in send()#15783benpicco merged 2 commits intoRIOT-OS:masterfrom
benpicco merged 2 commits intoRIOT-OS:masterfrom
Conversation
maribu
added
Type: bug
Member
Author
|
I added the minor label as I don't expect that the error handling code will be much executed, if ever. |
maribu
force-pushed
the
stm32_eth_fix_error_handling
branch
from
ff77602 to
1ddacfe
Compare
An earlier version of periph_eth used to always pack the first chunk of the outgoing frame to the first DMA descriptor by telling the DMA to jump back to the first descriptor within the last descriptor. This worked fine unless the frame was send in one chunk (as e.g. lwip does), which resulted due to a hardware bug in a frame being send out twice. For that reason, the behavior was changed to cycle throw the linked DMA descriptor list in round-robin fashion. However, the error checking was not updated accordingly. Hence, the error check might run over (parts of) unrelated frames and fail to detect errors correctly. This commit fixes the issue and also provides proper return codes for errors. Additionally, an DMA reset is performed on detected errors during RX/TX. I'm not sure if/when this is needed, as error conditions are neigh impossible to produce. But better be safe than sorry.
Add ENABLE_DEBUG_VERBOSE flag, so that the noise during debugging can be reduced. This is super helpful when testing under load, as otherwise there is just too much noise in the output.
maribu
force-pushed
the
stm32_eth_fix_error_handling
branch
from
1ddacfe to
21264b8
Compare
1 task
benpicco
added
the
CI: ready for build
benpicco
approved these changes
Jan 22, 2021
Contributor
|
Looks good to me! |
Member
Author
|
Thanks :-) |
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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 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.
Contribution description
An earlier version of periph_eth used to always pack the first chunk of the
outgoing frame to the first DMA descriptor by telling the DMA to jump back
to the first descriptor within the last descriptor. This worked fine unless
the frame was send in one chunk (as e.g. lwip does), which resulted due to a
hardware bug in a frame being send out twice. For that reason, the behavior was
changed to cycle throw the linked DMA descriptor list in round-robin fashion.
However, the error checking was not updated accordingly. Hence, the error
check might run over (parts of) unrelated frames and fail to detect errors
correctly.
This commit fixes the issue and also provides proper return codes for errors.
Additionally, an DMA reset is performed on detected errors during RX/TX. I'm
not sure if/when this is needed, as error conditions are neigh impossible to
produce. But better be safe than sorry.
Testing procedure
ENABLE_DEBUG(but notENABLE_DEBUG_VERBOSE!) to1incpu/stm32/periph/eth.cto see if errors actually occur.USEMODULE=stm32_eth_auto make flash term -C examples/gnrc_networking.stm32_eth_autoenables auto-negotiation. (We might want to make this the default, btw.)sudo ethtool -s eth0 advertise 0x001. This result in the Linux only advertising 10 Mbps half duplex.ping -I eth0 -s 1024 -i 0.001 <RIOT_IPV6_ADDR>. You might need to run this withsudoto be able to send at 1 kHzping -c 100 -i 1 -s 1024 <LINUX_IPV6_ADDR>.You should see reasonable error messages, some packet loss via ping, but connectivity should remain unaffected.
Issues/PRs references
None