cpu/stm32/periph_eth: fix typo in initialization code#18416
cpu/stm32/periph_eth: fix typo in initialization code#18416benpicco merged 1 commit intoRIOT-OS:masterfrom
Conversation
maribu
added
Type: bug
github-actions
bot
added
Area: cpu
benpicco
added
the
CI: ready for build
took many hours of debugging over the course of a couple of days 😢 @chrysn: Since I need to ping you anyway due to this being backport material: Would this be a memory corruption that rust could catch? The corruption takes place in the DMA, not in the CPU, so this may be tricky to check. |
|
The PR and commit description makes zero sense. |
maribu
force-pushed
the
cpu/stm32/periph/stm32_eth/bugfix
branch
from
ddffbc8 to
0086283
Compare
maribu
changed the title
No, it doesn't. The typo is in the index, not in the variable name. I was jumping to incorrect conclusions here. @fabian18 gnrc_ndp_opt_sl2a_build adds new |
benpicco
changed the title
A single character type resulted in way fewer TX descriptors being
available than allocated. Not only resulted this in wasting memory,
but also when more iolist chunks than descriptors are send, the
```C
assert(iolist_count(iolist) <= ETH_TX_DESCRIPTOR_COUNT);
```
does not trigger. As a result, old TX descriptors are being overwritten
in this case.
maribu
force-pushed
the
cpu/stm32/periph/stm32_eth/bugfix
branch
from
0086283 to
82fbe08
Compare
|
On the Rust question: Depends on how it'd be implemented. Bounds checks are generally done in Rust (with provisions to optimize them out often), so if it were to write out of bounds, that would have resulted in a runtime error, or in this case likely even a build error. In the "wasting memory" case, if I understand it correctly, it wouldn't have caught anything, but there's also nothing worse happening than not using some memory. Took me a while to see what's happening, but yes I agree this is backport material. As we need to have an RC3 anyway due to #18413, this is not creating much overhead. I'm taking the liberty to prioritize this in the build queue to enable a speedy backport. (edit: only moved #18403 as the others should be quick enough; #18396 turned out to be 200 and not 20 tests but was already some way in when I saw that) |
Oh, I missed that part. If the error were in the name ... well, I think it's hard to compare, for in Rust that list might be initialized statically, so there'd be no danger of confusion. Such linked lists are generally not very idiomatic Rust, though, and if DMA requires them, it might be that one'd use and initialize them either through some local unsafe code (in which things like that could easily happen) or an abstraction for this kind of DMA lists. |
|
Hurray, thank you for spotting and fixing this! |
fabian18
added
CI: ready for build
|
restarted Murdock, due to binary hashes mismatching with and without Kconfig |
|
Thx :) |
|
Backport provided in #18420 |
4 participants
Contribution description
A single character type resulted in way fewer TX descriptors being available than allocated. Not only resulted this in wasting memory, but also when more iolist chunks than descriptors are send, the
does not trigger. As a result, old TX descriptors are being overwritten in this case.
Testing procedure
@fabian18 has a test that reliably triggers the bug. He should be able to confirm it is gone now. But actually, it should be pretty obvious from the code that this fixes a typo.
Issues/PRs references
None